Skip to content

Commit

Permalink
Auto-imports: fix some exports being incorrectly stored as re-exports…
Browse files Browse the repository at this point in the history 
… of others due to key conflict (#45792)

* Ensure symbol key unique when target is a local symbol exported elsewhere

* Add test

* Support targets without declarations

* Best key yet

* A-ha moment

* Clean up types

* Update API

* Update unit test
  • Loading branch information
@andrewbranch
andrewbranch committed Oct 8, 2021
1 parent f6c0231 commit 64b8172
Show file tree
Hide file tree
Showing 8 changed files with 222 additions and 96 deletions.
128 changes: 86 additions & 42 deletions src/services/completions.ts
View file
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;
}
}
}
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
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;
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
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

0 comments on commit 64b8172

Please sign in to comment.