fix(eslint-plugin): [prefer-optional-chain] fixer produces wrong logic (

#5919) * fix(eslint-plugin): [prefer-optional-chain] fixer produces wrong logic (#1438) * Update packages/eslint-plugin/src/rules/prefer-optional-chain.ts * fix(eslint-plugin): [prefer-optional-chain] fix tests * fix(eslint-plugin): [prefer-optional-chain] fix tests Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com> Co-authored-by: Святослав Зайцев <sz@agentapp.ru>
typescript-eslint · Jan 26, 2023 · b0f6c8e · b0f6c8e
1 parent 033e87c
commit b0f6c8e
Show file tree

Hide file tree

Showing 3 changed files with 273 additions and 179 deletions.
diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts
@@ -490,10 +490,20 @@ function reportIfMoreThanOne({
       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 += ` ${
-        previous.right.operator
-      } ${sourceCode.getText(previous.right.right)}`;
+      optionallyChainedCode += ` ${operator} ${sourceCode.getText(
+        previous.right.right,
+      )}`;
     }
 
     context.report({

diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/base-cases.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/base-cases.ts
@@ -0,0 +1,228 @@
+import type { TSESLint } from '@typescript-eslint/utils';
+
+import type rule from '../../../src/rules/prefer-optional-chain';
+import type {
+  InferMessageIdsTypeFromRule,
+  InferOptionsTypeFromRule,
+} from '../../../src/util';
+
+type InvalidTestCase = TSESLint.InvalidTestCase<
+  InferMessageIdsTypeFromRule<typeof rule>,
+  InferOptionsTypeFromRule<typeof rule>
+>;
+
+interface BaseCase {
+  canReplaceAndWithOr: boolean;
+  output: string;
+  code: string;
+}
+
+const mapper = (c: BaseCase): InvalidTestCase => ({
+  code: c.code.trim(),
+  output: null,
+  errors: [
+    {
+      messageId: 'preferOptionalChain',
+      suggestions: [
+        {
+          messageId: 'optionalChainSuggest',
+          output: c.output.trim(),
+        },
+      ],
+    },
+  ],
+});
+
+const baseCases: Array<BaseCase> = [
+  // chained members
+  {
+    code: 'foo && foo.bar',
+    output: 'foo?.bar',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo.bar && foo.bar.baz',
+    output: 'foo.bar?.baz',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo && foo()',
+    output: 'foo?.()',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo.bar && foo.bar()',
+    output: 'foo.bar?.()',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz.buzz',
+    output: 'foo?.bar?.baz?.buzz',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo.bar && foo.bar.baz && foo.bar.baz.buzz',
+    output: 'foo.bar?.baz?.buzz',
+    canReplaceAndWithOr: true,
+  },
+  // case with a jump (i.e. a non-nullish prop)
+  {
+    code: 'foo && foo.bar && foo.bar.baz.buzz',
+    output: 'foo?.bar?.baz.buzz',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo.bar && foo.bar.baz.buzz',
+    output: 'foo.bar?.baz.buzz',
+    canReplaceAndWithOr: true,
+  },
+  // case where for some reason there is a doubled up expression
+  {
+    code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz && foo.bar.baz.buzz',
+    output: 'foo?.bar?.baz?.buzz',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo.bar && foo.bar.baz && foo.bar.baz && foo.bar.baz.buzz',
+    output: 'foo.bar?.baz?.buzz',
+    canReplaceAndWithOr: true,
+  },
+  // chained members with element access
+  {
+    code: 'foo && foo[bar] && foo[bar].baz && foo[bar].baz.buzz',
+    output: 'foo?.[bar]?.baz?.buzz',
+    canReplaceAndWithOr: true,
+  },
+  {
+    // case with a jump (i.e. a non-nullish prop)
+    code: 'foo && foo[bar].baz && foo[bar].baz.buzz',
+    output: 'foo?.[bar].baz?.buzz',
+    canReplaceAndWithOr: true,
+  },
+  // case with a property access in computed property
+  {
+    code: 'foo && foo[bar.baz] && foo[bar.baz].buzz',
+    output: 'foo?.[bar.baz]?.buzz',
+    canReplaceAndWithOr: true,
+  },
+  // case with this keyword
+  {
+    code: 'foo[this.bar] && foo[this.bar].baz',
+    output: 'foo[this.bar]?.baz',
+    canReplaceAndWithOr: true,
+  },
+  // chained calls
+  {
+    code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz.buzz()',
+    output: 'foo?.bar?.baz?.buzz()',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz.buzz && foo.bar.baz.buzz()',
+    output: 'foo?.bar?.baz?.buzz?.()',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo.bar && foo.bar.baz && foo.bar.baz.buzz && foo.bar.baz.buzz()',
+    output: 'foo.bar?.baz?.buzz?.()',
+    canReplaceAndWithOr: true,
+  },
+  // case with a jump (i.e. a non-nullish prop)
+  {
+    code: 'foo && foo.bar && foo.bar.baz.buzz()',
+    output: 'foo?.bar?.baz.buzz()',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo.bar && foo.bar.baz.buzz()',
+    output: 'foo.bar?.baz.buzz()',
+    canReplaceAndWithOr: true,
+  },
+  {
+    // case with a jump (i.e. a non-nullish prop)
+    code: 'foo && foo.bar && foo.bar.baz.buzz && foo.bar.baz.buzz()',
+    output: 'foo?.bar?.baz.buzz?.()',
+    canReplaceAndWithOr: true,
+  },
+  {
+    // case with a call expr inside the chain for some inefficient reason
+    code: 'foo && foo.bar() && foo.bar().baz && foo.bar().baz.buzz && foo.bar().baz.buzz()',
+    output: 'foo?.bar()?.baz?.buzz?.()',
+    canReplaceAndWithOr: true,
+  },
+  // chained calls with element access
+  {
+    code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz[buzz]()',
+    output: 'foo?.bar?.baz?.[buzz]()',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz[buzz] && foo.bar.baz[buzz]()',
+    output: 'foo?.bar?.baz?.[buzz]?.()',
+    canReplaceAndWithOr: true,
+  },
+  // (partially) pre-optional chained
+  {
+    code: 'foo && foo?.bar && foo?.bar.baz && foo?.bar.baz[buzz] && foo?.bar.baz[buzz]()',
+    output: 'foo?.bar?.baz?.[buzz]?.()',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo && foo?.bar.baz && foo?.bar.baz[buzz]',
+    output: 'foo?.bar.baz?.[buzz]',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo && foo?.() && foo?.().bar',
+    output: 'foo?.()?.bar',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo.bar && foo.bar?.() && foo.bar?.().baz',
+    output: 'foo.bar?.()?.baz',
+    canReplaceAndWithOr: true,
+  },
+  {
+    code: 'foo !== null && foo.bar !== null',
+    output: 'foo?.bar != null',
+    canReplaceAndWithOr: false,
+  },
+  {
+    code: 'foo != null && foo.bar != null',
+    output: 'foo?.bar != null',
+    canReplaceAndWithOr: false,
+  },
+  {
+    code: 'foo != null && foo.bar !== null',
+    output: 'foo?.bar != null',
+    canReplaceAndWithOr: false,
+  },
+  {
+    code: 'foo !== null && foo.bar != null',
+    output: 'foo?.bar != null',
+    canReplaceAndWithOr: false,
+  },
+];
+
+interface Selector {
+  all(): Array<InvalidTestCase>;
+  select<K extends Exclude<keyof BaseCase, 'code' | 'output'>>(
+    key: K,
+    value: BaseCase[K],
+  ): Selector;
+}
+
+const selector = (cases: Array<BaseCase>): Selector => ({
+  all: () => cases.map(mapper),
+  select: <K extends Exclude<keyof BaseCase, 'code' | 'output'>>(
+    key: K,
+    value: BaseCase[K],
+  ): Selector => {
+    const selectedCases = baseCases.filter(c => c[key] === value);
+    return selector(selectedCases);
+  },
+});
+
+const { all, select } = selector(baseCases);
+
+export { all, select };