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

Fix: no-sequences is reporting incorrect locations #12241

Merged
merged 2 commits into from Sep 14, 2019
Merged

Conversation

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 8, 2019

What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix

Tell us about your environment

  • ESLint Version: 6.3.0
  • Node Version: 10.16.0
  • npm Version: 6.9.0

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

default

Please show your full configuration:

Configuration
module.exports = {
  parserOptions: {
    ecmaVersion: 2015,
  }
};

What did you do? Please include the actual source code causing the issue.

Demo link

/* eslint no-sequences:error */

(1), 2;
((1)) , (2);
while((1) , 2);

What did you expect to happen?

3 errors with correct locations of the comma operator.

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

It's reporting the location of the first closing paren after the first expression in the sequence.

  3:3  error  Unexpected use of comma operator  no-sequences
  4:4  error  Unexpected use of comma operator  no-sequences
  5:9  error  Unexpected use of comma operator  no-sequences

What changes did you make? (Give an overview)

Find the first comma token instead of just first token (which could be a closing paren token).

Additionally, the rule will now report comma operator's loc instead of just loc.start.

Is there anything you'd like reviewers to focus on?

I have a question. This rule, and most of the rules that report single-char tokens are reporting locations as token.loc.start instead of the full token.loc (which has the start and the end).

Why is that? This has a strange effects in editors, e.g. Demo does not highlight anything (at least in my browser), VSCode usually highlights the first character before the token (if it isn't an empty space) instead of the token character.

@eslint eslint bot added the triage label Sep 8, 2019
@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Sep 8, 2019

Why is that?

I would guess historical reasons more than anything else. We didn't always support reporting of end locations, so some of these rules' logic could simply have been written before we supported that. I think we should always require ranges of length 1 or higher personally, but that would be a core breaking change.

For this case specifically, I would be in support of highlighting the comma (with start/end), or the whole sequence.

@mdjermanovic

This comment has been minimized.

Copy link
Member Author

mdjermanovic commented Sep 8, 2019

It's changed now to report firstCommaToken.loc instead of just firstCommaToken.loc.start.

@mdjermanovic mdjermanovic added accepted and removed triage labels Sep 8, 2019
@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Sep 13, 2019

I have a question. This rule, and most of the rules that report single-char tokens are reporting locations as token.loc.start instead of the full token.loc (which has the start and the end).

It's a historical reason. After I added that feature, I had planned to update core rules (#3307 (comment)), but I had failed to make time.

Copy link
Member

mysticatea left a comment

LGTM, thank you!

@ilyavolodin ilyavolodin merged commit ae17d1c into master Sep 14, 2019
16 checks passed
16 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
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 #20190908.15 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
@ilyavolodin ilyavolodin deleted the nosequences-location branch Sep 14, 2019
@eslint eslint bot locked and limited conversation to collaborators Mar 13, 2020
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

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