Skip to content
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

Update: Support generator yields in no constant condition #8762

Merged
merged 8 commits into from Jul 21, 2017

Conversation

VictorHom
Copy link
Member

@VictorHom VictorHom commented Jun 18, 2017

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

[X] Changes an existing rule (template)

What rule do you want to change?
no-constant-conditions

Does this change cause the rule to produce more or fewer warnings?
produce less warning

How will the change be implemented? (New option, new default behavior, etc.)?
it's supporting yields in generator functions that might otherwise be recognized as violating no-constant-condition rule

Please provide some example code that this change will affect:
Supports this as good:

function*() {
    while(true) {
        yield 'foo';
    } 
}

You can check the issue at #8566

What does the rule currently do for this code?
Currently, it would just mark the loop as a violation of the rule (While (true) is a breaking the rule)

What will the rule do after it's changed?
It would pass in the generator function example if you yield in what might otherwise be considered a constant-condition.

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

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

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

@VictorHom VictorHom changed the title Update: support generator yields in no constant condition Update: (WIP) support generator yields in no constant condition Jun 18, 2017
@VictorHom
Copy link
Member Author

@platinumazure when you get some time, could I get your thoughts on how to approach the two failing tests I have?

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules labels Jun 21, 2017
Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I left a suggestion for how to fix the failing test cases.

