no-cond-assign analyzes too deep into conditionals

[Satyam]

What version are you using?

3.2.2

What did you do?

const arr = [1, 2, 3];

if (arr.some(val => {
  let b = true;
  if (val === 2) {
    b = false;
  }
  return b;
})) console.log('returned true');

What happened?

With this eslintrc:

module.exports = {
    "env": {
        "browser": true,
        "es6": true
    },
    "extends": "eslint:recommended",
    "rules": {
      "no-console": 0,
      "no-cond-assign": [2, 'always']
    }
};

It signaled:

Unexpected assignment within an 'if' statement  no-cond-assign

What did you expect to happen?

It should have ceased to analyze once in the callback of Array.prototype.some and should not care at all about any assignments within. The purpose of the rule is to avoid mistakenly doing an assignment instead of a comparison within a conditional, not to prevent assignments at all.

[platinumazure]

I agree, this seems like a bug.

Are you willing/able to write a fix? (The answer can be no- we just like to make sure folks have a chance to contribute!)

[Satyam]

No, sorry, I'm afraid I wouldn't know where to start, not a clue.

I detected it because the code that worked fine with ESLint 2 started giving me this error when upgrading. Since I was using the airbnb config I reported it there (airbnb/javascript#1007) since I tried the code sample above with the standard and eslint:recommended configs and had no problem with any of those. The last comment on that issue pointed me to the the eslintrc config shown above.

[pmcelhaney]

I can have a look. Give me a couple days.

[platinumazure]

@Satyam Thanks very much, had to ask.

@pmcelhaney Thanks a lot, you rock. If you don't end up having time, please post back here so someone else knows they can start looking. (No pressure whatsoever, we just want to keep things moving.)

[Satyam]

@platinumazure Sure, I understand, no problem, thanks for asking. If I were to try a fix, I would simply bog things down. I can't beat @pmcelhaney even if it takes twice as much. Thanks.

[pmcelhaney]

Turns out it depends on how you have the rule configured.

This config considers your code invalid.

"no-cond-assign": [2, "always"]

These configs consider your code valid ("except-parens" is the default).

"no-cond-assign": [2, "except-parens"],
"no-cond-assign": 2

The existing unit tests actually enforce this difference, which I think is a mistake. The documentation doesn't say anything about what should happen if the condition is an inline function, and being inside a function is different from being wrapped in parentheses.

[platinumazure]

I agree that we should always allow assignments inside the body of a
function, regardless of parentheses.

@pmcelhaney Are you saying that our parentheses check is also too loose? I
think only parentheses immediately inside the if condition parentheses
should count there, but if the function call parentheses are also
suppressing a lint warning, that's probably not good either.

Simple test case might be something like this: if (doSomething(foo = 5)) {}

If those parentheses are misbehaving, let's open a separate issue for that.

On Aug 15, 2016 10:37 AM, "Patrick McElhaney" notifications@github.com
wrote:

Turns out it depends on how you have the rule configured.

This config considers your code invalid.

"no-cond-assign": [2, "always"]

These configs consider your code valid ("except-parens" is the default).

"no-cond-assign": [2, "except-parens"],"no-cond-assign": 2

The existing unit tests actually enforce this difference, which I think is
a mistake. The documentation doesn't say anything about what should happen
if the condition is an inline function, and being inside a function is
different from being wrapped in parentheses.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6908 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARWevHgk7XGU2K6PL-QpWGqb4-d7_obks5qgIe_gaJpZM4JkWZK
.

[pmcelhaney]

The two options are implemented with different algorithms. That's why this bug shows up in one case but not the other.

I just put if (doSomething(foo = 5)) {} into the valid tests and it passed. Not sure if my change fixed it. Should I go ahead and add that test to the PR?

[platinumazure]

No, I think that might be a separate issue. To me the spirit of the
"except-parens" option is that an assignment in redundant parentheses is
intentional. The argument list of a call expression, on the other hand,
must be surrounded by parentheses and so that test should not be valid. But
it is, so I think there's a separate issue there.

It also masked the original poster's test case (that is, OP's test case
does not show a lint error unless "always" is configured).

Sorry if I've confused the issue. Does this help at all?

On Aug 15, 2016 10:53 AM, "Patrick McElhaney" notifications@github.com
wrote:

The two options are implemented with different algorithms. That's why this
bug shows up in one case but not the other.

I just put if (doSomething(foo = 5)) {} into the valid tests and it
passed. Not sure if my change fixed it. Should I go ahead and add that test
to the PR?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6908 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARWeixbAjzI3wOURekrZNb0mJLX0HTPks5qgIttgaJpZM4JkWZK
.

[Satyam]

@pmcelhaney That rule is exactly the one that the airbnb config has set: https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/rules/errors.js#L7

The code doSomething(foo = 5) whether inside a conditional or not, should produce an error, but not under this rule. Just like a programmer might have mistakenly used an assignment instead of a double or triple equal, the guy writing the above code might have confused the new ES6 default parameter value feature, which is valid for a function declaration but not function invocation.

[Satyam]

@platinumazure I agree, the rule should accept clearly redundant sets of parenthesis, not others that might reasonably be there.

[pmcelhaney]

I went ahead and added tests to clarify if(doSomething(foo = 5)) is not a violation of this rule.

When the rule looks at if (bar = doSomething(foo = 5)), it should see if (bar = [CallExpression]). It cares about the assignment to bar, but the assignment to foo is outside its purview.

As @Satyam said, the code doSomething(foo = 5) alone is suspect, but that should be covered by a new rule. no-arg-assign?

[platinumazure]

Apologies, I definitely introduced confusion.

I don't believe we should fix if (doSomething(a=5)) in this issue, either in terms of a no-arg-assign rule or in terms of deciding that (a=5) should or shouldn't be considered "parenthesized" for the "except-parens" option.

I would prefer to just check that

• An assignment
• Within a FunctionExpression
• Within the test of an if statement or other conditional statement

Should not be flagged, simply because the assignment is in a different scope.

Can we keep this issue and any related PRs scoped to that approach? Please feel free to create a separate issue for any other problem that we've discussed in here.

Thanks for your understanding. Again, I apologize for my part in allowing the discussion to diverge.

[pmcelhaney]

No worries, I've updated the PR so that it only addresses the issue at hand.

[nzakas]

@platinumazure you didn't mark this as accepted, have you verified this as a bug?

[platinumazure]

Apologies @nzakas, I hadn't had time to verify until just now.

Here's a snippet that repros in the demo:

/* eslint no-cond-assign: ["error", "always"] */
if (arr.some(function (item) { item = 5; })) {
    doSomething();
}

As discussed earlier in the issue, there is a separate issue (parentheses being too zealously considered) that prevents this from showing up as a bug with the default "except-parens" configuration. But I believe we can accept this bug, unless you disagree.