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
Update: Support generator yields in no constant condition #8762
Conversation
LGTM |
@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. |
@platinumazure when you get some time, could I get your thoughts on how to approach the two failing tests I have? |
There was a problem hiding this 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" }] } |
There was a problem hiding this comment.
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;
}
}
}
}
There was a problem hiding this comment.
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; ;) {
}
}
lib/rules/no-constant-condition.js
Outdated
checkLoops = options.checkLoops !== false, | ||
sourceCode = context.getSourceCode(); | ||
|
||
let containsYield = false; |
There was a problem hiding this comment.
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.)
LGTM |
There was a problem hiding this 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" }] } |
There was a problem hiding this comment.
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 } } |
There was a problem hiding this comment.
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 } });
lib/rules/no-constant-condition.js
Outdated
checkLoops = options.checkLoops !== false, | ||
loopSetStack = []; | ||
|
||
let setOfConstantLoops = new Set(); |
There was a problem hiding this comment.
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.)
lib/rules/no-constant-condition.js
Outdated
* @private | ||
*/ | ||
function exitFunction(node) { | ||
if (setOfConstantLoops.size > 0) { |
There was a problem hiding this comment.
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.
lib/rules/no-constant-condition.js
Outdated
@@ -144,10 +206,18 @@ module.exports = { | |||
|
|||
return { | |||
ConditionalExpression: checkConstantCondition, | |||
"ConditionalExpression:exit": checkConstantConditionLoopInSet, |
There was a problem hiding this comment.
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.
lib/rules/no-constant-condition.js
Outdated
@@ -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) { |
There was a problem hiding this comment.
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.
lib/rules/no-constant-condition.js
Outdated
* @returns {void} | ||
* @private | ||
*/ | ||
function checkIfConstant(node) { |
There was a problem hiding this comment.
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
?
lib/rules/no-constant-condition.js
Outdated
* @returns {void} | ||
* @private | ||
*/ | ||
function clearSetOfLoops() { |
There was a problem hiding this comment.
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.
lib/rules/no-constant-condition.js
Outdated
IfStatement: checkConstantCondition, | ||
"IfStatement:exit": checkIfConstant, |
There was a problem hiding this comment.
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.
LGTM |
9fd4dcd
to
fe67ba0
Compare
LGTM |
Thanks for going through this with me. it's very helpful for me. |
Hmm, that case is difficult. I guess one solution would be to only "enter" a -ForStatement: checkLoop
+"ForStatement > .test": node => checkLoop(node.parent) |
LGTM |
7876819
to
e4f3850
Compare
LGTM |
…lint#8566)" This reverts commit 7bcd3cc.
LGTM |
LGTM |
lib/rules/no-constant-condition.js
Outdated
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." }); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 } } |
There was a problem hiding this comment.
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.
LGTM |
LGTM |
lib/rules/no-constant-condition.js
Outdated
ForStatement: checkLoop | ||
"DoWhileStatement:exit": checkConstantConditionLoopInSet, | ||
ForStatement: checkLoop, | ||
"ForStatement > .init * > YieldExpression, ForStatement > .init": node => checkYieldInInitForLoop(node), |
There was a problem hiding this comment.
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" }]
}
]
});
There was a problem hiding this comment.
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.
lib/rules/no-constant-condition.js
Outdated
*/ | ||
function checkYieldInInitForLoop(node) { | ||
if (node.type === "YieldExpression" || (node.right && node.right.type === "YieldExpression")) { | ||
context.report({ node: node.parent, message: "Unexpected constant condition." }); |
There was a problem hiding this comment.
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;
}
}
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
LGTM |
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:
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?