eslint --fix with dot-location breaks code

[nightpool]

• 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 is also an issue on 5.15.3

[platinumazure]

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.

[mdjermanovic]

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]

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