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

src/compiler/core.ts

@@ -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.

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<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[] {

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,16 +476,20 @@ 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) {
             return { codeActions: undefined, sourceDisplay: undefined };
         }
 
         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,

tests/cases/fourslash/completionsImport_ofAlias_preferShortPath.ts

@@ -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`,
+});