From c82c9a9744bc799d020579eeca5e19935e603427 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Wed, 3 Aug 2022 13:58:15 -0700 Subject: [PATCH] Fix bugs in module specifier generation with `paths`/`typesVersions` (#49792) * Write a test and a huge comment * Finish fixing everything * Clean up comment * Remove obsolete comment * Fix comment trailing off * Optimize to hit the file system much less --- src/compiler/moduleSpecifiers.ts | 138 ++++++++++++++---- .../importNameCodeFix_pathsWithExtension.ts | 24 +++ .../importNameCodeFix_typesVersions.ts | 11 +- 3 files changed, 141 insertions(+), 32 deletions(-) create mode 100644 tests/cases/fourslash/importNameCodeFix_pathsWithExtension.ts diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts index 44d20c9ff59..e38dd465985 100644 --- a/src/compiler/moduleSpecifiers.ts +++ b/src/compiler/moduleSpecifiers.ts @@ -121,7 +121,7 @@ namespace ts.moduleSpecifiers { const info = getInfo(importingSourceFileName, host); const modulePaths = getAllModulePaths(importingSourceFileName, toFileName, host, userPreferences, options); return firstDefined(modulePaths, modulePath => tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, userPreferences, /*packageNameOnly*/ undefined, options.overrideImportMode)) || - getLocalModuleSpecifier(toFileName, info, compilerOptions, host, preferences); + getLocalModuleSpecifier(toFileName, info, compilerOptions, host, options.overrideImportMode || importingSourceFile.impliedNodeFormat, preferences); } export function tryGetModuleSpecifiersFromCache( @@ -257,7 +257,7 @@ namespace ts.moduleSpecifiers { } if (!specifier && !modulePath.isRedirect) { - const local = getLocalModuleSpecifier(modulePath.path, info, compilerOptions, host, preferences); + const local = getLocalModuleSpecifier(modulePath.path, info, compilerOptions, host, options.overrideImportMode || importingSourceFile.impliedNodeFormat, preferences); if (pathIsBareSpecifier(local)) { pathsSpecifiers = append(pathsSpecifiers, local); } @@ -293,7 +293,7 @@ namespace ts.moduleSpecifiers { return { getCanonicalFileName, importingSourceFileName, sourceDirectory }; } - function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, { ending, relativePreference }: Preferences): string { + function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, importMode: SourceFile["impliedNodeFormat"], { ending, relativePreference }: Preferences): string { const { baseUrl, paths, rootDirs } = compilerOptions; const { sourceDirectory, getCanonicalFileName } = info; const relativePath = rootDirs && tryGetModuleNameFromRootDirs(rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName, ending, compilerOptions) || @@ -308,9 +308,8 @@ namespace ts.moduleSpecifiers { return relativePath; } - const importRelativeToBaseUrl = removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions); - const fromPaths = paths && tryGetModuleNameFromPaths(removeFileExtension(relativeToBaseUrl), importRelativeToBaseUrl, paths); - const nonRelative = fromPaths === undefined && baseUrl !== undefined ? importRelativeToBaseUrl : fromPaths; + const fromPaths = paths && tryGetModuleNameFromPaths(relativeToBaseUrl, paths, getAllowedEndings(ending, compilerOptions, importMode), host, compilerOptions); + const nonRelative = fromPaths === undefined && baseUrl !== undefined ? removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions) : fromPaths; if (!nonRelative) { return relativePath; } @@ -559,27 +558,100 @@ namespace ts.moduleSpecifiers { } } - function tryGetModuleNameFromPaths(relativeToBaseUrlWithIndex: string, relativeToBaseUrl: string, paths: MapLike): string | undefined { + function getAllowedEndings(preferredEnding: Ending, compilerOptions: CompilerOptions, importMode: SourceFile["impliedNodeFormat"]) { + if (getEmitModuleResolutionKind(compilerOptions) >= ModuleResolutionKind.Node16 && importMode === ModuleKind.ESNext) { + return [Ending.JsExtension]; + } + switch (preferredEnding) { + case Ending.JsExtension: return [Ending.JsExtension, Ending.Minimal, Ending.Index]; + case Ending.Index: return [Ending.Index, Ending.Minimal, Ending.JsExtension]; + case Ending.Minimal: return [Ending.Minimal, Ending.Index, Ending.JsExtension]; + default: Debug.assertNever(preferredEnding); + } + } + + function tryGetModuleNameFromPaths(relativeToBaseUrl: string, paths: MapLike, allowedEndings: Ending[], host: ModuleSpecifierResolutionHost, compilerOptions: CompilerOptions): string | undefined { for (const key in paths) { for (const patternText of paths[key]) { - const pattern = removeFileExtension(normalizePath(patternText)); + const pattern = normalizePath(patternText); const indexOfStar = pattern.indexOf("*"); + // In module resolution, if `pattern` itself has an extension, a file with that extension is looked up directly, + // meaning a '.ts' or '.d.ts' extension is allowed to resolve. This is distinct from the case where a '*' substitution + // causes a module specifier to have an extension, i.e. the extension comes from the module specifier in a JS/TS file + // and matches the '*'. For example: + // + // Module Specifier | Path Mapping (key: [pattern]) | Interpolation | Resolution Action + // ---------------------->------------------------------->--------------------->--------------------------------------------------------------- + // import "@app/foo" -> "@app/*": ["./src/app/*.ts"] -> "./src/app/foo.ts" -> tryFile("./src/app/foo.ts") || [continue resolution algorithm] + // import "@app/foo.ts" -> "@app/*": ["./src/app/*"] -> "./src/app/foo.ts" -> [continue resolution algorithm] + // + // (https://github.com/microsoft/TypeScript/blob/ad4ded80e1d58f0bf36ac16bea71bc10d9f09895/src/compiler/moduleNameResolver.ts#L2509-L2516) + // + // The interpolation produced by both scenarios is identical, but only in the former, where the extension is encoded in + // the path mapping rather than in the module specifier, will we prioritize a file lookup on the interpolation result. + // (In fact, currently, the latter scenario will necessarily fail since no resolution mode recognizes '.ts' as a valid + // extension for a module specifier.) + // + // Here, this means we need to be careful about whether we generate a match from the target filename (typically with a + // .ts extension) or the possible relative module specifiers representing that file: + // + // Filename | Relative Module Specifier Candidates | Path Mapping | Filename Result | Module Specifier Results + // --------------------<----------------------------------------------<------------------------------<-------------------||---------------------------- + // dist/haha.d.ts <- dist/haha, dist/haha.js <- "@app/*": ["./dist/*.d.ts"] <- @app/haha || (none) + // dist/haha.d.ts <- dist/haha, dist/haha.js <- "@app/*": ["./dist/*"] <- (none) || @app/haha, @app/haha.js + // dist/foo/index.d.ts <- dist/foo, dist/foo/index, dist/foo/index.js <- "@app/*": ["./dist/*.d.ts"] <- @app/foo/index || (none) + // dist/foo/index.d.ts <- dist/foo, dist/foo/index, dist/foo/index.js <- "@app/*": ["./dist/*"] <- (none) || @app/foo, @app/foo/index, @app/foo/index.js + // dist/wow.js.js <- dist/wow.js, dist/wow.js.js <- "@app/*": ["./dist/*.js"] <- @app/wow.js || @app/wow, @app/wow.js + // + // The "Filename Result" can be generated only if `pattern` has an extension. Care must be taken that the list of + // relative module specifiers to run the interpolation (a) is actually valid for the module resolution mode, (b) takes + // into account the existence of other files (e.g. 'dist/wow.js' cannot refer to 'dist/wow.js.js' if 'dist/wow.js' + // exists) and (c) that they are ordered by preference. The last row shows that the filename result and module + // specifier results are not mutually exclusive. Note that the filename result is a higher priority in module + // resolution, but as long criteria (b) above is met, I don't think its result needs to be the highest priority result + // in module specifier generation. I have included it last, as it's difficult to tell exactly where it should be + // sorted among the others for a particular value of `importModuleSpecifierEnding`. + const candidates: { ending: Ending | undefined, value: string }[] = allowedEndings.map(ending => ({ + ending, + value: removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions) + })); + if (tryGetExtensionFromPath(pattern)) { + candidates.push({ ending: undefined, value: relativeToBaseUrl }); + } + if (indexOfStar !== -1) { - const prefix = pattern.substr(0, indexOfStar); - const suffix = pattern.substr(indexOfStar + 1); - if (relativeToBaseUrl.length >= prefix.length + suffix.length && - startsWith(relativeToBaseUrl, prefix) && - endsWith(relativeToBaseUrl, suffix) || - !suffix && relativeToBaseUrl === removeTrailingDirectorySeparator(prefix)) { - const matchedStar = relativeToBaseUrl.substr(prefix.length, relativeToBaseUrl.length - suffix.length - prefix.length); - return key.replace("*", matchedStar); + const prefix = pattern.substring(0, indexOfStar); + const suffix = pattern.substring(indexOfStar + 1); + for (const { ending, value } of candidates) { + if (value.length >= prefix.length + suffix.length && + startsWith(value, prefix) && + endsWith(value, suffix) && + validateEnding({ ending, value }) + ) { + const matchedStar = value.substring(prefix.length, value.length - suffix.length); + return key.replace("*", matchedStar); + } } } - else if (pattern === relativeToBaseUrl || pattern === relativeToBaseUrlWithIndex) { + else if ( + some(candidates, c => c.ending !== Ending.Minimal && pattern === c.value) || + some(candidates, c => c.ending === Ending.Minimal && pattern === c.value && validateEnding(c)) + ) { return key; } } } + + function validateEnding({ ending, value }: { ending: Ending | undefined, value: string }) { + // Optimization: `removeExtensionAndIndexPostFix` can query the file system (a good bit) if `ending` is `Minimal`, the basename + // is 'index', and a `host` is provided. To avoid that until it's unavoidable, we ran the function with no `host` above. Only + // here, after we've checked that the minimal ending is indeed a match (via the length and prefix/suffix checks / `some` calls), + // do we check that the host-validated result is consistent with the answer we got before. If it's not, it falls back to the + // `Ending.Index` result, which should already be in the list of candidates if `Minimal` was. (Note: the assumption here is + // that every module resolution mode that supports dropping extensions also supports dropping `/index`. Like literally + // everything else in this file, this logic needs to be updated if that's not true in some future module resolution mode.) + return ending !== Ending.Minimal || value === removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions, host); + } } const enum MatchingMode { @@ -677,10 +749,10 @@ namespace ts.moduleSpecifiers { // Simplify the full file path to something that can be resolved by Node. + const preferences = getPreferences(host, userPreferences, options, importingSourceFile); let moduleSpecifier = path; let isPackageRootPath = false; if (!packageNameOnly) { - const preferences = getPreferences(host, userPreferences, options, importingSourceFile); let packageRootIndex = parts.packageRootIndex; let moduleFileName: string | undefined; while (true) { @@ -732,15 +804,13 @@ namespace ts.moduleSpecifiers { const packageRootPath = path.substring(0, packageRootIndex); const packageJsonPath = combinePaths(packageRootPath, "package.json"); let moduleFileToTry = path; + let maybeBlockedByTypesVersions = false; const cachedPackageJson = host.getPackageJsonInfoCache?.()?.getPackageJsonInfo(packageJsonPath); if (typeof cachedPackageJson === "object" || cachedPackageJson === undefined && host.fileExists(packageJsonPath)) { const packageJsonContent = cachedPackageJson?.packageJsonContent || JSON.parse(host.readFile!(packageJsonPath)!); + const importMode = overrideMode || importingSourceFile.impliedNodeFormat; if (getEmitModuleResolutionKind(options) === ModuleResolutionKind.Node16 || getEmitModuleResolutionKind(options) === ModuleResolutionKind.NodeNext) { - // `conditions` *could* be made to go against `importingSourceFile.impliedNodeFormat` if something wanted to generate - // an ImportEqualsDeclaration in an ESM-implied file or an ImportCall in a CJS-implied file. But since this function is - // usually called to conjure an import out of thin air, we don't have an existing usage to call `getModeForUsageAtIndex` - // with, so for now we just stick with the mode of the file. - const conditions = ["node", overrideMode || importingSourceFile.impliedNodeFormat === ModuleKind.ESNext ? "import" : "require", "types"]; + const conditions = ["node", importMode === ModuleKind.ESNext ? "import" : "require", "types"]; const fromExports = packageJsonContent.exports && typeof packageJsonContent.name === "string" ? tryGetModuleNameFromExports(options, path, packageRootPath, getPackageNameFromTypesPackageName(packageJsonContent.name), packageJsonContent.exports, conditions) : undefined; @@ -760,19 +830,31 @@ namespace ts.moduleSpecifiers { if (versionPaths) { const subModuleName = path.slice(packageRootPath.length + 1); const fromPaths = tryGetModuleNameFromPaths( - removeFileExtension(subModuleName), - removeExtensionAndIndexPostFix(subModuleName, Ending.Minimal, options), - versionPaths.paths + subModuleName, + versionPaths.paths, + getAllowedEndings(preferences.ending, options, importMode), + host, + options ); - if (fromPaths !== undefined) { + if (fromPaths === undefined) { + maybeBlockedByTypesVersions = true; + } + else { moduleFileToTry = combinePaths(packageRootPath, fromPaths); } } // If the file is the main module, it can be imported by the package name const mainFileRelative = packageJsonContent.typings || packageJsonContent.types || packageJsonContent.main || "index.js"; - if (isString(mainFileRelative)) { + if (isString(mainFileRelative) && !(maybeBlockedByTypesVersions && matchPatternOrExact(tryParsePatterns(versionPaths!.paths), mainFileRelative))) { + // The 'main' file is also subject to mapping through typesVersions, and we couldn't come up with a path + // explicitly through typesVersions, so if it matches a key in typesVersions now, it's not reachable. + // (The only way this can happen is if some file in a package that's not resolvable from outside the + // package got pulled into the program anyway, e.g. transitively through a file that *is* reachable. It + // happens very easily in fourslash tests though, since every test file listed gets included. See + // importNameCodeFix_typesVersions.ts for an example.) const mainExportFile = toPath(mainFileRelative, packageRootPath, getCanonicalFileName); if (removeFileExtension(mainExportFile) === removeFileExtension(getCanonicalFileName(moduleFileToTry))) { + // ^ An arbitrary removal of file extension for this comparison is almost certainly wrong return { packageRootPath, moduleFileToTry }; } } diff --git a/tests/cases/fourslash/importNameCodeFix_pathsWithExtension.ts b/tests/cases/fourslash/importNameCodeFix_pathsWithExtension.ts new file mode 100644 index 00000000000..b3cd69844ae --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFix_pathsWithExtension.ts @@ -0,0 +1,24 @@ +/// + +// @Filename: /tsconfig.json +//// { +//// "compilerOptions": { +//// "target": "ESNext", +//// "module": "Node16", +//// "moduleResolution": "Node16", +//// "rootDir": "./src", +//// "outDir": "./dist", +//// "paths": { +//// "#internals/*": ["./src/internals/*.ts"] +//// } +//// }, +//// "include": ["src"] +//// } + +// @Filename: /src/internals/example.ts +//// export function helloWorld() {} + +// @Filename: /src/index.ts +//// helloWorld/**/ + +verify.importFixModuleSpecifiers("", ["#internals/example"], { importModuleSpecifierEnding: "js" }); diff --git a/tests/cases/fourslash/importNameCodeFix_typesVersions.ts b/tests/cases/fourslash/importNameCodeFix_typesVersions.ts index 9f69e3eb44f..185461353d2 100644 --- a/tests/cases/fourslash/importNameCodeFix_typesVersions.ts +++ b/tests/cases/fourslash/importNameCodeFix_typesVersions.ts @@ -6,17 +6,17 @@ // @Filename: /node_modules/unified/package.json //// { //// "name": "unified", -//// "types": "types/ts3.4/index.d.ts", +//// "types": "types/ts3.444/index.d.ts", //// "typesVersions": { //// ">=4.0": { -//// "types/ts3.4/*": [ +//// "types/ts3.444/*": [ //// "types/ts4.0/*" //// ] //// } //// } //// } -// @Filename: /node_modules/unified/types/ts3.4/index.d.ts +// @Filename: /node_modules/unified/types/ts3.444/index.d.ts //// export declare const x: number; // @Filename: /node_modules/unified/types/ts4.0/index.d.ts @@ -30,5 +30,8 @@ verify.importFixModuleSpecifiers("", [ "unified", - "unified/types/ts3.4/", // TODO: this is wrong #49034 + // This obviously doesn't look like a desired module specifier, but the package.json is misconfigured + // (taken from a real-world example). The fact that it resolves (according to TS) is good enough to + // generate it. + "unified/types/ts3.444/index.js", ], { importModuleSpecifierEnding: "js" });