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

curly: multi-or-nest option reports an error for a single-statment block with a semicolon on the next line #12370

Closed
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly Hacktoberfest Recommended issue for those participating in Hacktoberfest https://hacktoberfest.digitalocean.com/ help wanted The team would welcome a contribution from the community for this issue rule Relates to ESLint's core rules

Comments

@cherryblossom000
Copy link
Contributor

cherryblossom000 commented Oct 4, 2019

Tell us about your environment

  • ESLint Version: 6.5.1
  • Node Version: 12.11.0
  • npm Version: 6.11.3

What parser (default, Babel-ESLint, etc.) are you using? default

Please show your full configuration:

Configuration
{
  "rules": {
    "curly": [2, "multi-or-nest"]
  }
}

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

I used the following code in file.js:

if (true) console.log('some statement')
;(function () {})()

The issue occurs whenever the statement in the if block (or any block) has the semicolon on the following line.

Then I linted it:

eslint file.js

What did you expect to happen?
No error would occur as there is only 1 statement in the if block.

What actually happened? Please include the actual, raw output from ESLint.
I got the following error:

file.js
  1:1  error  Expected { after 'if' condition  curly

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

When fixing it with eslint --fix file.js, it gets changed to

if (true) {console.log('some statement')
;}(function () {})()

which conflicts with the "semi": [2, "never"] rule as that would change it to:

if (true) {console.log('some statement')
}(function () {})()

which would cause curly to report Unnecessary { after 'if' condition.

Are you willing to submit a pull request to fix this bug?
Yes.

@cherryblossom000 cherryblossom000 added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Oct 4, 2019
@cherryblossom000 cherryblossom000 changed the title curly: multi-or-nest option reports an error for a single-statment block with no braces before a line beginning with a semicolon curly: multi-or-nest option reports an error for a single-statment block with a semicolon on the next line Oct 4, 2019
@cherryblossom000
Copy link
Contributor Author

I think the issue is here:

eslint/lib/rules/curly.js

Lines 99 to 104 in c47fa0d

function isOneLiner(node) {
const first = sourceCode.getFirstToken(node),
last = sourceCode.getLastToken(node);
return first.loc.start.line === last.loc.end.line;
}

last would be the semicolon, which is on a different line to the first token in

console.log('some statement')
;

@kaicataldo
Copy link
Member

Verified using our demo. Nice find! Definitely seems like we should use the previous token for this check in this case.

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion rule Relates to ESLint's core rules Hacktoberfest Recommended issue for those participating in Hacktoberfest https://hacktoberfest.digitalocean.com/ help wanted The team would welcome a contribution from the community for this issue and removed triage An ESLint team member will look at this issue soon labels Oct 4, 2019
@cherryblossom000
Copy link
Contributor Author

I fixed it in #12378.

kaicataldo pushed a commit that referenced this issue Nov 1, 2019
#12378)

* Update: Curly multi-or-nest flagging semis on next line (fixes #12370)

* Chore: Add more tests to the multi-or-nest option for curly

Check that multi-or-nest it fixes cases like
if (foo) {
  doSomething()
  ;
}

and that it ignores cases with an empty statement like
if (foo)
;
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 1, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly Hacktoberfest Recommended issue for those participating in Hacktoberfest https://hacktoberfest.digitalocean.com/ help wanted The team would welcome a contribution from the community for this issue rule Relates to ESLint's core rules
Projects
None yet
2 participants