From 70b61463724b8c192ff9ec34fa5a8c860ca7391f Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Wed, 19 Jan 2022 14:20:05 -0800 Subject: [PATCH 1/2] Avoid auto-importing from barrel re-exporting index files that are likely to make an import cycle --- src/harness/fourslashImpl.ts | 5 +- src/harness/fourslashInterfaceImpl.ts | 4 +- src/services/codefixes/importFixes.ts | 100 ++++++++++++------ tests/cases/fourslash/fourslash.ts | 2 +- .../importNameCodeFix_barrelExport2.ts | 33 ++++++ .../importNameCodeFix_barrelExport3.ts | 32 ++++++ 6 files changed, 140 insertions(+), 36 deletions(-) create mode 100644 tests/cases/fourslash/importNameCodeFix_barrelExport2.ts create mode 100644 tests/cases/fourslash/importNameCodeFix_barrelExport3.ts diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts index 19b82ac6542a0..1b7bf6f452180 100644 --- a/src/harness/fourslashImpl.ts +++ b/src/harness/fourslashImpl.ts @@ -3133,11 +3133,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 index 8fdd386af1e61..b8089bc34d7fd 100644 --- 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 index 1c18f80316c89..d655396db16ad 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -73,9 +73,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, host, program, preferences, useAutoImportProvider); + const exportInfo = getAllReExportingModules(sourceFile, symbol, moduleSymbol, symbolName, 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 }); } @@ -236,32 +236,35 @@ namespace ts.codefix { NotAllowed = 1 << 2, } type ImportFix = 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; } /** Information needed to augment an existing import declaration. */ @@ -304,7 +307,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 { @@ -383,7 +386,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 }; } @@ -579,7 +582,7 @@ namespace ts.codefix { position: number | undefined, isValidTypeOnlyUseSite: boolean, useRequire: boolean, - moduleSymbols: readonly SymbolExportInfo[], + exportInfo: readonly SymbolExportInfo[], host: LanguageServiceHost, preferences: UserPreferences, fromCacheOnly?: boolean, @@ -593,7 +596,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); @@ -602,7 +605,7 @@ 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, @@ -610,6 +613,7 @@ namespace ts.codefix { useRequire, addAsTypeOnly, exportInfo, + isReExport: i > 0, } ); }); @@ -648,46 +652,80 @@ namespace ts.codefix { } } - interface FixesInfo { readonly fixes: readonly ImportFix[]; readonly symbolName: string; } + interface FixesInfo { readonly fixes: readonly ImportFix[], readonly symbolName: string } function getFixesInfo(context: CodeFixContextBase, errorCode: number, pos: number, useAutoImportProvider: boolean): FixesInfo | undefined { const symbolToken = getTokenAtPosition(context.sourceFile, pos); 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; 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 ImportFix[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter): readonly ImportFix[] { - return sort(fixes, (a, b) => compareValues(a.kind, b.kind) || compareModuleSpecifiers(a, b, sourceFile, program, packageJsonImportFilter.allowsImportingSpecifier)); + function sortFixes(fixes: readonly ImportFix[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter, host: LanguageServiceHost): readonly ImportFix[] { + 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(fixes: readonly T[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter): T | undefined { + function getBestFix(fixes: readonly ImportFix[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter, host: LanguageServiceHost): ImportFix | 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: ImportFix, b: ImportFix, importingFile: SourceFile, program: Program, allowsImportingSpecifier: (specifier: string) => boolean): Comparison { + function compareModuleSpecifiers( + a: ImportFix, + b: ImportFix, + 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: ImportFix, 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 { @@ -702,9 +740,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 }; } function getUmdSymbol(token: Node, checker: TypeChecker): Symbol | undefined { @@ -774,8 +812,8 @@ namespace ts.codefix { const isValidTypeOnlyUseSite = isValidTypeOnlyAliasUseSite(symbolToken); const useRequire = shouldUseRequire(sourceFile, program); - const exportInfos = getExportInfos(symbolName, getMeaningFromLocation(symbolToken), cancellationToken, sourceFile, program, useAutoImportProvider, host, preferences); - const fixes = arrayFrom(flatMapIterator(exportInfos.entries(), ([_, exportInfos]) => + const exportInfo = getExportInfos(symbolName, 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 }; } diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index b971a07b92444..b2b210da875e9 100644 --- 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 new file mode 100644 index 0000000000000..97a2926ae1167 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFix_barrelExport2.ts @@ -0,0 +1,33 @@ +/// + +// @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 new file mode 100644 index 0000000000000..108df23b6f0e6 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFix_barrelExport3.ts @@ -0,0 +1,32 @@ +/// + +// @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" }); From f273ddd681c07e229acbf5c46a7760d64515f841 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Thu, 27 Jan 2022 11:28:18 -0800 Subject: [PATCH 2/2] Finish fixing merge conflict --- src/services/codefixes/importFixes.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 849f7624d6cf8..57c50e460f74f 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -81,11 +81,7 @@ namespace ts.codefix { const symbolName = getNameForExportedSymbol(exportedSymbol, getEmitScriptTarget(compilerOptions)); const checker = program.getTypeChecker(); const symbol = checker.getMergedSymbol(skipAlias(exportedSymbol, checker)); -<<<<<<< HEAD - const exportInfo = getAllReExportingModules(sourceFile, symbol, moduleSymbol, symbolName, host, program, preferences, useAutoImportProvider); -======= - const exportInfos = getAllReExportingModules(sourceFile, symbol, moduleSymbol, symbolName, /*isJsxTagName*/ false, host, program, preferences, useAutoImportProvider); ->>>>>>> main + const exportInfo = getAllReExportingModules(sourceFile, symbol, moduleSymbol, symbolName, /*isJsxTagName*/ false, host, program, preferences, useAutoImportProvider); const useRequire = shouldUseRequire(sourceFile, program); const fix = getImportFixForSymbol(sourceFile, exportInfo, moduleSymbol, symbolName, program, /*position*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences); if (fix) {