Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add no-useless-assignment rule #17625

Merged
merged 40 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
fa14942
feat: add `no-useless-assignment` rule
ota-meshi Oct 7, 2023
020ee4f
fix: add test cases and fix
ota-meshi Oct 7, 2023
35327ba
chore: remove wrong comment
ota-meshi Oct 7, 2023
ca5c5c2
test: add test and update
ota-meshi Oct 9, 2023
3d63f0f
feat: report if it is not used between assignments
ota-meshi Oct 9, 2023
7e2692e
fix: add test case and fix
ota-meshi Oct 9, 2023
6bb7f65
Apply suggestions from code review
ota-meshi Oct 10, 2023
fb050c3
docs: update
ota-meshi Oct 10, 2023
eb5e982
fix: false negatives for same segments & refactor
ota-meshi Oct 20, 2023
d9e3769
Merge remote-tracking branch 'upstream/main' into no-useless-assignment
ota-meshi Oct 20, 2023
c8ab128
fix: rule-types.json
ota-meshi Oct 20, 2023
d3c1223
fix: add update assignment test case & fix
ota-meshi Oct 23, 2023
96dd2ac
fix: add test for global vars & fix
ota-meshi Oct 23, 2023
1a2efc0
Update docs/src/rules/no-useless-assignment.md
ota-meshi Nov 5, 2023
e21b292
docs: update description
ota-meshi Nov 5, 2023
06f9698
docs: add further_reading
ota-meshi Nov 5, 2023
a68ab8d
chore: improve jsdoc and rename function
ota-meshi Nov 5, 2023
09a2a22
docs: update doc
ota-meshi Nov 5, 2023
a102c12
Merge remote-tracking branch 'upstream/main' into no-useless-assignment
ota-meshi Nov 5, 2023
5c56a3e
fix: false positives for try catch
ota-meshi Nov 6, 2023
6ca9835
test: fix tests/conf/eslint-all.js
ota-meshi Nov 6, 2023
153e5dc
feat: add no-useless-assignment to eslint-config-eslint
ota-meshi Nov 6, 2023
91c0a23
docs: fix document
ota-meshi Nov 14, 2023
d4683ce
Update docs/src/rules/no-useless-assignment.md
ota-meshi Nov 17, 2023
7bb5240
fix: false negatives
ota-meshi Nov 30, 2023
360674d
docs: add example code
ota-meshi Nov 30, 2023
7e696f8
test: move test case
ota-meshi Nov 30, 2023
a2b9339
fix: false positives for assignment pattern
ota-meshi Dec 9, 2023
d0e6ef9
Merge remote-tracking branch 'upstream/main' into no-useless-assignment
ota-meshi Dec 12, 2023
6fce049
test: use flat-rule-tester
ota-meshi Dec 12, 2023
a83dfde
Update lib/rules/no-useless-assignment.js
ota-meshi Dec 17, 2023
8a10c7f
fix: false positives for `finally` block
ota-meshi Dec 17, 2023
a6e5843
fix: false positives for markVariableAsUsed()
ota-meshi Dec 17, 2023
cf5dd30
chore: rename function
ota-meshi Dec 17, 2023
b3d0d92
fix: false positives for unreachable segments
ota-meshi Dec 17, 2023
8ecc87c
Update lib/rules/no-useless-assignment.js
ota-meshi Dec 27, 2023
011330a
Merge remote-tracking branch 'upstream/main' into no-useless-assignment
ota-meshi Dec 27, 2023
adf0fb4
test: revert tests/conf/eslint-all.js
ota-meshi Dec 27, 2023
fe8053b
Merge remote-tracking branch 'upstream/main' into no-useless-assignment
ota-meshi Jan 9, 2024
f616420
test: use rule-tester
ota-meshi Jan 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions docs/src/_data/further_reading_links.json
Original file line number Diff line number Diff line change
Expand Up @@ -747,5 +747,40 @@
"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://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://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
}
}
2 changes: 2 additions & 0 deletions docs/src/rules/no-unused-vars.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
---
title: no-unused-vars
rule_type: problem
related_rules:
- no-useless-assignment
---


Expand Down
156 changes: 156 additions & 0 deletions docs/src/rules/no-useless-assignment.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
---
title: no-useless-assignment
rule_type: suggestion
related_rules:
- no-unused-vars
further_reading:
- https://en.wikipedia.org/wiki/Dead_store
- https://rules.sonarsource.com/javascript/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
---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure to include more references for people. See the links in #17559 (comment)

