fix(parser): stop block recovery at a following control-flow statement - #10227
Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryLow Risk Overview Diagnostics for a nested Tests add cases for a broken Reviewed by Cursor Bugbot for commit e1d0ab3. 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 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on orizi and TomerStarkware).
crates/cairo-lang-parser/src/recovery.rs line 108 at r1 (raw file):
| SyntaxKind::TerminalMatch | SyntaxKind::TerminalReturn | SyntaxKind::TerminalIf
If we have return, what about continue?
crates/cairo-lang-parser/src/parser_test_data/diagnostics/if line 348 at r1 (raw file):
^ error[E1001]: Missing token '{'.
Add a test for bad if but good braces:
if {
while ....
}
The `block!` stop-set used by `parse_block`'s `skip_until` only listed
`let`/`match`/`return`. When recovering a broken block-owner header (e.g. an
`if` with a malformed condition), recovery walked past a subsequent, fully
valid `if`/`while`/`loop`/`for` statement's header and consumed its `{ ... }`
as the broken construct's body — silently deleting that statement from the AST
and reparenting its block, with no diagnostic pointing at it.
Add `TerminalIf`/`TerminalWhile`/`TerminalLoop`/`TerminalFor` to `block!` so
recovery halts at the next control-flow statement instead of swallowing it. The
broken block-owner now gets a synthesized empty body (missing `{`/`}`
diagnostics) and the following statement is preserved.
Updates the existing `diagnostics/if` "if inside if condition" golden (recovery
now keeps the inner `if`-expression as its own statement) and adds a regression
case covering a broken `if` header followed by a `while`.
aaf222c to
e1d0ab3
Compare
orizi
left a comment
There was a problem hiding this comment.
@orizi made 2 comments.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on eytan-starkware and TomerStarkware).
crates/cairo-lang-parser/src/recovery.rs line 108 at r1 (raw file):
Previously, eytan-starkware wrote…
If we have return, what about continue?
Done.
crates/cairo-lang-parser/src/parser_test_data/diagnostics/if line 348 at r1 (raw file):
Previously, eytan-starkware wrote…
Add a test for bad if but good braces:
if {
while ....
}
Done.
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).
TL;DR
Improved parser error recovery for broken
ifstatement headers to prevent consuming subsequent control-flow statements as part of theifbody.What changed?
The
block!recovery macro now includesif,while,loop, andforas recovery stop tokens, in addition to the existinglet,match, andreturn. This ensures that when the parser encounters a malformedifheader (e.g.,if a ==with a missing right-hand expression), it stops recovery at the next control-flow keyword rather than consuming it as part of the brokenifblock.Additionally, the diagnostic reported for a missing
{after a brokenifheader was changed from a "Skipped tokens" error to a more precise "Missing token '{'" error, with a corresponding "Missing token '}'" error to close the implicit block.How to test?
The existing parser diagnostic test suite covers this behavior. A new test case was added verifying that a broken
ifheader followed by awhilestatement correctly produces missing-token diagnostics without swallowing thewhileas theifbody:Why make this change?
Previously, a malformed
ifheader would cause the parser to greedily consume the{ ... }block of a subsequent control-flow statement (e.g.,while,loop,for) as the body of the brokenif. This led to confusing diagnostics and incorrect AST structure. By treating control-flow keywords as recovery boundaries, the parser now handles these cases more gracefully and produces cleaner, more accurate error messages.