feat(eslint-plugin): [no-unnecessary-condition] add rule

Adds a no-unnecessary-condition, a rule that uses type information to determine if a condition is necessary. Any condition that's indicated by types to always be truthy or falsy is considered to be unnecessary. Essentially a stronger version of no-constant-condition from core eslint. For example: ```ts const items = ['foo', 'bar']; if(items) { } // Error: items is always truthy ```
typescript-eslint · Jul 12, 2019 · dbf5656 · dbf5656
1 parent 528c432
commit dbf5656
Show file tree

Hide file tree

Showing 6 changed files with 323 additions and 0 deletions.
diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md
@@ -160,6 +160,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e
 | [`@typescript-eslint/no-require-imports`](./docs/rules/no-require-imports.md)                             | Disallows invocation of `require()`                                                                                                                 |                    |          |                   |
 | [`@typescript-eslint/no-this-alias`](./docs/rules/no-this-alias.md)                                       | Disallow aliasing `this`                                                                                                                            |                    |          |                   |
 | [`@typescript-eslint/no-type-alias`](./docs/rules/no-type-alias.md)                                       | Disallow the use of type aliases                                                                                                                    |                    |          |                   |
+| [`@typescript-eslint/no-unnecessary-condition`](./docs/rules/no-unnecessary-condition.md)                 | Prevents conditionals where the type is always truthy or always falsy                                                                               |                    |          | :thought_balloon: |
 | [`@typescript-eslint/no-unnecessary-qualifier`](./docs/rules/no-unnecessary-qualifier.md)                 | Warns when a namespace qualifier is unnecessary                                                                                                     |                    | :wrench: | :thought_balloon: |
 | [`@typescript-eslint/no-unnecessary-type-assertion`](./docs/rules/no-unnecessary-type-assertion.md)       | Warns if a type assertion does not change the type of an expression                                                                                 |                    | :wrench: | :thought_balloon: |
 | [`@typescript-eslint/no-unused-vars`](./docs/rules/no-unused-vars.md)                                     | Disallow unused variables                                                                                                                           | :heavy_check_mark: |          |                   |

diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md
@@ -0,0 +1,57 @@
+# Condition expressions must be necessary
+
+Any expression being used as a condition must be able to evaluate as truthy or falsy in order to be considered "necessary". Conversely, any expression that always evaluates to truthy or always evaluates to falsy, as determined by the type of the expression, is considered unnecessary and will be flagged by this rule.
+
+The following expressions are checked:
+
+- Arguments to the `&&`, `||` and `?:` (ternary) operators
+- Conditions for `if`, `for`, `while`, and `do-while` statements.
+
+Examples of **incorrect** code for this rule:
+
+```ts
+function head<T>(items: T[]) {
+  if (items) {
+    return items[0].toUpperCase();
+  }
+}
+
+const foo = 'foo';
+if (foo) {
+}
+```
+
+Examples of **correct** code for this rule:
+
+```ts
+function head<T>(items: T[]) {
+  if (items.length) {
+    return items[0].toUpperCase();
+  }
+}
+
+declare const foo: string;
+// Necessary, since foo might be ''. (If undesired, consider using `strict-boolean-expressions` rule)
+if (foo) {
+}
+```
+
+## Options
+
+Accepts an object with the following options:
+
+- `ignoreRhs` (default `false`) - doesn't check if the right-hand side of `&&` and `||` is a necessary condition. For example, the following code is valid with this option on:
+
+```ts
+function head<T>(items: T[]) {
+  return items.length && items[0].toUpperCase();
+}
+```
+
+## When Not To Use It
+
+The main downside to using this rule is the need for type information.
+
+## Related To
+
+- ESLint: [no-constant-condition](https://eslint.org/docs/rules/no-constant-condition) - this rule is essentially a stronger versison
diff --git a/packages/eslint-plugin/src/configs/all.json b/packages/eslint-plugin/src/configs/all.json
@@ -43,6 +43,7 @@
     "@typescript-eslint/no-require-imports": "error",
     "@typescript-eslint/no-this-alias": "error",
     "@typescript-eslint/no-type-alias": "error",
+    "@typescript-eslint/no-unnecessary-condition": "error",
     "@typescript-eslint/no-unnecessary-qualifier": "error",
     "@typescript-eslint/no-unnecessary-type-assertion": "error",
     "no-unused-vars": "off",

diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts
@@ -35,6 +35,7 @@ import noRequireImports from './no-require-imports';
 import noThisAlias from './no-this-alias';
 import noTripleSlashReference from './no-triple-slash-reference';
 import noTypeAlias from './no-type-alias';
+import noUnnecessaryCondition from './no-unnecessary-condition';
 import noUnnecessaryQualifier from './no-unnecessary-qualifier';
 import noUnnecessaryTypeAssertion from './no-unnecessary-type-assertion';
 import noUnusedVars from './no-unused-vars';
@@ -97,6 +98,7 @@ export default {
   'no-this-alias': noThisAlias,
   'no-triple-slash-reference': noTripleSlashReference,
   'no-type-alias': noTypeAlias,
+  'no-unnecessary-condition': noUnnecessaryCondition,
   'no-unnecessary-qualifier': noUnnecessaryQualifier,
   'no-unnecessary-type-assertion': noUnnecessaryTypeAssertion,
   'no-unused-vars': noUnusedVars,

diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
@@ -0,0 +1,143 @@
+import {
+  TSESTree,
+  AST_NODE_TYPES,
+} from '@typescript-eslint/experimental-utils';
+import ts, { TypeFlags } from 'typescript';
+import {
+  isTypeFlagSet,
+  unionTypeParts,
+  isFalsyType,
+  isBooleanLiteralType,
+  isLiteralType,
+} from 'tsutils';
+import {
+  createRule,
+  getParserServices,
+  getConstrainedTypeAtLocation,
+} from '../util';
+
+// Truthiness utilities
+// #region
+const isTruthyLiteral = (type: ts.Type) =>
+  isBooleanLiteralType(type, true) || (isLiteralType(type) && type.value);
+
+const isPossiblyFalsy = (type: ts.Type) =>
+  unionTypeParts(type)
+    // PossiblyFalsy flag includes literal values, so exclude ones that
+    // are definitely truthy
+    .filter(t => !isTruthyLiteral(t))
+    .some(type => isTypeFlagSet(type, ts.TypeFlags.PossiblyFalsy));
+
+const isPossiblyTruthy = (type: ts.Type) =>
+  unionTypeParts(type).some(type => !isFalsyType(type));
+// #endregion
+
+type ExpressionWithTest =
+  | TSESTree.ConditionalExpression
+  | TSESTree.DoWhileStatement
+  | TSESTree.ForStatement
+  | TSESTree.IfStatement
+  | TSESTree.WhileStatement;
+
+type Options = [
+  {
+    ignoreRhs?: boolean;
+  }
+];
+
+export default createRule<Options, 'alwaysTruthy' | 'alwaysFalsy' | 'never'>({
+  name: 'no-unnecessary-conditionals',
+  meta: {
+    type: 'suggestion',
+    docs: {
+      description:
+        'Prevents conditionals where the type is always truthy or always falsy',
+      category: 'Best Practices',
+      recommended: false,
+    },
+    schema: [
+      {
+        type: 'object',
+        properties: {
+          ignoreRhs: {
+            type: 'boolean',
+          },
+        },
+        additionalProperties: false,
+      },
+    ],
+    messages: {
+      alwaysTruthy: 'Unnecessary conditional, value is always truthy.',
+      alwaysFalsy: 'Unnecessary conditional, value is always falsy.',
+      never: 'Unnecessary conditional, value is `never`',
+    },
+  },
+  defaultOptions: [
+    {
+      ignoreRhs: false,
+    },
+  ],
+  create(context, [{ ignoreRhs }]) {
+    const service = getParserServices(context);
+    const checker = service.program.getTypeChecker();
+
+    /**
+     * Checks if a conditional node is necessary:
+     * if the type of the node is always true or always false, it's not necessary.
+     */
+    function checkNode(node: TSESTree.Node): void {
+      const tsNode = service.esTreeNodeToTSNodeMap.get<ts.ExpressionStatement>(
+        node,
+      );
+      const type = getConstrainedTypeAtLocation(checker, tsNode);
+
+      // Conditional is always necessary if it involves `any` or `unknown`
+      if (isTypeFlagSet(type, TypeFlags.Any | TypeFlags.Unknown)) {
+        return;
+      }
+      if (isTypeFlagSet(type, TypeFlags.Never)) {
+        context.report({ node, messageId: 'never' });
+      } else if (!isPossiblyTruthy(type)) {
+        context.report({ node, messageId: 'alwaysFalsy' });
+      } else if (!isPossiblyFalsy(type)) {
+        context.report({ node, messageId: 'alwaysTruthy' });
+      }
+    }
+
+    /**
+     * Checks that a testable expression is necessarily conditional, reports otherwise.
+     * Filters all LogicalExpressions to prevent some duplicate reports.
+     */
+    function checkIfTestExpressionIsNecessaryConditional(
+      node: ExpressionWithTest,
+    ): void {
+      if (
+        node.test !== null &&
+        node.test.type !== AST_NODE_TYPES.LogicalExpression
+      ) {
+        checkNode(node.test);
+      }
+    }
+
+    /**
+     * Checks that a logical expression contains a boolean, reports otherwise.
+     */
+    function checkLogicalExpressionForUnnecessaryConditionals(
+      node: TSESTree.LogicalExpression,
+    ): void {
+      checkNode(node.left);
+      if (!ignoreRhs) {
+        checkNode(node.right);
+      }
+    }
+
+    return {
+      ConditionalExpression: checkIfTestExpressionIsNecessaryConditional,
+      DoWhileStatement: checkIfTestExpressionIsNecessaryConditional,
+      ForStatement: checkIfTestExpressionIsNecessaryConditional,
+      IfStatement: checkIfTestExpressionIsNecessaryConditional,
+      WhileStatement: checkIfTestExpressionIsNecessaryConditional,
+      LogicalExpression: checkLogicalExpressionForUnnecessaryConditionals,
+    };
+  },
+});
diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts
@@ -0,0 +1,119 @@
+import path from 'path';
+import rule from '../../src/rules/no-unnecessary-condition';
+import { RuleTester } from '../RuleTester';
+
+const rootPath = path.join(process.cwd(), 'tests/fixtures/');
+
+const ruleTester = new RuleTester({
+  parser: '@typescript-eslint/parser',
+  parserOptions: {
+    tsconfigRootDir: rootPath,
+    project: './tsconfig.json',
+  },
+});
+
+type MessageId = 'alwaysTruthy' | 'alwaysFalsy' | 'never';
+const ruleError = (line: number, column: number, messageId: MessageId) => ({
+  messageId,
+  line,
+  column,
+});
+
+const necessaryConditionTest = (condition: string) => `
+declare const b1: ${condition};
+declare const b2: boolean;
+const t1 = b1 && b2;
+`;
+
+const unnecessaryConditionTest = (condition: string, messageId: MessageId) => ({
+  code: necessaryConditionTest(condition),
+  errors: [ruleError(4, 12, messageId)],
+});
+
+ruleTester.run('no-unnecessary-conditionals', rule, {
+  valid: [
+    `
+declare const b1: boolean;
+declare const b2: boolean;
+const t1 = b1 && b2;
+const t2 = b1 || b2;
+if(b1 && b2) {}
+while(b1 && b2) {}
+for (let i = 0; (b1 && b2); i++) { break; }
+const t1 = (b1 && b2) ? 'yes' : 'no'`,
+    necessaryConditionTest('false | 5'), // Truthy literal and falsy literal
+    necessaryConditionTest('boolean | "foo"'), // boolean and truthy literal
+    necessaryConditionTest('0 | boolean'), // boolean and falsy literal
+    necessaryConditionTest('boolean | object'), // boolean and always-truthy type
+    necessaryConditionTest('false | object'), // always truthy type and falsy literal
+    // always falsy type and always truthy type
+    necessaryConditionTest('null | object'),
+    necessaryConditionTest('undefined | true'),
+    necessaryConditionTest('void | true'),
+
+    necessaryConditionTest('any'), // any
+    necessaryConditionTest('unknown'), // unknown
+
+    // Supports ignoring the RHS
+    {
+      code: `
+declare const b1: boolean;
+declare const b2: true;
+if(b1 && b2) {}`,
+      options: [{ ignoreRhs: true }],
+    },
+  ],
+  invalid: [
+    // Ensure that it's checking in all the right places
+    {
+      code: `
+const b1 = true;
+declare const b2: boolean;
+const t1 = b1 && b2;
+const t2 = b1 || b2;
+if(b1 && b2) {}
+while(b1 && b2) {}
+for (let i = 0; (b1 && b2); i++) { break; }
+const t1 = (b1 && b2) ? 'yes' : 'no'`,
+      errors: [
+        ruleError(4, 12, 'alwaysTruthy'),
+        ruleError(5, 12, 'alwaysTruthy'),
+        ruleError(6, 4, 'alwaysTruthy'),
+        ruleError(7, 7, 'alwaysTruthy'),
+        ruleError(8, 18, 'alwaysTruthy'),
+        ruleError(9, 13, 'alwaysTruthy'),
+      ],
+    },
+    // Ensure that it's complaining about the right things
+    unnecessaryConditionTest('object', 'alwaysTruthy'),
+    unnecessaryConditionTest('object | true', 'alwaysTruthy'),
+    unnecessaryConditionTest('"" | false', 'alwaysFalsy'), // Two falsy literals
+    unnecessaryConditionTest('"always truthy"', 'alwaysTruthy'),
+    unnecessaryConditionTest(`undefined`, 'alwaysFalsy'),
+    unnecessaryConditionTest(`null`, 'alwaysFalsy'),
+    unnecessaryConditionTest(`void`, 'alwaysFalsy'),
+    unnecessaryConditionTest('never', 'never'),
+
+    // Still errors on in the expected locations when ignoring RHS
+    {
+      options: [{ ignoreRhs: true }],
+      code: `
+const b1 = true;
+const b2 = false;
+const t1 = b1 && b2;
+const t2 = b1 || b2;
+if(b1 && b2) {}
+while(b1 && b2) {}
+for (let i = 0; (b1 && b2); i++) { break; }
+const t1 = (b1 && b2) ? 'yes' : 'no'`,
+      errors: [
+        ruleError(4, 12, 'alwaysTruthy'),
+        ruleError(5, 12, 'alwaysTruthy'),
+        ruleError(6, 4, 'alwaysTruthy'),
+        ruleError(7, 7, 'alwaysTruthy'),
+        ruleError(8, 18, 'alwaysTruthy'),
+        ruleError(9, 13, 'alwaysTruthy'),
+      ],
+    },
+  ],
+});