Fix bug in importTracker: default and namespace imports are not exclusive (microsoft#24965)

Author: Andy

src/services/importTracker.ts

@@ -103,23 +103,17 @@ namespace ts.FindAllReferences {
                             break; // TODO: GH#23879
 
                         case SyntaxKind.ImportEqualsDeclaration:
-                            handleNamespaceImport(direct, direct.name, hasModifier(direct, ModifierFlags.Export));
+                            handleNamespaceImport(direct, direct.name, hasModifier(direct, ModifierFlags.Export), /*alreadyAddedDirect*/ false);
                             break;
 
                         case SyntaxKind.ImportDeclaration:
+                            directImports.push(direct);
                             const namedBindings = direct.importClause && direct.importClause.namedBindings;
                             if (namedBindings && namedBindings.kind === SyntaxKind.NamespaceImport) {
-                                handleNamespaceImport(direct, namedBindings.name);
+                                handleNamespaceImport(direct, namedBindings.name, /*isReExport*/ false, /*alreadyAddedDirect*/ true);
                             }
-                            else if (isDefaultImport(direct)) {
-                                const sourceFileLike = getSourceFileLikeForImportDeclaration(direct);
-                                if (!isAvailableThroughGlobal) {
-                                    addIndirectUser(sourceFileLike); // Add a check for indirect uses to handle synthetic default imports
-                                }
-                                directImports.push(direct);
-                            }
-                            else {
-                                directImports.push(direct);
+                            else if (!isAvailableThroughGlobal && isDefaultImport(direct)) {
+                                addIndirectUser(getSourceFileLikeForImportDeclaration(direct)); // Add a check for indirect uses to handle synthetic default imports
                             }
                             break;
 
@@ -145,10 +139,10 @@ namespace ts.FindAllReferences {
             }
         }
 
-        function handleNamespaceImport(importDeclaration: ImportEqualsDeclaration | ImportDeclaration, name: Identifier, isReExport?: boolean): void {
+        function handleNamespaceImport(importDeclaration: ImportEqualsDeclaration | ImportDeclaration, name: Identifier, isReExport: boolean, alreadyAddedDirect: boolean): void {
             if (exportKind === ExportKind.ExportEquals) {
                 // This is a direct import, not import-as-namespace.
-                directImports.push(importDeclaration);
+                if (!alreadyAddedDirect) directImports.push(importDeclaration);
             }
             else if (!isAvailableThroughGlobal) {
                 const sourceFileLike = getSourceFileLikeForImportDeclaration(importDeclaration);
@@ -247,34 +241,30 @@ namespace ts.FindAllReferences {
                 return;
             }
 
-            const { importClause } = decl;
-            if (!importClause) {
-                return;
-            }
+            const { name, namedBindings } = decl.importClause || { name: undefined, namedBindings: undefined };
 
-            const { namedBindings } = importClause;
-            if (namedBindings && namedBindings.kind === SyntaxKind.NamespaceImport) {
-                handleNamespaceImportLike(namedBindings.name);
-                return;
-            }
-
-            if (exportKind === ExportKind.Named) {
-                searchForNamedImport(namedBindings as NamedImports | undefined); // tslint:disable-line no-unnecessary-type-assertion (TODO: GH#18217)
-            }
-            else {
-                // `export =` might be imported by a default import if `--allowSyntheticDefaultImports` is on, so this handles both ExportKind.Default and ExportKind.ExportEquals
-                const { name } = importClause;
-                // If a default import has the same name as the default export, allow to rename it.
-                // Given `import f` and `export default function f`, we will rename both, but for `import g` we will rename just that.
-                if (name && (!isForRename || name.escapedText === symbolEscapedNameNoDefault(exportSymbol))) {
-                    const defaultImportAlias = checker.getSymbolAtLocation(name)!;
-                    addSearch(name, defaultImportAlias);
+            if (namedBindings) {
+                switch (namedBindings.kind) {
+                    case SyntaxKind.NamespaceImport:
+                        handleNamespaceImportLike(namedBindings.name);
+                        break;
+                    case SyntaxKind.NamedImports:
+                        // 'default' might be accessed as a named import `{ default as foo }`.
+                        if (exportKind === ExportKind.Named || exportKind === ExportKind.Default) {
+                            searchForNamedImport(namedBindings);
+                        }
+                        break;
+                    default:
+                        Debug.assertNever(namedBindings);
                 }
+            }
 
-                // 'default' might be accessed as a named import `{ default as foo }`.
-                if (exportKind === ExportKind.Default) {
-                    searchForNamedImport(namedBindings);
-                }
+            // `export =` might be imported by a default import if `--allowSyntheticDefaultImports` is on, so this handles both ExportKind.Default and ExportKind.ExportEquals.
+            // If a default import has the same name as the default export, allow to rename it.
+            // Given `import f` and `export default function f`, we will rename both, but for `import g` we will rename just that.
+            if (name && (exportKind === ExportKind.Default || exportKind === ExportKind.ExportEquals) && (!isForRename || name.escapedText === symbolEscapedNameNoDefault(exportSymbol))) {
+                const defaultImportAlias = checker.getSymbolAtLocation(name)!;
+                addSearch(name, defaultImportAlias);
             }
         }
 

tests/cases/fourslash/findAllRefsDefaultImport.ts

@@ -0,0 +1,13 @@
+/// <reference path="fourslash.ts" />
+
+// @Filename: /a.ts
+////export default function [|{| "isWriteAccess": true, "isDefinition": true |}a|]() {}
+
+// @Filename: /b.ts
+////import [|{| "isWriteAccess": true, "isDefinition": true |}a|], * as ns from "./a";
+
+const [r0, r1] = test.ranges();
+const a: FourSlashInterface.ReferenceGroup = { definition: "function a(): void", ranges: [r0] };
+const b: FourSlashInterface.ReferenceGroup = { definition: "(alias) function a(): void\nimport a", ranges: [r1] };
+verify.referenceGroups(r0, [a, b]);
+verify.referenceGroups(r1, [b, a]);

tests/cases/fourslash/fourslash.ts

@@ -255,7 +255,7 @@ declare namespace FourSlashInterface {
          * For each of starts, asserts the ranges that are referenced from there.
          * This uses the 'findReferences' command instead of 'getReferencesAtPosition', so references are grouped by their definition.
          */
-        referenceGroups(starts: ArrayOrSingle<string> | ArrayOrSingle<Range>, parts: Array<{ definition: ReferencesDefinition, ranges: Range[] }>): void;
+        referenceGroups(starts: ArrayOrSingle<string> | ArrayOrSingle<Range>, parts: Array<FourSlashInterface.ReferenceGroup>): void;
         singleReferenceGroup(definition: ReferencesDefinition, ranges?: Range[]): void;
         rangesAreOccurrences(isWriteAccess?: boolean): void;
         rangesWithSameTextAreRenameLocations(): void;
@@ -511,6 +511,10 @@ declare namespace FourSlashInterface {
         text: string;
         range: Range;
     }
+    interface ReferenceGroup {
+        readonly definition: ReferencesDefinition;
+        readonly ranges: ReadonlyArray<Range>;
+    }
     interface Diagnostic {
         message: string;
         /** @default `test.ranges()[0]` */