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

[array-bracket-newline] doesn't handle inline comments properly #12416

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 rule Relates to ESLint's core rules

Comments

@fabiospampinato
Copy link

Tell us about your environment

  • ESLint Version: 6.5.1
  • Node Version: 10.15.0
  • npm Version: 6.4.1

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

@typescript-eslint/parser

Please show your full configuration:

Here it is, just uncomment the "array-bracket-newline" rule.

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

const asd = [ // comment
  123,
  123
];
npx eslint src --ext '.ts, .tsx'

What did you expect to happen?

No error detected.

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

Adding an inline comment like that trips this rule, the array is actually consistently written as far as I'm concerned but the rule is kind of considering the inline comment as an item of the array.

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

I'm not too familiar with the AST, but with some pointers perhaps I can help.

@fabiospampinato fabiospampinato added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Oct 13, 2019
@mdjermanovic
Copy link
Member

This is related to the consistent option only.

Online Demo Link

/*eslint array-bracket-newline: ["error", "consistent"]*/

const asd = [ // comment
  123,
  123
];
3:13 - There should be no linebreak after '['. (array-bracket-newline)
6:1 - There should be no linebreak before ']'. (array-bracket-newline)

This looks like a bug to me. The above example is auto-fixed to:

/*eslint array-bracket-newline: ["error", "consistent"]*/

const asd = [ // comment
  123,
  123];

where the rule still reports the first error. This could be manually fixed to not report the error like this:

/*eslint array-bracket-newline: ["error", "consistent"]*/

const asd = [123, // comment
  123];

or like this:

/*eslint array-bracket-newline: ["error", "consistent"]*/

const asd = [ 
  // comment
  123,
  123
];

I don't think that the rule intends to enforce neither of those fixes, given how the other options work. In particular, always doesn't report an error for the same example:

/*eslint array-bracket-newline: ["error", "always"]*/

const asd = [ // comment
  123,
  123
];

It seems inconsistent that always sees the same example as two correct linebreaks after/before the brackets, while consistent doesn't.

The code for the consistent option checks if there is a linebreak between [ and the first token inside (including comments). Based on the result, it requires/disallows linebreaks between brackets and other tokens (not including comments).

Couldn't find anything specific about this in #9136 and #9206 where the option is implemented and there are no test cases with the consistent option and some comments.

I think that this should be accepted as a bug.

Bug fix would be to check for a linebreak between the [ and the first non-comment token. I'm not 100% sure but it seems that this fix even can't produce more errors in other cases.

@mdjermanovic mdjermanovic added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Oct 14, 2019
@fabiospampinato
Copy link
Author

It seems inconsistent that always sees the same example as two correct linebreaks after/before the brackets, while consistent doesn't.

Yeah I didn't even know that. Btw this might be a good idea for discovering new bugs, if something that passes an "always" or "never" rule doesn't pass a "consistent" rule there's probably an issue.

@mdjermanovic
Copy link
Member

The proposed fix is to change the logic that applies only when the option is consistent.

Instead of checking for a linebreak between [ and the first following token (incl. comments), the fixed code would check for a linebreak between [ and the first non-comment token.

I can't make an example where this change would produce new (potentially unwanted) warnings on an array that doesn't have any warnings at the moment. It seems impossible.

So, this looks like a bug and the fix looks safe. But, I'll leave this open for some time before accepting, maybe other team members have a different opinion.

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 15, 2019
@kaicataldo
Copy link
Member

Agreed that this seems like a bug.

@mdjermanovic
Copy link
Member

I'm working on this.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 19, 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 Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.