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

no-unused-vars highlights at wrong position #14324

Closed
M393 opened this issue Apr 15, 2021 · 6 comments · Fixed by #14335
Closed

no-unused-vars highlights at wrong position #14324

M393 opened this issue Apr 15, 2021 · 6 comments · Fixed by #14335
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 repro:yes
Projects

Comments

@M393
Copy link

M393 commented Apr 15, 2021

With no-unused-vars enabled eslint highlights the last use of the variable. If it is used left and right of an assignment it should mark the one on the left.

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

Instead of this:

let x = [];
x = x.concat(x);
             ^

What did you expect to happen?
It should be this:

let x = [];
x = x.concat(x);
^

What actually happened? Please copy-paste the actual, raw output from ESLint.

Steps to reproduce this issue:

  1. eslint demo

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

@M393 M393 added bug ESLint is working incorrectly repro:needed labels Apr 15, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Apr 15, 2021
@nzakas
Copy link
Member

nzakas commented Apr 16, 2021

Confirmed. We are probably just highlighting the last reference.

@nzakas nzakas moved this from Needs Triage to Ready to Implement in Triage Apr 16, 2021
@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Apr 16, 2021
@snitin315
Copy link
Contributor

@nzakas If there are no objections I will work on a fix for this one.

@aladdin-add
Copy link
Member

@snitin315 please feel free to open a PR! 😄

@snitin315
Copy link
Contributor

I see here https://github.com/eslint/eslint/blob/master/lib/rules/no-unused-vars.js#L628-L630 that it reports the last reference whenever there are multiple references.

How should we check if we want to report the last one or any other one?

@aladdin-add
Copy link
Member

I'd prefer report the var where it was defined. But let's see what the others' thoughts. :)

@nzakas
Copy link
Member

nzakas commented Apr 17, 2021

@aladdin-add we chose to highlight the last reference because that’s where the most likely error is. It’s easy to find the declaration but that last reference might not be obvious.

@snitin315 basically, you want the last write reference. You can tell by using ref.isWrite().

@mdjermanovic mdjermanovic moved this from Ready to Implement to Pull Request Opened in Triage Apr 22, 2021
Triage automation moved this from Pull Request Opened to Complete Apr 23, 2021
mdjermanovic pushed a commit that referenced this issue Apr 23, 2021
…) (#14335)

* Fix: highlight last write reference for no-unused-vars (fixes #14324)

* test: updates

* Chore: add test case

* Fix: apply suggestions

* Chore: update tests

* Chore: more tests
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Oct 21, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Oct 21, 2021
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 repro:yes
Projects
No open projects
Triage
Complete
Development

Successfully merging a pull request may close this issue.

4 participants