[CLOSED] Switch js code hinting to be powered by tern.

[core-ai-bot]

Issue by eztierney
Monday Apr 15, 2013 at 15:46 GMT
Originally opened as adobe/brackets#3439

---

This pull request switches the js code hinting to use tern (http://ternjs.net). The hints are now based on tern's type inference library. When a type can be inferred, the hints are based on that type, and when a type can't be inferrred then the hinting falls back to guessing. In addition to type inference, types can also be determined based on jsdoc annotations, and an understanding of what types are exposed from requirejs modules.

This also adds hinting for function types (can display the types of the arguments of the function being called), and support for a jump to definition feature.

For more details, see
https://github.com/adobe-research/brackets/wiki/JavaScript-Code-Intelligence-Investigation

---

eztierney included the following code: https://github.com/adobe/brackets/pull/3439/commits

[core-ai-bot]

Comment by njx
Monday Apr 15, 2013 at 18:50 GMT

---

@eztierney - could you give a quick summary of what kinds of hinting should be working (e.g. if I'm in the Brackets codebase)? For example, is hinting of modules imported via require() supposed to work? Basically, a summary of where we should be getting more accurate type-based hinting and where we'll be getting the older heuristic hinting would be helpful.

[core-ai-bot]

Comment by njx
Monday Apr 15, 2013 at 18:54 GMT

---

Also, it doesn't look like there are any unit tests for any of the new functionality (tern-based hinting or jump to definition), if I'm reading the diffs right--it looks like there are just some updates to the existing unit tests for the heuristic hinting. Is that correct?

[core-ai-bot]

Comment by pfarland
Monday Apr 15, 2013 at 19:18 GMT

---

I believe all hints are now based on information from Tern, it is just how they are filtered and sorted after the fact that restores the legacy behavior for scope-distance based guesses. QE has a number of Tern unit tests but they have not yet ported any of these to Brackets style jasmine tests.

[core-ai-bot]

Comment by eztierney
Monday Apr 15, 2013 at 19:19 GMT

---

Sure, here's a quick summary of what should be working.

Hinting of modules imported via require() should be working. Hinting modules imported through brackets.getModule is not working - it will just fall back on guessing in that case. Currently we search for the modules relative to the current directory, and relative to the project root. If we can't find the module, then we will fall back on guessing.

var CodeHintManager    = require("editor/CodeHintManager");
CodeHintManager.  // should hint the properties exported by the CodeHintManager module   

Hinting based on type inference should be working.

var s = "a string";
s. //hints the string properties

The hinting is based on all the possible types that a property could have, so if a property potentially has multiple types, you will see the hints for all those types.

var s = "a string";
if( something() ) {
  s = 10;
}
s. // hints number and string properties 

Hinting based on jsdoc type annotations should be working.

/**`@`param {string}  a*/
function foo(a) {
  a. //hints the string properties
}

Hinting for function types should be working.

var CodeHintManager    = require("editor/CodeHintManager");
CodeHintManager.handleChange(  // should display the function signature if hinting is requested here   

Go to definition

var CodeHintManager    = require("editor/CodeHintManager");
CodeHintManager.handleChange()  // cmd+j on handleChange should jump to the definition of handleChange

[core-ai-bot]

Comment by eztierney
Monday Apr 15, 2013 at 19:51 GMT

---

All the hints are powered by tern now. The existing unittests were modified, as many of the existing tests could take advantage of the type based code hints. But we do need to add tests for the new features, such as go to definition, and funciton type hinting.

[core-ai-bot]

Comment by dangoor
Wednesday Apr 17, 2013 at 19:48 GMT

---

I'm going to start reviewing this code soon. I've actually started with "scenario testing". Specifically, I'm comparing the Sprint 23 hinting behavior with that of this pull request, which will give me a bit more background into what the code does before I dive into it.

I'm still doing some testing, and so far I like a lot of what I see. There are a couple of cases I've noted that are not as good as the old behavior.

• $("body"). or $("body").e – the old hinter somehow knows about jQuery. jQuery is really, really common.
• in the file StringMatch.js (where I've been doing my testing), typing "st" in the StringMatcher.prototype.match function
  • in sprint 23, yields "str" and "stringMatch"
  • with this branch, it yields "screenTop", "scrollTo", "setTimeout", "str", "String", "stringMatch", "StringMatcher", "switch"
  • the additional matches provided by the new code are nice, but the two matches provided by the sprint 23 code seem more likely than the first 3 listed alphabetically in the new code
• string literal hinting, which we've already noted

Those things noted, I'll also mention the positives I've seen so far:

• Help for function parameters is awesome (though I wonder about the little horizontally scrolling box, which is a question for XD)
• new code: In multiFieldSort, type "fields." and get label and matchGoodness vs. "forEach" and "priority"
• new code: type "fields.label." and get toExponential, toFixed and toString... these are great!
• new code: type Object.d and get things like Object.defineProperties vs. "d", "domains", "document", "data"
• new code: type "foo". and get things like charAt and charCodeAt vs. "StringMatcher" and "multiFieldSort"
• Anywhere in the file, type "com" and the old code produces nothing, the new code provides plenty of possibilities

So, some of these new hints are fantastically superior, some are just different and a couple of cases are not quite as good as what we've got in sprint 23.

Overall, the direction is great and I'm really happy to see this work happening.

[core-ai-bot]

Comment by dangoor
Wednesday Apr 17, 2013 at 19:56 GMT

---

To expand on my previous comment a little:

• for $("body").e
  • sprint 23 produces "each", "extend" on top
  • this branch produces: "E", "editor", "Element", "ellipse"
• for $("body").
  • sprint 23 produces "length", "text", "getJSON", "one", ...
  • this branch produces "_autoReconnect", "_buildNumber", "_cleanup", "_commandCount"

I wanted to give an idea of the jQuery methods that show up.

[core-ai-bot]

Comment by njx
Wednesday Apr 17, 2013 at 20:37 GMT

---

FYI, I'm running into adobe-research/brackets#35 a lot. This is the only major issue I've seen so far, but would want it to be addressed before we ship as it causes a lot of hints to be entered when I don't want them.

[core-ai-bot]

Comment by dloverin
Wednesday Apr 17, 2013 at 21:34 GMT

---

I'll add the JQuery to the hints. This should solve the issue.

[core-ai-bot]

Comment by pfarland
Thursday Apr 18, 2013 at 01:01 GMT

---

Hi Kevin, Darrell recently honored the request from Brackets for us to use the StringMatch utility in proposals. For the behavior you're seeing where typing "st" matches "scrollTop", "scrollTo", "setTimeout" before "str"... I'm guessing this is coincidentally due to the bug we logged for StringMatch.js in issue #3411 ?

I'm still on the fence about using StringMatch because it can conflict with the idea of returning identifiers categorized by increasing scope depth. This concept was in the original extension, so I'm not sure which is more important?

[core-ai-bot]

Comment by dangoor
Thursday Apr 18, 2013 at 01:02 GMT

---

Adding the jQuery API to the hints definitely did the trick there (and now produces better results than the sprint 23 version does).

[core-ai-bot]

Comment by dangoor
Thursday Apr 18, 2013 at 01:21 GMT

---

Hi Pete, I'm okay with stringMatch's scoring in this case, but I think that the hinting just needs a higher priority element to its sorting or a boost applied based on the same scheme you use for coloring the results. An additional (reasonably significant) boost is likely a better way to go.

A boost could either be added in your code before sorting or we can add an optional argument for StringMatcher.match (or stringMatch) to add a boost for a given string.

What do you think?

[core-ai-bot]

Comment by peterflynn
Thursday Apr 18, 2013 at 01:49 GMT

---

@pfarland I assume you're using StringMatch.basicMatchSort() to sort the results currently? If you use the lower-level StringMatch.multiFieldSort() that it's built on you can easily sort first by scope buckets and then by match quality. Or if you want a looser boost rather than strict buckets, you can always roll your own sort by accessing the result items' matchGoodness property directly...

[core-ai-bot]

Comment by dloverin
Thursday Apr 18, 2013 at 02:24 GMT

---

We are using multiFieldSort() with matchGoodness being the highest priority. I would expect a string that matches a prefix in the hints would have a higher matchGoodness score than one that just matches camel case. If that was the case then "str" would be sorted to the top and there wouldn't be an issue.

[core-ai-bot]

Comment by dangoor
Thursday Apr 18, 2013 at 02:34 GMT

---

@dloverin I think I agree with you that a prefix match deserves a boost big enough to put it ahead of a camelCase match. But, that still doesn't completely fix this case. "st" would then match "str", "String", "stringMatch", "StringMatcher", and I would argue that the order should be "str", "stringMatch", "StringMatcher".

Not a blocker, certainly, and I can look into resolving #3411 during this sprint (should not be a lot of work).