feat: add suggestions to `no-unused-vars` #18352

Tanujkanti4441 · 2024-04-16T17:08:00Z

Prerequisites checklist

I have read the contributing guidelines.

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

netlify · 2024-04-16T17:08:17Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`34be242`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/6659f8d1e1046f0008edcab8

nzakas

It looks like the whole file has been reformatted, making it difficult to view the diff. Can you fix the formatting?

nzakas · 2024-04-16T20:54:38Z

lib/rules/no-unused-vars.js

@@ -5,6 +5,8 @@

 "use strict";

+// const { parents } = require("cheerio/lib/api/traversing");
+// const { remove } = require("../linter/rule-fixer");


Looks like your editor might have added some unintentional imports?

Tanujkanti4441 · 2024-04-22T09:49:25Z

@eslint/eslint-team, code in this PR seems to work fine but having linting error that Method 'fix' expected no return value can i get some help in figuring out what is wrong or is there something i am not aware of?

aladdin-add · 2024-04-22T09:52:48Z

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 return null;

Tanujkanti4441 · 2024-04-23T14:40:43Z

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 return null;

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 Method 'fix' expected no return value

lib/rules/no-unused-vars.js

Tanujkanti4441 · 2024-04-26T15:17:26Z

I think it's ready for review now.

nzakas

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?

nzakas · 2024-04-26T17:37:32Z

lib/rules/no-unused-vars.js

+                                         * @returns {Object} fixer object
+                                         */
+                                        function fixVariables(node) {
+                                            if (node.parent.type === "VariableDeclarator") {


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;

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?

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.

nzakas · 2024-04-26T17:38:24Z

lib/rules/no-unused-vars.js

+                                                    return fixer.removeRange(node.range);
+                                                }
+
+                                                if (sourceCode.getTokenBefore(node).value === "(" && sourceCode.getTokenAfter(node).value === ",") {


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.

nzakas · 2024-04-26T17:39:02Z

lib/rules/no-unused-vars.js

+                                            return node.elements.filter(e => e !== null).length === 1;
+                                        }
+
+                                        if (parent.type === "VariableDeclarator") {


Same comment about references throughout. We want to avoid a.b.c.d as much as possible.

mdjermanovic · 2024-05-02T15:00:42Z

lib/rules/no-unused-vars.js

@@ -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}}'."


Suggested change

removeVar: "remove unused variable '{{varName}}'."

removeVar: "Remove unused variable '{{varName}}'."

mdjermanovic · 2024-05-02T15:24:13Z

lib/rules/no-unused-vars.js

+                                        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);
+                                                    }


Patterns can also appear in for-in/for-of loops:

for (const { foo } of bar);

After the fix:

for ( of bar);

mdjermanovic · 2024-05-02T15:29:53Z

lib/rules/no-unused-vars.js

                                ? getAssignedMessageData(unusedVar)
