For import completion, if multiple re-exports exist, choose the one with the shortest path (#20049)

* For import completion, if multiple re-exports exist, choose the one with the shortest path

* Code review
This commit is contained in:
Andy
2017-11-17 14:35:32 -08:00
committed by GitHub
parent e7adb1ce79
commit 97bb471e48
4 changed files with 131 additions and 28 deletions

View File

@@ -394,6 +394,14 @@ namespace ts {
return result;
}
export function mapIterator<T, U>(iter: Iterator<T>, mapFn: (x: T) => U): Iterator<U> {
return { next };
function next(): { value: U, done: false } | { value: never, done: true } {
const iterRes = iter.next();
return iterRes.done ? iterRes : { value: mapFn(iterRes.value), done: false };
}
}
// Maps from T to T and avoids allocation if all elements map to themselves
export function sameMap<T>(array: T[], f: (x: T, i: number) => T): T[];
export function sameMap<T>(array: ReadonlyArray<T>, f: (x: T, i: number) => T): ReadonlyArray<T>;
@@ -917,6 +925,36 @@ namespace ts {
return array.slice().sort(comparer);
}
export function best<T>(iter: Iterator<T>, isBetter: (a: T, b: T) => boolean): T | undefined {
const x = iter.next();
if (x.done) {
return undefined;
}
let best = x.value;
while (true) {
const { value, done } = iter.next();
if (done) {
return best;
}
if (isBetter(value, best)) {
best = value;
}
}
}
export function arrayIterator<T>(array: ReadonlyArray<T>): Iterator<T> {
let i = 0;
return { next: () => {
if (i === array.length) {
return { value: undefined as never, done: true };
}
else {
i++;
return { value: array[i - 1], done: false };
}
}};
}
/**
* Stable sort of an array. Elements equal to each other maintain their relative position in the array.
*/
@@ -1122,6 +1160,10 @@ namespace ts {
return result;
}
export function toArray<T>(value: T | ReadonlyArray<T>): ReadonlyArray<T> {
return isArray(value) ? value : [value];
}
/**
* Calls `callback` for each entry in the map, returning the first truthy result.
* Use `map.forEach` instead for normal iteration.

View File

@@ -184,8 +184,10 @@ namespace ts.codefix {
Equals
}
export function getCodeActionForImport(moduleSymbol: Symbol, context: ImportCodeFixOptions): ImportCodeAction[] {
const declarations = getImportDeclarations(moduleSymbol, context.checker, context.sourceFile, context.cachedImportDeclarations);
export function getCodeActionForImport(moduleSymbols: Symbol | ReadonlyArray<Symbol>, context: ImportCodeFixOptions): ImportCodeAction[] {
moduleSymbols = toArray(moduleSymbols);
const declarations = flatMap(moduleSymbols, moduleSymbol =>
getImportDeclarations(moduleSymbol, context.checker, context.sourceFile, context.cachedImportDeclarations));
const actions: ImportCodeAction[] = [];
if (context.symbolToken) {
// It is possible that multiple import statements with the same specifier exist in the file.
@@ -207,7 +209,7 @@ namespace ts.codefix {
}
}
}
actions.push(getCodeActionForAddImport(moduleSymbol, context, declarations));
actions.push(getCodeActionForAddImport(moduleSymbols, context, declarations));
return actions;
}
@@ -313,16 +315,19 @@ namespace ts.codefix {
}
}
export function getModuleSpecifierForNewImport(sourceFile: SourceFile, moduleSymbol: Symbol, options: CompilerOptions, getCanonicalFileName: (file: string) => string, host: LanguageServiceHost): string | undefined {
const moduleFileName = moduleSymbol.valueDeclaration.getSourceFile().fileName;
const sourceDirectory = getDirectoryPath(sourceFile.fileName);
export function getModuleSpecifierForNewImport(sourceFile: SourceFile, moduleSymbols: ReadonlyArray<Symbol>, options: CompilerOptions, getCanonicalFileName: (file: string) => string, host: LanguageServiceHost): string | undefined {
const choices = mapIterator(arrayIterator(moduleSymbols), moduleSymbol => {
const moduleFileName = moduleSymbol.valueDeclaration.getSourceFile().fileName;
const sourceDirectory = getDirectoryPath(sourceFile.fileName);
return tryGetModuleNameFromAmbientModule(moduleSymbol) ||
tryGetModuleNameFromTypeRoots(options, host, getCanonicalFileName, moduleFileName) ||
tryGetModuleNameAsNodeModule(options, moduleFileName, host, getCanonicalFileName, sourceDirectory) ||
tryGetModuleNameFromBaseUrl(options, moduleFileName, getCanonicalFileName) ||
options.rootDirs && tryGetModuleNameFromRootDirs(options.rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName) ||
removeExtensionAndIndexPostFix(getRelativePath(moduleFileName, sourceDirectory, getCanonicalFileName), options);
return tryGetModuleNameFromAmbientModule(moduleSymbol) ||
tryGetModuleNameFromTypeRoots(options, host, getCanonicalFileName, moduleFileName) ||
tryGetModuleNameAsNodeModule(options, moduleFileName, host, getCanonicalFileName, sourceDirectory) ||
tryGetModuleNameFromBaseUrl(options, moduleFileName, getCanonicalFileName) ||
options.rootDirs && tryGetModuleNameFromRootDirs(options.rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName) ||
removeExtensionAndIndexPostFix(getRelativePath(moduleFileName, sourceDirectory, getCanonicalFileName), options);
});
return best(choices, (a, b) => a.length < b.length);
}
function tryGetModuleNameFromAmbientModule(moduleSymbol: Symbol): string | undefined {
@@ -543,7 +548,7 @@ namespace ts.codefix {
}
function getCodeActionForAddImport(
moduleSymbol: Symbol,
moduleSymbols: ReadonlyArray<Symbol>,
ctx: ImportCodeFixOptions,
declarations: ReadonlyArray<AnyImportSyntax>): ImportCodeAction {
const fromExistingImport = firstDefined(declarations, declaration => {
@@ -565,7 +570,7 @@ namespace ts.codefix {
}
const moduleSpecifier = firstDefined(declarations, moduleSpecifierFromAnyImport)
|| getModuleSpecifierForNewImport(ctx.sourceFile, moduleSymbol, ctx.compilerOptions, ctx.getCanonicalFileName, ctx.host);
|| getModuleSpecifierForNewImport(ctx.sourceFile, moduleSymbols, ctx.compilerOptions, ctx.getCanonicalFileName, ctx.host);
return getCodeActionForNewImport(ctx, moduleSpecifier);
}
@@ -659,24 +664,33 @@ namespace ts.codefix {
symbolName = symbol.name;
}
else {
Debug.fail("Either the symbol or the JSX namespace should be a UMD global if we got here");
throw Debug.fail("Either the symbol or the JSX namespace should be a UMD global if we got here");
}
const allowSyntheticDefaultImports = getAllowSyntheticDefaultImports(compilerOptions);
return getCodeActionForImport(symbol, { ...context, symbolName, kind: getUmdImportKind(compilerOptions) });
}
function getUmdImportKind(compilerOptions: CompilerOptions) {
// Import a synthetic `default` if enabled.
if (allowSyntheticDefaultImports) {
return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Default });
if (getAllowSyntheticDefaultImports(compilerOptions)) {
return ImportKind.Default;
}
const moduleKind = getEmitModuleKind(compilerOptions);
// When a synthetic `default` is unavailable, use `import..require` if the module kind supports it.
if (moduleKind === ModuleKind.AMD || moduleKind === ModuleKind.CommonJS || moduleKind === ModuleKind.UMD) {
return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Equals });
const moduleKind = getEmitModuleKind(compilerOptions);
switch (moduleKind) {
case ModuleKind.AMD:
case ModuleKind.CommonJS:
case ModuleKind.UMD:
return ImportKind.Equals;
case ModuleKind.System:
case ModuleKind.ES2015:
case ModuleKind.ESNext:
case ModuleKind.None:
// Fall back to the `import * as ns` style import.
return ImportKind.Namespace;
default:
throw Debug.assertNever(moduleKind);
}
// Fall back to the `import * as ns` style import.
return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Namespace });
}
function getActionsForNonUMDImport(context: ImportCodeFixContext, allSourceFiles: ReadonlyArray<SourceFile>, cancellationToken: CancellationToken): ImportCodeAction[] {

View File

@@ -443,7 +443,7 @@ namespace ts.Completions {
}
case "symbol": {
const { symbol, location, symbolToOriginInfoMap } = symbolCompletion;
const { codeActions, sourceDisplay } = getCompletionEntryCodeActionsAndSourceDisplay(symbolToOriginInfoMap, symbol, typeChecker, host, compilerOptions, sourceFile, formatContext, getCanonicalFileName);
const { codeActions, sourceDisplay } = getCompletionEntryCodeActionsAndSourceDisplay(symbolToOriginInfoMap, symbol, typeChecker, host, compilerOptions, sourceFile, formatContext, getCanonicalFileName, allSourceFiles);
const kindModifiers = SymbolDisplay.getSymbolModifiers(symbol);
const { displayParts, documentation, symbolKind, tags } = SymbolDisplay.getSymbolDisplayPartsDocumentationAndSymbolKind(typeChecker, symbol, sourceFile, location, location, SemanticMeaning.All);
return { name, kindModifiers, kind: symbolKind, displayParts, documentation, tags, codeActions, source: sourceDisplay };
@@ -476,6 +476,7 @@ namespace ts.Completions {
sourceFile: SourceFile,
formatContext: formatting.FormatContext,
getCanonicalFileName: GetCanonicalFileName,
allSourceFiles: ReadonlyArray<SourceFile>,
): { codeActions: CodeAction[] | undefined, sourceDisplay: SymbolDisplayPart[] | undefined } {
const symbolOriginInfo = symbolToOriginInfoMap[getSymbolId(symbol)];
if (!symbolOriginInfo) {
@@ -483,9 +484,12 @@ namespace ts.Completions {
}
const { moduleSymbol, isDefaultExport } = symbolOriginInfo;
const exportedSymbol = skipAlias(symbol.exportSymbol || symbol, checker);
const moduleSymbols = getAllReExportingModules(exportedSymbol, checker, allSourceFiles);
Debug.assert(contains(moduleSymbols, moduleSymbol));
const sourceDisplay = [textPart(codefix.getModuleSpecifierForNewImport(sourceFile, moduleSymbol, compilerOptions, getCanonicalFileName, host))];
const codeActions = codefix.getCodeActionForImport(moduleSymbol, {
const sourceDisplay = [textPart(codefix.getModuleSpecifierForNewImport(sourceFile, moduleSymbols, compilerOptions, getCanonicalFileName, host))];
const codeActions = codefix.getCodeActionForImport(moduleSymbols, {
host,
checker,
newLineCharacter: host.getNewLine(),
@@ -500,6 +504,18 @@ namespace ts.Completions {
return { sourceDisplay, codeActions };
}
function getAllReExportingModules(exportedSymbol: Symbol, checker: TypeChecker, allSourceFiles: ReadonlyArray<SourceFile>): ReadonlyArray<Symbol> {
const result: Symbol[] = [];
codefix.forEachExternalModule(checker, allSourceFiles, module => {
for (const exported of checker.getExportsOfModule(module)) {
if (skipAlias(exported, checker) === exportedSymbol) {
result.push(module);
}
}
});
return result;
}
export function getCompletionEntrySymbol(
typeChecker: TypeChecker,
log: (message: string) => void,

View File

@@ -0,0 +1,31 @@
/// <reference path="fourslash.ts" />
// Test that the completion is for the shortest path, even if that's a re-export.
// Note that `source` in completionEntries will still be the original exporting path, but we use the re-export in completionDetails.
// @moduleResolution: node
// @module: commonJs
// @Filename: /foo/index.ts
////export { foo } from "./lib/foo";
// @Filename: /foo/lib/foo.ts
////export const foo = 0;
// @Filename: /user.ts
////fo/**/
goTo.marker("");
const options = { includeExternalModuleExports: true, sourceDisplay: "./foo" };
verify.completionListContains({ name: "foo", source: "/foo/lib/foo" }, "const foo: 0", "", "const", /*spanIndex*/ undefined, /*hasAction*/ true, options);
verify.not.completionListContains({ name: "foo", source: "/foo/index" }, undefined, undefined, undefined, undefined, undefined, options);
verify.applyCodeActionFromCompletion("", {
name: "foo",
source: "/foo/lib/foo",
description: `Import 'foo' from "./foo".`,
// TODO: GH#18445
newFileContent: `import { foo } from "./foo";\r
\r
fo`,
});