From fa14942904cde045f20024f744024f0f56711484 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sat, 7 Oct 2023 15:52:29 +0900 Subject: [PATCH 01/35] feat: add `no-useless-assignment` rule --- docs/src/rules/no-unused-vars.md | 2 + docs/src/rules/no-useless-assignment.md | 96 ++++++ lib/rules/index.js | 1 + lib/rules/no-useless-assignment.js | 289 +++++++++++++++++++ packages/js/src/configs/eslint-all.js | 1 + tests/lib/rules/no-useless-assignment.js | 353 +++++++++++++++++++++++ tools/rule-types.json | 1 + 7 files changed, 743 insertions(+) create mode 100644 docs/src/rules/no-useless-assignment.md create mode 100644 lib/rules/no-useless-assignment.js create mode 100644 tests/lib/rules/no-useless-assignment.js diff --git a/docs/src/rules/no-unused-vars.md b/docs/src/rules/no-unused-vars.md index 1b3e59ce93b..4cc36ec3160 100644 --- a/docs/src/rules/no-unused-vars.md +++ b/docs/src/rules/no-unused-vars.md @@ -1,6 +1,8 @@ --- title: no-unused-vars rule_type: problem +related_rules: +- no-useless-assignment --- diff --git a/docs/src/rules/no-useless-assignment.md b/docs/src/rules/no-useless-assignment.md new file mode 100644 index 00000000000..d4fffe48c31 --- /dev/null +++ b/docs/src/rules/no-useless-assignment.md @@ -0,0 +1,96 @@ +--- +title: no-useless-assignment +rule_type: suggestion +related_rules: +- no-unused-vars +--- + + + +If the variable is not used after assignment is needless and most likely indicates that there is some behavior that the author wanted but didn't correctly do. + +## Rule Details + +This rule aims to report variables that are not used after assignment. + +Examples of **incorrect** code for this rule: + +::: incorrect + +```js +/* eslint no-useless-assignment: "error" */ + +function foo() { + let v = 'used'; + doSomething(v); + v = 'unused'; +} + +function foo() { + let v = 'used'; + if (condition) { + v = 'unused'; + return + } + doSomething(v); +} + +function foo() { + let v = 'used'; + if (condition) { + doSomething(v); + } else { + v = 'unused'; + } +} +``` + +::: + +Examples of **correct** code for this rule: + +::: correct + +```js +/* eslint no-useless-assignment: "error" */ + +function foo() { + let v = 'used'; + doSomething(v); + v = 'used-2'; + doSomething(v); +} + +function foo() { + let v = 'used'; + if (condition) { + v = 'used-2'; + doSomething(v); + return + } +} + +function foo() { + let v = 'used'; + if (condition) { + doSomething(v); + } else { + v = 'used-2'; + doSomething(v); + } +} + +function foo () { + let v = 'used'; + for (let i = 0; i < 10; i++) { + doSomething(v); + v = 'used in next iteration'; + } +} +``` + +::: + +## When Not To Use It + +If you don't want to be notified about unnecessary arguments, you can safely turn this rule off. diff --git a/lib/rules/index.js b/lib/rules/index.js index 840abe73b0f..97281c6754a 100644 --- a/lib/rules/index.js +++ b/lib/rules/index.js @@ -229,6 +229,7 @@ module.exports = new LazyLoadingRuleMap(Object.entries({ "no-unused-private-class-members": () => require("./no-unused-private-class-members"), "no-unused-vars": () => require("./no-unused-vars"), "no-use-before-define": () => require("./no-use-before-define"), + "no-useless-assignment": () => require("./no-useless-assignment"), "no-useless-backreference": () => require("./no-useless-backreference"), "no-useless-call": () => require("./no-useless-call"), "no-useless-catch": () => require("./no-useless-catch"), diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js new file mode 100644 index 00000000000..bd977ebc617 --- /dev/null +++ b/lib/rules/no-useless-assignment.js @@ -0,0 +1,289 @@ +/** + * @fileoverview A rule to disallow unnecessary assignments`. + * @author Yosuke Ota + */ + +"use strict"; + +const { findVariable } = require("@eslint-community/eslint-utils"); + +//------------------------------------------------------------------------------ +// Types +//------------------------------------------------------------------------------ + +/** @typedef {import("estree").Node} ASTNode */ +/** @typedef {import("estree").Pattern} Pattern */ +/** @typedef {import("estree").Identifier} Identifier */ +/** @typedef {import("eslint-scope").Scope} Scope */ +/** @typedef {import("eslint-scope").Variable} Variable */ +/** @typedef {import("../linter/code-path-analysis/code-path")} CodePath */ +/** @typedef {import("../linter/code-path-analysis/code-path-segment")} CodePathSegment */ + +//------------------------------------------------------------------------------ +// Helpers +//------------------------------------------------------------------------------ + +/** + * Extract identifier from the given pattern node used on the left-hand side of the assignment. + * @param {Pattern|null} pattern The pattern node to extract identifier + * @returns {Iterable} The extracted identifier + */ +function *extractIdentifiersFromPattern(pattern) { + if (!pattern) { + return; + } + if (pattern.type === "Identifier") { + yield pattern; + } + switch (pattern.type) { + case "ObjectPattern": + for (const property of pattern.properties) { + yield* extractIdentifiersFromPattern(property.type === "Property" ? property.value : property.argument); + } + return; + case "ArrayPattern": + for (const element of pattern.elements) { + yield* extractIdentifiersFromPattern(element); + } + return; + case "RestElement": + yield* extractIdentifiersFromPattern(pattern.argument); + return; + case "AssignmentPattern": + yield* extractIdentifiersFromPattern(pattern.right); + + // no default + } +} + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +/** @type {import('../shared/types').Rule} */ +module.exports = { + meta: { + type: "suggestion", + + docs: { + description: "Disallow variables that are not used after assignment", + recommended: false, + url: "https://eslint.org/docs/latest/rules/no-useless-assignment" + }, + + schema: [], + + messages: { + unnecessaryAssignment: "This assigned value is not used in subsequent statements." + } + }, + + create(context) { + const sourceCode = context.sourceCode; + + /** + * @typedef {Object} ScopeStack + * @property {CodePath} codePath The code path of this scope stack. + * @property {Scope} scope The scope of this scope stack. + * @property {ScopeStack} upper The upper scope stack. + * @property {CodePathSegment[]} currentSegments The current code path segments. + * @property {Record>} writes Map of collection of wrote variables within segment ID. + * @property {Record>} segmentIdentifiers Map of collection of identifiers within segment ID. + */ + /** @type {ScopeStack} */ + let scopeStack = null; + + /** @type {Set} */ + const codePathStartScopes = new Set(); + + /** + * Gets the scope of code path start from given scope + * @param {Scope} scope The initial scope + * @returns {Scope} The scope of code path start + * @throws {Error} Unexpected error + */ + function getCodePathStartScope(scope) { + let target = scope; + + while (target) { + if (codePathStartScopes.has(target)) { + return target; + } + target = target.upper; + } + throw new Error("Should be unreachable"); + } + + /** + * Verify the given scope stack. + * @param {ScopeStack} target The scope stack to verify. + * @returns {void} + */ + function verify(target) { + + /** + * Checks whether the given identifier is used in the segment. + * @param {CodePathSegment} segment The code path segment. + * @param {Identifier} identifier The identifier to check. + * @returns {boolean} `true` if the identifier is used in the segment. + */ + function isInSegment(segment, identifier) { + return target.segmentIdentifiers[segment.id].has(identifier); + } + + /** + * Verify the given write variable. + * @param {CodePathSegment} segment The current code path segment. + * @param {Variable} variable The write variable to verify. + * @param {Identifier} identifier The written identifier. + * @returns {void} + */ + function verifyWrittenVariable(segment, variable, identifier) { + + /* + * If the scope of the variable is outside the current code path scope, + * we cannot track whether this assignment is not used. + */ + if (target.scope !== getCodePathStartScope(variable.scope)) { + return; + } + + // Variables exported by "exported" block comments + if (variable.eslintExported) { + return; + } + + // Variables exported by ESM export syntax + if ( + variable.scope.type === "module" && + variable.defs + .some(def => ( + (def.type === "Variable" && def.parent.parent.type === "ExportNamedDeclaration") || + (def.type === "FunctionName" && def.node.parent.type === "ExportNamedDeclaration") || + (def.type === "ClassName" && def.node.parent.type === "ExportNamedDeclaration") + )) + ) { + return; + } + + for (const reference of variable.references) { + if (!reference.isRead()) { + continue; + } + + /* + * If the scope of the reference is outside the current code path scope, + * we cannot track whether this assignment is not used. + * For example, it can also be called asynchronously. + */ + if (target.scope !== getCodePathStartScope(reference.from)) { + return; + } + + if (identifier.range[0] < reference.identifier.range[0] && isInSegment(segment, reference.identifier)) { + + // Uses in statements after the written identifier. + return; + } + + // Check subsequent segments. + const alreadyChecked = new Set(); + const queueSegments = [...segment.nextSegments]; + + while (queueSegments.length > 0) { + const nextSegment = queueSegments.shift(); + + if (alreadyChecked.has(nextSegment.id)) { + continue; + } + alreadyChecked.add(nextSegment.id); + + if (isInSegment(nextSegment, reference.identifier)) { + + // It is used + return; + } + + queueSegments.push(...nextSegment.nextSegments); + } + } + context.report({ + node: identifier, + messageId: "unnecessaryAssignment" + }); + } + + target.codePath.traverseSegments(segment => { + if (!target.writes[segment.id]) { + return; + } + for (const [variable, identifier] of target.writes[segment.id]) { + verifyWrittenVariable(segment, variable, identifier); + } + }); + } + + return { + + /** + * Adds information of a constructor into the stack. + * @param {CodePath} codePath A code path which was started. + * @param {ASTNode} node The current node. + * @returns {void} + */ + onCodePathStart(codePath, node) { + const scope = sourceCode.getScope(node); + + scopeStack = { + upper: scopeStack, + codePath, + scope, + currentSegments: [], + segmentIdentifiers: Object.create(null), + writes: Object.create(null) + }; + codePathStartScopes.add(scopeStack.scope); + }, + onCodePathEnd() { + verify(scopeStack); + + scopeStack = scopeStack.upper; + }, + onCodePathSegmentStart(segment) { + scopeStack.currentSegments.unshift(segment); + scopeStack.segmentIdentifiers[segment.id] = new Set(); + }, + onUnreachableCodePathSegmentStart(segment) { + scopeStack.currentSegments.unshift(segment); + scopeStack.segmentIdentifiers[segment.id] = new Set(); + }, + onCodePathSegmentEnd() { + scopeStack.currentSegments.shift(); + }, + onUnreachableCodePathSegmentEnd() { + scopeStack.currentSegments.shift(); + }, + Identifier(node) { + const segmentId = scopeStack.currentSegments[0].id; + + scopeStack.segmentIdentifiers[segmentId].add(node); + }, + "AssignmentExpression, UpdateExpression"(node) { + const segmentId = scopeStack.currentSegments[0].id; + const writes = scopeStack.writes[segmentId] || (scopeStack.writes[segmentId] = new Map()); + + for (const identifier of extractIdentifiersFromPattern(node.left)) { + const scope = sourceCode.getScope(identifier); + + /** @type {Variable} */ + const variable = findVariable(scope, identifier); + + if (!variable) { + return; + } + writes.set(variable, identifier); + } + } + }; + } +}; diff --git a/packages/js/src/configs/eslint-all.js b/packages/js/src/configs/eslint-all.js index 52f580035a8..626f70b6bf8 100644 --- a/packages/js/src/configs/eslint-all.js +++ b/packages/js/src/configs/eslint-all.js @@ -203,6 +203,7 @@ module.exports = Object.freeze({ "no-unused-private-class-members": "error", "no-unused-vars": "error", "no-use-before-define": "error", + "no-useless-assignment": "error", "no-useless-backreference": "error", "no-useless-call": "error", "no-useless-catch": "error", diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js new file mode 100644 index 00000000000..7ff3bfead98 --- /dev/null +++ b/tests/lib/rules/no-useless-assignment.js @@ -0,0 +1,353 @@ +/** + * @fileoverview Tests for no-useless-assignment rule. + * @author Yosuke Ota + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require("../../../lib/rules/no-useless-assignment"); +const { RuleTester } = require("../../../lib/rule-tester"); + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const ruleTester = new RuleTester({ + parserOptions: { ecmaVersion: 2022, sourceType: "module" } +}); + +ruleTester.run("no-useless-assignment", rule, { + valid: [ + + // Basic tests + `let v = 'used'; + console.log(v); + v = 'used-2' + console.log(v);`, + `function foo() { + let v = 'used'; + console.log(v); + v = 'used-2'; + console.log(v); + }`, + `function foo() { + let v = 'used'; + if (condition) { + v = 'used-2'; + console.log(v); + return + } + }`, + `function foo() { + let v = 'used'; + if (condition) { + console.log(v); + } else { + v = 'used-2'; + console.log(v); + } + }`, + `var foo = function () { + let v = 'used'; + console.log(v); + v = 'used-2' + console.log(v); + }`, + `var foo = () => { + let v = 'used'; + console.log(v); + v = 'used-2' + console.log(v); + }`, + `class foo { + static { + let v = 'used'; + console.log(v); + v = 'used-2' + console.log(v); + } + }`, + `function foo () { + let v = 'used'; + for (let i = 0; i < 10; i++) { + console.log(v); + v = 'used in next iteration'; + } + }`, + + // Exported + `export let foo = 'used'; + console.log(foo); + foo = 'unused like but exported';`, + `export function foo () {}; + console.log(foo); + foo = 'unused like but exported';`, + `export class foo {}; + console.log(foo); + foo = 'unused like but exported';`, + { + code: + `/* exported foo */ + let foo = 'used'; + console.log(foo); + foo = 'unused like but exported with directive';`, + parserOptions: { sourceType: "script" } + }, + + // Value may be used in other scopes. + `function foo () { + let v = 'used'; + console.log(v); + function bar() { + v = 'used in outer scope'; + } + bar(); + console.log(v); + }`, + `function foo () { + let v = 'used'; + console.log(v); + setTimeout(() => console.log(v), 1); + v = 'used in other scope'; + }`, + + // Loops + `function foo () { + let v = 'used'; + console.log(v); + for (let i = 0; i < 10; i++) { + if (condition) { + v = 'maybe used'; + continue; + } + console.log(v); + } + }` + ], + invalid: [ + { + code: + `let v = 'used'; + console.log(v); + v = 'unused'`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 3, + column: 13 + } + ] + }, + { + code: + `function foo() { + let v = 'used'; + console.log(v); + v = 'unused'; + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + } + ] + }, + { + code: + `function foo() { + let v = 'used'; + if (condition) { + v = 'unused'; + return + } + console.log(v); + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 21 + } + ] + }, + { + code: + `function foo() { + let v = 'used'; + if (condition) { + console.log(v); + } else { + v = 'unused'; + } + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 6, + column: 21 + } + ] + }, + { + code: + `var foo = function () { + let v = 'used'; + console.log(v); + v = 'unused' + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + } + ] + }, + { + code: + `var foo = () => { + let v = 'used'; + console.log(v); + v = 'unused' + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + } + ] + }, + { + code: + `class foo { + static { + let v = 'used'; + console.log(v); + v = 'unused' + } + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 5, + column: 21 + } + ] + }, + + // Variable used in other scopes, but write only. + { + code: + `function foo () { + let v = 'used'; + console.log(v); + setTimeout(() => v = 42, 1); + v = 'unused and variable is only updated in other scopes'; + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 5, + column: 17 + } + ] + }, + + // Code Path Segment End Statements + { + code: + `function foo () { + let v = 'used'; + console.log(v); + v = 'unused'; + return; + console.log(v); + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + } + ] + }, + { + code: + `function foo () { + let v = 'used'; + console.log(v); + v = 'unused'; + throw new Error(); + console.log(v); + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + } + ] + }, + { + code: + `function foo () { + let v = 'used'; + console.log(v); + for (let i = 0; i < 10; i++) { + v = 'unused'; + continue; + console.log(v); + } + } + function bar () { + let v = 'used'; + console.log(v); + for (let i = 0; i < 10; i++) { + v = 'unused'; + break; + console.log(v); + } + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 5, + column: 21 + }, + { + messageId: "unnecessaryAssignment", + line: 14, + column: 21 + } + ] + }, + { + code: + `function foo () { + let v = 'used'; + console.log(v); + for (let i = 0; i < 10; i++) { + if (condition) { + v = 'unused'; + break; + } + console.log(v); + } + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 6, + column: 25 + } + ] + } + + ] +}); diff --git a/tools/rule-types.json b/tools/rule-types.json index 35895f3838a..7f95e69e128 100644 --- a/tools/rule-types.json +++ b/tools/rule-types.json @@ -216,6 +216,7 @@ "no-unused-private-class-members": "problem", "no-unused-vars": "problem", "no-use-before-define": "problem", + "no-useless-assignment": "suggestion", "no-useless-backreference": "problem", "no-useless-call": "suggestion", "no-useless-catch": "suggestion", From 020ee4ffad29820146dd7a2211d5c228f66186f9 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sat, 7 Oct 2023 16:14:11 +0900 Subject: [PATCH 02/35] fix: add test cases and fix --- lib/rules/no-useless-assignment.js | 20 ++-- tests/lib/rules/no-useless-assignment.js | 113 +++++++++++++++++++++++ 2 files changed, 124 insertions(+), 9 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index bd977ebc617..6e46e609101 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -29,20 +29,20 @@ const { findVariable } = require("@eslint-community/eslint-utils"); * @returns {Iterable} The extracted identifier */ function *extractIdentifiersFromPattern(pattern) { - if (!pattern) { - return; - } - if (pattern.type === "Identifier") { - yield pattern; - } switch (pattern.type) { + case "Identifier": + yield pattern; + return; case "ObjectPattern": for (const property of pattern.properties) { - yield* extractIdentifiersFromPattern(property.type === "Property" ? property.value : property.argument); + yield* extractIdentifiersFromPattern(property.type === "Property" ? property.value : property); } return; case "ArrayPattern": for (const element of pattern.elements) { + if (!element) { + continue; + } yield* extractIdentifiersFromPattern(element); } return; @@ -50,7 +50,7 @@ function *extractIdentifiersFromPattern(pattern) { yield* extractIdentifiersFromPattern(pattern.argument); return; case "AssignmentPattern": - yield* extractIdentifiersFromPattern(pattern.right); + yield* extractIdentifiersFromPattern(pattern.left); // no default } @@ -272,7 +272,9 @@ module.exports = { const segmentId = scopeStack.currentSegments[0].id; const writes = scopeStack.writes[segmentId] || (scopeStack.writes[segmentId] = new Map()); - for (const identifier of extractIdentifiersFromPattern(node.left)) { + const pattern = node.type === "AssignmentExpression" ? node.left : node.argument; + + for (const identifier of extractIdentifiersFromPattern(pattern)) { const scope = sourceCode.getScope(identifier); /** @type {Variable} */ diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index 7ff3bfead98..9c77dce2970 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -98,6 +98,34 @@ ruleTester.run("no-useless-assignment", rule, { parserOptions: { sourceType: "script" } }, + // Update + `function foo() { + let a = 42; + console.log(a); + a++; + console.log(a); + }`, + `function foo() { + let a = 42; + console.log(a); + a--; + console.log(a); + }`, + + // Assign to complex patterns + `function foo() { + let a = 'used', b = 'used', c = 'used', d = 'used'; + console.log(a, b, c, d); + ({ a, arr: [b, c, ...d] } = fn()); + console.log(a, b, c, d); + }`, + `function foo() { + let a = 'used', b = 'used', c = 'used'; + console.log(a, b, c); + ({ a = 'unused', foo: b, ...c } = fn()); + console.log(a, b, c); + }`, + // Value may be used in other scopes. `function foo () { let v = 'used'; @@ -241,6 +269,91 @@ ruleTester.run("no-useless-assignment", rule, { ] }, + // Update + { + code: + `function foo() { + let a = 42; + console.log(a); + a++; + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + } + ] + }, + { + code: + `function foo() { + let a = 42; + console.log(a); + a--; + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + } + ] + }, + + // Assign to complex patterns + { + code: + `function foo() { + let a = 'used', b = 'used', c = 'used', d = 'used'; + console.log(a, b, c, d); + ({ a, arr: [b, c, ...d] } = fn()); + console.log(c); + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 20 + }, + { + messageId: "unnecessaryAssignment", + line: 4, + column: 29 + }, + { + messageId: "unnecessaryAssignment", + line: 4, + column: 38 + } + ] + }, + { + code: + `function foo() { + let a = 'used', b = 'used', c = 'used'; + console.log(a, b, c); + ({ a = 'unused', foo: b, ...c } = fn()); + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 20 + }, + { + messageId: "unnecessaryAssignment", + line: 4, + column: 39 + }, + { + messageId: "unnecessaryAssignment", + line: 4, + column: 45 + } + ] + }, + // Variable used in other scopes, but write only. { code: From 35327ba101e290fab9cc5c34b68a0a1320ec5c56 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sat, 7 Oct 2023 16:16:06 +0900 Subject: [PATCH 03/35] chore: remove wrong comment --- lib/rules/no-useless-assignment.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 6e46e609101..b807e6a6abd 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -224,13 +224,6 @@ module.exports = { } return { - - /** - * Adds information of a constructor into the stack. - * @param {CodePath} codePath A code path which was started. - * @param {ASTNode} node The current node. - * @returns {void} - */ onCodePathStart(codePath, node) { const scope = sourceCode.getScope(node); From ca5c5c22ba92937284b6644b05a3fa06c3d91e1d Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Mon, 9 Oct 2023 14:38:13 +0900 Subject: [PATCH 04/35] test: add test and update --- docs/src/rules/no-useless-assignment.md | 10 ++ lib/rules/no-useless-assignment.js | 123 ++++++++++++++++------- tests/lib/rules/no-useless-assignment.js | 102 ++++++++++++++++++- 3 files changed, 197 insertions(+), 38 deletions(-) diff --git a/docs/src/rules/no-useless-assignment.md b/docs/src/rules/no-useless-assignment.md index d4fffe48c31..2a1d959a0c5 100644 --- a/docs/src/rules/no-useless-assignment.md +++ b/docs/src/rules/no-useless-assignment.md @@ -43,6 +43,16 @@ function foo() { v = 'unused'; } } + +function foo() { + let v = 'used'; + if (condition) { + let v = 'used'; + console.log(v); + v = 'unused'; + } + console.log(v); +} ``` ::: diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index b807e6a6abd..3e98aa14d99 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -86,9 +86,18 @@ module.exports = { * @property {CodePath} codePath The code path of this scope stack. * @property {Scope} scope The scope of this scope stack. * @property {ScopeStack} upper The upper scope stack. - * @property {CodePathSegment[]} currentSegments The current code path segments. - * @property {Record>} writes Map of collection of wrote variables within segment ID. - * @property {Record>} segmentIdentifiers Map of collection of identifiers within segment ID. + * @property {Record} segments Map of ScopeStackSegmentInfo. + * @property {ScopeStackSegmentInfo[]} currentSegments The current ScopeStackSegmentInfo. + */ + /** + * @typedef {Object} ScopeStackSegmentInfo + * @property {Map} writes Map of collection of wrote variables. + * @property {Identifier|null} first The first identifier that appears within the segment. + * @property {Identifier|null} last The last identifier that appears within the segment. + * + * `first` and `last` are used to determine whether an identifier exists within the segment position range. + * Since it is used as a range of segments, we should originally hold all nodes, not just identifiers, + * but since the only nodes to be judged are identifiers, it is sufficient to have a range of identifiers. */ /** @type {ScopeStack} */ let scopeStack = null; @@ -128,7 +137,14 @@ module.exports = { * @returns {boolean} `true` if the identifier is used in the segment. */ function isInSegment(segment, identifier) { - return target.segmentIdentifiers[segment.id].has(identifier); + const segmentInfo = target.segments[segment.id]; + + return ( + segmentInfo.first && + segmentInfo.last && + segmentInfo.first.range[0] <= identifier.range[0] && + identifier.range[1] <= segmentInfo.last.range[1] + ); } /** @@ -140,6 +156,31 @@ module.exports = { */ function verifyWrittenVariable(segment, variable, identifier) { + const subsequentSegmentData = { + subsequentSegments: new Set(), + queueSegments: [...segment.nextSegments] + }; + + + /** + * Gets the subsequent segments from the current segment. + * @returns {Iterable} the subsequent segments + */ + function *getSubsequentSegments() { + yield* subsequentSegmentData.subsequentSegments; + + while (subsequentSegmentData.queueSegments.length > 0) { + const nextSegment = subsequentSegmentData.queueSegments.shift(); + + if (subsequentSegmentData.subsequentSegments.has(nextSegment)) { + continue; + } + subsequentSegmentData.subsequentSegments.add(nextSegment); + subsequentSegmentData.queueSegments.push(...nextSegment.nextSegments); + yield nextSegment; + } + } + /* * If the scope of the variable is outside the current code path scope, * we cannot track whether this assignment is not used. @@ -159,8 +200,20 @@ module.exports = { variable.defs .some(def => ( (def.type === "Variable" && def.parent.parent.type === "ExportNamedDeclaration") || - (def.type === "FunctionName" && def.node.parent.type === "ExportNamedDeclaration") || - (def.type === "ClassName" && def.node.parent.type === "ExportNamedDeclaration") + ( + def.type === "FunctionName" && + ( + def.node.parent.type === "ExportNamedDeclaration" || + def.node.parent.type === "ExportDefaultDeclaration" + ) + ) || + ( + def.type === "ClassName" && + ( + def.node.parent.type === "ExportNamedDeclaration" || + def.node.parent.type === "ExportDefaultDeclaration" + ) + ) )) ) { return; @@ -171,6 +224,12 @@ module.exports = { continue; } + if (reference.identifier.parent.type === "ExportSpecifier") { + + // `export { ... }` reference. All assignments are used externally. + return; + } + /* * If the scope of the reference is outside the current code path scope, * we cannot track whether this assignment is not used. @@ -187,24 +246,12 @@ module.exports = { } // Check subsequent segments. - const alreadyChecked = new Set(); - const queueSegments = [...segment.nextSegments]; - - while (queueSegments.length > 0) { - const nextSegment = queueSegments.shift(); - - if (alreadyChecked.has(nextSegment.id)) { - continue; - } - alreadyChecked.add(nextSegment.id); - - if (isInSegment(nextSegment, reference.identifier)) { + for (const subsequentSegment of getSubsequentSegments()) { + if (isInSegment(subsequentSegment, reference.identifier)) { // It is used return; } - - queueSegments.push(...nextSegment.nextSegments); } } context.report({ @@ -214,10 +261,9 @@ module.exports = { } target.codePath.traverseSegments(segment => { - if (!target.writes[segment.id]) { - return; - } - for (const [variable, identifier] of target.writes[segment.id]) { + const segmentInfo = target.segments[segment.id]; + + for (const [variable, identifier] of segmentInfo.writes) { verifyWrittenVariable(segment, variable, identifier); } }); @@ -231,9 +277,8 @@ module.exports = { upper: scopeStack, codePath, scope, - currentSegments: [], - segmentIdentifiers: Object.create(null), - writes: Object.create(null) + segments: Object.create(null), + currentSegments: [] }; codePathStartScopes.add(scopeStack.scope); }, @@ -243,12 +288,16 @@ module.exports = { scopeStack = scopeStack.upper; }, onCodePathSegmentStart(segment) { - scopeStack.currentSegments.unshift(segment); - scopeStack.segmentIdentifiers[segment.id] = new Set(); + const segmentInfo = { writes: new Map(), first: null, last: null }; + + scopeStack.segments[segment.id] = segmentInfo; + scopeStack.currentSegments.unshift(segmentInfo); }, onUnreachableCodePathSegmentStart(segment) { - scopeStack.currentSegments.unshift(segment); - scopeStack.segmentIdentifiers[segment.id] = new Set(); + const segmentInfo = { writes: new Map(), first: null, last: null }; + + scopeStack.segments[segment.id] = segmentInfo; + scopeStack.currentSegments.unshift(segmentInfo); }, onCodePathSegmentEnd() { scopeStack.currentSegments.shift(); @@ -257,13 +306,15 @@ module.exports = { scopeStack.currentSegments.shift(); }, Identifier(node) { - const segmentId = scopeStack.currentSegments[0].id; + const segmentInfo = scopeStack.currentSegments[0]; - scopeStack.segmentIdentifiers[segmentId].add(node); + if (!segmentInfo.first) { + segmentInfo.first = node; + } + segmentInfo.last = node; }, "AssignmentExpression, UpdateExpression"(node) { - const segmentId = scopeStack.currentSegments[0].id; - const writes = scopeStack.writes[segmentId] || (scopeStack.writes[segmentId] = new Map()); + const segmentInfo = scopeStack.currentSegments[0]; const pattern = node.type === "AssignmentExpression" ? node.left : node.argument; @@ -276,7 +327,7 @@ module.exports = { if (!variable) { return; } - writes.set(variable, identifier); + segmentInfo.writes.set(variable, identifier); } } }; diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index 9c77dce2970..2e878f37190 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -22,6 +22,10 @@ const ruleTester = new RuleTester({ ruleTester.run("no-useless-assignment", rule, { valid: [ + `let foo = 'used'; + export { foo }; + console.log(foo); + foo = 'unused like but exported';`, // Basic tests `let v = 'used'; @@ -89,6 +93,20 @@ ruleTester.run("no-useless-assignment", rule, { `export class foo {}; console.log(foo); foo = 'unused like but exported';`, + `export default function foo () {}; + console.log(foo); + foo = 'unused like but exported';`, + `export default class foo {}; + console.log(foo); + foo = 'unused like but exported';`, + `function foo () {}; + export { foo }; + console.log(foo); + foo = 'unused like but exported';`, + `class foo {}; + export { foo }; + console.log(foo); + foo = 'unused like but exported';`, { code: `/* exported foo */ @@ -98,6 +116,11 @@ ruleTester.run("no-useless-assignment", rule, { parserOptions: { sourceType: "script" } }, + // Unknown variable + `v = 'used'; + console.log(v); + v = 'unused'`, + // Update `function foo() { let a = 42; @@ -125,6 +148,16 @@ ruleTester.run("no-useless-assignment", rule, { ({ a = 'unused', foo: b, ...c } = fn()); console.log(a, b, c); }`, + `function foo() { + let a = {}; + console.log(a); + a.b = 'unused like, but maybe used in setter'; + }`, + `function foo() { + let a = { b: 42 }; + console.log(a); + a.b++; + }`, // Value may be used in other scopes. `function foo () { @@ -268,6 +301,22 @@ ruleTester.run("no-useless-assignment", rule, { } ] }, + { + code: + `function foo() { + let v = 'used'; + console.log(v); + v = 'unused'; + v = 'unused'; + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 5, + column: 17 + } + ] + }, // Update { @@ -307,7 +356,7 @@ ruleTester.run("no-useless-assignment", rule, { `function foo() { let a = 'used', b = 'used', c = 'used', d = 'used'; console.log(a, b, c, d); - ({ a, arr: [b, c, ...d] } = fn()); + ({ a, arr: [b, c,, ...d] } = fn()); console.log(c); }`, errors: [ @@ -324,7 +373,7 @@ ruleTester.run("no-useless-assignment", rule, { { messageId: "unnecessaryAssignment", line: 4, - column: 38 + column: 39 } ] }, @@ -373,6 +422,55 @@ ruleTester.run("no-useless-assignment", rule, { }, // Code Path Segment End Statements + { + code: + `function foo() { + let v = 'used'; + if (condition) { + let v = 'used'; + console.log(v); + v = 'unused'; + } + console.log(v); + v = 'unused'; + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 6, + column: 21 + }, + { + messageId: "unnecessaryAssignment", + line: 9, + column: 17 + } + ] + }, + { + code: + `function foo() { + let v = 'used'; + if (condition) { + console.log(v); + v = 'unused'; + } else { + v = 'unused'; + } + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 5, + column: 21 + }, + { + messageId: "unnecessaryAssignment", + line: 7, + column: 21 + } + ] + }, { code: `function foo () { From 3d63f0fe2797c8894fa9f01d95e744a8da6e16e7 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Mon, 9 Oct 2023 23:36:00 +0900 Subject: [PATCH 05/35] feat: report if it is not used between assignments --- docs/src/rules/no-useless-assignment.md | 10 ++ lib/rules/no-useless-assignment.js | 197 ++++++++++++++--------- tests/lib/rules/no-useless-assignment.js | 38 ++++- 3 files changed, 169 insertions(+), 76 deletions(-) diff --git a/docs/src/rules/no-useless-assignment.md b/docs/src/rules/no-useless-assignment.md index 2a1d959a0c5..9636ec66254 100644 --- a/docs/src/rules/no-useless-assignment.md +++ b/docs/src/rules/no-useless-assignment.md @@ -44,6 +44,15 @@ function foo() { } } +function foo() { + let v = 'unused'; + if (condition) { + v = 'used'; + doSomething(v); + return + } +} + function foo() { let v = 'used'; if (condition) { @@ -78,6 +87,7 @@ function foo() { doSomething(v); return } + doSomething(v); } function foo() { diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 3e98aa14d99..8e4e84aaddc 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -86,12 +86,13 @@ module.exports = { * @property {CodePath} codePath The code path of this scope stack. * @property {Scope} scope The scope of this scope stack. * @property {ScopeStack} upper The upper scope stack. - * @property {Record} segments Map of ScopeStackSegmentInfo. + * @property {Record} segments The map of ScopeStackSegmentInfo. * @property {ScopeStackSegmentInfo[]} currentSegments The current ScopeStackSegmentInfo. + * @property {Map} assignments The map of list of AssignmentInfo for each variable. */ /** * @typedef {Object} ScopeStackSegmentInfo - * @property {Map} writes Map of collection of wrote variables. + * @property {CodePathSegment} segment The code path segment. * @property {Identifier|null} first The first identifier that appears within the segment. * @property {Identifier|null} last The last identifier that appears within the segment. * @@ -99,6 +100,12 @@ module.exports = { * Since it is used as a range of segments, we should originally hold all nodes, not just identifiers, * but since the only nodes to be judged are identifiers, it is sufficient to have a range of identifiers. */ + /** + * @typedef {Object} AssignmentInfo + * @property {Variable} variable The variable that is assigned. + * @property {Identifier} identifier The identifier that is assigned. + * @property {CodePathSegment} segment The code path segment where the assignment was made. + */ /** @type {ScopeStack} */ let scopeStack = null; @@ -148,26 +155,27 @@ module.exports = { } /** - * Verify the given write variable. - * @param {CodePathSegment} segment The current code path segment. - * @param {Variable} variable The write variable to verify. - * @param {Identifier} identifier The written identifier. + * Verify the given assignment info. + * @param {AssignmentInfo} assignment The assignment info to verify. + * @param {AssignmentInfo[]} otherAssignments The list of assignment info for variables other than `assignment`. * @returns {void} */ - function verifyWrittenVariable(segment, variable, identifier) { + function verifyAssignment(assignment, otherAssignments) { + otherAssignments.sort((a, b) => a.identifier.range[0] - b.identifier.range[0]); const subsequentSegmentData = { + results: [], subsequentSegments: new Set(), - queueSegments: [...segment.nextSegments] + queueSegments: [...assignment.segment.nextSegments] }; /** * Gets the subsequent segments from the current segment. - * @returns {Iterable} the subsequent segments + * @returns {Iterable<{ segment: CodePathSegment, assignment?: AssignmentInfo }>} the subsequent segments */ function *getSubsequentSegments() { - yield* subsequentSegmentData.subsequentSegments; + yield* subsequentSegmentData.results; while (subsequentSegmentData.queueSegments.length > 0) { const nextSegment = subsequentSegmentData.queueSegments.shift(); @@ -176,59 +184,30 @@ module.exports = { continue; } subsequentSegmentData.subsequentSegments.add(nextSegment); - subsequentSegmentData.queueSegments.push(...nextSegment.nextSegments); - yield nextSegment; + + const result = { + segment: nextSegment, + assignment: otherAssignments.find(otherAssignment => otherAssignment.segment === nextSegment) + }; + + if (!result.assignment) { + subsequentSegmentData.queueSegments.push(...nextSegment.nextSegments); + } + + subsequentSegmentData.results.push(result); + yield result; } } - /* - * If the scope of the variable is outside the current code path scope, - * we cannot track whether this assignment is not used. - */ - if (target.scope !== getCodePathStartScope(variable.scope)) { - return; - } + const readReferences = assignment.variable.references.filter(reference => reference.isRead()); - // Variables exported by "exported" block comments - if (variable.eslintExported) { - return; - } + if (!readReferences.length) { - // Variables exported by ESM export syntax - if ( - variable.scope.type === "module" && - variable.defs - .some(def => ( - (def.type === "Variable" && def.parent.parent.type === "ExportNamedDeclaration") || - ( - def.type === "FunctionName" && - ( - def.node.parent.type === "ExportNamedDeclaration" || - def.node.parent.type === "ExportDefaultDeclaration" - ) - ) || - ( - def.type === "ClassName" && - ( - def.node.parent.type === "ExportNamedDeclaration" || - def.node.parent.type === "ExportDefaultDeclaration" - ) - ) - )) - ) { + // Maybe unused variable return; } - for (const reference of variable.references) { - if (!reference.isRead()) { - continue; - } - - if (reference.identifier.parent.type === "ExportSpecifier") { - - // `export { ... }` reference. All assignments are used externally. - return; - } + for (const reference of readReferences) { /* * If the scope of the reference is outside the current code path scope, @@ -239,7 +218,10 @@ module.exports = { return; } - if (identifier.range[0] < reference.identifier.range[0] && isInSegment(segment, reference.identifier)) { + if ( + assignment.identifier.range[0] < reference.identifier.range[0] && + isInSegment(assignment.segment, reference.identifier) + ) { // Uses in statements after the written identifier. return; @@ -247,7 +229,15 @@ module.exports = { // Check subsequent segments. for (const subsequentSegment of getSubsequentSegments()) { - if (isInSegment(subsequentSegment, reference.identifier)) { + if (isInSegment(subsequentSegment.segment, reference.identifier)) { + if ( + subsequentSegment.assignment && + subsequentSegment.assignment.identifier.range[0] < reference.identifier.range[0] + ) { + + // There was another assignment before the reference. Therefore, it has not been used yet. + continue; + } // It is used return; @@ -255,18 +245,16 @@ module.exports = { } } context.report({ - node: identifier, + node: assignment.identifier, messageId: "unnecessaryAssignment" }); } - target.codePath.traverseSegments(segment => { - const segmentInfo = target.segments[segment.id]; - - for (const [variable, identifier] of segmentInfo.writes) { - verifyWrittenVariable(segment, variable, identifier); - } - }); + for (const assignments of target.assignments.values()) { + assignments.forEach((assignment, index) => { + verifyAssignment(assignment, [...assignments.slice(0, index), ...assignments.slice(index + 1)]); + }); + } } return { @@ -278,7 +266,8 @@ module.exports = { codePath, scope, segments: Object.create(null), - currentSegments: [] + currentSegments: [], + assignments: new Map() }; codePathStartScopes.add(scopeStack.scope); }, @@ -288,13 +277,13 @@ module.exports = { scopeStack = scopeStack.upper; }, onCodePathSegmentStart(segment) { - const segmentInfo = { writes: new Map(), first: null, last: null }; + const segmentInfo = { segment, first: null, last: null }; scopeStack.segments[segment.id] = segmentInfo; scopeStack.currentSegments.unshift(segmentInfo); }, onUnreachableCodePathSegmentStart(segment) { - const segmentInfo = { writes: new Map(), first: null, last: null }; + const segmentInfo = { segment, first: null, last: null }; scopeStack.segments[segment.id] = segmentInfo; scopeStack.currentSegments.unshift(segmentInfo); @@ -313,10 +302,19 @@ module.exports = { } segmentInfo.last = node; }, - "AssignmentExpression, UpdateExpression"(node) { + "VariableDeclarator[init!=null], AssignmentExpression, UpdateExpression"(node) { const segmentInfo = scopeStack.currentSegments[0]; + const assignments = scopeStack.assignments; - const pattern = node.type === "AssignmentExpression" ? node.left : node.argument; + let pattern; + + if (node.type === "VariableDeclarator") { + pattern = node.id; + } else if (node.type === "AssignmentExpression") { + pattern = node.left; + } else { // UpdateExpression + pattern = node.argument; + } for (const identifier of extractIdentifiersFromPattern(pattern)) { const scope = sourceCode.getScope(identifier); @@ -325,9 +323,64 @@ module.exports = { const variable = findVariable(scope, identifier); if (!variable) { - return; + continue; + } + + /* + * If the scope of the variable is outside the current code path scope, + * we cannot track whether this assignment is not used. + */ + if (scopeStack.scope !== getCodePathStartScope(variable.scope)) { + continue; + } + + // Variables exported by "exported" block comments + if (variable.eslintExported) { + continue; + } + + // Variables exported by ESM export syntax + if (variable.scope.type === "module") { + if ( + variable.defs + .some(def => ( + (def.type === "Variable" && def.parent.parent.type === "ExportNamedDeclaration") || + ( + def.type === "FunctionName" && + ( + def.node.parent.type === "ExportNamedDeclaration" || + def.node.parent.type === "ExportDefaultDeclaration" + ) + ) || + ( + def.type === "ClassName" && + ( + def.node.parent.type === "ExportNamedDeclaration" || + def.node.parent.type === "ExportDefaultDeclaration" + ) + ) + )) + ) { + continue; + } + if (variable.references.some(reference => reference.identifier.parent.type === "ExportSpecifier")) { + + // It have `export { ... }` reference. + continue; + } + } + + let list = assignments.get(variable); + + if (!list) { + list = []; + assignments.set(variable, list); } - segmentInfo.writes.set(variable, identifier); + list.push({ + variable, + identifier, + segment: segmentInfo.segment + }); } } }; diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index 2e878f37190..186dd41bc76 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -22,10 +22,6 @@ const ruleTester = new RuleTester({ ruleTester.run("no-useless-assignment", rule, { valid: [ - `let foo = 'used'; - export { foo }; - console.log(foo); - foo = 'unused like but exported';`, // Basic tests `let v = 'used'; @@ -45,6 +41,7 @@ ruleTester.run("no-useless-assignment", rule, { console.log(v); return } + console.log(v); }`, `function foo() { let v = 'used'; @@ -82,6 +79,12 @@ ruleTester.run("no-useless-assignment", rule, { v = 'used in next iteration'; } }`, + `function foo () { + let i = 0; + i++; + i++; + console.log(i); + }`, // Exported `export let foo = 'used'; @@ -99,6 +102,10 @@ ruleTester.run("no-useless-assignment", rule, { `export default class foo {}; console.log(foo); foo = 'unused like but exported';`, + `let foo = 'used'; + export { foo }; + console.log(foo); + foo = 'unused like but exported';`, `function foo () {}; export { foo }; console.log(foo); @@ -301,6 +308,24 @@ ruleTester.run("no-useless-assignment", rule, { } ] }, + { + code: + `function foo() { + let v = 'unused'; + if (condition) { + v = 'used'; + console.log(v); + return + } + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 2, + column: 21 + } + ] + }, { code: `function foo() { @@ -310,6 +335,11 @@ ruleTester.run("no-useless-assignment", rule, { v = 'unused'; }`, errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + }, { messageId: "unnecessaryAssignment", line: 5, From 7e2692e937478a6256bc452e9488a57f258af9b7 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Tue, 10 Oct 2023 00:01:51 +0900 Subject: [PATCH 06/35] fix: add test case and fix --- lib/rules/no-useless-assignment.js | 43 ++++++++++++++++++++---- tests/lib/rules/no-useless-assignment.js | 41 ++++++++++++++++++++++ 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 8e4e84aaddc..1c77095dee2 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -160,13 +160,13 @@ module.exports = { * @param {AssignmentInfo[]} otherAssignments The list of assignment info for variables other than `assignment`. * @returns {void} */ - function verifyAssignment(assignment, otherAssignments) { + function verifyAssignment({ segment, variable, identifier }, otherAssignments) { otherAssignments.sort((a, b) => a.identifier.range[0] - b.identifier.range[0]); const subsequentSegmentData = { results: [], subsequentSegments: new Set(), - queueSegments: [...assignment.segment.nextSegments] + queueSegments: [...segment.nextSegments] }; @@ -199,7 +199,27 @@ module.exports = { } } - const readReferences = assignment.variable.references.filter(reference => reference.isRead()); + let cacheOtherAssignment = null; + + /** + * Get other assignment on the current segment and after current assignment. + * @returns {AssignmentInfo|undefined} other assignment + */ + function getOtherAssignmentAfterAssignment() { + if (cacheOtherAssignment) { + return cacheOtherAssignment.result; + } + cacheOtherAssignment = { + result: otherAssignments + .find(otherAssignment => + otherAssignment.segment === segment && + identifier.range[0] < otherAssignment.identifier.range[0]) + }; + + return cacheOtherAssignment.result; + } + + const readReferences = variable.references.filter(reference => reference.isRead()); if (!readReferences.length) { @@ -219,10 +239,21 @@ module.exports = { } if ( - assignment.identifier.range[0] < reference.identifier.range[0] && - isInSegment(assignment.segment, reference.identifier) + identifier.range[0] < reference.identifier.range[0] && + isInSegment(segment, reference.identifier) ) { + const otherAssignment = getOtherAssignmentAfterAssignment(); + + if ( + otherAssignment && + otherAssignment.identifier.range[0] < reference.identifier.range[0] + ) { + + // There was another assignment before the reference. Therefore, it has not been used yet. + continue; + } + // Uses in statements after the written identifier. return; } @@ -245,7 +276,7 @@ module.exports = { } } context.report({ - node: assignment.identifier, + node: identifier, messageId: "unnecessaryAssignment" }); } diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index 186dd41bc76..1d446b3bea6 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -347,6 +347,47 @@ ruleTester.run("no-useless-assignment", rule, { } ] }, + { + code: ` + let v; + v = 'unused'; + if (foo) { + v = 'used'; + } else { + v = 'used'; + } + console.log(v);`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 3, + column: 13 + } + ] + }, + { + code: + `function foo() { + let v = 'used'; + console.log(v); + v = 'unused'; + v = 'unused'; + v = 'used'; + console.log(v); + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + }, + { + messageId: "unnecessaryAssignment", + line: 5, + column: 17 + } + ] + }, // Update { From 6bb7f65ecf74a272695c9005b393ce5a48469d18 Mon Sep 17 00:00:00 2001 From: Yosuke Ota Date: Tue, 10 Oct 2023 19:25:09 +0900 Subject: [PATCH 07/35] Apply suggestions from code review Co-authored-by: Nicholas C. Zakas --- lib/rules/no-useless-assignment.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 1c77095dee2..76b6688ff57 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -63,10 +63,10 @@ function *extractIdentifiersFromPattern(pattern) { /** @type {import('../shared/types').Rule} */ module.exports = { meta: { - type: "suggestion", + type: "problem", docs: { - description: "Disallow variables that are not used after assignment", + description: "Disallow variable assignments when the value is not used", recommended: false, url: "https://eslint.org/docs/latest/rules/no-useless-assignment" }, From fb050c30d8a82f01401e14a207ea4069ce5a95be Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Tue, 10 Oct 2023 21:58:54 +0900 Subject: [PATCH 08/35] docs: update --- docs/src/rules/no-useless-assignment.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/rules/no-useless-assignment.md b/docs/src/rules/no-useless-assignment.md index 9636ec66254..2ae4b6d5991 100644 --- a/docs/src/rules/no-useless-assignment.md +++ b/docs/src/rules/no-useless-assignment.md @@ -11,7 +11,7 @@ If the variable is not used after assignment is needless and most likely indicat ## Rule Details -This rule aims to report variables that are not used after assignment. +This rule aims to report variable assignments when the value is not used. Examples of **incorrect** code for this rule: From eb5e98232cac7e31892ed0b71caa022e62f5540d Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Fri, 20 Oct 2023 10:08:29 +0900 Subject: [PATCH 09/35] fix: false negatives for same segments & refactor --- lib/rules/no-useless-assignment.js | 127 +++++++++++++++-------- tests/lib/rules/no-useless-assignment.js | 103 +++++++++++++++++- 2 files changed, 185 insertions(+), 45 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 76b6688ff57..03aeb863b27 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -95,7 +95,6 @@ module.exports = { * @property {CodePathSegment} segment The code path segment. * @property {Identifier|null} first The first identifier that appears within the segment. * @property {Identifier|null} last The last identifier that appears within the segment. - * * `first` and `last` are used to determine whether an identifier exists within the segment position range. * Since it is used as a range of segments, we should originally hold all nodes, not just identifiers, * but since the only nodes to be judged are identifiers, it is sufficient to have a range of identifiers. @@ -106,6 +105,7 @@ module.exports = { * @property {Identifier} identifier The identifier that is assigned. * @property {CodePathSegment} segment The code path segment where the assignment was made. */ + /** @type {ScopeStack} */ let scopeStack = null; @@ -127,7 +127,9 @@ module.exports = { } target = target.upper; } - throw new Error("Should be unreachable"); + + // Should be unreachable + return null; } /** @@ -156,23 +158,51 @@ module.exports = { /** * Verify the given assignment info. - * @param {AssignmentInfo} assignment The assignment info to verify. - * @param {AssignmentInfo[]} otherAssignments The list of assignment info for variables other than `assignment`. + * @param {AssignmentInfo} targetAssignment The assignment info to verify. + * @param {AssignmentInfo[]} allAssignments The list of all assignment info for variables. * @returns {void} */ - function verifyAssignment({ segment, variable, identifier }, otherAssignments) { - otherAssignments.sort((a, b) => a.identifier.range[0] - b.identifier.range[0]); + function verifyAssignment(targetAssignment, allAssignments) { + + /** + * @typedef {Object} SubsequentSegmentData + * @property {CodePathSegment} segment The code path segment + * @property {AssignmentInfo} [assignment] The first occurrence of the assignment within the segment. + * There is no need to check if the variable is used after this assignment, + * as the value it was assigned will be used. + */ + /** + * Information used in `getSubsequentSegments()`. + * To avoid unnecessary iterations, cache information that has already been iterated over, + * and if additional iterations are needed, start iterating from the retained position. + */ const subsequentSegmentData = { + + /** + * Cache of subsequent segment information list that have already been iterated. + * @type {SubsequentSegmentData[]} + */ results: [], + + /** + * Subsequent segments that have already been iterated on. Used to avoid infinite loops. + * @type {Set} + */ subsequentSegments: new Set(), - queueSegments: [...segment.nextSegments] + + /** + * Unexplored code path segment. + * If additional iterations are needed, consume this information and iterate. + * @type {CodePathSegment[]} + */ + queueSegments: [...targetAssignment.segment.nextSegments] }; /** * Gets the subsequent segments from the current segment. - * @returns {Iterable<{ segment: CodePathSegment, assignment?: AssignmentInfo }>} the subsequent segments + * @returns {Iterable} the subsequent segments */ function *getSubsequentSegments() { yield* subsequentSegmentData.results; @@ -185,48 +215,47 @@ module.exports = { } subsequentSegmentData.subsequentSegments.add(nextSegment); - const result = { - segment: nextSegment, - assignment: otherAssignments.find(otherAssignment => otherAssignment.segment === nextSegment) - }; + const assignmentInSegment = allAssignments.find(otherAssignment => otherAssignment.segment === nextSegment); - if (!result.assignment) { + if (!assignmentInSegment) { + + /* + * Stores the next segment to explore. + * If `assignmentInSegment` exists, + * we are guarding it because we don't need to explore the next segment. + */ subsequentSegmentData.queueSegments.push(...nextSegment.nextSegments); } + /** @type {SubsequentSegmentData} */ + const result = { + segment: nextSegment, + assignment: assignmentInSegment + }; + subsequentSegmentData.results.push(result); yield result; } } - let cacheOtherAssignment = null; - /** - * Get other assignment on the current segment and after current assignment. - * @returns {AssignmentInfo|undefined} other assignment - */ - function getOtherAssignmentAfterAssignment() { - if (cacheOtherAssignment) { - return cacheOtherAssignment.result; - } - cacheOtherAssignment = { - result: otherAssignments - .find(otherAssignment => - otherAssignment.segment === segment && - identifier.range[0] < otherAssignment.identifier.range[0]) - }; - - return cacheOtherAssignment.result; - } - - const readReferences = variable.references.filter(reference => reference.isRead()); + const readReferences = targetAssignment.variable.references.filter(reference => reference.isRead()); if (!readReferences.length) { - // Maybe unused variable + // It is unused variable return; } + /** + * Other assignment on the current segment and after current assignment. + */ + const otherAssignmentAfterTargetAssignment = allAssignments + .find(assignment => + assignment !== targetAssignment && + assignment.segment === targetAssignment.segment && + targetAssignment.identifier.range[0] < assignment.identifier.range[0]); + for (const reference of readReferences) { /* @@ -238,16 +267,15 @@ module.exports = { return; } + // Checks if it is used in the same segment as the target assignment. if ( - identifier.range[0] < reference.identifier.range[0] && - isInSegment(segment, reference.identifier) + targetAssignment.identifier.range[0] < reference.identifier.range[0] && + isInSegment(targetAssignment.segment, reference.identifier) ) { - const otherAssignment = getOtherAssignmentAfterAssignment(); - if ( - otherAssignment && - otherAssignment.identifier.range[0] < reference.identifier.range[0] + otherAssignmentAfterTargetAssignment && + otherAssignmentAfterTargetAssignment.identifier.range[0] < reference.identifier.range[0] ) { // There was another assignment before the reference. Therefore, it has not been used yet. @@ -258,6 +286,15 @@ module.exports = { return; } + if (otherAssignmentAfterTargetAssignment) { + + /* + * The assignment was followed by another assignment in the same segment. + * Therefore, there is no need to check the next segment. + */ + continue; + } + // Check subsequent segments. for (const subsequentSegment of getSubsequentSegments()) { if (isInSegment(subsequentSegment.segment, reference.identifier)) { @@ -276,15 +313,17 @@ module.exports = { } } context.report({ - node: identifier, + node: targetAssignment.identifier, messageId: "unnecessaryAssignment" }); } + // Verify that each assignment in the code path is used. for (const assignments of target.assignments.values()) { - assignments.forEach((assignment, index) => { - verifyAssignment(assignment, [...assignments.slice(0, index), ...assignments.slice(index + 1)]); - }); + assignments.sort((a, b) => a.identifier.range[0] - b.identifier.range[0]); + for (const assignment of assignments) { + verifyAssignment(assignment, assignments); + } } } diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index 1d446b3bea6..c7843d68dd1 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -52,6 +52,15 @@ ruleTester.run("no-useless-assignment", rule, { console.log(v); } }`, + `function foo() { + let v = 'used'; + if (condition) { + // + } else { + v = 'used-2'; + } + console.log(v); + }`, `var foo = function () { let v = 'used'; console.log(v); @@ -128,6 +137,9 @@ ruleTester.run("no-useless-assignment", rule, { console.log(v); v = 'unused'`, + // Unused variable + "let v = 'used variable';", + // Update `function foo() { let a = 42; @@ -347,6 +359,25 @@ ruleTester.run("no-useless-assignment", rule, { } ] }, + { + code: + `function foo() { + let v = 'used'; + console.log(v); + v = 'unused'; + v = 'used'; + console.log(v); + v = 'used'; + console.log(v); + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + } + ] + }, { code: ` let v; @@ -388,6 +419,77 @@ ruleTester.run("no-useless-assignment", rule, { } ] }, + { + code: + `function foo() { + let v = 'unused'; + if (condition) { + if (condition2) { + v = 'used-2'; + } else { + v = 'used-3'; + } + } else { + v = 'used-4'; + } + console.log(v); + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 2, + column: 21 + } + ] + }, + { + code: + `function foo() { + let v; + if (condition) { + v = 'unused'; + } else { + // + } + if (condition2) { + v = 'used-1'; + } else { + v = 'used-2'; + } + console.log(v); + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 21 + } + ] + }, + { + code: + `function foo() { + let v = 'used'; + if (condition) { + v = 'unused'; + v = 'unused'; + v = 'used'; + } + console.log(v); + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 21 + }, + { + messageId: "unnecessaryAssignment", + line: 5, + column: 21 + } + ] + }, // Update { @@ -630,6 +732,5 @@ ruleTester.run("no-useless-assignment", rule, { } ] } - ] }); From c8ab1281b9ffe5e1631ed26a714fedf1d5273236 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Fri, 20 Oct 2023 10:14:18 +0900 Subject: [PATCH 10/35] fix: rule-types.json --- tools/rule-types.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/rule-types.json b/tools/rule-types.json index 7f95e69e128..8a021f04556 100644 --- a/tools/rule-types.json +++ b/tools/rule-types.json @@ -216,7 +216,7 @@ "no-unused-private-class-members": "problem", "no-unused-vars": "problem", "no-use-before-define": "problem", - "no-useless-assignment": "suggestion", + "no-useless-assignment": "problem", "no-useless-backreference": "problem", "no-useless-call": "suggestion", "no-useless-catch": "suggestion", From d3c122378571f465f2cf4f7fe452605577e30b66 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Tue, 24 Oct 2023 08:12:35 +0900 Subject: [PATCH 11/35] fix: add update assignment test case & fix --- lib/rules/no-useless-assignment.js | 9 +++++++-- tests/lib/rules/no-useless-assignment.js | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 03aeb863b27..250004fad24 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -14,6 +14,9 @@ const { findVariable } = require("@eslint-community/eslint-utils"); /** @typedef {import("estree").Node} ASTNode */ /** @typedef {import("estree").Pattern} Pattern */ /** @typedef {import("estree").Identifier} Identifier */ +/** @typedef {import("estree").VariableDeclarator} VariableDeclarator */ +/** @typedef {import("estree").AssignmentExpression} AssignmentExpression */ +/** @typedef {import("estree").UpdateExpression} UpdateExpression */ /** @typedef {import("eslint-scope").Scope} Scope */ /** @typedef {import("eslint-scope").Variable} Variable */ /** @typedef {import("../linter/code-path-analysis/code-path")} CodePath */ @@ -103,6 +106,7 @@ module.exports = { * @typedef {Object} AssignmentInfo * @property {Variable} variable The variable that is assigned. * @property {Identifier} identifier The identifier that is assigned. + * @property {VariableDeclarator|AssignmentExpression|UpdateExpression} node The node where the variable was updated. * @property {CodePathSegment} segment The code path segment where the assignment was made. */ @@ -275,7 +279,7 @@ module.exports = { if ( otherAssignmentAfterTargetAssignment && - otherAssignmentAfterTargetAssignment.identifier.range[0] < reference.identifier.range[0] + otherAssignmentAfterTargetAssignment.node.range[1] <= reference.identifier.range[0] ) { // There was another assignment before the reference. Therefore, it has not been used yet. @@ -300,7 +304,7 @@ module.exports = { if (isInSegment(subsequentSegment.segment, reference.identifier)) { if ( subsequentSegment.assignment && - subsequentSegment.assignment.identifier.range[0] < reference.identifier.range[0] + subsequentSegment.assignment.node.range[1] <= reference.identifier.range[0] ) { // There was another assignment before the reference. Therefore, it has not been used yet. @@ -449,6 +453,7 @@ module.exports = { list.push({ variable, identifier, + node, segment: segmentInfo.segment }); } diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index c7843d68dd1..ecead66d4ab 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -154,6 +154,26 @@ ruleTester.run("no-useless-assignment", rule, { console.log(a); }`, + // Assign with update + `function foo() { + let a = 42; + console.log(a); + a = 10; + a = a + 1; + console.log(a); + }`, + `function foo() { + let a = 42; + console.log(a); + a = 10; + if (cond) { + a = a + 1; + } else { + a = 2 + a; + } + console.log(a); + }`, + // Assign to complex patterns `function foo() { let a = 'used', b = 'used', c = 'used', d = 'used'; From 96dd2acb9dd479ae654b202a7190cdddbed9e14f Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Tue, 24 Oct 2023 08:36:37 +0900 Subject: [PATCH 12/35] fix: add test for global vars & fix --- lib/rules/no-useless-assignment.js | 5 +++++ tests/lib/rules/no-useless-assignment.js | 23 ++++++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 250004fad24..1161375114e 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -400,6 +400,11 @@ module.exports = { continue; } + // We don't know where global variables are used. + if (variable.scope.type === "global" && variable.defs.length === 0) { + continue; + } + /* * If the scope of the variable is outside the current code path scope, * we cannot track whether this assignment is not used. diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index ecead66d4ab..ec258abaaca 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -226,7 +226,28 @@ ruleTester.run("no-useless-assignment", rule, { } console.log(v); } - }` + }`, + + // Ignore known globals + `/* globals foo */ + const bk = foo; + foo = 42; + try { + // process + } finally { + foo = bk; + }`, + { + code: ` + const bk = console; + console = { log () {} }; + try { + // process + } finally { + console = bk; + }`, + env: { browser: true } + } ], invalid: [ { From 1a2efc0c5764844793be330436af9d8710d3207c Mon Sep 17 00:00:00 2001 From: Yosuke Ota Date: Sun, 5 Nov 2023 13:06:39 +0900 Subject: [PATCH 13/35] Update docs/src/rules/no-useless-assignment.md Co-authored-by: Nicholas C. Zakas --- docs/src/rules/no-useless-assignment.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/rules/no-useless-assignment.md b/docs/src/rules/no-useless-assignment.md index 2ae4b6d5991..6f781a17aa8 100644 --- a/docs/src/rules/no-useless-assignment.md +++ b/docs/src/rules/no-useless-assignment.md @@ -113,4 +113,4 @@ function foo () { ## When Not To Use It -If you don't want to be notified about unnecessary arguments, you can safely turn this rule off. +If you don't want to be notified about values that are never read, you can safely disable this rule. From e21b29218c821eaeeeceab00cf7b95b5e5647e8f Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sun, 5 Nov 2023 13:01:07 +0900 Subject: [PATCH 14/35] docs: update description --- docs/src/rules/no-useless-assignment.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/src/rules/no-useless-assignment.md b/docs/src/rules/no-useless-assignment.md index 6f781a17aa8..c2b5f29c3ce 100644 --- a/docs/src/rules/no-useless-assignment.md +++ b/docs/src/rules/no-useless-assignment.md @@ -6,8 +6,13 @@ related_rules: --- +[Wikipedia describes a "dead store"](https://en.wikipedia.org/wiki/Dead_store) as follows: -If the variable is not used after assignment is needless and most likely indicates that there is some behavior that the author wanted but didn't correctly do. +> In computer programming, a local variable that is assigned a value but is not read by any subsequent instruction is referred to as a **dead store**. + +"Dead stores" waste processing and memory, so it is better to remove unnecessary assignments to variables. + +Also, if the author intended the variable to be used, there is likely a mistake around the dead store. ## Rule Details From 06f9698bc0bff9086c0ffa6228d4815e46b2d5de Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sun, 5 Nov 2023 13:07:33 +0900 Subject: [PATCH 15/35] docs: add further_reading --- docs/src/_data/further_reading_links.json | 56 +++++++++++++++++++++++ docs/src/rules/no-useless-assignment.md | 9 ++++ 2 files changed, 65 insertions(+) diff --git a/docs/src/_data/further_reading_links.json b/docs/src/_data/further_reading_links.json index a33ce6c0b3b..327a0ae9f96 100644 --- a/docs/src/_data/further_reading_links.json +++ b/docs/src/_data/further_reading_links.json @@ -747,5 +747,61 @@ "logo": "https://codepoints.net/favicon.ico", "title": "U+1680 OGHAM SPACE MARK:   – Unicode – Codepoints", "description": " , codepoint U+1680 OGHAM SPACE MARK in Unicode, is located in the block “Ogham”. It belongs to the Ogham script and is a Space Separator." + }, + "https://en.wikipedia.org/wiki/Dead_store": { + "domain": "en.wikipedia.org", + "url": "https://en.wikipedia.org/wiki/Dead_store", + "logo": "https://en.wikipedia.org/static/apple-touch/wikipedia.png", + "title": "Dead store - Wikipedia", + "description": null + }, + "https://www.npmjs.com/package/eslint-plugin-no-useless-assign": { + "domain": "www.npmjs.com", + "url": "https://www.npmjs.com/package/eslint-plugin-no-useless-assign", + "logo": "https://static-production.npmjs.com/1996fcfdf7ca81ea795f67f093d7f449.png", + "title": "eslint-plugin-no-useless-assign", + "description": "Prevent useless assignment.. Latest version: 1.0.3, last published: 4 years ago. Start using eslint-plugin-no-useless-assign in your project by running `npm i eslint-plugin-no-useless-assign`. There are 5 other projects in the npm registry using eslint-plugin-no-useless-assign." + }, + "https://sonarsource.atlassian.net/browse/RSPEC-1854": { + "domain": "sonarsource.atlassian.net", + "url": "https://sonarsource.atlassian.net/browse/RSPEC-1854", + "logo": "https://sonarsource.atlassian.net/images/jira-safari-pintab-icon.svg", + "title": "[RSPEC-1854] - Jira", + "description": null + }, + "https://rules.sonarsource.com/javascript/RSPEC-1854/": { + "domain": "rules.sonarsource.com", + "url": "https://rules.sonarsource.com/javascript/RSPEC-1854/", + "logo": "https://rules.sonarsource.com/favicon.ico", + "title": "JavaScript static code analysis: Unused assignments should be removed", + "description": "Dead stores refer to assignments made to local variables that are subsequently never used or immediately overwritten. Such assignments are\nunnecessary and don’t contribute to the functionality or clarity of the code. They may even negatively impact performance. Removing them enhances code\ncleanlines…" + }, + "https://rules.sonarsource.com/typescript/RSPEC-1854/": { + "domain": "rules.sonarsource.com", + "url": "https://rules.sonarsource.com/typescript/RSPEC-1854/", + "logo": "https://rules.sonarsource.com/favicon.ico", + "title": "TypeScript static code analysis: Unused assignments should be removed", + "description": "Dead stores refer to assignments made to local variables that are subsequently never used or immediately overwritten. Such assignments are\nunnecessary and don’t contribute to the functionality or clarity of the code. They may even negatively impact performance. Removing them enhances code\ncleanlines…" + }, + "https://cwe.mitre.org/data/definitions/563.html": { + "domain": "cwe.mitre.org", + "url": "https://cwe.mitre.org/data/definitions/563.html", + "logo": "https://cwe.mitre.org/favicon.ico", + "title": "CWE - CWE-563: Assignment to Variable without Use (4.13)", + "description": "Common Weakness Enumeration (CWE) is a list of software weaknesses." + }, + "https://wiki.sei.cmu.edu/confluence/display/c/MSC13-C.+Detect+and+remove+unused+values": { + "domain": "wiki.sei.cmu.edu", + "url": "https://wiki.sei.cmu.edu/confluence/display/c/MSC13-C.+Detect+and+remove+unused+values", + "logo": "https://wiki.sei.cmu.edu/confluence/s/-ctumb3/9012/tu5x00/7/_/favicon.ico", + "title": "MSC13-C. Detect and remove unused values - SEI CERT C Coding Standard - Confluence", + "description": null + }, + "https://wiki.sei.cmu.edu/confluence/display/java/MSC56-J.+Detect+and+remove+superfluous+code+and+values": { + "domain": "wiki.sei.cmu.edu", + "url": "https://wiki.sei.cmu.edu/confluence/display/java/MSC56-J.+Detect+and+remove+superfluous+code+and+values", + "logo": "https://wiki.sei.cmu.edu/confluence/s/-ctumb3/9012/tu5x00/7/_/favicon.ico", + "title": "MSC56-J. Detect and remove superfluous code and values - SEI CERT Oracle Coding Standard for Java - Confluence", + "description": null } } \ No newline at end of file diff --git a/docs/src/rules/no-useless-assignment.md b/docs/src/rules/no-useless-assignment.md index c2b5f29c3ce..58b411a014d 100644 --- a/docs/src/rules/no-useless-assignment.md +++ b/docs/src/rules/no-useless-assignment.md @@ -3,6 +3,15 @@ title: no-useless-assignment rule_type: suggestion related_rules: - no-unused-vars +further_reading: +- https://en.wikipedia.org/wiki/Dead_store +- https://www.npmjs.com/package/eslint-plugin-no-useless-assign +- https://sonarsource.atlassian.net/browse/RSPEC-1854 +- https://rules.sonarsource.com/javascript/RSPEC-1854/ +- https://rules.sonarsource.com/typescript/RSPEC-1854/ +- https://cwe.mitre.org/data/definitions/563.html +- https://wiki.sei.cmu.edu/confluence/display/c/MSC13-C.+Detect+and+remove+unused+values +- https://wiki.sei.cmu.edu/confluence/display/java/MSC56-J.+Detect+and+remove+superfluous+code+and+values --- From a68ab8d5974a7017ae166bac1a3d681c3df05dfc Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sun, 5 Nov 2023 23:34:55 +0900 Subject: [PATCH 16/35] chore: improve jsdoc and rename function --- lib/rules/no-useless-assignment.js | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 1161375114e..eb90203fa44 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -149,7 +149,7 @@ module.exports = { * @param {Identifier} identifier The identifier to check. * @returns {boolean} `true` if the identifier is used in the segment. */ - function isInSegment(segment, identifier) { + function isIdentifierUsedInSegment(segment, identifier) { const segmentInfo = target.segments[segment.id]; return ( @@ -161,12 +161,13 @@ module.exports = { } /** - * Verify the given assignment info. + * Verifies whether the given assignment info is an unused assignment. + * Report if it is an unused assignment. * @param {AssignmentInfo} targetAssignment The assignment info to verify. * @param {AssignmentInfo[]} allAssignments The list of all assignment info for variables. * @returns {void} */ - function verifyAssignment(targetAssignment, allAssignments) { + function verifyAssignmentInfoIsUnused(targetAssignment, allAssignments) { /** * @typedef {Object} SubsequentSegmentData @@ -205,7 +206,8 @@ module.exports = { /** - * Gets the subsequent segments from the current segment. + * Gets the subsequent segments from the segment of + * the assignment currently being validated (targetAssignment). * @returns {Iterable} the subsequent segments */ function *getSubsequentSegments() { @@ -247,7 +249,10 @@ module.exports = { if (!readReferences.length) { - // It is unused variable + /* + * It is not an unnecessary assignment, it is clearly an unnecessary (unused) variable + * and should not be reported in the this rule. And it is reported by `no-unused-vars`. + */ return; } @@ -274,7 +279,7 @@ module.exports = { // Checks if it is used in the same segment as the target assignment. if ( targetAssignment.identifier.range[0] < reference.identifier.range[0] && - isInSegment(targetAssignment.segment, reference.identifier) + isIdentifierUsedInSegment(targetAssignment.segment, reference.identifier) ) { if ( @@ -301,7 +306,7 @@ module.exports = { // Check subsequent segments. for (const subsequentSegment of getSubsequentSegments()) { - if (isInSegment(subsequentSegment.segment, reference.identifier)) { + if (isIdentifierUsedInSegment(subsequentSegment.segment, reference.identifier)) { if ( subsequentSegment.assignment && subsequentSegment.assignment.node.range[1] <= reference.identifier.range[0] @@ -326,7 +331,7 @@ module.exports = { for (const assignments of target.assignments.values()) { assignments.sort((a, b) => a.identifier.range[0] - b.identifier.range[0]); for (const assignment of assignments) { - verifyAssignment(assignment, assignments); + verifyAssignmentInfoIsUnused(assignment, assignments); } } } From 09a2a226f3fcdf714dc231cc92c769e413134907 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sun, 5 Nov 2023 23:44:16 +0900 Subject: [PATCH 17/35] docs: update doc --- docs/src/rules/no-useless-assignment.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/docs/src/rules/no-useless-assignment.md b/docs/src/rules/no-useless-assignment.md index 58b411a014d..5f0ef110fb2 100644 --- a/docs/src/rules/no-useless-assignment.md +++ b/docs/src/rules/no-useless-assignment.md @@ -123,6 +123,21 @@ function foo () { } ``` +This rule will not report variables that are never read. +Because it's clearly an unused variable. If you want it reported, please enable the [no-unused-vars](./no-unused-vars) rule. + +::: correct + +```js +/* eslint no-useless-assignment: "error" */ + +function foo() { + let v = 'unused'; + v = 'unused-2' + doSomething(); +} +``` + ::: ## When Not To Use It From 5c56a3ed4520835492543b61d2a4a7cbb506d8e8 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Mon, 6 Nov 2023 23:37:29 +0900 Subject: [PATCH 18/35] fix: false positives for try catch --- lib/rules/no-useless-assignment.js | 2 +- tests/lib/rules/no-useless-assignment.js | 115 +++++++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index eb90203fa44..ff4eb9f1063 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -381,7 +381,7 @@ module.exports = { } segmentInfo.last = node; }, - "VariableDeclarator[init!=null], AssignmentExpression, UpdateExpression"(node) { + ":matches(VariableDeclarator[init!=null], AssignmentExpression, UpdateExpression):exit"(node) { const segmentInfo = scopeStack.currentSegments[0]; const assignments = scopeStack.assignments; diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index ec258abaaca..e493ec7e9c6 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -247,7 +247,47 @@ ruleTester.run("no-useless-assignment", rule, { console = bk; }`, env: { browser: true } + }, + + // Try catch + `let message = 'init'; + try { + const result = call(); + message = result.message; + } catch (e) { + // ignore + } + console.log(message)`, + `let message = 'init'; + try { + message = call().message; + } catch (e) { + // ignore + } + console.log(message)`, + `let v = 'init'; + try { + v = callA(); + try { + v = callB(); + } catch (e) { + // ignore + } + } catch (e) { + // ignore } + console.log(v)`, + `let v = 'init'; + try { + try { + v = callA(); + } catch (e) { + // ignore + } + } catch (e) { + // ignore + } + console.log(v)` ], invalid: [ { @@ -772,6 +812,81 @@ ruleTester.run("no-useless-assignment", rule, { column: 25 } ] + }, + + // Try catch + { + code: + `let message = 'unused'; + try { + const result = call(); + message = result.message; + } catch (e) { + message = 'used'; + } + console.log(message)`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 1, + column: 5 + } + ] + }, + { + code: + `let message = 'unused'; + try { + message = 'used'; + console.log(message) + } catch (e) { + }`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 1, + column: 5 + } + ] + }, + { + code: + `let message = 'unused'; + try { + message = call(); + } catch (e) { + message = 'used'; + } + console.log(message)`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 1, + column: 5 + } + ] + }, + { + code: + `let v = 'unused'; + try { + v = callA(); + try { + v = callB(); + } catch (e) { + // ignore + } + } catch (e) { + v = 'used'; + } + console.log(v)`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 1, + column: 5 + } + ] } ] }); From 6ca9835c7fba88b5aa8eb7affc81143c7b54a03a Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Mon, 6 Nov 2023 23:46:54 +0900 Subject: [PATCH 19/35] test: fix tests/conf/eslint-all.js --- tests/conf/eslint-all.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conf/eslint-all.js b/tests/conf/eslint-all.js index 6475965a02a..4124fc9e9ed 100644 --- a/tests/conf/eslint-all.js +++ b/tests/conf/eslint-all.js @@ -31,7 +31,7 @@ describe("eslint-all", () => { const someRule = "yoda"; assert.include(ruleNames, someRule); - assert.isBelow(count, 200); + assert.isAtLeast(count, 200); }); it("should configure all rules as errors", () => { From 153e5dca1727c614a0ab52973e58af71e257af4e Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Tue, 7 Nov 2023 00:05:02 +0900 Subject: [PATCH 20/35] feat: add no-useless-assignment to eslint-config-eslint --- lib/linter/code-path-analysis/code-path.js | 6 +++--- lib/linter/config-comment-parser.js | 17 +++++++---------- lib/linter/linter.js | 2 +- lib/rules/array-bracket-newline.js | 2 +- lib/rules/indent-legacy.js | 2 +- lib/rules/indent.js | 2 +- lib/rules/key-spacing.js | 2 +- lib/rules/max-len.js | 2 +- lib/rules/no-dupe-class-members.js | 2 +- lib/rules/no-empty-function.js | 4 ++-- lib/rules/no-loss-of-precision.js | 2 +- lib/rules/no-trailing-spaces.js | 5 ++--- lib/rules/utils/ast-utils.js | 4 ++-- lib/rules/yield-star-spacing.js | 2 +- lib/source-code/token-store/index.js | 4 ++-- packages/eslint-config-eslint/base.js | 1 + packages/eslint-config-eslint/eslintrc.js | 1 + 17 files changed, 29 insertions(+), 31 deletions(-) diff --git a/lib/linter/code-path-analysis/code-path.js b/lib/linter/code-path-analysis/code-path.js index 3bf570d754b..5f850d50435 100644 --- a/lib/linter/code-path-analysis/code-path.js +++ b/lib/linter/code-path-analysis/code-path.js @@ -180,9 +180,9 @@ class CodePath { const lastSegment = resolvedOptions.last; // set up initial location information - let record = null; - let index = 0; - let end = 0; + let record; + let index; + let end; let segment = null; // segments that have already been visited during traversal diff --git a/lib/linter/config-comment-parser.js b/lib/linter/config-comment-parser.js index cde261204f5..8effcaf10bf 100644 --- a/lib/linter/config-comment-parser.js +++ b/lib/linter/config-comment-parser.js @@ -72,11 +72,9 @@ module.exports = class ConfigCommentParser { parseJsonConfig(string, location) { debug("Parsing JSON config"); - let items = {}; - // Parses a JSON-like comment by the same way as parsing CLI option. try { - items = levn.parse("Object", string) || {}; + const items = levn.parse("Object", string) || {}; // Some tests say that it should ignore invalid comments such as `/*eslint no-alert:abc*/`. // Also, commaless notations have invalid severity: @@ -99,11 +97,15 @@ module.exports = class ConfigCommentParser { * Optionator cannot parse commaless notations. * But we are supporting that. So this is a fallback for that. */ - items = {}; const normalizedString = string.replace(/([-a-zA-Z0-9/]+):/gu, "\"$1\":").replace(/(\]|[0-9])\s+(?=")/u, "$1,"); try { - items = JSON.parse(`{${normalizedString}}`); + const items = JSON.parse(`{${normalizedString}}`); + + return { + success: true, + config: items + }; } catch (ex) { debug("Manual parsing failed."); @@ -121,11 +123,6 @@ module.exports = class ConfigCommentParser { }; } - - return { - success: true, - config: items - }; } /** diff --git a/lib/linter/linter.js b/lib/linter/linter.js index e195812e513..eeb3a8cb6df 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -2059,7 +2059,7 @@ class Linter { * SourceCodeFixer. */ verifyAndFix(text, config, options) { - let messages = [], + let messages, fixedResult, fixed = false, passNumber = 0, diff --git a/lib/rules/array-bracket-newline.js b/lib/rules/array-bracket-newline.js index 12ef5b612d6..9eb725825a9 100644 --- a/lib/rules/array-bracket-newline.js +++ b/lib/rules/array-bracket-newline.js @@ -74,7 +74,7 @@ module.exports = { function normalizeOptionValue(option) { let consistent = false; let multiline = false; - let minItems = 0; + let minItems; if (option) { if (option === "consistent") { diff --git a/lib/rules/indent-legacy.js b/lib/rules/indent-legacy.js index 78bf965cb3f..a1d4c3a232e 100644 --- a/lib/rules/indent-legacy.js +++ b/lib/rules/indent-legacy.js @@ -830,7 +830,7 @@ module.exports = { } let indent; - let nodesToCheck = []; + let nodesToCheck; /* * For this statements we should check indent from statement beginning, diff --git a/lib/rules/indent.js b/lib/rules/indent.js index 9bcbd640c4d..df62017244d 100644 --- a/lib/rules/indent.js +++ b/lib/rules/indent.js @@ -1407,7 +1407,7 @@ module.exports = { PropertyDefinition(node) { const firstToken = sourceCode.getFirstToken(node); const maybeSemicolonToken = sourceCode.getLastToken(node); - let keyLastToken = null; + let keyLastToken; // Indent key. if (node.computed) { diff --git a/lib/rules/key-spacing.js b/lib/rules/key-spacing.js index 19fc0167ae0..33fa29943aa 100644 --- a/lib/rules/key-spacing.js +++ b/lib/rules/key-spacing.js @@ -489,7 +489,7 @@ module.exports = { } } - let messageId = ""; + let messageId; if (isExtra) { messageId = side === "key" ? "extraKey" : "extraValue"; diff --git a/lib/rules/max-len.js b/lib/rules/max-len.js index 138a0f239fd..3a3ab65e27f 100644 --- a/lib/rules/max-len.js +++ b/lib/rules/max-len.js @@ -344,7 +344,7 @@ module.exports = { * comments to check. */ if (commentsIndex < comments.length) { - let comment = null; + let comment; // iterate over comments until we find one past the current line do { diff --git a/lib/rules/no-dupe-class-members.js b/lib/rules/no-dupe-class-members.js index 2a7a9e810dc..ffa2e46325d 100644 --- a/lib/rules/no-dupe-class-members.js +++ b/lib/rules/no-dupe-class-members.js @@ -82,7 +82,7 @@ module.exports = { } const state = getState(name, node.static); - let isDuplicate = false; + let isDuplicate; if (kind === "get") { isDuplicate = (state.init || state.get); diff --git a/lib/rules/no-empty-function.js b/lib/rules/no-empty-function.js index 2fcb75534ac..b7dee94c4ea 100644 --- a/lib/rules/no-empty-function.js +++ b/lib/rules/no-empty-function.js @@ -40,7 +40,7 @@ const ALLOW_OPTIONS = Object.freeze([ */ function getKind(node) { const parent = node.parent; - let kind = ""; + let kind; if (node.type === "ArrowFunctionExpression") { return "arrowFunctions"; @@ -73,7 +73,7 @@ function getKind(node) { } // Detects prefix. - let prefix = ""; + let prefix; if (node.generator) { prefix = "generator"; diff --git a/lib/rules/no-loss-of-precision.js b/lib/rules/no-loss-of-precision.js index b3635e3d509..c50d8a89055 100644 --- a/lib/rules/no-loss-of-precision.js +++ b/lib/rules/no-loss-of-precision.js @@ -64,7 +64,7 @@ module.exports = { */ function notBaseTenLosesPrecision(node) { const rawString = getRaw(node).toUpperCase(); - let base = 0; + let base; if (rawString.startsWith("0B")) { base = 2; diff --git a/lib/rules/no-trailing-spaces.js b/lib/rules/no-trailing-spaces.js index eede46c8634..c188b1fa899 100644 --- a/lib/rules/no-trailing-spaces.js +++ b/lib/rules/no-trailing-spaces.js @@ -129,8 +129,7 @@ module.exports = { comments = sourceCode.getAllComments(), commentLineNumbers = getCommentLineNumbers(comments); - let totalLength = 0, - fixRange = []; + let totalLength = 0; for (let i = 0, ii = lines.length; i < ii; i++) { const lineNumber = i + 1; @@ -177,7 +176,7 @@ module.exports = { continue; } - fixRange = [rangeStart, rangeEnd]; + const fixRange = [rangeStart, rangeEnd]; if (!ignoreComments || !commentLineNumbers.has(lineNumber)) { report(node, location, fixRange); diff --git a/lib/rules/utils/ast-utils.js b/lib/rules/utils/ast-utils.js index bebb4d5168b..434c908e52d 100644 --- a/lib/rules/utils/ast-utils.js +++ b/lib/rules/utils/ast-utils.js @@ -1801,8 +1801,8 @@ module.exports = { */ getFunctionHeadLoc(node, sourceCode) { const parent = node.parent; - let start = null; - let end = null; + let start; + let end; if (parent.type === "Property" || parent.type === "MethodDefinition" || parent.type === "PropertyDefinition") { start = parent.loc.start; diff --git a/lib/rules/yield-star-spacing.js b/lib/rules/yield-star-spacing.js index 9a67b78d25f..5cf3804abb8 100644 --- a/lib/rules/yield-star-spacing.js +++ b/lib/rules/yield-star-spacing.js @@ -79,7 +79,7 @@ module.exports = { const after = leftToken.value === "*"; const spaceRequired = mode[side]; const node = after ? leftToken : rightToken; - let messageId = ""; + let messageId; if (spaceRequired) { messageId = side === "before" ? "missingBefore" : "missingAfter"; diff --git a/lib/source-code/token-store/index.js b/lib/source-code/token-store/index.js index 46a96b2f4b1..d222c87bbf4 100644 --- a/lib/source-code/token-store/index.js +++ b/lib/source-code/token-store/index.js @@ -37,8 +37,8 @@ function createIndexMap(tokens, comments) { const map = Object.create(null); let tokenIndex = 0; let commentIndex = 0; - let nextStart = 0; - let range = null; + let nextStart; + let range; while (tokenIndex < tokens.length || commentIndex < comments.length) { nextStart = (commentIndex < comments.length) ? comments[commentIndex].range[0] : Number.MAX_SAFE_INTEGER; diff --git a/packages/eslint-config-eslint/base.js b/packages/eslint-config-eslint/base.js index 083336919e1..76091f759aa 100644 --- a/packages/eslint-config-eslint/base.js +++ b/packages/eslint-config-eslint/base.js @@ -163,6 +163,7 @@ const jsConfigs = [js.configs.recommended, { } ], "no-use-before-define": "error", + "no-useless-assignment": "error", "no-useless-call": "error", "no-useless-computed-key": "error", "no-useless-concat": "error", diff --git a/packages/eslint-config-eslint/eslintrc.js b/packages/eslint-config-eslint/eslintrc.js index 31ce7f252c0..18eb626756c 100644 --- a/packages/eslint-config-eslint/eslintrc.js +++ b/packages/eslint-config-eslint/eslintrc.js @@ -320,6 +320,7 @@ module.exports = { } ], "no-use-before-define": "error", + "no-useless-assignment": "error", "no-useless-call": "error", "no-useless-computed-key": "error", "no-useless-concat": "error", From 91c0a2364b99618290d92b44fbdd5bf2464d43b8 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Tue, 14 Nov 2023 10:56:08 +0900 Subject: [PATCH 21/35] docs: fix document --- docs/src/_data/further_reading_links.json | 21 ------------------ docs/src/rules/no-useless-assignment.md | 27 ++++++++++++----------- 2 files changed, 14 insertions(+), 34 deletions(-) diff --git a/docs/src/_data/further_reading_links.json b/docs/src/_data/further_reading_links.json index 327a0ae9f96..db29e79227f 100644 --- a/docs/src/_data/further_reading_links.json +++ b/docs/src/_data/further_reading_links.json @@ -755,20 +755,6 @@ "title": "Dead store - Wikipedia", "description": null }, - "https://www.npmjs.com/package/eslint-plugin-no-useless-assign": { - "domain": "www.npmjs.com", - "url": "https://www.npmjs.com/package/eslint-plugin-no-useless-assign", - "logo": "https://static-production.npmjs.com/1996fcfdf7ca81ea795f67f093d7f449.png", - "title": "eslint-plugin-no-useless-assign", - "description": "Prevent useless assignment.. Latest version: 1.0.3, last published: 4 years ago. Start using eslint-plugin-no-useless-assign in your project by running `npm i eslint-plugin-no-useless-assign`. There are 5 other projects in the npm registry using eslint-plugin-no-useless-assign." - }, - "https://sonarsource.atlassian.net/browse/RSPEC-1854": { - "domain": "sonarsource.atlassian.net", - "url": "https://sonarsource.atlassian.net/browse/RSPEC-1854", - "logo": "https://sonarsource.atlassian.net/images/jira-safari-pintab-icon.svg", - "title": "[RSPEC-1854] - Jira", - "description": null - }, "https://rules.sonarsource.com/javascript/RSPEC-1854/": { "domain": "rules.sonarsource.com", "url": "https://rules.sonarsource.com/javascript/RSPEC-1854/", @@ -776,13 +762,6 @@ "title": "JavaScript static code analysis: Unused assignments should be removed", "description": "Dead stores refer to assignments made to local variables that are subsequently never used or immediately overwritten. Such assignments are\nunnecessary and don’t contribute to the functionality or clarity of the code. They may even negatively impact performance. Removing them enhances code\ncleanlines…" }, - "https://rules.sonarsource.com/typescript/RSPEC-1854/": { - "domain": "rules.sonarsource.com", - "url": "https://rules.sonarsource.com/typescript/RSPEC-1854/", - "logo": "https://rules.sonarsource.com/favicon.ico", - "title": "TypeScript static code analysis: Unused assignments should be removed", - "description": "Dead stores refer to assignments made to local variables that are subsequently never used or immediately overwritten. Such assignments are\nunnecessary and don’t contribute to the functionality or clarity of the code. They may even negatively impact performance. Removing them enhances code\ncleanlines…" - }, "https://cwe.mitre.org/data/definitions/563.html": { "domain": "cwe.mitre.org", "url": "https://cwe.mitre.org/data/definitions/563.html", diff --git a/docs/src/rules/no-useless-assignment.md b/docs/src/rules/no-useless-assignment.md index 5f0ef110fb2..3e101958afb 100644 --- a/docs/src/rules/no-useless-assignment.md +++ b/docs/src/rules/no-useless-assignment.md @@ -5,10 +5,7 @@ related_rules: - no-unused-vars further_reading: - https://en.wikipedia.org/wiki/Dead_store -- https://www.npmjs.com/package/eslint-plugin-no-useless-assign -- https://sonarsource.atlassian.net/browse/RSPEC-1854 - https://rules.sonarsource.com/javascript/RSPEC-1854/ -- https://rules.sonarsource.com/typescript/RSPEC-1854/ - https://cwe.mitre.org/data/definitions/563.html - https://wiki.sei.cmu.edu/confluence/display/c/MSC13-C.+Detect+and+remove+unused+values - https://wiki.sei.cmu.edu/confluence/display/java/MSC56-J.+Detect+and+remove+superfluous+code+and+values @@ -22,6 +19,10 @@ further_reading: "Dead stores" waste processing and memory, so it is better to remove unnecessary assignments to variables. Also, if the author intended the variable to be used, there is likely a mistake around the dead store. +For example, + +* you should have used a stored value but forgot to do so. +* you made a mistake in the name of the variable to be stored. ## Rule Details @@ -34,13 +35,13 @@ Examples of **incorrect** code for this rule: ```js /* eslint no-useless-assignment: "error" */ -function foo() { +function fn1() { let v = 'used'; doSomething(v); v = 'unused'; } -function foo() { +function fn2() { let v = 'used'; if (condition) { v = 'unused'; @@ -49,7 +50,7 @@ function foo() { doSomething(v); } -function foo() { +function fn3() { let v = 'used'; if (condition) { doSomething(v); @@ -58,7 +59,7 @@ function foo() { } } -function foo() { +function fn4() { let v = 'unused'; if (condition) { v = 'used'; @@ -67,7 +68,7 @@ function foo() { } } -function foo() { +function fn5() { let v = 'used'; if (condition) { let v = 'used'; @@ -87,14 +88,14 @@ Examples of **correct** code for this rule: ```js /* eslint no-useless-assignment: "error" */ -function foo() { +function fn1() { let v = 'used'; doSomething(v); v = 'used-2'; doSomething(v); } -function foo() { +function fn2() { let v = 'used'; if (condition) { v = 'used-2'; @@ -104,7 +105,7 @@ function foo() { doSomething(v); } -function foo() { +function fn3() { let v = 'used'; if (condition) { doSomething(v); @@ -114,7 +115,7 @@ function foo() { } } -function foo () { +function fn4() { let v = 'used'; for (let i = 0; i < 10; i++) { doSomething(v); @@ -131,7 +132,7 @@ Because it's clearly an unused variable. If you want it reported, please enable ```js /* eslint no-useless-assignment: "error" */ -function foo() { +function fn() { let v = 'unused'; v = 'unused-2' doSomething(); From d4683ce4b49a748776daf7a2a763dffa7f2eb065 Mon Sep 17 00:00:00 2001 From: Yosuke Ota Date: Sat, 18 Nov 2023 00:11:24 +0900 Subject: [PATCH 22/35] Update docs/src/rules/no-useless-assignment.md Co-authored-by: Milos Djermanovic --- docs/src/rules/no-useless-assignment.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/src/rules/no-useless-assignment.md b/docs/src/rules/no-useless-assignment.md index 3e101958afb..97d4aa597a4 100644 --- a/docs/src/rules/no-useless-assignment.md +++ b/docs/src/rules/no-useless-assignment.md @@ -124,6 +124,8 @@ function fn4() { } ``` +::: + This rule will not report variables that are never read. Because it's clearly an unused variable. If you want it reported, please enable the [no-unused-vars](./no-unused-vars) rule. From 7bb52405682e7fdd06edac6c466cfaadad6ded4a Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Thu, 30 Nov 2023 09:23:41 +0900 Subject: [PATCH 23/35] fix: false negatives --- lib/rules/no-useless-assignment.js | 33 ++++++++++++++++++++---- tests/lib/rules/no-useless-assignment.js | 28 ++++++++++++++++++++ 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index ff4eb9f1063..4364dedac0a 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -260,10 +260,33 @@ module.exports = { * Other assignment on the current segment and after current assignment. */ const otherAssignmentAfterTargetAssignment = allAssignments - .find(assignment => - assignment !== targetAssignment && - assignment.segment === targetAssignment.segment && - targetAssignment.identifier.range[0] < assignment.identifier.range[0]); + .find(assignment => { + if (assignment === targetAssignment || assignment.segment !== targetAssignment.segment) { + return false; + } + if (targetAssignment.node.range[1] <= assignment.node.range[0]) { + + /* + * The assignment is located after the target assignment. + * e.g. x=1; x=2 + * ^^^ ^^^ assignment + * └ targetAssignment + */ + return true; + } + if (assignment.node.range[0] <= targetAssignment.node.range[0] && targetAssignment.node.range[1] <= assignment.node.range[1]) { + + /* + * The assignment wrap the target assignment. + * e.g. x=(x=1); + * ^^^ targetAssignment + * ^^^^^^^ assignment + */ + return true; + } + + return false; + }); for (const reference of readReferences) { @@ -278,7 +301,7 @@ module.exports = { // Checks if it is used in the same segment as the target assignment. if ( - targetAssignment.identifier.range[0] < reference.identifier.range[0] && + targetAssignment.node.range[1] <= reference.identifier.range[0] && isIdentifierUsedInSegment(targetAssignment.segment, reference.identifier) ) { diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index e493ec7e9c6..b3e18c70ea4 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -571,6 +571,34 @@ ruleTester.run("no-useless-assignment", rule, { } ] }, + { + code: ` + var x = 1; // used + x = x + 1; // unused + x = 5; // used + f(x);`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 3, + column: 13 + } + ] + }, + { + code: ` + var x = 1; // used + x = // used + x++; // unused + f(x);`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + } + ] + }, // Update { From 360674d68b9b5ba29589494c398d9bcf20737769 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Thu, 30 Nov 2023 09:27:13 +0900 Subject: [PATCH 24/35] docs: add example code --- docs/src/rules/no-useless-assignment.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/src/rules/no-useless-assignment.md b/docs/src/rules/no-useless-assignment.md index 97d4aa597a4..0faf447a5df 100644 --- a/docs/src/rules/no-useless-assignment.md +++ b/docs/src/rules/no-useless-assignment.md @@ -24,6 +24,14 @@ For example, * you should have used a stored value but forgot to do so. * you made a mistake in the name of the variable to be stored. +```js +let id = "x1234"; // this is a "dead store" - this value ("x1234") is never read + +id = generateId(); + +doSomethingWith(id); +``` + ## Rule Details This rule aims to report variable assignments when the value is not used. From 7e696f84fb6e362b5fd4258a1ae6987a1eedbea3 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Thu, 30 Nov 2023 09:30:40 +0900 Subject: [PATCH 25/35] test: move test case --- tests/lib/rules/no-useless-assignment.js | 58 ++++++++++++------------ 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index b3e18c70ea4..59e1860ddf3 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -571,34 +571,6 @@ ruleTester.run("no-useless-assignment", rule, { } ] }, - { - code: ` - var x = 1; // used - x = x + 1; // unused - x = 5; // used - f(x);`, - errors: [ - { - messageId: "unnecessaryAssignment", - line: 3, - column: 13 - } - ] - }, - { - code: ` - var x = 1; // used - x = // used - x++; // unused - f(x);`, - errors: [ - { - messageId: "unnecessaryAssignment", - line: 4, - column: 17 - } - ] - }, // Update { @@ -915,6 +887,36 @@ ruleTester.run("no-useless-assignment", rule, { column: 5 } ] + }, + + // An assignment within an assignment. + { + code: ` + var x = 1; // used + x = x + 1; // unused + x = 5; // used + f(x);`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 3, + column: 13 + } + ] + }, + { + code: ` + var x = 1; // used + x = // used + x++; // unused + f(x);`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 4, + column: 17 + } + ] } ] }); From a2b9339abd15b5710a796f57c4da9ed360a4c017 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sat, 9 Dec 2023 14:11:18 +0900 Subject: [PATCH 26/35] fix: false positives for assignment pattern --- lib/rules/no-useless-assignment.js | 100 +++++++++++++++++++---- tests/lib/rules/no-useless-assignment.js | 41 +++++++++- 2 files changed, 123 insertions(+), 18 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 4364dedac0a..2483956c42a 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -17,6 +17,7 @@ const { findVariable } = require("@eslint-community/eslint-utils"); /** @typedef {import("estree").VariableDeclarator} VariableDeclarator */ /** @typedef {import("estree").AssignmentExpression} AssignmentExpression */ /** @typedef {import("estree").UpdateExpression} UpdateExpression */ +/** @typedef {import("estree").Expression} Expression */ /** @typedef {import("eslint-scope").Scope} Scope */ /** @typedef {import("eslint-scope").Variable} Variable */ /** @typedef {import("../linter/code-path-analysis/code-path")} CodePath */ @@ -28,7 +29,7 @@ const { findVariable } = require("@eslint-community/eslint-utils"); /** * Extract identifier from the given pattern node used on the left-hand side of the assignment. - * @param {Pattern|null} pattern The pattern node to extract identifier + * @param {Pattern} pattern The pattern node to extract identifier * @returns {Iterable} The extracted identifier */ function *extractIdentifiersFromPattern(pattern) { @@ -59,6 +60,64 @@ function *extractIdentifiersFromPattern(pattern) { } } + +/** + * Checks whether the given identifier node is evaluated after the assignment identifier. + * @param {AssignmentInfo} assignment The assignment info. + * @param {Identifier} identifier The identifier to check. + * @returns {boolean} `true` if the given identifier node is evaluated after the assignment identifier. + */ +function isIdentifierEvaluatedAfterAssignment(assignment, identifier) { + if (identifier.range[0] < assignment.identifier.range[1]) { + return false; + } + if ( + assignment.expression && + assignment.expression.range[0] <= identifier.range[0] && + identifier.range[1] <= assignment.expression.range[1] + ) { + + /* + * The identifier node is in an expression that is evaluated before the assignment. + * e.g. x = id; + * ^^ identifier to check + * ^ assignment identifier + */ + return false; + } + + /* + * e.g. + * x = 42; id; + * ^^ identifier to check + * ^ assignment identifier + * let { x, y = id } = obj; + * ^^ identifier to check + * ^ assignment identifier + */ + return true; +} + +/** + * Checks whether the given identifier node is used between the assigned identifier and the equal sign. + * + * e.g. let { x, y = x } = obj; + * ^ identifier to check + * ^ assigned identifier + * @param {AssignmentInfo} assignment The assignment info. + * @param {Identifier} identifier The identifier to check. + * @returns {boolean} `true` if the given identifier node is used between the assigned identifier and the equal sign. + */ +function isIdentifierUsedBetweenAssignedAndEqualSign(assignment, identifier) { + if (!assignment.expression) { + return false; + } + return ( + assignment.identifier.range[1] <= identifier.range[0] && + identifier.range[1] <= assignment.expression.range[0] + ); +} + //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -107,6 +166,7 @@ module.exports = { * @property {Variable} variable The variable that is assigned. * @property {Identifier} identifier The identifier that is assigned. * @property {VariableDeclarator|AssignmentExpression|UpdateExpression} node The node where the variable was updated. + * @property {Expression|null} expression The expression that is evaluated before the assignment. * @property {CodePathSegment} segment The code path segment where the assignment was made. */ @@ -221,7 +281,11 @@ module.exports = { } subsequentSegmentData.subsequentSegments.add(nextSegment); - const assignmentInSegment = allAssignments.find(otherAssignment => otherAssignment.segment === nextSegment); + const assignmentInSegment = allAssignments + .find(otherAssignment => ( + otherAssignment.segment === nextSegment && + !isIdentifierUsedBetweenAssignedAndEqualSign(otherAssignment, targetAssignment.identifier) + )); if (!assignmentInSegment) { @@ -264,20 +328,17 @@ module.exports = { if (assignment === targetAssignment || assignment.segment !== targetAssignment.segment) { return false; } - if (targetAssignment.node.range[1] <= assignment.node.range[0]) { - - /* - * The assignment is located after the target assignment. - * e.g. x=1; x=2 - * ^^^ ^^^ assignment - * └ targetAssignment - */ + if (isIdentifierEvaluatedAfterAssignment(targetAssignment, assignment.identifier)) { return true; } - if (assignment.node.range[0] <= targetAssignment.node.range[0] && targetAssignment.node.range[1] <= assignment.node.range[1]) { + if ( + assignment.expression && + assignment.expression.range[0] <= targetAssignment.identifier.range[0] && + targetAssignment.identifier.range[1] <= assignment.expression.range[1] + ) { /* - * The assignment wrap the target assignment. + * The target assignment is in an expression that is evaluated before the assignment. * e.g. x=(x=1); * ^^^ targetAssignment * ^^^^^^^ assignment @@ -301,13 +362,16 @@ module.exports = { // Checks if it is used in the same segment as the target assignment. if ( - targetAssignment.node.range[1] <= reference.identifier.range[0] && - isIdentifierUsedInSegment(targetAssignment.segment, reference.identifier) + isIdentifierEvaluatedAfterAssignment(targetAssignment, reference.identifier) && + ( + isIdentifierUsedBetweenAssignedAndEqualSign(targetAssignment, reference.identifier) || + isIdentifierUsedInSegment(targetAssignment.segment, reference.identifier) + ) ) { if ( otherAssignmentAfterTargetAssignment && - otherAssignmentAfterTargetAssignment.node.range[1] <= reference.identifier.range[0] + isIdentifierEvaluatedAfterAssignment(otherAssignmentAfterTargetAssignment, reference.identifier) ) { // There was another assignment before the reference. Therefore, it has not been used yet. @@ -332,7 +396,7 @@ module.exports = { if (isIdentifierUsedInSegment(subsequentSegment.segment, reference.identifier)) { if ( subsequentSegment.assignment && - subsequentSegment.assignment.node.range[1] <= reference.identifier.range[0] + isIdentifierEvaluatedAfterAssignment(subsequentSegment.assignment, reference.identifier) ) { // There was another assignment before the reference. Therefore, it has not been used yet. @@ -409,11 +473,14 @@ module.exports = { const assignments = scopeStack.assignments; let pattern; + let expression = null; if (node.type === "VariableDeclarator") { pattern = node.id; + expression = node.init; } else if (node.type === "AssignmentExpression") { pattern = node.left; + expression = node.right; } else { // UpdateExpression pattern = node.argument; } @@ -487,6 +554,7 @@ module.exports = { variable, identifier, node, + expression, segment: segmentInfo.segment }); } diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index 59e1860ddf3..c0dfb37ad23 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -287,7 +287,23 @@ ruleTester.run("no-useless-assignment", rule, { } catch (e) { // ignore } - console.log(v)` + console.log(v)`, + + // An expression within an assignment. + `const obj = { a: 5 }; + const { a, b = a } = obj; + console.log(b); // 5`, + `const arr = [6]; + const [c, d = c] = arr; + console.log(d); // 6`, + `const obj = { a: 1 }; + let { + a, + b = (a = 2) + } = obj; + console.log(a, b);`, + `let { a, b: {c = a} = {} } = obj; + console.log(c);` ], invalid: [ { @@ -889,7 +905,7 @@ ruleTester.run("no-useless-assignment", rule, { ] }, - // An assignment within an assignment. + // An expression within an assignment. { code: ` var x = 1; // used @@ -917,6 +933,27 @@ ruleTester.run("no-useless-assignment", rule, { column: 17 } ] + }, + { + code: `const obj = { a: 1 }; + let { + a, + b = (a = 2) + } = obj; + a = 3 + console.log(a, b);`, + errors: [ + { + messageId: "unnecessaryAssignment", + line: 3, + column: 17 + }, + { + messageId: "unnecessaryAssignment", + line: 4, + column: 22 + } + ] } ] }); From 6fce0491cd539e00da82fa00aed68310ca484f7f Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Tue, 12 Dec 2023 09:12:28 +0900 Subject: [PATCH 27/35] test: use flat-rule-tester --- tests/lib/rules/no-useless-assignment.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index c0dfb37ad23..0ba58d9129f 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -10,17 +10,15 @@ //------------------------------------------------------------------------------ const rule = require("../../../lib/rules/no-useless-assignment"); -const { RuleTester } = require("../../../lib/rule-tester"); +const FlatRuleTester = require("../../../lib/rule-tester/flat-rule-tester"); //------------------------------------------------------------------------------ // Tests //------------------------------------------------------------------------------ -const ruleTester = new RuleTester({ - parserOptions: { ecmaVersion: 2022, sourceType: "module" } -}); +const flatRuleTester = new FlatRuleTester(); -ruleTester.run("no-useless-assignment", rule, { +flatRuleTester.run("no-useless-assignment", rule, { valid: [ // Basic tests @@ -129,7 +127,7 @@ ruleTester.run("no-useless-assignment", rule, { let foo = 'used'; console.log(foo); foo = 'unused like but exported with directive';`, - parserOptions: { sourceType: "script" } + languageOptions: { sourceType: "script" } }, // Unknown variable @@ -246,7 +244,9 @@ ruleTester.run("no-useless-assignment", rule, { } finally { console = bk; }`, - env: { browser: true } + languageOptions: { + globals: { console: false } + } }, // Try catch From a83dfde7a84fae1c64a5f1945c24297fee03cf4b Mon Sep 17 00:00:00 2001 From: Yosuke Ota Date: Sun, 17 Dec 2023 21:22:13 +0900 Subject: [PATCH 28/35] Update lib/rules/no-useless-assignment.js Co-authored-by: Milos Djermanovic --- lib/rules/no-useless-assignment.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 2483956c42a..ba96a31b101 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -314,8 +314,8 @@ module.exports = { if (!readReferences.length) { /* - * It is not an unnecessary assignment, it is clearly an unnecessary (unused) variable - * and should not be reported in the this rule. And it is reported by `no-unused-vars`. + * It is not just an unnecessary assignment, but an unnecessary (unused) variable + * and thus should not be reported by this rule because it is reported by `no-unused-vars`. */ return; } From 8a10c7f1e20eb0b4cd474d24067361e9dac1d678 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sun, 17 Dec 2023 21:20:41 +0900 Subject: [PATCH 29/35] fix: false positives for `finally` block --- lib/rules/no-useless-assignment.js | 42 +++++++++++++----------- tests/lib/rules/no-useless-assignment.js | 9 ++++- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index ba96a31b101..40d2404ebc2 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -149,7 +149,7 @@ module.exports = { * @property {Scope} scope The scope of this scope stack. * @property {ScopeStack} upper The upper scope stack. * @property {Record} segments The map of ScopeStackSegmentInfo. - * @property {ScopeStackSegmentInfo[]} currentSegments The current ScopeStackSegmentInfo. + * @property {Set} currentSegments The current CodePathSegments. * @property {Map} assignments The map of list of AssignmentInfo for each variable. */ /** @@ -167,7 +167,7 @@ module.exports = { * @property {Identifier} identifier The identifier that is assigned. * @property {VariableDeclarator|AssignmentExpression|UpdateExpression} node The node where the variable was updated. * @property {Expression|null} expression The expression that is evaluated before the assignment. - * @property {CodePathSegment} segment The code path segment where the assignment was made. + * @property {CodePathSegment[]} segments The code path segments where the assignment was made. */ /** @type {ScopeStack} */ @@ -261,7 +261,7 @@ module.exports = { * If additional iterations are needed, consume this information and iterate. * @type {CodePathSegment[]} */ - queueSegments: [...targetAssignment.segment.nextSegments] + queueSegments: targetAssignment.segments.flatMap(segment => segment.nextSegments) }; @@ -283,7 +283,7 @@ module.exports = { const assignmentInSegment = allAssignments .find(otherAssignment => ( - otherAssignment.segment === nextSegment && + otherAssignment.segments.includes(nextSegment) && !isIdentifierUsedBetweenAssignedAndEqualSign(otherAssignment, targetAssignment.identifier) )); @@ -325,7 +325,10 @@ module.exports = { */ const otherAssignmentAfterTargetAssignment = allAssignments .find(assignment => { - if (assignment === targetAssignment || assignment.segment !== targetAssignment.segment) { + if ( + assignment === targetAssignment || + assignment.segments.every(segment => !targetAssignment.segments.includes(segment)) + ) { return false; } if (isIdentifierEvaluatedAfterAssignment(targetAssignment, assignment.identifier)) { @@ -365,7 +368,7 @@ module.exports = { isIdentifierEvaluatedAfterAssignment(targetAssignment, reference.identifier) && ( isIdentifierUsedBetweenAssignedAndEqualSign(targetAssignment, reference.identifier) || - isIdentifierUsedInSegment(targetAssignment.segment, reference.identifier) + targetAssignment.segments.some(segment => isIdentifierUsedInSegment(segment, reference.identifier)) ) ) { @@ -432,7 +435,7 @@ module.exports = { codePath, scope, segments: Object.create(null), - currentSegments: [], + currentSegments: new Set(), assignments: new Map() }; codePathStartScopes.add(scopeStack.scope); @@ -446,30 +449,31 @@ module.exports = { const segmentInfo = { segment, first: null, last: null }; scopeStack.segments[segment.id] = segmentInfo; - scopeStack.currentSegments.unshift(segmentInfo); + scopeStack.currentSegments.add(segment); }, onUnreachableCodePathSegmentStart(segment) { const segmentInfo = { segment, first: null, last: null }; scopeStack.segments[segment.id] = segmentInfo; - scopeStack.currentSegments.unshift(segmentInfo); + scopeStack.currentSegments.add(segment); }, - onCodePathSegmentEnd() { - scopeStack.currentSegments.shift(); + onCodePathSegmentEnd(segment) { + scopeStack.currentSegments.delete(segment); }, - onUnreachableCodePathSegmentEnd() { - scopeStack.currentSegments.shift(); + onUnreachableCodePathSegmentEnd(segment) { + scopeStack.currentSegments.delete(segment); }, Identifier(node) { - const segmentInfo = scopeStack.currentSegments[0]; + for (const segment of scopeStack.currentSegments) { + const segmentInfo = scopeStack.segments[segment.id]; - if (!segmentInfo.first) { - segmentInfo.first = node; + if (!segmentInfo.first) { + segmentInfo.first = node; + } + segmentInfo.last = node; } - segmentInfo.last = node; }, ":matches(VariableDeclarator[init!=null], AssignmentExpression, UpdateExpression):exit"(node) { - const segmentInfo = scopeStack.currentSegments[0]; const assignments = scopeStack.assignments; let pattern; @@ -555,7 +559,7 @@ module.exports = { identifier, node, expression, - segment: segmentInfo.segment + segments: [...scopeStack.currentSegments] }); } } diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index 0ba58d9129f..6eca38b3dff 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -249,7 +249,7 @@ flatRuleTester.run("no-useless-assignment", rule, { } }, - // Try catch + // Try catch finally `let message = 'init'; try { const result = call(); @@ -288,6 +288,13 @@ flatRuleTester.run("no-useless-assignment", rule, { // ignore } console.log(v)`, + `let a; + try { + foo(); + } finally { + a = 5; + } + console.log(a);`, // An expression within an assignment. `const obj = { a: 5 }; From a6e5843b49ac1d2b4de611458009d0b535181a58 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sun, 17 Dec 2023 21:21:12 +0900 Subject: [PATCH 30/35] fix: false positives for markVariableAsUsed() --- lib/rules/no-useless-assignment.js | 5 +++-- tests/lib/rules/no-useless-assignment.js | 28 +++++++++++++++++++++++- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 40d2404ebc2..e0ae5508bc2 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -512,8 +512,9 @@ module.exports = { continue; } - // Variables exported by "exported" block comments - if (variable.eslintExported) { + // Variables marked by `markVariableAsUsed()` or + // exported by "exported" block comment. + if (variable.eslintUsed) { continue; } diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index 6eca38b3dff..d54acd606e6 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -16,7 +16,26 @@ const FlatRuleTester = require("../../../lib/rule-tester/flat-rule-tester"); // Tests //------------------------------------------------------------------------------ -const flatRuleTester = new FlatRuleTester(); +const flatRuleTester = new FlatRuleTester({ + plugins: { + test: { + rules: { + "use-a": { + create(context) { + const sourceCode = context.sourceCode; + + return { + VariableDeclaration(node) { + sourceCode.markVariableAsUsed("a", node); + } + }; + } + } + } + } + } +}); + flatRuleTester.run("no-useless-assignment", rule, { valid: [ @@ -130,6 +149,13 @@ flatRuleTester.run("no-useless-assignment", rule, { languageOptions: { sourceType: "script" } }, + // Mark variables as used via markVariableAsUsed() + `/*eslint test/use-a:1*/ + let a = 'used'; + console.log(a); + a = 'unused like but marked by markVariableAsUsed()'; + `, + // Unknown variable `v = 'used'; console.log(v); From cf5dd30937377a606fc104f06d19010ff776ee32 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sun, 17 Dec 2023 21:24:19 +0900 Subject: [PATCH 31/35] chore: rename function --- lib/rules/no-useless-assignment.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index e0ae5508bc2..db49f3871c6 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -221,13 +221,13 @@ module.exports = { } /** - * Verifies whether the given assignment info is an unused assignment. + * Verifies whether the given assignment info is an used assignment. * Report if it is an unused assignment. * @param {AssignmentInfo} targetAssignment The assignment info to verify. * @param {AssignmentInfo[]} allAssignments The list of all assignment info for variables. * @returns {void} */ - function verifyAssignmentInfoIsUnused(targetAssignment, allAssignments) { + function verifyAssignmentIsUsed(targetAssignment, allAssignments) { /** * @typedef {Object} SubsequentSegmentData @@ -421,7 +421,7 @@ module.exports = { for (const assignments of target.assignments.values()) { assignments.sort((a, b) => a.identifier.range[0] - b.identifier.range[0]); for (const assignment of assignments) { - verifyAssignmentInfoIsUnused(assignment, assignments); + verifyAssignmentIsUsed(assignment, assignments); } } } From b3d0d9231a282fb8f202698701d3cff6dfbe0135 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Sun, 17 Dec 2023 21:30:31 +0900 Subject: [PATCH 32/35] fix: false positives for unreachable segments --- lib/rules/no-useless-assignment.js | 15 ++++++--------- tests/lib/rules/no-useless-assignment.js | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index db49f3871c6..2c4b81f1002 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -451,18 +451,9 @@ module.exports = { scopeStack.segments[segment.id] = segmentInfo; scopeStack.currentSegments.add(segment); }, - onUnreachableCodePathSegmentStart(segment) { - const segmentInfo = { segment, first: null, last: null }; - - scopeStack.segments[segment.id] = segmentInfo; - scopeStack.currentSegments.add(segment); - }, onCodePathSegmentEnd(segment) { scopeStack.currentSegments.delete(segment); }, - onUnreachableCodePathSegmentEnd(segment) { - scopeStack.currentSegments.delete(segment); - }, Identifier(node) { for (const segment of scopeStack.currentSegments) { const segmentInfo = scopeStack.segments[segment.id]; @@ -474,6 +465,12 @@ module.exports = { } }, ":matches(VariableDeclarator[init!=null], AssignmentExpression, UpdateExpression):exit"(node) { + if (scopeStack.currentSegments.size === 0) { + + // Ignore unreachable segments + return; + } + const assignments = scopeStack.assignments; let pattern; diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index d54acd606e6..2f55a9becbf 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -164,6 +164,23 @@ flatRuleTester.run("no-useless-assignment", rule, { // Unused variable "let v = 'used variable';", + // Unreachable + `function foo() { + return; + + const x = 1; + if (y) { + bar(x); + } + }`, + `function foo() { + const x = 1; + console.log(x); + return; + + x = 'Foo' + }`, + // Update `function foo() { let a = 42; From 8ecc87c587d79d435377f435b7be32cf4756aaa3 Mon Sep 17 00:00:00 2001 From: Yosuke Ota Date: Wed, 27 Dec 2023 15:44:55 +0900 Subject: [PATCH 33/35] Update lib/rules/no-useless-assignment.js Co-authored-by: Nicholas C. Zakas --- lib/rules/no-useless-assignment.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/no-useless-assignment.js b/lib/rules/no-useless-assignment.js index 2c4b81f1002..cac8ba1fcd1 100644 --- a/lib/rules/no-useless-assignment.js +++ b/lib/rules/no-useless-assignment.js @@ -327,7 +327,7 @@ module.exports = { .find(assignment => { if ( assignment === targetAssignment || - assignment.segments.every(segment => !targetAssignment.segments.includes(segment)) + assignment.segments.length && assignment.segments.every(segment => !targetAssignment.segments.includes(segment)) ) { return false; } From adf0fb4ab1f785d12629335d18155bf2a2bb1a05 Mon Sep 17 00:00:00 2001 From: yosuke ota Date: Wed, 27 Dec 2023 15:55:15 +0900 Subject: [PATCH 34/35] test: revert tests/conf/eslint-all.js --- tests/conf/eslint-all.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conf/eslint-all.js b/tests/conf/eslint-all.js index 4124fc9e9ed..6475965a02a 100644 --- a/tests/conf/eslint-all.js +++ b/tests/conf/eslint-all.js @@ -31,7 +31,7 @@ describe("eslint-all", () => { const someRule = "yoda"; assert.include(ruleNames, someRule); - assert.isAtLeast(count, 200); + assert.isBelow(count, 200); }); it("should configure all rules as errors", () => { From f6164208faa8139abf5d9a2186dd68c2219f2149 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Tue, 9 Jan 2024 17:24:07 +0900 Subject: [PATCH 35/35] test: use rule-tester --- tests/lib/rules/no-useless-assignment.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/lib/rules/no-useless-assignment.js b/tests/lib/rules/no-useless-assignment.js index 2f55a9becbf..4219df0da48 100644 --- a/tests/lib/rules/no-useless-assignment.js +++ b/tests/lib/rules/no-useless-assignment.js @@ -10,13 +10,13 @@ //------------------------------------------------------------------------------ const rule = require("../../../lib/rules/no-useless-assignment"); -const FlatRuleTester = require("../../../lib/rule-tester/flat-rule-tester"); +const RuleTester = require("../../../lib/rule-tester/rule-tester"); //------------------------------------------------------------------------------ // Tests //------------------------------------------------------------------------------ -const flatRuleTester = new FlatRuleTester({ +const ruleTester = new RuleTester({ plugins: { test: { rules: { @@ -37,7 +37,7 @@ const flatRuleTester = new FlatRuleTester({ }); -flatRuleTester.run("no-useless-assignment", rule, { +ruleTester.run("no-useless-assignment", rule, { valid: [ // Basic tests