diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts index 3dd8f01663816..7a8cb46688f88 100644 --- 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 index 208631c135767..dc0779de3d827 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 3c1dc5026ac86..57c50e460f74f 100644 --- 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,7 +632,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, @@ -637,6 +640,7 @@ namespace ts.codefix { 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(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 index 8f40bd2ffa6f6..955d87c6f6346 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" });