Skip to content

Fix module resolver bug exposed by malformed package.json import patterns #2680

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andrewbranch merged 16 commits into from
Feb 9, 2026
+90 −2
Merged

Fix module resolver bug exposed by malformed package.json import patterns #2680

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
df286f8
Initial plan
Copilot Feb 4, 2026
a4781f7
Fix crash with malformed package.json imports mapping
Copilot Feb 4, 2026
7dee5c1
Address code review comments
Copilot Feb 4, 2026
0edcafc
Add fix summary documentation
Copilot Feb 4, 2026
2b730dc
Remove investigation docs per code review
Copilot Feb 4, 2026
90aa27c
Fix root cause: resolvedUsingTsExtension should check module specifier
Copilot Feb 4, 2026
ab2810c
Restore panics in checker.go per code review
Copilot Feb 4, 2026
77a5486
Fix resolvedUsingTsExtension to check both conditions
Copilot Feb 4, 2026
934d7c5
Simplify resolvedUsingTsExtension logic per code review
Copilot Feb 4, 2026
eb80f5f
Add test baselines for packageJsonImportsMalformedPattern
Copilot Feb 6, 2026
c3031cc
Add test for wildcard matching TS extension in module specifier
Copilot Feb 6, 2026
91ff619
Add TryExtractTSExtensionLoose for best-effort extraction
Copilot Feb 7, 2026
60d385b
Move tryExtractTSExtensionLoose to checker.go per code review
Copilot Feb 7, 2026
85d2d5e
Address code review: inline helper, add directives, rename tests
Copilot Feb 7, 2026
ef63974
Merge branch 'main' into copilot/fix-package-json-imports
andrewbranch Feb 9, 2026
1c72bd8
Delete unused baseline
andrewbranch Feb 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion internal/module/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1601,10 +1601,13 @@ func (r *resolutionState) loadFileNameFromPackageJSONField(extensions extensions
if extensions&extensionsTypeScript != 0 && tspath.HasImplementationTSFileExtension(candidate) || extensions&extensionsDeclaration != 0 && tspath.IsDeclarationFileName(candidate) {
if path, ok := r.tryFile(candidate, onlyRecordFailures); ok {
extension := tspath.TryExtractTSExtension(path)
// resolvedUsingTsExtension should be true if the original module specifier has a TS extension.
// Check r.name (the original module specifier) rather than packageJSONValue (which may be a pattern).
Copy link
Copy Markdown
Member

@andrewbranch andrewbranch Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolvedUsingTsExtension means the combination of two things:

  1. The module specifier as written literally ends in .ts or similar
  2. That .ts ending was used to match a .ts file on disk, as opposed to matching a package name, directory name, "exports" key or pattern, or whatever. For patterns, it’s tricky, because import "pkg/foo.ts" qualifies under "./*": "./*" but not under "./*.ts": "./*.ts".

This change is wrong because it only checks (1). (There would be no need to implement this logic in the module resolver if that were the only criteria, since it can be trivially checked in the checker.)

The original logic here was trying to say "if we're trying to look up a .ts file but the package.json field we’re looking up didn’t explicitly mention a .ts extension, it must have come from the module specifier mapping onto a pattern in this field." From the failing test, we know that can be only partially true, where part of the extension comes from the module specifier and part comes from the field.

Off the top of my head, it seems like resolvedUsingTsExtension should only be true if both (a) r.name has a TS extension and (b) packageJSONValue ends in *. But you’ll have to verify by running the full test suite.

Copy link
Copy Markdown
Contributor Author

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 77a5486. Now checking both conditions:

  1. r.name has a TS extension (module specifier literally ends in .ts)
  2. packageJSONValue ends with * (extension was used to match, not from the pattern itself)

This correctly distinguishes import "pkg/foo.ts" under pattern ./* from ./*.ts.

Copy link
Copy Markdown
Member

@andrewbranch andrewbranch Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot slight correction, consider mapping import "pkg/foo.ts.omg" through "./*.omg": "./*": this, tragically, should result in resolvedUsingTsExtension: true even though the module specifier does not end in .ts. I think maybe you don’t need to check r.name at all; it might be enough to say that if the field ended in * and the candidate file ended in .ts, that star must have matched a TS extension... if that still leaves some holes, there will need to be a more complicated plumbing through of which part of the pattern matched the module specifier, which we can tackle separately from the crash fix.

Copy link
Copy Markdown
Contributor Author

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 934d7c5. Simplified to only check:

  • Pattern ends with * (packageJSONValue)
  • Candidate file has TS extension

This correctly handles import "pkg/foo.ts.omg" with pattern "./*.omg": "./*" (resolves to true, since * matched .ts) without needing to check r.name.

Copy link
Copy Markdown
Member

@andrewbranch andrewbranch Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot add a new test case for this

Copy link
Copy Markdown
Member

@andrewbranch andrewbranch Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot No, I think we should make an ad-hoc utility function that uses strings.Contains to look for the path.SupportedTSExtensionsFlat in the module specifier to do a best-effort extraction. It's possible that function will misidentify which extension matched the star in an insane case like "foo.ts.mts.cts.oops" but the truth is nobody is ever going to hit this case in real life so it doesn't matter. Also the error message will still say "An import path cannot end with ..." which mislabels the problem in this case, but again, it's not worth carving out a new error message for this. Not crashing + issuing an error is close enough. But we do need an error here.

Copy link
Copy Markdown
Contributor Author

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 91ff619. Created TryExtractTSExtensionLoose that uses strings.Contains to do best-effort extraction. The checker now reports an error for cases like "#/foo.ts.omg" instead of skipping or panicking. The error message says "can only end with" which is slightly misleading, but acceptable for these rare edge cases.

Copy link
Copy Markdown
Member

@andrewbranch andrewbranch Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the name, and I don't want it exported from tspath. Just put it in checker.go near where it's used

Copy link
Copy Markdown
Member

@andrewbranch andrewbranch Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot

Copy link
Copy Markdown
Contributor Author

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 60d385b. Moved the function to checker.go as an unexported helper tryExtractTSExtensionLoose and placed it near where it's used.

resolvedUsingTsExtension := tspath.TryExtractTSExtension(r.name) != ""
return &resolved{
path: path,
extension: extension,
resolvedUsingTsExtension: packageJSONValue != "" && !strings.HasSuffix(packageJSONValue, extension),
resolvedUsingTsExtension: resolvedUsingTsExtension,
}
}
return continueSearching()
Expand Down
30 changes: 30 additions & 0 deletions testdata/tests/cases/compiler/packageJsonImportsMalformedPattern.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// @module: nodenext
// @moduleResolution: nodenext
// @filename: src/a.ts
import * as b from "#/b.";

b.foo();

// @filename: src/b.ts
export function foo() {}

// @filename: package.json
{
"imports": {
"#/*": {
"types": "./src/*ts",
"default": "./dist/*js"
}
}
}

// @filename: tsconfig.json
{
"compilerOptions": {
"module": "nodenext",
"moduleResolution": "nodenext",
"rootDir": "src",
"outDir": "dist"
},
"include": ["src"]
}