fix(typescript-estree): correct ChainExpression interaction with pare…

…ntheses and non-nulls (#2380)

packages/eslint-plugin/src/rules/no-non-null-asserted-optional-chain.ts

@@ -7,8 +7,6 @@ import * as ts from 'typescript';
 import * as semver from 'semver';
 import * as util from '../util';
 
-type MessageIds = 'noNonNullOptionalChain' | 'suggestRemovingNonNull';
-
 const is3dot9 = semver.satisfies(
   ts.version,
   `>= 3.9.0 || >= 3.9.1-rc || >= 3.9.0-beta`,
@@ -17,7 +15,7 @@ const is3dot9 = semver.satisfies(
   },
 );
 
-export default util.createRule<[], MessageIds>({
+export default util.createRule({
   name: 'no-non-null-asserted-optional-chain',
   meta: {
     type: 'problem',
@@ -37,64 +35,22 @@ export default util.createRule<[], MessageIds>({
   },
   defaultOptions: [],
   create(context) {
-    const sourceCode = context.getSourceCode();
-
-    function isValidFor3dot9(node: TSESTree.ChainExpression): boolean {
-      if (!is3dot9) {
-        return false;
-      }
-
-      // TS3.9 made a breaking change to how non-null works with optional chains.
-      // Pre-3.9,  `x?.y!.z` means `(x?.y).z` - i.e. it essentially scrubbed the optionality from the chain
-      // Post-3.9, `x?.y!.z` means `x?.y!.z`  - i.e. it just asserts that the property `y` is non-null, not the result of `x?.y`.
-      // This means that for > 3.9, x?.y!.z is valid!
-      // NOTE: these cases are still invalid:
-      // - x?.y.z!
-      // - (x?.y)!.z
-
-      const parent = util.nullThrows(
-        node.parent,
-        util.NullThrowsReasons.MissingParent,
-      );
-      const grandparent = util.nullThrows(
-        parent.parent,
-        util.NullThrowsReasons.MissingParent,
-      );
-
-      if (
-        grandparent.type !== AST_NODE_TYPES.MemberExpression &&
-        grandparent.type !== AST_NODE_TYPES.CallExpression
-      ) {
-        return false;
-      }
-
-      const lastChildToken = util.nullThrows(
-        sourceCode.getLastToken(parent, util.isNotNonNullAssertionPunctuator),
-        util.NullThrowsReasons.MissingToken('last token', node.type),
-      );
-      if (util.isClosingParenToken(lastChildToken)) {
-        return false;
-      }
-
-      const tokenAfterNonNull = sourceCode.getTokenAfter(parent);
-      if (
-        tokenAfterNonNull != null &&
-        util.isClosingParenToken(tokenAfterNonNull)
-      ) {
-        return false;
-      }
+    // TS3.9 made a breaking change to how non-null works with optional chains.
+    // Pre-3.9,  `x?.y!.z` means `(x?.y).z` - i.e. it essentially scrubbed the optionality from the chain
+    // Post-3.9, `x?.y!.z` means `x?.y!.z`  - i.e. it just asserts that the property `y` is non-null, not the result of `x?.y`.
+    // This means that for > 3.9, x?.y!.z is valid!
+    //
+    // NOTE: these cases are still invalid for 3.9:
+    // - x?.y.z!
+    // - (x?.y)!.z
 
-      return true;
-    }
-
-    return {
+    const baseSelectors = {
+      // non-nulling a wrapped chain will scrub all nulls introduced by the chain
+      // (x?.y)!
+      // (x?.())!
       'TSNonNullExpression > ChainExpression'(
         node: TSESTree.ChainExpression,
       ): void {
-        if (isValidFor3dot9(node)) {
-          return;
-        }
-
         // selector guarantees this assertion
         const parent = node.parent as TSESTree.TSNonNullExpression;
         context.report({
@@ -114,6 +70,84 @@ export default util.createRule<[], MessageIds>({
           ],
         });
       },
+
+      // non-nulling at the end of a chain will scrub all nulls introduced by the chain
+      // x?.y!
+      // x?.()!
+      'ChainExpression > TSNonNullExpression'(
+        node: TSESTree.TSNonNullExpression,
+      ): void {
+        context.report({
+          node,
+          messageId: 'noNonNullOptionalChain',
+          // use a suggestion instead of a fixer, because this can obviously break type checks
+          suggest: [
+            {
+              messageId: 'suggestRemovingNonNull',
+              fix(fixer): TSESLint.RuleFix {
+                return fixer.removeRange([node.range[1] - 1, node.range[1]]);
+              },
+            },
+          ],
+        });
+      },
+    };
+
+    if (is3dot9) {
+      return baseSelectors;
+    }
+
+    return {
+      ...baseSelectors,
+      [[
+        // > :not(ChainExpression) because that case is handled by a previous selector
+        'MemberExpression > TSNonNullExpression.object > :not(ChainExpression)',
+        'CallExpression > TSNonNullExpression.callee > :not(ChainExpression)',
+      ].join(', ')](child: TSESTree.Node): void {
+        // selector guarantees this assertion
+        const node = child.parent as TSESTree.TSNonNullExpression;
+
+        let current = child;
+        while (current) {
+          switch (current.type) {
+            case AST_NODE_TYPES.MemberExpression:
+              if (current.optional) {
+                // found an optional chain! stop traversing
+                break;
+              }
+
+              current = current.object;
+              continue;
+
+            case AST_NODE_TYPES.CallExpression:
+              if (current.optional) {
+                // found an optional chain! stop traversing
+                break;
+              }
+
+              current = current.callee;
+              continue;
+
+            default:
+              // something that's not a ChainElement, which means this is not an optional chain we want to check
+              return;
+          }
+        }
+
+        context.report({
+          node,
+          messageId: 'noNonNullOptionalChain',
+          // use a suggestion instead of a fixer, because this can obviously break type checks
+          suggest: [
+            {
+              messageId: 'suggestRemovingNonNull',
+              fix(fixer): TSESLint.RuleFix {
+                return fixer.removeRange([node.range[1] - 1, node.range[1]]);
+              },
+            },
+          ],
+        });
+      },
     };
   },
 });

packages/eslint-plugin/src/rules/prefer-optional-chain.ts

@@ -234,6 +234,12 @@ export default util.createRule({
       }
 
       if (node.type === AST_NODE_TYPES.ChainExpression) {
+        /* istanbul ignore if */ if (
+          node.expression.type === AST_NODE_TYPES.TSNonNullExpression
+        ) {
+          // this shouldn't happen
+          return '';
+        }
         return getText(node.expression);
       }
 

packages/eslint-plugin/tests/rules/no-non-null-asserted-optional-chain.test.ts

@@ -8,7 +8,11 @@ const ruleTester = new RuleTester({
 ruleTester.run('no-non-null-asserted-optional-chain', rule, {
   valid: [
     'foo.bar!;',
+    'foo.bar!.baz;',
+    'foo.bar!.baz();',
     'foo.bar()!;',
+    'foo.bar()!();',
+    'foo.bar()!.baz;',
     'foo?.bar;',
     'foo?.bar();',
     '(foo?.bar).baz!;',

packages/types/src/ts-estree.ts

@@ -308,7 +308,10 @@ export type Node =
 export type Accessibility = 'public' | 'protected' | 'private';
 export type BindingPattern = ArrayPattern | ObjectPattern;
 export type BindingName = BindingPattern | Identifier;
-export type ChainElement = CallExpression | MemberExpression;
+export type ChainElement =
+  | CallExpression
+  | MemberExpression
+  | TSNonNullExpression;
 export type ClassElement =
   | ClassProperty
   | MethodDefinition

packages/typescript-estree/src/convert.ts

@@ -14,7 +14,7 @@ import {
   getTextForTokenKind,
   getTSNodeAccessibility,
   hasModifier,
-  isChildOptionalChain,
+  isChildUnwrappableOptionalChain,
   isComma,
   isComputedProperty,
   isESTreeClassMember,
@@ -406,24 +406,36 @@ export class Converter {
     tsNode:
       | ts.PropertyAccessExpression
       | ts.ElementAccessExpression
-      | ts.CallExpression,
+      | ts.CallExpression
+      | ts.NonNullExpression,
   ): TSESTree.ChainExpression | TSESTree.ChainElement {
-    let child = (node.type === AST_NODE_TYPES.MemberExpression
-      ? node.object
-      : node.callee) as TSESTree.Node;
-    const isChildOptional = isChildOptionalChain(tsNode, child);
+    const { child, isOptional } = ((): {
+      child: TSESTree.Node;
+      isOptional: boolean;
+    } => {
+      if (node.type === AST_NODE_TYPES.MemberExpression) {
+        return { child: node.object, isOptional: node.optional };
+      }
+      if (node.type === AST_NODE_TYPES.CallExpression) {
+        return { child: node.callee, isOptional: node.optional };
+      }
+      return { child: node.expression, isOptional: false };
+    })();
+    const isChildUnwrappable = isChildUnwrappableOptionalChain(tsNode, child);
 
-    if (!isChildOptional && !node.optional) {
+    if (!isChildUnwrappable && !isOptional) {
       return node;
     }
 
-    if (isChainExpression(child)) {
+    if (isChildUnwrappable && isChainExpression(child)) {
       // unwrap the chain expression child
-      child = child.expression;
+      const newChild = child.expression;
       if (node.type === AST_NODE_TYPES.MemberExpression) {
-        node.object = child;
+        node.object = newChild;
+      } else if (node.type === AST_NODE_TYPES.CallExpression) {
+        node.callee = newChild;
       } else {
-        node.callee = child;
+        node.expression = newChild;
       }
     }
 
@@ -2209,10 +2221,12 @@ export class Converter {
       }
 
       case SyntaxKind.NonNullExpression: {
-        return this.createNode<TSESTree.TSNonNullExpression>(node, {
+        const nnExpr = this.createNode<TSESTree.TSNonNullExpression>(node, {
           type: AST_NODE_TYPES.TSNonNullExpression,
           expression: this.convertChild(node.expression),
         });
+
+        return this.convertChainExpression(nnExpr, node);
       }
 
       case SyntaxKind.TypeLiteral: {

packages/typescript-estree/src/node-utils.ts

@@ -1,7 +1,6 @@
 import unescape from 'lodash/unescape';
 import * as ts from 'typescript';
 import { AST_NODE_TYPES, AST_TOKEN_TYPES, TSESTree } from './ts-estree';
-import { typescriptVersionIsAtLeast } from './version-check';
 
 const SyntaxKind = ts.SyntaxKind;
 
@@ -452,11 +451,12 @@ export function isChainExpression(
 /**
  * Returns true of the child of property access expression is an optional chain
  */
-export function isChildOptionalChain(
+export function isChildUnwrappableOptionalChain(
   node:
     | ts.PropertyAccessExpression
     | ts.ElementAccessExpression
-    | ts.CallExpression,
+    | ts.CallExpression
+    | ts.NonNullExpression,
   child: TSESTree.Node,
 ): boolean {
   if (
@@ -467,30 +467,6 @@ export function isChildOptionalChain(
     return true;
   }
 
-  if (!typescriptVersionIsAtLeast['3.9']) {
-    return false;
-  }
-
-  // TS3.9 made a breaking change to how non-null works with optional chains.
-  // Pre-3.9,  `x?.y!.z` means `(x?.y).z` - i.e. it essentially scrubbed the optionality from the chain
-  // Post-3.9, `x?.y!.z` means `x?.y!.z`  - i.e. it just asserts that the property `y` is non-null, not the result of `x?.y`
-
-  if (
-    child.type !== AST_NODE_TYPES.TSNonNullExpression ||
-    !isChainExpression(child.expression)
-  ) {
-    return false;
-  }
-
-  if (
-    // make sure it's not (x.y)!.z
-    node.expression.kind === ts.SyntaxKind.NonNullExpression &&
-    (node.expression as ts.NonNullExpression).expression.kind !==
-      ts.SyntaxKind.ParenthesizedExpression
-  ) {
-    return true;
-  }
-
   return false;
 }
 

packages/typescript-estree/src/ts-estree/estree-to-ts-node-types.ts

@@ -23,7 +23,8 @@ export interface EstreeToTsNodeTypes {
   [AST_NODE_TYPES.ChainExpression]:
     | ts.CallExpression
     | ts.PropertyAccessExpression
-    | ts.ElementAccessExpression;
+    | ts.ElementAccessExpression
+    | ts.NonNullExpression;
   [AST_NODE_TYPES.ClassBody]: ts.ClassDeclaration | ts.ClassExpression;
   [AST_NODE_TYPES.ClassDeclaration]: ts.ClassDeclaration;
   [AST_NODE_TYPES.ClassExpression]: ts.ClassExpression;

packages/typescript-estree/tests/ast-alignment/fixtures-to-test.ts

@@ -411,21 +411,6 @@ tester.addFixturePatternConfig('typescript/basics', {
      * https://github.com/babel/babel/issues/11939
      */
     'catch-clause-with-annotation',
-
-    /**
-     * Optional chaining
-     * Babel has updated to ESTree's representation, and we haven't yet
-     * TODO: remove this with the v4 release
-     */
-    'optional-chain-call-with-non-null-assertion',
-    'optional-chain-call-with-parens',
-    'optional-chain-call',
-    'optional-chain-element-access-with-non-null-assertion',
-    'optional-chain-element-access-with-parens',
-    'optional-chain-element-access',
-    'optional-chain-with-non-null-assertion',
-    'optional-chain-with-parens',
-    'optional-chain',
   ],
   ignoreSourceType: [
     /**
@@ -479,14 +464,6 @@ tester.addFixturePatternConfig('typescript/decorators/property-decorators', {
 
 tester.addFixturePatternConfig('typescript/expressions', {
   fileType: 'ts',
-  ignore: [
-    /**
-     * Optional chaining
-     * Babel has updated to ESTree's representation, and we haven't yet
-     * TODO: remove this with the v4 release
-     */
-    'optional-call-expression-type-arguments',
-  ],
 });
 
 tester.addFixturePatternConfig('typescript/errorRecovery', {