fix: parser error recovery (I-125) - #111
Open
artefactop wants to merge 1 commit into
Open
Conversation
I-125: the parser had zero recovery — a single syntax error made parse_source bail, discarding the whole AST and suppressing every semantic diagnostic in the file (violating R9's accumulation rule and R10's recover-at-statement-boundary requirement). - AST: new StmtKind::Error placeholder; astgen lowers it to nothing (the parser already emitted the diagnostic). - Parser: statements now parse as lines (stmt + terminating newlines) with chumsky recover_with(via_parser(...)) at two points: statement-level (skip garbage to the next boundary, yielding an Error node) and tail-level (same-line garbage after a valid statement prefix collapses the whole line to one Error node, so half-parsed prefixes never reach sema). Blocks handle the unterminated final statement before Dedent/EOF explicitly. - Driver: parse_source returns the partial program plus diagnostics; run/build/ir thread them into the middle-end sink so parse and sema diagnostics co-surface in one run. - Tests: parser unit tests for recovery/multi-error/EOF cases, driver tests for partial-AST and parse+sema co-surfacing, and a golden multi-error integration test.
📝 WalkthroughWalkthroughThe parser now recovers from statement-level syntax errors by producing partial ASTs and diagnostics. Downstream lowering skips error nodes. Parse, semantic, and ownership diagnostics are combined across parse, IR, run, and build pipelines. Issue I-125 was removed. ChangesParser recovery and diagnostic propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Source
participant Parser
participant AST
participant ASTGen
participant Pipeline
participant Diagnostics
Source->>Parser: parse source statements
Parser->>AST: create valid statements and Error nodes
Parser->>Diagnostics: collect parse diagnostics
Parser->>Pipeline: return partial AST and diagnostics
Pipeline->>ASTGen: lower partial AST
ASTGen->>Pipeline: continue with valid statements
Pipeline->>Diagnostics: add semantic and ownership diagnostics
Diagnostics-->>Pipeline: finalize combined diagnostics
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ryo-frontend/src/parser.rs (3)
103-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated garbage-skip parser.
The same
garbage_token().repeated().at_least(1).ignored()chain appears three times, at lines 104-107, 114-117, and 132-135. A small named helper removes the duplication and gives the resynchronization rule one definition.♻️ Proposed helper
+/// Skip at least one non-boundary token. Recovering over zero tokens +/// at a clean boundary would emit a spurious error. +fn skip_garbage<'a, I>() -> impl Parser<'a, I, (), extra::Err<Rich<'a, Token>>> + Clone + 'a +where + I: ValueInput<'a, Token = Token, Span = SimpleSpan>, +{ + garbage_token().repeated().at_least(1).ignored() +}- let stmt_rec = stmt.clone().recover_with(via_parser( - garbage_token() - .repeated() - .at_least(1) - .ignored() - .map_with(|_, e| error_stmt(e.span())), - )); + let stmt_rec = stmt + .clone() + .recover_with(via_parser(skip_garbage().map_with(|_, e| error_stmt(e.span()))));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ryo-frontend/src/parser.rs` around lines 103 - 139, Extract the repeated garbage-skip chain into a named local helper near the parser definitions, then reuse it in the recovery parsers for stmt_rec, tail, and last. Keep the helper’s behavior unchanged: require at least one garbage token and ignore the parsed result.
1843-1855: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a broken block-final statement before
Dedent.
recovers_from_trailing_garbage_without_newline_at_eofcovers theend()terminator. Thepeek(Token::Dedent)terminator used byindented_blockis the harder path, because it is zero-width, and the PR description names it as a specific case the implementation handles. No test exercises it.💚 Proposed test
#[test] fn recovers_from_broken_block_final_statement() { // The last body line is broken and sits directly against the // block's `Dedent` (no terminating newline of its own). let (program, errs, _pool) = lex_and_parse_recovering("fn main():\n\tx = 1\n\ty = = 2\nz = 3\n"); assert_eq!(errs.len(), 1, "expected one parse error: {errs:?}"); let program = program.expect("recovery must produce a partial program"); let StmtKind::FunctionDef(func) = &program.statements[0].kind else { panic!("expected FunctionDef"); }; assert_eq!(func.body.len(), 2); assert!(matches!(func.body[1].kind, StmtKind::Error)); assert_eq!(program.statements.len(), 2); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ryo-frontend/src/parser.rs` around lines 1843 - 1855, Add a regression test beside recovers_from_trailing_garbage_without_newline_at_eof that parses a function whose final indented statement is malformed immediately before Dedent, followed by a valid top-level statement. Assert one parse error, successful partial recovery, a two-statement function body with the final body statement as StmtKind::Error, and preservation of the following top-level statement.
56-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a zero-width carrier for the lookahead.
empty().and_is(just(token))reads as the intended pure lookahead fortokenand avoids the misleadingend().or_not()carrier.♻️ Proposed simplification
- end().or_not().and_is(just(token)).ignored() + empty().and_is(just(token)).ignored()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ryo-frontend/src/parser.rs` around lines 56 - 64, Update the peek function’s lookahead carrier from end().or_not() to empty(), keeping the existing and_is(just(token)).ignored() logic unchanged so the parser remains zero-width while checking the next token.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ryo-driver/src/pipeline.rs`:
- Around line 92-94: Reorder the operations in the parse command so
display_ast(&program, &pool) runs immediately after parse_source and before
finalize_diags(diags, &input, &name)?. Match the ordering already used by
ir_command, preserving AST output for recovered parse errors while still
returning diagnostic failures afterward.
---
Nitpick comments:
In `@ryo-frontend/src/parser.rs`:
- Around line 103-139: Extract the repeated garbage-skip chain into a named
local helper near the parser definitions, then reuse it in the recovery parsers
for stmt_rec, tail, and last. Keep the helper’s behavior unchanged: require at
least one garbage token and ignore the parsed result.
- Around line 1843-1855: Add a regression test beside
recovers_from_trailing_garbage_without_newline_at_eof that parses a function
whose final indented statement is malformed immediately before Dedent, followed
by a valid top-level statement. Assert one parse error, successful partial
recovery, a two-statement function body with the final body statement as
StmtKind::Error, and preservation of the following top-level statement.
- Around line 56-64: Update the peek function’s lookahead carrier from
end().or_not() to empty(), keeping the existing and_is(just(token)).ignored()
logic unchanged so the parser remains zero-width while checking the next token.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9bc9756e-4ee3-4720-bd66-b441bd287edb
📒 Files selected for processing (6)
ISSUES.mdryo-core/src/ast.rsryo-driver/src/pipeline.rsryo-frontend/src/astgen.rsryo-frontend/src/parser.rsryo/tests/integration_tests.rs
💤 Files with no reviewable changes (1)
- ISSUES.md
Comment on lines
+92
to
94
| let (program, diags) = parse_source(&input, &mut pool, &name)?; | ||
| finalize_diags(diags, &input, &name)?; | ||
| display_ast(&program, &pool); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
ryo parse hides the partial AST on a recovered parse error.
finalize_diags returns Err when any diagnostic has Severity::Error. A recovered parse error always produces one. The ? therefore returns before display_ast, so ryo parse prints diagnostics and no AST.
ir_command orders these operations the other way at lines 357-360, so ryo ir --emit=ast does print the partial AST. Two commands now behave differently for the same input. Print the AST first to match ir_command and to deliver the recovery benefit this PR adds.
🐛 Proposed fix
let (program, diags) = parse_source(&input, &mut pool, &name)?;
- finalize_diags(diags, &input, &name)?;
display_ast(&program, &pool);
+ finalize_diags(diags, &input, &name)?;
Ok(())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let (program, diags) = parse_source(&input, &mut pool, &name)?; | |
| finalize_diags(diags, &input, &name)?; | |
| display_ast(&program, &pool); | |
| let (program, diags) = parse_source(&input, &mut pool, &name)?; | |
| display_ast(&program, &pool); | |
| finalize_diags(diags, &input, &name)?; | |
| Ok(()) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ryo-driver/src/pipeline.rs` around lines 92 - 94, Reorder the operations in
the parse command so display_ast(&program, &pool) runs immediately after
parse_source and before finalize_diags(diags, &input, &name)?. Match the
ordering already used by ir_command, preserving AST output for recovered parse
errors while still returning diagnostic failures afterward.
Contributor
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
1 participant
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.
Summary
Fixes I-125 — the parser had zero error recovery: one syntax error discarded the whole AST and suppressed every semantic diagnostic in the file, violating R9 (accumulate diagnostics) and R10 (emit a diagnostic, synchronize at the next statement boundary, produce a partial AST).
StmtKind::Errorplaceholder; astgen lowers it to nothing (the parser already emitted the diagnostic)stmt+ terminating newlines) with chumskyrecover_with(via_parser(...))synchronization at statement and line-tail boundaries; a broken line collapses to oneErrornode and parsing resumes at the next lineparse_sourcereturns the partial program + diagnostics;run/build/irthread them into the middle-end sink so parse and sema diagnostics co-surface in a single runTests
Summary by CodeRabbit
New Features
Bug Fixes
Tests