fix(parser): anchor literal-validation diagnostics at the token text. - #10103
Conversation
spans were built from the trivia-inclusive offset/width; base them on the text (after leading trivia). Consolidates the three take_terminal_ validators.*
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryLow Risk Overview Validation diagnostics are now anchored at the start of the terminal’s text ( New parser diagnostic tests cover illegal escapes and non-ASCII short strings when the literal is the first token on its line. Reviewed by Cursor Bugbot for commit 4068f90. Bugbot is set up for automated code reviews on this repo. Configure here. |
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on orizi and TomerStarkware).
crates/cairo-lang-parser/src/parser.rs line 3170 at r1 (raw file):
let text = peek.text.long(self.db); let leading_width = trivia_total_width(self.db, &peek.leading_trivia); let green = self.take::<Terminal>();
Cant we get text from green? Also we only need the leading width inside the error case so we should do that inside the if
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on eytan-starkware and TomerStarkware).
crates/cairo-lang-parser/src/parser.rs line 3170 at r1 (raw file):
Previously, eytan-starkware wrote…
Cant we get text from green? Also we only need the leading width inside the error case so we should do that inside the if
unfortunately - it seems we cannot. as we know nothing about the type of the terminal at this point, since it is generic.
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 1 file, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).
Summary
Fixes incorrect diagnostic span anchoring for string/short-string/number literal validation errors when the literal appears with leading trivia (e.g., indented on its own line). Previously, diagnostic spans were computed relative to
self.offset, which includes the terminal's leading trivia width, causing the error underline to point at the wrong position in the source. Now, the span is anchored at the start of the terminal's text (after leading trivia), so the caret and underline correctly point to the literal itself.The three
take_terminal_*methods are also consolidated into a single generictake_validated_terminalhelper that accepts a validation function, eliminating the repeated peek/take/diagnostic pattern.Type of change
Please check one:
Why is this change needed?
When a string or short-string literal appeared as the first token on a new line (i.e., with leading whitespace trivia), validation diagnostics such as "Invalid string escaping" or "Short string literals can only include ASCII characters" would point to the wrong column — offset by the width of the leading whitespace — rather than pointing directly at the literal.
What was the behavior or documentation before?
Diagnostic spans for literal validation errors were computed using
self.offsetandself.current_width, which included the terminal's leading trivia. This caused the error underline to be shifted left, pointing into the indentation whitespace rather than the literal text.What is the behavior or documentation after?
Diagnostic spans are now anchored at
self.offset + leading_trivia_width, so the span starts at the first character of the literal text. The underline and caret in error messages correctly point to the literal, even when it is indented or preceded by other trivia.New test cases cover
'\p'and'\u{1024}'appearing with leading trivia (literal first on its line) to confirm correct span reporting.Related issue or discussion (if any)
Additional context
The
trivia_total_widthhelper is used to compute the leading trivia width from the peeked token before it is consumed byself.take::<Terminal>().