Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 22, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 48 additions & 20 deletions src/services/codefixes/importFixes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
}
Expand Down Expand Up @@ -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(
Expand All @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 };
}

Expand Down
20 changes: 13 additions & 7 deletions src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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();
Expand All @@ -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++;
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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)) {
Expand All @@ -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 = {
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -2707,8 +2711,10 @@ namespace ts.Completions {
host,
program,
sourceFile,
position,
preferences,
!!importCompletionNode,
isValidTypeOnlyAliasUseSite(location),
context => {
exportInfo.search(
sourceFile.path,
Expand Down Expand Up @@ -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;

Expand Down
31 changes: 31 additions & 0 deletions tests/cases/fourslash/completionsImport_preferUpdatingExistingImport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
},
});