diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 0bfc433c477c7..52729c1e8fb5a 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -3883,14 +3883,15 @@ namespace ts { const links = getSymbolLinks(symbol); let specifier = links.specifierCache && links.specifierCache.get(contextFile.path); if (!specifier) { - specifier = flatten(moduleSpecifiers.getModuleSpecifiers( + specifier = moduleSpecifiers.getModuleSpecifierForDeclarationFile( symbol, compilerOptions, contextFile, context.tracker.moduleResolverHost, context.tracker.moduleResolverHost.getSourceFiles!(), // TODO: GH#18217 - { importModuleSpecifierPreference: "non-relative" } - ))[0]; + { importModuleSpecifierPreference: "non-relative" }, + host.redirectTargetsMap, + ); links.specifierCache = links.specifierCache || createMap(); links.specifierCache.set(contextFile.path, specifier); } diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts index b243ff348146f..a82f5b854e12a 100644 --- a/src/compiler/moduleSpecifiers.ts +++ b/src/compiler/moduleSpecifiers.ts @@ -1,4 +1,4 @@ -// Used by importFixes to synthesize import module specifiers. +// Used by importFixes, getEditsForFileRename, and declaration emit to synthesize import module specifiers. /* @internal */ namespace ts.moduleSpecifiers { export interface ModuleSpecifierPreferences { @@ -14,13 +14,26 @@ namespace ts.moduleSpecifiers { host: ModuleSpecifierResolutionHost, files: ReadonlyArray, preferences: ModuleSpecifierPreferences = {}, + redirectTargetsMap: RedirectTargetsMap, ): string { const info = getInfo(compilerOptions, importingSourceFile, importingSourceFileName, host); - const modulePaths = getAllModulePaths(files, toFileName, info.getCanonicalFileName, host); + const modulePaths = getAllModulePaths(files, importingSourceFileName, toFileName, info.getCanonicalFileName, host, redirectTargetsMap); return firstDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions)) || first(getLocalModuleSpecifiers(toFileName, info, compilerOptions, preferences)); } + export function getModuleSpecifierForDeclarationFile( + moduleSymbol: Symbol, + compilerOptions: CompilerOptions, + importingSourceFile: SourceFile, + host: ModuleSpecifierResolutionHost, + files: ReadonlyArray, + preferences: ModuleSpecifierPreferences, + redirectTargetsMap: RedirectTargetsMap, + ): string { + return first(first(getModuleSpecifiers(moduleSymbol, compilerOptions, importingSourceFile, host, files, preferences, redirectTargetsMap))); + } + // For each symlink/original for a module, returns a list of ways to import that file. export function getModuleSpecifiers( moduleSymbol: Symbol, @@ -29,6 +42,7 @@ namespace ts.moduleSpecifiers { host: ModuleSpecifierResolutionHost, files: ReadonlyArray, preferences: ModuleSpecifierPreferences, + redirectTargetsMap: RedirectTargetsMap, ): ReadonlyArray> { const ambient = tryGetModuleNameFromAmbientModule(moduleSymbol); if (ambient) return [[ambient]]; @@ -37,7 +51,8 @@ namespace ts.moduleSpecifiers { if (!files) { return Debug.fail("Files list must be present to resolve symlinks in specifier resolution"); } - const modulePaths = getAllModulePaths(files, getSourceFileOfNode(moduleSymbol.valueDeclaration).fileName, info.getCanonicalFileName, host); + const moduleSourceFile = getSourceFileOfNode(moduleSymbol.valueDeclaration); + const modulePaths = getAllModulePaths(files, importingSourceFile.path, moduleSourceFile.fileName, info.getCanonicalFileName, host, redirectTargetsMap); const global = mapDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions)); return global.length ? global.map(g => [g]) : modulePaths.map(moduleFileName => @@ -142,64 +157,69 @@ namespace ts.moduleSpecifiers { return firstDefined(imports, ({ text }) => pathIsRelative(text) ? fileExtensionIs(text, Extension.Js) : undefined) || false; } - function discoverProbableSymlinks(files: ReadonlyArray, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost) { + function stringsEqual(a: string, b: string, getCanonicalFileName: GetCanonicalFileName): boolean { + return getCanonicalFileName(a) === getCanonicalFileName(b); + } + + // KLUDGE: Don't assume one 'node_modules' links to another. More likely a single directory inside the node_modules is the symlink. + // ALso, don't assume that an `@foo` directory is linked. More likely the contents of that are linked. + function isNodeModulesOrScopedPackageDirectory(s: string, getCanonicalFileName: GetCanonicalFileName): boolean { + return getCanonicalFileName(s) === "node_modules" || startsWith(s, "@"); + } + + function guessDirectorySymlink(a: string, b: string, cwd: string, getCanonicalFileName: GetCanonicalFileName): [string, string] { + const aParts = getPathComponents(toPath(a, cwd, getCanonicalFileName)); + const bParts = getPathComponents(toPath(b, cwd, getCanonicalFileName)); + while (!isNodeModulesOrScopedPackageDirectory(aParts[aParts.length - 2], getCanonicalFileName) && + !isNodeModulesOrScopedPackageDirectory(bParts[bParts.length - 2], getCanonicalFileName) && + stringsEqual(aParts[aParts.length - 1], bParts[bParts.length - 1], getCanonicalFileName)) { + aParts.pop(); + bParts.pop(); + } + return [getPathFromPathComponents(aParts), getPathFromPathComponents(bParts)]; + } + + function discoverProbableSymlinks(files: ReadonlyArray, getCanonicalFileName: GetCanonicalFileName, cwd: string): ReadonlyMap { + const result = createMap(); const symlinks = mapDefined(files, sf => sf.resolvedModules && firstDefinedIterator(sf.resolvedModules.values(), res => res && res.originalPath && res.resolvedFileName !== res.originalPath ? [res.resolvedFileName, res.originalPath] : undefined)); - const result = createMap(); - if (symlinks) { - const currentDirectory = host.getCurrentDirectory ? host.getCurrentDirectory() : ""; - const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive; - for (const [resolvedPath, originalPath] of symlinks) { - const resolvedParts = getPathComponents(toPath(resolvedPath, currentDirectory, getCanonicalFileName)); - const originalParts = getPathComponents(toPath(originalPath, currentDirectory, getCanonicalFileName)); - while (compareStrings(resolvedParts[resolvedParts.length - 1], originalParts[originalParts.length - 1]) === Comparison.EqualTo) { - resolvedParts.pop(); - originalParts.pop(); - } - result.set(getPathFromPathComponents(originalParts), getPathFromPathComponents(resolvedParts)); - } + for (const [resolvedPath, originalPath] of symlinks) { + const [commonResolved, commonOriginal] = guessDirectorySymlink(resolvedPath, originalPath, cwd, getCanonicalFileName); + result.set(commonOriginal, commonResolved); } return result; } - function getAllModulePathsUsingIndirectSymlinks(files: ReadonlyArray, target: string, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost) { - const links = discoverProbableSymlinks(files, getCanonicalFileName, host); - const paths = arrayFrom(links.keys()); - let options: string[] | undefined; - const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive; - for (const path of paths) { - const resolved = links.get(path)!; - if (compareStrings(target.slice(0, resolved.length + 1), resolved + "/") === Comparison.EqualTo) { - const relative = getRelativePathFromDirectory(resolved, target, getCanonicalFileName); - const option = resolvePath(path, relative); - if (!host.fileExists || host.fileExists(option)) { - if (!options) options = []; - options.push(option); - } - } - } - if (options) { - options.push(target); // Since these are speculative, we also include the original resolved name as a possibility - return options; - } - return [target]; - } - /** * Looks for existing imports that use symlinks to this module. - * Only if no symlink is available, the real path will be used. + * Symlinks will be returned first so they are preferred over the real path. */ - function getAllModulePaths(files: ReadonlyArray, importedFileName: string, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost): ReadonlyArray { - const symlinks = mapDefined(files, sf => - sf.resolvedModules && firstDefinedIterator(sf.resolvedModules.values(), res => - res && res.resolvedFileName === importedFileName ? res.originalPath : undefined)); + function getAllModulePaths(files: ReadonlyArray, importingFileName: string, importedFileName: string, getCanonicalFileName: GetCanonicalFileName, host: ModuleSpecifierResolutionHost, redirectTargetsMap: RedirectTargetsMap): ReadonlyArray { + const redirects = redirectTargetsMap.get(importedFileName); + const importedFileNames = redirects ? [...redirects, importedFileName] : [importedFileName]; const cwd = host.getCurrentDirectory ? host.getCurrentDirectory() : ""; - const baseOptions = getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(importedFileName, cwd), getCanonicalFileName, host); - if (symlinks.length === 0) { - return baseOptions; - } - return deduplicate(concatenate(baseOptions, flatMap(symlinks, importedFileName => getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(importedFileName, cwd), getCanonicalFileName, host)))); + const targets = importedFileNames.map(f => getNormalizedAbsolutePath(f, cwd)); + const links = discoverProbableSymlinks(files, getCanonicalFileName, cwd); + + const result: string[] = []; + const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive; + links.forEach((resolved, path) => { + if (startsWithDirectory(importingFileName, resolved, getCanonicalFileName)) { + return; // Don't want to a package to globally import from itself + } + + const target = targets.find(t => compareStrings(t.slice(0, resolved.length + 1), resolved + "/") === Comparison.EqualTo); + if (target === undefined) return; + + const relative = getRelativePathFromDirectory(resolved, target, getCanonicalFileName); + const option = resolvePath(path, relative); + if (!host.fileExists || host.fileExists(option)) { + result.push(option); + } + }); + result.push(...targets); + return result; } function getRelativePathNParents(relativePath: string): number { diff --git a/src/compiler/program.ts b/src/compiler/program.ts index 363940c5a79b5..c70477b4a17ce 100644 --- a/src/compiler/program.ts +++ b/src/compiler/program.ts @@ -634,7 +634,8 @@ namespace ts { const packageIdToSourceFile = createMap(); // Maps from a SourceFile's `.path` to the name of the package it was imported with. let sourceFileToPackageName = createMap(); - let redirectTargetsSet = createMap(); + // Key is a file name. Value is the (non-empty, or undefined) list of files that redirect to it. + let redirectTargetsMap = createMultiMap(); const filesByName = createMap(); let missingFilePaths: ReadonlyArray | undefined; @@ -752,7 +753,7 @@ namespace ts { getSourceFileFromReference, getLibFileFromReference, sourceFileToPackageName, - redirectTargetsSet, + redirectTargetsMap, isEmittedFile, getConfigFileParsingDiagnostics, getResolvedModuleWithFailedLookupLocationsFromCache, @@ -1073,7 +1074,7 @@ namespace ts { fileChanged = false; newSourceFile = oldSourceFile; // Use the redirect. } - else if (oldProgram.redirectTargetsSet.has(oldSourceFile.path)) { + else if (oldProgram.redirectTargetsMap.has(oldSourceFile.path)) { // If a redirected-to source file changes, the redirect may be broken. if (newSourceFile !== oldSourceFile) { return oldProgram.structureIsReused = StructureIsReused.Not; @@ -1220,7 +1221,7 @@ namespace ts { resolvedProjectReferences = oldProgram.getProjectReferences(); sourceFileToPackageName = oldProgram.sourceFileToPackageName; - redirectTargetsSet = oldProgram.redirectTargetsSet; + redirectTargetsMap = oldProgram.redirectTargetsMap; return oldProgram.structureIsReused = StructureIsReused.Completely; } @@ -2079,7 +2080,7 @@ namespace ts { // Some other SourceFile already exists with this package name and version. // Instead of creating a duplicate, just redirect to the existing one. const dupFile = createRedirectSourceFile(fileFromPackageId, file!, fileName, path); // TODO: GH#18217 - redirectTargetsSet.set(fileFromPackageId.path, true); + redirectTargetsMap.add(fileFromPackageId.path, fileName); filesByName.set(path, dupFile); sourceFileToPackageName.set(path, packageId.name); processingOtherFiles!.push(dupFile); diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 2703f95e70884..169c93fed2ad4 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2551,7 +2551,7 @@ namespace ts { /** * If two source files are for the same version of the same package, one will redirect to the other. * (See `createRedirectSourceFile` in program.ts.) - * The redirect will have this set. The redirected-to source file will be in `redirectTargetsSet`. + * The redirect will have this set. The redirected-to source file will be in `redirectTargetsMap`. */ /* @internal */ redirectInfo?: RedirectInfo; @@ -2798,7 +2798,7 @@ namespace ts { /** Given a source file, get the name of the package it was imported from. */ /* @internal */ sourceFileToPackageName: Map; /** Set of all source files that some other source file redirects to. */ - /* @internal */ redirectTargetsSet: Map; + /* @internal */ redirectTargetsMap: MultiMap; /** Is the file emitted file */ /* @internal */ isEmittedFile(file: string): boolean; @@ -2807,6 +2807,9 @@ namespace ts { getProjectReferences(): (ResolvedProjectReference | undefined)[] | undefined; } + /* @internal */ + export type RedirectTargetsMap = ReadonlyMap>; + export interface ResolvedProjectReference { commandLine: ParsedCommandLine; sourceFile: SourceFile; @@ -2884,6 +2887,8 @@ namespace ts { getSourceFiles(): ReadonlyArray; getSourceFile(fileName: string): SourceFile | undefined; getResolvedTypeReferenceDirectives(): ReadonlyMap; + + readonly redirectTargetsMap: RedirectTargetsMap; } export interface TypeChecker { diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 0ac533f7658dd..00032c8460f15 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -7157,6 +7157,12 @@ namespace ts { return path.slice(0, Math.max(rootLength, path.lastIndexOf(directorySeparator))); } + export function startsWithDirectory(fileName: string, directoryName: string, getCanonicalFileName: GetCanonicalFileName): boolean { + const canonicalFileName = getCanonicalFileName(fileName); + const canonicalDirectoryName = getCanonicalFileName(directoryName); + return startsWith(canonicalFileName, canonicalDirectoryName + "/") || startsWith(canonicalFileName, canonicalDirectoryName + "\\"); + } + export function isUrl(path: string) { return getEncodedRootLength(path) < 0; } diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index bd1a50ac5f926..2c96e1f796af1 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -275,7 +275,7 @@ namespace ts.codefix { ): ReadonlyArray { const isJs = isSourceFileJavaScript(sourceFile); const choicesForEachExportingModule = flatMap>(moduleSymbols, ({ moduleSymbol, importKind, exportedSymbolIsTypeOnly }) => { - const modulePathsGroups = moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getCompilerOptions(), sourceFile, host, program.getSourceFiles(), preferences); + const modulePathsGroups = moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getCompilerOptions(), sourceFile, host, program.getSourceFiles(), preferences, program.redirectTargetsMap); return modulePathsGroups.map(group => group.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.assertDefined(position) } : { kind: ImportFixKind.AddNew, moduleSpecifier, importKind })); diff --git a/src/services/documentHighlights.ts b/src/services/documentHighlights.ts index 6976cd2ea2001..f0f3c165d7eab 100644 --- a/src/services/documentHighlights.ts +++ b/src/services/documentHighlights.ts @@ -28,7 +28,7 @@ namespace ts.DocumentHighlights { const map = arrayToMultiMap(referenceEntries.map(FindAllReferences.toHighlightSpan), e => e.fileName, e => e.span); return arrayFrom(map.entries(), ([fileName, highlightSpans]) => { if (!sourceFilesSet.has(fileName)) { - Debug.assert(program.redirectTargetsSet.has(fileName)); + Debug.assert(program.redirectTargetsMap.has(fileName)); const redirectTarget = program.getSourceFile(fileName); const redirect = find(sourceFilesToSearch, f => !!f.redirectInfo && f.redirectInfo.redirectTarget === redirectTarget)!; fileName = redirect.fileName; diff --git a/src/services/getEditsForFileRename.ts b/src/services/getEditsForFileRename.ts index 75dd608e6c475..bd00ce5dd9e23 100644 --- a/src/services/getEditsForFileRename.ts +++ b/src/services/getEditsForFileRename.ts @@ -156,7 +156,7 @@ namespace ts { // Need an update if the imported file moved, or the importing file moved and was using a relative path. return toImport !== undefined && (toImport.updated || (importingSourceFileMoved && pathIsRelative(importLiteral.text))) - ? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, allFiles, preferences) + ? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, allFiles, preferences, program.redirectTargetsMap) : undefined; }); } diff --git a/src/testRunner/unittests/tsserverProjectSystem.ts b/src/testRunner/unittests/tsserverProjectSystem.ts index 1eab4e38aeb6b..d59af8d98b6b9 100644 --- a/src/testRunner/unittests/tsserverProjectSystem.ts +++ b/src/testRunner/unittests/tsserverProjectSystem.ts @@ -9528,6 +9528,59 @@ export function Test2() { }); }); + describe("duplicate packages", () => { + // Tests that 'moduleSpecifiers.ts' will import from the redirecting file, and not from the file it redirects to, if that can provide a global module specifier. + it("works with import fixes", () => { + const packageContent = "export const foo: number;"; + const packageJsonContent = JSON.stringify({ name: "foo", version: "1.2.3" }); + const aFooIndex: File = { path: "/a/node_modules/foo/index.d.ts", content: packageContent }; + const aFooPackage: File = { path: "/a/node_modules/foo/package.json", content: packageJsonContent }; + const bFooIndex: File = { path: "/b/node_modules/foo/index.d.ts", content: packageContent }; + const bFooPackage: File = { path: "/b/node_modules/foo/package.json", content: packageJsonContent }; + + const userContent = 'import("foo");\nfoo'; + const aUser: File = { path: "/a/user.ts", content: userContent }; + const bUser: File = { path: "/b/user.ts", content: userContent }; + const tsconfig: File = { + path: "/tsconfig.json", + content: "{}", + }; + + const host = createServerHost([aFooIndex, aFooPackage, bFooIndex, bFooPackage, aUser, bUser, tsconfig]); + const session = createSession(host); + + openFilesForSession([aUser, bUser], session); + + for (const user of [aUser, bUser]) { + const response = executeSessionRequest(session, protocol.CommandTypes.GetCodeFixes, { + file: user.path, + startLine: 2, + startOffset: 1, + endLine: 2, + endOffset: 4, + errorCodes: [Diagnostics.Cannot_find_name_0.code], + }); + assert.deepEqual | undefined>(response, [ + { + description: `Import 'foo' from module "foo"`, + fixName: "import", + fixId: "fixMissingImport", + fixAllDescription: "Add all missing imports", + changes: [{ + fileName: user.path, + textChanges: [{ + start: { line: 1, offset: 1 }, + end: { line: 1, offset: 1 }, + newText: 'import { foo } from "foo";\n\n', + }], + }], + commands: undefined, + }, + ]); + } + }); + }); + describe("tsserverProjectSystem Untitled files", () => { it("Can convert positions to locations", () => { const aTs: File = { path: "/proj/a.ts", content: "" }; diff --git a/tests/cases/fourslash/importNameCodeFix_symlink_own_package.ts b/tests/cases/fourslash/importNameCodeFix_symlink_own_package.ts new file mode 100644 index 0000000000000..67dfd71fff1b6 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFix_symlink_own_package.ts @@ -0,0 +1,21 @@ +/// + +// @Filename: /packages/b/b0.ts +// @Symlink: /node_modules/b/b0.ts +////x; + +// @Filename: /packages/b/b1.ts +// @Symlink: /node_modules/b/b1.ts +////import { a } from "a"; +////export const x = 0; + +// @Filename: /packages/a/index.d.ts +// @Symlink: /node_modules/a/index.d.ts +////export const a: number; + +goTo.file("/packages/b/b0.ts"); +verify.importFixAtPosition([ +`import { x } from "./b1"; + +x;`, +]); diff --git a/tests/cases/fourslash/importNameCodeFix_symlink_own_package_2.ts b/tests/cases/fourslash/importNameCodeFix_symlink_own_package_2.ts new file mode 100644 index 0000000000000..f1dec143d34e2 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFix_symlink_own_package_2.ts @@ -0,0 +1,17 @@ +/// + +// @Filename: /packages/a/test.ts +// @Symlink: /node_modules/a/test.ts +////x; + +// @Filename: /packages/a/utils.ts +// @Symlink: /node_modules/a/utils.ts +////import {} from "a/utils"; +////export const x = 0; + +goTo.file("/packages/a/test.ts"); +verify.importFixAtPosition([ +`import { x } from "./utils"; + +x;`, +]);