Skip to content

Commit c82c9a9

Browse files
andrewbranchandrewbranch
authored
Fix bugs in module specifier generation with paths/typesVersions (microsoft#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
1 parent 59c91f6 commit c82c9a9

File tree

3 files changed

+141
-32
lines changed

3 files changed

+141
-32
lines changed

‎src/compiler/moduleSpecifiers.ts

Lines changed: 110 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ namespace ts.moduleSpecifiers {
121121
const info = getInfo(importingSourceFileName, host);
122122
const modulePaths = getAllModulePaths(importingSourceFileName, toFileName, host, userPreferences, options);
123123
return firstDefined(modulePaths, modulePath => tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, userPreferences, /*packageNameOnly*/ undefined, options.overrideImportMode)) ||
124-
getLocalModuleSpecifier(toFileName, info, compilerOptions, host, preferences);
124+
getLocalModuleSpecifier(toFileName, info, compilerOptions, host, options.overrideImportMode || importingSourceFile.impliedNodeFormat, preferences);
125125
}
126126

127127
export function tryGetModuleSpecifiersFromCache(
@@ -257,7 +257,7 @@ namespace ts.moduleSpecifiers {
257257
}
258258

259259
if (!specifier && !modulePath.isRedirect) {
260-
const local = getLocalModuleSpecifier(modulePath.path, info, compilerOptions, host, preferences);
260+
const local = getLocalModuleSpecifier(modulePath.path, info, compilerOptions, host, options.overrideImportMode || importingSourceFile.impliedNodeFormat, preferences);
261261
if (pathIsBareSpecifier(local)) {
262262
pathsSpecifiers = append(pathsSpecifiers, local);
263263
}
@@ -293,7 +293,7 @@ namespace ts.moduleSpecifiers {
293293
return { getCanonicalFileName, importingSourceFileName, sourceDirectory };
294294
}
295295

296-
function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, { ending, relativePreference }: Preferences): string {
296+
function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, importMode: SourceFile["impliedNodeFormat"], { ending, relativePreference }: Preferences): string {
297297
const { baseUrl, paths, rootDirs } = compilerOptions;
298298
const { sourceDirectory, getCanonicalFileName } = info;
299299
const relativePath = rootDirs && tryGetModuleNameFromRootDirs(rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName, ending, compilerOptions) ||
@@ -308,9 +308,8 @@ namespace ts.moduleSpecifiers {
308308
return relativePath;
309309
}
310310

311-
const importRelativeToBaseUrl = removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions);
312-
const fromPaths = paths && tryGetModuleNameFromPaths(removeFileExtension(relativeToBaseUrl), importRelativeToBaseUrl, paths);
313-
const nonRelative = fromPaths === undefined && baseUrl !== undefined ? importRelativeToBaseUrl : fromPaths;
311+
const fromPaths = paths && tryGetModuleNameFromPaths(relativeToBaseUrl, paths, getAllowedEndings(ending, compilerOptions, importMode), host, compilerOptions);
312+
const nonRelative = fromPaths === undefined && baseUrl !== undefined ? removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions) : fromPaths;
314313
if (!nonRelative) {
315314
return relativePath;
316315
}
@@ -559,27 +558,100 @@ namespace ts.moduleSpecifiers {
559558
}
560559
}
561560

