Sort import fixes by number of directory separators (#42614)

* Add failing test

* Sort all import fixes by number of directory separators

Author: andrewbranch

src/compiler/moduleSpecifiers.ts

@@ -255,16 +255,8 @@ namespace ts.moduleSpecifiers {
         return firstDefined(imports, ({ text }) => pathIsRelative(text) ? hasJSFileExtension(text) : undefined) || false;
     }
 
-    function numberOfDirectorySeparators(str: string) {
-        const match = str.match(/\//g);
-        return match ? match.length : 0;
-    }
-
     function comparePathsByRedirectAndNumberOfDirectorySeparators(a: ModulePath, b: ModulePath) {
-        return compareBooleans(b.isRedirect, a.isRedirect) || compareValues(
-            numberOfDirectorySeparators(a.path),
-            numberOfDirectorySeparators(b.path)
-        );
+        return compareBooleans(b.isRedirect, a.isRedirect) || compareNumberOfDirectorySeparators(a.path, b.path);
     }
 
     function getNearestAncestorDirectoryWithPackageJson(host: ModuleSpecifierResolutionHost, fileName: string) {

src/compiler/utilities.ts

@@ -6586,6 +6586,18 @@ namespace ts {
         return false;
     }
 
+    function numberOfDirectorySeparators(str: string) {
+        const match = str.match(/\//g);
+        return match ? match.length : 0;
+    }
+
+    export function compareNumberOfDirectorySeparators(path1: string, path2: string) {
+        return compareValues(
+            numberOfDirectorySeparators(path1),
+            numberOfDirectorySeparators(path2)
+        );
+    }
+
     /**
      * Extension boundaries by priority. Lower numbers indicate higher priorities, and are
      * aligned to the offset of the highest priority extension in the

src/services/codefixes/importFixes.ts

@@ -215,15 +215,15 @@ namespace ts.codefix {
             : getAllReExportingModules(sourceFile, exportedSymbol, moduleSymbol, symbolName, host, program, /*useAutoImportProvider*/ true);
         const useRequire = shouldUseRequire(sourceFile, program);
         const preferTypeOnlyImport = compilerOptions.importsNotUsedAsValues === ImportsNotUsedAsValues.Error && !isSourceFileJS(sourceFile) && isValidTypeOnlyAliasUseSite(getTokenAtPosition(sourceFile, position));
-        const moduleSpecifier = first(getNewImportInfos(program, sourceFile, position, preferTypeOnlyImport, useRequire, exportInfos, host, preferences)).moduleSpecifier;
+        const moduleSpecifier = getBestFix(getNewImportInfos(program, sourceFile, position, preferTypeOnlyImport, useRequire, exportInfos, host, preferences), sourceFile, program, host).moduleSpecifier;
         const fix = getImportFixForSymbol(sourceFile, exportInfos, moduleSymbol, symbolName, program, position, preferTypeOnlyImport, useRequire, host, preferences);
         return { moduleSpecifier, codeAction: codeFixActionToCodeAction(codeActionForFix({ host, formatContext, preferences }, sourceFile, symbolName, fix, getQuotePreference(sourceFile, preferences))) };
     }
 
     function getImportFixForSymbol(sourceFile: SourceFile, exportInfos: readonly SymbolExportInfo[], moduleSymbol: Symbol, symbolName: string, program: Program, position: number | undefined, preferTypeOnlyImport: boolean, useRequire: boolean, host: LanguageServiceHost, preferences: UserPreferences) {
         Debug.assert(exportInfos.some(info => info.moduleSymbol === moduleSymbol), "Some exportInfo should match the specified moduleSymbol");
         // We sort the best codefixes first, so taking `first` is best.
-        return first(getFixForImport(exportInfos, symbolName, position, preferTypeOnlyImport, useRequire, program, sourceFile, host, preferences));
+        return getBestFix(getFixForImport(exportInfos, symbolName, position, preferTypeOnlyImport, useRequire, program, sourceFile, host, preferences), sourceFile, program, host);
     }
 
     function codeFixActionToCodeAction({ description, changes, commands }: CodeFixAction): CodeAction {
@@ -424,28 +424,13 @@ namespace ts.codefix {
     ): readonly (FixAddNewImport | FixUseImportType)[] {
         const isJs = isSourceFileJS(sourceFile);
         const compilerOptions = program.getCompilerOptions();
-        const { allowsImportingSpecifier } = createAutoImportFilter(sourceFile, program, host);
-
-        const choicesForEachExportingModule = flatMap(moduleSymbols, ({ moduleSymbol, importKind, exportedSymbolIsTypeOnly }) =>
+        return flatMap(moduleSymbols, ({ moduleSymbol, importKind, exportedSymbolIsTypeOnly }) =>
             moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getTypeChecker(), compilerOptions, sourceFile, createModuleSpecifierResolutionHost(program, host), preferences)
                 .map((moduleSpecifier): FixAddNewImport | FixUseImportType =>
                     // `position` should only be undefined at a missing jsx namespace, in which case we shouldn't be looking for pure types.
                     exportedSymbolIsTypeOnly && isJs
                         ? { kind: ImportFixKind.ImportType, moduleSpecifier, position: Debug.checkDefined(position, "position should be defined") }
                         : { kind: ImportFixKind.AddNew, moduleSpecifier, importKind, useRequire, typeOnly: preferTypeOnlyImport }));
-
-        // Sort by presence in package.json, then shortest paths first
-        return sort(choicesForEachExportingModule, (a, b) => {
-            const allowsImportingA = allowsImportingSpecifier(a.moduleSpecifier);
-            const allowsImportingB = allowsImportingSpecifier(b.moduleSpecifier);
-            if (allowsImportingA && !allowsImportingB) {
-                return -1;
-            }
-            if (allowsImportingB && !allowsImportingA) {
-                return 1;
-            }
-            return a.moduleSpecifier.length - b.moduleSpecifier.length;
-        });
     }
 
     function getFixesForAddImport(
@@ -479,7 +464,31 @@ namespace ts.codefix {
         const info = errorCode === Diagnostics._0_refers_to_a_UMD_global_but_the_current_file_is_a_module_Consider_adding_an_import_instead.code
             ? getFixesInfoForUMDImport(context, symbolToken)
             : isIdentifier(symbolToken) ? getFixesInfoForNonUMDImport(context, symbolToken, useAutoImportProvider) : undefined;
-        return info && { ...info, fixes: sort(info.fixes, (a, b) => a.kind - b.kind) };
+        return info && { ...info, fixes: sortFixes(info.fixes, context.sourceFile, context.program, context.host) };
+    }
+
+    function sortFixes(fixes: readonly ImportFix[], sourceFile: SourceFile, program: Program, host: LanguageServiceHost): readonly ImportFix[] {
+        const { allowsImportingSpecifier } = createAutoImportFilter(sourceFile, program, host);
+        return sort(fixes, (a, b) => compareValues(a.kind, b.kind) || compareModuleSpecifiers(a, b, allowsImportingSpecifier));
+    }
+
+    function getBestFix<T extends ImportFix>(fixes: readonly T[], sourceFile: SourceFile, program: Program, host: LanguageServiceHost): T {
+        // These will always be placed first if available, and are better than other kinds
+        if (fixes[0].kind === ImportFixKind.UseNamespace || fixes[0].kind === ImportFixKind.AddToExisting) {
+            return fixes[0];
+        }
+        const { allowsImportingSpecifier } = createAutoImportFilter(sourceFile, program, host);
+        return fixes.reduce((best, fix) =>
+            compareModuleSpecifiers(fix, best, allowsImportingSpecifier) === Comparison.LessThan ? fix : best
+        );
+    }
+
+    function compareModuleSpecifiers(a: ImportFix, b: ImportFix, allowsImportingSpecifier: (specifier: string) => boolean): Comparison {
+        if (a.kind !== ImportFixKind.UseNamespace && b.kind !== ImportFixKind.UseNamespace) {
+            return compareBooleans(allowsImportingSpecifier(a.moduleSpecifier), allowsImportingSpecifier(b.moduleSpecifier))
+                || compareNumberOfDirectorySeparators(a.moduleSpecifier, b.moduleSpecifier);
+        }
+        return Comparison.EqualTo;
     }
 
     function getFixesInfoForUMDImport({ sourceFile, program, host, preferences }: CodeFixContextBase, token: Node): FixesInfo | undefined {

tests/cases/fourslash/importNameCodeFix_sortByDistance.ts

@@ -0,0 +1,26 @@
+/// <reference path="fourslash.ts" />
+
+// @module: commonjs
+
+// @Filename: /src/admin/utils/db/db.ts
+//// export const db = {};
+
+// @Filename: /src/admin/utils/db/index.ts
+//// export * from "./db";
+
+// @Filename: /src/client/helpers/db.ts
+//// export const db = {};
+
+// @Filename: /src/client/db.ts
+//// export const db = {};
+
+// @Filename: /src/client/foo.ts
+//// db/**/
+
+goTo.marker("");
+verify.importFixAtPosition([
+  `import { db } from "./db";\n\ndb`,
+  `import { db } from "./helpers/db";\n\ndb`,
+  `import { db } from "../admin/utils/db";\n\ndb`,
+  `import { db } from "../admin/utils/db/db";\n\ndb`
+]);