feat: Add `reportUsedIgnorePattern` option to `no-unused-vars` rule

[Pearce-Ropion]

Prerequisites checklist

• [x] I have read the contributing guidelines.

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

• [x] 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 rule do you want to change?

no-unused-vars

What change do you want to make (place an "X" next to just one item)?

• [x] Generate more warnings
• [ ] Generate fewer warnings
• [ ] Implement autofix
• [ ] Implement suggestions

How will the change be implemented (place an "X" next to just one item)?

• [x] A new option
• [ ] A new default behavior
• [ ] Other

Please provide some example code that this change will affect:

Example 1:

/* eslint no-unused-vars: ["error", { "varsIgnorePattern": "^_" }] */

const _a = 5;
const _b = _a + 5

Example 2:

/* eslint no-unused-vars: ["error", { "argsIgnorePattern": "^_" }] */

function foo(_a) {
  return _a + 5
}

Example 3:

/* eslint no-unused-vars: ["error", { "destructuredArrayIgnorePattern": "^_" }] */

const [ a, _b ] = items;
console.log(a + _b);

Example 4:

/* eslint no-unused-vars: ["error", { "caughtErrorsIgnorePattern": "^_" }] */

try {}
catch(_err) {
  console.error(_err);
}

What does the rule currently do for this code?

In all examples, no warnings are reported.

What will the rule do after it's changed?

When the new option reportUsedIgnorePattern is enabled, each of the examples above will report; indicating that a variable that has been marked as an ignored unused variable is being used.

What changes did you make? (Give an overview)

This change adds a new option reportUsedIgnorePattern to the no-unused-vars rule. When enabled, the rule will report any instances of variables that match any of the valid ignore pattern options when that variable is actually being used.

Resolves: #17568

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

When finding all references in order to report on the correct nodes, it is also finding the variable declaration. So a question for the team would be which nodes should this option report on:

• Should all references including the declaration be reported?
• Should only the variable declaration be reported?
• Should only references not including the declaration be reported

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: Pearce-Ropion / name: Pearce Ropion (a364f2fa9c0d3ee0cb1ba8bd161a66a496bec51a, 12d57da97e916c6803fc4738c9c0937246a87177, 418a092720ae6ac80e559bbd6f0eb43d32c050ed, d97b7fd0f62b10579411b300f4f805427686d3a1, a91ea4fbdd213d39981d148a0e6ae95951cc1633, 1eefec568a8962247bb11533be89a4514a2fdccc, 9fa1956be5a998d084c52f4268f8ed41cfbb9873, 6a0b6b51d81f4f8815f6cd46ff29700687d39d6b)

[eslint-github-bot[bot]]

Hi @Pearce-Ropion!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.
• The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

[netlify[bot]]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	9fa1956be5a998d084c52f4268f8ed41cfbb9873
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/660398e1192bd30008c67bd6

[mdjermanovic]

• Should all references including the declaration be reported?
• Should only the variable declaration be reported?
• Should only references not including the declaration be reported

Would it make sense to report only on the reasons the variable is considered used, which are usually its read references?

This is how it currently works:

/* eslint no-unused-vars: ["error", {
    varsIgnorePattern: "^_",
    reportUsedIgnorePattern: true
}] */

var _a; // not reported

_a = 1; // this is reported but doesn't seem relevant?

foo(_a); // this is also reported, ok

But this variable is considered used only because of the read reference so it might make sense to report like this:

/* eslint no-unused-vars: ["error", {
    varsIgnorePattern: "^_",
    reportUsedIgnorePattern: true
}] */

var _a; // not reported

_a = 1; // not reported

foo(_a); // reported

[Pearce-Ropion]

• Should all references including the declaration be reported?
• Should only the variable declaration be reported?
• Should only references not including the declaration be reported

Would it make sense to report only on the reasons the variable is considered used, which are usually its read references?

This is how it currently works:

/* eslint no-unused-vars: ["error", {
    varsIgnorePattern: "^_",
    reportUsedIgnorePattern: true
}] */

var _a; // not reported

_a = 1; // this is reported but doesn't seem relevant?

foo(_a); // this is also reported, ok

But this variable is considered used only because of the read reference so it might make sense to report like this:

/* eslint no-unused-vars: ["error", {
    varsIgnorePattern: "^_",
    reportUsedIgnorePattern: true
}] */

var _a; // not reported

_a = 1; // not reported

foo(_a); // reported

That makes sense to me. I couldn't entirely remember what counted as used. However, I would also expect that a user shouldn't be allowed to write to an ignored unused variable since it should be unused.

[github-actions[bot]]

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.

[Pearce-Ropion]

I changed it to skip all variable initializations but otherwise report on all references. I also doesn't check whether the variable scope matches anymore so it can handle variables defined in the outer scope which are used in an inner scope.

