Skip to content
This repository was archived by the owner on Oct 3, 2024. It is now read-only.

Commit 345151b

Browse files
Rule S2589: Boolean expressions should not be gratuitous (#231)
1 parent afd2c19 commit 345151b

File tree

6 files changed

+485
-0
lines changed

6 files changed

+485
-0
lines changed

README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ Code Smells, or maintainability issues, are raised for places of code which migh
2929
* Collection sizes and array length comparisons should make sense ([`no-collection-size-mischeck`])
3030
* String literals should not be duplicated ([`no-duplicate-string`])
3131
* Two branches in a conditional structure should not have exactly the same implementation ([`no-duplicated-branches`])
32+
* Boolean expressions should not be gratuitous ([`no-gratuitous-expressions`])
3233
* Functions should not have identical implementations ([`no-identical-functions`])
3334
* Boolean checks should not be inverted ([`no-inverted-boolean-check`]) (:wrench: *fixable*)
3435
* "switch" statements should not be nested ([`no-nested-switch`])
@@ -55,6 +56,7 @@ Code Smells, or maintainability issues, are raised for places of code which migh
5556
[`no-element-overwrite`]: ./docs/rules/no-element-overwrite.md
5657
[`no-empty-collection`]: ./docs/rules/no-empty-collection.md
5758
[`no-extra-arguments`]: ./docs/rules/no-extra-arguments.md
59+
[`no-gratuitous-expressions`]: ./docs/rules/no-gratuitous-expressions.md
5860
[`no-identical-conditions`]: ./docs/rules/no-identical-conditions.md
5961
[`no-identical-expressions`]: ./docs/rules/no-identical-expressions.md
6062
[`no-identical-functions`]: ./docs/rules/no-identical-functions.md
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# no-gratuitous-expressions
2+
3+
If a boolean expression doesn’t change the evaluation of the condition, then it is entirely unnecessary, and can be removed. If it is gratuitous
4+
because it does not match the programmer’s intent, then it’s a bug and the expression should be fixed.
5+
6+
## Noncompliant Code Example
7+
8+
```javascript
9+
if (a) {
10+
if (a) { // Noncompliant
11+
doSomething();
12+
}
13+
}
14+
```
15+
16+
## Compliant Solution
17+
18+
```javascript
19+
if (a) {
20+
if (b) {
21+
doSomething();
22+
}
23+
}
24+
25+
// or
26+
if (a) {
27+
doSomething();
28+
}
29+
```
30+
31+
## See
32+
33+
<ul>
34+
<li> <a href="http://cwe.mitre.org/data/definitions/571">MITRE, CWE-571</a> - Expression is Always True </li>
35+
<li> <a href="http://cwe.mitre.org/data/definitions/570">MITRE, CWE-570</a> - Expression is Always False </li>
36+
</ul>
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
src/brackets/src/search/QuickOpen.js: 444
2+
src/jest/packages/jest-cli/src/reporters/summary_reporter.js: 101
3+
src/react-native/Libraries/Animated/src/AnimatedImplementation.js: 98
4+
src/react-native/Libraries/Renderer/ReactFabric-prod.js: 254
5+
src/react-native/Libraries/Renderer/ReactNativeRenderer-prod.js: 350
6+
src/spectrum/src/components/avatar/hoverProfile.js: 131
7+
src/spectrum/src/components/stripeCardForm/modalWell.js: 54
8+
src/spectrum/src/views/navbar/index.js: 200,215

src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ const sonarjsRules: [string, TSESLint.Linter.RuleLevel][] = [
3131
['no-element-overwrite', 'error'],
3232
['no-empty-collection', 'error'],
3333
['no-extra-arguments', 'error'],
34+
['no-gratuitous-expressions', 'error'],
3435
['no-identical-conditions', 'error'],
3536
['no-identical-functions', 'error'],
3637
['no-identical-expressions', 'error'],
Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,242 @@
1+
/*
2+
* eslint-plugin-sonarjs
3+
* Copyright (C) 2018-2021 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
21+
import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';
22+
import { EncodedMessage } from '../utils/locations';
23+
import { isIdentifier, isIfStatement } from '../utils/nodes';
24+
import { Rule } from '../utils/types';
25+
26+
const rule: Rule.RuleModule = {
27+
meta: {
28+
type: 'suggestion',
29+
schema: [
30+
{
31+
// internal parameter for rules having secondary locations
32+
enum: ['sonar-runtime'],
33+
},
34+
],
35+
},
36+
37+
create(context: Rule.RuleContext) {
38+
const truthyMap: Map<TSESTree.Statement, TSESLint.Scope.Reference[]> = new Map();
39+
const falsyMap: Map<TSESTree.Statement, TSESLint.Scope.Reference[]> = new Map();
40+
41+
return {
42+
IfStatement: (node: TSESTree.Node) => {
43+
const { test } = node as TSESTree.IfStatement;
44+
if (test.type === 'Literal' && typeof test.value === 'boolean') {
45+
report(test, undefined, context, test.value);
46+
}
47+
},
48+
49+
':statement': (node: TSESTree.Node) => {
50+
const { parent } = node;
51+
if (isIfStatement(parent)) {
52+
// we visit 'consequent' and 'alternate' and not if-statement directly in order to get scope for 'consequent'
53+
const currentScope = context.getScope();
54+
55+
if (parent.consequent === node) {
56+
const { truthy, falsy } = collectKnownIdentifiers(parent.test);
57+
truthyMap.set(parent.consequent, transformAndFilter(truthy, currentScope));
58+
falsyMap.set(parent.consequent, transformAndFilter(falsy, currentScope));
59+
} else if (parent.alternate === node && isIdentifier(parent.test)) {
60+
falsyMap.set(parent.alternate, transformAndFilter([parent.test], currentScope));
61+
}
62+
}
63+
},
64+
65+
':statement:exit': (node: TSESTree.Node) => {
66+
const stmt = node as TSESTree.Statement;
67+
truthyMap.delete(stmt);
68+
falsyMap.delete(stmt);
69+
},
70+
71+
Identifier: (node: TSESTree.Node) => {
72+
const id = node as TSESTree.Identifier;
73+
const symbol = getSymbol(id, context.getScope());
74+
const { parent } = node;
75+
if (!symbol || !parent) {
76+
return;
77+
}
78+
if (
79+
!isLogicalAnd(parent) &&
80+
!isLogicalOrLhs(id, parent) &&
81+
!isIfStatement(parent) &&
82+
!isLogicalNegation(parent)
83+
) {
84+
return;
85+
}
86+
87+
const checkIfKnownAndReport = (
88+
map: Map<TSESTree.Statement, TSESLint.Scope.Reference[]>,
89+
truthy: boolean,
90+
) => {
91+
map.forEach(references => {
92+
const ref = references.find(ref => ref.resolved === symbol);
93+
if (ref) {
94+
report(id, ref, context, truthy);
95+
}
96+
});
97+
};
98+
99+
checkIfKnownAndReport(truthyMap, true);
100+
checkIfKnownAndReport(falsyMap, false);
101+
},
102+
103+
Program: () => {
104+
truthyMap.clear();
105+
falsyMap.clear();
106+
},
107+
};
108+
},
109+
};
110+
111+
function collectKnownIdentifiers(expression: TSESTree.Expression) {
112+
const truthy: TSESTree.Identifier[] = [];
113+
const falsy: TSESTree.Identifier[] = [];
114+
115+
const checkExpr = (expr: TSESTree.Expression) => {
116+
if (isIdentifier(expr)) {
117+
truthy.push(expr);
118+
} else if (isLogicalNegation(expr)) {
119+
if (isIdentifier(expr.argument)) {
120+
falsy.push(expr.argument);
121+
} else if (isLogicalNegation(expr.argument) && isIdentifier(expr.argument.argument)) {
122+
truthy.push(expr.argument.argument);
123+
}
124+
}
125+
};
126+
127+
let current = expression;
128+
checkExpr(current);
129+
while (isLogicalAnd(current)) {
130+
checkExpr(current.right);
131+
current = current.left;
132+
}
133+
checkExpr(current);
134+
135+
return { truthy, falsy };
136+
}
137+
138+
function isLogicalAnd(expression: TSESTree.Node): expression is TSESTree.LogicalExpression {
139+
return expression.type === 'LogicalExpression' && expression.operator === '&&';
140+
}
141+
142+
function isLogicalOrLhs(
143+
id: TSESTree.Identifier,
144+
expression: TSESTree.Node,
145+
): expression is TSESTree.LogicalExpression {
146+
return (
147+
expression.type === 'LogicalExpression' &&
148+
expression.operator === '||' &&
149+
expression.left === id
150+
);
151+
}
152+
153+
function isLogicalNegation(expression: TSESTree.Node): expression is TSESTree.UnaryExpression {
154+
return expression.type === 'UnaryExpression' && expression.operator === '!';
155+
}
156+
157+
function isDefined<T>(x: T | undefined | null): x is T {
158+
return x != null;
159+
}
160+
161+
function getSymbol(id: TSESTree.Identifier, scope: TSESLint.Scope.Scope) {
162+
const ref = scope.references.find(r => r.identifier === id);
163+
if (ref) {
164+
return ref.resolved;
165+
}
166+
return null;
167+
}
168+
169+
function getFunctionScope(scope: TSESLint.Scope.Scope): TSESLint.Scope.Scope | null {
170+
if (scope.type === 'function') {
171+
return scope;
172+
} else if (!scope.upper) {
173+
return null;
174+
}
175+
return getFunctionScope(scope.upper);
176+
}
177+
178+
function mightBeWritten(symbol: TSESLint.Scope.Variable, currentScope: TSESLint.Scope.Scope) {
179+
return symbol.references
180+
.filter(ref => ref.isWrite())
181+
.find(ref => {
182+
const refScope = ref.from;
183+
184+
let cur: TSESLint.Scope.Scope | null = refScope;
185+
while (cur) {
186+
if (cur === currentScope) {
187+
return true;
188+
}
189+
cur = cur.upper;
190+
}
191+
192+
const currentFunc = getFunctionScope(currentScope);
193+
const refFunc = getFunctionScope(refScope);
194+
return refFunc !== currentFunc;
195+
});
196+
}
197+
198+
function transformAndFilter(ids: TSESTree.Identifier[], currentScope: TSESLint.Scope.Scope) {
199+
return ids
200+
.map(id => currentScope.upper!.references.find(r => r.identifier === id))
201+
.filter(isDefined)
202+
.filter(ref => isDefined(ref.resolved))
203+
.filter(ref => !mightBeWritten(ref.resolved!, currentScope));
204+
}
205+
206+
function report(
207+
id: TSESTree.Node,
208+
ref: TSESLint.Scope.Reference | undefined,
209+
context: Rule.RuleContext,
210+
truthy: boolean,
211+
) {
212+
const value = truthy ? 'truthy' : 'falsy';
213+
214+
const encodedMessage: EncodedMessage = {
215+
message: `This always evaluates to ${value}. Consider refactoring this code.`,
216+
secondaryLocations: getSecondaryLocations(ref, value),
217+
};
218+
219+
context.report({
220+
message: JSON.stringify(encodedMessage),
221+
node: id,
222+
});
223+
}
224+
225+
function getSecondaryLocations(ref: TSESLint.Scope.Reference | undefined, truthy: string) {
226+
if (ref) {
227+
const secLoc = ref.identifier.loc!;
228+
return [
229+
{
230+
message: `Evaluated here to be ${truthy}`,
231+
line: secLoc.start.line,
232+
column: secLoc.start.column,
233+
endLine: secLoc.end.line,
234+
endColumn: secLoc.end.column,
235+
},
236+
];
237+
} else {
238+
return [];
239+
}
240+
}
241+
242+
export = rule;

0 commit comments

Comments
 (0)