Repository navigation
Report JS diagnostics at missing nodes without skipping trivia - #65332
Open
Jemmy (JemmyIndex) wants to merge 1 commit into
Open
Jemmy (JemmyIndex) wants to merge 1 commit into
Jemmy (JemmyIndex) wants to merge 1 commit into
Conversation
`jsErrorAtRange` unconditionally skipped leading trivia when creating the diagnostic span. For a missing node the location is empty (pos == end), so skipping the following whitespace moved the start past the end: the TS8010 on `var R: ;` in a .js file was reported at column 8 instead of 7, and `--pretty` crashed in strings.Repeat with a negative count because the span length was -1. Route node-anchored JS diagnostics through getErrorSpanForNode, which only skips trivia for present nodes. This matches the classic compiler, where createDiagnosticForNodeInSourceFile goes through getErrorSpanForNode and skips trivia only when the node is not missing. The three call sites that pass a node list (type parameters, parameters' modifiers, type arguments) keep the range helper, mirroring createDiagnosticForNodeArray, which skips trivia unconditionally. Fixes microsoft#65078
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the node-aware JavaScript diagnostic span changes.
No open issues 🎉
What changed in this PR
Fixes #65078 by using node-aware spans for JavaScript diagnostics, preventing missing nodes from producing inverted ranges and crashing pretty diagnostic output.
Changes:
- Adds
jsErrorAtNodeand migrates node-based diagnostics to the existing span helper. - Preserves range-based handling for node-list diagnostics.
- Adds regression tests for missing type annotations and leading trivia before present annotations.
| File | Description |
|---|---|
tsc/internal/parser/parser.go |
Uses node-aware diagnostic spans to handle missing nodes correctly. |
tsc/internal/parser/parser_test.go |
Verifies diagnostic positions for missing and present type annotations. |
Review effort: Balanced
| // missing (empty) node do not skip trivia: skipping moved the start past the | ||
| // end, producing an inverted span that reported the wrong column and crashed | ||
| // the --pretty writer with a negative strings.Repeat count. | ||
| func TestJSDiagnosticsForMissingNodesDoNotSkipTrivia(t *testing.T) { |
There was a problem hiding this comment.
Why a unit test and not a compiler test or tsctest?
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #65078.
The bug
In a
.jsfile,var R: ;reported TS8010 at column 8 and then crashed:jsErrorAtRangebuilt its span asNewTextRange(SkipTrivia(sourceText, loc.Pos()), loc.End()). When the type annotation ismissing its location is empty (
pos == end == 6), and skipping the whitespace before;moved the start to 7, so the span had length -1.
--pretty falseshowed the resultingwrong column, and the pretty writer passed the negative length to
strings.Repeat.TypeScript 6.0.3 reports TS8010 at column 7 with an empty span, because the classic
compiler creates the diagnostic through
createDiagnosticForNodeInSourceFile→getErrorSpanForNode, which skips trivia only when the node is present:The fix
jsErrorAtRangewas doing the span computation itself instead of using the parser'sexisting
getErrorSpanForNode, which already implements the missing-node rule. AddjsErrorAtNodeand use it for the 21 diagnostics that are anchored at a node; the threediagnostics anchored at a node list (type parameters, parameter modifiers, type
arguments) keep the range-based helper, because
createDiagnosticForNodeArrayalso skipstrivia unconditionally for lists.
Nothing changes for present nodes:
getErrorSpanForNodestill skips trivia for them.Verification
var R: ;in a.jsfile now reports TS8010 at1:7in both modes and--prettyrenders the empty underline instead of panicking:
var a: number→ TS8010 at the type,class C { x: string },function f(p: string) {},interface I {}→ TS8006 at the name.go test ./tsc/internal/parser/passes, including the newTestJSDiagnosticsForMissingNodesDoNotSkipTriviaregression test.AI assistance
This PR was written with AI assistance, and the change is deliberately scoped to how the parser computes the span of JS-only diagnostics that are anchored at a node. I reproduced the crash and verified the fix manually:
--prettyno longer panics, TS8010 onvar R: ;is now reported at1:7with an empty span like TypeScript 6.0.3, andvar a: number,class C { x: string },function f(p: string) {}andinterface I {}keep their existing spans.go test ./tsc/internal/parser/passes locally, including the new regression test. I understand the implementation and am happy to answer questions about it.