From 31637fac94bb1fa4e1f04d4812ba341894aa413b Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 15 Nov 2017 12:40:43 -0800 Subject: [PATCH 1/2] For import completion, if multiple re-exports exist, choose the one with the shortest path --- src/compiler/core.ts | 42 ++++++++++++ src/services/codefixes/importFixes.ts | 64 +++++++++++-------- src/services/completions.ts | 22 ++++++- ...mpletionsImport_ofAlias_preferShortPath.ts | 31 +++++++++ 4 files changed, 131 insertions(+), 28 deletions(-) create mode 100644 tests/cases/fourslash/completionsImport_ofAlias_preferShortPath.ts diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 9f839923e7d1c..a746a569c499d 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -394,6 +394,14 @@ namespace ts { return result; } + export function mapIter(iter: Iterator, mapFn: (x: T) => U): Iterator { + 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(array: T[], f: (x: T, i: number) => T): T[]; export function sameMap(array: ReadonlyArray, f: (x: T, i: number) => T): ReadonlyArray; @@ -917,6 +925,36 @@ namespace ts { return array.slice().sort(comparer); } + export function best(iter: Iterator, 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 arrayIter(array: ReadonlyArray): Iterator { + 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(value: T | ReadonlyArray): ReadonlyArray { + 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. diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 8971d1aa75ecf..87fe5a5b5cb1b 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -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, 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, options: CompilerOptions, getCanonicalFileName: (file: string) => string, host: LanguageServiceHost): string | undefined { + const choices = mapIter(arrayIter(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) || - removeFileExtension(getRelativePath(moduleFileName, sourceDirectory, getCanonicalFileName)); + 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) || + removeFileExtension(getRelativePath(moduleFileName, sourceDirectory, getCanonicalFileName)); + }); + return best(choices, (a, b) => a.length < b.length); } function tryGetModuleNameFromAmbientModule(moduleSymbol: Symbol): string | undefined { @@ -546,7 +551,7 @@ namespace ts.codefix { } function getCodeActionForAddImport( - moduleSymbol: Symbol, + moduleSymbols: ReadonlyArray, ctx: ImportCodeFixOptions, declarations: ReadonlyArray): ImportCodeAction { const fromExistingImport = firstDefined(declarations, declaration => { @@ -568,7 +573,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); } @@ -662,24 +667,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, cancellationToken: CancellationToken): ImportCodeAction[] { diff --git a/src/services/completions.ts b/src/services/completions.ts index 423919f535360..18cec0d535776 100644 --- a/src/services/completions.ts +++ b/src/services/completions.ts @@ -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, ): { 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): ReadonlyArray { + 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, diff --git a/tests/cases/fourslash/completionsImport_ofAlias_preferShortPath.ts b/tests/cases/fourslash/completionsImport_ofAlias_preferShortPath.ts new file mode 100644 index 0000000000000..6ffdc06dac777 --- /dev/null +++ b/tests/cases/fourslash/completionsImport_ofAlias_preferShortPath.ts @@ -0,0 +1,31 @@ +/// + +// 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/index" }; // TODO: GH#19962 +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/index".`, + // TODO: GH#18445 + newFileContent: `import { foo } from "./foo/index";\r +\r +fo`, +}); From bc8d45ee9c3a1cf0fc98b0b3d82a1190eba29254 Mon Sep 17 00:00:00 2001 From: andy-ms Date: Thu, 16 Nov 2017 16:56:09 -0800 Subject: [PATCH 2/2] Code review --- src/compiler/core.ts | 6 +++--- src/services/codefixes/importFixes.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index a746a569c499d..beff50578ad42 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -394,9 +394,9 @@ namespace ts { return result; } - export function mapIter(iter: Iterator, mapFn: (x: T) => U): Iterator { + export function mapIterator(iter: Iterator, mapFn: (x: T) => U): Iterator { return { next }; - function next(): { value: U, done: false } | { value: never, done: true } { + function next(): { value: U, done: false } | { value: never, done: true } { const iterRes = iter.next(); return iterRes.done ? iterRes : { value: mapFn(iterRes.value), done: false }; } @@ -942,7 +942,7 @@ namespace ts { } } - export function arrayIter(array: ReadonlyArray): Iterator { + export function arrayIterator(array: ReadonlyArray): Iterator { let i = 0; return { next: () => { if (i === array.length) { diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 87fe5a5b5cb1b..f204d1a249652 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -316,7 +316,7 @@ namespace ts.codefix { } export function getModuleSpecifierForNewImport(sourceFile: SourceFile, moduleSymbols: ReadonlyArray, options: CompilerOptions, getCanonicalFileName: (file: string) => string, host: LanguageServiceHost): string | undefined { - const choices = mapIter(arrayIter(moduleSymbols), moduleSymbol => { + const choices = mapIterator(arrayIterator(moduleSymbols), moduleSymbol => { const moduleFileName = moduleSymbol.valueDeclaration.getSourceFile().fileName; const sourceDirectory = getDirectoryPath(sourceFile.fileName);