feat(eslint-plugin): [no-unnec-cond] check optional chaining (#1315)

jridgewell · bradzacher · bradzacher · commit a2a8a0a90195 · 2019-12-16T23:43:34.000+10:30
Co-authored-by: Brad Zacher &lt;brad.zacher@gmail.com&gt;
diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md
@@ -179,7 +179,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`                                                                                                                            | :heavy_check_mark: |          |                   |
 | [`@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-condition`](./docs/rules/no-unnecessary-condition.md)                 | Prevents conditionals where the type is always truthy or always falsy                                                                               |                    | :wrench: | :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-arguments`](./docs/rules/no-unnecessary-type-arguments.md)       | Warns if an explicitly specified type argument is the default for that type parameter                                                               |                    | :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                                                                                 | :heavy_check_mark: | :wrench: | :thought_balloon: |
diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md
@@ -5,7 +5,8 @@ Any expression being used as a condition must be able to evaluate as truthy or f
 The following expressions are checked:
 
 - Arguments to the `&&`, `||` and `?:` (ternary) operators
-- Conditions for `if`, `for`, `while`, and `do-while` statements.
+- Conditions for `if`, `for`, `while`, and `do-while` statements
+- Base values of optional chain expressions
 
 Examples of **incorrect** code for this rule:
 
@@ -22,6 +23,11 @@ function foo(arg: 'bar' | 'baz') {
   if (arg) {
   }
 }
+
+function bar<T>(arg: string) {
+  // arg can never be nullish, so ?. is unnecessary
+  return arg?.length;
+}
 ```
 
 Examples of **correct** code for this rule:
@@ -39,6 +45,11 @@ function foo(arg: string) {
   if (arg) {
   }
 }
+
+function bar(arg?: string | null) {
+  // Necessary, since arg might be nullish
+  return arg?.length;
+}
 ```
 
 ## Options
diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
@@ -1,6 +1,7 @@
 import {
   TSESTree,
   AST_NODE_TYPES,
+  AST_TOKEN_TYPES,
 } from '@typescript-eslint/experimental-utils';
 import ts, { TypeFlags } from 'typescript';
 import {
@@ -14,6 +15,9 @@ import {
   createRule,
   getParserServices,
   getConstrainedTypeAtLocation,
+  isNullableType,
+  nullThrows,
+  NullThrowsReasons,
 } from '../util';
 
 // Truthiness utilities
@@ -65,7 +69,8 @@ export type MessageId =
   | 'neverNullish'
   | 'alwaysNullish'
   | 'literalBooleanExpression'
-  | 'never';
+  | 'never'
+  | 'neverOptionalChain';
 export default createRule<Options, MessageId>({
   name: 'no-unnecessary-conditionals',
   meta: {
@@ -91,6 +96,7 @@ export default createRule<Options, MessageId>({
         additionalProperties: false,
       },
     ],
+    fixable: 'code',
     messages: {
       alwaysTruthy: 'Unnecessary conditional, value is always truthy.',
       alwaysFalsy: 'Unnecessary conditional, value is always falsy.',
@@ -101,6 +107,7 @@ export default createRule<Options, MessageId>({
       literalBooleanExpression:
         'Unnecessary conditional, both sides of the expression are literal values',
       never: 'Unnecessary conditional, value is `never`',
+      neverOptionalChain: 'Unnecessary optional chain on a non-nullish value',
     },
   },
   defaultOptions: [
@@ -112,6 +119,7 @@ export default createRule<Options, MessageId>({
   create(context, [{ allowConstantLoopConditions, ignoreRhs }]) {
     const service = getParserServices(context);
     const checker = service.program.getTypeChecker();
+    const sourceCode = context.getSourceCode();
 
     function getNodeType(node: TSESTree.Node): ts.Type {
       const tsNode = service.esTreeNodeToTSNodeMap.get(node);
@@ -260,6 +268,57 @@ export default createRule<Options, MessageId>({
       checkNode(node.test);
     }
 
+    function checkOptionalChain(
+      node: TSESTree.OptionalMemberExpression | TSESTree.OptionalCallExpression,
+      beforeOperator: TSESTree.Node,
+      fix: '' | '.',
+    ): void {
+      // We only care if this step in the chain is optional. If just descend
+      // from an optional chain, then that's fine.
+      if (!node.optional) {
+        return;
+      }
+
+      const type = getNodeType(node);
+      if (
+        isTypeFlagSet(type, ts.TypeFlags.Any) ||
+        isTypeFlagSet(type, ts.TypeFlags.Unknown) ||
+        isNullableType(type, { allowUndefined: true })
+      ) {
+        return;
+      }
+
+      const questionDotOperator = nullThrows(
+        sourceCode.getTokenAfter(
+          beforeOperator,
+          token =>
+            token.type === AST_TOKEN_TYPES.Punctuator && token.value === '?.',
+        ),
+        NullThrowsReasons.MissingToken('operator', node.type),
+      );
+
+      context.report({
+        node,
+        loc: questionDotOperator.loc,
+        messageId: 'neverOptionalChain',
+        fix(fixer) {
+          return fixer.replaceText(questionDotOperator, fix);
+        },
+      });
+    }
+
+    function checkOptionalMemberExpression(
+      node: TSESTree.OptionalMemberExpression,
+    ): void {
+      checkOptionalChain(node, node.object, '.');
+    }
+
+    function checkOptionalCallExpression(
+      node: TSESTree.OptionalCallExpression,
+    ): void {
+      checkOptionalChain(node, node.callee, '');
+    }
+
     return {
       BinaryExpression: checkIfBinaryExpressionIsNecessaryConditional,
       ConditionalExpression: checkIfTestExpressionIsNecessaryConditional,
@@ -268,6 +327,8 @@ export default createRule<Options, MessageId>({
       IfStatement: checkIfTestExpressionIsNecessaryConditional,
       LogicalExpression: checkLogicalExpressionForUnnecessaryConditionals,
       WhileStatement: checkIfLoopIsNecessaryConditional,
+      OptionalMemberExpression: checkOptionalMemberExpression,
+      OptionalCallExpression: checkOptionalCallExpression,
     };
   },
 });
diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts
@@ -121,6 +121,62 @@ do {} while(true)
       `,
       options: [{ allowConstantLoopConditions: true }],
     },
+    `
+let foo: undefined | { bar: true };
+foo?.bar;
+`,
+    `
+let foo: null | { bar: true };
+foo?.bar;
+`,
+    `
+let foo: undefined;
+foo?.bar;
+`,
+    `
+let foo: undefined;
+foo?.bar.baz;
+`,
+    `
+let foo: null;
+foo?.bar;
+`,
+    `
+let anyValue: any;
+anyValue?.foo;
+`,
+    `
+let unknownValue: unknown;
+unknownValue?.foo;
+`,
+    `
+let foo: undefined | (() => {});
+foo?.();
+`,
+    `
+let foo: null | (() => {});
+foo?.();
+`,
+    `
+let foo: undefined;
+foo?.();
+`,
+    `
+let foo: undefined;
+foo?.().bar;
+`,
+    `
+let foo: null;
+foo?.();
+`,
+    `
+let anyValue: any;
+anyValue?.();
+`,
+    `
+let unknownValue: unknown;
+unknownValue?.();
+`,
   ],
   invalid: [
     // Ensure that it's checking in all the right places
@@ -267,5 +323,155 @@ do {} while(true)
         ruleError(4, 13, 'alwaysTruthy'),
       ],
     },
+    {
+      code: `
+let foo = { bar: true };
+foo?.bar;
+foo ?. bar;
+foo ?.
+  bar;
+foo
+  ?. bar;
+`,
+      output: `
+let foo = { bar: true };
+foo.bar;
+foo . bar;
+foo .
+  bar;
+foo
+  . bar;
+`,
+      errors: [
+        {
+          messageId: 'neverOptionalChain',
+          line: 3,
+          column: 4,
+          endLine: 3,
+          endColumn: 6,
+        },
+        {
+          messageId: 'neverOptionalChain',
+          line: 4,
+          column: 5,
+          endLine: 4,
+          endColumn: 7,
+        },
+        {
+          messageId: 'neverOptionalChain',
+          line: 5,
+          column: 5,
+          endLine: 5,
+          endColumn: 7,
+        },
+        {
+          messageId: 'neverOptionalChain',
+          line: 8,
+          column: 3,
+          endLine: 8,
+          endColumn: 5,
+        },
+      ],
+    },
+    {
+      code: `
+let foo = () => {};
+foo?.();
+foo ?. ();
+foo ?.
+  ();
+foo
+  ?. ();
+`,
+      output: `
+let foo = () => {};
+foo();
+foo  ();
+foo${' '}
+  ();
+foo
+   ();
+`,
+      errors: [
+        {
+          messageId: 'neverOptionalChain',
+          line: 3,
+          column: 4,
+          endLine: 3,
+          endColumn: 6,
+        },
+        {
+          messageId: 'neverOptionalChain',
+          line: 4,
+          column: 5,
+          endLine: 4,
+          endColumn: 7,
+        },
+        {
+          messageId: 'neverOptionalChain',
+          line: 5,
+          column: 5,
+          endLine: 5,
+          endColumn: 7,
+        },
+        {
+          messageId: 'neverOptionalChain',
+          line: 8,
+          column: 3,
+          endLine: 8,
+          endColumn: 5,
+        },
+      ],
+    },
+    {
+      code: `
+let foo = () => {};
+foo?.(bar);
+foo ?. (bar);
+foo ?.
+  (bar);
+foo
+  ?. (bar);
+`,
+      output: `
+let foo = () => {};
+foo(bar);
+foo  (bar);
+foo${' '}
+  (bar);
+foo
+   (bar);
+`,
+      errors: [
+        {
+          messageId: 'neverOptionalChain',
+          line: 3,
+          column: 4,
+          endLine: 3,
+          endColumn: 6,
+        },
+        {
+          messageId: 'neverOptionalChain',
+          line: 4,
+          column: 5,
+          endLine: 4,
+          endColumn: 7,
+        },
+        {
+          messageId: 'neverOptionalChain',
+          line: 5,
+          column: 5,
+          endLine: 5,
+          endColumn: 7,
+        },
+        {
+          messageId: 'neverOptionalChain',
+          line: 8,
+          column: 3,
+          endLine: 8,
+          endColumn: 5,
+        },
+      ],
+    },
   ],
 });
