Skip to content

Auto-imports: fix some exports being incorrectly stored as re-exports of others due to key conflict #45792

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 8 commits into from
Oct 8, 2021
Merged

Auto-imports: fix some exports being incorrectly stored as re-exports of others due to key conflict #45792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ba90719
Ensure symbol key unique when target is a local symbol exported elsew…
andrewbranch Sep 9, 2021
00e2c65
Add test
andrewbranch Sep 9, 2021
106e1a9
Support targets without declarations
andrewbranch Sep 9, 2021
20676c7
Best key yet
andrewbranch Sep 9, 2021
fef7e8c
A-ha moment
andrewbranch Oct 8, 2021
6d35017
Clean up types
andrewbranch Oct 8, 2021
dcd0ed8
Update API
andrewbranch Oct 8, 2021
c4283e3
Update unit test
andrewbranch Oct 8, 2021
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
128 changes: 86 additions & 42 deletions src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,23 +74,23 @@ namespace ts.Completions {

interface SymbolOriginInfo {
kind: SymbolOriginInfoKind;
symbolName?: string;
moduleSymbol?: Symbol;
isDefaultExport?: boolean;
isFromPackageJson?: boolean;
exportName?: string;
fileName?: string;
moduleSpecifier?: string;
}

interface SymbolOriginInfoExport extends SymbolOriginInfo {
symbolName: string;
moduleSymbol: Symbol;
isDefaultExport: boolean;
exportName: string;
exportMapKey: string;
}

interface SymbolOriginInfoResolvedExport extends SymbolOriginInfoExport {
interface SymbolOriginInfoResolvedExport extends SymbolOriginInfo {
symbolName: string;
moduleSymbol: Symbol;
exportName: string;
moduleSpecifier: string;
}

Expand Down Expand Up @@ -294,6 +294,10 @@ namespace ts.Completions {
}
}

function completionEntryDataIsResolved(data: CompletionEntryDataAutoImport | undefined): data is CompletionEntryDataResolved {
return !!data?.moduleSpecifier;
}

function continuePreviousIncompleteResponse(
cache: IncompleteCompletionsCache,
file: SourceFile,
Expand All @@ -308,9 +312,6 @@ namespace ts.Completions {

const lowerCaseTokenText = location.text.toLowerCase();
const exportMap = getExportInfoMap(file, host, program, cancellationToken);
const checker = program.getTypeChecker();
const autoImportProvider = host.getPackageJsonAutoImportProvider?.();
const autoImportProviderChecker = autoImportProvider?.getTypeChecker();
const newEntries = resolvingModuleSpecifiers(
"continuePreviousIncompleteResponse",
host,
Expand All @@ -320,7 +321,7 @@ namespace ts.Completions {
/*isForImportStatementCompletion*/ false,
context => {
const entries = mapDefined(previousResponse.entries, entry => {
if (!entry.hasAction || !entry.source || !entry.data || entry.data.moduleSpecifier) {
if (!entry.hasAction || !entry.source || !entry.data || completionEntryDataIsResolved(entry.data)) {
// Not an auto import or already resolved; keep as is
return entry;
}
Expand All @@ -329,13 +330,8 @@ namespace ts.Completions {
return undefined;
}

const { symbol, origin } = Debug.checkDefined(getAutoImportSymbolFromCompletionEntryData(entry.name, entry.data, program, host));
const info = exportMap.get(
file.path,
entry.name,
symbol,
origin.moduleSymbol.name,
origin.isFromPackageJson ? autoImportProviderChecker! : checker);
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)));
if (!result) return entry;
Expand Down Expand Up @@ -760,14 +756,56 @@ namespace ts.Completions {
return text.replace(/\$/gm, "\\$");
}

function originToCompletionEntryData(origin: SymbolOriginInfoExport): CompletionEntryData | undefined {
return {
function originToCompletionEntryData(origin: SymbolOriginInfoExport | SymbolOriginInfoResolvedExport): CompletionEntryData | undefined {
const ambientModuleName = origin.fileName ? undefined : stripQuotes(origin.moduleSymbol.name);
const isPackageJsonImport = origin.isFromPackageJson ? true : undefined;
if (originIsResolvedExport(origin)) {
const resolvedData: CompletionEntryDataResolved = {
exportName: origin.exportName,
moduleSpecifier: origin.moduleSpecifier,
ambientModuleName,
fileName: origin.fileName,
isPackageJsonImport,
};
return resolvedData;
}
const unresolvedData: CompletionEntryDataUnresolved = {
exportName: origin.exportName,
exportMapKey: origin.exportMapKey,
fileName: origin.fileName,
ambientModuleName: origin.fileName ? undefined : stripQuotes(origin.moduleSymbol.name),
isPackageJsonImport: origin.isFromPackageJson ? true : undefined,
moduleSpecifier: originIsResolvedExport(origin) ? origin.moduleSpecifier : undefined,
};
return unresolvedData;
}

function completionEntryDataToSymbolOriginInfo(data: CompletionEntryData, completionName: string, moduleSymbol: Symbol): SymbolOriginInfoExport | SymbolOriginInfoResolvedExport {
const isDefaultExport = data.exportName === InternalSymbolName.Default;
const isFromPackageJson = !!data.isPackageJsonImport;
if (completionEntryDataIsResolved(data)) {
const resolvedOrigin: SymbolOriginInfoResolvedExport = {
kind: SymbolOriginInfoKind.ResolvedExport,
exportName: data.exportName,
moduleSpecifier: data.moduleSpecifier,
symbolName: completionName,
fileName: data.fileName,
moduleSymbol,
isDefaultExport,
isFromPackageJson,
};
return resolvedOrigin;
}
const unresolvedOrigin: SymbolOriginInfoExport = {
kind: SymbolOriginInfoKind.Export,
exportName: data.exportName,
exportMapKey: data.exportMapKey,
symbolName: completionName,
fileName: data.fileName,
moduleSymbol,
isDefaultExport,
isFromPackageJson,
};
return unresolvedOrigin;
}

function getInsertTextAndReplacementSpanForImportCompletion(name: string, importCompletionNode: Node, contextToken: Node | undefined, origin: SymbolOriginInfoResolvedExport, useSemicolons: boolean, options: CompilerOptions, preferences: UserPreferences) {
Expand Down Expand Up @@ -1762,19 +1800,35 @@ namespace ts.Completions {
const index = symbols.length;
symbols.push(firstAccessibleSymbol);
const moduleSymbol = firstAccessibleSymbol.parent;
if (!moduleSymbol || !isExternalModuleSymbol(moduleSymbol)) {
if (!moduleSymbol ||
!isExternalModuleSymbol(moduleSymbol) ||
typeChecker.tryGetMemberInModuleExportsAndProperties(firstAccessibleSymbol.name, moduleSymbol) !== firstAccessibleSymbol
) {
symbolToOriginInfoMap[index] = { kind: getNullableSymbolOriginInfoKind(SymbolOriginInfoKind.SymbolMemberNoExport) };
}
else {
const origin: SymbolOriginInfoExport = {
kind: getNullableSymbolOriginInfoKind(SymbolOriginInfoKind.SymbolMemberExport),
const fileName = isExternalModuleNameRelative(stripQuotes(moduleSymbol.name)) ? getSourceFileOfModule(moduleSymbol)?.fileName : undefined;
const { moduleSpecifier } = codefix.getModuleSpecifierForBestExportInfo([{
exportKind: ExportKind.Named,
moduleFileName: fileName,
isFromPackageJson: false,
moduleSymbol,
isDefaultExport: false,
symbolName: firstAccessibleSymbol.name,
exportName: firstAccessibleSymbol.name,
fileName: isExternalModuleNameRelative(stripQuotes(moduleSymbol.name)) ? cast(moduleSymbol.valueDeclaration, isSourceFile).fileName : undefined,
};
symbolToOriginInfoMap[index] = origin;
symbol: firstAccessibleSymbol,
targetFlags: skipAlias(firstAccessibleSymbol, typeChecker).flags,
}], sourceFile, program, host, preferences) || {};

if (moduleSpecifier) {
const origin: SymbolOriginInfoResolvedExport = {
kind: getNullableSymbolOriginInfoKind(SymbolOriginInfoKind.SymbolMemberExport),
moduleSymbol,
isDefaultExport: false,
symbolName: firstAccessibleSymbol.name,
exportName: firstAccessibleSymbol.name,
fileName,
moduleSpecifier,
};
symbolToOriginInfoMap[index] = origin;
}
Comment on lines -1769 to +1831
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the larger set of changes in the PR, but it’s kind of a drive-by fix for a probable ancient bug that the real change, the addition of exportMapKey to SymbolOriginInfoExport, forced me to consider. This has always been a weird ad-hoc auto-import for when you need to import a symbol in order to do a property completion, as shown in this test.

I think there have likely been cases where fetching the actual auto-import (in completion details) for this completion would have failed due to some oversimplified assumptions here. I’m now guarding against that a bit more by resolving its module specifier up front and ensuring that the symbol can definitely be re-retrieved by looking for its name in the exports of the module. I could have sort of ignored this problem, but it felt better to deal with it.

}
}
else if (preferences.includeCompletionsWithInsertText) {
Expand Down Expand Up @@ -2034,7 +2088,7 @@ namespace ts.Completions {
preferences,
!!importCompletionNode,
context => {
exportInfo.forEach(sourceFile.path, (info, symbolName, isFromAmbientModule) => {
exportInfo.forEach(sourceFile.path, (info, symbolName, isFromAmbientModule, exportMapKey) => {
if (!isIdentifierText(symbolName, getEmitScriptTarget(host.getCompilationSettings()))) return;
if (!detailsEntryId && isStringANonContextualKeyword(symbolName)) return;
// `targetFlags` should be the same for each `info`
Expand All @@ -2056,6 +2110,7 @@ namespace ts.Completions {
kind: moduleSpecifier ? SymbolOriginInfoKind.ResolvedExport : SymbolOriginInfoKind.Export,
moduleSpecifier,
symbolName,
exportMapKey,
exportName: exportInfo.exportKind === ExportKind.ExportEquals ? InternalSymbolName.ExportEquals : exportInfo.symbol.name,
fileName: exportInfo.moduleFileName,
isDefaultExport,
Expand Down Expand Up @@ -2974,7 +3029,7 @@ namespace ts.Completions {
return { contextToken: previousToken as Node, previousToken: previousToken as Node };
}

function getAutoImportSymbolFromCompletionEntryData(name: string, data: CompletionEntryData, program: Program, host: LanguageServiceHost): { symbol: Symbol, origin: SymbolOriginInfoExport } | undefined {
function getAutoImportSymbolFromCompletionEntryData(name: string, data: CompletionEntryData, program: Program, host: LanguageServiceHost): { symbol: Symbol, origin: SymbolOriginInfoExport | SymbolOriginInfoResolvedExport } | undefined {
const containingProgram = data.isPackageJsonImport ? host.getPackageJsonAutoImportProvider!()! : program;
const checker = containingProgram.getTypeChecker();
const moduleSymbol =
Expand All @@ -2989,18 +3044,7 @@ namespace ts.Completions {
if (!symbol) return undefined;
const isDefaultExport = data.exportName === InternalSymbolName.Default;
symbol = isDefaultExport && getLocalSymbolForExportDefault(symbol) || symbol;
return {
symbol,
origin: {
kind: data.moduleSpecifier ? SymbolOriginInfoKind.ResolvedExport : SymbolOriginInfoKind.Export,
moduleSymbol,
symbolName: name,
isDefaultExport,
exportName: data.exportName,
fileName: data.fileName,
isFromPackageJson: !!data.isPackageJsonImport,
}
};
return { symbol, origin: completionEntryDataToSymbolOriginInfo(data, name, moduleSymbol) };
}

interface CompletionEntryDisplayNameForSymbol {
Expand Down
34 changes: 12 additions & 22 deletions src/services/exportInfoMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ namespace ts {
isUsableByFile(importingFile: Path): boolean;
clear(): void;
add(importingFile: Path, symbol: Symbol, key: __String, moduleSymbol: Symbol, moduleFile: SourceFile | undefined, exportKind: ExportKind, isFromPackageJson: boolean, scriptTarget: ScriptTarget, checker: TypeChecker): void;
get(importingFile: Path, importedName: string, symbol: Symbol, moduleName: string, checker: TypeChecker): readonly SymbolExportInfo[] | undefined;
forEach(importingFile: Path, action: (info: readonly SymbolExportInfo[], name: string, isFromAmbientModule: boolean) => void): void;
get(importingFile: Path, key: string): readonly SymbolExportInfo[] | undefined;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The central insight of this PR is that the cache key does not actually need to be derivable from an updated program later on, because the only thing that ever calls get is continuePreviousIncompleteResponse in completions.ts, and rather than recomputing the key from symbols it currently has access to, we can just save the key we originally came up with on CompletionEntryData and use that to access the cache in continuePreviousIncompleteResponse. This means the only criteria that the key needs to satisfy is that it produces proper grouping while building the map. This opens the door to using getSymbolId(skipAlias(someExport)) in the key, which doesn't have the same very weird pitfalls as trying to find a way to uniquely identify the target symbol that remains stable over program updates.

forEach(importingFile: Path, action: (info: readonly SymbolExportInfo[], name: string, isFromAmbientModule: boolean, key: string) => void): void;
releaseSymbols(): void;
isEmpty(): boolean;
/** @returns Whether the change resulted in the cache being cleared */
Expand Down Expand Up @@ -87,34 +87,35 @@ namespace ts {
: getNameForExportedSymbol(namedSymbol, scriptTarget);
const moduleName = stripQuotes(moduleSymbol.name);
const id = exportInfoId++;
const target = skipAlias(symbol, checker);
const storedSymbol = symbol.flags & SymbolFlags.Transient ? undefined : symbol;
const storedModuleSymbol = moduleSymbol.flags & SymbolFlags.Transient ? undefined : moduleSymbol;
if (!storedSymbol || !storedModuleSymbol) symbols.set(id, [symbol, moduleSymbol]);

exportInfo.add(key(importedName, symbol, moduleName, checker), {
exportInfo.add(key(importedName, symbol, isExternalModuleNameRelative(moduleName) ? undefined : moduleName, checker), {
id,
symbolTableKey,
symbolName: importedName,
moduleName,
moduleFile,
moduleFileName: moduleFile?.fileName,
exportKind,
targetFlags: skipAlias(symbol, checker).flags,
targetFlags: target.flags,
isFromPackageJson,
symbol: storedSymbol,
moduleSymbol: storedModuleSymbol,
});
},
get: (importingFile, importedName, symbol, moduleName, checker) => {
get: (importingFile, key) => {
if (importingFile !== usableByFileName) return;
const result = exportInfo.get(key(importedName, symbol, moduleName, checker));
const result = exportInfo.get(key);
return result?.map(rehydrateCachedInfo);
},
forEach: (importingFile, action) => {
if (importingFile !== usableByFileName) return;
exportInfo.forEach((info, key) => {
const { symbolName, ambientModuleName } = parseKey(key);
action(info.map(rehydrateCachedInfo), symbolName, !!ambientModuleName);
action(info.map(rehydrateCachedInfo), symbolName, !!ambientModuleName, key);
});
},
releaseSymbols: () => {
Expand Down Expand Up @@ -183,29 +184,18 @@ namespace ts {
};
}

function key(importedName: string, symbol: Symbol, moduleName: string, checker: TypeChecker) {
const unquoted = stripQuotes(moduleName);
const moduleKey = isExternalModuleNameRelative(unquoted) ? "/" : unquoted;
const target = skipAlias(symbol, checker);
return `${importedName}|${createSymbolKey(target)}|${moduleKey}`;
function key(importedName: string, symbol: Symbol, ambientModuleName: string | undefined, checker: TypeChecker): string {
const moduleKey = ambientModuleName || "";
return `${importedName}|${getSymbolId(skipAlias(symbol, checker))}|${moduleKey}`;
}

function parseKey(key: string) {
const symbolName = key.substring(0, key.indexOf("|"));
const moduleKey = key.substring(key.lastIndexOf("|") + 1);
const ambientModuleName = moduleKey === "/" ? undefined : moduleKey;
const ambientModuleName = moduleKey === "" ? undefined : moduleKey;
return { symbolName, ambientModuleName };
}

function createSymbolKey(symbol: Symbol) {
let key = symbol.name;
while (symbol.parent) {
key += `,${symbol.parent.name}`;
symbol = symbol.parent;
}
return key;
}

function fileIsGlobalOnly(file: SourceFile) {
return !file.commonJsModuleIndicator && !file.externalModuleIndicator && !file.moduleAugmentations && !file.ambientModuleNames;
}
Expand Down
28 changes: 18 additions & 10 deletions src/services/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1187,24 +1187,32 @@ namespace ts {
entries: CompletionEntry[];
}

export interface CompletionEntryData {
/** The file name declaring the export's module symbol, if it was an external module */
fileName?: string;
/** The module name (with quotes stripped) of the export's module symbol, if it was an ambient module */
ambientModuleName?: string;
/** True if the export was found in the package.json AutoImportProvider */
isPackageJsonImport?: true;
export interface CompletionEntryDataAutoImport {
/**
* The name of the property or export in the module's symbol table. Differs from the completion name
* in the case of InternalSymbolName.ExportEquals and InternalSymbolName.Default.
*/
exportName: string;
/**
* Set for auto imports with eagerly resolved module specifiers.
*/
moduleSpecifier?: string;
/** The file name declaring the export's module symbol, if it was an external module */
fileName?: string;
/** The module name (with quotes stripped) of the export's module symbol, if it was an ambient module */
ambientModuleName?: string;
/** True if the export was found in the package.json AutoImportProvider */
isPackageJsonImport?: true;
}

export interface CompletionEntryDataUnresolved extends CompletionEntryDataAutoImport {
/** The key in the `ExportMapCache` where the completion entry's `SymbolExportInfo[]` is found */
exportMapKey: string;
}

export interface CompletionEntryDataResolved extends CompletionEntryDataAutoImport {
moduleSpecifier: string;
}

export type CompletionEntryData = CompletionEntryDataUnresolved | CompletionEntryDataResolved;

// see comments in protocol.ts
export interface CompletionEntry {
name: string;
Expand Down
Loading