Update: Report assignment expression location in no-cond-assign

[mdjermanovic]

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

[X] Changes an existing rule

What rule do you want to change?

no-cond-assign - changes just reported locations.

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

In general, the same.

But, if we consider eslint-disable comments, in some cases with the always option this change can produce more or fewer warnings because the reported line could change.

How will the change be implemented? (New option, new default behavior, etc.)?

New default behavior

Please provide some example code that this change will affect:

/* eslint no-cond-assign: ["error"] */

if (a = b) {
    foo();
}

if (a.bar = b) {
    foo();
}

/* eslint no-cond-assign: ["error", "always"] */

if (a = b) {
    foo();
}

if (a.bar = b) {
    foo();
}

What does the rule currently do for this code?

The default "except-parens" option and the "always" option have completely different implementations, and the reported locations are quite different.

The default "except-parens" option reports just the start location of the test node (which is an assignment expression). This highlights only the first token (or nothing in the online demo).

The always option highlights the whole if node.

What will the rule do after it's changed?

Always report the location of the assignment expression, with both start and end.

What changes did you make? (Give an overview)

Changed reported locations for both options.

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

This change can produce more warnings when the option is always and the first line is disabled and the assignment is not on the first line.

/* eslint no-cond-assign: ["error", "always"] */

// eslint-disable-next-line no-cond-assign
if (
    a = b
) {
	foo();
}

[platinumazure]

Is there a reason why the IfStatement node is the reported node? It seems to me that we should just always report the AssignmentExpression.

[mdjermanovic]

Is there a reason why the IfStatement node is the reported node? It seems to me that we should just always report the AssignmentExpression.

Sorry, the PR description was confusing, that's exactly what this PR does - reports the 'invalid' AssignmentExpression node regardless of the option.

I agree there is no reason to report IfStatement, especially when the option is set to "always" which reports even the assignments nested in the test condition.

[platinumazure]

I'm using this review to try to explain what I'm getting at a little better. (Please refer to the inline comment)

[platinumazure]

I assume that if we are reporting the node.test location (node.test.loc), then node.type === "IfStatement".

Why not just report node: node.test (on line 108)? Then the code automatically uses the location of that node (i.e., node.test.loc) when preparing the lint report.

I don't think we are getting any value in reporting on the IfStatement but setting the location equal to the AssignmentExpression's location.

[mdjermanovic]

Ah, sorry I didn't understand the first time!

Since the rule targets the assignment, I completely agree. I'm not sure why was the IfStatement reported. Maybe because the message is "Expected a conditional expression and instead saw an assignment.". There is a following comment in the code:

// must match JSHint's error message
missing: "Expected a conditional expression and instead saw an assignment."

[mdjermanovic]

#4040 might be relevant to this question. The loc was added to point to the test node instead of just changing the reported node to the test node. I'm not sure why.

[mdjermanovic]

The changes in this PR indeed look really unusual compared to all other rules.

Looks much better to simply report the AssignmentExpression node, as you suggested.

I'll modify this, which will change nodeType in the linting output, I guess that isn't information of particular importance?

[mdjermanovic]

This is done now.

[platinumazure]

I think this could be accepted as a bug fix, personally.

[ilyavolodin]

Agree. This should be bug fix or chore.

[kaicataldo]

LGTM, thanks! Mind fixing up the merge conflicts so we can land this?

[mdjermanovic]

LGTM, thanks! Mind fixing up the merge conflicts so we can land this?

Rebased and fixed now

[kaicataldo]

LGTM, thanks!

[kaicataldo]

One last question before we merge this - it looks like the consensus is to accept this as a bug fix. It seems to me like it should still be considered a semver-minor change because it can lead to more errors in the case described in the original description. Thoughts?