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

eslint --fix with dot-location breaks code #11868

Closed
nightpool opened this issue Jun 21, 2019 · 4 comments

Comments

@nightpool
Copy link

commented Jun 21, 2019

  • ESLint Version: 5.16.0
  • Node Version: 8.1.4
  • npm Version: 6.7.0

ESLint playground link

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

/*eslint dot-location: ["error", "object"]*/

var foo = (
  cond1 == true &&
  cond2func()
).toString();
eslint --fix

What did you expect to happen?

No change to file

What actually happened? Please include the actual, raw output from ESLint.

/*eslint dot-location: ["error", "object"]*/

var foo = (
  cond1 == true &&
  cond2func().
)toString();

As you can see, this code doesn't even parse. eslint --fix should never break correctness and certainly shouldn't ever output something that fails to parse.

@nightpool

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

This is also an issue on 5.15.3

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Hi @nightpool, thanks for the issue.

My guess is we're not adequately taking the parentheses into account. The ESTree spec defines the object node as ending just after cond2func() in your example. If the lint rule places the dot "after the object node", then it won't work correctly. Instead, it should place it "after the previous token before the dot", which should account for the parentheses.

I would bet a similar bug exists if the property is surrounded by parentheses and the lint rule is configured to attach the dot to the property. It may place the dot "before the property node" rather than "before the next token after the dot".

If I'm correct, whoever works on a fix for this should make sure they don't accidentally pick up a comment token in the updated fixer logic.

@kaicataldo kaicataldo added evaluating rule and removed triage labels Jun 21, 2019

@mdjermanovic

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

I'd like to work on this, of course if the issue is going to be accepted (guess it will, because fix produces syntax error).

My guess is we're not adequately taking the parentheses into account. The ESTree spec defines the object node as ending just after cond2func() in your example. If the lint rule places the dot "after the object node", then it won't work correctly. Instead, it should place it "after the previous token before the dot", which should account for the parentheses.

This part of the fix works exactly as you described, and should be corrected to work exactly as you said.

I would bet a similar bug exists if the property is surrounded by parentheses and the lint rule is configured to attach the dot to the property. It may place the dot "before the property node" rather than "before the next token after the dot".

It takes the first token before the property, then safely checks if it is really a dot punctuator, and gives up if it isn't. So there would be no reports and no fixes. Regardless of the options, the node is completely ignored if the first non-comment token before the property node is not the dot.

This is actually very helpful at the moment, given the fact that the code never checks the computed flag (I believe this check should be added before anything else).

Apart from the !computed, perhaps it would be better to leave this part as is, at least for now? I don't think there is a single possible case of a non-comment token between the dot and the property in current js (parentheses are not a problem, a.(b) is not valid syntax), proposals (e.g. #x would be all in the property, ?. is on the left side and it would be a new expression type anyway), typescript or any code that uses babel-eslint. And if really something unexpected comes between, the node will be simply ignored (and perhaps it should be).

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I've reproduced the issue on the ESLint demo, so I'm marking this as accepted. Thanks @nightpool for the bug report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.