From 64b8172f0688c7567f4a880754a7f972423f092a Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Fri, 8 Oct 2021 15:20:12 -0700 Subject: [PATCH] Auto-imports: fix some exports being incorrectly stored as re-exports 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 --- src/services/completions.ts | 128 ++++++++++++------ src/services/exportInfoMap.ts | 34 ++--- src/services/types.ts | 28 ++-- .../unittests/tsserver/completions.ts | 10 +- .../reference/api/tsserverlibrary.d.ts | 25 ++-- tests/baselines/reference/api/typescript.d.ts | 25 ++-- .../autoImportSameNameDefaultExported.ts | 35 +++++ .../completionsImport_reexportTransient.ts | 33 +++++ 8 files changed, 222 insertions(+), 96 deletions(-) create mode 100644 tests/cases/fourslash/autoImportSameNameDefaultExported.ts create mode 100644 tests/cases/fourslash/completionsImport_reexportTransient.ts diff --git a/src/services/completions.ts b/src/services/completions.ts index ce8c310b77..e76f60ba50 100644 --- a/src/services/completions.ts +++ b/src/services/completions.ts @@ -74,13 +74,9 @@ 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 { @@ -88,9 +84,13 @@ namespace ts.Completions { moduleSymbol: Symbol; isDefaultExport: boolean; exportName: string; + exportMapKey: string; } - interface SymbolOriginInfoResolvedExport extends SymbolOriginInfoExport { + interface SymbolOriginInfoResolvedExport extends SymbolOriginInfo { + symbolName: string; + moduleSymbol: Symbol; + exportName: string; moduleSpecifier: string; } @@ -294,6 +294,10 @@ namespace ts.Completions { } } + function completionEntryDataIsResolved(data: CompletionEntryDataAutoImport | undefined): data is CompletionEntryDataResolved { + return !!data?.moduleSpecifier; + } + function continuePreviousIncompleteResponse( cache: IncompleteCompletionsCache, file: SourceFile, @@ -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, @@ -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; } @@ -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; @@ -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) { @@ -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) { @@ -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` @@ -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, @@ -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 = @@ -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 { diff --git a/src/services/exportInfoMap.ts b/src/services/exportInfoMap.ts index 9482034a9f..4d49f63117 100644 --- a/src/services/exportInfoMap.ts +++ b/src/services/exportInfoMap.ts @@ -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 */ @@ -87,11 +87,12 @@ 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, @@ -99,22 +100,22 @@ namespace ts { 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: () => { @@ -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; } diff --git a/src/services/types.ts b/src/services/types.ts index 816c83771b..0659e6295e 100644 --- a/src/services/types.ts +++ b/src/services/types.ts @@ -1187,24 +1187,32 @@ namespace ts { entries: CompletionEntry[]; } - export interface CompletionEntryData { + 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; + 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; - /** - * 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; } + 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; diff --git a/src/testRunner/unittests/tsserver/completions.ts b/src/testRunner/unittests/tsserver/completions.ts index ef201bfd12..4dff530a75 100644 --- a/src/testRunner/unittests/tsserver/completions.ts +++ b/src/testRunner/unittests/tsserver/completions.ts @@ -42,8 +42,14 @@ namespace ts.projectSystem { source: "/a", sourceDisplay: undefined, isSnippet: undefined, - data: { exportName: "foo", fileName: "/a.ts", ambientModuleName: undefined, isPackageJsonImport: undefined, moduleSpecifier: undefined } + data: { exportName: "foo", fileName: "/a.ts", ambientModuleName: undefined, isPackageJsonImport: undefined } }; + + // `data.exportMapKey` contains a SymbolId so should not be mocked up with an expected value here. + // Just assert that it's a string and then delete it so we can compare everything else with `deepEqual`. + const exportMapKey = (response?.entries[0].data as any)?.exportMapKey; + assert.isString(exportMapKey); + delete (response?.entries[0].data as any).exportMapKey; assert.deepEqual(response, { isGlobalCompletion: true, isIncomplete: undefined, @@ -55,7 +61,7 @@ namespace ts.projectSystem { const detailsRequestArgs: protocol.CompletionDetailsRequestArgs = { ...requestLocation, - entryNames: [{ name: "foo", source: "/a", data: { exportName: "foo", fileName: "/a.ts" } }], + entryNames: [{ name: "foo", source: "/a", data: { exportName: "foo", fileName: "/a.ts", exportMapKey } }], }; const detailsResponse = executeSessionRequest(session, protocol.CommandTypes.CompletionDetails, detailsRequestArgs); diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index 1a806cfdfa..349e025e58 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -6367,23 +6367,28 @@ declare namespace ts { isIncomplete?: true; entries: CompletionEntry[]; } - interface CompletionEntryData { + 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; + 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; - /** - * 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; } + interface CompletionEntryDataUnresolved extends CompletionEntryDataAutoImport { + /** The key in the `ExportMapCache` where the completion entry's `SymbolExportInfo[]` is found */ + exportMapKey: string; + } + interface CompletionEntryDataResolved extends CompletionEntryDataAutoImport { + moduleSpecifier: string; + } + type CompletionEntryData = CompletionEntryDataUnresolved | CompletionEntryDataResolved; interface CompletionEntry { name: string; kind: ScriptElementKind; diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts index 7f87f0af9d..2a2c398553 100644 --- a/tests/baselines/reference/api/typescript.d.ts +++ b/tests/baselines/reference/api/typescript.d.ts @@ -6367,23 +6367,28 @@ declare namespace ts { isIncomplete?: true; entries: CompletionEntry[]; } - interface CompletionEntryData { + 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; + 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; - /** - * 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; } + interface CompletionEntryDataUnresolved extends CompletionEntryDataAutoImport { + /** The key in the `ExportMapCache` where the completion entry's `SymbolExportInfo[]` is found */ + exportMapKey: string; + } + interface CompletionEntryDataResolved extends CompletionEntryDataAutoImport { + moduleSpecifier: string; + } + type CompletionEntryData = CompletionEntryDataUnresolved | CompletionEntryDataResolved; interface CompletionEntry { name: string; kind: ScriptElementKind; diff --git a/tests/cases/fourslash/autoImportSameNameDefaultExported.ts b/tests/cases/fourslash/autoImportSameNameDefaultExported.ts new file mode 100644 index 0000000000..aad12f9cca --- /dev/null +++ b/tests/cases/fourslash/autoImportSameNameDefaultExported.ts @@ -0,0 +1,35 @@ +/// + +// @module: commonjs + +// @Filename: /node_modules/antd/index.d.ts +//// declare function Table(): void; +//// export default Table; + +// @Filename: /node_modules/rc-table/index.d.ts +//// declare function Table(): void; +//// export default Table; + +// @Filename: /index.ts +//// Table/**/ + +verify.completions({ + marker: "", + exact: completion.globalsPlus([{ + name: "Table", + source: "antd", + sourceDisplay: "antd", + sortText: completion.SortText.AutoImportSuggestions, + hasAction: true, + }, { + name: "Table", + source: "rc-table", + sourceDisplay: "rc-table", + sortText: completion.SortText.AutoImportSuggestions, + hasAction: true, + }]), + preferences: { + includeCompletionsForModuleExports: true, + allowIncompleteCompletions: true, + } +}); diff --git a/tests/cases/fourslash/completionsImport_reexportTransient.ts b/tests/cases/fourslash/completionsImport_reexportTransient.ts new file mode 100644 index 0000000000..30b16c9e85 --- /dev/null +++ b/tests/cases/fourslash/completionsImport_reexportTransient.ts @@ -0,0 +1,33 @@ +/// + +// @esModuleInterop: true + +// @Filename: /transient.d.ts +//// declare const map: { [K in "one"]: number }; +//// export = map; + +// @Filename: /r1.ts +//// export { one } from "./transient"; + +// @Filename: /r2.ts +//// export { one } from "./r1"; + +// @Filename: /index.ts +//// one/**/ + +goTo.marker(""); + +verify.completions({ + marker: "", + exact: completion.globalsPlus([{ + name: "one", + source: "./transient", + sourceDisplay: "./transient", + hasAction: true, + sortText: completion.SortText.AutoImportSuggestions, + }]), + preferences: { + includeCompletionsForModuleExports: true, + allowIncompleteCompletions: true, + } +});