-                                : getDefinedMessageData(unusedVar)
+                                : getDefinedMessageData(unusedVar),
+                            suggest: [


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;

Done till here!

mdjermanovic · 2024-05-02T15:42:23Z

lib/rules/no-unused-vars.js

+                                                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 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.

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.

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;.

fix for some nested patterns has implemented, working on some others, so marked it as draft.

github-actions · 2024-05-28T22:31:32Z

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.

nzakas

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.

nzakas · 2024-05-29T14:20:37Z

lib/rules/no-unused-vars.js

+                                         * @param {number} skips number of token to skip
+                                         * @returns {number} start range of token before the identifier
+                                         */
+                                        function getBeforeToken(node, skips) {


This doesn't seem to describe what the function is actually doing. Maybe getPreviousTokenStart would be a better name?

nzakas · 2024-05-29T14:21:16Z

lib/rules/no-unused-vars.js

+                                         * @param {number} skips number of token to skip
+                                         * @returns {number} end range of token after the identifier
+                                         */
+                                        function getAfterToken(node, skips) {


Same here. Maybe getNextTokenEnd would make more sense?

nzakas · 2024-05-29T14:22:34Z

lib/rules/no-unused-vars.js

+                                         * @param {ASTNode} node parent of identifier
+                                         * @returns {boolean} true if node is function
+                                         */
+                                        function isTypeFunction(node) {


We have a utility function for this:

eslint/lib/rules/utils/ast-utils.js

Line 125 in 80747d2

function isFunction(node) {

nzakas · 2024-05-29T14:22:59Z

lib/rules/no-unused-vars.js

+                                         * @param {ASTNode} node node to check
+                                         * @returns {boolean} true if node is forOf or forIn loop
+                                         */
+                                        function isLoop(node) {


We have a utility function for this:

eslint/lib/rules/utils/ast-utils.js

Line 141 in 80747d2

function isLoop(node) {

nzakas · 2024-05-29T14:24:52Z

lib/rules/no-unused-vars.js

+                                         * @returns {Object} fixer object
+                                         */
+                                        function fixVariables(node) {
+                                            if (node.parent.type === "VariableDeclarator") {


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.

nzakas · 2024-05-29T14:27:12Z

lib/rules/no-unused-vars.js

+                                            if (node.parent.type === "VariableDeclarator") {
+
+                                                // skip variable in for (const [ foo ] of bar);
+                                                if (isLoop(node.parent.parent.parent)) {


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.

nzakas · 2024-05-29T14:27:23Z

lib/rules/no-unused-vars.js

+                                                    return null;
+                                                }
+
+                                                if (node.parent.parent.declarations.length === 1) {


Please add a comment describing what this does.

nzakas · 2024-05-29T14:27:37Z

lib/rules/no-unused-vars.js

+                                                    return fixer.removeRange(node.parent.parent.range);
+                                                }
+
+                                                if (getTokenBeforeValue(node.parent) === ",") {


Please add a comment describing what this does.

nzakas · 2024-05-29T14:28:11Z

lib/rules/no-unused-vars.js

+                                                return fixer.removeRange([node.parent.range[0], getAfterToken(node.parent)]);
+                                            }
+
+                                            if (isTypeFunction(node.parent)) {


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?

nzakas · 2024-05-29T14:28:23Z

lib/rules/no-unused-vars.js

+                                                return fixer.removeRange([getBeforeToken(node), node.range[1]]);
+                                            }
+
+                                            if (getTokenBeforeValue(node) === ":") {


Please add a comment describing what this does.

feat: add suggestions to no-unused-vars

d52c75c

Tanujkanti4441 requested a review from a team as a code owner April 16, 2024 17:08

eslint-github-bot bot added the feature This change adds a new feature to ESLint label Apr 16, 2024

github-actions bot added the rule Relates to ESLint's core rules label Apr 16, 2024

nzakas reviewed Apr 16, 2024

View reviewed changes

aladdin-add marked this pull request as draft April 17, 2024 03:40

Tanujkanti4441 added 4 commits April 17, 2024 10:08

revert formatting

c82e588

fix linting and CI errors

a3c1b14

fix linting errors

7609261

fix errors

63130df

fasttime reviewed Apr 23, 2024

View reviewed changes

lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved

lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved

Tanujkanti4441 added 2 commits April 24, 2024 20:54

fix linting errors

b65836d

add more tests

ad68c37

Tanujkanti4441 marked this pull request as ready for review April 26, 2024 15:16

nzakas reviewed Apr 26, 2024

View reviewed changes

Tanujkanti4441 added 3 commits April 28, 2024 20:57

add info comments and storing variables

dcc18f9

fix errors

758a867

update code

947d0bd

mdjermanovic reviewed May 2, 2024

View reviewed changes

mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label May 2, 2024

Tanujkanti4441 added 3 commits May 4, 2024 20:59

skip fix on referenced variables

74948f7

fix error

e45c898

add fixes to nested patterns

96db861

Tanujkanti4441 marked this pull request as draft May 16, 2024 13:14

add more fixes for nested patterns

f9b96e7

Tanujkanti4441 marked this pull request as ready for review May 18, 2024 11:06

github-actions bot added the Stale label May 28, 2024

Rec0iL99 removed the Stale label May 29, 2024

Rec0iL99 requested review from nzakas, mdjermanovic and fasttime May 29, 2024 14:17

nzakas reviewed May 29, 2024

View reviewed changes

add more info comments

34be242

	removeVar: "remove unused variable '{{varName}}'."
	removeVar: "Remove unused variable '{{varName}}'."

feat: add suggestions to no-unused-vars #18352

Are you sure you want to change the base?

feat: add suggestions to no-unused-vars #18352

Conversation

Tanujkanti4441 commented Apr 16, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

netlify bot commented Apr 16, 2024 • edited

✅ Deploy Preview for docs-eslint canceled.

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tanujkanti4441 commented Apr 22, 2024

aladdin-add commented Apr 22, 2024 • edited

Tanujkanti4441 commented Apr 23, 2024 • edited

Tanujkanti4441 commented Apr 26, 2024

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tanujkanti4441 Apr 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 28, 2024

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: add suggestions to `no-unused-vars` #18352

feat: add suggestions to `no-unused-vars` #18352

netlify bot commented Apr 16, 2024 •

edited

aladdin-add commented Apr 22, 2024 •

edited

Tanujkanti4441 commented Apr 23, 2024 •

edited

Tanujkanti4441 Apr 28, 2024 •

edited