feat(eslint-plugin): [prefer-optional-chain] handle cases where the f…

…irst operands are unrelated to the rest of the chain and add type info (#6397)
typescript-eslint · Jul 7, 2023 · 02a37c4 · 02a37c4
1 parent ca3fc02
commit 02a37c4
Show file tree

Hide file tree

Showing 27 changed files with 4,531 additions and 2,125 deletions.
diff --git a/packages/ast-spec/src/expression/BinaryExpression/BinaryOperatorToText.ts b/packages/ast-spec/src/expression/BinaryExpression/BinaryOperatorToText.ts
@@ -0,0 +1,35 @@
+import type { SyntaxKind } from 'typescript';
+
+// the members of ts.BinaryOperator
+export interface BinaryOperatorToText {
+  [SyntaxKind.InstanceOfKeyword]: 'instanceof';
+  [SyntaxKind.InKeyword]: 'in';
+
+  // math
+  [SyntaxKind.AsteriskAsteriskToken]: '**';
+  [SyntaxKind.AsteriskToken]: '*';
+  [SyntaxKind.SlashToken]: '/';
+  [SyntaxKind.PercentToken]: '%';
+  [SyntaxKind.PlusToken]: '+';
+  [SyntaxKind.MinusToken]: '-';
+
+  // bitwise
+  [SyntaxKind.AmpersandToken]: '&';
+  [SyntaxKind.BarToken]: '|';
+  [SyntaxKind.CaretToken]: '^';
+  [SyntaxKind.LessThanLessThanToken]: '<<';
+  [SyntaxKind.GreaterThanGreaterThanToken]: '>>';
+  [SyntaxKind.GreaterThanGreaterThanGreaterThanToken]: '>>>';
+
+  // logical
+  [SyntaxKind.AmpersandAmpersandToken]: '&&';
+  [SyntaxKind.BarBarToken]: '||';
+  [SyntaxKind.LessThanToken]: '<';
+  [SyntaxKind.LessThanEqualsToken]: '<=';
+  [SyntaxKind.GreaterThanToken]: '>';
+  [SyntaxKind.GreaterThanEqualsToken]: '>=';
+  [SyntaxKind.EqualsEqualsToken]: '==';
+  [SyntaxKind.EqualsEqualsEqualsToken]: '===';
+  [SyntaxKind.ExclamationEqualsEqualsToken]: '!==';
+  [SyntaxKind.ExclamationEqualsToken]: '!=';
+}
diff --git a/packages/ast-spec/src/expression/BinaryExpression/spec.ts b/packages/ast-spec/src/expression/BinaryExpression/spec.ts
@@ -2,10 +2,14 @@ import type { AST_NODE_TYPES } from '../../ast-node-types';
 import type { BaseNode } from '../../base/BaseNode';
 import type { PrivateIdentifier } from '../../special/PrivateIdentifier/spec';
 import type { Expression } from '../../unions/Expression';
+import type { ValueOf } from '../../utils';
+import type { BinaryOperatorToText } from './BinaryOperatorToText';
+
+export * from './BinaryOperatorToText';
 
 export interface BinaryExpression extends BaseNode {
   type: AST_NODE_TYPES.BinaryExpression;
-  operator: string;
+  operator: ValueOf<BinaryOperatorToText>;
   left: Expression | PrivateIdentifier;
   right: Expression;
 }
diff --git a/packages/ast-spec/tests/BinaryOperatorToText.type-test.ts b/packages/ast-spec/tests/BinaryOperatorToText.type-test.ts
@@ -0,0 +1,20 @@
+import type {
+  AssignmentOperator,
+  BinaryOperator,
+  SyntaxKind,
+} from 'typescript';
+
+import type { BinaryOperatorToText } from '../src';
+
+type BinaryOperatorWithoutInvalidTypes = Exclude<
+  BinaryOperator,
+  | AssignmentOperator // --> AssignmentExpression
+  | SyntaxKind.CommaToken // -> SequenceExpression
+  | SyntaxKind.QuestionQuestionToken // -> LogicalExpression
+>;
+type _Test = {
+  readonly [T in BinaryOperatorWithoutInvalidTypes]: BinaryOperatorToText[T];
+  // If there are any BinaryOperator members that don't have a corresponding
+  // BinaryOperatorToText, then this line will error with "Type 'T' cannot
+  // be used to index type 'BinaryOperatorToText'."
+};
diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.md b/packages/eslint-plugin/docs/rules/prefer-optional-chain.md
@@ -57,13 +57,204 @@ 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
+
+In the context of the descriptions below a "loose boolean" operand is any operand that implicitly coerces the value to a boolean.
+Specifically the argument of the not operator (`!loose`) or a bare value in a logical expression (`loose && looser`).
+
+### `allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing`
+
+When this option is `true`, the rule will not provide an auto-fixer for cases where the return type of the expression would change. For example for the expression `!foo || foo.bar` the return type of the expression is `true | T`, however for the equivalent optional chain `foo?.bar` the return type of the expression is `undefined | T`. Thus changing the code from a logical expression to an optional chain expression has altered the type of the expression.
+
+In some cases this distinction _may_ matter - which is why these fixers are considered unsafe - they may break the build! For example in the following code:
+
+```ts
+declare const foo: { bar: boolean } | null | undefined;
+declare function acceptsBoolean(arg: boolean): void;
+
+// ✅ typechecks succesfully as the expression only returns `boolean`
+acceptsBoolean(foo != null && foo.bar);
+
+// ❌ typechecks UNSUCCESSFULLY as the expression returns `boolean | undefined`
+acceptsBoolean(foo != null && foo.bar);
+```
+
+This style of code isn't super common - which means having this option set to `true` _should_ be safe in most codebases. However we default it to `false` due to its unsafe nature. We have provided this option for convenience because it increases the autofix cases covered by the rule. If you set option to `true` the onus is entirely on you and your team to ensure that each fix is correct and safe and that it does not break the build.
+
+When this option is `false` unsafe cases will have suggestion fixers provided instead of auto-fixers - meaning you can manually apply the fix using your IDE tooling.
+
+### `checkAny`
+
+When this option is `true` the rule will check operands that are typed as `any` when inspecting "loose boolean" operands.
+
+<!--tabs-->
+
+#### ❌ Incorrect for `checkAny: true`
+
+```ts
+declare const thing: any;
+
+thing && thing.toString();
+```
+
+#### ✅ Correct for `checkAny: false`
+
+```ts
+declare const thing: any;
+
+thing && thing.toString();
+```
+
+<!--/tabs-->
+
+### `checkUnknown`
+
+When this option is `true` the rule will check operands that are typed as `unknown` when inspecting "loose boolean" operands.
+
+<!--tabs-->
+
+#### ❌ Incorrect for `checkUnknown: true`
+
+```ts
+declare const thing: unknown;
+
+thing && thing.toString();
+```
+
+#### ✅ Correct for `checkUnknown: false`
+
+```ts
+declare const thing: unknown;
+
+thing && thing.toString();
+```
+
+<!--/tabs-->
+
+### `checkString`
+
+When this option is `true` the rule will check operands that are typed as `string` when inspecting "loose boolean" operands.
+
+<!--tabs-->
+
+#### ❌ Incorrect for `checkString: true`
+
+```ts
+declare const thing: string;
+
+thing && thing.toString();
+```
+
+#### ✅ Correct for `checkString: false`
+
+```ts
+declare const thing: string;
+
+thing && thing.toString();
+```
+
+<!--/tabs-->
+
+### `checkNumber`
+
+When this option is `true` the rule will check operands that are typed as `number` when inspecting "loose boolean" operands.
+
+<!--tabs-->
+
+#### ❌ Incorrect for `checkNumber: true`
+
+```ts
+declare const thing: number;
+
+thing && thing.toString();
+```
+
+#### ✅ Correct for `checkNumber: false`
+
+```ts
+declare const thing: number;
+
+thing && thing.toString();
+```
+
+<!--/tabs-->
+
+### `checkBoolean`
+
+When this option is `true` the rule will check operands that are typed as `boolean` when inspecting "loose boolean" operands.
+
+<!--tabs-->
+
+#### ❌ Incorrect for `checkBoolean: true`
+
+```ts
+declare const thing: boolean;
+
+thing && thing.toString();
+```
+
+#### ✅ Correct for `checkBoolean: false`
+
+```ts
+declare const thing: boolean;
+
+thing && thing.toString();
+```
+
+<!--/tabs-->
+
+### `checkBigInt`
+
+When this option is `true` the rule will check operands that are typed as `bigint` when inspecting "loose boolean" operands.
+
+<!--tabs-->
+
+#### ❌ Incorrect for `checkBigInt: true`
+
+```ts
+declare const thing: bigint;
+
+thing && thing.toString();
+```
+
+#### ✅ Correct for `checkBigInt: false`
+
+```ts
+declare const thing: bigint;
+
+thing && thing.toString();
+```
+
+<!--/tabs-->
+
+### `requireNullish`
+
+When this option is `true` the rule will skip operands that are not typed with `null` and/or `undefined` when inspecting "loose boolean" operands.
+
+<!--tabs-->
+
+#### ❌ Incorrect for `requireNullish: true`
+
+```ts
+declare const thing1: string | null;
+thing1 && thing1.toString();
+```
+
+#### ✅ Correct for `requireNullish: true`
+
+```ts
+declare const thing1: string | null;
+thing1?.toString();
+
+declare const thing2: string;
+thing2 && thing2.toString();
+```
+
+<!--/tabs-->
 
 ## When Not To Use It
 
-If you don't mind using more explicit `&&`s, you don't need this rule.
+If you don't mind using more explicit `&&`s/`||`s, you don't need this rule.
 
 ## Further Reading
 

diff --git a/packages/eslint-plugin/package.json b/packages/eslint-plugin/package.json
@@ -51,13 +51,15 @@
     "generate:configs": "yarn tsx tools/generate-configs.ts",
     "lint": "nx lint",
     "test": "jest --coverage",
+    "test-single": "jest --no-coverage",
     "typecheck": "tsc -p tsconfig.json --noEmit"
   },
   "dependencies": {
     "@eslint-community/regexpp": "^4.5.0",
     "@typescript-eslint/scope-manager": "5.61.0",
     "@typescript-eslint/type-utils": "5.61.0",
     "@typescript-eslint/utils": "5.61.0",
+    "@typescript-eslint/visitor-keys": "5.61.0",
     "debug": "^4.3.4",
     "grapheme-splitter": "^1.0.4",
     "graphemer": "^1.4.0",

diff --git a/packages/eslint-plugin/src/configs/disable-type-checked.ts b/packages/eslint-plugin/src/configs/disable-type-checked.ts
@@ -37,6 +37,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,6 +24,5 @@ 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',
   },
 };
diff --git a/packages/eslint-plugin/src/rules/class-literal-property-style.ts b/packages/eslint-plugin/src/rules/class-literal-property-style.ts
@@ -65,7 +65,7 @@ export default util.createRule<Options, MessageIds>({
           if (
             node.kind !== 'get' ||
             !node.value.body ||
-            !node.value.body.body.length
+            node.value.body.body.length === 0
           ) {
             return;
           }

diff --git a/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts b/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts
@@ -229,7 +229,7 @@ export default util.createRule<Options, MessageIds>({
      * Checks if a function name is allowed and should not be checked.
      */
     function isAllowedName(node: TSESTree.Node | undefined): boolean {
-      if (!node || !options.allowedNames || !options.allowedNames.length) {
+      if (!node || !options.allowedNames || options.allowedNames.length === 0) {
         return false;
       }
 

diff --git a/packages/eslint-plugin/src/rules/indent.ts b/packages/eslint-plugin/src/rules/indent.ts
@@ -198,7 +198,7 @@ export default util.createRule<Options, MessageIds>({
         // transform it to a BinaryExpression
         return rules['BinaryExpression, LogicalExpression']({
           type: AST_NODE_TYPES.BinaryExpression,
-          operator: 'as',
+          operator: 'as' as any,
           left: node.expression,
           // the first typeAnnotation includes the as token
           right: node.typeAnnotation as any,
@@ -217,7 +217,7 @@ export default util.createRule<Options, MessageIds>({
           test: {
             parent: node,
             type: AST_NODE_TYPES.BinaryExpression,
-            operator: 'extends',
+            operator: 'extends' as any,
             left: node.checkType as any,
             right: node.extendsType as any,
 

diff --git a/packages/eslint-plugin/src/rules/no-extraneous-class.ts b/packages/eslint-plugin/src/rules/no-extraneous-class.ts
@@ -72,9 +72,8 @@ export default util.createRule<Options, MessageIds>({
     ): boolean => {
       return !!(
         allowWithDecorator &&
-        node &&
-        node.decorators &&
-        node.decorators.length
+        node?.decorators &&
+        node.decorators.length !== 0
       );
     };
 

diff --git a/packages/eslint-plugin/src/rules/prefer-includes.ts b/packages/eslint-plugin/src/rules/prefer-includes.ts
@@ -160,8 +160,7 @@ export default createRule({
           .getProperty('includes')
           ?.getDeclarations();
         if (
-          includesMethodDecl == null ||
-          !includesMethodDecl.some(includesMethodDecl =>
+          !includesMethodDecl?.some(includesMethodDecl =>
             hasSameParameters(includesMethodDecl, instanceofMethodDecl),
           )
         ) {

diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts
@@ -0,0 +1,14 @@
+export type PreferOptionalChainMessageIds =
+  | 'preferOptionalChain'
+  | 'optionalChainSuggest';
+
+export interface PreferOptionalChainOptions {
+  checkAny?: boolean;
+  checkUnknown?: boolean;
+  checkString?: boolean;
+  checkNumber?: boolean;
+  checkBoolean?: boolean;
+  checkBigInt?: boolean;
+  requireNullish?: boolean;
+  allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing?: boolean;
+}