Update: no-undefined handles properties/classes/modules (fixes #7964) #7966

platinumazure · 2017-01-23T04:06:07Z

What is the purpose of this pull request? (put an "X" next to item)

[x] Bug fix

What changes did you make? (Give an overview)

Fixed no-undefined to ensure that property keys (non-computed) were not incorrectly flagged. Although property keys are Identifier nodes, non-computed property keys are really strings at the end of the day.

Well, that's where we started, but then we decided to ensure that destructuring and import/export were handled correctly. In order to do that more reliably and (hopefully) ensure the rule keeps up with no syntax, I decided to refactor to use escope variables. This ended up improving performance by about 80% (at least when the rule is run on our codebase), so that's a huge win. (Identifer visitor was hit a lot, so even the performance penalty of iterating over scopes again still made this rule much faster overall.)

Labeling as "Update" in case more warnings are introduced in destructuring or import/export cases.

Is there anything you'd like reviewers to focus on?

Does this need a documentation update?

mention-bot · 2017-01-23T04:06:08Z

@platinumazure, thanks for your PR! By analyzing the history of the files in this pull request, we identified @michaelficarra, @vitorbal and @kaicataldo to be potential reviewers.

eslintbot · 2017-01-23T04:06:09Z

LGTM

not-an-aardvark · 2017-01-23T04:12:36Z

lib/rules/no-undefined.js

+
+                    // Only non-computed keys are valid here
+                    return node === node.parent.key &&
+                        !node.parent.computed;


I think this needs to include a && !node.parent.shorthand check, otherwise the rule will have a false negative with

({ undefined }); // this should get reported, because it's equivalent to ({ undefined: undefined });

not-an-aardvark · 2017-01-23T04:19:00Z

tests/lib/rules/no-undefined.js

+        { code: "undefined = true", errors },
+        { code: "var undefined = true", errors },
+        { code: "({ bar: undefined })", errors },
+        { code: "({ bar: undefined } = foo)", errors }


It might be useful to have tests for

({ undefined }); // invalid var { undefined } = foo; // invalid ({ undefined() {} }); // valid ({ undefined: function undefined() {} }); // invalid (EDITED) var foo = function undefined() {}; // invalid (EDITED) function undefined() {} // invalid, I think? It does create a variable class Foo { undefined() {} } // valid class Foo { [undefined]() {} } // invalid

Agreed. I will see how many of those pass before this PR, as well.

not-an-aardvark · 2017-01-23T04:21:00Z

lib/rules/no-undefined.js

+                    return node === node.parent.key &&
+                        !node.parent.computed;
+
+                default:


There should also be a MethodDefinition case here, to prevent reporting an error for

class Foo { undefined() {} } // valid

ljharb · 2017-01-23T04:11:02Z

tests/lib/rules/no-undefined.js

+        { code: "undefined = true", errors },
+        { code: "var undefined = true", errors },
+        { code: "({ bar: undefined })", errors },
+        { code: "({ bar: undefined } = foo)", errors }


what about both of these examples with var?

Seemed unimportant, but couldn't hurt.

One creates a new variable, the other assigns to an existing one (possibly a global one) - they're different :-)

mysticatea · 2017-01-23T04:46:01Z

Could we simplify this by escope's references?

platinumazure · 2017-01-23T05:01:15Z

I can look into that possibility, sure.

…

On Jan 22, 2017 10:46 PM, "Toru Nagashima" ***@***.***> wrote: Could we simplify this by escope's references? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7966 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWerzVbD7BZM9Gg6LIgx8vKzqu_A5Jks5rVDCKgaJpZM4LqpRl> .

platinumazure · 2017-01-23T22:33:05Z

What I think I'm going to do is: Add a commit with missing test cases and logic; and then try to refactor to use escope references, while making sure the existing and new test cases pass. (Before all that, I'll rebase, of course.) It would be awesome to just handle this via escope references, since presumably the string keys are not references. I just want to make sure all the test cases are covered first.

cc @not-an-aardvark, @mysticatea

eslintbot · 2017-01-28T18:11:11Z

LGTM

eslintbot · 2017-01-28T19:05:25Z

LGTM

platinumazure · 2017-01-28T19:10:01Z

Okay, it's currently handling everything that's been requested, except for when undefined is used as a function identifier while being associated with a variable or object key of a different name.

For some reason, I thought var foo = function bar() {} would also put bar in the same scope as foo, but that doesn't seem to be the case in Chrome. Is bar in the inner function scope (same level as params or locals in bar)? Or is it not put in any scope at all and the name is pointless?

I'm also wondering how we really want the rule to behave as a whole with regard to extra identifiers. We could say that it should forbid all references to undefined that read from or write to the current scope; or we could say that it should forbid all references to undefined except object or method keys, and anything else (even if it doesn't try to read a variable or create a new variable) is forbidden.

