Repository: alangpierce/sucrase (ts)

Issue:
Incorrect elision of named import if typings of a callback function contains a parameter with the same name
I came across a bit of an odd bug while using Sucrase.

In the following code, notice that `someVariable` is used as an import name but also used in the type definition of the argument of the function. 
```typescript
import { someVariable } from './someFile';

function Foo(arg: (someVariable: any) => void) {
  console.log(arg, someVariable);
}
```

Somehow, in the generated code by Sucrase, the import is lost while it should have been kept
```javascript
function Foo(arg) {
  console.log(arg, someVariable);
}
```

Renaming the import or the parameter name of the callback type will fix the issue


Playground link https://sucrase.io/#transforms=jsx%2Ctypescript&enableLegacyTypeScriptModuleInterop=true&enableLegacyBabel5ModuleInterop=true&compareWithTypeScript=true&compressedCode=H4sIAKqarmQAA12Psc7CMBCD9zyFxQKVEN1BPyNPgNiv4egfcclVlwCqEO9OihiA1bI%2F22hblP%2BQEeKgVnALIugYxlGvfEQ3Il%2B8UWb3dtyRNfKBLFAnjAdOphHzVTvJuyA83zhXqXsbUbSCEkWuHQxPIh35MwayqhU2zD5Zs8lPaax7Ug%2BWzMtX7mfamYfiTpfkS9CEneqCrF9j8YlaT5wGf1tcNRwb3B3gNWUVXon2U2L59aPZuMcTOupspAwBAAA%3D

Pull request:
Fix shadowed global detection to ignore declarations in type tokens
Fixes #808

All identifier tokens, including type tokens, have a role assigned by the parser that indicates whether they're a declaration, usage, etc, and even for type tokens this is important for TS automatic export elision. When identifying shadowed globals, though, only non-type declarations should be considered. This fixes an issue where parameters within TS function types were inadvertently being treated as declarations.

Patch:
diff --git a/src/identifyShadowedGlobals.ts b/src/identifyShadowedGlobals.ts
index b872f2f..550f60b 100644
--- a/src/identifyShadowedGlobals.ts
+++ b/src/identifyShadowedGlobals.ts
@@ -31,6 +31,7 @@ export function hasShadowedGlobals(tokens: TokenProcessor, globalNames: Set<stri
   for (const token of tokens.tokens) {
     if (
       token.type === tt.name &&
+      !token.isType &&
       isNonTopLevelDeclaration(token) &&
       globalNames.has(tokens.identifierNameForToken(token))
     ) {
@@ -64,7 +65,7 @@ function markShadowedGlobals(
 
     const token = tokens.tokens[i];
     const name = tokens.identifierNameForToken(token);
-    if (scopeStack.length > 1 && token.type === tt.name && globalNames.has(name)) {
+    if (scopeStack.length > 1 && !token.isType && token.type === tt.name && globalNames.has(name)) {
       if (isBlockScopedDeclaration(token)) {
         markShadowedForScope(scopeStack[scopeStack.length - 1], tokens, name);
       } else if (isFunctionScopedDeclaration(token)) {


Test patch:
diff --git a/test/identifyShadowedGlobals-test.ts b/test/identifyShadowedGlobals-test.ts
index e91d0c4..a8814e3 100644
--- a/test/identifyShadowedGlobals-test.ts
+++ b/test/identifyShadowedGlobals-test.ts
@@ -8,7 +8,7 @@ import {parse} from "../src/parser";
 import TokenProcessor from "../src/TokenProcessor";
 
 function assertHasShadowedGlobals(code: string, expected: boolean): void {
-  const file = parse(code, false, false, false);
+  const file = parse(code, false, true, false);
   const nameManager = new NameManager(code, file.tokens);
   const helperManager = new HelperManager(nameManager);
   const tokenProcessor = new TokenProcessor(code, file.tokens, false, false, helperManager);
@@ -30,7 +30,7 @@ function assertHasShadowedGlobals(code: string, expected: boolean): void {
 }
 
 describe("identifyShadowedGlobals", () => {
-  it("properly does an up-front that there are any shadowed globals", () => {
+  it("properly does an up-front check that there are any shadowed globals", () => {
     assertHasShadowedGlobals(
       `
       import a from 'a';
@@ -53,4 +53,17 @@ describe("identifyShadowedGlobals", () => {
       false,
     );
   });
+
+  it("does not treat parameters within types as real declarations", () => {
+    assertHasShadowedGlobals(
+      `
+      import a from 'a';
+      
+      function foo(f: (a: number) => void) {
+        console.log(a);
+      }
+    `,
+      false,
+    );
+  });
 });
diff --git a/test/typescript-test.ts b/test/typescript-test.ts
index 144f12e..54661dd 100644
--- a/test/typescript-test.ts
+++ b/test/typescript-test.ts
@@ -1614,6 +1614,27 @@ describe("typescript transform", () => {
     );
   });
 
+  it("does not consider parameter lists in types to shadow names", () => {
+    assertTypeScriptESMResult(
+      `
+      import { someVariable } from './someFile';
+      import { otherVariable } from './otherFile';
+      
+      function Foo(arg: (someVariable: any) => void, otherVariable) {
+        console.log(arg, someVariable, otherVariable);
+      }
+    `,
+      `
+      import { someVariable } from './someFile';
+
+      
+      function Foo(arg, otherVariable) {
+        console.log(arg, someVariable, otherVariable);
+      }
+    `,
+    );
+  });
+
   it("produces proper import statements when eliding ESM imported names", () => {
     assertTypeScriptESMResult(
       `