Skip to content

Commit 97bb471

Browse files
author
Andy
authored
For import completion, if multiple re-exports exist, choose the one with the shortest path (microsoft#20049)
* For import completion, if multiple re-exports exist, choose the one with the shortest path * Code review
1 parent e7adb1c commit 97bb471

4 files changed

Lines changed: 131 additions & 28 deletions

File tree

‎src/compiler/core.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,14 @@ namespace ts {
394394
return result;
395395
}
396396

397+
export function mapIterator<T, U>(iter: Iterator<T>, mapFn: (x: T) => U): Iterator<U> {
398+
return { next };
399+
function next(): { value: U, done: false } | { value: never, done: true } {
400+
const iterRes = iter.next();
401+
return iterRes.done ? iterRes : { value: mapFn(iterRes.value), done: false };
402+
}
403+
}
404+
397405
// Maps from T to T and avoids allocation if all elements map to themselves
398406
export function sameMap<T>(array: T[], f: (x: T, i: number) => T): T[];
399407
export function sameMap<T>(array: ReadonlyArray<T>, f: (x: T, i: number) => T): ReadonlyArray<T>;
@@ -917,6 +925,36 @@ namespace ts {
917925
return array.slice().sort(comparer);
918926
}
919927

928+
export function best<T>(iter: Iterator<T>, isBetter: (a: T, b: T) => boolean): T | undefined {
929+
const x = iter.next();
930+
if (x.done) {
931+
return undefined;
932+
}
933+
let best = x.value;
934+
while (true) {
935+
const { value, done } = iter.next();
936+
if (done) {
937+
return best;
938+
}
939+
if (isBetter(value, best)) {
940+
best = value;
941+
}
942+
}
943+
}
944+
945+
export function arrayIterator<T>(array: ReadonlyArray<T>): Iterator<T> {
946+
let i = 0;
947+
return { next: () => {
948+
if (i === array.length) {
949+
return { value: undefined as never, done: true };
950+
}
951+
else {
952+
i++;
953+
return { value: array[i - 1], done: false };
954+
}
955+
}};
956+
}
957+
920958
/**
921959
* Stable sort of an array. Elements equal to each other maintain their relative position in the array.
922960
*/
@@ -1122,6 +1160,10 @@ namespace ts {
11221160
return result;
11231161
}
11241162

1163+
export function toArray<T>(value: T | ReadonlyArray<T>): ReadonlyArray<T> {
1164+
return isArray(value) ? value : [value];
1165+
}
1166+
11251167
/**
11261168
* Calls `callback` for each entry in the map, returning the first truthy result.
11271169
* Use `map.forEach` instead for normal iteration.

‎src/services/codefixes/importFixes.ts‎

Lines changed: 39 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,10 @@ namespace ts.codefix {
184184
Equals
185185
}
186186

187-
export function getCodeActionForImport(moduleSymbol: Symbol, context: ImportCodeFixOptions): ImportCodeAction[] {
188-
const declarations = getImportDeclarations(moduleSymbol, context.checker, context.sourceFile, context.cachedImportDeclarations);
187+
export function getCodeActionForImport(moduleSymbols: Symbol | ReadonlyArray<Symbol>, context: ImportCodeFixOptions): ImportCodeAction[] {
188+
moduleSymbols = toArray(moduleSymbols);
189+
const declarations = flatMap(moduleSymbols, moduleSymbol =>
190+
getImportDeclarations(moduleSymbol, context.checker, context.sourceFile, context.cachedImportDeclarations));
189191
const actions: ImportCodeAction[] = [];
190192
if (context.symbolToken) {
191193
// It is possible that multiple import statements with the same specifier exist in the file.
@@ -207,7 +209,7 @@ namespace ts.codefix {
207209
}
208210
}
209211
}
210-
actions.push(getCodeActionForAddImport(moduleSymbol, context, declarations));
212+
actions.push(getCodeActionForAddImport(moduleSymbols, context, declarations));
211213
return actions;
212214
}
213215

@@ -313,16 +315,19 @@ namespace ts.codefix {
313315
}
314316
}
315317

316-
export function getModuleSpecifierForNewImport(sourceFile: SourceFile, moduleSymbol: Symbol, options: CompilerOptions, getCanonicalFileName: (file: string) => string, host: LanguageServiceHost): string | undefined {
317-
const moduleFileName = moduleSymbol.valueDeclaration.getSourceFile().fileName;
318-
const sourceDirectory = getDirectoryPath(sourceFile.fileName);
318+
export function getModuleSpecifierForNewImport(sourceFile: SourceFile, moduleSymbols: ReadonlyArray<Symbol>, options: CompilerOptions, getCanonicalFileName: (file: string) => string, host: LanguageServiceHost): string | undefined {
319+
const choices = mapIterator(arrayIterator(moduleSymbols), moduleSymbol => {
320+
const moduleFileName = moduleSymbol.valueDeclaration.getSourceFile().fileName;
321+
const sourceDirectory = getDirectoryPath(sourceFile.fileName);
319322

320-
return tryGetModuleNameFromAmbientModule(moduleSymbol) ||
321-
tryGetModuleNameFromTypeRoots(options, host, getCanonicalFileName, moduleFileName) ||
322-
tryGetModuleNameAsNodeModule(options, moduleFileName, host, getCanonicalFileName, sourceDirectory) ||
323-
tryGetModuleNameFromBaseUrl(options, moduleFileName, getCanonicalFileName) ||
324-
options.rootDirs && tryGetModuleNameFromRootDirs(options.rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName) ||
325-
removeExtensionAndIndexPostFix(getRelativePath(moduleFileName, sourceDirectory, getCanonicalFileName), options);
323+
return tryGetModuleNameFromAmbientModule(moduleSymbol) ||
324+
tryGetModuleNameFromTypeRoots(options, host, getCanonicalFileName, moduleFileName) ||
325+
tryGetModuleNameAsNodeModule(options, moduleFileName, host, getCanonicalFileName, sourceDirectory) ||
326+
tryGetModuleNameFromBaseUrl(options, moduleFileName, getCanonicalFileName) ||
327+
options.rootDirs && tryGetModuleNameFromRootDirs(options.rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName) ||
328+
removeExtensionAndIndexPostFix(getRelativePath(moduleFileName, sourceDirectory, getCanonicalFileName), options);
329+
});
330+
return best(choices, (a, b) => a.length < b.length);
326331
}
327332

328333
function tryGetModuleNameFromAmbientModule(moduleSymbol: Symbol): string | undefined {
@@ -543,7 +548,7 @@ namespace ts.codefix {
543548
}
544549

545550
function getCodeActionForAddImport(
546-
moduleSymbol: Symbol,
551+
moduleSymbols: ReadonlyArray<Symbol>,
547552
ctx: ImportCodeFixOptions,
548553
declarations: ReadonlyArray<AnyImportSyntax>): ImportCodeAction {
549554
const fromExistingImport = firstDefined(declarations, declaration => {
@@ -565,7 +570,7 @@ namespace ts.codefix {
565570
}
566571

567572
const moduleSpecifier = firstDefined(declarations, moduleSpecifierFromAnyImport)
568-
|| getModuleSpecifierForNewImport(ctx.sourceFile, moduleSymbol, ctx.compilerOptions, ctx.getCanonicalFileName, ctx.host);
573+
|| getModuleSpecifierForNewImport(ctx.sourceFile, moduleSymbols, ctx.compilerOptions, ctx.getCanonicalFileName, ctx.host);
569574
return getCodeActionForNewImport(ctx, moduleSpecifier);
570575
}
571576

@@ -659,24 +664,33 @@ namespace ts.codefix {
659664
symbolName = symbol.name;
660665
}
661666
else {
662-
Debug.fail("Either the symbol or the JSX namespace should be a UMD global if we got here");
667+
throw Debug.fail("Either the symbol or the JSX namespace should be a UMD global if we got here");
663668
}
664669

665-
const allowSyntheticDefaultImports = getAllowSyntheticDefaultImports(compilerOptions);
666-
670+
return getCodeActionForImport(symbol, { ...context, symbolName, kind: getUmdImportKind(compilerOptions) });
671+
}
672+
function getUmdImportKind(compilerOptions: CompilerOptions) {
667673
// Import a synthetic `default` if enabled.
668-
if (allowSyntheticDefaultImports) {
669-
return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Default });
674+
if (getAllowSyntheticDefaultImports(compilerOptions)) {
675+
return ImportKind.Default;
670676
}
671-
const moduleKind = getEmitModuleKind(compilerOptions);
672677

673678
// When a synthetic `default` is unavailable, use `import..require` if the module kind supports it.
674-
if (moduleKind === ModuleKind.AMD || moduleKind === ModuleKind.CommonJS || moduleKind === ModuleKind.UMD) {
675-
return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Equals });
679+
const moduleKind = getEmitModuleKind(compilerOptions);
680+
switch (moduleKind) {
681+
case ModuleKind.AMD:
682+
case ModuleKind.CommonJS:
683+
case ModuleKind.UMD:
684+
return ImportKind.Equals;
685+
case ModuleKind.System:
686+
case ModuleKind.ES2015:
687+
case ModuleKind.ESNext:
688+
case ModuleKind.None:
689+
// Fall back to the `import * as ns` style import.
690+
return ImportKind.Namespace;
691+
default:
692+
throw Debug.assertNever(moduleKind);
676693
}
677-
678-
// Fall back to the `import * as ns` style import.
679-
return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Namespace });
680694
}
681695

682696
function getActionsForNonUMDImport(context: ImportCodeFixContext, allSourceFiles: ReadonlyArray<SourceFile>, cancellationToken: CancellationToken): ImportCodeAction[] {

‎src/services/completions.ts‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -443,7 +443,7 @@ namespace ts.Completions {
443443
}
444444
case "symbol": {
445445
const { symbol, location, symbolToOriginInfoMap } = symbolCompletion;
446-
const { codeActions, sourceDisplay } = getCompletionEntryCodeActionsAndSourceDisplay(symbolToOriginInfoMap, symbol, typeChecker, host, compilerOptions, sourceFile, formatContext, getCanonicalFileName);
446+
const { codeActions, sourceDisplay } = getCompletionEntryCodeActionsAndSourceDisplay(symbolToOriginInfoMap, symbol, typeChecker, host, compilerOptions, sourceFile, formatContext, getCanonicalFileName, allSourceFiles);
447447
const kindModifiers = SymbolDisplay.getSymbolModifiers(symbol);
448448
const { displayParts, documentation, symbolKind, tags } = SymbolDisplay.getSymbolDisplayPartsDocumentationAndSymbolKind(typeChecker, symbol, sourceFile, location, location, SemanticMeaning.All);
449449
return { name, kindModifiers, kind: symbolKind, displayParts, documentation, tags, codeActions, source: sourceDisplay };
@@ -476,16 +476,20 @@ namespace ts.Completions {
476476
sourceFile: SourceFile,
477477
formatContext: formatting.FormatContext,
478478
getCanonicalFileName: GetCanonicalFileName,
479+
allSourceFiles: ReadonlyArray<SourceFile>,
479480
): { codeActions: CodeAction[] | undefined, sourceDisplay: SymbolDisplayPart[] | undefined } {
480481
const symbolOriginInfo = symbolToOriginInfoMap[getSymbolId(symbol)];
481482
if (!symbolOriginInfo) {
482483
return { codeActions: undefined, sourceDisplay: undefined };
483484
}
484485

485486
const { moduleSymbol, isDefaultExport } = symbolOriginInfo;
487+
const exportedSymbol = skipAlias(symbol.exportSymbol || symbol, checker);
488+
const moduleSymbols = getAllReExportingModules(exportedSymbol, checker, allSourceFiles);
489+
Debug.assert(contains(moduleSymbols, moduleSymbol));
486490

487-
const sourceDisplay = [textPart(codefix.getModuleSpecifierForNewImport(sourceFile, moduleSymbol, compilerOptions, getCanonicalFileName, host))];
488-
const codeActions = codefix.getCodeActionForImport(moduleSymbol, {
491+
const sourceDisplay = [textPart(codefix.getModuleSpecifierForNewImport(sourceFile, moduleSymbols, compilerOptions, getCanonicalFileName, host))];
492+
const codeActions = codefix.getCodeActionForImport(moduleSymbols, {
489493
host,
490494
checker,
491495
newLineCharacter: host.getNewLine(),
@@ -500,6 +504,18 @@ namespace ts.Completions {
500504
return { sourceDisplay, codeActions };
501505
}
502506

507+
function getAllReExportingModules(exportedSymbol: Symbol, checker: TypeChecker, allSourceFiles: ReadonlyArray<SourceFile>): ReadonlyArray<Symbol> {
508+
const result: Symbol[] = [];
509+
codefix.forEachExternalModule(checker, allSourceFiles, module => {
510+
for (const exported of checker.getExportsOfModule(module)) {
511+
if (skipAlias(exported, checker) === exportedSymbol) {
512+
result.push(module);
513+
}
514+
}
515+
});
516+
return result;
517+
}
518+
503519
export function getCompletionEntrySymbol(
504520
typeChecker: TypeChecker,
505521
log: (message: string) => void,

‎tests/cases/fourslash/completionsImport_ofAlias_preferShortPath.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// Test that the completion is for the shortest path, even if that's a re-export.
4+
// Note that `source` in completionEntries will still be the original exporting path, but we use the re-export in completionDetails.
5+
6+
// @moduleResolution: node
7+
// @module: commonJs
8+
9+
// @Filename: /foo/index.ts
10+
////export { foo } from "./lib/foo";
11+
12+
// @Filename: /foo/lib/foo.ts
13+
////export const foo = 0;
14+
15+
// @Filename: /user.ts
16+
////fo/**/
17+
18+
goTo.marker("");
19+
const options = { includeExternalModuleExports: true, sourceDisplay: "./foo" };
20+
verify.completionListContains({ name: "foo", source: "/foo/lib/foo" }, "const foo: 0", "", "const", /*spanIndex*/ undefined, /*hasAction*/ true, options);
21+
verify.not.completionListContains({ name: "foo", source: "/foo/index" }, undefined, undefined, undefined, undefined, undefined, options);
22+
23+
verify.applyCodeActionFromCompletion("", {
24+
name: "foo",
25+
source: "/foo/lib/foo",
26+
description: `Import 'foo' from "./foo".`,
27+
// TODO: GH#18445
28+
newFileContent: `import { foo } from "./foo";\r
29+
\r
30+
fo`,
31+
});

0 commit comments

Comments
 (0)