We can use the "further_reading" key to list these out.



[Wikipedia describes a "dead store"](https://en.wikipedia.org/wiki/Dead_store) as follows:

> 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need more in this section. Please show an example.

For example,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't think I was clear. I'm looking for a code example in this section, something like:

let id = "x1234";    // this is a "dead store" - this value is never read

id = generateId();

doSomethingWith(id);

That way, we can explain that "x1234" is a value that can't possibly be used, and then we say this type of error can occur while refactoring.


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

Examples of **incorrect** code for this rule:

::: incorrect

```js
/* eslint no-useless-assignment: "error" */

function fn1() {
let v = 'used';
doSomething(v);
v = 'unused';
}

function fn2() {
let v = 'used';
if (condition) {
v = 'unused';
return
}
doSomething(v);
}

function fn3() {
let v = 'used';
if (condition) {
doSomething(v);
} else {
v = 'unused';
}
}

function fn4() {
let v = 'unused';
if (condition) {
v = 'used';
doSomething(v);
return
}
}

function fn5() {
let v = 'used';
if (condition) {
let v = 'used';
console.log(v);
v = 'unused';
}
console.log(v);
}
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it is now, the code in this example cannot be parsed in the Playground with the default { "sourceType": "module" } because of the duplicated function name foo (see here). This can be easily fixed by giving the functions unique names. That's what we did in https://github.com/eslint/eslint/pull/17633/files in other places. Since this is a new rule, it would be good to have the examples working in the Playground right from the start.


:::

Examples of **correct** code for this rule:

::: correct

```js
/* eslint no-useless-assignment: "error" */

function fn1() {
let v = 'used';
doSomething(v);
v = 'used-2';
doSomething(v);
}

function fn2() {
let v = 'used';
if (condition) {
v = 'used-2';
doSomething(v);
return
}
doSomething(v);
}

function fn3() {
let v = 'used';
if (condition) {
doSomething(v);
} else {
v = 'used-2';
doSomething(v);
}
}

function fn4() {
let v = 'used';
for (let i = 0; i < 10; i++) {
doSomething(v);
v = 'used in next iteration';
}
}
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, the function names in this code block should be unique.


ota-meshi marked this conversation as resolved.
Show resolved Hide resolved
:::

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 fn() {
let v = 'unused';
v = 'unused-2'
doSomething();
}
```

:::

## When Not To Use It

If you don't want to be notified about values that are never read, you can safely disable this rule.
6 changes: 3 additions & 3 deletions lib/linter/code-path-analysis/code-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,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
Expand Down
17 changes: 7 additions & 10 deletions lib/linter/config-comment-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,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:
Expand All @@ -102,11 +100,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.");

Expand All @@ -124,11 +126,6 @@ module.exports = class ConfigCommentParser {
};

}

return {
success: true,
config: items
};
}

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/linter/linter.js
Original file line number Diff line number Diff line change
Expand Up @@ -2139,7 +2139,7 @@ class Linter {
* SourceCodeFixer.
*/
verifyAndFix(text, config, options) {
let messages = [],
let messages,
fixedResult,
fixed = false,
passNumber = 0,
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/array-bracket-newline.js
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/indent-legacy.js
Original file line number Diff line number Diff line change
Expand Up @@ -830,7 +830,7 @@ module.exports = {
}

let indent;
let nodesToCheck = [];
let nodesToCheck;

/*
* For this statements we should check indent from statement beginning,
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/indent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
1 change: 1 addition & 0 deletions lib/rules/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/key-spacing.js
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ module.exports = {
}
}

let messageId = "";
let messageId;

if (isExtra) {
messageId = side === "key" ? "extraKey" : "extraValue";
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/max-len.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/no-dupe-class-members.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions lib/rules/no-empty-function.js
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -73,7 +73,7 @@ function getKind(node) {
}

// Detects prefix.
let prefix = "";
let prefix;

if (node.generator) {
prefix = "generator";
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/no-loss-of-precision.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 2 additions & 3 deletions lib/rules/no-trailing-spaces.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -177,7 +176,7 @@ module.exports = {
continue;
}

fixRange = [rangeStart, rangeEnd];
const fixRange = [rangeStart, rangeEnd];

if (!ignoreComments || !commentLineNumbers.has(lineNumber)) {
report(node, location, fixRange);
Expand Down
Loading