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 reportUsedIgnorePattern
option to no-unused-vars
rule
#17662
Conversation
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.
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 |
✅ Deploy Preview for docs-eslint canceled.
|
reportUsedIgnorePattern
option to no-unused-vars
rulereportUsedIgnorePattern
option to no-unused-vars
rule
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. |
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. |
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. |
@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. |
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 apologies for the delay in reviewing this PR.
I'm not sure what the common use cases for the /*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: 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. |
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); |
Hi @Pearce-Ropion, some changes are requested. you can check out. |
1ae9983
to
1218fdc
Compare
lib/rules/no-unused-vars.js
Outdated
@@ -92,7 +103,8 @@ module.exports = { | |||
vars: "all", | |||
args: "after-used", | |||
ignoreRestSiblings: false, | |||
caughtErrors: "all" | |||
caughtErrors: "all", | |||
reportUsedIgnorePattern: false, |
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.
reportUsedIgnorePattern: false, | |
reportUsedIgnorePattern: false |
To fix the lint error.
I'm still getting the same error, looks like this case still hasn't been addressed? |
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: |
lib/rules/no-unused-vars.js
Outdated
if (config.reportUsedIgnorePattern && isUsedVariable(variable)) { | ||
usedIgnoredVars.push(variable); | ||
} |
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.
Instead of collecting these vars and later figuring out why they were considered as used ignored, we could just report them right away with the data we have at this point:
if (config.reportUsedIgnorePattern && isUsedVariable(variable)) { | |
usedIgnoredVars.push(variable); | |
} | |
if (config.reportUsedIgnorePattern && isUsedVariable(variable)) { | |
context.report({ | |
node: def.name, | |
messageId: "usedIgnoredVar", | |
data: { | |
varName: variable.name, | |
additional: `. Used elements of array destructuring must not match ${config.destructuredArrayIgnorePattern}` | |
} | |
}); | |
} |
(the same for catch arguments, params and variables).
That would fix #17662 (comment) and #17662 (comment).
@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 |
Address comments Fix tests Report on all variables except for initializers Report only on declarations Match message formatting with other rule messages Move variable.eslintUsed into isUsedVariable function Refactor report messages for consistency Fix tests
1218fdc
to
1eefec5
Compare
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 |
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.
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 for contributing!
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
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)?
How will the change be implemented (place an "X" next to just one item)?
Please provide some example code that this change will affect:
Example 1:
Example 2:
Example 3:
Example 4:
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 theno-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: