feat(eslint-plugin): [prefer-nullish-coalescing] add ignoreTernaryTes…

…ts option (#4965) * feat(eslint-plugin): added ignoreTernaryTests option to prefer-nullish-coalescing rule * feat(eslint-plugin): added checking of loose equal ternary cases for prefer-nullish-coalescing rule * feat(eslint-plugin): fixed typo in docs for prefer-nullish-coalescing rule * feat(eslint-plugin): added more test cases for prefer-nullish-coalescing rule * feat(eslint-plugin): added support for MemberExpressions for ignoreTernaryTests option * refactor(eslint-plugin): cleanup prefer-nullish-coalescing rule * test(eslint-plugin): added missing test case for prefer-nullish-coalescing rule * chore: removed currently not supported comment Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com> * refactor: simplified return statement Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com> * refactor: renamed utilities * refactor: used new utilities in prefer-optional-chain.ts * refactor: simplified prefer-nullish-coalescing.ts * test: added test case for prefer-nullish-coalescing * refactor: renamed message id to preferNullishOverOr * fix: covered edge case where we have two loose comparisons * refactor: removed useless .trim * chore: fixed typo * test: added more test cases * fix: fixed tests * chore: fixed typo * test: added more test cases * refactor: inlined getOperator and getNodes * perf: skip type check if both null and undefined are not checked * Apply suggestions from code review Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com>
typescript-eslint · Jul 23, 2022 · f82727f · f82727f
1 parent 78823cc
commit f82727f
Show file tree

Hide file tree

Showing 9 changed files with 609 additions and 37 deletions.
diff --git a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md
@@ -46,19 +46,61 @@ This rule aims enforce the usage of the safer operator.
 ```ts
 type Options = [
   {
+    ignoreTernaryTests?: boolean;
     ignoreConditionalTests?: boolean;
     ignoreMixedLogicalExpressions?: boolean;
   },
 ];
 
 const defaultOptions = [
   {
+    ignoreTernaryTests: true;
     ignoreConditionalTests: true,
     ignoreMixedLogicalExpressions: true,
   },
 ];
 ```
 
+### `ignoreTernaryTests`
+
+Setting this option to `true` (the default) will cause the rule to ignore any ternary expressions that could be simplified by using the nullish coalescing operator.
+
+Incorrect code for `ignoreTernaryTests: false`, and correct code for `ignoreTernaryTests: true`:
+
+```ts
+const foo: any = 'bar';
+foo !== undefined && foo !== null ? foo : 'a string';
+foo === undefined || foo === null ? 'a string' : foo;
+foo == undefined ? 'a string' : foo;
+foo == null ? 'a string' : foo;
+
+const foo: string | undefined = 'bar';
+foo !== undefined ? foo : 'a string';
+foo === undefined ? 'a string' : foo;
+
+const foo: string | null = 'bar';
+foo !== null ? foo : 'a string';
+foo === null ? 'a string' : foo;
+```
+
+Correct code for `ignoreTernaryTests: false`:
+
+```ts
+const foo: any = 'bar';
+foo ?? 'a string';
+foo ?? 'a string';
+foo ?? 'a string';
+foo ?? 'a string';
+
+const foo: string | undefined = 'bar';
+foo ?? 'a string';
+foo ?? 'a string';
+
+const foo: string | null = 'bar';
+foo ?? 'a string';
+foo ?? 'a string';
+```
+
 ### `ignoreConditionalTests`
 
 Setting this option to `true` (the default) will cause the rule to ignore any cases that are located within a conditional test.

diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts
@@ -5,14 +5,20 @@ import {
   TSESTree,
 } from '@typescript-eslint/utils';
 import * as util from '../util';
+import * as ts from 'typescript';
 
 export type Options = [
   {
     ignoreConditionalTests?: boolean;
+    ignoreTernaryTests?: boolean;
     ignoreMixedLogicalExpressions?: boolean;
   },
 ];
-export type MessageIds = 'preferNullish' | 'suggestNullish';
+
+export type MessageIds =
+  | 'preferNullishOverOr'
+  | 'preferNullishOverTernary'
+  | 'suggestNullish';
 
 export default util.createRule<Options, MessageIds>({
   name: 'prefer-nullish-coalescing',
@@ -27,8 +33,10 @@ export default util.createRule<Options, MessageIds>({
     },
     hasSuggestions: true,
     messages: {
-      preferNullish:
+      preferNullishOverOr:
         'Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.',
+      preferNullishOverTernary:
+        'Prefer using nullish coalescing operator (`??`) instead of a ternary expression, as it is simpler to read.',
       suggestNullish: 'Fix to nullish coalescing operator (`??`).',
     },
     schema: [
@@ -38,6 +46,9 @@ export default util.createRule<Options, MessageIds>({
           ignoreConditionalTests: {
             type: 'boolean',
           },
+          ignoreTernaryTests: {
+            type: 'boolean',
+          },
           ignoreMixedLogicalExpressions: {
             type: 'boolean',
           },
@@ -52,15 +63,182 @@ export default util.createRule<Options, MessageIds>({
   defaultOptions: [
     {
       ignoreConditionalTests: true,
+      ignoreTernaryTests: true,
       ignoreMixedLogicalExpressions: true,
     },
   ],
-  create(context, [{ ignoreConditionalTests, ignoreMixedLogicalExpressions }]) {
+  create(
+    context,
+    [
+      {
+        ignoreConditionalTests,
+        ignoreTernaryTests,
+        ignoreMixedLogicalExpressions,
+      },
+    ],
+  ) {
     const parserServices = util.getParserServices(context);
     const sourceCode = context.getSourceCode();
     const checker = parserServices.program.getTypeChecker();
 
     return {
+      ConditionalExpression(node: TSESTree.ConditionalExpression): void {
+        if (ignoreTernaryTests) {
+          return;
+        }
+
+        let operator: '==' | '!=' | '===' | '!==' | undefined;
+        let nodesInsideTestExpression: TSESTree.Node[] = [];
+        if (node.test.type === AST_NODE_TYPES.BinaryExpression) {
+          nodesInsideTestExpression = [node.test.left, node.test.right];
+          if (
+            node.test.operator === '==' ||
+            node.test.operator === '!=' ||
+            node.test.operator === '===' ||
+            node.test.operator === '!=='
+          ) {
+            operator = node.test.operator;
+          }
+        } else if (
+          node.test.type === AST_NODE_TYPES.LogicalExpression &&
+          node.test.left.type === AST_NODE_TYPES.BinaryExpression &&
+          node.test.right.type === AST_NODE_TYPES.BinaryExpression
+        ) {
+          nodesInsideTestExpression = [
+            node.test.left.left,
+            node.test.left.right,
+            node.test.right.left,
+            node.test.right.right,
+          ];
+          if (node.test.operator === '||') {
+            if (
+              node.test.left.operator === '===' &&
+              node.test.right.operator === '==='
+            ) {
+              operator = '===';
+            } else if (
+              ((node.test.left.operator === '===' ||
+                node.test.right.operator === '===') &&
+                (node.test.left.operator === '==' ||
+                  node.test.right.operator === '==')) ||
+              (node.test.left.operator === '==' &&
+                node.test.right.operator === '==')
+            ) {
+              operator = '==';
+            }
+          } else if (node.test.operator === '&&') {
+            if (
+              node.test.left.operator === '!==' &&
+              node.test.right.operator === '!=='
+            ) {
+              operator = '!==';
+            } else if (
+              ((node.test.left.operator === '!==' ||
+                node.test.right.operator === '!==') &&
+                (node.test.left.operator === '!=' ||
+                  node.test.right.operator === '!=')) ||
+              (node.test.left.operator === '!=' &&
+                node.test.right.operator === '!=')
+            ) {
+              operator = '!=';
+            }
+          }
+        }
+
+        if (!operator) {
+          return;
+        }
+
+        let identifier: TSESTree.Node | undefined;
+        let hasUndefinedCheck = false;
+        let hasNullCheck = false;
+
+        // we check that the test only contains null, undefined and the identifier
+        for (const testNode of nodesInsideTestExpression) {
+          if (util.isNullLiteral(testNode)) {
+            hasNullCheck = true;
+          } else if (util.isUndefinedIdentifier(testNode)) {
+            hasUndefinedCheck = true;
+          } else if (
+            (operator === '!==' || operator === '!=') &&
+            util.isNodeEqual(testNode, node.consequent)
+          ) {
+            identifier = testNode;
+          } else if (
+            (operator === '===' || operator === '==') &&
+            util.isNodeEqual(testNode, node.alternate)
+          ) {
+            identifier = testNode;
+          } else {
+            return;
+          }
+        }
+
+        if (!identifier) {
+          return;
+        }
+
+        const isFixable = ((): boolean => {
+          // it is fixable if we check for both null and undefined, or not if neither
+          if (hasUndefinedCheck === hasNullCheck) {
+            return hasUndefinedCheck;
+          }
+
+          // it is fixable if we loosely check for either null or undefined
+          if (operator === '==' || operator === '!=') {
+            return true;
+          }
+
+          const tsNode = parserServices.esTreeNodeToTSNodeMap.get(identifier);
+          const type = checker.getTypeAtLocation(tsNode);
+          const flags = util.getTypeFlags(type);
+
+          if (flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)) {
+            return false;
+          }
+
+          const hasNullType = (flags & ts.TypeFlags.Null) !== 0;
+
+          // it is fixable if we check for undefined and the type is not nullable
+          if (hasUndefinedCheck && !hasNullType) {
+            return true;
+          }
+
+          const hasUndefinedType = (flags & ts.TypeFlags.Undefined) !== 0;
+
+          // it is fixable if we check for null and the type can't be undefined
+          return hasNullCheck && !hasUndefinedType;
+        })();
+
+        if (isFixable) {
+          context.report({
+            node,
+            messageId: 'preferNullishOverTernary',
+            suggest: [
+              {
+                messageId: 'suggestNullish',
+                fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix {
+                  const [left, right] =
+                    operator === '===' || operator === '=='
+                      ? [node.alternate, node.consequent]
+                      : [node.consequent, node.alternate];
+                  return fixer.replaceText(
+                    node,
+                    `${sourceCode.text.slice(
+                      left.range[0],
+                      left.range[1],
+                    )} ?? ${sourceCode.text.slice(
+                      right.range[0],
+                      right.range[1],
+                    )}`,
+                  );
+                },
+              },
+            ],
+          });
+        }
+      },
+
       'LogicalExpression[operator = "||"]'(
         node: TSESTree.LogicalExpression,
       ): void {
@@ -110,7 +288,7 @@ export default util.createRule<Options, MessageIds>({
 
         context.report({
           node: barBarOperator,
-          messageId: 'preferNullish',
+          messageId: 'preferNullishOverOr',
           suggest: [
             {
               messageId: 'suggestNullish',

diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts
@@ -437,24 +437,10 @@ function isValidChainTarget(
   - foo !== undefined
   - foo != undefined
   */
-  if (
+  return (
     node.type === AST_NODE_TYPES.BinaryExpression &&
     ['!==', '!='].includes(node.operator) &&
-    isValidChainTarget(node.left, allowIdentifier)
-  ) {
-    if (
-      node.right.type === AST_NODE_TYPES.Identifier &&
-      node.right.name === 'undefined'
-    ) {
-      return true;
-    }
-    if (
-      node.right.type === AST_NODE_TYPES.Literal &&
-      node.right.value === null
-    ) {
-      return true;
-    }
-  }
-
-  return false;
+    isValidChainTarget(node.left, allowIdentifier) &&
+    (util.isUndefinedIdentifier(node.right) || util.isNullLiteral(node.right))
+  );
 }
diff --git a/packages/eslint-plugin/src/util/index.ts b/packages/eslint-plugin/src/util/index.ts
@@ -9,6 +9,9 @@ export * from './getThisExpression';
 export * from './getWrappingFixer';
 export * from './misc';
 export * from './objectIterators';
+export * from './isNullLiteral';
+export * from './isUndefinedIdentifier';
+export * from './isNodeEqual';
 
 // this is done for convenience - saves migrating all of the old rules
 export * from '@typescript-eslint/type-utils';

diff --git a/packages/eslint-plugin/src/util/isNodeEqual.ts b/packages/eslint-plugin/src/util/isNodeEqual.ts
@@ -0,0 +1,31 @@
+import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/utils';
+
+export function isNodeEqual(a: TSESTree.Node, b: TSESTree.Node): boolean {
+  if (a.type !== b.type) {
+    return false;
+  }
+  if (
+    a.type === AST_NODE_TYPES.ThisExpression &&
+    b.type === AST_NODE_TYPES.ThisExpression
+  ) {
+    return true;
+  }
+  if (a.type === AST_NODE_TYPES.Literal && b.type === AST_NODE_TYPES.Literal) {
+    return a.value === b.value;
+  }
+  if (
+    a.type === AST_NODE_TYPES.Identifier &&
+    b.type === AST_NODE_TYPES.Identifier
+  ) {
+    return a.name === b.name;
+  }
+  if (
+    a.type === AST_NODE_TYPES.MemberExpression &&
+    b.type === AST_NODE_TYPES.MemberExpression
+  ) {
+    return (
+      isNodeEqual(a.property, b.property) && isNodeEqual(a.object, b.object)
+    );
+  }
+  return false;
+}
diff --git a/packages/eslint-plugin/src/util/isNullLiteral.ts b/packages/eslint-plugin/src/util/isNullLiteral.ts
@@ -0,0 +1,5 @@
+import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/utils';
+
+export function isNullLiteral(i: TSESTree.Node): boolean {
+  return i.type === AST_NODE_TYPES.Literal && i.value === null;
+}
diff --git a/packages/eslint-plugin/src/util/isUndefinedIdentifier.ts b/packages/eslint-plugin/src/util/isUndefinedIdentifier.ts
@@ -0,0 +1,5 @@
+import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/utils';
+
+export function isUndefinedIdentifier(i: TSESTree.Node): boolean {
+  return i.type === AST_NODE_TYPES.Identifier && i.name === 'undefined';
+}