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
feat: add suggestions to no-unused-vars
#18352
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.
It looks like the whole file has been reformatted, making it difficult to view the diff. Can you fix the formatting?
lib/rules/no-unused-vars.js
Outdated
@@ -5,6 +5,8 @@ | |||
|
|||
"use strict"; | |||
|
|||
// const { parents } = require("cheerio/lib/api/traversing"); | |||
// const { remove } = require("../linter/rule-fixer"); |
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.
Looks like your editor might have added some unintentional imports?
@eslint/eslint-team, code in this PR seems to work fine but having linting error that |
It's required to return a value in fixers (to catch possible errors). if you don't want to fix in some cases, please use |
Thanks for reply! but is this suggestion also true for the following code? return fixer.removeRange(parent.parent.range); because it actually returns a value and having error |
I think it's ready for review now. |
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.
Left some comments related to storing variables for later use.
Also, there are a lot of if
statements that aren't obvious what they're doing. Can we add some comments describing what each is accomplishing?
* @returns {Object} fixer object | ||
*/ | ||
function fixVariables(node) { | ||
if (node.parent.type === "VariableDeclarator") { |
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.
It looks like you're using node.parent
a lot, which requires looking up that property each time. I'd suggest saving a reference to both the parent and parent type so you don't have to keep evaluating it each time:
const parent = node.parent;
const parentType = parent.type;
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.
Here node.parent
is just used in fixVariable
function and parent is already declared as
const parent = id.parent
is it ok if i don't replace node.parent
for now because it will help me if i use the function later?
lib/rules/no-unused-vars.js
Outdated
return fixer.removeRange(node.range); | ||
} | ||
|
||
if (sourceCode.getTokenBefore(node).value === "(" && sourceCode.getTokenAfter(node).value === ",") { |
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.
You're also doing getTokenBefore(node)
and getTokenAfter(node)
a few times here. It's best to store those values in a variable to save the function call.
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!
lib/rules/no-unused-vars.js
Outdated
return node.elements.filter(e => e !== null).length === 1; | ||
} | ||
|
||
if (parent.type === "VariableDeclarator") { |
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.
Same comment about references throughout. We want to avoid a.b.c.d as much as possible.
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!
lib/rules/no-unused-vars.js
Outdated
@@ -98,7 +100,8 @@ module.exports = { | |||
|
|||
messages: { | |||
unusedVar: "'{{varName}}' is {{action}} but never used{{additional}}.", | |||
usedIgnoredVar: "'{{varName}}' is marked as ignored but is used{{additional}}." | |||
usedIgnoredVar: "'{{varName}}' is marked as ignored but is used{{additional}}.", | |||
removeVar: "remove unused variable '{{varName}}'." |
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.
removeVar: "remove unused variable '{{varName}}'." | |
removeVar: "Remove unused variable '{{varName}}'." |
lib/rules/no-unused-vars.js
Outdated
if (parent.parent.type === "ObjectPattern") { | ||
if (parent.parent.properties.length === 1) { | ||
|
||
// var {a} = foo; | ||
if (parent.parent.parent.type === "VariableDeclarator") { | ||
if (parent.parent.parent.parent.declarations.length === 1) { | ||
return fixer.removeRange(parent.parent.parent.parent.range); | ||
} |
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.
Patterns can also appear in for-in/for-of loops:
for (const { foo } of bar);
After the fix:
for ( of bar);
? getAssignedMessageData(unusedVar) | ||
: getDefinedMessageData(unusedVar) | ||
: getDefinedMessageData(unusedVar), | ||
suggest: [ |
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'm not sure if we should remove the declaration if there are references to that variable.
var x;
x = 1;
After the fix:
x = 1;
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 till here!
lib/rules/no-unused-vars.js
Outdated
if (getTokenBeforeValue(parent.parent) === ":") { | ||
if (parent.parent.parent.parent.type === "ObjectPattern") { | ||
if (parent.parent.parent.parent.properties.length === 1) { | ||
return fixVariables(parent.parent.parent.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.
There seems to be a lot of repeated code for different combinations of patterns in patterns. Can we maybe unify the code? For example, when we determine that something can be removed, recursively check the parent and so on until we find the uppermost node that can be removed.
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.
is this statement suggesting to use loops? may be i don't get it properly.
how about if we use function to reduce the repetition.
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.
Yes, loops or recursion. That would avoid code repetition and cover more cases.
For example, the current implementation provides a suggestion to fix const [[foo]] = bar;
because it has a branch that specifically covers the array pattern in array pattern case. But it doesn't provide a suggestion to fix const [[[foo]]] = bar;
although it's basically the same case, just one more level deep. The same for any combination of patterns, for example the current implementation provides a suggestion to fix const { a: [b] } = c;
, but doesn't provide a suggestion to fix const [{ a: [b] }] = c;
.
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.
fix for some nested patterns has implemented, working on some others, so marked it as draft.
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
added suggestion to
no-unused-vars
rule.Is there anything you'd like reviewers to focus on?
Fixes: #17545