Fix auto-import completions sometimes not updating existing imports (microsoft#48815)

andrewbranch · web-flow · commit c99380f87bbd · 2022-04-22T15:32:52.000-07:00
diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts
@@ -83,7 +83,7 @@ namespace ts.codefix {
             const symbol = checker.getMergedSymbol(skipAlias(exportedSymbol, checker));
             const exportInfo = getAllReExportingModules(sourceFile, symbol, moduleSymbol, symbolName, /*isJsxTagName*/ false, host, program, preferences, useAutoImportProvider);
             const useRequire = shouldUseRequire(sourceFile, program);
-            const fix = getImportFixForSymbol(sourceFile, exportInfo, moduleSymbol, symbolName, program, /*position*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences);
+            const fix = getImportFixForSymbol(sourceFile, exportInfo, moduleSymbol, program, /*useNamespaceInfo*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences);
             if (fix) {
                 addImport({ fixes: [fix], symbolName, errorIdentifierText: undefined });
             }
@@ -310,7 +310,7 @@ namespace ts.codefix {
             : getAllReExportingModules(sourceFile, targetSymbol, moduleSymbol, symbolName, isJsxTagName, host, program, preferences, /*useAutoImportProvider*/ true);
         const useRequire = shouldUseRequire(sourceFile, program);
         const isValidTypeOnlyUseSite = isValidTypeOnlyAliasUseSite(getTokenAtPosition(sourceFile, position));
-        const fix = Debug.checkDefined(getImportFixForSymbol(sourceFile, exportInfos, moduleSymbol, symbolName, program, position, isValidTypeOnlyUseSite, useRequire, host, preferences));
+        const fix = Debug.checkDefined(getImportFixForSymbol(sourceFile, exportInfos, moduleSymbol, program, { symbolName, position }, isValidTypeOnlyUseSite, useRequire, host, preferences));
         return {
             moduleSpecifier: fix.moduleSpecifier,
             codeAction: codeFixActionToCodeAction(codeActionForFix(
@@ -331,10 +331,10 @@ namespace ts.codefix {
         return fix && codeFixActionToCodeAction(codeActionForFix({ host, formatContext, preferences }, sourceFile, symbolName, fix, includeSymbolNameInDescription, QuotePreference.Double, compilerOptions));
     }
 
-    function getImportFixForSymbol(sourceFile: SourceFile, exportInfos: readonly SymbolExportInfo[], moduleSymbol: Symbol, symbolName: string, program: Program, position: number | undefined, isValidTypeOnlyUseSite: boolean, useRequire: boolean, host: LanguageServiceHost, preferences: UserPreferences) {
+    function getImportFixForSymbol(sourceFile: SourceFile, exportInfos: readonly SymbolExportInfo[], moduleSymbol: Symbol, program: Program, useNamespaceInfo: { position: number, symbolName: string } | undefined, isValidTypeOnlyUseSite: boolean, useRequire: boolean, host: LanguageServiceHost, preferences: UserPreferences) {
         Debug.assert(exportInfos.some(info => info.moduleSymbol === moduleSymbol || info.symbol.parent === moduleSymbol), "Some exportInfo should match the specified moduleSymbol");
         const packageJsonImportFilter = createPackageJsonImportFilter(sourceFile, preferences, host);
-        return getBestFix(getImportFixes(exportInfos, symbolName, position, isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences), sourceFile, program, packageJsonImportFilter, host);
+        return getBestFix(getImportFixes(exportInfos, useNamespaceInfo, isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences).fixes, sourceFile, program, packageJsonImportFilter, host);
     }
 
     function codeFixActionToCodeAction({ description, changes, commands }: CodeFixAction): CodeAction {
@@ -396,20 +396,23 @@ namespace ts.codefix {
 
     export function getModuleSpecifierForBestExportInfo(
         exportInfo: readonly SymbolExportInfo[],
+        symbolName: string,
+        position: number,
+        isValidTypeOnlyUseSite: boolean,
         importingFile: SourceFile,
         program: Program,
         host: LanguageServiceHost,
         preferences: UserPreferences,
         packageJsonImportFilter?: PackageJsonImportFilter,
         fromCacheOnly?: boolean,
     ): { exportInfo?: SymbolExportInfo, moduleSpecifier: string, computedWithoutCacheCount: number } | undefined {
-        const { fixes, computedWithoutCacheCount } = getNewImportFixes(
+        const { fixes, computedWithoutCacheCount } = getImportFixes(
+            exportInfo,
+            { symbolName, position },
+            isValidTypeOnlyUseSite,
+            /*useRequire*/ false,
             program,
             importingFile,
-            /*position*/ undefined,
-            /*isValidTypeOnlyUseSite*/ false,
-            /*useRequire*/ false,
-            exportInfo,
             host,
             preferences,
             fromCacheOnly);
@@ -419,23 +422,46 @@ namespace ts.codefix {
 
     function getImportFixes(
         exportInfos: readonly SymbolExportInfo[],
-        symbolName: string,
+        useNamespaceInfo: {
+            symbolName: string,
+            position: number,
+        } | undefined,
         /** undefined only for missing JSX namespace */
-        position: number | undefined,
         isValidTypeOnlyUseSite: boolean,
         useRequire: boolean,
         program: Program,
         sourceFile: SourceFile,
         host: LanguageServiceHost,
         preferences: UserPreferences,
-    ): readonly ImportFixWithModuleSpecifier[] {
+        fromCacheOnly?: boolean,
+    ): { computedWithoutCacheCount: number, fixes: readonly ImportFixWithModuleSpecifier[] } {
         const checker = program.getTypeChecker();
         const existingImports = flatMap(exportInfos, info => getExistingImportDeclarations(info, checker, sourceFile, program.getCompilerOptions()));
-        const useNamespace = position === undefined ? undefined : tryUseExistingNamespaceImport(existingImports, symbolName, position, checker);
+        const useNamespace = useNamespaceInfo && tryUseExistingNamespaceImport(existingImports, useNamespaceInfo.symbolName, useNamespaceInfo.position, checker);
         const addToExisting = tryAddToExistingImport(existingImports, isValidTypeOnlyUseSite, checker, program.getCompilerOptions());
-        // Don't bother providing an action to add a new import if we can add to an existing one.
-        const addImport = addToExisting ? [addToExisting] : getFixesForAddImport(exportInfos, existingImports, program, sourceFile, position, isValidTypeOnlyUseSite, useRequire, host, preferences);
-        return [...(useNamespace ? [useNamespace] : emptyArray), ...addImport];
+        if (addToExisting) {
+            // Don't bother providing an action to add a new import if we can add to an existing one.
+            return {
+                computedWithoutCacheCount: 0,
+                fixes: [...(useNamespace ? [useNamespace] : emptyArray), addToExisting],
+            };
+        }
+
+        const { fixes, computedWithoutCacheCount = 0 } = getFixesForAddImport(
+            exportInfos,
+            existingImports,
+            program,
+            sourceFile,
+            useNamespaceInfo?.position,
+            isValidTypeOnlyUseSite,
+            useRequire,
+            host,
+            preferences,
+            fromCacheOnly);
+        return {
+            computedWithoutCacheCount,
+            fixes: [...(useNamespace ? [useNamespace] : emptyArray), ...fixes],
+        };
     }
 
     function tryUseExistingNamespaceImport(existingImports: readonly FixAddToExistingImportInfo[], symbolName: string, position: number, checker: TypeChecker): FixUseNamespaceImport | undefined {
@@ -658,9 +684,10 @@ namespace ts.codefix {
         useRequire: boolean,
         host: LanguageServiceHost,
         preferences: UserPreferences,
-    ): readonly (FixAddNewImport | FixAddJsdocTypeImport)[] {
+        fromCacheOnly?: boolean,
+    ): { computedWithoutCacheCount?: number, fixes: readonly (FixAddNewImport | FixAddJsdocTypeImport)[] } {
         const existingDeclaration = firstDefined(existingImports, info => newImportInfoFromExistingSpecifier(info, isValidTypeOnlyUseSite, useRequire, program.getTypeChecker(), program.getCompilerOptions()));
-        return existingDeclaration ? [existingDeclaration] : getNewImportFixes(program, sourceFile, position, isValidTypeOnlyUseSite, useRequire, exportInfos, host, preferences).fixes;
+        return existingDeclaration ? { fixes: [existingDeclaration] } : getNewImportFixes(program, sourceFile, position, isValidTypeOnlyUseSite, useRequire, exportInfos, host, preferences, fromCacheOnly);
     }
 
     function newImportInfoFromExistingSpecifier(
@@ -782,7 +809,8 @@ namespace ts.codefix {
         const symbolName = umdSymbol.name;
         const exportInfo: readonly SymbolExportInfo[] = [{ symbol: umdSymbol, moduleSymbol: symbol, moduleFileName: undefined, exportKind: ExportKind.UMD, targetFlags: symbol.flags, isFromPackageJson: false }];
         const useRequire = shouldUseRequire(sourceFile, program);
-        const fixes = getImportFixes(exportInfo, symbolName, isIdentifier(token) ? token.getStart(sourceFile) : undefined, /*isValidTypeOnlyUseSite*/ false, useRequire, program, sourceFile, host, preferences);
+        const position = isIdentifier(token) ? token.getStart(sourceFile) : undefined;
+        const fixes = getImportFixes(exportInfo, position ? { position, symbolName } : undefined, /*isValidTypeOnlyUseSite*/ false, useRequire, program, sourceFile, host, preferences).fixes;
         return { fixes, symbolName, errorIdentifierText: tryCast(token, isIdentifier)?.text };
     }
     function getUmdSymbol(token: Node, checker: TypeChecker): Symbol | undefined {
@@ -855,7 +883,7 @@ namespace ts.codefix {
         const useRequire = shouldUseRequire(sourceFile, program);
         const exportInfo = getExportInfos(symbolName, isJSXTagName(symbolToken), getMeaningFromLocation(symbolToken), cancellationToken, sourceFile, program, useAutoImportProvider, host, preferences);
         const fixes = arrayFrom(flatMapIterator(exportInfo.entries(), ([_, exportInfos]) =>
-            getImportFixes(exportInfos, symbolName, symbolToken.getStart(sourceFile), isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences)));
+            getImportFixes(exportInfos, { symbolName, position: symbolToken.getStart(sourceFile) }, isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences).fixes));
         return { fixes, symbolName, errorIdentifierText: symbolToken.text };
     }
 
diff --git a/src/services/completions.ts b/src/services/completions.ts
@@ -170,7 +170,7 @@ namespace ts.Completions {
     const enum GlobalsSearch { Continue, Success, Fail }
 
     interface ModuleSpecifierResolutioContext {
-        tryResolve: (exportInfo: readonly SymbolExportInfo[], isFromAmbientModule: boolean) => ModuleSpecifierResolutionResult | undefined;
+        tryResolve: (exportInfo: readonly SymbolExportInfo[], symbolName: string, isFromAmbientModule: boolean) => ModuleSpecifierResolutionResult | undefined;
         resolutionLimitExceeded: () => boolean;
     }
 
@@ -184,8 +184,10 @@ namespace ts.Completions {
         host: LanguageServiceHost,
         program: Program,
         sourceFile: SourceFile,
+        position: number,
         preferences: UserPreferences,
         isForImportStatementCompletion: boolean,
+        isValidTypeOnlyUseSite: boolean,
         cb: (context: ModuleSpecifierResolutioContext) => TReturn,
     ): TReturn {
         const start = timestamp();
@@ -204,9 +206,9 @@ namespace ts.Completions {
         host.log?.(`${logPrefix}: ${timestamp() - start}`);
         return result;
 
-        function tryResolve(exportInfo: readonly SymbolExportInfo[], isFromAmbientModule: boolean): ModuleSpecifierResolutionResult | undefined {
+        function tryResolve(exportInfo: readonly SymbolExportInfo[], symbolName: string, isFromAmbientModule: boolean): ModuleSpecifierResolutionResult | undefined {
             if (isFromAmbientModule) {
-                const result = codefix.getModuleSpecifierForBestExportInfo(exportInfo, sourceFile, program, host, preferences);
+                const result = codefix.getModuleSpecifierForBestExportInfo(exportInfo, symbolName, position, isValidTypeOnlyUseSite, sourceFile, program, host, preferences);
                 if (result) {
                     ambientCount++;
                 }
@@ -215,7 +217,7 @@ namespace ts.Completions {
             const shouldResolveModuleSpecifier = isForImportStatementCompletion || preferences.allowIncompleteCompletions && resolvedCount < moduleSpecifierResolutionLimit;
             const shouldGetModuleSpecifierFromCache = !shouldResolveModuleSpecifier && preferences.allowIncompleteCompletions && cacheAttemptCount < moduleSpecifierResolutionCacheAttemptLimit;
             const result = (shouldResolveModuleSpecifier || shouldGetModuleSpecifierFromCache)
-                ? codefix.getModuleSpecifierForBestExportInfo(exportInfo, sourceFile, program, host, preferences, packageJsonImportFilter, shouldGetModuleSpecifierFromCache)
+                ? codefix.getModuleSpecifierForBestExportInfo(exportInfo, symbolName, position, isValidTypeOnlyUseSite, sourceFile, program, host, preferences, packageJsonImportFilter, shouldGetModuleSpecifierFromCache)
                 : undefined;
 
             if (!shouldResolveModuleSpecifier && !shouldGetModuleSpecifierFromCache || shouldGetModuleSpecifierFromCache && !result) {
@@ -358,8 +360,10 @@ namespace ts.Completions {
             host,
             program,
             file,
+            location.getStart(),
             preferences,
             /*isForImportStatementCompletion*/ false,
+            isValidTypeOnlyAliasUseSite(location),
             context => {
                 const entries = mapDefined(previousResponse.entries, entry => {
                     if (!entry.hasAction || !entry.source || !entry.data || completionEntryDataIsResolved(entry.data)) {
@@ -374,7 +378,7 @@ namespace ts.Completions {
                     const { origin } = Debug.checkDefined(getAutoImportSymbolFromCompletionEntryData(entry.name, entry.data, program, host));
                     const info = exportMap.get(file.path, entry.data.exportMapKey);
 
-                    const result = info && context.tryResolve(info, !isExternalModuleNameRelative(stripQuotes(origin.moduleSymbol.name)));
+                    const result = info && context.tryResolve(info, entry.name, !isExternalModuleNameRelative(stripQuotes(origin.moduleSymbol.name)));
                     if (!result) return entry;
 
                     const newOrigin: SymbolOriginInfoResolvedExport = {
@@ -2428,7 +2432,7 @@ namespace ts.Completions {
                             moduleSymbol,
                             symbol: firstAccessibleSymbol,
                             targetFlags: skipAlias(firstAccessibleSymbol, typeChecker).flags,
-                        }], sourceFile, program, host, preferences) || {};
+                        }], firstAccessibleSymbol.name, position, isValidTypeOnlyAliasUseSite(location), sourceFile, program, host, preferences) || {};
 
                         if (moduleSpecifier) {
                             const origin: SymbolOriginInfoResolvedExport = {
@@ -2707,8 +2711,10 @@ namespace ts.Completions {
                 host,
                 program,
                 sourceFile,
+                position,
                 preferences,
                 !!importCompletionNode,
+                isValidTypeOnlyAliasUseSite(location),
                 context => {
                     exportInfo.search(
                         sourceFile.path,
@@ -2737,7 +2743,7 @@ namespace ts.Completions {
 
                             // If we don't need to resolve module specifiers, we can use any re-export that is importable at all
                             // (We need to ensure that at least one is importable to show a completion.)
-                            const { exportInfo = defaultExportInfo, moduleSpecifier } = context.tryResolve(info, isFromAmbientModule) || {};
+                            const { exportInfo = defaultExportInfo, moduleSpecifier } = context.tryResolve(info, symbolName, isFromAmbientModule) || {};
                             const isDefaultExport = exportInfo.exportKind === ExportKind.Default;
                             const symbol = isDefaultExport && getLocalSymbolForExportDefault(exportInfo.symbol) || exportInfo.symbol;
 
diff --git a/tests/cases/fourslash/completionsImport_preferUpdatingExistingImport.ts b/tests/cases/fourslash/completionsImport_preferUpdatingExistingImport.ts
@@ -0,0 +1,31 @@
+/// <reference path="fourslash.ts" />
+
+// @module: commonjs
+
+// @Filename: /deep/module/why/you/want/this/path.ts
+//// export const x = 0;
+//// export const y = 1;
+
+// @Filename: /nice/reexport.ts
+//// import { x, y } from "../deep/module/why/you/want/this/path";
+//// export { x, y };
+
+// @Filename: /index.ts
+//// import { x } from "./deep/module/why/you/want/this/path";
+////
+//// y/**/
+
+verify.completions({
+  marker: "",
+  exact: completion.globalsPlus(["x", {
+    name: "y",
+    source: "./deep/module/why/you/want/this/path",
+    sourceDisplay: "./deep/module/why/you/want/this/path",
+    sortText: completion.SortText.AutoImportSuggestions,
+    hasAction: true,
+  }]),
+  preferences: {
+    includeCompletionsForModuleExports: true,
+    allowIncompleteCompletions: true,
+  },
+});