Any thoughts?

ljharb · 2017-01-28T19:11:20Z

@platinumazure the lexical binding created by a function's syntactical name is only available inside the function - the point is that it's useful for recursion, and since foo could be reassigned, bar will always safely point to the function itself.

platinumazure · 2017-01-28T19:12:46Z

@ljharb Thanks, that makes sense. In that case, I think the spirit of the rule says that a case like foo = function undefined() {} must be invalid, since it's not "merely" an object key or other string, do you agree?

not-an-aardvark · 2017-01-28T19:14:51Z

I would say that the rule should ban anything that creates a variable called undefined in any scope. So for example, this should be disallowed:

var foo = function undefined() {
  // undefined is a function here
};

({
  foo: function undefined() {
    // undefined is a function here
  }
})

(This contradicts what I said here because I'd forgotten that undefined would be in-scope in the function body.)

eslintbot · 2017-01-28T19:14:55Z

LGTM

platinumazure · 2017-01-28T19:16:13Z

Awesome. I've just pushed a commit that should address those points.

@not-an-aardvark Can you please review for whether the test cases are sufficient? (I'm not liking the way the code is currently laid out, so I will refactor it, possibly to try using escope references as @mysticatea suggested. But as I mentioned before, I wanted to get the test cases correct first.)

not-an-aardvark · 2017-01-28T19:20:48Z

The tests look good. The one thing I might add is

({ [undefined]: foo }); // invalid

platinumazure · 2017-01-28T19:21:18Z

Good call, I'll add that. Thanks!

…

On Jan 28, 2017 1:20 PM, "Teddy Katz" ***@***.***> wrote: The tests look good. The one thing I might add is ({ [undefined]: foo }); // invalid — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7966 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWeiPBS6-D2DisbSvXIFzaphlwZMW6ks5rW5UQgaJpZM4LqpRl> .

eslintbot · 2017-01-28T19:24:46Z

LGTM

platinumazure · 2017-01-28T19:25:29Z

@not-an-aardvark Good catch, that was not behaving correctly. Hopefully all is well now with regard to test cases. I'll refactor the code later this weekend.

eslintbot · 2017-01-30T03:22:11Z

LGTM

mysticatea · 2017-01-30T12:50:40Z

I have some ideas:

// valid
import undefined as a from "foo"
import {undefined as a} from "foo"
export {undefined} from "foo"
export {undefined as a} from "foo"
export {a as undefined} from "foo"

// invalid
import undefined from "foo"
import * as undefined from "foo"
import {undefined} from "foo"
import {a as undefined} from "foo"
export {undefined}
let a = [b, ...undefined]  // SpreadElement
;[a, ...undefined] = b  // RestElement
;[a = undefined] = b  // AssignmentPattern

ljharb · 2017-01-30T16:14:48Z

Also any object destructuring/rest pattern that creates an undefined binding

platinumazure · 2017-01-30T17:04:48Z

Removing "do not merge" since the dependent PR has been merged and this one has been rebased. (Still isn't ready to go, of course.)

platinumazure · 2017-01-30T23:33:31Z

I also want to note that the original issue focused specifically on simple object keys. On some level, I'm wondering if it's worth fixing that and opening a separate issue for ensuring import/export and other syntax is well supported.

In the meanwhile, I'm halfway down the rabbit hole for escope. I got many test cases to work by warning on variable references, but it's not sufficient for cases where there is merely a declaration at play, so I'm playing around some more trying to figure that out.

eslintbot · 2017-02-03T07:54:52Z

LGTM

eslintbot · 2017-02-03T07:58:37Z

LGTM

platinumazure · 2017-02-03T08:02:08Z

@not-an-aardvark @mysticatea Just pushed some commits to switch to escope and to add the missing test cases. Would you mind giving this another review, please?

@mysticatea I didn't implement the following test case as it seems to be experimental syntax:

// valid
import undefined as a from "foo"

Otherwise I think I've covered everything you suggested.

eslintbot · 2017-02-03T08:06:10Z

LGTM

platinumazure · 2017-02-03T08:10:59Z

Oh-- thinking this might need to be Update since some of the new import/export tests might not have been covered by the old code. I'll try to verify that in the next few days, but if anyone already knows the answer and wants to save some time, I would welcome that.

mysticatea · 2017-02-03T10:16:51Z

@mysticatea I didn't implement the following test case as it seems to be experimental syntax:

Oops, I'm sorry, it's my copy-paste mistake.

mysticatea

Thank you, almost LGTM.
There are some suggestions.

mysticatea · 2017-02-03T10:29:12Z

lib/rules/no-undefined.js

+         */
+        function checkScope(scope) {
+            const undefinedVar = scope.variables
+                ? scope.variables.filter(variable => variable.name === "undefined")[0]


scope has ES6 Map object which is mapping variable names to each variable. It would be more efficient.

const undefinedVar = scope.set.get("undefined") if (!undefinedVar) { return }

(I'm not sure why the name of the Map is set.)

mysticatea · 2017-02-03T10:39:41Z

lib/rules/no-undefined.js

+                .filter(ref => !ref.init)
+                .forEach(ref => report(ref.identifier));
+
+            defs.forEach(def => report(def.name));


I felt that making the array of Identifiers is nice. Only a bit. Feel free ignore this comment.

// All Identifiers of declarations and references. const identifiers = new Set([ ...undefinedVar.defs.map(def => def.name), ...undefinedVar.references.map(ref => ref.identifier) ]) identifiers.forEach(report) // ← or for-of loop.

I'm hoping to avoid using sets of ASTNodes because that depends on escope maintaining referential equality between their definition names and their reference identifiers in this case. (Unless I'm wrong about how Sets work and they actually do deep compares of their members?)

We can expect escope to not clone AST nodes.
We have already depended on it; for example, some rules are using reference.Identifier.parent property ESLint added after scope analysis 😉.

At this point, I'm not implementing this change yet since what I have seems pretty good. @mysticatea If you really want to see this in, please leave a review requesting changes and I'll do it.

mysticatea · 2017-02-03T10:47:26Z

lib/rules/no-undefined.js

+                 */
+                globalScope.through
+                    .filter(isUndefinedReference)
+                    .forEach(ref => report(ref.identifier));


I guess this fallback is unnecessary since the undefined variable exists always.

I'll check into this.

@mysticatea You're right. It seems that scope.through is for when you want to catch variables that were assigned in a lower scope but which have percolated into the current scope without having been originally defined there. Since I'm iterating over all scopes anyway, the variable will be iterated over once I get to its immediate containing scope. I've just removed this logic and added a test case that was missing, and all is well.

mysticatea · 2017-02-03T10:50:37Z

tests/lib/rules/no-undefined.js

+const ruleTester = new RuleTester({
+    parserOptions: {
+        ecmaVersion: 6,
+        sourceType: "module"


sourceType: "module" creates extra scope for modules, so I think it should not apply to all test cases.

eslintbot · 2017-02-03T16:02:41Z

LGTM

not-an-aardvark · 2017-02-04T03:36:37Z

lib/rules/no-undefined.js

-                    if (!parent || parent.type !== "MemberExpression" || node !== parent.property || parent.computed) {
-                        context.report({ node, message: "Unexpected use of undefined." });
-                    }
+                    stack.push.apply(stack, scope.childScopes);


I'm slightly concerned about performance here, since this requires another traversal of the scope tree. Would it be better to check the scopes during the initial AST traversal?

Actually, there are several rules which are traversing variables by the same way. This is a tail call optimized recursive calls.

Probably we can make events to traverse every variable in another PR; e.g. onVariable.

I ran this command on my branch and on its merge-base:

TIMING=1 node ./bin/eslint.js --no-eslintrc --parser-options "ecmaVersion: 6" --rule "no-undefined: error" lib tests

On master, the rule ran for 280-330ms. On my branch, the rule ran for 44-48ms. I'm open to suggestions on how I can improve my testing in case I'm doing something wrong there, but I assume that the switch from "Identifier" visitor to "Program:exit" helps quite a bit.

I confirmed it. For our codebase, this implementation is x5 faster than original implementation.

https://gist.github.com/mysticatea/65b6e7a3298b6dc255389e79725f9c70

I guess, it's because there are lots of unrelated Identifier nodes. Relatively the number of scopes is not much.

platinumazure · 2017-02-04T03:48:34Z

The main/only reason I did it this way was so I wouldn't have to bother with whether a BlockStatement counts as a new scope or not (yes in ES2015+, no in ES5). If we had events for entering and leaving scope without having to worry about node type and ecmaVersion, I would be happy to refactor in that direction. @mysticatea Can code path analysis be used for this purpose?

…

On Feb 3, 2017 9:41 PM, "Teddy Katz" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/rules/no-undefined.js <#7966 (review)>: > - if (!parent || parent.type !== "MemberExpression" || node !== parent.property || parent.computed) { - context.report({ node, message: "Unexpected use of undefined." }); - } + stack.push.apply(stack, scope.childScopes); I'm slightly concerned about performance here, since this requires another traversal of the scope tree. Would it be better to check the scopes during the initial AST traversal? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7966 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWesjr34obdKKqgx3-P5dekLRW2rGgks5rY_NPgaJpZM4LqpRl> .

eslintbot · 2017-02-07T01:21:18Z

LGTM

eslintbot · 2017-02-07T01:35:03Z

LGTM

eslintbot · 2017-02-07T01:38:29Z

LGTM

platinumazure · 2017-02-07T01:43:10Z

Ran the new tests against the old code. A few cases where 2 errors would have been reported at the same location (which I've since fixed down to one), and other cases where they were valid but saw reports. I never saw an increase in reports. So I could have left this at "Fix", but with all the refactoring (and performance improvements), it feels right to make this an "Update".

@mysticatea I think this might be ready for review once more-- if you have time. Thanks!

mysticatea

LGTM, thank you!

I'm OK if it's Fix:.

platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly do not merge This pull request should not be merged yet rule Relates to ESLint's core rules labels Jan 23, 2017

not-an-aardvark suggested changes Jan 23, 2017

View reviewed changes

ljharb reviewed Jan 23, 2017

View reviewed changes

platinumazure force-pushed the no-undefined-false-positives branch from 04bae5f to 2794cf6 Compare January 28, 2017 18:11

platinumazure removed the do not merge This pull request should not be merged yet label Jan 30, 2017

platinumazure force-pushed the no-undefined-false-positives branch from 2c71a78 to 0595124 Compare February 3, 2017 07:58

platinumazure force-pushed the no-undefined-false-positives branch from 0595124 to a792889 Compare February 3, 2017 08:06

mysticatea reviewed Feb 3, 2017

View reviewed changes

not-an-aardvark reviewed Feb 4, 2017

View reviewed changes

not-an-aardvark approved these changes Feb 5, 2017

View reviewed changes

gyandeeps approved these changes Feb 6, 2017

View reviewed changes

platinumazure force-pushed the no-undefined-false-positives branch from e782156 to ab7310c Compare February 7, 2017 01:21

platinumazure force-pushed the no-undefined-false-positives branch from ab7310c to ec86b3f Compare February 7, 2017 01:35

Update: no-undefined handles properties/classes/modules (fixes #7964)

58512ac

platinumazure force-pushed the no-undefined-false-positives branch from ec86b3f to 58512ac Compare February 7, 2017 01:38

platinumazure changed the title ~~Fix: no-undefined warned on non-computed property keys (fixes #7964)~~ Update: no-undefined handles properties/classes/modules (fixes #7964) Feb 7, 2017

mysticatea approved these changes Feb 7, 2017

View reviewed changes

alberto merged commit e228d56 into master Feb 7, 2017

platinumazure deleted the no-undefined-false-positives branch February 8, 2017 00:38

This was referenced Feb 20, 2017

Update dependency eslint to version 3.19.0 singapore/lint-condo#244

Merged

Update dependency eslint to version 3.16.0 renovatebot/renovate#111

Merged

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Update: no-undefined handles properties/classes/modules (fixes #7964) #7966

Update: no-undefined handles properties/classes/modules (fixes #7964) #7966

Conversation

platinumazure commented Jan 23, 2017 • edited

mention-bot commented Jan 23, 2017

eslintbot commented Jan 23, 2017

Choose a reason for hiding this comment

not-an-aardvark Jan 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mysticatea commented Jan 23, 2017

platinumazure commented Jan 23, 2017 via email

platinumazure commented Jan 23, 2017

eslintbot commented Jan 28, 2017

eslintbot commented Jan 28, 2017

platinumazure commented Jan 28, 2017

ljharb commented Jan 28, 2017

platinumazure commented Jan 28, 2017

not-an-aardvark commented Jan 28, 2017

eslintbot commented Jan 28, 2017

platinumazure commented Jan 28, 2017

not-an-aardvark commented Jan 28, 2017

platinumazure commented Jan 28, 2017 via email

eslintbot commented Jan 28, 2017

platinumazure commented Jan 28, 2017

eslintbot commented Jan 30, 2017

mysticatea commented Jan 30, 2017

ljharb commented Jan 30, 2017

platinumazure commented Jan 30, 2017

platinumazure commented Jan 30, 2017

eslintbot commented Feb 3, 2017

eslintbot commented Feb 3, 2017

platinumazure commented Feb 3, 2017

eslintbot commented Feb 3, 2017

platinumazure commented Feb 3, 2017

mysticatea commented Feb 3, 2017

mysticatea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mysticatea Feb 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure commented Feb 4, 2017 via email

eslintbot commented Feb 7, 2017

eslintbot commented Feb 7, 2017

eslintbot commented Feb 7, 2017

platinumazure commented Feb 7, 2017 • edited

mysticatea left a comment

Choose a reason for hiding this comment

platinumazure commented Jan 23, 2017 •

edited

not-an-aardvark Jan 23, 2017 •

edited

mysticatea Feb 3, 2017 •

edited

platinumazure commented Feb 7, 2017 •

edited