Avoid auto-importing from barrel re-exporting index files that are likely to make an import cycle (microsoft#47516)

andrewbranch · web-flow · commit 954d0442eb8c · 2022-02-10T15:05:08.000-08:00
* Avoid auto-importing from barrel re-exporting index files that are likely to make an import cycle

* Finish fixing merge conflict
diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts
@@ -3146,11 +3146,12 @@ namespace FourSlash {
             });
         }
 
-        public verifyImportFixModuleSpecifiers(markerName: string, moduleSpecifiers: string[]) {
+        public verifyImportFixModuleSpecifiers(markerName: string, moduleSpecifiers: string[], preferences?: ts.UserPreferences) {
             const marker = this.getMarkerByName(markerName);
             const codeFixes = this.getCodeFixes(marker.fileName, ts.Diagnostics.Cannot_find_name_0.code, {
                 includeCompletionsForModuleExports: true,
-                includeCompletionsWithInsertText: true
+                includeCompletionsWithInsertText: true,
+                ...preferences,
             }, marker.position).filter(f => f.fixName === ts.codefix.importFixName);
 
             const actualModuleSpecifiers = ts.mapDefined(codeFixes, fix => {
diff --git a/src/harness/fourslashInterfaceImpl.ts b/src/harness/fourslashInterfaceImpl.ts
@@ -478,8 +478,8 @@ namespace FourSlashInterface {
             this.state.verifyImportFixAtPosition(expectedTextArray, errorCode, preferences);
         }
 
-        public importFixModuleSpecifiers(marker: string, moduleSpecifiers: string[]) {
-            this.state.verifyImportFixModuleSpecifiers(marker, moduleSpecifiers);
+        public importFixModuleSpecifiers(marker: string, moduleSpecifiers: string[], preferences?: ts.UserPreferences) {
+            this.state.verifyImportFixModuleSpecifiers(marker, moduleSpecifiers, preferences);
         }
 
         public navigationBar(json: any, options?: { checkSpans?: boolean }) {
diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts
@@ -81,9 +81,9 @@ namespace ts.codefix {
             const symbolName = getNameForExportedSymbol(exportedSymbol, getEmitScriptTarget(compilerOptions));
             const checker = program.getTypeChecker();
             const symbol = checker.getMergedSymbol(skipAlias(exportedSymbol, checker));
-            const exportInfos = getAllReExportingModules(sourceFile, symbol, moduleSymbol, symbolName, /*isJsxTagName*/ false, host, program, preferences, useAutoImportProvider);
+            const exportInfo = getAllReExportingModules(sourceFile, symbol, moduleSymbol, symbolName, /*isJsxTagName*/ false, host, program, preferences, useAutoImportProvider);
             const useRequire = shouldUseRequire(sourceFile, program);
-            const fix = getImportFixForSymbol(sourceFile, exportInfos, moduleSymbol, symbolName, program, /*position*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences);
+            const fix = getImportFixForSymbol(sourceFile, exportInfo, moduleSymbol, symbolName, program, /*position*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences);
             if (fix) {
                 addImport({ fixes: [fix], symbolName, errorIdentifierText: undefined });
             }
@@ -248,32 +248,35 @@ namespace ts.codefix {
     }
     type ImportFix = FixUseNamespaceImport | FixAddJsdocTypeImport | FixAddToExistingImport | FixAddNewImport | FixPromoteTypeOnlyImport;
     type ImportFixWithModuleSpecifier = FixUseNamespaceImport | FixAddJsdocTypeImport | FixAddToExistingImport | FixAddNewImport;
-    interface FixUseNamespaceImport {
+
+    // Properties are be undefined if fix is derived from an existing import
+    interface ImportFixBase {
+        readonly isReExport?: boolean;
+        readonly exportInfo?: SymbolExportInfo;
+        readonly moduleSpecifier: string;
+    }
+    interface FixUseNamespaceImport extends ImportFixBase {
         readonly kind: ImportFixKind.UseNamespace;
         readonly namespacePrefix: string;
         readonly position: number;
-        readonly moduleSpecifier: string;
     }
-    interface FixAddJsdocTypeImport {
+    interface FixAddJsdocTypeImport extends ImportFixBase {
         readonly kind: ImportFixKind.JsdocTypeImport;
-        readonly moduleSpecifier: string;
         readonly position: number;
+        readonly isReExport: boolean;
         readonly exportInfo: SymbolExportInfo;
     }
-    interface FixAddToExistingImport {
+    interface FixAddToExistingImport extends ImportFixBase {
         readonly kind: ImportFixKind.AddToExisting;
         readonly importClauseOrBindingPattern: ImportClause | ObjectBindingPattern;
-        readonly moduleSpecifier: string;
         readonly importKind: ImportKind.Default | ImportKind.Named;
         readonly addAsTypeOnly: AddAsTypeOnly;
     }
-    interface FixAddNewImport {
+    interface FixAddNewImport extends ImportFixBase {
         readonly kind: ImportFixKind.AddNew;
-        readonly moduleSpecifier: string;
         readonly importKind: ImportKind;
         readonly addAsTypeOnly: AddAsTypeOnly;
         readonly useRequire: boolean;
-        readonly exportInfo?: SymbolExportInfo;
     }
     interface FixPromoteTypeOnlyImport {
         readonly kind: ImportFixKind.PromoteTypeOnly;
@@ -331,7 +334,7 @@ namespace ts.codefix {
     function getImportFixForSymbol(sourceFile: SourceFile, exportInfos: readonly SymbolExportInfo[], moduleSymbol: Symbol, symbolName: string, program: Program, position: number | undefined, isValidTypeOnlyUseSite: boolean, useRequire: boolean, host: LanguageServiceHost, preferences: UserPreferences) {
         Debug.assert(exportInfos.some(info => info.moduleSymbol === moduleSymbol || info.symbol.parent === moduleSymbol), "Some exportInfo should match the specified moduleSymbol");
         const packageJsonImportFilter = createPackageJsonImportFilter(sourceFile, preferences, host);
-        return getBestFix(getImportFixes(exportInfos, symbolName, position, isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences), sourceFile, program, packageJsonImportFilter);
+        return getBestFix(getImportFixes(exportInfos, symbolName, position, isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences), sourceFile, program, packageJsonImportFilter, host);
     }
 
     function codeFixActionToCodeAction({ description, changes, commands }: CodeFixAction): CodeAction {
@@ -410,7 +413,7 @@ namespace ts.codefix {
             host,
             preferences,
             fromCacheOnly);
-        const result = getBestFix(fixes, importingFile, program, packageJsonImportFilter || createPackageJsonImportFilter(importingFile, preferences, host));
+        const result = getBestFix(fixes, importingFile, program, packageJsonImportFilter || createPackageJsonImportFilter(importingFile, preferences, host), host);
         return result && { ...result, computedWithoutCacheCount };
     }
 
@@ -606,7 +609,7 @@ namespace ts.codefix {
         position: number | undefined,
         isValidTypeOnlyUseSite: boolean,
         useRequire: boolean,
-        moduleSymbols: readonly SymbolExportInfo[],
+        exportInfo: readonly SymbolExportInfo[],
         host: LanguageServiceHost,
         preferences: UserPreferences,
         fromCacheOnly?: boolean,
@@ -620,7 +623,7 @@ namespace ts.codefix {
             : (moduleSymbol: Symbol, checker: TypeChecker) => moduleSpecifiers.getModuleSpecifiersWithCacheInfo(moduleSymbol, checker, compilerOptions, sourceFile, moduleSpecifierResolutionHost, preferences);
 
         let computedWithoutCacheCount = 0;
-        const fixes = flatMap(moduleSymbols, exportInfo => {
+        const fixes = flatMap(exportInfo, (exportInfo, i) => {
             const checker = getChecker(exportInfo.isFromPackageJson);
             const { computedWithoutCache, moduleSpecifiers } = getModuleSpecifiers(exportInfo.moduleSymbol, checker);
             const importedSymbolHasValueMeaning = !!(exportInfo.targetFlags & SymbolFlags.Value);
@@ -629,14 +632,15 @@ namespace ts.codefix {
             return moduleSpecifiers?.map((moduleSpecifier): FixAddNewImport | FixAddJsdocTypeImport =>
                 // `position` should only be undefined at a missing jsx namespace, in which case we shouldn't be looking for pure types.
                 !importedSymbolHasValueMeaning && isJs && position !== undefined
-                    ? { kind: ImportFixKind.JsdocTypeImport, moduleSpecifier, position, exportInfo }
+                    ? { kind: ImportFixKind.JsdocTypeImport, moduleSpecifier, position, exportInfo, isReExport: i > 0 }
                     : {
                         kind: ImportFixKind.AddNew,
                         moduleSpecifier,
                         importKind: getImportKind(sourceFile, exportInfo.exportKind, compilerOptions),
                         useRequire,
                         addAsTypeOnly,
                         exportInfo,
+                        isReExport: i > 0,
                     }
             );
         });
@@ -675,7 +679,7 @@ namespace ts.codefix {
         }
     }
 
-    interface FixesInfo { readonly fixes: readonly ImportFix[]; readonly symbolName: string; readonly errorIdentifierText: string | undefined; }
+    interface FixesInfo { readonly fixes: readonly ImportFix[], readonly symbolName: string, readonly errorIdentifierText: string | undefined }
     function getFixesInfo(context: CodeFixContextBase, errorCode: number, pos: number, useAutoImportProvider: boolean): FixesInfo | undefined {
         const symbolToken = getTokenAtPosition(context.sourceFile, pos);
         let info;
@@ -695,39 +699,73 @@ namespace ts.codefix {
         }
 
         const packageJsonImportFilter = createPackageJsonImportFilter(context.sourceFile, context.preferences, context.host);
-        return info && { ...info, fixes: sortFixes(info.fixes, context.sourceFile, context.program, packageJsonImportFilter) };
+        return info && { ...info, fixes: sortFixes(info.fixes, context.sourceFile, context.program, packageJsonImportFilter, context.host) };
     }
 
-    function sortFixes(fixes: readonly ImportFixWithModuleSpecifier[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter): readonly ImportFixWithModuleSpecifier[] {
-        return sort(fixes, (a, b) => compareValues(a.kind, b.kind) || compareModuleSpecifiers(a, b, sourceFile, program, packageJsonImportFilter.allowsImportingSpecifier));
+    function sortFixes(fixes: readonly ImportFixWithModuleSpecifier[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter, host: LanguageServiceHost): readonly ImportFixWithModuleSpecifier[] {
+        const _toPath = (fileName: string) => toPath(fileName, host.getCurrentDirectory(), hostGetCanonicalFileName(host));
+        return sort(fixes, (a, b) => compareValues(a.kind, b.kind) || compareModuleSpecifiers(a, b, sourceFile, program, packageJsonImportFilter.allowsImportingSpecifier, _toPath));
     }
 
-    function getBestFix<T extends ImportFixWithModuleSpecifier>(fixes: readonly T[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter): T | undefined {
+    function getBestFix(fixes: readonly ImportFixWithModuleSpecifier[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter, host: LanguageServiceHost): ImportFixWithModuleSpecifier | undefined {
         if (!some(fixes)) return;
         // 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];
         }
+
         return fixes.reduce((best, fix) =>
             // Takes true branch of conditional if `fix` is better than `best`
-            compareModuleSpecifiers(fix, best, sourceFile, program, packageJsonImportFilter.allowsImportingSpecifier) === Comparison.LessThan ? fix : best
+            compareModuleSpecifiers(
+                fix,
+                best,
+                sourceFile,
+                program,
+                packageJsonImportFilter.allowsImportingSpecifier,
+                fileName => toPath(fileName, host.getCurrentDirectory(), hostGetCanonicalFileName(host)),
+            ) === Comparison.LessThan ? fix : best
         );
     }
 
     /** @returns `Comparison.LessThan` if `a` is better than `b`. */
-    function compareModuleSpecifiers(a: ImportFixWithModuleSpecifier, b: ImportFixWithModuleSpecifier, importingFile: SourceFile, program: Program, allowsImportingSpecifier: (specifier: string) => boolean): Comparison {
+    function compareModuleSpecifiers(
+        a: ImportFixWithModuleSpecifier,
+        b: ImportFixWithModuleSpecifier,
+        importingFile: SourceFile,
+        program: Program,
+        allowsImportingSpecifier: (specifier: string) => boolean,
+        toPath: (fileName: string) => Path,
+    ): Comparison {
         if (a.kind !== ImportFixKind.UseNamespace && b.kind !== ImportFixKind.UseNamespace) {
             return compareBooleans(allowsImportingSpecifier(b.moduleSpecifier), allowsImportingSpecifier(a.moduleSpecifier))
                 || compareNodeCoreModuleSpecifiers(a.moduleSpecifier, b.moduleSpecifier, importingFile, program)
-                || compareBooleans(isOnlyDotsAndSlashes(a.moduleSpecifier), isOnlyDotsAndSlashes(b.moduleSpecifier))
+                || compareBooleans(
+                    isFixPossiblyReExportingImportingFile(a, importingFile, program.getCompilerOptions(), toPath),
+                    isFixPossiblyReExportingImportingFile(b, importingFile, program.getCompilerOptions(), toPath))
                 || compareNumberOfDirectorySeparators(a.moduleSpecifier, b.moduleSpecifier);
         }
         return Comparison.EqualTo;
     }
 
-    const notDotOrSlashPattern = /[^.\/]/;
-    function isOnlyDotsAndSlashes(path: string) {
-        return !notDotOrSlashPattern.test(path);
+    // This is a simple heuristic to try to avoid creating an import cycle with a barrel re-export.
+    // E.g., do not `import { Foo } from ".."` when you could `import { Foo } from "../Foo"`.
+    // This can produce false positives or negatives if re-exports cross into sibling directories
+    // (e.g. `export * from "../whatever"`) or are not named "index" (we don't even try to consider
+    // this if we're in a resolution mode where you can't drop trailing "/index" from paths).
+    function isFixPossiblyReExportingImportingFile(fix: ImportFixWithModuleSpecifier, importingFile: SourceFile, compilerOptions: CompilerOptions, toPath: (fileName: string) => Path): boolean {
+        if (fix.isReExport &&
+            fix.exportInfo?.moduleFileName &&
+            getEmitModuleResolutionKind(compilerOptions) === ModuleResolutionKind.NodeJs &&
+            isIndexFileName(fix.exportInfo.moduleFileName)
+        ) {
+            const reExportDir = toPath(getDirectoryPath(fix.exportInfo.moduleFileName));
+            return startsWith((importingFile.path), reExportDir);
+        }
+        return false;
+    }
+
+    function isIndexFileName(fileName: string) {
+        return getBaseFileName(fileName, [".js", ".jsx", ".d.ts", ".ts", ".tsx"], /*ignoreCase*/ true) === "index";
     }
 
     function compareNodeCoreModuleSpecifiers(a: string, b: string, importingFile: SourceFile, program: Program): Comparison {
@@ -742,9 +780,9 @@ namespace ts.codefix {
         if (!umdSymbol) return undefined;
         const symbol = checker.getAliasedSymbol(umdSymbol);
         const symbolName = umdSymbol.name;
-        const exportInfos: readonly SymbolExportInfo[] = [{ symbol: umdSymbol, moduleSymbol: symbol, moduleFileName: undefined, exportKind: ExportKind.UMD, targetFlags: symbol.flags, isFromPackageJson: false }];
+        const exportInfo: readonly SymbolExportInfo[] = [{ symbol: umdSymbol, moduleSymbol: symbol, moduleFileName: undefined, exportKind: ExportKind.UMD, targetFlags: symbol.flags, isFromPackageJson: false }];
         const useRequire = shouldUseRequire(sourceFile, program);
-        const fixes = getImportFixes(exportInfos, symbolName, isIdentifier(token) ? token.getStart(sourceFile) : undefined, /*isValidTypeOnlyUseSite*/ false, useRequire, program, sourceFile, host, preferences);
+        const fixes = getImportFixes(exportInfo, symbolName, isIdentifier(token) ? token.getStart(sourceFile) : undefined, /*isValidTypeOnlyUseSite*/ false, useRequire, program, sourceFile, host, preferences);
         return { fixes, symbolName, errorIdentifierText: tryCast(token, isIdentifier)?.text };
     }
     function getUmdSymbol(token: Node, checker: TypeChecker): Symbol | undefined {
@@ -814,8 +852,8 @@ namespace ts.codefix {
 
         const isValidTypeOnlyUseSite = isValidTypeOnlyAliasUseSite(symbolToken);
         const useRequire = shouldUseRequire(sourceFile, program);
-        const exportInfos = getExportInfos(symbolName, isJSXTagName(symbolToken), getMeaningFromLocation(symbolToken), cancellationToken, sourceFile, program, useAutoImportProvider, host, preferences);
-        const fixes = arrayFrom(flatMapIterator(exportInfos.entries(), ([_, exportInfos]) =>
+        const exportInfo = getExportInfos(symbolName, isJSXTagName(symbolToken), getMeaningFromLocation(symbolToken), cancellationToken, sourceFile, program, useAutoImportProvider, host, preferences);
+        const fixes = arrayFrom(flatMapIterator(exportInfo.entries(), ([_, exportInfos]) =>
             getImportFixes(exportInfos, symbolName, symbolToken.getStart(sourceFile), isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences)));
         return { fixes, symbolName, errorIdentifierText: symbolToken.text };
     }
diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts
@@ -364,7 +364,7 @@ declare namespace FourSlashInterface {
         fileAfterApplyingRefactorAtMarker(markerName: string, expectedContent: string, refactorNameToApply: string, formattingOptions?: FormatCodeOptions): void;
         getAndApplyCodeFix(errorCode?: number, index?: number): void;
         importFixAtPosition(expectedTextArray: string[], errorCode?: number, options?: UserPreferences): void;
-        importFixModuleSpecifiers(marker: string, moduleSpecifiers: string[]): void;
+        importFixModuleSpecifiers(marker: string, moduleSpecifiers: string[], options?: UserPreferences): void;
 
         navigationBar(json: any, options?: { checkSpans?: boolean }): void;
         navigationTree(json: any, options?: { checkSpans?: boolean }): void;
diff --git a/tests/cases/fourslash/importNameCodeFix_barrelExport2.ts b/tests/cases/fourslash/importNameCodeFix_barrelExport2.ts
@@ -0,0 +1,33 @@
+/// <reference path="fourslash.ts" />
+
+// @module: commonjs
+// @baseUrl: /
+
+// @Filename: /proj/foo/a.ts
+//// export const A = 0;
+
+// @Filename: /proj/foo/b.ts
+//// export {};
+//// A/*sibling*/
+
+// @Filename: /proj/foo/index.ts
+//// export * from "./a";
+//// export * from "./b";
+
+// @Filename: /proj/index.ts
+//// export * from "./foo";
+//// export * from "./src";
+
+// @Filename: /proj/src/a.ts
+//// export {};
+//// A/*parent*/
+
+// @Filename: /proj/src/utils.ts
+//// export function util() { return "util"; }
+//// export { A } from "../foo/a";
+
+// @Filename: /proj/src/index.ts
+//// export * from "./a";
+
+verify.importFixModuleSpecifiers("sibling", ["proj/foo/a", "proj/src/utils", "proj", "proj/foo"], { importModuleSpecifierPreference: "non-relative" });
+verify.importFixModuleSpecifiers("parent", ["proj/foo", "proj/foo/a", "proj/src/utils", "proj"], { importModuleSpecifierPreference: "non-relative" });
diff --git a/tests/cases/fourslash/importNameCodeFix_barrelExport3.ts b/tests/cases/fourslash/importNameCodeFix_barrelExport3.ts
@@ -0,0 +1,32 @@
+/// <reference path="fourslash.ts" />
+
+// @module: commonjs
+
+// @Filename: /foo/a.ts
+//// export const A = 0;
+
+// @Filename: /foo/b.ts
+//// export {};
+//// A/*sibling*/
+
+// @Filename: /foo/index.ts
+//// export * from "./a";
+//// export * from "./b";
+
+// @Filename: /index.ts
+//// export * from "./foo";
+//// export * from "./src";
+
+// @Filename: /src/a.ts
+//// export {};
+//// A/*parent*/
+
+// @Filename: /src/index.ts
+//// export * from "./a";
+
+verify.importFixModuleSpecifiers("sibling", ["./a", "./index", "../index"], { importModuleSpecifierEnding: "index" });
+// Here, "../foo/a" and "../foo/index" actually have the same sorting score,
+// so the original program order is preserved, which seems coincidentally probably good?
+// In other words, a re-export is preferable only if it saves on directory separators
+// and isn't in an ancestor directory of the importing file.
+verify.importFixModuleSpecifiers("parent", ["../foo/a", "../foo/index", "../index"], { importModuleSpecifierEnding: "index" });

Original file line number	Diff line number	Diff line change
`@@ -478,8 +478,8 @@ namespace FourSlashInterface {`
`478`	`478`	`this.state.verifyImportFixAtPosition(expectedTextArray, errorCode, preferences);`
`479`	`479`	`}`
`480`	`480`
`481`		`- public importFixModuleSpecifiers(marker: string, moduleSpecifiers: string[]) {`
`482`		`- this.state.verifyImportFixModuleSpecifiers(marker, moduleSpecifiers);`
	`481`	`+ public importFixModuleSpecifiers(marker: string, moduleSpecifiers: string[], preferences?: ts.UserPreferences) {`
	`482`	`+ this.state.verifyImportFixModuleSpecifiers(marker, moduleSpecifiers, preferences);`
`483`	`483`	`}`
`484`	`484`
`485`	`485`	`public navigationBar(json: any, options?: { checkSpans?: boolean }) {`