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: Report assignment expression location in no-cond-assign #12465

Merged
merged 2 commits into from Nov 22, 2019

Conversation

@mdjermanovic
Copy link
Member

mdjermanovic commented Oct 19, 2019

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).

image

The always option highlights the whole if node.

image

What will the rule do after it's changed?

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

image

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
Copy link
Member

platinumazure commented Oct 20, 2019

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
Copy link
Member Author

mdjermanovic commented Oct 20, 2019

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.

Copy link
Member

platinumazure left a comment

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

@@ -106,7 +106,7 @@ module.exports = {

context.report({
node,
loc: node.test.loc.start,
loc: node.test.loc,

This comment has been minimized.

Copy link
@platinumazure

platinumazure Oct 20, 2019

Member

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.

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Oct 20, 2019

Author Member

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."

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Oct 20, 2019

Author Member

#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.

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Oct 21, 2019

Author Member

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?

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Oct 21, 2019

Author Member

This is done now.

@platinumazure
Copy link
Member

platinumazure commented Oct 21, 2019

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

@ilyavolodin
Copy link
Member

ilyavolodin commented Oct 22, 2019

Agree. This should be bug fix or chore.

Copy link
Member

kaicataldo left a comment

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

@mdjermanovic mdjermanovic force-pushed the nocondassign-loc branch from 6d56878 to 2349e27 Nov 4, 2019
@mdjermanovic
Copy link
Member Author

mdjermanovic commented Nov 4, 2019

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

Rebased and fixed now

Copy link
Member

kaicataldo left a comment

LGTM, thanks!

@kaicataldo kaicataldo added accepted and removed evaluating labels Nov 4, 2019
@kaicataldo
Copy link
Member

kaicataldo commented Nov 4, 2019

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?

@btmills btmills merged commit 97c745d into master Nov 22, 2019
18 checks passed
18 checks passed
Verify Files
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 8.10.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191104.1 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@btmills btmills deleted the nocondassign-loc branch Nov 22, 2019
@viceice viceice mentioned this pull request Nov 28, 2019
0 of 1 task complete
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.