[Pearce-Ropion]

@mdjermanovic I think this is working as expected now. Can you think of any other cases that I could test for? Otherwise, I think we are good to go.

[github-actions[bot]]

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.

[mdjermanovic]

@Pearce-Ropion apologies for the delay in reviewing this PR.

I changed it to skip all variable initializations but otherwise report on all references.

I'm not sure what the common use cases for the varsIgnorePattern option are, but if user has a code like the following where the rule doesn't report any errors:

/*eslint no-unused-vars: ["error", { varsIgnorePattern: "^_", reportUsedIgnorePattern: true }]*/

var _foo;

_foo = 5;

and then they add more code that causes the variable to be considered used, so that the rule now reports it:

/*eslint no-unused-vars: ["error", { varsIgnorePattern: "^_", reportUsedIgnorePattern: true }]*/

var _foo;

_foo = 5;

bar(_foo);

then, I think it makes the most sense to report only on the references where this rule considers the variable to be used: _foo in bar(_foo), because _foo = 5 has no effect on whether the variable is considered used, and it was valid before bar(_foo) was added.

Alternatively, we could report only the declaration assuming that the variable name is wrong because it was intended to be used (as opposed to reporting references, in which case the assumption is that the variable that wasn't intended to be used is mistakenly used).

Curious what others think about this.

[Pearce-Ropion]

No worries, eslint is a massive project and I don't want to burden anyone.

then, I think it makes the most sense to report only on the references where this rule considers the variable to be used: _foo in bar(_foo), because _foo = 5 has no effect on whether the variable is considered used, and it was valid before bar(_foo) was added.

Alternatively, we could report only the declaration assuming that the variable name is wrong because it was intended to be used (as opposed to reporting references, in which case the assumption is that the variable that wasn't intended to be used is mistakenly used).

Using vscode's eslint extension as an example, on one hand it would be useful to see reported issues on the usages since that's what is considered invalid according to the new option. However, It would be more clear what the problem is and how to fix it at the declaration, so it'd probably be better to do it there.

[nzakas]

then, I think it makes the most sense to report only on the references where this rule considers the variable to be used: _foo in bar(_foo), because _foo = 5 has no effect on whether the variable is considered used, and it was valid before bar(_foo) was added.

I think this makes the most sense. :+1:

@Pearce-Ropion sorry for the delay, we've been busy working on v9. Can you please rebase and make any further changes? Thanks!

[nzakas]

Oops, accidentally hit "close"

[Pearce-Ropion]

Oops, accidentally hit "close"

lol, I was going to reopen once I pushed the changes.

[github-actions[bot]]

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.

[Pearce-Ropion]

This is still active. Waiting for a review.

[mdjermanovic]

In this case, the message seems misleading:

/*eslint no-unused-vars: ["error", {
    "reportUsedIgnorePattern": true,
    "destructuredArrayIgnorePattern": "^_",
    "varsIgnorePattern": "[iI]gnored"
}]*/

let _x; // '_x' is marked as ignored but is used. Used vars must not match /[iI]gnored/u.

[_x] = arr;

foo(_x);

[Tanujkanti4441]

Hi @Pearce-Ropion, some changes are requested. you can check out.

[mdjermanovic]

In this case, the message seems misleading:

/*eslint no-unused-vars: ["error", {
    "reportUsedIgnorePattern": true,
    "destructuredArrayIgnorePattern": "^_",
    "varsIgnorePattern": "[iI]gnored"
}]*/

let _x; // '_x' is marked as ignored but is used. Used vars must not match /[iI]gnored/u.

[_x] = arr;

foo(_x);

I'm still getting the same error, looks like this case still hasn't been addressed?

[mdjermanovic]

Here's a similar case that should also be fixed:

/*eslint no-unused-vars: ["error", {
    "reportUsedIgnorePattern": true,
    "destructuredArrayIgnorePattern": "^_",
    "varsIgnorePattern": "[iI]gnored"
}]*/

const [ignored] = arr; // 'ignored' is marked as ignored but is used. Used elements of array destructuring must not match /^_/u.

foo(ignored);

The message should be: 'ignored' is marked as ignored but is used. Used vars must not match /[iI]gnored/u.

[nzakas]

@Pearce-Ropion are you still working on this?

[Pearce-Ropion]

@Pearce-Ropion are you still working on this?

Yes, apologies. I've been slammed with work. I'll get the proposed suggestions in this weekend

[Pearce-Ropion]

Ok, I've applied @mdjermanovic's suggestions to report as soon as we detect the used ignored variable. I also explicitly added the test cases that @mdjermanovic used.

I've also updated the message data handling. I had already updated the other message data handlers as per previous comments, but I noticed that all 3 of the message data getters were doing basically the same thing. So I've added a getVariableDescription helper function to get the required info for the message data base on the type of the variable that is currently being reported.