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

Vscode and atom highlight locations are unexpected when end location is not provided #12334

Closed
golopot opened this issue Sep 29, 2019 · 28 comments
Closed

Comments

@golopot
Copy link
Contributor

@golopot golopot commented Sep 29, 2019

Summary
Many rules reports only start location and no end location. For these rules, editors often renders tildes in unexpected places.

Examples
In these examples, the rules only reports start location but no end locations. Instead of highlighting tokens like \n, (, or {, the token before the said token is highlighted.

/* eslint semi: [2, 'always'] */
const a = Math.random
//             ~~~~~~    actual highlight in vscode
//                   ~   expected highlight
//                   ^   reported start location


/* eslint object-curly-spacing: [2, 'always'] */
const b = {b: Math.random}
//                 ~~~~~~    actual highlight in vscode
//                       ~   expected highlight
//                       ^   reported start location


/* eslint space-in-parens: [2, 'never']  */
function ffff( a ) {}
//       ~~~~    actual highlight in vscode
//           ~   expected highlight
//           ^   reported start location

There are many other rules suffering from this problem, most of them are named *space* or *spacing*.

How to fix
The problem disappear if the both start location and end location are reported and start location is different to end location. That is, the location range is not zero-width-ed.

There are several places this can be done:

  1. Modify each rules to report end location. Pros: explicit. Cons: lots of place needs change.

  2. Modify ESLint core, automatically add end location when end location is missing. Cons: not explicit.

  3. Modify vscode-eslint plugin to automatically add end location when end location is missing.

@g-plane
Copy link
Member

@g-plane g-plane commented Sep 29, 2019

Maybe we can modify rules, because this can benefit formatters as well. However, I'm not sure how formatters behave when reporting those rules.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Sep 29, 2019

I think one of the tricky things with some rules like semi is that there might not be a character to highlight if it's at the end of the line. Example from above:

/* eslint semi: [2, 'always'] */
const a = Math.random
//                   ^ If there's no whitespace after the last character (and many editors can be configured
//                     to do this automatically), will VS Code be able to highlight it?

@golopot
Copy link
Contributor Author

@golopot golopot commented Sep 29, 2019

I have tested that case, report the location of the linefeed (with the end location at the next line) will do well in vscode.

@eslint-deprecated
Copy link
Contributor

@eslint-deprecated eslint-deprecated bot commented Nov 3, 2019

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Nov 4, 2019

Reopening this because this is actively being worked on.

@kaicataldo kaicataldo reopened this Nov 4, 2019
@kaicataldo kaicataldo self-assigned this Nov 4, 2019
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Nov 4, 2019

Leaving the "evaluating" label on here because we haven't officially accepted this, but have been evaluating and accepting smaller individual PRs that solve this.

@aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Nov 13, 2019

@eslint/eslint-team, it seems an useful enhancement, can we get one more 👍 to accept this issue?

@btmills
Copy link
Member

@btmills btmills commented Nov 14, 2019

Is the decision here to accept this issue as blanket acceptance for PRs to individual rules that add end locations to reports (option 1 in the original issue description) so we don't have to evaluate them individually? I've added my 👍 under that understanding, and if that is indeed the case, then this can be marked as accepted.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Nov 14, 2019

@btmills I think that makes the most sense? Ideally, it would be nice to audit the rules and make a list we can keep track of here.

@golopot
Copy link
Contributor Author

@golopot golopot commented Mar 16, 2020

... Ideally, it would be nice to audit the rules and make a list we can keep track of here.

This is a list of rules that reports start column but misses end column. The list is acquired by patching rule tester in a dirty way.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Mar 16, 2020

I will champion this. Given @btmills's comment here, I'm going to mark this as accepted. @golopot Thanks for spearheading this!

nzakas pushed a commit that referenced this issue Jul 7, 2020
nzakas pushed a commit that referenced this issue Jul 7, 2020
nzakas pushed a commit that referenced this issue Jul 7, 2020
nzakas pushed a commit that referenced this issue Jul 7, 2020
mdjermanovic added a commit that referenced this issue Jul 11, 2020
@fisker
Copy link
Contributor

@fisker fisker commented Sep 11, 2020

Any interest to implement a visualize tester like this to test locations? Or even better add to RuleTester?

@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Sep 11, 2020

Do you mean snapshot testing ?

@fisker
Copy link
Contributor

@fisker fisker commented Sep 14, 2020

I mean this code frame

    `␊
      1 | function foo() {␊
    > 2 | 	function bar() {}␊
        | 	^^^^^^^^^^^^ Move function 'bar' to the outer scope.␊
      3 | }␊
    `

Easier to test.

@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Sep 14, 2020

I meant, how would we assert that ? I can think of snapshot testing only other than any hacky approach.
Or you meant to do the assertion like it is currently setup and just visually display the frame ?

@fisker
Copy link
Contributor

@fisker fisker commented Sep 14, 2020

Yes, snapshot should be the easiest way.

@nzakas
Copy link
Member

@nzakas nzakas commented Sep 15, 2020

@fisker I’d suggest opening a separate issue to discuss your idea.

@snitin315
Copy link
Contributor

@snitin315 snitin315 commented Jul 2, 2021

Taking this up.

@snitin315 snitin315 assigned snitin315 and unassigned kaicataldo Jul 2, 2021
snitin315 added a commit that referenced this issue Jul 14, 2021
btmills pushed a commit that referenced this issue Jul 17, 2021
… (#14798)

* Update: add end location to report in `consistent-return`

* Update: report end location for `function` keyword
mdjermanovic pushed a commit that referenced this issue Jul 25, 2021
…4809)

* Update: add end location to report in `unicode-bom`

* Update: report start location only
snitin315 added a commit that referenced this issue Jul 27, 2021
bmish added a commit to bmish/eslint that referenced this issue Jul 27, 2021
* master:
  Chore: Adopt `eslint-plugin/require-meta-docs-url` rule internally (eslint#14823)
  Docs: New syntax issue template (eslint#14826)
  Chore: assertions on reporting loc in `unicode-bom` (refs eslint#12334) (eslint#14809)
  Docs: fix multiple broken links (eslint#14833)
  Chore: use `actions/setup-node@v2` (eslint#14816)
  Docs: Update README team and sponsors
  7.31.0
  Build: changelog update for 7.31.0
  Upgrade: @eslint/eslintrc to v0.4.3 (eslint#14808)
  Update: add end location to report in `consistent-return` (refs eslint#12334) (eslint#14798)
  Docs: update BUG_REPORT template (eslint#14787)
  Docs: provide more context to no-eq-null (eslint#14801)
btmills pushed a commit that referenced this issue Jul 30, 2021
)

* Update: change reporting location for `curly` rule

* Chore: refactor code

* Chore: refactor code

* Update: report body/consequent/alternate node

* Chore: update comment

* Chore: refactor code
mdjermanovic pushed a commit that referenced this issue Aug 21, 2021
…14840)

* Chore: assertions on reporting loc in `eol-last` (refs #12334)

* Update: improve end report location for `never` option in eol-last
@snitin315
Copy link
Contributor

@snitin315 snitin315 commented Aug 23, 2021

This is all done 🎉 .

@snitin315 snitin315 closed this Aug 23, 2021
@nzakas
Copy link
Member

@nzakas nzakas commented Aug 24, 2021

Awesome! Thanks to everyone who helped push through this. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants