feat(eslint-plugin): [prefer-optional-chain] use type checking for st…

…rict falsiness
typescript-eslint · Mar 20, 2023 · 8d847e3 · 8d847e3
1 parent 28a64b5
commit 8d847e3
Show file tree

Hide file tree

Showing 7 changed files with 908 additions and 169 deletions.
diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.md b/packages/eslint-plugin/docs/rules/prefer-optional-chain.md
@@ -57,9 +57,35 @@ foo?.a?.b?.c?.d?.e;
 
 <!--/tabs-->
 
-:::note
-There are a few edge cases where this rule will false positive. Use your best judgement when evaluating reported errors.
-:::
+## Options
+
+### `looseFalsiness`
+
+By default, this rule will ignore cases that could result with different result values when switched to optional chaining.
+For example, the following `box != null && box.value` would not be flagged by default.
+
+<!--tabs-->
+
+### ❌ Incorrect
+
+```ts
+declare const box: { value: number } | null;
+// Type: false | number
+box != null && box.value;
+```
+
+### ✅ Correct
+
+```ts
+declare const box: { value: number } | null;
+// Type: undefined | number
+box?.value;
+```
+
+<!--/tabs-->
+
+If you don't mind your code considering all falsy values the same (e.g. the `false` and `undefined` above), you can enable `looseFalsiness: true`.
+Doing so makes the rule slightly incorrect - but speeds it by not having to ask TypeScript's type checker for information.
 
 ## When Not To Use It
 

diff --git a/packages/eslint-plugin/src/configs/disable-type-checked.ts b/packages/eslint-plugin/src/configs/disable-type-checked.ts
@@ -35,6 +35,7 @@ export = {
     '@typescript-eslint/non-nullable-type-assertion-style': 'off',
     '@typescript-eslint/prefer-includes': 'off',
     '@typescript-eslint/prefer-nullish-coalescing': 'off',
+    '@typescript-eslint/prefer-optional-chain': 'off',
     '@typescript-eslint/prefer-readonly': 'off',
     '@typescript-eslint/prefer-readonly-parameter-types': 'off',
     '@typescript-eslint/prefer-reduce-type-parameter': 'off',

diff --git a/packages/eslint-plugin/src/configs/stylistic.ts b/packages/eslint-plugin/src/configs/stylistic.ts
@@ -24,7 +24,6 @@ export = {
     '@typescript-eslint/prefer-for-of': 'error',
     '@typescript-eslint/prefer-function-type': 'error',
     '@typescript-eslint/prefer-namespace-keyword': 'error',
-    '@typescript-eslint/prefer-optional-chain': 'error',
     '@typescript-eslint/sort-type-constituents': 'error',
   },
 };
diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts
@@ -1,5 +1,6 @@
 import type { TSESLint, TSESTree } from '@typescript-eslint/utils';
 import { AST_NODE_TYPES } from '@typescript-eslint/utils';
+import * as tsutils from 'ts-api-utils';
 import * as ts from 'typescript';
 
 import * as util from '../util';
@@ -38,19 +39,109 @@ export default util.createRule({
       description:
         'Enforce using concise optional chain expressions instead of chained logical ands, negated logical ors, or empty objects',
       recommended: 'stylistic',
+      requiresTypeChecking: true,
     },
     hasSuggestions: true,
     messages: {
       preferOptionalChain:
         "Prefer using an optional chain expression instead, as it's more concise and easier to read.",
       optionalChainSuggest: 'Change to an optional chain.',
     },
-    schema: [],
+    schema: [
+      {
+        type: 'object',
+        properties: {
+          looseFalsiness: {
+            description: 'Whether to consider all nullable values equivalent',
+            type: 'boolean',
+          },
+        },
+      },
+    ],
   },
-  defaultOptions: [],
-  create(context) {
+  defaultOptions: [
+    {
+      looseFalsiness: false,
+    },
+  ],
+  create(context, [{ looseFalsiness }]) {
     const sourceCode = context.getSourceCode();
-    const services = util.getParserServices(context, true);
+    const services = util.getParserServices(context);
+
+    interface ReportIfMoreThanOneOptions {
+      expressionCount: number;
+      initialNodeForType: TSESTree.Node;
+      previous: TSESTree.LogicalExpression;
+      optionallyChainedCode: string;
+      shouldHandleChainedAnds: boolean;
+    }
+
+    function reportIfMoreThanOne({
+      expressionCount,
+      initialNodeForType,
+      previous,
+      optionallyChainedCode,
+      shouldHandleChainedAnds,
+    }: ReportIfMoreThanOneOptions): void {
+      if (expressionCount <= 1) {
+        return;
+      }
+
+      if (!looseFalsiness) {
+        const initialType = services.getTypeAtLocation(initialNodeForType);
+
+        if (
+          util.isTypeFlagSet(
+            initialType,
+            ts.TypeFlags.BigIntLike |
+              ts.TypeFlags.Null |
+              ts.TypeFlags.NumberLike |
+              ts.TypeFlags.StringLike,
+          ) ||
+          tsutils
+            .unionTypeParts(initialType)
+            .some(subType => util.isTypeIntrinsic(subType, 'false'))
+        ) {
+          return;
+        }
+      }
+
+      if (
+        shouldHandleChainedAnds &&
+        previous.right.type === AST_NODE_TYPES.BinaryExpression
+      ) {
+        let operator = previous.right.operator;
+        if (
+          previous.right.operator === '!==' &&
+          // TODO(#4820): Use the type checker to know whether this is `null`
+          previous.right.right.type === AST_NODE_TYPES.Literal &&
+          previous.right.right.raw === 'null'
+        ) {
+          // case like foo !== null && foo.bar !== null
+          operator = '!=';
+        }
+        // case like foo && foo.bar !== someValue
+        optionallyChainedCode += ` ${operator} ${sourceCode.getText(
+          previous.right.right,
+        )}`;
+      }
+
+      context.report({
+        node: previous,
+        messageId: 'preferOptionalChain',
+        suggest: [
+          {
+            messageId: 'optionalChainSuggest',
+            fix: (fixer): TSESLint.RuleFix[] => [
+              fixer.replaceText(
+                previous,
+                `${shouldHandleChainedAnds ? '' : '!'}${optionallyChainedCode}`,
+              ),
+            ],
+          },
+        ],
+      });
+    }
 
     return {
       'LogicalExpression[operator="||"], LogicalExpression[operator="??"]'(
@@ -187,10 +278,9 @@ export default util.createRule({
 
         reportIfMoreThanOne({
           expressionCount,
+          initialNodeForType: initialIdentifierOrNotEqualsExpr,
           previous,
           optionallyChainedCode,
-          sourceCode,
-          context,
           shouldHandleChainedAnds: false,
         });
       },
@@ -271,10 +361,9 @@ export default util.createRule({
 
         reportIfMoreThanOne({
           expressionCount,
+          initialNodeForType: initialIdentifierOrNotEqualsExpr,
           previous,
           optionallyChainedCode,
-          sourceCode,
-          context,
           shouldHandleChainedAnds: true,
         });
       },
@@ -472,67 +561,6 @@ const ALLOWED_NON_COMPUTED_PROP_TYPES: ReadonlySet<AST_NODE_TYPES> = new Set([
   AST_NODE_TYPES.PrivateIdentifier,
 ]);
 
-interface ReportIfMoreThanOneOptions {
-  expressionCount: number;
-  previous: TSESTree.LogicalExpression;
-  optionallyChainedCode: string;
-  sourceCode: Readonly<TSESLint.SourceCode>;
-  context: Readonly<
-    TSESLint.RuleContext<
-      'preferOptionalChain' | 'optionalChainSuggest',
-      never[]
-    >
-  >;
-  shouldHandleChainedAnds: boolean;
-}
-
-function reportIfMoreThanOne({
-  expressionCount,
-  previous,
-  optionallyChainedCode,
-  sourceCode,
-  context,
-  shouldHandleChainedAnds,
-}: ReportIfMoreThanOneOptions): void {
-  if (expressionCount > 1) {
-    if (
-      shouldHandleChainedAnds &&
-      previous.right.type === AST_NODE_TYPES.BinaryExpression
-    ) {
-      let operator = previous.right.operator;
-      if (
-        previous.right.operator === '!==' &&
-        // TODO(#4820): Use the type checker to know whether this is `null`
-        previous.right.right.type === AST_NODE_TYPES.Literal &&
-        previous.right.right.raw === 'null'
-      ) {
-        // case like foo !== null && foo.bar !== null
-        operator = '!=';
-      }
-      // case like foo && foo.bar !== someValue
-      optionallyChainedCode += ` ${operator} ${sourceCode.getText(
-        previous.right.right,
-      )}`;
-    }
-
-    context.report({
-      node: previous,
-      messageId: 'preferOptionalChain',
-      suggest: [
-        {
-          messageId: 'optionalChainSuggest',
-          fix: (fixer): TSESLint.RuleFix[] => [
-            fixer.replaceText(
-              previous,
-              `${shouldHandleChainedAnds ? '' : '!'}${optionallyChainedCode}`,
-            ),
-          ],
-        },
-      ],
-    });
-  }
-}
-
 interface NormalizedPattern {
   invalidOptionallyChainedPrivateProperty: boolean;
   expressionCount: number;