562-
function tryGetModuleNameFromPaths(relativeToBaseUrlWithIndex: string, relativeToBaseUrl: string, paths: MapLike<readonly string[]>): string | undefined {
561+
function getAllowedEndings(preferredEnding: Ending, compilerOptions: CompilerOptions, importMode: SourceFile["impliedNodeFormat"]) {
562+
if (getEmitModuleResolutionKind(compilerOptions) >= ModuleResolutionKind.Node16 && importMode === ModuleKind.ESNext) {
563+
return [Ending.JsExtension];
564+
}
565+
switch (preferredEnding) {
566+
case Ending.JsExtension: return [Ending.JsExtension, Ending.Minimal, Ending.Index];
567+
case Ending.Index: return [Ending.Index, Ending.Minimal, Ending.JsExtension];
568+
case Ending.Minimal: return [Ending.Minimal, Ending.Index, Ending.JsExtension];
569+
default: Debug.assertNever(preferredEnding);
570+
}
571+
}
572+
573+
function tryGetModuleNameFromPaths(relativeToBaseUrl: string, paths: MapLike<readonly string[]>, allowedEndings: Ending[], host: ModuleSpecifierResolutionHost, compilerOptions: CompilerOptions): string | undefined {
563574
for (const key in paths) {
564575
for (const patternText of paths[key]) {
565-
const pattern = removeFileExtension(normalizePath(patternText));
576+
const pattern = normalizePath(patternText);
566577
const indexOfStar = pattern.indexOf("*");
578+
// In module resolution, if `pattern` itself has an extension, a file with that extension is looked up directly,
579+
// meaning a '.ts' or '.d.ts' extension is allowed to resolve. This is distinct from the case where a '*' substitution
580+
// causes a module specifier to have an extension, i.e. the extension comes from the module specifier in a JS/TS file
581+
// and matches the '*'. For example:
582+
//
583+
// Module Specifier | Path Mapping (key: [pattern]) | Interpolation | Resolution Action
584+
// ---------------------->------------------------------->--------------------->---------------------------------------------------------------
585+
// import "@app/foo" -> "@app/*": ["./src/app/*.ts"] -> "./src/app/foo.ts" -> tryFile("./src/app/foo.ts") || [continue resolution algorithm]
586+
// import "@app/foo.ts" -> "@app/*": ["./src/app/*"] -> "./src/app/foo.ts" -> [continue resolution algorithm]
587+
//
588+
// (https://github.com/microsoft/TypeScript/blob/ad4ded80e1d58f0bf36ac16bea71bc10d9f09895/src/compiler/moduleNameResolver.ts#L2509-L2516)
589+
//
590+
// The interpolation produced by both scenarios is identical, but only in the former, where the extension is encoded in
591+
// the path mapping rather than in the module specifier, will we prioritize a file lookup on the interpolation result.
592+
// (In fact, currently, the latter scenario will necessarily fail since no resolution mode recognizes '.ts' as a valid
593+
// extension for a module specifier.)
594+
//
595+
// Here, this means we need to be careful about whether we generate a match from the target filename (typically with a
596+
// .ts extension) or the possible relative module specifiers representing that file:
597+
//
598+
// Filename | Relative Module Specifier Candidates | Path Mapping | Filename Result | Module Specifier Results
599+
// --------------------<----------------------------------------------<------------------------------<-------------------||----------------------------
600+
// dist/haha.d.ts <- dist/haha, dist/haha.js <- "@app/*": ["./dist/*.d.ts"] <- @app/haha || (none)
601+
// dist/haha.d.ts <- dist/haha, dist/haha.js <- "@app/*": ["./dist/*"] <- (none) || @app/haha, @app/haha.js
602+
// dist/foo/index.d.ts <- dist/foo, dist/foo/index, dist/foo/index.js <- "@app/*": ["./dist/*.d.ts"] <- @app/foo/index || (none)
603+
// 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
604+
// dist/wow.js.js <- dist/wow.js, dist/wow.js.js <- "@app/*": ["./dist/*.js"] <- @app/wow.js || @app/wow, @app/wow.js
605+
//
606+
// The "Filename Result" can be generated only if `pattern` has an extension. Care must be taken that the list of
607+
// relative module specifiers to run the interpolation (a) is actually valid for the module resolution mode, (b) takes
608+
// into account the existence of other files (e.g. 'dist/wow.js' cannot refer to 'dist/wow.js.js' if 'dist/wow.js'
609+
// exists) and (c) that they are ordered by preference. The last row shows that the filename result and module
610+
// specifier results are not mutually exclusive. Note that the filename result is a higher priority in module
611+
// resolution, but as long criteria (b) above is met, I don't think its result needs to be the highest priority result
612+
// in module specifier generation. I have included it last, as it's difficult to tell exactly where it should be
613+
// sorted among the others for a particular value of `importModuleSpecifierEnding`.
614+
const candidates: { ending: Ending | undefined, value: string }[] = allowedEndings.map(ending => ({
615+
ending,
616+
value: removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions)
617+
}));
618+
if (tryGetExtensionFromPath(pattern)) {
619+
candidates.push({ ending: undefined, value: relativeToBaseUrl });
620+
}
621+
567622
if (indexOfStar !== -1) {
568-
const prefix = pattern.substr(0, indexOfStar);
569-
const suffix = pattern.substr(indexOfStar + 1);
570-
if (relativeToBaseUrl.length >= prefix.length + suffix.length &&
571-
startsWith(relativeToBaseUrl, prefix) &&
572-
endsWith(relativeToBaseUrl, suffix) ||
573-
!suffix && relativeToBaseUrl === removeTrailingDirectorySeparator(prefix)) {
574-
const matchedStar = relativeToBaseUrl.substr(prefix.length, relativeToBaseUrl.length - suffix.length - prefix.length);
575-
return key.replace("*", matchedStar);
623+
const prefix = pattern.substring(0, indexOfStar);
624+
const suffix = pattern.substring(indexOfStar + 1);
625+
for (const { ending, value } of candidates) {
626+
if (value.length >= prefix.length + suffix.length &&
627+
startsWith(value, prefix) &&
628+
endsWith(value, suffix) &&
629+
validateEnding({ ending, value })
630+
) {
631+
const matchedStar = value.substring(prefix.length, value.length - suffix.length);
632+
return key.replace("*", matchedStar);
633+
}
576634
}
577635
}
578-
else if (pattern === relativeToBaseUrl || pattern === relativeToBaseUrlWithIndex) {
636+
else if (
637+
some(candidates, c => c.ending !== Ending.Minimal && pattern === c.value) ||
638+
some(candidates, c => c.ending === Ending.Minimal && pattern === c.value && validateEnding(c))
639+
) {
579640
return key;
580641
}
581642
}
582643
}
644+
645+
function validateEnding({ ending, value }: { ending: Ending | undefined, value: string }) {
646+
// Optimization: `removeExtensionAndIndexPostFix` can query the file system (a good bit) if `ending` is `Minimal`, the basename
647+
// is 'index', and a `host` is provided. To avoid that until it's unavoidable, we ran the function with no `host` above. Only
648+
// here, after we've checked that the minimal ending is indeed a match (via the length and prefix/suffix checks / `some` calls),
649+
// 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
650+
// `Ending.Index` result, which should already be in the list of candidates if `Minimal` was. (Note: the assumption here is
651+
// that every module resolution mode that supports dropping extensions also supports dropping `/index`. Like literally
652+
// everything else in this file, this logic needs to be updated if that's not true in some future module resolution mode.)
653+
return ending !== Ending.Minimal || value === removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions, host);
654+
}
583655
}
584656

585657
const enum MatchingMode {
@@ -677,10 +749,10 @@ namespace ts.moduleSpecifiers {
677749

678750
// Simplify the full file path to something that can be resolved by Node.
679751

752+
const preferences = getPreferences(host, userPreferences, options, importingSourceFile);
680753
let moduleSpecifier = path;
681754
let isPackageRootPath = false;
682755
if (!packageNameOnly) {
683-
const preferences = getPreferences(host, userPreferences, options, importingSourceFile);
684756
let packageRootIndex = parts.packageRootIndex;
685757
let moduleFileName: string | undefined;
686758
while (true) {
@@ -732,15 +804,13 @@ namespace ts.moduleSpecifiers {
732804
const packageRootPath = path.substring(0, packageRootIndex);
733805
const packageJsonPath = combinePaths(packageRootPath, "package.json");
734806
let moduleFileToTry = path;
807+
let maybeBlockedByTypesVersions = false;
735808
const cachedPackageJson = host.getPackageJsonInfoCache?.()?.getPackageJsonInfo(packageJsonPath);
736809
if (typeof cachedPackageJson === "object" || cachedPackageJson === undefined && host.fileExists(packageJsonPath)) {
737810
const packageJsonContent = cachedPackageJson?.packageJsonContent || JSON.parse(host.readFile!(packageJsonPath)!);
811+
const importMode = overrideMode || importingSourceFile.impliedNodeFormat;
738812
if (getEmitModuleResolutionKind(options) === ModuleResolutionKind.Node16 || getEmitModuleResolutionKind(options) === ModuleResolutionKind.NodeNext) {
739-
// `conditions` *could* be made to go against `importingSourceFile.impliedNodeFormat` if something wanted to generate
740-
// an ImportEqualsDeclaration in an ESM-implied file or an ImportCall in a CJS-implied file. But since this function is
741-
// usually called to conjure an import out of thin air, we don't have an existing usage to call `getModeForUsageAtIndex`
742-
// with, so for now we just stick with the mode of the file.
743-
const conditions = ["node", overrideMode || importingSourceFile.impliedNodeFormat === ModuleKind.ESNext ? "import" : "require", "types"];
813+
const conditions = ["node", importMode === ModuleKind.ESNext ? "import" : "require", "types"];
744814
const fromExports = packageJsonContent.exports && typeof packageJsonContent.name === "string"
745815
? tryGetModuleNameFromExports(options, path, packageRootPath, getPackageNameFromTypesPackageName(packageJsonContent.name), packageJsonContent.exports, conditions)
746816
: undefined;
@@ -760,19 +830,31 @@ namespace ts.moduleSpecifiers {
760830
if (versionPaths) {
761831
const subModuleName = path.slice(packageRootPath.length + 1);
762832
const fromPaths = tryGetModuleNameFromPaths(
763-
removeFileExtension(subModuleName),
764-
removeExtensionAndIndexPostFix(subModuleName, Ending.Minimal, options),
765-
versionPaths.paths
833+
subModuleName,
834+
versionPaths.paths,
835+
getAllowedEndings(preferences.ending, options, importMode),
836+
host,
837+
options
766838
);
767-
if (fromPaths !== undefined) {
839+
if (fromPaths === undefined) {
840+
maybeBlockedByTypesVersions = true;
841+
}
842+
else {
768843
moduleFileToTry = combinePaths(packageRootPath, fromPaths);
769844
}
770845
}
771846
// If the file is the main module, it can be imported by the package name
772847
const mainFileRelative = packageJsonContent.typings || packageJsonContent.types || packageJsonContent.main || "index.js";
773-
if (isString(mainFileRelative)) {
848+
if (isString(mainFileRelative) && !(maybeBlockedByTypesVersions && matchPatternOrExact(tryParsePatterns(versionPaths!.paths), mainFileRelative))) {
849+
// The 'main' file is also subject to mapping through typesVersions, and we couldn't come up with a path
850+
// explicitly through typesVersions, so if it matches a key in typesVersions now, it's not reachable.
851+
// (The only way this can happen is if some file in a package that's not resolvable from outside the
852+
// package got pulled into the program anyway, e.g. transitively through a file that *is* reachable. It
853+
// happens very easily in fourslash tests though, since every test file listed gets included. See
854+
// importNameCodeFix_typesVersions.ts for an example.)
774855
const mainExportFile = toPath(mainFileRelative, packageRootPath, getCanonicalFileName);
775856
if (removeFileExtension(mainExportFile) === removeFileExtension(getCanonicalFileName(moduleFileToTry))) {
857+
// ^ An arbitrary removal of file extension for this comparison is almost certainly wrong
776858
return { packageRootPath, moduleFileToTry };
777859
}
778860
}

‎tests/cases/fourslash/importNameCodeFix_pathsWithExtension.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @Filename: /tsconfig.json
4+
//// {
5+
//// "compilerOptions": {
6+
//// "target": "ESNext",
7+
//// "module": "Node16",
8+
//// "moduleResolution": "Node16",
9+
//// "rootDir": "./src",
10+
//// "outDir": "./dist",
11+
//// "paths": {
12+
//// "#internals/*": ["./src/internals/*.ts"]
13+
//// }
14+
//// },
15+
//// "include": ["src"]
16+
//// }
17+
18+
// @Filename: /src/internals/example.ts
19+
//// export function helloWorld() {}
20+
21+
// @Filename: /src/index.ts
22+
//// helloWorld/**/
23+
24+
verify.importFixModuleSpecifiers("", ["#internals/example"], { importModuleSpecifierEnding: "js" });

‎tests/cases/fourslash/importNameCodeFix_typesVersions.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,17 @@
66
// @Filename: /node_modules/unified/package.json
77
//// {
88
//// "name": "unified",
9-
//// "types": "types/ts3.4/index.d.ts",
9+
//// "types": "types/ts3.444/index.d.ts",
1010
//// "typesVersions": {
1111
//// ">=4.0": {
12-
//// "types/ts3.4/*": [
12+
//// "types/ts3.444/*": [
1313
//// "types/ts4.0/*"
1414
//// ]
1515
//// }
1616
//// }
1717
//// }
1818

19-
// @Filename: /node_modules/unified/types/ts3.4/index.d.ts
19+
// @Filename: /node_modules/unified/types/ts3.444/index.d.ts
2020
//// export declare const x: number;
2121

2222
// @Filename: /node_modules/unified/types/ts4.0/index.d.ts
@@ -30,5 +30,8 @@
3030

3131
verify.importFixModuleSpecifiers("", [
3232
"unified",
33-
"unified/types/ts3.4/", // TODO: this is wrong #49034
33+
// This obviously doesn't look like a desired module specifier, but the package.json is misconfigured
34+
// (taken from a real-world example). The fact that it resolves (according to TS) is good enough to
35+
// generate it.
36+
"unified/types/ts3.444/index.js",
3437
], { importModuleSpecifierEnding: "js" });

0 commit comments

Comments
 (0)