Rule S2589: Boolean expressions should not be gratuitous (#231)

README.md

@@ -29,6 +29,7 @@ Code Smells, or maintainability issues, are raised for places of code which migh
 * Collection sizes and array length comparisons should make sense ([`no-collection-size-mischeck`])
 * String literals should not be duplicated ([`no-duplicate-string`])
 * Two branches in a conditional structure should not have exactly the same implementation ([`no-duplicated-branches`])
+* Boolean expressions should not be gratuitous ([`no-gratuitous-expressions`])
 * Functions should not have identical implementations ([`no-identical-functions`])
 * Boolean checks should not be inverted ([`no-inverted-boolean-check`]) (:wrench: *fixable*)
 * "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
 [`no-element-overwrite`]: ./docs/rules/no-element-overwrite.md
 [`no-empty-collection`]: ./docs/rules/no-empty-collection.md
 [`no-extra-arguments`]: ./docs/rules/no-extra-arguments.md
+[`no-gratuitous-expressions`]: ./docs/rules/no-gratuitous-expressions.md
 [`no-identical-conditions`]: ./docs/rules/no-identical-conditions.md
 [`no-identical-expressions`]: ./docs/rules/no-identical-expressions.md
 [`no-identical-functions`]: ./docs/rules/no-identical-functions.md

docs/rules/no-gratuitous-expressions.md

@@ -0,0 +1,36 @@
+# no-gratuitous-expressions
+
+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
+because it does not match the programmer’s intent, then it’s a bug and the expression should be fixed.
+
+## Noncompliant Code Example
+
+```javascript
+if (a) {
+  if (a) { // Noncompliant
+    doSomething();
+  }
+}
+```
+
+## Compliant Solution
+
+```javascript
+if (a) {
+  if (b) {
+    doSomething();
+  }
+}
+
+// or
+if (a) {
+  doSomething();
+}
+```
+
+## See
+
+<ul>
+  <li> <a href="http://cwe.mitre.org/data/definitions/571">MITRE, CWE-571</a> - Expression is Always True </li>
+  <li> <a href="http://cwe.mitre.org/data/definitions/570">MITRE, CWE-570</a> - Expression is Always False </li>
+</ul>

ruling/snapshots/no-gratuitous-expressions

@@ -0,0 +1,8 @@
+src/brackets/src/search/QuickOpen.js: 444
+src/jest/packages/jest-cli/src/reporters/summary_reporter.js: 101
+src/react-native/Libraries/Animated/src/AnimatedImplementation.js: 98
+src/react-native/Libraries/Renderer/ReactFabric-prod.js: 254
+src/react-native/Libraries/Renderer/ReactNativeRenderer-prod.js: 350
+src/spectrum/src/components/avatar/hoverProfile.js: 131
+src/spectrum/src/components/stripeCardForm/modalWell.js: 54
+src/spectrum/src/views/navbar/index.js: 200,215

src/index.ts

@@ -31,6 +31,7 @@ const sonarjsRules: [string, TSESLint.Linter.RuleLevel][] = [
   ['no-element-overwrite', 'error'],
   ['no-empty-collection', 'error'],
   ['no-extra-arguments', 'error'],
+  ['no-gratuitous-expressions', 'error'],
   ['no-identical-conditions', 'error'],
   ['no-identical-functions', 'error'],
   ['no-identical-expressions', 'error'],

src/rules/no-gratuitous-expressions.ts

@@ -0,0 +1,242 @@
+/*
+ * eslint-plugin-sonarjs
+ * Copyright (C) 2018-2021 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';
+import { EncodedMessage } from '../utils/locations';
+import { isIdentifier, isIfStatement } from '../utils/nodes';
+import { Rule } from '../utils/types';
+
+const rule: Rule.RuleModule = {
+  meta: {
+    type: 'suggestion',
+    schema: [
+      {
+        // internal parameter for rules having secondary locations
+        enum: ['sonar-runtime'],
+      },
+    ],
+  },
+
+  create(context: Rule.RuleContext) {
+    const truthyMap: Map<TSESTree.Statement, TSESLint.Scope.Reference[]> = new Map();
+    const falsyMap: Map<TSESTree.Statement, TSESLint.Scope.Reference[]> = new Map();
+
+    return {
+      IfStatement: (node: TSESTree.Node) => {
+        const { test } = node as TSESTree.IfStatement;
+        if (test.type === 'Literal' && typeof test.value === 'boolean') {
+          report(test, undefined, context, test.value);
+        }
+      },
+
+      ':statement': (node: TSESTree.Node) => {
+        const { parent } = node;
+        if (isIfStatement(parent)) {
+          // we visit 'consequent' and 'alternate' and not if-statement directly in order to get scope for 'consequent'
+          const currentScope = context.getScope();
+
+          if (parent.consequent === node) {
+            const { truthy, falsy } = collectKnownIdentifiers(parent.test);
+            truthyMap.set(parent.consequent, transformAndFilter(truthy, currentScope));
+            falsyMap.set(parent.consequent, transformAndFilter(falsy, currentScope));
+          } else if (parent.alternate === node && isIdentifier(parent.test)) {
+            falsyMap.set(parent.alternate, transformAndFilter([parent.test], currentScope));
+          }
+        }
+      },
+
+      ':statement:exit': (node: TSESTree.Node) => {
+        const stmt = node as TSESTree.Statement;
+        truthyMap.delete(stmt);
+        falsyMap.delete(stmt);
+      },
+
+      Identifier: (node: TSESTree.Node) => {
+        const id = node as TSESTree.Identifier;
+        const symbol = getSymbol(id, context.getScope());
+        const { parent } = node;
+        if (!symbol || !parent) {
+          return;
+        }
+        if (
+          !isLogicalAnd(parent) &&
+          !isLogicalOrLhs(id, parent) &&
+          !isIfStatement(parent) &&
+          !isLogicalNegation(parent)
+        ) {
+          return;
+        }
+
+        const checkIfKnownAndReport = (
+          map: Map<TSESTree.Statement, TSESLint.Scope.Reference[]>,
+          truthy: boolean,
+        ) => {
+          map.forEach(references => {
+            const ref = references.find(ref => ref.resolved === symbol);
+            if (ref) {
+              report(id, ref, context, truthy);
+            }
+          });
+        };
+
+        checkIfKnownAndReport(truthyMap, true);
+        checkIfKnownAndReport(falsyMap, false);
+      },
+
+      Program: () => {
+        truthyMap.clear();
+        falsyMap.clear();
+      },
+    };
+  },
+};
+
+function collectKnownIdentifiers(expression: TSESTree.Expression) {
+  const truthy: TSESTree.Identifier[] = [];
+  const falsy: TSESTree.Identifier[] = [];
+
+  const checkExpr = (expr: TSESTree.Expression) => {
+    if (isIdentifier(expr)) {
+      truthy.push(expr);
+    } else if (isLogicalNegation(expr)) {
+      if (isIdentifier(expr.argument)) {
+        falsy.push(expr.argument);
+      } else if (isLogicalNegation(expr.argument) && isIdentifier(expr.argument.argument)) {
+        truthy.push(expr.argument.argument);
+      }
+    }
+  };
+
+  let current = expression;
+  checkExpr(current);
+  while (isLogicalAnd(current)) {
+    checkExpr(current.right);
+    current = current.left;
+  }
+  checkExpr(current);
+
+  return { truthy, falsy };
+}
+
+function isLogicalAnd(expression: TSESTree.Node): expression is TSESTree.LogicalExpression {
+  return expression.type === 'LogicalExpression' && expression.operator === '&&';
+}
+
+function isLogicalOrLhs(
+  id: TSESTree.Identifier,
+  expression: TSESTree.Node,
+): expression is TSESTree.LogicalExpression {
+  return (
+    expression.type === 'LogicalExpression' &&
+    expression.operator === '||' &&
+    expression.left === id
+  );
+}
+
+function isLogicalNegation(expression: TSESTree.Node): expression is TSESTree.UnaryExpression {
+  return expression.type === 'UnaryExpression' && expression.operator === '!';
+}
+
+function isDefined<T>(x: T | undefined | null): x is T {
+  return x != null;
+}
+
+function getSymbol(id: TSESTree.Identifier, scope: TSESLint.Scope.Scope) {
+  const ref = scope.references.find(r => r.identifier === id);
+  if (ref) {
+    return ref.resolved;
+  }
+  return null;
+}
+
+function getFunctionScope(scope: TSESLint.Scope.Scope): TSESLint.Scope.Scope | null {
+  if (scope.type === 'function') {
+    return scope;
+  } else if (!scope.upper) {
+    return null;
+  }
+  return getFunctionScope(scope.upper);
+}
+
+function mightBeWritten(symbol: TSESLint.Scope.Variable, currentScope: TSESLint.Scope.Scope) {
+  return symbol.references
+    .filter(ref => ref.isWrite())
+    .find(ref => {
+      const refScope = ref.from;
+
+      let cur: TSESLint.Scope.Scope | null = refScope;
+      while (cur) {
+        if (cur === currentScope) {
+          return true;
+        }
+        cur = cur.upper;
+      }
+
+      const currentFunc = getFunctionScope(currentScope);
+      const refFunc = getFunctionScope(refScope);
+      return refFunc !== currentFunc;
+    });
+}
+
+function transformAndFilter(ids: TSESTree.Identifier[], currentScope: TSESLint.Scope.Scope) {
+  return ids
+    .map(id => currentScope.upper!.references.find(r => r.identifier === id))
+    .filter(isDefined)
+    .filter(ref => isDefined(ref.resolved))
+    .filter(ref => !mightBeWritten(ref.resolved!, currentScope));
+}
+
+function report(
+  id: TSESTree.Node,
+  ref: TSESLint.Scope.Reference | undefined,
+  context: Rule.RuleContext,
+  truthy: boolean,
+) {
+  const value = truthy ? 'truthy' : 'falsy';
+
+  const encodedMessage: EncodedMessage = {
+    message: `This always evaluates to ${value}. Consider refactoring this code.`,
+    secondaryLocations: getSecondaryLocations(ref, value),
+  };
+
+  context.report({
+    message: JSON.stringify(encodedMessage),
+    node: id,
+  });
+}
+
+function getSecondaryLocations(ref: TSESLint.Scope.Reference | undefined, truthy: string) {
+  if (ref) {
+    const secLoc = ref.identifier.loc!;
+    return [
+      {
+        message: `Evaluated here to be ${truthy}`,
+        line: secLoc.start.line,
+        column: secLoc.start.column,
+        endLine: secLoc.end.line,
+        endColumn: secLoc.end.column,
+      },
+    ];
+  } else {
+    return [];
+  }
+}
+
+export = rule;