-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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: reporting variable used in catch block in no-useless-assignment
#19423
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would like @mdjermanovic to review before merging.
lib/rules/no-useless-assignment.js
Outdated
while (currentNode) { | ||
if (currentNode.type === "CatchClause") { | ||
return true; | ||
} | ||
currentNode = currentNode.parent; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This search up to the root increases the timing for this rule by around 20% when linting this project. Can we maybe store CatchClause
nodes in scopeStack
, and at some later point check whether a reference is in the range of some of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this:
CatchClause(node) {
scopeStack.catchClauses.add(node);
},
But seems it's not storing the catchClause
nodes that are unreachable is it the intended behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you push the changes with a test case that doesn't work due to this so I can take a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Tell us about your environment (
npx eslint --env-info
):What parser are you using (place an "X" next to just one item)?
[x]
Default (Espree)
[ ]
@typescript-eslint/parser
[ ]
@babel/eslint-parser
[ ]
vue-eslint-parser
[ ]
@angular-eslint/template-parser
[ ]
Other
Please show your full configuration:
Configuration
What did you do? Please include the actual source code causing the issue.\
What did you expect to happen?
intermediaryValue
won't get reported as it is used incatch
block.What actually happened? Please include the actual, raw output from ESLint.
it reports the
intermediaryValue
as unused variable.Is there anything you'd like reviewers to focus on?
fixes #19245