Properly elide TS imports even when referenced by shadowing variables (…

…#342)

Fixes #298

TypeScript is required to remove imports where all bindings are never referenced
in a value position, but previously it could have false positives when there are
shadowed variables with the same name. Now, we do the intelligent scope-based
shadowed global detection like we do with import replacement and properly elide
when necessary.

src/index.ts

@@ -7,6 +7,7 @@ import {Scope} from "./parser/tokenizer/state";
 import TokenProcessor from "./TokenProcessor";
 import RootTransformer from "./transformers/RootTransformer";
 import formatTokens from "./util/formatTokens";
+import getTSImportedNames from "./util/getTSImportedNames";
 
 export type Transform = "jsx" | "typescript" | "flow" | "imports";
 
@@ -136,10 +137,14 @@ function getSucraseContext(code: string, options: Options): SucraseContext {
       enableLegacyTypeScriptModuleInterop,
     );
     importProcessor.preprocessTokens();
+    // We need to mark shadowed globals after processing imports so we know that the globals are,
+    // but before type-only import pruning, since that relies on shadowing information.
+    identifyShadowedGlobals(tokenProcessor, scopes, importProcessor.getGlobalNames());
     if (options.transforms.includes("typescript")) {
       importProcessor.pruneTypeOnlyImports();
     }
-    identifyShadowedGlobals(tokenProcessor, scopes, importProcessor.getGlobalNames());
+  } else if (options.transforms.includes("typescript")) {
+    identifyShadowedGlobals(tokenProcessor, scopes, getTSImportedNames(tokenProcessor));
   }
   return {tokenProcessor, scopes, nameManager, importProcessor};
 }

src/util/getNonTypeIdentifiers.ts

@@ -12,7 +12,8 @@ export function getNonTypeIdentifiers(tokens: TokenProcessor): Set<string> {
       !token.isType &&
       (token.identifierRole === IdentifierRole.Access ||
         token.identifierRole === IdentifierRole.ObjectShorthand ||
-        token.identifierRole === IdentifierRole.ExportAccess)
+        token.identifierRole === IdentifierRole.ExportAccess) &&
+      !token.shadowsGlobal
     ) {
       nonTypeIdentifiers.add(tokens.identifierNameForToken(token));
     }

src/util/getTSImportedNames.ts

@@ -0,0 +1,88 @@
+import {ContextualKeyword} from "../parser/tokenizer";
+import {TokenType as tt} from "../parser/tokenizer/types";
+import TokenProcessor from "../TokenProcessor";
+
+/**
+ * Special case code to scan for imported names in ESM TypeScript. We need to do this so we can
+ * properly get globals so we can compute shadowed globals.
+ *
+ * This is similar to logic in CJSImportProcessor, but trimmed down to avoid logic with CJS
+ * replacement and flow type imports.
+ */
+export default function getTSImportedNames(tokens: TokenProcessor): Set<string> {
+  const importedNames = new Set();
+  for (let i = 0; i < tokens.tokens.length; i++) {
+    if (
+      tokens.matchesAtIndex(i, [tt._import]) &&
+      !tokens.matchesAtIndex(i, [tt._import, tt.name, tt.eq])
+    ) {
+      collectNamesForImport(tokens, i, importedNames);
+    }
+  }
+  return importedNames;
+}
+
+function collectNamesForImport(
+  tokens: TokenProcessor,
+  index: number,
+  importedNames: Set<string>,
+): void {
+  index++;
+
+  if (tokens.matchesAtIndex(index, [tt.parenL])) {
+    // Dynamic import, so nothing to do
+    return;
+  }
+
+  if (tokens.matchesAtIndex(index, [tt.name])) {
+    importedNames.add(tokens.identifierNameAtIndex(index));
+    index++;
+    if (tokens.matchesAtIndex(index, [tt.comma])) {
+      index++;
+    }
+  }
+
+  if (tokens.matchesAtIndex(index, [tt.star])) {
+    // * as
+    index += 2;
+    importedNames.add(tokens.identifierNameAtIndex(index));
+    index++;
+  }
+
+  if (tokens.matchesAtIndex(index, [tt.braceL])) {
+    index++;
+    collectNamesForNamedImport(tokens, index, importedNames);
+  }
+}
+
+function collectNamesForNamedImport(
+  tokens: TokenProcessor,
+  index: number,
+  importedNames: Set<string>,
+): void {
+  while (true) {
+    if (tokens.matchesAtIndex(index, [tt.braceR])) {
+      return;
+    }
+
+    // We care about the local name, which might be the first token, or if there's an "as", is the
+    // one after that.
+    let name = tokens.identifierNameAtIndex(index);
+    index++;
+    if (tokens.matchesContextualAtIndex(index, ContextualKeyword._as)) {
+      index++;
+      name = tokens.identifierNameAtIndex(index);
+      index++;
+    }
+    importedNames.add(name);
+    if (tokens.matchesAtIndex(index, [tt.comma, tt.braceR])) {
+      return;
+    } else if (tokens.matchesAtIndex(index, [tt.braceR])) {
+      return;
+    } else if (tokens.matchesAtIndex(index, [tt.comma])) {
+      index++;
+    } else {
+      throw new Error(`Unexpected token: ${JSON.stringify(tokens.tokens[index])}`);
+    }
+  }
+}

test/typescript-test.ts

@@ -1270,4 +1270,58 @@ describe("typescript transform", () => {
     `,
     );
   });
+
+  it("properly elides CJS imports that only have value references in shadowed names", () => {
+    assertTypeScriptResult(
+      `
+      import T from './T';
+      
+      const x: T = 3;
+
+      function foo() {
+        let T = 3;
+        console.log(T);
+      }
+    `,
+      `"use strict";${IMPORT_DEFAULT_PREFIX}
+      
+      
+      const x = 3;
+
+      function foo() {
+        let T = 3;
+        console.log(T);
+      }
+    `,
+    );
+  });
+
+  it("properly elides ESM imports that only have value references in shadowed names", () => {
+    assertTypeScriptESMResult(
+      `
+      import T, {a as b, c} from './T';
+      import {d, e} from './foo';
+      
+      const x: T = 3;
+      console.log(e);
+
+      function foo() {
+        let T = 3, b = 4, c = 5, d = 6;
+        console.log(T, b, c, d);
+      }
+    `,
+      `
+
+      import { e} from './foo';
+      
+      const x = 3;
+      console.log(e);
+
+      function foo() {
+        let T = 3, b = 4, c = 5, d = 6;
+        console.log(T, b, c, d);
+      }
+    `,
+    );
+  });
 });