{ code: "function* foo(){while(true){if (true) {yield 'foo';}}}", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Unexpected constant condition.", type: "IfStatement" }] },
{ code: "function* foo(){if (true) {yield 'foo';}}", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Unexpected constant condition.", type: "IfStatement" }] },
{ code: "function* foo(){while(true){yield 'foo';}while(true){}}", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Unexpected constant condition.", type: "WhileStatement" }] },
{ code: "var a = function* foo(){while(true){} yield 'foo';}", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Unexpected constant condition.", type: "WhileStatement" }] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more test cases to consider:

// This should be valid because the `yield` would pause both loops.
while (true) {
  while (true) {
    yield;
  }
}
// This should be invalid because the `yield` would not pause the loop.
while (true) {
  function* foo() {
    yield;
  }
}
// This should be invalid because the `yield` does not get executed on every loop iteration.
function* foo() {
  for (let foo = yield; ;) {}
}
// This should be valid because the `yield` gets executed on every loop iteration.
function* foo() {
  for (; yield; ) {}
}
// This should be valid because the `yield` gets executed on every loop iteration.
function* foo() {
  for (; ; yield) {}
}
// This should be valid because the second `yield` would pause the loop.
function* foo() {
  while (true) {
    function* foo() {
      yield;
    }
    yield;
  }
}
// invalid for the outer loop, valid for the inner loop
function foo() {
  while (true) {
    function* bar() {
      while (true) {
        yield;
      }
    }
  }
}

Copy link
Member Author

@VictorHom VictorHom Jul 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@not-an-aardvark this is awesome advice. I made some updates and would appreciate a review when you have the time.

I am failing one of the cases you pointed out. What's a way to handle checking if the yield is happening in the initialization (first parameter of the for):

function* foo() {
    for (let foo = yield; ;) {
    }
}

checkLoops = options.checkLoops !== false,
sourceCode = context.getSourceCode();

let containsYield = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like having a single containsYield variable will not be able to handle the case where one loop in the file contains yield, and another doesn't.

Additionally, we should make sure the rule handles the case where a yield inside a loop is in a function (which would prevent it from pausing the loop). See my other comment with test cases for examples of this.

Instead, my recommendation would be to do the following while traversing the AST:

  • Keep of a set of infinite loops for which there hasn't been any yield statement found yet.
    • When an infinite loop is entered, add it to the set.
    • When a yield is encountered, remove everything from the set.
    • When a loop is exited, report it if it's in the set (and optionally remove it from the set, since it's no longer important).
    • When entering a function, push the current set onto a stack and work with a new empty set.
    • When exiting a function, discard the current set, then pop the old set from the stack and continue working with that one.

By using this strategy, you can always keep track of the current loops that the traverser is inside.

A vaguely similar strategy is used here and here in the object-shorthand rule. (The goal of that logic is slightly different in that it's trying detect the scope of a given this expression within a given arrow function, rather than a yield expression within a given loop.)

@eslintbot
Copy link

LGTM

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this generally looks good. I left a few small suggestions -- I think there are some cases where the variable/function names are slightly vague, which makes the logic a bit hard to follow

{ code: "while (true) { function* foo() {yield;}}", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Unexpected constant condition.", type: "WhileStatement" }] },
{ code: "function* foo(){if (true) {yield 'foo';}}", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Unexpected constant condition.", type: "IfStatement" }] },
{ code: "function* foo() {for (let foo = yield; ;) {}}", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Unexpected constant condition.", type: "ForStatement" }] },
{ code: "function foo() {while (true) {function* bar() {while (true) {yield;}}}}", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Unexpected constant condition.", type: "WhileStatement" }] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Can you space these tests out a bit more rather than having each test be a single line?

{
    code: "function* foo(){while(true){} yield 'foo';}",
    errors: [{ message: "Unexpected constant condition.", type: "WhileStatement" }]
}

Personally, I think that makes the tests easier to read -- I find it a bit difficult to read the them when they're compacted like that.

{ code: "function* foo(){while (true) { while(true) {yield;}}}", parserOptions: { ecmaVersion: 6 } },
{ code: "function* foo() {for (; yield; ) {}}", parserOptions: { ecmaVersion: 6 } },
{ code: "function* foo() {for (; ; yield) {}}", parserOptions: { ecmaVersion: 6 } },
{ code: "function* foo() {while (true) {function* foo() {yield;}yield;}}", parserOptions: { ecmaVersion: 6 } }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Instead of specifying parserOptions: { ecmaVersion: 6 } for each of these tests, it would be better to add it to the RuleTester constructor:

const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });

checkLoops = options.checkLoops !== false,
loopSetStack = [];

let setOfConstantLoops = new Set();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Can you give this variable a more descriptive name? It's not immediately clear what loops are supposed to be contained in the set. (Something like loopsInCurrentScope seems like it would be a good name.)

* @private
*/
function exitFunction(node) {
if (setOfConstantLoops.size > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this condition ever occur? It seems like any loop in the set would have already been removed when the loop was exited, so it will never still be in the set when the function is exited.

@@ -144,10 +206,18 @@ module.exports = {

return {
ConditionalExpression: checkConstantCondition,
"ConditionalExpression:exit": checkConstantConditionLoopInSet,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line necessary? It seems like ConditionalExpression nodes would never be in setOfConstantLoops anyway.

@@ -115,15 +118,74 @@ module.exports = {
}

/**
* Reports when the given node contains a constant condition.
* Tracks when the given node contains a constant condition.
* @param {ASTNode} node The AST node to check.
* @returns {void}
* @private
*/
function checkConstantCondition(node) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename this function to something like trackConstantConditionLoop? There are several functions in this file that do different things related to checking constant conditions, so the current name is a bit confusing.

* @returns {void}
* @private
*/
function checkIfConstant(node) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename this function to something like reportIfConstant?

* @returns {void}
* @private
*/
function clearSetOfLoops() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly about this, but personally it seems unnecessary to me to have a function for this -- in my opinion, it adds unnecessary indirection.

IfStatement: checkConstantCondition,
"IfStatement:exit": checkIfConstant,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the IfStatement and ConditionalExpression listeners use reportIfConstant rather than checkConstantCondition? It seems like the checkConstantCondition function is only intended for loops.

Also, if I'm understanding correctly I think the ConditionalExpression:exit and IfStatement:exit handlers can be removed, since they don't need to be added to the set of loops.

@eslintbot
Copy link

LGTM

@VictorHom VictorHom force-pushed the pr/no-constant-condition_generators branch from 9fd4dcd to fe67ba0 Compare July 2, 2017 13:02
@eslintbot
Copy link

LGTM

@VictorHom
Copy link
Member Author

Thanks for going through this with me. it's very helpful for me.
@not-an-aardvark
Do you know what I can do to pass this case in the invalid tests?
function* foo() {for (let foo = yield; ;) {}}

@not-an-aardvark
Copy link
Member

Hmm, that case is difficult. I guess one solution would be to only "enter" a ForStatement when entering the test clause. One way to do this could be to change the ForStatement listener to a selector:

-ForStatement: checkLoop
+"ForStatement > .test": node => checkLoop(node.parent)

@eslintbot
Copy link

LGTM

@VictorHom VictorHom force-pushed the pr/no-constant-condition_generators branch from 7876819 to e4f3850 Compare July 15, 2017 01:01
@eslintbot
Copy link

LGTM

@VictorHom VictorHom changed the title Update: (WIP) support generator yields in no constant condition Update: Support generator yields in no constant condition Jul 15, 2017
@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

function checkYieldInInitForLoop(node) {
if ((node.type === "VariableDeclaration" && node.declarations[0].init.type === "YieldExpression") ||
(node.type === "AssignmentExpression" && node.right.type === "YieldExpression")) {
context.report({ node: node.parent, message: "Unexpected constant condition." });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is quite correct, because it will miss more complicated uses of yield:

for (let foo = 1 + 2 + 3 + yield; bar; baz) {}

Have you tried this approach?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally tried that approach. The following 2 test cases fail:

function* foo() {for (let foo = yield; ;) {}}
function* foo() {for (foo = yield; ;) {}}

for the counter case you provided:

for (let foo = 1 + 2 + 3 + yield; bar; baz) {}

The current implementation and the suggestions from the approach lead to the error:

A fatal parsing error occurred: Parsing error: Can not use 'yield' as identifier inside a generator

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my test case was invalid -- I meant to use

function* foo() { for (let foo = 1 + 2 + 3 + (yield); bar; baz) {} }

The rule currently does not report an error for this case, but it should.

{ code: "function* foo(){while (true) { while(true) {yield;}}}", parserOptions: { ecmaVersion: 6 } },
{ code: "function* foo() {for (; yield; ) {}}", parserOptions: { ecmaVersion: 6 } },
{ code: "function* foo() {for (; ; yield) {}}", parserOptions: { ecmaVersion: 6 } },
{ code: "function* foo() {while (true) {function* foo() {yield;}yield;}}", parserOptions: { ecmaVersion: 6 } }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: parserOptions: { ecmaVersion: 6 } can be removed from the individual tests here since it's provided in the constructor.

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

ForStatement: checkLoop
"DoWhileStatement:exit": checkConstantConditionLoopInSet,
ForStatement: checkLoop,
"ForStatement > .init * > YieldExpression, ForStatement > .init": node => checkYieldInInitForLoop(node),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following should work:

View diff
diff --git a/lib/rules/no-constant-condition.js b/lib/rules/no-constant-condition.js
index 9f38c790..e666c776 100644
--- a/lib/rules/no-constant-condition.js
+++ b/lib/rules/no-constant-condition.js
@@ -187,18 +187,6 @@ module.exports = {
             }
         }
 
-        /**
-        * Checks node for yield in init of forloop
-        * @param {ASTNode} node The AST node to check.
-        * @returns {void}
-        * @private
-        */
-        function checkYieldInInitForLoop(node) {
-            if (node.type === "YieldExpression" || (node.right && node.right.type === "YieldExpression")) {
-                context.report({ node: node.parent, message: "Unexpected constant condition." });
-            }
-        }
-
         //--------------------------------------------------------------------------
         // Public
         //--------------------------------------------------------------------------
@@ -210,8 +198,7 @@ module.exports = {
             "WhileStatement:exit": checkConstantConditionLoopInSet,
             DoWhileStatement: checkLoop,
             "DoWhileStatement:exit": checkConstantConditionLoopInSet,
-            ForStatement: checkLoop,
-            "ForStatement > .init * > YieldExpression, ForStatement > .init": node => checkYieldInInitForLoop(node),
+            "ForStatement > .test": node => checkLoop(node.parent),
             "ForStatement:exit": checkConstantConditionLoopInSet,
             FunctionDeclaration: enterFunction,
             "FunctionDeclaration:exit": exitFunction,

I think the confusion happened because I accidentally asked you to add some incorrect tests. I was under the impression that the rule was supposed to report things like for(; ; ); (i.e. for loops that are missing a test property), but I was wrong -- sorry about that.

When testing this locally, I made the following corrections to the tests, which made them all pass:

View diff
diff --git a/tests/lib/rules/no-constant-condition.js b/tests/lib/rules/no-constant-condition.js
index 312f04f5..8897b340 100644
--- a/tests/lib/rules/no-constant-condition.js
+++ b/tests/lib/rules/no-constant-condition.js
@@ -66,7 +66,10 @@ ruleTester.run("no-constant-condition", rule, {
         { code: "function* foo(){while (true) { while(true) {yield;}}}" },
         { code: "function* foo() {for (; yield; ) {}}" },
         { code: "function* foo() {for (; ; yield) {}}" },
-        { code: "function* foo() {while (true) {function* foo() {yield;}yield;}}" }
+        { code: "function* foo() {while (true) {function* foo() {yield;}yield;}}" },
+
+        "function* foo() { for (let x = yield; x < 10; x++); }",
+        "function* foo() { for (let x = yield; ; x++) { yield; } }"
     ],
     invalid: [
         { code: "for(;true;);", errors: [{ message: "Unexpected constant condition.", type: "ForStatement" }] },
@@ -139,11 +142,11 @@ ruleTester.run("no-constant-condition", rule, {
             errors: [{ message: "Unexpected constant condition.", type: "IfStatement" }]
         },
         {
-            code: "function* foo() {for (let foo = yield; ;) {}}",
-            errors: [{ message: "Unexpected constant condition.", type: "VariableDeclarator" }]
+            code: "function* foo() {for (let foo = yield; true;) {}}",
+            errors: [{ message: "Unexpected constant condition.", type: "ForStatement" }]
         },
         {
-            code: "function* foo() {for (foo = yield; ;) {}}",
+            code: "function* foo() {for (foo = yield; true;) {}}",
             errors: [{ message: "Unexpected constant condition.", type: "ForStatement" }]
         },
         {
@@ -151,8 +154,8 @@ ruleTester.run("no-constant-condition", rule, {
             errors: [{ message: "Unexpected constant condition.", type: "WhileStatement" }]
         },
         {
-            code: "function* foo() { for (let foo = 1 + 2 + 3 + (yield); bar; baz) {} }",
-            errors: [{ message: "Unexpected constant condition.", type: "BinaryExpression" }]
+            code: "function* foo() { for (let foo = 1 + 2 + 3 + (yield); true; baz) {} }",
+            errors: [{ message: "Unexpected constant condition.", type: "ForStatement" }]
         }
     ]
 });

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@not-an-aardvark awesome thanks for the review. my bad - I didn't catch to look at the test part of the forloop to decide if something would run as a constant.

I updated the test cases.

*/
function checkYieldInInitForLoop(node) {
if (node.type === "YieldExpression" || (node.right && node.right.type === "YieldExpression")) {
context.report({ node: node.parent, message: "Unexpected constant condition." });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current approach isn't correct because it doesn't actually check the condition of the loop, so the following is reported even though the condition isn't constant:

// should not be reported
function* foo() {
  for (let x = yield; x < 10; x++);
}

Also, there could be another yield in the loop that would prevent it from being infinite:

// should not be reported
function* foo() {
  for (let x = yield; ; ) {
    yield;
  }
}

@eslintbot
Copy link

LGTM

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@eslintbot
Copy link

LGTM

@btmills btmills merged commit 3bebcfd into eslint:master Jul 21, 2017
@VictorHom VictorHom deleted the pr/no-constant-condition_generators branch July 21, 2017 15:59
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants