-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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?
lib/rules/no-unused-vars.js
Outdated
* @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?
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 I understand what you're saying. I see node.parent
multiple times in the following lines, and there's just no need to keep doing the lookup.
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.
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
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've added some comments, but my previous comments weren't addressed: There are a lot of if
statements that aren't obvious as to what they do. Please add comments for all of them.
Also, we need to cut down on the number of times we have node.parent
and node.parent.parent
throughout. These references should be stored in variables to avoid the cost of property lookup over and over again.
lib/rules/no-unused-vars.js
Outdated
* @param {number} skips number of token to skip | ||
* @returns {number} start range of token before the identifier | ||
*/ | ||
function getBeforeToken(node, skips) { |
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 doesn't seem to describe what the function is actually doing. Maybe getPreviousTokenStart
would be a better name?
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
* @param {number} skips number of token to skip | ||
* @returns {number} end range of token after the identifier | ||
*/ | ||
function getAfterToken(node, skips) { |
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 here. Maybe getNextTokenEnd
would make more sense?
lib/rules/no-unused-vars.js
Outdated
* @param {ASTNode} node parent of identifier | ||
* @returns {boolean} true if node is function | ||
*/ | ||
function isTypeFunction(node) { |
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.
We have a utility function for this:
eslint/lib/rules/utils/ast-utils.js
Line 125 in 80747d2
function isFunction(node) { |
lib/rules/no-unused-vars.js
Outdated
* @param {ASTNode} node node to check | ||
* @returns {boolean} true if node is forOf or forIn loop | ||
*/ | ||
function isLoop(node) { |
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.
We have a utility function for this:
eslint/lib/rules/utils/ast-utils.js
Line 141 in 80747d2
function isLoop(node) { |
lib/rules/no-unused-vars.js
Outdated
* @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.
I'm not sure I understand what you're saying. I see node.parent
multiple times in the following lines, and there's just no need to keep doing the lookup.
lib/rules/no-unused-vars.js
Outdated
if (node.parent.type === "VariableDeclarator") { | ||
|
||
// skip variable in for (const [ foo ] of bar); | ||
if (isLoop(node.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.
Again, a lot of node.parent
and node.parent.parent
in the following lines. These should be pulled up into variables to avoid constant lookup.
lib/rules/no-unused-vars.js
Outdated
return null; | ||
} | ||
|
||
if (node.parent.parent.declarations.length === 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.
Please add a comment describing what this does.
lib/rules/no-unused-vars.js
Outdated
return fixer.removeRange(node.parent.parent.range); | ||
} | ||
|
||
if (getTokenBeforeValue(node.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.
Please add a comment describing what this does.
lib/rules/no-unused-vars.js
Outdated
return fixer.removeRange([node.parent.range[0], getAfterToken(node.parent)]); | ||
} | ||
|
||
if (isTypeFunction(node.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.
Please add a comment describing what this does.
Also note, this is almost exactly the same as earlier in the file. Maybe there's a way to extract into a function?
return fixer.removeRange([getBeforeToken(node), node.range[1]]); | ||
} | ||
|
||
if (getTokenBeforeValue(node) === ":") { |
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.
Please add a comment describing what this does.
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!
ci failing has been fixed in the main branch, can you please rebase it, thanks! |
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. Thanks!
Leaving open for @mdjermanovic and @fasttime to verify their comments.
Thanks for working on this @Tanujkanti4441. After running There are 99 lines never hit by a test case, all but one in the new code. I haven't looked into the detail of what the missing tests would need to check, but I think you could figure that out quickly since you are already familiar with the logic. Sometimes the relevant code is already indicated in the comments, like this one: // fix 'let { a } = foo, bar = "hello";' to 'let bar = "hello";' if 'a' is unused, same for arrays. Those should be a good start to add invalid test cases. |
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
sorry! wasn't active due to some work and health issues, working on it. |
34be242
to
4419825
Compare
@fasttime, have written most of the test cases, but not sure about how to write the test where i have to return null, do you have any suggestions? |
Thanks for the update @Tanujkanti4441, I think it's okay to leave that line uncovered if it cannot be reached in a test case. Probably a pattern like |
@Tanujkanti4441 Sorry for the delay, I will review this PR soon. |
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 is a merge conflict.
} | ||
|
||
// // remove unused declared variable with single declaration like 'var a = b;' | ||
return fixer.removeRange(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.
If the code is like:
let a;
foo(a = 1);
The rule now suggests to remove let a;
, which seems incorrect, since a
is assigned a value.
} | ||
|
||
// remove unused varible that is in a sequence [a,b] fixes to [a] | ||
if (tokenBefore.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.
if (tokenBefore.value === ",") { | |
if (tokenBefore?.value === ",") { |
tokenBefore
could be null
, here and in other parts of the fix
function.
Maybe we could add a test with code like a => a = 1;
to make sure that the rule doesn't crash (currently it does).
data: { | ||
varName: unusedVar.name | ||
}, | ||
fix(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.
Can we extract the fix
method into a function before the line context.report({
for better readability, and unindent it by 12 spaces?
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