Repository navigation
[api] Skip disk-layout import diagnostics for customized module resolutions - #64638
Conversation
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
What changed in this PR
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.
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.)
c719769 to
7370406
Compare
|
Andrew Branch (@andrewbranch) Thanks, that makes sense. I reworked the PR that way: the field is gone, |
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?
|
Andrew Branch (@andrewbranch) Thanks! Callback results are covered too: |
A resolution answered by a static
moduleResolutionsentry or aresolveModuleNamecallback never setsResolvedUsingTsExtension, so underrewriteRelativeImportExtensionsa relative./x.tsvalue import it answers raises a false TS2876 wheretscreports 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
StaticModuleResolutionis unchanged frommain.ResolvedModulegains an internalIsCustomResolution, set instaticModuleResolutionToResolvedModule, which static entries and callback results both go through and the built-in fallbacks never do. When it is set, the checker skips theResolvedUsingTsExtensionblock ofresolveExternalModule: TS2846, TS5097, TS2876, TS2877 and TS2878. Only TS2876 could fire for a customized resolution before, and the diagnostics outside that block still apply.TestCustomModuleResolutionsSkipUnsafeRewriteDiagnosticresolves./b.tsthrough a static entry and./c.tsthrough the callback in one program and expects no semantic diagnostics. Onmainit reports two TS2876.One related behavior is unchanged: emit still rewrites
./b.tsto./b.jsfrom 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.
Backlogmilestone (required)mainbranchnpx hereby testnpx hereby lintnpx hereby check:formatFixes #64630