Fix: no-cond-assign within a function expression (fixes #6908) #6909

pmcelhaney · 2016-08-15T15:26:10Z

What issue does this pull request address?
#6908

Stops checking to see if an assignment is within a condition if the assignment is wrapped in a call expression, making the behavior consistent between "except-parens" and "always"

What changes did you make? (Give an overview)
I noticed that the behavior depends on whether the configuration is set to "always" or "except-parens". If configured to "except-parens" the behavior was as expected in the bug report. "always" is more strict.

Is there anything you'd like reviewers to focus on?
This might be a breaking change. I actually moved one of the unit tests from invalid to valid. But we should probably discuss that in the issue.

mention-bot · 2016-08-15T15:26:11Z

@pmcelhaney, thanks for your PR! By analyzing the annotation information on this pull request, we identified @mysticatea, @vitorbal and @spmurrayzzz to be potential reviewers

eslintbot · 2016-08-15T15:26:11Z

Thanks for the pull request, @pmcelhaney! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.
The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

pmcelhaney · 2016-08-15T15:40:47Z

tests/lib/rules/no-cond-assign.js

@@ -36,7 +36,8 @@ ruleTester.run("no-cond-assign", rule, {
        "while (someNode || (someNode = parentNode)) { }",
        "do { } while (someNode || (someNode = parentNode));",
        "for (;someNode || (someNode = parentNode););",
-        "if ((function(node) { return (node = parentNode); })(someNode)) { }",
+        { code: "if ((function(node) { return (node = parentNode); })(someNode)) { }", options: ["except-parens"] },
+        { code: "if ((function(node) { return (node = parentNode); })(someNode)) { }", options: ["always"] },


Moved the rule from invalid to valid in the "always" config, and updated the "except-parens" config to be more explicit, so you can see that the code is now considered valid in either case.

eslintbot · 2016-08-15T17:01:15Z

Thanks for the pull request, @pmcelhaney! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.
The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2016-08-15T17:20:49Z

Thanks for the pull request, @pmcelhaney! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.
The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2016-08-15T17:48:19Z

LGTM

platinumazure · 2016-08-15T17:50:25Z

tests/lib/rules/no-cond-assign.js

+        { code: "if ((function(node) { return (node = parentNode); })(someNode)) { }", options: ["except-parens"] },
+        { code: "if ((function(node) { return (node = parentNode); })(someNode)) { }", options: ["always"] },
+        { code: "if ((node => node = parentNode)(someNode)) { }", options: ["except-parens"], parserOptions: { ecmaVersion: 6 } },
+        { code: "if ((node => node = parentNode)(someNode)) { }", options: ["always"], parserOptions: { ecmaVersion: 6 } },


I'd love to see a test just for the function expression itself (not immediately invoked). Yes, that's a useless/constant condition, but it'd be good to make sure that case is covered anyway.

platinumazure · 2016-08-15T17:50:54Z

LGTM except for this:

I'd love to see a test just for the function expression itself (not immediately invoked). Yes, that's a useless/constant condition, but it'd be good to make sure that case is covered anyway.

eslintbot · 2016-08-15T18:01:51Z

LGTM

pmcelhaney · 2016-08-15T18:04:07Z

Okay, I've added a couple more test cases to cover the function expression without invocation.

platinumazure · 2016-08-16T21:23:38Z

lib/rules/no-cond-assign.js

@@ -59,7 +59,7 @@ module.exports = {
                if (isConditionalTestExpression(currentAncestor)) {
                    return currentAncestor.parent;
                }
-            } while ((currentAncestor = currentAncestor.parent));
+            } while ((currentAncestor = currentAncestor.parent) && currentAncestor.type !== "FunctionExpression" && currentAncestor.type !== "ArrowFunctionExpression");


Would ast-utils' isFunction method work here?

platinumazure · 2016-08-16T21:25:31Z

Thanks @pmcelhaney! I've added one more comment.

LGTM, except:

Wondering if ast-utils might come in handy when checking that the node type is FunctionExpression or ArrowFunctionExpression? (The function I named also checks FunctionDeclaration, which syntactically can't be in an if statement, but you would need to triple-check that the upward traversal definitely stops at the conditional statement.)

If that's not feasible or desired, then this can be merged (as far as I'm concerned). Thanks!

Stops checking to see if an assignment is within a condition if the assignment is wrapped in a function expression, making the behavior consistent between "except-parens" and "always"

eslintbot · 2016-08-17T14:11:28Z

LGTM

pmcelhaney · 2016-08-17T14:13:25Z

Added the astUtils function. Good idea. Thanks!

platinumazure · 2016-08-22T21:33:00Z

Oh man, I can't believe I let this sit. Sorry! LGTM.

jquerybot added the CLA: Valid label Aug 15, 2016

pmcelhaney reviewed Aug 15, 2016
View reviewed changes

pmcelhaney force-pushed the fix-6908-no-cond-assign-false-positive branch from 8a313d7 to 8e018cb Compare August 15, 2016 17:01

pmcelhaney force-pushed the fix-6908-no-cond-assign-false-positive branch from 8e018cb to 45484eb Compare August 15, 2016 17:20

pmcelhaney force-pushed the fix-6908-no-cond-assign-false-positive branch from 45484eb to 3b83cc3 Compare August 15, 2016 17:48

platinumazure reviewed Aug 15, 2016
View reviewed changes

pmcelhaney force-pushed the fix-6908-no-cond-assign-false-positive branch from 3b83cc3 to d86baa5 Compare August 15, 2016 18:01

pmcelhaney changed the title ~~fix(no-cond-assign): allow assignment within a call expression (fixes…~~ Fix: no-cond-assign within a function expression (fixes #6908) Aug 16, 2016

platinumazure reviewed Aug 16, 2016
View reviewed changes

Fix: no-cond-assign within a function expression (fixes eslint#6908)

d4ad00f

Stops checking to see if an assignment is within a condition if the assignment is wrapped in a function expression, making the behavior consistent between "except-parens" and "always"

pmcelhaney force-pushed the fix-6908-no-cond-assign-false-positive branch from d86baa5 to d4ad00f Compare August 17, 2016 14:11

platinumazure merged commit a063d4e into eslint:master Aug 22, 2016

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: no-cond-assign within a function expression (fixes #6908) #6909

Fix: no-cond-assign within a function expression (fixes #6908) #6909

pmcelhaney commented Aug 15, 2016 •

edited

mention-bot commented Aug 15, 2016

eslintbot commented Aug 15, 2016

pmcelhaney Aug 15, 2016

eslintbot commented Aug 15, 2016

eslintbot commented Aug 15, 2016

eslintbot commented Aug 15, 2016

platinumazure Aug 15, 2016

platinumazure commented Aug 15, 2016 •

edited

eslintbot commented Aug 15, 2016

pmcelhaney commented Aug 15, 2016

platinumazure Aug 16, 2016

platinumazure commented Aug 16, 2016

eslintbot commented Aug 17, 2016

pmcelhaney commented Aug 17, 2016

platinumazure commented Aug 22, 2016

Fix: no-cond-assign within a function expression (fixes #6908) #6909

Fix: no-cond-assign within a function expression (fixes #6908) #6909

Conversation

pmcelhaney commented Aug 15, 2016 • edited

mention-bot commented Aug 15, 2016

eslintbot commented Aug 15, 2016

pmcelhaney Aug 15, 2016

Choose a reason for hiding this comment

eslintbot commented Aug 15, 2016

eslintbot commented Aug 15, 2016

eslintbot commented Aug 15, 2016

platinumazure Aug 15, 2016

Choose a reason for hiding this comment

platinumazure commented Aug 15, 2016 • edited

eslintbot commented Aug 15, 2016

pmcelhaney commented Aug 15, 2016

platinumazure Aug 16, 2016

Choose a reason for hiding this comment

platinumazure commented Aug 16, 2016

eslintbot commented Aug 17, 2016

pmcelhaney commented Aug 17, 2016

platinumazure commented Aug 22, 2016

pmcelhaney commented Aug 15, 2016 •

edited

platinumazure commented Aug 15, 2016 •

edited