Repository navigation
[api] Skip disk-layout import diagnostics for customized module resolutions - #64638
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused change consistently preserves the flag across both resolution paths and includes targeted regression coverage.
Review effort: Balanced
Findings: None
Preserves resolvedUsingTsExtension in TypeScript API resolver overrides, preventing false TS2876 diagnostics and fixing #64630.
Changes:
- Adds the optional protocol field and preserves it during resolution conversion.
- Adds regression tests for static entries and callback results.
| File | Description |
|---|---|
| tsc/internal/api/session_module_resolution_test.go | Tests flag preservation and static-resolution diagnostics. |
| tsc/internal/api/proto.go | Adds the optional resolution flag. |
| tsc/internal/api/module_resolution.go | Copies the flag into resolved modules. |
| packages/typescript/src/api/proto.generated.ts | Exposes the flag in the generated interface. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
Andrew Branch (andrewbranch)
left a comment
There was a problem hiding this comment.
This was very intentionally left off, but the intention was just never to issue these kinds of diagnostics against customized resolutions. It looks like I skimmed through the checker blocks that check ResolvedUsingTsExtension too fast and only saw errors happening in the positive case, but that one of course happens in the negative case. Instead of adding this field, we should just skip any checker diagnostics that use file structure on disk as a rationale for their existence. (I think we'll have to add another boolean to ResolvedModule to track this, but maybe there's already an easy way I'm not thinking of.)
Sorry, something went wrong.
|
Andrew Branch (@andrewbranch) Thanks, that makes sense. I reworked the PR that way: the field is gone, ResolvedModule gains an internal IsCustomResolution that staticModuleResolutionToResolvedModule sets for static entries and callback results, and the checker skips the ResolvedUsingTsExtension block in resolveExternalModule when it is set. I found no existing field that tells a customized resolution apart, so the boolean was the smallest way I saw. |
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The localized change implements the documented policy, preserves built-in checks, and covers both custom-resolution paths.
Review effort: Balanced
Findings: None
Sorry, something went wrong.
Andrew Branch (andrewbranch)
left a comment
There was a problem hiding this comment.
Thanks, this looks good! Isn't the same thing needed for resolutions supplied by the callback API?
Sorry, something went wrong.
|
Andrew Branch (@andrewbranch) Thanks! Callback results are covered too: callbackModuleResolver.resolveModuleName decodes the callback's answer as a StaticModuleResolution and returns it through staticModuleResolutionToResolvedModule (L122), the same function static entries use, so both get IsCustomResolution. The test resolves ./c.ts through the callback for that reason, and on main it reports TS2876 for both imports. I also moved the bools together in ResolvedModule. |
Sorry, something went wrong.
Andrew Branch (andrewbranch)
left a comment
There was a problem hiding this comment.
Thanks!
Sorry, something went wrong.
A resolution answered by a static moduleResolutions entry or a resolveModuleName callback never sets ResolvedUsingTsExtension, so under rewriteRelativeImportExtensions a relative ./x.ts value import it answers raises a false TS2876 where tsc reports none.
Following the review, customized resolutions now skip the checker diagnostics that rest on how the built-in resolver mapped the specifier onto disk, and StaticModuleResolution is unchanged from main. ResolvedModule gains an internal IsCustomResolution, set in staticModuleResolutionToResolvedModule, which static entries and callback results both go through and the built-in fallbacks never do. When it is set, the checker skips the ResolvedUsingTsExtension block of resolveExternalModule: TS2846, TS5097, TS2876, TS2877 and TS2878. Only TS2876 could fire for a customized resolution before, and the diagnostics outside that block still apply.
TestCustomModuleResolutionsSkipUnsafeRewriteDiagnostic resolves ./b.ts through a static entry and ./c.ts through the callback in one program and expects no semantic diagnostics. On main it reports two TS2876.
One related behavior is unchanged: emit still rewrites ./b.ts to ./b.js from the specifier text, and with this block skipped the checker no longer verifies that a customized target has the matching output path.
I met this while building deadset-ts, the TypeScript analyzer of deadset, on the TypeScript 7 API. That work hit a small set of related defects, which is why a few reports come from me.
An AI coding agent wrote this patch. I have read, built and tested it and will handle the review.
Fixes #64630