From a364f2fa9c0d3ee0cb1ba8bd161a66a496bec51a Mon Sep 17 00:00:00 2001 From: Pearce Date: Wed, 18 Oct 2023 10:43:42 -0700 Subject: [PATCH] Add `reportUsedIgnorePattern` option to `no-unused-vars` rule 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 --- docs/src/rules/no-unused-vars.md | 43 ++++++- lib/rules/no-unused-vars.js | 200 +++++++++++++++++++++++++----- tests/lib/rules/no-unused-vars.js | 74 ++++++++++- 3 files changed, 277 insertions(+), 40 deletions(-) diff --git a/docs/src/rules/no-unused-vars.md b/docs/src/rules/no-unused-vars.md index cc361e955e4..07b2c0a2cc7 100644 --- a/docs/src/rules/no-unused-vars.md +++ b/docs/src/rules/no-unused-vars.md @@ -137,7 +137,13 @@ By default this rule is enabled with `all` option for caught errors and variable ```json { "rules": { - "no-unused-vars": ["error", { "vars": "all", "args": "after-used", "caughtErrors": "all", "ignoreRestSiblings": false }] + "no-unused-vars": ["error", { + "vars": "all", + "args": "after-used", + "caughtErrors": "all", + "ignoreRestSiblings": false, + "reportUsedIgnorePattern": false + }] } } ``` @@ -435,8 +441,6 @@ class Bar { } ``` -::: - Examples of **correct** code for the `{ "ignoreClassWithStaticInitBlock": true }` option ::: correct @@ -453,6 +457,39 @@ class Foo { } ``` +### reportUsedIgnorePattern + +The `reportUsedIgnorePattern` option is a boolean (default: `false`). +Using this option will report variables that match any of the valid ignore +pattern options (`varsIgnorePattern`, `argsIgnorePattern`, `caughtErrorsIgnorePattern`, or +`destructuredArrayIgnorePattern`) if they have been used. + +Examples of **incorrect** code for the `{ "reportUsedIgnorePattern": true }` option: + +::: incorrect + +```js +/*eslint no-unused-vars: ["error", { "reportUsedIgnorePattern": true, "varsIgnorePattern": "[iI]gnored" }]*/ + +var firstVarIgnored = 1; +var secondVar = 2; +console.log(firstVarIgnored, secondVar); +``` + +::: + +Examples of **correct** code for the `{ "reportUsedIgnorePattern": true }` option: + +::: correct + +```js +/*eslint no-unused-vars: ["error", { "reportUsedIgnorePattern": true, "varsIgnorePattern": "[iI]gnored" }]*/ + +var firstVar = 1; +var secondVar = 2; +console.log(firstVar, secondVar); +``` + ::: ## When Not To Use It diff --git a/lib/rules/no-unused-vars.js b/lib/rules/no-unused-vars.js index 90b76e6f2d1..d0b0f613aeb 100644 --- a/lib/rules/no-unused-vars.js +++ b/lib/rules/no-unused-vars.js @@ -23,6 +23,13 @@ const astUtils = require("./utils/ast-utils"); * @property {string} additional Any additional info to be appended at the end. */ +/** + * Bag of data used for formatting the `usedIgnoredVar` lint message. + * @typedef {Object} UsedIgnoredVarMessageData + * @property {string} varName The name of the unused var. + * @property {string} additional Any additional info to be appended at the end. + */ + //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -73,6 +80,9 @@ module.exports = { }, ignoreClassWithStaticInitBlock: { type: "boolean" + }, + reportUsedIgnorePattern: { + type: "boolean" } }, additionalProperties: false @@ -82,7 +92,8 @@ module.exports = { ], messages: { - unusedVar: "'{{varName}}' is {{action}} but never used{{additional}}." + unusedVar: "'{{varName}}' is {{action}} but never used{{additional}}.", + usedIgnoredVar: "'{{varName}}' is marked as ignored but is used{{additional}}." } }, @@ -96,7 +107,8 @@ module.exports = { args: "after-used", ignoreRestSiblings: false, caughtErrors: "all", - ignoreClassWithStaticInitBlock: false + ignoreClassWithStaticInitBlock: false, + reportUsedIgnorePattern: false }; const firstOption = context.options[0]; @@ -110,6 +122,7 @@ module.exports = { config.ignoreRestSiblings = firstOption.ignoreRestSiblings || config.ignoreRestSiblings; config.caughtErrors = firstOption.caughtErrors || config.caughtErrors; config.ignoreClassWithStaticInitBlock = firstOption.ignoreClassWithStaticInitBlock || config.ignoreClassWithStaticInitBlock; + config.reportUsedIgnorePattern = firstOption.reportUsedIgnorePattern || config.reportUsedIgnorePattern; if (firstOption.varsIgnorePattern) { config.varsIgnorePattern = new RegExp(firstOption.varsIgnorePattern, "u"); @@ -136,22 +149,40 @@ module.exports = { * @returns {UnusedVarMessageData} The message data to be used with this unused variable. */ function getDefinedMessageData(unusedVar) { - const defType = unusedVar.defs && unusedVar.defs[0] && unusedVar.defs[0].type; - let type; - let pattern; - - if (defType === "CatchClause" && config.caughtErrorsIgnorePattern) { - type = "args"; - pattern = config.caughtErrorsIgnorePattern.toString(); - } else if (defType === "Parameter" && config.argsIgnorePattern) { - type = "args"; - pattern = config.argsIgnorePattern.toString(); - } else if (defType !== "Parameter" && defType !== "CatchClause" && config.varsIgnorePattern) { - type = "vars"; - pattern = config.varsIgnorePattern.toString(); - } + const def = unusedVar.defs && unusedVar.defs[0]; + let additional = ""; + + if (def) { + let type; + let pattern; + + switch (def.type) { + case "CatchClause": + if (config.caughtErrorsIgnorePattern) { + type = "args"; + pattern = config.caughtErrorsIgnorePattern; + } + break; + + case "Parameter": + if (config.argsIgnorePattern) { + type = "args"; + pattern = config.argsIgnorePattern; + } + break; + + default: + if (config.varsIgnorePattern) { + type = "vars"; + pattern = config.varsIgnorePattern; + } + break; + } - const additional = type ? `. Allowed unused ${type} must match ${pattern}` : ""; + if (type) { + additional = `. Allowed unused ${type} must match ${pattern.toString()}`; + } + } return { varName: unusedVar.name, @@ -167,13 +198,24 @@ module.exports = { * @returns {UnusedVarMessageData} The message data to be used with this unused variable. */ function getAssignedMessageData(unusedVar) { - const def = unusedVar.defs[0]; + const def = unusedVar.defs && unusedVar.defs[0]; let additional = ""; - if (config.destructuredArrayIgnorePattern && def && def.name.parent.type === "ArrayPattern") { - additional = `. Allowed unused elements of array destructuring patterns must match ${config.destructuredArrayIgnorePattern.toString()}`; - } else if (config.varsIgnorePattern) { - additional = `. Allowed unused vars must match ${config.varsIgnorePattern.toString()}`; + if (def) { + let type; + let pattern; + + if (def.name.parent.type === "ArrayPattern" && config.destructuredArrayIgnorePattern) { + type = "elements of array destructuring"; + pattern = config.destructuredArrayIgnorePattern; + } else if (config.varsIgnorePattern) { + type = "vars"; + pattern = config.varsIgnorePattern; + } + + if (type) { + additional = `. Allowed unused ${type} must match ${pattern.toString()}`; + } } return { @@ -183,6 +225,58 @@ module.exports = { }; } + /** + * Generate the warning message about a variable being used even though + * it is marked as being ignored. + * @param {Variable} usedIgnoredVar eslint-scope variable object + * @returns {UsedIgnoredVarMessageData} The message data to be used with + * this used ignored variable. + */ + function getUsedIgnoredMessageData(usedIgnoredVar) { + const def = usedIgnoredVar.defs && usedIgnoredVar.defs[0]; + let additional = ""; + + if (def) { + let type; + let pattern; + + switch (def.type) { + case "CatchClause": + if (config.caughtErrorsIgnorePattern) { + type = "args"; + pattern = config.caughtErrorsIgnorePattern; + } + break; + + case "Parameter": + if (config.argsIgnorePattern) { + type = "args"; + pattern = config.argsIgnorePattern; + } + break; + + default: + if (def.name.parent.type === "ArrayPattern" && config.destructuredArrayIgnorePattern) { + type = "elements of array destructuring"; + pattern = config.destructuredArrayIgnorePattern; + } else if (config.varsIgnorePattern) { + type = "vars"; + pattern = config.varsIgnorePattern; + } + break; + } + + if (type) { + additional = `. Used ${type} must not match ${pattern.toString()}`; + } + } + + return { + varName: usedIgnoredVar.name, + additional + }; + } + //-------------------------------------------------------------------------- // Helpers //-------------------------------------------------------------------------- @@ -532,8 +626,13 @@ module.exports = { * @private */ function isUsedVariable(variable) { - const functionNodes = getFunctionDefinitions(variable), - isFunctionDefinition = functionNodes.length > 0; + if (variable.eslintUsed) { + return true; + } + + const functionNodes = getFunctionDefinitions(variable); + const isFunctionDefinition = functionNodes.length > 0; + let rhsNode = null; return variable.references.some(ref => { @@ -571,11 +670,14 @@ module.exports = { /** * Gets an array of variables without read references. * @param {Scope} scope an eslint-scope Scope object. - * @param {Variable[]} unusedVars an array that saving result. - * @returns {Variable[]} unused variables of the scope and descendant scopes. + * @param {Variable[]} unusedVars an array containing unused variables that should be reported + * @param {Variable[]} usedIgnoredVars an array containing used variables that match an ignore pattern + * @returns {[Variable[], Variable[]]} an array containing + * 1. unused variables of the scope and descendant scopes. + * 2. used variables that match an ignore pattern of the scope and descendant scope * @private */ - function collectUnusedVariables(scope, unusedVars) { + function collectUnusedVariables(scope, unusedVars, usedIgnoredVars) { const variables = scope.variables; const childScopes = scope.childScopes; let i, l; @@ -589,8 +691,13 @@ module.exports = { continue; } - // skip function expression names and variables marked with markVariableAsUsed() - if (scope.functionExpressionScope || variable.eslintUsed) { + // skip function expression names + if (scope.functionExpressionScope) { + continue; + } + + // skip variables marked with markVariableAsUsed() + if (!config.reportUsedIgnorePattern && variable.eslintUsed) { continue; } @@ -615,6 +722,10 @@ module.exports = { config.destructuredArrayIgnorePattern && config.destructuredArrayIgnorePattern.test(def.name.name) ) { + if (config.reportUsedIgnorePattern && isUsedVariable(variable)) { + usedIgnoredVars.push(variable); + } + continue; } @@ -634,6 +745,10 @@ module.exports = { // skip ignored parameters if (config.caughtErrorsIgnorePattern && config.caughtErrorsIgnorePattern.test(def.name.name)) { + if (config.reportUsedIgnorePattern && isUsedVariable(variable)) { + usedIgnoredVars.push(variable); + } + continue; } } else if (type === "Parameter") { @@ -650,6 +765,10 @@ module.exports = { // skip ignored parameters if (config.argsIgnorePattern && config.argsIgnorePattern.test(def.name.name)) { + if (config.reportUsedIgnorePattern && isUsedVariable(variable)) { + usedIgnoredVars.push(variable); + } + continue; } @@ -661,6 +780,10 @@ module.exports = { // skip ignored variables if (config.varsIgnorePattern && config.varsIgnorePattern.test(def.name.name)) { + if (config.reportUsedIgnorePattern && isUsedVariable(variable)) { + usedIgnoredVars.push(variable); + } + continue; } } @@ -673,10 +796,10 @@ module.exports = { } for (i = 0, l = childScopes.length; i < l; ++i) { - collectUnusedVariables(childScopes[i], unusedVars); + collectUnusedVariables(childScopes[i], unusedVars, usedIgnoredVars); } - return unusedVars; + return [unusedVars, usedIgnoredVars]; } //-------------------------------------------------------------------------- @@ -685,7 +808,7 @@ module.exports = { return { "Program:exit"(programNode) { - const unusedVars = collectUnusedVariables(sourceCode.getScope(programNode), []); + const [unusedVars, usedIgnoredVars] = collectUnusedVariables(sourceCode.getScope(programNode), [], []); for (let i = 0, l = unusedVars.length; i < l; ++i) { const unusedVar = unusedVars[i]; @@ -722,8 +845,19 @@ module.exports = { }); } } + + if (config.reportUsedIgnorePattern) { + for (const usedIgnoredVar of usedIgnoredVars) { + for (const definition of usedIgnoredVar.defs) { + context.report({ + node: definition.name, + messageId: "usedIgnoredVar", + data: getUsedIgnoredMessageData(usedIgnoredVar) + }); + } + } + } } }; - } }; diff --git a/tests/lib/rules/no-unused-vars.js b/tests/lib/rules/no-unused-vars.js index de85050248f..e32bec7bb5c 100644 --- a/tests/lib/rules/no-unused-vars.js +++ b/tests/lib/rules/no-unused-vars.js @@ -88,6 +88,24 @@ function assignedError(varName, additional = "", type = "Identifier") { }; } +/** + * Returns an expected error for used-but-ignored variables. + * @param {string} varName The name of the variable + * @param {string} [additional] The additional text for the message data + * @param {string} [type] The node type (defaults to "Identifier") + * @returns {Object} An expected error object + */ +function usedIgnoredError(varName, additional = "", type = "Identifier") { + return { + messageId: "usedIgnoredVar", + data: { + varName, + additional + }, + type + }; +} + ruleTester.run("no-unused-vars", rule, { valid: [ "var foo = 5;\n\nlabel: while (true) {\n console.log(foo);\n break label;\n}", @@ -462,6 +480,21 @@ ruleTester.run("no-unused-vars", rule, { code: "class Foo { static {} }", options: [{ ignoreClassWithStaticInitBlock: false, varsIgnorePattern: "^Foo" }], languageOptions: { ecmaVersion: 2022 } + }, + // https://github.com/eslint/eslint/issues/17568 + { + code: "const a = 5; const _c = a + 5;", + options: [{ args: "all", varsIgnorePattern: "^_", reportUsedIgnorePattern: true }], + languageOptions: { ecmaVersion: 6 } + }, + { + code: "(function foo(a, _b) { return a + 5 })(5)", + options: [{ args: "all", argsIgnorePattern: "^_", reportUsedIgnorePattern: true }] + }, + { + code: "const [ a, _b, c ] = items;\nconsole.log(a+c);", + options: [{ destructuredArrayIgnorePattern: "^_", reportUsedIgnorePattern: true }], + languageOptions: { ecmaVersion: 6 } } ], invalid: [ @@ -621,12 +654,12 @@ ruleTester.run("no-unused-vars", rule, { languageOptions: { ecmaVersion: 2020 }, errors: [ { - ...assignedError("a", ". Allowed unused elements of array destructuring patterns must match /^_/u"), + ...assignedError("a", ". Allowed unused elements of array destructuring must match /^_/u"), line: 3, column: 20 }, { - ...assignedError("c", ". Allowed unused elements of array destructuring patterns must match /^_/u"), + ...assignedError("c", ". Allowed unused elements of array destructuring must match /^_/u"), line: 3, column: 27 } @@ -644,12 +677,12 @@ ruleTester.run("no-unused-vars", rule, { languageOptions: { ecmaVersion: 2020 }, errors: [ { - ...assignedError("a", ". Allowed unused elements of array destructuring patterns must match /^_/u"), + ...assignedError("a", ". Allowed unused elements of array destructuring must match /^_/u"), line: 3, column: 20 }, { - ...assignedError("c", ". Allowed unused elements of array destructuring patterns must match /^_/u"), + ...assignedError("c", ". Allowed unused elements of array destructuring must match /^_/u"), line: 3, column: 27 }, @@ -1611,6 +1644,39 @@ c = foo1`, options: [{ ignoreClassWithStaticInitBlock: true }], languageOptions: { ecmaVersion: 2022 }, errors: [{ ...definedError("Foo"), line: 1, column: 7 }] + }, + // https://github.com/eslint/eslint/issues/17568 + { + code: "const _a = 5;const _b = _a + 5", + options: [{ args: "all", varsIgnorePattern: "^_", reportUsedIgnorePattern: true }], + languageOptions: { ecmaVersion: 6 }, + errors: [ + usedIgnoredError("_a", ". Used vars must not match /^_/u") + ] + }, + { + code: "const _a = 42; foo(() => _a);", + options: [{ args: "all", varsIgnorePattern: "^_", reportUsedIgnorePattern: true }], + languageOptions: { ecmaVersion: 6 }, + errors: [usedIgnoredError("_a", ". Used vars must not match /^_/u")] + }, + { + code: "(function foo(_a) { return _a + 5 })(5)", + options: [{ args: "all", argsIgnorePattern: "^_", reportUsedIgnorePattern: true }], + errors: [usedIgnoredError("_a", ". Used args must not match /^_/u")] + }, + { + code: "const [ a, _b ] = items;\nconsole.log(a+_b);", + options: [{ destructuredArrayIgnorePattern: "^_", reportUsedIgnorePattern: true }], + languageOptions: { ecmaVersion: 6 }, + errors: [ + usedIgnoredError("_b", ". Used elements of array destructuring must not match /^_/u") + ] + }, + { + code: "try{}catch(_err){console.error(_err)}", + options: [{ caughtErrors: "all", caughtErrorsIgnorePattern: "^_", reportUsedIgnorePattern: true }], + errors: [usedIgnoredError("_err", ". Used args must not match /^_/u")] } ] });