fix(ts-estree): make sure that every node can be converted to tsNode (#…

…287) * fix(ts-estree): improve types in fix export * fix(ts-estree): register node to tsNodeToESTreeNodeMap only once * fix(eslint-plugin): fix crash in promise-function-async * fix(ts-estree): make sure that every node can be translated to tsNode
typescript-eslint · Feb 19, 2019 · 9f1d314 · 9f1d314
1 parent 80bd72c
commit 9f1d314
Show file tree

Hide file tree

Showing 6 changed files with 121 additions and 81 deletions.
diff --git a/packages/eslint-plugin/src/rules/promise-function-async.ts b/packages/eslint-plugin/src/rules/promise-function-async.ts
@@ -92,10 +92,13 @@ export default util.createRule<Options, MessageIds>({
 
     function validateNode(node: TSESTree.Node) {
       const originalNode = parserServices.esTreeNodeToTSNodeMap.get(node);
-      const [callSignature] = checker
+      const signatures = checker
         .getTypeAtLocation(originalNode)
         .getCallSignatures();
-      const returnType = checker.getReturnTypeOfSignature(callSignature);
+      if (!signatures.length) {
+        return;
+      }
+      const returnType = checker.getReturnTypeOfSignature(signatures[0]);
 
       if (!util.containsTypeByName(returnType, allAllowedPromiseNames)) {
         return;

diff --git a/packages/eslint-plugin/tests/rules/promise-function-async.test.ts b/packages/eslint-plugin/tests/rules/promise-function-async.test.ts
@@ -45,7 +45,11 @@ class Test {
     return new Promise<void>();
   }
 }
-    `
+    `,
+    // https://github.com/typescript-eslint/typescript-eslint/issues/227
+    `export function valid(n: number) { return n; }`,
+    `export default function invalid(n: number) { return n; }`,
+    `class Foo { constructor() { } }`
   ],
   invalid: [
     {

diff --git a/packages/typescript-estree/src/convert.ts b/packages/typescript-estree/src/convert.ts
@@ -10,7 +10,6 @@ import {
   canContainDirective,
   createError,
   findNextToken,
-  fixExports,
   getBinaryExpressionType,
   getDeclarationKind,
   getLastModifier,
@@ -111,29 +110,78 @@ export class Converter {
       this.allowPattern = allowPattern;
     }
 
-    let result: TSESTree.BaseNode | null = this.convertNode(
-      node as TSNode,
-      parent || node.parent
-    );
+    let result = this.convertNode(node as TSNode, parent || node.parent);
 
-    if (result && this.options.shouldProvideParserServices) {
-      this.tsNodeToESTreeNodeMap.set(node, result);
-      if (
-        node.kind !== SyntaxKind.ParenthesizedExpression &&
-        node.kind !== SyntaxKind.ComputedPropertyName
-      ) {
-        // Parenthesized expressions and computed property names do not have individual nodes in ESTree.
-        // Therefore, result.type will never "match" node.kind if it is a ParenthesizedExpression
-        // or a ComputedPropertyName and, furthermore, will overwrite the "matching" node
-        this.esTreeNodeToTSNodeMap.set(result, node);
-      }
-    }
+    this.registerTSNodeInNodeMap(node, result);
 
     this.inTypeMode = typeMode;
     this.allowPattern = pattern;
     return result;
   }
 
+  /**
+   * Fixes the exports of the given ts.Node
+   * @param node   the ts.Node
+   * @param result result
+   * @returns the ESTreeNode with fixed exports
+   */
+  private fixExports<T extends TSESTree.ExportDeclaration>(
+    node: ts.Node,
+    result: T
+  ): TSESTree.ExportDefaultDeclaration | TSESTree.ExportNamedDeclaration | T {
+    // check for exports
+    if (node.modifiers && node.modifiers[0].kind === SyntaxKind.ExportKeyword) {
+      /**
+       * Make sure that original node is registered instead of export
+       */
+      this.registerTSNodeInNodeMap(node, result);
+
+      const exportKeyword = node.modifiers[0];
+      const nextModifier = node.modifiers[1];
+      const declarationIsDefault =
+        nextModifier && nextModifier.kind === SyntaxKind.DefaultKeyword;
+
+      const varToken = declarationIsDefault
+        ? findNextToken(nextModifier, this.ast, this.ast)
+        : findNextToken(exportKeyword, this.ast, this.ast);
+
+      result.range[0] = varToken!.getStart(this.ast);
+      result.loc = getLocFor(result.range[0], result.range[1], this.ast);
+
+      if (declarationIsDefault) {
+        return this.createNode<TSESTree.ExportDefaultDeclaration>(node, {
+          type: AST_NODE_TYPES.ExportDefaultDeclaration,
+          declaration: result,
+          range: [exportKeyword.getStart(this.ast), result.range[1]]
+        });
+      } else {
+        return this.createNode<TSESTree.ExportNamedDeclaration>(node, {
+          type: AST_NODE_TYPES.ExportNamedDeclaration,
+          declaration: result,
+          specifiers: [],
+          source: null,
+          range: [exportKeyword.getStart(this.ast), result.range[1]]
+        });
+      }
+    }
+
+    return result;
+  }
+
+  /**
+   * Register specific TypeScript node into map with first ESTree node provided
+   */
+  private registerTSNodeInNodeMap(
+    node: ts.Node,
+    result: TSESTree.BaseNode | null
+  ) {
+    if (result && this.options.shouldProvideParserServices) {
+      if (!this.tsNodeToESTreeNodeMap.has(node)) {
+        this.tsNodeToESTreeNodeMap.set(node, result);
+      }
+    }
+  }
+
   /**
    * Converts a TypeScript node into an ESTree node.
    * @param child the child ts.Node
@@ -176,6 +224,9 @@ export class Converter {
       result.loc = getLocFor(result.range[0], result.range[1], this.ast);
     }
 
+    if (result && this.options.shouldProvideParserServices) {
+      this.esTreeNodeToTSNodeMap.set(result, node);
+    }
     return result as T;
   }
 
@@ -410,11 +461,7 @@ export class Converter {
         break;
     }
 
-    if (result && this.options.shouldProvideParserServices) {
-      this.tsNodeToESTreeNodeMap.set(node, result);
-      this.esTreeNodeToTSNodeMap.set(result, node);
-    }
-
+    this.registerTSNodeInNodeMap(node, result);
     return result;
   }
 
@@ -715,7 +762,7 @@ export class Converter {
         }
 
         // check for exports
-        return fixExports(node, result, this.ast);
+        return this.fixExports(node, result);
       }
 
       case SyntaxKind.VariableDeclaration: {
@@ -764,7 +811,7 @@ export class Converter {
         }
 
         // check for exports
-        return fixExports(node, result, this.ast);
+        return this.fixExports(node, result);
       }
 
       // mostly for for-of, for-in
@@ -1431,7 +1478,7 @@ export class Converter {
         }
 
         // check for exports
-        return fixExports(node, result, this.ast);
+        return this.fixExports(node, result);
       }
 
       // Modules
@@ -2095,7 +2142,7 @@ export class Converter {
         }
 
         // check for exports
-        return fixExports(node, result, this.ast);
+        return this.fixExports(node, result);
       }
 
       case SyntaxKind.MethodSignature: {
@@ -2310,7 +2357,7 @@ export class Converter {
           result.declare = true;
         }
         // check for exports
-        return fixExports(node, result, this.ast);
+        return this.fixExports(node, result);
       }
 
       case SyntaxKind.FirstTypeNode: {
@@ -2356,7 +2403,7 @@ export class Converter {
           result.decorators = node.decorators.map(el => this.convertChild(el));
         }
         // ...then check for exports
-        return fixExports(node, result, this.ast);
+        return this.fixExports(node, result);
       }
 
       case SyntaxKind.EnumMember: {
@@ -2384,7 +2431,7 @@ export class Converter {
           result.global = true;
         }
         // ...then check for exports
-        return fixExports(node, result, this.ast);
+        return this.fixExports(node, result);
       }
 
       // TypeScript specific types

diff --git a/packages/typescript-estree/src/node-utils.ts b/packages/typescript-estree/src/node-utils.ts
@@ -447,54 +447,6 @@ export function isOptional(node: {
     : false;
 }
 
-/**
- * Fixes the exports of the given ts.Node
- * @param node   the ts.Node
- * @param result result
- * @param ast    the AST
- * @returns the ESTreeNode with fixed exports
- */
-export function fixExports<T extends TSESTree.BaseNode>(
-  node: ts.Node,
-  result: T,
-  ast: ts.SourceFile
-): TSESTree.ExportDefaultDeclaration | TSESTree.ExportNamedDeclaration | T {
-  // check for exports
-  if (node.modifiers && node.modifiers[0].kind === SyntaxKind.ExportKeyword) {
-    const exportKeyword = node.modifiers[0];
-    const nextModifier = node.modifiers[1];
-    const declarationIsDefault =
-      nextModifier && nextModifier.kind === SyntaxKind.DefaultKeyword;
-
-    const varToken = declarationIsDefault
-      ? findNextToken(nextModifier, ast, ast)
-      : findNextToken(exportKeyword, ast, ast);
-
-    result.range![0] = varToken!.getStart(ast);
-    result.loc = getLocFor(result.range![0], result.range![1], ast);
-
-    if (declarationIsDefault) {
-      return {
-        type: AST_NODE_TYPES.ExportDefaultDeclaration,
-        declaration: result as any,
-        range: [exportKeyword.getStart(ast), result.range![1]],
-        loc: getLocFor(exportKeyword.getStart(ast), result.range![1], ast)
-      };
-    } else {
-      return {
-        type: AST_NODE_TYPES.ExportNamedDeclaration,
-        declaration: result as any,
-        range: [exportKeyword.getStart(ast), result.range![1]],
-        loc: getLocFor(exportKeyword.getStart(ast), result.range![1], ast),
-        specifiers: [],
-        source: null
-      };
-    }
-  }
-
-  return result;
-}
-
 /**
  * Returns the type of a given ts.Token
  * @param token the ts.Token

diff --git a/packages/typescript-estree/src/ts-estree/ts-estree.ts b/packages/typescript-estree/src/ts-estree/ts-estree.ts
@@ -127,6 +127,7 @@ export type Node =
   | JSXOpeningFragment
   | JSXSpreadAttribute
   | JSXSpreadChild
+  | JSXMemberExpression
   | JSXText
   | LabeledStatement
   | Literal
@@ -273,7 +274,7 @@ export type ExportDeclaration =
   | TSInterfaceDeclaration
   | TSModuleDeclaration
   | TSTypeAliasDeclaration
-  | VariableDeclarator;
+  | VariableDeclaration;
 export type Expression =
   | ArrowFunctionExpression
   | AssignmentExpression

diff --git a/packages/typescript-estree/tests/lib/convert.ts b/packages/typescript-estree/tests/lib/convert.ts
@@ -149,4 +149,37 @@ describe('convert', () => {
     );
     checkMaps(ast);
   });
+
+  it('nodeMaps should contain export node', () => {
+    const ast = convertCode(`export function foo () {}`);
+
+    const instance = new Converter(ast, {
+      errorOnUnknownASTType: false,
+      useJSXTextNode: false,
+      shouldProvideParserServices: true
+    });
+    const program = instance.convertProgram();
+    const maps = instance.getASTMaps();
+
+    function checkMaps(child: any) {
+      child.forEachChild((node: any) => {
+        if (node.kind !== ts.SyntaxKind.EndOfFileToken) {
+          expect(ast).toBe(
+            maps.esTreeNodeToTSNodeMap.get(maps.tsNodeToESTreeNodeMap.get(ast))
+          );
+        }
+        checkMaps(node);
+      });
+    }
+
+    expect(ast).toBe(
+      maps.esTreeNodeToTSNodeMap.get(maps.tsNodeToESTreeNodeMap.get(ast))
+    );
+
+    expect(maps.esTreeNodeToTSNodeMap.get(program.body[0])).toBeDefined();
+    expect(program.body[0]).not.toBe(
+      maps.tsNodeToESTreeNodeMap.get(ast.statements[0])
+    );
+    checkMaps(ast);
+  });
 });