Fix duplicate auto-import completions (#42850)

* Fix duplicate auto-import completions

* Update src/services/completions.ts

Co-authored-by: Daniel Rosenwasser <[email protected]>

Co-authored-by: Daniel Rosenwasser <[email protected]>

Author: andrewbranch

src/compiler/utilities.ts

@@ -5511,10 +5511,9 @@ namespace ts {
     }
 
     /** Add a value to a set, and return true if it wasn't already present. */
-    export function addToSeen(seen: ESMap<string, true>, key: string | number): boolean;
-    export function addToSeen<T>(seen: ESMap<string, T>, key: string | number, value: T): boolean;
-    export function addToSeen<T>(seen: ESMap<string, T>, key: string | number, value: T = true as any): boolean {
-        key = String(key);
+    export function addToSeen<K>(seen: ESMap<K, true>, key: K): boolean;
+    export function addToSeen<K, T>(seen: ESMap<K, T>, key: K, value: T): boolean;
+    export function addToSeen<K, T>(seen: ESMap<K, T>, key: K, value: T = true as any): boolean {
         if (seen.has(key)) {
             return false;
         }

src/harness/fourslashImpl.ts

@@ -933,7 +933,7 @@ namespace FourSlash {
             assert.equal(actual.sortText, expected.sortText || ts.Completions.SortText.LocationPriority, this.messageAtLastKnownMarker(`Actual entry: ${JSON.stringify(actual)}`));
 
             if (expected.text !== undefined) {
-                const actualDetails = this.getCompletionEntryDetails(actual.name, actual.source)!;
+                const actualDetails = ts.Debug.checkDefined(this.getCompletionEntryDetails(actual.name, actual.source), `No completion details available for name '${actual.name}' and source '${actual.source}'`);
                 assert.equal(ts.displayPartsToString(actualDetails.displayParts), expected.text, "Expected 'text' property to match 'displayParts' string");
                 assert.equal(ts.displayPartsToString(actualDetails.documentation), expected.documentation || "", "Expected 'documentation' property to match 'documentation' display parts string");
                 // TODO: GH#23587

src/services/codefixes/convertToTypeOnlyExport.ts

@@ -12,7 +12,7 @@ namespace ts.codefix {
         },
         fixIds: [fixId],
         getAllCodeActions: context => {
-            const fixedExportDeclarations = new Map<string, true>();
+            const fixedExportDeclarations = new Map<number, true>();
             return codeFixAll(context, errorCodes, (changes, diag) => {
                 const exportSpecifier = getExportSpecifierForDiagnosticSpan(diag, context.sourceFile);
                 if (exportSpecifier && addToSeen(fixedExportDeclarations, getNodeId(exportSpecifier.parent.parent))) {

src/services/codefixes/fixAwaitInSyncFunction.ts

@@ -16,7 +16,7 @@ namespace ts.codefix {
         },
         fixIds: [fixId],
         getAllCodeActions: context => {
-            const seen = new Map<string, true>();
+            const seen = new Map<number, true>();
             return codeFixAll(context, errorCodes, (changes, diag) => {
                 const nodes = getNodes(diag.file, diag.start);
                 if (!nodes || !addToSeen(seen, getNodeId(nodes.insertBefore))) return;

src/services/codefixes/fixClassDoesntImplementInheritedAbstractMember.ts

@@ -15,7 +15,7 @@ namespace ts.codefix {
         },
         fixIds: [fixId],
         getAllCodeActions: context => {
-            const seenClassDeclarations = new Map<string, true>();
+            const seenClassDeclarations = new Map<number, true>();
             return codeFixAll(context, errorCodes, (changes, diag) => {
                 const classDeclaration = getClass(diag.file, diag.start);
                 if (addToSeen(seenClassDeclarations, getNodeId(classDeclaration))) {

src/services/codefixes/fixClassIncorrectlyImplementsInterface.ts

@@ -17,7 +17,7 @@ namespace ts.codefix {
         },
         fixIds: [fixId],
         getAllCodeActions(context) {
-            const seenClassDeclarations = new Map<string, true>();
+            const seenClassDeclarations = new Map<number, true>();
             return codeFixAll(context, errorCodes, (changes, diag) => {
                 const classDeclaration = getClass(diag.file, diag.start);
                 if (addToSeen(seenClassDeclarations, getNodeId(classDeclaration))) {

src/services/codefixes/fixClassSuperMustPrecedeThisAccess.ts

@@ -15,7 +15,7 @@ namespace ts.codefix {
         fixIds: [fixId],
         getAllCodeActions(context) {
             const { sourceFile } = context;
-            const seenClasses = new Map<string, true>(); // Ensure we only do this once per class.
+            const seenClasses = new Map<number, true>(); // Ensure we only do this once per class.
             return codeFixAll(context, errorCodes, (changes, diag) => {
                 const nodes = getNodes(diag.file, diag.start);
                 if (!nodes) return;

src/services/completions.ts

@@ -1645,7 +1645,7 @@ namespace ts.Completions {
         }
 
         /** True if symbol is a type or a module containing at least one type. */
-        function symbolCanBeReferencedAtTypeLocation(symbol: Symbol, seenModules = new Map<string, true>()): boolean {
+        function symbolCanBeReferencedAtTypeLocation(symbol: Symbol, seenModules = new Map<SymbolId, true>()): boolean {
             const sym = skipAlias(symbol.exportSymbol || symbol, typeChecker);
             return !!(sym.flags & SymbolFlags.Type) ||
                 !!(sym.flags & SymbolFlags.Module) &&
@@ -1655,50 +1655,7 @@ namespace ts.Completions {
 
         /**
          * Gathers symbols that can be imported from other files, de-duplicating along the way. Symbols can be "duplicates"
-         * if re-exported from another module, e.g. `export { foo } from "./a"`. That syntax creates a fresh symbol, but
-         * it’s just an alias to the first, and both have the same name, so we generally want to filter those aliases out,
-         * if and only if the the first can be imported (it may be excluded due to package.json filtering in
-         * `codefix.forEachExternalModuleToImportFrom`).
-         *
-         * Example. Imagine a chain of node_modules re-exporting one original symbol:
-         *
-         * ```js
-         *  node_modules/x/index.js         node_modules/y/index.js           node_modules/z/index.js
-         * +-----------------------+      +--------------------------+      +--------------------------+
-         * |                       |      |                          |      |                          |
-         * | export const foo = 0; | <--- | export { foo } from 'x'; | <--- | export { foo } from 'y'; |
-         * |                       |      |                          |      |                          |
-         * +-----------------------+      +--------------------------+      +--------------------------+
-         * ```
-         *
-         * Also imagine three buckets, which we’ll reference soon:
-         *
-         * ```md
-         * |                  |      |                      |      |                   |
-         * |   **Bucket A**   |      |    **Bucket B**      |      |    **Bucket C**   |
-         * |    Symbols to    |      | Aliases to symbols   |      | Symbols to return |
-         * |    definitely    |      | in Buckets A or C    |      | if nothing better |
-         * |      return      |      | (don’t return these) |      |    comes along    |
-         * |__________________|      |______________________|      |___________________|
-         * ```
-         *
-         * We _probably_ want to show `foo` from 'x', but not from 'y' or 'z'. However, if 'x' is not in a package.json, it
-         * will not appear in a `forEachExternalModuleToImportFrom` iteration. Furthermore, the order of iterations is not
-         * guaranteed, as it is host-dependent. Therefore, when presented with the symbol `foo` from module 'y' alone, we
-         * may not be sure whether or not it should go in the list. So, we’ll take the following steps:
-         *
-         * 1. Resolve alias `foo` from 'y' to the export declaration in 'x', get the symbol there, and see if that symbol is
-         *    already in Bucket A (symbols we already know will be returned). If it is, put `foo` from 'y' in Bucket B
-         *    (symbols that are aliases to symbols in Bucket A). If it’s not, put it in Bucket C.
-         * 2. Next, imagine we see `foo` from module 'z'. Again, we resolve the alias to the nearest export, which is in 'y'.
-         *    At this point, if that nearest export from 'y' is in _any_ of the three buckets, we know the symbol in 'z'
-         *    should never be returned in the final list, so put it in Bucket B.
-         * 3. Next, imagine we see `foo` from module 'x', the original. Syntactically, it doesn’t look like a re-export, so
-         *    we can just check Bucket C to see if we put any aliases to the original in there. If they exist, throw them out.
-         *    Put this symbol in Bucket A.
-         * 4. After we’ve iterated through every symbol of every module, any symbol left in Bucket C means that step 3 didn’t
-         *    occur for that symbol---that is, the original symbol is not in Bucket A, so we should include the alias. Move
-         *    everything from Bucket C to Bucket A.
+         * if re-exported from another module by the same name, e.g. `export { foo } from "./a"`.
          */
         function getSymbolsFromOtherSourceFileExports(target: ScriptTarget, host: LanguageServiceHost): readonly AutoImportSuggestion[] {
             const cached = importSuggestionsCache && importSuggestionsCache.get(
@@ -1713,16 +1670,8 @@ namespace ts.Completions {
 
             const startTime = timestamp();
             log(`getSymbolsFromOtherSourceFileExports: Recomputing list${detailsEntryId ? " for details entry" : ""}`);
-            const seenResolvedModules = new Map<string, true>();
-            const seenExports = new Map<string, true>();
-            /** Bucket B */
-            const aliasesToAlreadyIncludedSymbols = new Map<string, true>();
-            /** Bucket C */
-            const aliasesToReturnIfOriginalsAreMissing = new Map<string, { alias: Symbol, moduleSymbol: Symbol, isFromPackageJson: boolean }>();
-            /** Bucket A */
-            const results: AutoImportSuggestion[] = [];
-            /** Ids present in `results` for faster lookup */
-            const resultSymbolIds = new Map<string, true>();
+            const seenResolvedModules = new Map<SymbolId, true>();
+            const results = createMultiMap<SymbolId, AutoImportSuggestion>();
 
             codefix.forEachExternalModuleToImportFrom(program, host, sourceFile, !detailsEntryId, /*useAutoImportProvider*/ true, (moduleSymbol, _, program, isFromPackageJson) => {
                 // Perf -- ignore other modules if this is a request for details
@@ -1744,73 +1693,52 @@ namespace ts.Completions {
                 }
 
                 for (const symbol of typeChecker.getExportsAndPropertiesOfModule(moduleSymbol)) {
-                    const symbolId = getSymbolId(symbol).toString();
-                    // `getExportsAndPropertiesOfModule` can include duplicates
-                    if (!addToSeen(seenExports, symbolId)) {
-                        continue;
-                    }
                     // If this is `export { _break as break };` (a keyword) -- skip this and prefer the keyword completion.
                     if (some(symbol.declarations, d => isExportSpecifier(d) && !!d.propertyName && isIdentifierANonContextualKeyword(d.name))) {
                         continue;
                     }
 
-                    // If `symbol.parent !== moduleSymbol`, this is an `export * from "foo"` re-export. Those don't create new symbols.
-                    const isExportStarFromReExport = typeChecker.getMergedSymbol(symbol.parent!) !== resolvedModuleSymbol;
-                    // If `!!d.parent.parent.moduleSpecifier`, this is `export { foo } from "foo"` re-export, which creates a new symbol (thus isn't caught by the first check).
-                    if (isExportStarFromReExport || some(symbol.declarations, d => isExportSpecifier(d) && !d.propertyName && !!d.parent.parent.moduleSpecifier)) {
-                        // Walk the export chain back one module (step 1 or 2 in diagrammed example).
-                        // Or, in the case of `export * from "foo"`, `symbol` already points to the original export, so just use that.
-                        const nearestExportSymbol = isExportStarFromReExport ? symbol : getNearestExportSymbol(symbol);
-                        if (!nearestExportSymbol) continue;
-                        const nearestExportSymbolId = getSymbolId(nearestExportSymbol).toString();
-                        const symbolHasBeenSeen = resultSymbolIds.has(nearestExportSymbolId) || aliasesToAlreadyIncludedSymbols.has(nearestExportSymbolId);
-                        if (!symbolHasBeenSeen) {
-                            aliasesToReturnIfOriginalsAreMissing.set(nearestExportSymbolId, { alias: symbol, moduleSymbol, isFromPackageJson });
-                            aliasesToAlreadyIncludedSymbols.set(symbolId, true);
-                        }
-                        else {
-                            // Perf - we know this symbol is an alias to one that’s already covered in `symbols`, so store it here
-                            // in case another symbol re-exports this one; that way we can short-circuit as soon as we see this symbol id.
-                            addToSeen(aliasesToAlreadyIncludedSymbols, symbolId);
-                        }
-                    }
-                    else {
-                        // This is not a re-export, so see if we have any aliases pending and remove them (step 3 in diagrammed example)
-                        aliasesToReturnIfOriginalsAreMissing.delete(symbolId);
-                        pushSymbol(symbol, moduleSymbol, isFromPackageJson, /*skipFilter*/ false);
-                    }
+                    pushSymbol(symbol, moduleSymbol, isFromPackageJson, /*skipFilter*/ false);
                 }
             });
 
-            // By this point, any potential duplicates that were actually duplicates have been
-            // removed, so the rest need to be added. (Step 4 in diagrammed example)
-            aliasesToReturnIfOriginalsAreMissing.forEach(({ alias, moduleSymbol, isFromPackageJson }) => pushSymbol(alias, moduleSymbol, isFromPackageJson, /*skipFilter*/ false));
             log(`getSymbolsFromOtherSourceFileExports: ${timestamp() - startTime}`);
-            return results;
+            return flatten(arrayFrom(results.values()));
 
             function pushSymbol(symbol: Symbol, moduleSymbol: Symbol, isFromPackageJson: boolean, skipFilter: boolean) {
                 const isDefaultExport = symbol.escapedName === InternalSymbolName.Default;
+                const nonLocalSymbol = symbol;
                 if (isDefaultExport) {
                     symbol = getLocalSymbolForExportDefault(symbol) || symbol;
                 }
                 if (typeChecker.isUndefinedSymbol(symbol)) {
                     return;
                 }
-                addToSeen(resultSymbolIds, getSymbolId(symbol));
-                const origin: SymbolOriginInfoExport = { kind: SymbolOriginInfoKind.Export, moduleSymbol, isDefaultExport, isFromPackageJson };
-                results.push({
-                    symbol,
-                    symbolName: getNameForExportedSymbol(symbol, target),
-                    origin,
-                    skipFilter,
-                });
+                const original = skipAlias(nonLocalSymbol, typeChecker);
+                const symbolName = getNameForExportedSymbol(symbol, target);
+                const existingSuggestions = results.get(getSymbolId(original));
+                if (!some(existingSuggestions, s => s.symbolName === symbolName && moduleSymbolsAreDuplicateOrigins(moduleSymbol, s.origin.moduleSymbol))) {
+                    const origin: SymbolOriginInfoExport = { kind: SymbolOriginInfoKind.Export, moduleSymbol, isDefaultExport, isFromPackageJson };
+                    results.add(getSymbolId(original), {
+                        symbol,
+                        symbolName,
+                        origin,
+                        skipFilter,
+                    });
+                }
             }
         }
 
-        function getNearestExportSymbol(fromSymbol: Symbol) {
-            return findAlias(typeChecker, fromSymbol, alias => {
-                return some(alias.declarations, d => isExportSpecifier(d) || !!d.localSymbol);
-            });
+        /**
+         * Determines whether a module symbol is redundant with another for purposes of offering
+         * auto-import completions for exports of the same symbol. Exports of the same symbol
+         * will not be offered from different external modules, but they will be offered from
+         * different ambient modules.
+         */
+        function moduleSymbolsAreDuplicateOrigins(a: Symbol, b: Symbol) {
+            const ambientNameA = pathIsBareSpecifier(stripQuotes(a.name)) ? a.name : undefined;
+            const ambientNameB = pathIsBareSpecifier(stripQuotes(b.name)) ? b.name : undefined;
+            return ambientNameA === ambientNameB;
         }
 
         /**
@@ -2897,15 +2825,6 @@ namespace ts.Completions {
         return nodeIsMissing(left);
     }
 
-    function findAlias(typeChecker: TypeChecker, symbol: Symbol, predicate: (symbol: Symbol) => boolean): Symbol | undefined {
-        let currentAlias: Symbol | undefined = symbol;
-        while (currentAlias.flags & SymbolFlags.Alias && (currentAlias = typeChecker.getImmediateAliasedSymbol(currentAlias))) {
-            if (predicate(currentAlias)) {
-                return currentAlias;
-            }
-        }
-    }
-
     /** Determines if a type is exactly the same type resolved by the global 'self', 'global', or 'globalThis'. */
     function isProbablyGlobalType(type: Type, sourceFile: SourceFile, checker: TypeChecker) {
         // The type of `self` and `window` is the same in lib.dom.d.ts, but `window` does not exist in

src/services/findAllReferences.ts

@@ -231,7 +231,7 @@ namespace ts.FindAllReferences {
         }
         else {
             const queue = entries && [...entries];
-            const seenNodes = new Map<string, true>();
+            const seenNodes = new Map<number, true>();
             while (queue && queue.length) {
                 const entry = queue.shift() as NodeEntry;
                 if (!addToSeen(seenNodes, getNodeId(entry.node))) {
@@ -2154,7 +2154,7 @@ namespace ts.FindAllReferences {
          *                                The value of previousIterationSymbol is undefined when the function is first called.
          */
         function getPropertySymbolsFromBaseTypes<T>(symbol: Symbol, propertyName: string, checker: TypeChecker, cb: (symbol: Symbol) => T | undefined): T | undefined {
-            const seen = new Map<string, true>();
+            const seen = new Map<SymbolId, true>();
             return recur(symbol);
 
             function recur(symbol: Symbol): T | undefined {

src/services/textChanges.ts

@@ -261,7 +261,7 @@ namespace ts.textChanges {
     export class ChangeTracker {
         private readonly changes: Change[] = [];
         private readonly newFiles: { readonly oldFile: SourceFile | undefined, readonly fileName: string, readonly statements: readonly (Statement | SyntaxKind.NewLineTrivia)[] }[] = [];
-        private readonly classesWithNodesInsertedAtStart = new Map<string, { readonly node: ClassDeclaration | InterfaceDeclaration | ObjectLiteralExpression, readonly sourceFile: SourceFile }>(); // Set<ClassDeclaration> implemented as Map<node id, ClassDeclaration>
+        private readonly classesWithNodesInsertedAtStart = new Map<number, { readonly node: ClassDeclaration | InterfaceDeclaration | ObjectLiteralExpression, readonly sourceFile: SourceFile }>(); // Set<ClassDeclaration> implemented as Map<node id, ClassDeclaration>
         private readonly deletedNodes: { readonly sourceFile: SourceFile, readonly node: Node | NodeArray<TypeParameterDeclaration> }[] = [];
 
         public static fromContext(context: TextChangesContext): ChangeTracker {