Skip to content

Commit 17a8849

Browse files
mdjermanovicKai Cataldo
authored and
Kai Cataldo
committed
New: Add no-dupe-else-if rule (fixes #12469) (#12504)
* New: Add no-dupe-else-if rule (fixes #12469) * Add || and && logic * Rename variables * Update lib/rules/no-dupe-else-if.js Co-Authored-By: Kai Cataldo <[email protected]>
1 parent 41a78fd commit 17a8849

File tree

5 files changed

+619
-0
lines changed

5 files changed

+619
-0
lines changed

‎docs/rules/no-dupe-else-if.md‎

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
# Disallow duplicate conditions in `if-else-if` chains (no-dupe-else-if)
2+
3+
`if-else-if` chains are commonly used when there is a need to execute only one branch (or at most one branch) out of several possible branches, based on certain conditions.
4+
5+
```js
6+
if (a) {
7+
foo();
8+
} else if (b) {
9+
bar();
10+
} else if (c) {
11+
baz();
12+
}
13+
```
14+
15+
Two identical test conditions in the same chain are almost always a mistake in the code. Unless there are side effects in the expressions, a duplicate will evaluate to the same `true` or `false` value as the identical expression earlier in the chain, meaning that its branch can never execute.
16+
17+
```js
18+
if (a) {
19+
foo();
20+
} else if (b) {
21+
bar();
22+
} else if (b) {
23+
baz();
24+
}
25+
```
26+
27+
In the above example, `baz()` can never execute. Obviously, `baz()` could be executed only when `b` evaluates to `true`, but in that case `bar()` would be executed instead, since it's earlier in the chain.
28+
29+
## Rule Details
30+
31+
This rule disallows duplicate conditions in the same `if-else-if` chain.
32+
33+
Examples of **incorrect** code for this rule:
34+
35+
```js
36+
/*eslint no-dupe-else-if: "error"*/
37+
38+
if (isSomething(x)) {
39+
foo();
40+
} else if (isSomething(x)) {
41+
bar();
42+
}
43+
44+
if (a) {
45+
foo();
46+
} else if (b) {
47+
bar();
48+
} else if (c && d) {
49+
baz();
50+
} else if (c && d) {
51+
quux();
52+
} else {
53+
quuux();
54+
}
55+
56+
if (n === 1) {
57+
foo();
58+
} else if (n === 2) {
59+
bar();
60+
} else if (n === 3) {
61+
baz();
62+
} else if (n === 2) {
63+
quux();
64+
} else if (n === 5) {
65+
quuux();
66+
}
67+
```
68+
69+
Examples of **correct** code for this rule:
70+
71+
```js
72+
/*eslint no-dupe-else-if: "error"*/
73+
74+
if (isSomething(x)) {
75+
foo();
76+
} else if (isSomethingElse(x)) {
77+
bar();
78+
}
79+
80+
if (a) {
81+
foo();
82+
} else if (b) {
83+
bar();
84+
} else if (c && d) {
85+
baz();
86+
} else if (c && e) {
87+
quux();
88+
} else {
89+
quuux();
90+
}
91+
92+
if (n === 1) {
93+
foo();
94+
} else if (n === 2) {
95+
bar();
96+
} else if (n === 3) {
97+
baz();
98+
} else if (n === 4) {
99+
quux();
100+
} else if (n === 5) {
101+
quuux();
102+
}
103+
```
104+
105+
This rule can also detect some cases where the conditions are not identical, but the branch can never execute due to the logic of `||` and `&&` operators.
106+
107+
Examples of additional **incorrect** code for this rule:
108+
109+
```js
110+
/*eslint no-dupe-else-if: "error"*/
111+
112+
if (a || b) {
113+
foo();
114+
} else if (a) {
115+
bar();
116+
}
117+
118+
if (a) {
119+
foo();
120+
} else if (b) {
121+
bar();
122+
} else if (a || b) {
123+
baz();
124+
}
125+
126+
if (a) {
127+
foo();
128+
} else if (a && b) {
129+
bar();
130+
}
131+
132+
if (a && b) {
133+
foo();
134+
} else if (a && b && c) {
135+
bar();
136+
}
137+
138+
if (a || b) {
139+
foo();
140+
} else if (b && c) {
141+
bar();
142+
}
143+
144+
if (a) {
145+
foo();
146+
} else if (b && c) {
147+
bar();
148+
} else if (d && (c && e && b || a)) {
149+
baz();
150+
}
151+
```
152+
153+
Please note that this rule does not compare conditions from the chain with conditions inside statements, and will not warn in the cases such as follows:
154+
155+
```js
156+
if (a) {
157+
if (a) {
158+
foo();
159+
}
160+
}
161+
162+
if (a) {
163+
foo();
164+
} else {
165+
if (a) {
166+
bar();
167+
}
168+
}
169+
```
170+
171+
## When Not To Use It
172+
173+
In rare cases where you really need identical test conditions in the same chain, which necessarily means that the expressions in the chain are causing and relying on side effects, you will have to turn this rule off.
174+
175+
## Related Rules
176+
177+
* [no-duplicate-case](no-duplicate-case.md)
178+
* [no-lonely-if](no-lonely-if.md)

‎lib/rules/index.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ module.exports = new LazyLoadingRuleMap(Object.entries({
108108
"no-div-regex": () => require("./no-div-regex"),
109109
"no-dupe-args": () => require("./no-dupe-args"),
110110
"no-dupe-class-members": () => require("./no-dupe-class-members"),
111+
"no-dupe-else-if": () => require("./no-dupe-else-if"),
111112
"no-dupe-keys": () => require("./no-dupe-keys"),
112113
"no-duplicate-case": () => require("./no-duplicate-case"),
113114
"no-duplicate-imports": () => require("./no-duplicate-imports"),

‎lib/rules/no-dupe-else-if.js‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
/**
2+
* @fileoverview Rule to disallow duplicate conditions in if-else-if chains
3+
* @author Milos Djermanovic
4+
*/
5+
6+
"use strict";
7+
8+
//------------------------------------------------------------------------------
9+
// Requirements
10+
//------------------------------------------------------------------------------
11+
12+
const astUtils = require("./utils/ast-utils");
13+
14+
//------------------------------------------------------------------------------
15+
// Helpers
16+
//------------------------------------------------------------------------------
17+
18+
/**
19+
* Determines whether the first given array is a subset of the second given array.
20+
* @param {Function} comparator A function to compare two elements, should return `true` if they are equal.
21+
* @param {Array} arrA The array to compare from.
22+
* @param {Array} arrB The array to compare against.
23+
* @returns {boolean} `true` if the array `arrA` is a subset of the array `arrB`.
24+
*/
25+
function isSubsetByComparator(comparator, arrA, arrB) {
26+
return arrA.every(a => arrB.some(b => comparator(a, b)));
27+
}
28+
29+
/**
30+
* Splits the given node by the given logical operator.
31+
* @param {string} operator Logical operator `||` or `&&`.
32+
* @param {ASTNode} node The node to split.
33+
* @returns {ASTNode[]} Array of conditions that makes the node when joined by the operator.
34+
*/
35+
function splitByLogicalOperator(operator, node) {
36+
if (node.type === "LogicalExpression" && node.operator === operator) {
37+
return [...splitByLogicalOperator(operator, node.left), ...splitByLogicalOperator(operator, node.right)];
38+
}
39+
return [node];
40+
}
41+
42+
const splitByOr = splitByLogicalOperator.bind(null, "||");
43+
const splitByAnd = splitByLogicalOperator.bind(null, "&&");
44+
45+
//------------------------------------------------------------------------------
46+
// Rule Definition
47+
//------------------------------------------------------------------------------
48+
49+
module.exports = {
50+
meta: {
51+
type: "problem",
52+
53+
docs: {
54+
description: "disallow duplicate conditions in if-else-if chains",
55+
category: "Possible Errors",
56+
recommended: false,
57+
url: "https://eslint.org/docs/rules/no-dupe-else-if"
58+
},
59+
60+
schema: [],
61+
62+
messages: {
63+
unexpected: "This branch can never execute. Its condition is a duplicate or covered by previous conditions in the if-else-if chain."
64+
}
65+
},
66+
67+
create(context) {
68+
const sourceCode = context.getSourceCode();
69+
70+
/**
71+
* Determines whether the two given nodes are considered to be equal. In particular, given that the nodes
72+
* represent expressions in a boolean context, `||` and `&&` can be considered as commutative operators.
73+
* @param {ASTNode} a First node.
74+
* @param {ASTNode} b Second node.
75+
* @returns {boolean} `true` if the nodes are considered to be equal.
76+
*/
77+
function equal(a, b) {
78+
if (a.type !== b.type) {
79+
return false;
80+
}
81+
82+
if (
83+
a.type === "LogicalExpression" &&
84+
(a.operator === "||" || a.operator === "&&") &&
85+
a.operator === b.operator
86+
) {
87+
return equal(a.left, b.left) && equal(a.right, b.right) ||
88+
equal(a.left, b.right) && equal(a.right, b.left);
89+
}
90+
91+
return astUtils.equalTokens(a, b, sourceCode);
92+
}
93+
94+
const isSubset = isSubsetByComparator.bind(null, equal);
95+
96+
return {
97+
IfStatement(node) {
98+
const test = node.test,
99+
conditionsToCheck = test.type === "LogicalExpression" && test.operator === "&&"
100+
? [test, ...splitByAnd(test)]
101+
: [test];
102+
let current = node,
103+
listToCheck = conditionsToCheck.map(c => splitByOr(c).map(splitByAnd));
104+
105+
while (current.parent && current.parent.type === "IfStatement" && current.parent.alternate === current) {
106+
current = current.parent;
107+
108+
const currentOrOperands = splitByOr(current.test).map(splitByAnd);
109+
110+
listToCheck = listToCheck.map(orOperands => orOperands.filter(
111+
orOperand => !currentOrOperands.some(currentOrOperand => isSubset(currentOrOperand, orOperand))
112+
));
113+
114+
if (listToCheck.some(orOperands => orOperands.length === 0)) {
115+
context.report({ node: test, messageId: "unexpected" });
116+
break;
117+
}
118+
}
119+
}
120+
};
121+
}
122+
};

0 commit comments

Comments
 (0)