Fix module resolver bug exposed by malformed package.json import patterns#2680
Fix module resolver bug exposed by malformed package.json import patterns#2680
Conversation
This commit fixes a panic that occurred when processing imports with malformed package.json imports mappings. The crash happened when: 1. A package.json has a malformed pattern like './src/*ts' (missing dot) 2. An import like '#/b.' is used (ending with dot, no extension) 3. The pattern substitution resolves to a valid file 4. The checker tries to extract a TS extension but fails, causing a panic Root cause: The code assumed that if resolvedUsingTsExtension is true, then the moduleReference must have a TS extension. This assumption is violated when package.json pattern mapping produces a module reference without a valid TS extension. Fix: Skip error reporting when TryExtractTSExtension returns empty instead of panicking. This is the correct behavior because the specific errors about TS extensions don't apply when the module reference doesn't actually have a TS extension. Test case added: TestPackageJsonImportsMalformed reproduces the crash and verifies the fix.
- Remove unnecessary semicolon after function declaration - Remove trailing whitespace - Clarify note about import ending with dot
Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
andrewbranch
left a comment
andrewbranch
left a comment
There was a problem hiding this comment.
The panic is correct! Either IsDeclarationFileNam or TryExtractTSExtension must be incorrect
|
@copilot the panic was correct in the first if block you changed. That shouldn't be changed at all. In the second block, the panic message was misleading. In reality, it’s |
The panic was correct - it exposed a bug in the module resolver. Root cause: resolvedUsingTsExtension was being set based on whether the package.json pattern ended with the extension, not whether the original module specifier had a TS extension. With malformed patterns like "./src/*ts", the flag was incorrectly set to true even for imports like "#/b." (no TS extension), causing the panic. Fix: Check if the original module specifier (r.name) has a TS extension instead of checking the package.json pattern value. Added compiler test to verify the fix. Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
You're absolutely right! The panic exposed the real bug. The issue was in the module resolver: With malformed patterns like Fixed in 90aa27c by checking |
Per @andrewbranch's feedback: - First if block: panic is correct because IsDeclarationFileName check guarantees TryExtractTSExtension should succeed. Restored panic. - Second else if block: panic message was misleading, but with the resolver fix, ResolvedUsingTsExtension is now correctly set only when the module specifier has a TS extension. Restored panic. The fix in module/resolver.go (checking r.name instead of packageJSONValue) ensures both panics are now valid assertions. Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
You're right! I've restored both panics in ab2810c. The first if block's panic was indeed correct - For the second block, while the panic message was misleading, the root fix in the module resolver (checking |
internal/module/resolver.go
Outdated
| 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). |
There was a problem hiding this comment.
resolvedUsingTsExtension means the combination of two things:
- The module specifier as written literally ends in
.tsor similar - That
.tsending was used to match a.tsfile on disk, as opposed to matching a package name, directory name,"exports"key or pattern, or whatever. For patterns, it’s tricky, becauseimport "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.
There was a problem hiding this comment.
Fixed in 77a5486. Now checking both conditions:
r.namehas a TS extension (module specifier literally ends in.ts)packageJSONValueends with*(extension was used to match, not from the pattern itself)
This correctly distinguishes import "pkg/foo.ts" under pattern ./* from ./*.ts.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Fixed in 60d385b. Moved the function to checker.go as an unexported helper tryExtractTSExtensionLoose and placed it near where it's used.
Per @andrewbranch's review, resolvedUsingTsExtension should be true when: 1. The module specifier (r.name) literally ends in a TS extension, AND 2. That TS extension was used to match a file on disk (not a pattern) This is determined by checking if packageJSONValue ends with '*', which indicates the extension came from the module specifier mapping onto a pattern, not from the pattern itself. This distinguishes imports like 'pkg/foo.ts' under pattern './*' from './*.ts' where the pattern explicitly includes the extension. Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Per @andrewbranch's correction, the logic should be simpler: - Check if packageJSONValue ends with '*' (it's a pattern) - Check if the candidate file has a TS extension - If both are true, the * must have matched a TS extension This correctly handles cases like: - import "pkg/foo.ts" with pattern "./*" -> true - import "pkg/foo.ts.omg" with pattern "./*.omg" -> true (star matched .ts) - import "pkg/foo" with pattern "./*.ts" -> false (extension in pattern) No need to check r.name at all. Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
|
@copilot tests failed |
Added reference baselines for the new test case that verifies the fix for malformed package.json import patterns. The test now passes. Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Added test case for the edge case where a module specifier contains ".ts" that gets matched by a wildcard pattern (e.g., import "#/foo.ts.omg" with pattern "#/*.omg": "./src/*"). This test verifies that: 1. resolvedUsingTsExtension is correctly set to true (wildcard matched .ts) 2. The checker handles this gracefully without panicking Also fixed the second panic in checker.go to skip error reporting when TryExtractTSExtension returns empty, as this can happen when a wildcard matches a TS extension that's not at the end of the module specifier. Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Per @andrewbranch's feedback, instead of skipping error reporting, we now use a best-effort extraction that looks for any TS extension in the module specifier using strings.Contains. This allows the checker to report an error for edge cases like: - import "#/foo.ts.omg" (contains ".ts" even though it doesn't end with it) The error message says "An import path can only end with..." which is slightly misleading for this case, but close enough for edge cases that are unlikely in practice. The loose extraction may misidentify in pathological cases like "foo.ts.mts.cts.oops", but such cases are extremely rare. Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
All tests now pass. The previous failure was due to missing baselines, which have been added. |
Per @andrewbranch's feedback, moved the helper function from tspath package to checker.go as an unexported function near where it's used. This keeps the utility localized and avoids exporting it from tspath. Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
andrewbranch
left a comment
andrewbranch
left a comment
There was a problem hiding this comment.
- Inline
tryExtractTSExtensionLoose - Use
// @noEmit: trueand// @noTypesAndSymbols: truein the tests - Rename the test; nothing tested here is "malformed"
Per @andrewbranch's feedback: 1. Inlined tryExtractTSExtensionLoose directly where it's used 2. Added @noemit and @noTypesAndSymbols directives to both tests 3. Renamed tests to be more descriptive: - packageJsonImportsMalformedPattern -> packageJsonImportsWildcardNoCrash - packageJsonImportsWildcardMatchesTsExtension -> packageJsonImportsWildcardTsExtensionError The tests are clearer: first verifies no crash, second verifies error is reported. Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Addressed in 85d2d5e. The first block's panic remains unchanged (correct). The second block now uses inlined best-effort extraction with
All tests pass. Baselines have been updated with the test renames and directives in 85d2d5e.
Fixed in 85d2d5e:
|
Fix LS crash with package.json wildcard patterns - Complete
Original prompt
This section details on the original issue you should resolve
<issue_title>LS request crash when
package.jsonbizarrely maps specifier to.tsfile</issue_title><issue_description>Forgive me, this is the best repro I could find for this stack trace.
Request diagnostics for
src/a.ts, and request completions at/*$*/.