New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve function name inference #1971

Merged
merged 3 commits into from Nov 18, 2014

Conversation

Projects
None yet
4 participants
@jugglinmike
Copy link
Member

jugglinmike commented Nov 10, 2014

When used programmatically from Node.js, JSHint exposes a data function that returns an object with a functions key. This data describes the functions defined in the linted code. Currently, JSHint uses a simple heuristic to infer the name of functions, and this is often incorrect.

In gh-1893, I proposed removing JSHint's function name inference because I believed the name property of function to be well-defined by the ECMAScript 5 spec. I was wrong about that, but @caitp showed me how the ES6 spec strictly defines what it means for a function to have a name. This patch mimics much of the behavior defined by the specification.

A notable omission is names that are computed from expressions. For instance, the function in the following example should receive the name "Gozer the Traveler":

var name = "Gozer the Traveler";
var obj = {};
obj[name] = function() {};

...but this behavior cannot be replicated statically. Instead, such functions are reported as having the name "(expression)".

@jugglinmike

This comment has been minimized.

Copy link
Member

jugglinmike commented Nov 12, 2014

@caitp Since you encouraged me to try this (and since I refactored some of your recent changes), I'd love to get your input. Mind taking a look?

@caitp

This comment has been minimized.

Copy link
Member

caitp commented Nov 12, 2014

well the PR description sounds promising, it might take some time to go over the patch, do you have any key areas that you're iffy about you'd like feedback on?

@rwaldron

This comment has been minimized.

Copy link
Member

rwaldron commented Nov 12, 2014

@caitp I've also been reviewing this over the last few days, I told @jugglinmike to have you sign off on it :)

So I'm r+

@caitp

This comment has been minimized.

Copy link
Member

caitp commented Nov 12, 2014

I'm a slow reader D: but I'll do it, no worries =)

@rwaldron

This comment has been minimized.

Copy link
Member

rwaldron commented Nov 12, 2014

@caitp take your time, we're in no rush

@jugglinmike

This comment has been minimized.

Copy link
Member

jugglinmike commented Nov 12, 2014

Totally @caitp! Reviewing is tough; I just wanted to make sure this is on your radar. Here's a little more context, in case it helps:

  • The first commit (jugglinmike@bbfa1ec) isn't technically necessary for this feature, but it allowed me to avoid duplication in how we track the name of "getter" and "setter" functions.
  • The implementation itself is fairly concise, and I've just recently pushed a fixup! commit that streamlines things a bit further. If you review the changeset for the last two commits together (jugglinmike/jshint@5cfd252...81d865b), you'll see the the changes to src/jshint.js are not quite as spanning as the individual commits suggest.

@caitp caitp self-assigned this Nov 12, 2014

saveGetter(isStatic ? staticProps : props, name.value, true, isStatic);
saveAccessor(
getset.value, isStatic ? staticProps : props, name.value, name, true, isStatic
);

This comment has been minimized.

@caitp

caitp Nov 13, 2014

Member

style nit: I'd close the parentheses on the same line as the arguments

},
{
"name": "",

This comment has been minimized.

@caitp

caitp Nov 13, 2014

Member

I feel like these should be "(anonymous)" instead of the empty string, but that's just me

@caitp

This comment has been minimized.

Copy link
Member

caitp commented Nov 13, 2014

LGTM modulo comments (that I'd prefer "(anonymous)" over the empty string).

@jugglinmike

This comment has been minimized.

Copy link
Member

jugglinmike commented Nov 13, 2014

@caitp thanks! I went back to the spec, and based on 14.1.22 I think the value should actually be changed to "default". Does that sound right to you?

@caitp

This comment has been minimized.

Copy link
Member

caitp commented Nov 13, 2014

I'm not sure the "default" necessarily means the string "default", but I would check with people who know more about that. I think "(anonymous)" makes the actual meaning a bit clearer, though. (looks like there's a little editorial bug in that text, someone should file that)

@rwaldron

This comment has been minimized.

Copy link
Member

rwaldron commented Nov 13, 2014

https://bugs.ecmascript.org/show_bug.cgi?id=3384

Hold off on this until we hear back

@jugglinmike

This comment has been minimized.

Copy link
Member

jugglinmike commented Nov 13, 2014

@rwaldron you got it. In the mean time, I've made the code style alteration that @caitp requested. I'll fixup the fixup commits once we hear back on that bug report.

@caitp

This comment has been minimized.

Copy link
Member

caitp commented Nov 13, 2014

TBH it looks like a pretty minor editorial issue, I don't expect there's much we'll learn from the resolution of that bug that's "new" --- it's just a typo and an incorrect grammar production, afaik.

So all I really want to do here is make it clear that anonymous functions are anonymous (whether that is done by saying (anonymous) / (empty) or by yelling at the developer is... well, you can take creative liberties with that decision if you like!) --- I just think text will be better than no text.

@anba

This comment has been minimized.

Copy link

anba commented Nov 13, 2014

That part about "default" in 14.1.22 only applies to (default) FunctionDeclarations in ExportDeclarations.

@rwaldron

This comment has been minimized.

Copy link
Member

rwaldron commented Nov 13, 2014

That part about "default" in 14.1.22 only applies to (default) FunctionDeclarations in ExportDeclarations

This makes sense, thanks @anba

So let's just do the dummy string?

@jugglinmike

This comment has been minimized.

Copy link
Member

jugglinmike commented Nov 13, 2014

@rwaldron Sure--I set it to "(empty)". I haven't squashed yet because I updated the implementation to report "default" for anonymous functions that are being exported as per @anba's explanation above. If anyone cares to sign off on that, I'll squash and we can finally put this to bed :)

@caitp

This comment has been minimized.

Copy link
Member

caitp commented Nov 13, 2014

sgtm

@jugglinmike jugglinmike force-pushed the jugglinmike:anon-fn-names-6 branch from db1cb06 to e89e01e Nov 13, 2014

@rwaldron

This comment has been minimized.

Copy link
Member

rwaldron commented Nov 13, 2014

sgtm

+1

@jugglinmike

This comment has been minimized.

Copy link
Member

jugglinmike commented Nov 14, 2014

Okay, squashed and tests are green.

@rwaldron

This comment has been minimized.

Copy link
Member

rwaldron commented Nov 15, 2014

@jugglinmike lgtm, land at will

Implement more complete function name inference
The semantics of function objects' "name" property has been formalized
in the ECMAScript 6 specification. Implement the specified behavior with
two differences:

- Report functions whose name is derived from an expression as having
  the name "(expression)" (in these cases, the correct value cannot be
  determined statically).
- Report functions whose name is unset as having the name "(empty)".
  Although the specification dictates these functions be assigned the
  empty string ("") as a name, a more explicit name will be more
  informative to developers inspecting the generated report.

@jugglinmike jugglinmike force-pushed the jugglinmike:anon-fn-names-6 branch from e89e01e to 163c61c Nov 16, 2014

@jugglinmike jugglinmike merged commit 163c61c into jshint:master Nov 18, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jugglinmike

This comment has been minimized.

Copy link
Member

jugglinmike commented Nov 18, 2014

Thanks for all your help, Rick and Cait!

@jugglinmike jugglinmike referenced this pull request Feb 22, 2015

Merged

Class expr identifier #2185

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment