Handle processing diagnostics similar to Strada #1628
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR changes how processing diagnostics (like /// <reference ...> directives) are handled to match the behavior of Strada (the TypeScript compiler). Instead of including all processing diagnostics in GetProgramDiagnostics, it now only includes global processing diagnostics, while file-specific processing diagnostics are handled through getSemanticDiagnosticsForFile. This ensures that skipLibCheck: true properly filters out processing diagnostics from library files.
Key Changes:
- Modified
GetProgramDiagnosticsto only include global processing diagnostics - Updated
getSemanticDiagnosticsForFileto include file-specific processing diagnostics - Added a test case to verify that processing diagnostics in library files are properly skipped when
skipLibCheck: true
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| testdata/tests/cases/compiler/processingDiagnosticSkipLibCheck.ts | New test case demonstrating that processing diagnostics in library files should be skipped when skipLibCheck: true |
| internal/compiler/program.go | Modified diagnostic collection to separate global and file-specific processing diagnostics |
| testdata/baselines/reference/* | Updated baselines showing improved diagnostic behavior with file-specific error reporting |
| internal/execute/tsctests/tscbuild_test.go | Updated test comment reflecting corrected behavior |
| "../../animals/dog.ts", | ||
| "../../animals/index.ts", | ||
| "../../core/utilities.ts" | ||
| [ |
There was a problem hiding this comment.
As far as I could tell, these new errors are correct and show up in the Strada baselines for these tests.
| return SortAndDeduplicateDiagnostics(slices.Concat(p.programDiagnostics, p.includeProcessor.getDiagnostics(p).GetDiagnostics())) | ||
| return SortAndDeduplicateDiagnostics(slices.Concat( | ||
| p.programDiagnostics, | ||
| p.includeProcessor.getDiagnostics(p).GetGlobalDiagnostics())) |
There was a problem hiding this comment.
| p.includeProcessor.getDiagnostics(p).GetGlobalDiagnostics())) | |
| p.includeProcessor.getDiagnostics(p).GetGlobalDiagnostics(), | |
| )) |
Nit (sorry)
jakebailey
left a comment
jakebailey
left a comment
There was a problem hiding this comment.
LGTM, feel free to not do the formatting nit 😄
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
In Corsa,
GetProgramDiagnosticswas including all diagnostics of theincludeProcessor, which includes/// <reference ...>diagnostics and resolution diagnostics. Because we did that, an error e.g. in a bad/// <reference type="non-existent"/>in a lib file would be included in the output of a command line execution even if the project hadskipLibCheck: true.This PR changes this so that
GetProgramDiagnosticsnow only includes globalincludeProcessordiagnostics.This is basically what Strada did (see
program.getCombinedDiagnosticsthere).The file-specific
includeProcessordiagnostics are now obtained viagetSemanticDiagnosticsForFile. This is also what Strada did.The result is that in
getSemanticDiagnosticsForFile, we have a chance to check if that file should be skipped perskipLibCheck, and we'll also handle// @ts-ignorecorrectly as well for these diagnostics.