Repository navigation
Allow break and continue in match-arm expression position (B-619) - #3921
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds ChangesBreak/Continue as Expressions
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Parser
participant Lowerer
participant Formatter
Parser->>Parser: emit BREAK_EXPR / CONTINUE_EXPR
Parser-->>Lowerer: pass expression node
Lowerer->>Lowerer: wrap jump stmt in Expr::Block
Lowerer-->>Formatter: Expression::Break / Expression::Continue
Formatter->>Formatter: print wrapped arm body with semicolon
Possibly related PRs
Poem
🚥 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_fmt/src/ast/expressions.rs (1)
1811-1817: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated braceless-jump check into a helper.
The same
matches!(&self.body, Expression::Return(_) | Expression::Break(_) | Expression::Continue(_))check is duplicated three times (MatchArmtwice,CatchArmonce). A small helper, e.g.Expression::needs_stmt_semicolon(&self) -> bool, would avoid the triple duplication and reduce risk of the checks drifting apart if a fourth jump kind is added later.♻️ Proposed refactor
+impl Expression { + /// Whether this braceless jump (`return`/`break`/`continue`) requires an + /// explicit `;` when wrapped into a block by an arm printer. + pub(crate) fn needs_stmt_semicolon(&self) -> bool { + matches!( + self, + Expression::Return(_) | Expression::Break(_) | Expression::Continue(_) + ) + } +}Then replace each of the three
matches!call sites withself.body.needs_stmt_semicolon().Also applies to: 1880-1886, 2283-2289
🤖 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 `@baml_language/crates/baml_fmt/src/ast/expressions.rs` around lines 1811 - 1817, The braceless-jump semicolon check is duplicated across MatchArm and CatchArm, so extract it into a shared helper on Expression such as needs_stmt_semicolon() that encapsulates the Return/Break/Continue match. Then replace each repeated matches! call site in MatchArm and CatchArm with the new helper so the logic stays consistent if jump kinds change later.
🤖 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.
Nitpick comments:
In `@baml_language/crates/baml_fmt/src/ast/expressions.rs`:
- Around line 1811-1817: The braceless-jump semicolon check is duplicated across
MatchArm and CatchArm, so extract it into a shared helper on Expression such as
needs_stmt_semicolon() that encapsulates the Return/Break/Continue match. Then
replace each repeated matches! call site in MatchArm and CatchArm with the new
helper so the logic stays consistent if jump kinds change later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e643a277-2333-4d00-8bda-0b79327fe617
⛔ Files ignored due to path filters (2)
baml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/match_arm_break_continue.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler_parser/src/parser.rsbaml_language/crates/baml_compiler_syntax/src/syntax_kind.rsbaml_language/crates/baml_fmt/src/ast/expressions.rsbaml_language/crates/baml_fmt/src/lib.rsbaml_language/crates/baml_tests/baml_src/ns_match_arm_break_continue/match_arm_break_continue.baml
Binary size checks passed✅ 7 passed
Generated by |
antoniosarosi
force-pushed
the
antonio/b-619
branch
from
July 6, 2026 19:57
ee52095 to
1d62105
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_fmt/src/ast/expressions.rs (1)
69-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment doesn't mention new
Break/Continuevariants.The
VerbatimSpandoc comment at Lines 69-73 still only calls outExpression::UnknownandExpression::Returnas the variants it backs, even thoughExpression::Break/Expression::Continueare now also backed by it (Lines 62, 65).📝 Suggested doc tweak
-/// last *token* ranges. The trivia classifier keys leading/trailing comments to -/// individual token ranges, so [`Printable::leftmost_token`] / -/// [`Printable::rightmost_token`] must return those exact token ranges for a -/// comment to attach and emit. A whole-node span never matches a token key, so -/// a trailing comment on the node was silently dropped — the `defer` statement -/// comment-loss bug (B-629), and the same class of bug for a braceless `return` -/// arm. A whole-node span can also begin inside leading trivia (the parser +/// last *token* ranges. The trivia classifier keys leading/trailing comments to +/// individual token ranges, so [`Printable::leftmost_token`] / +/// [`Printable::rightmost_token`] must return those exact token ranges for a +/// comment to attach and emit. A whole-node span never matches a token key, so +/// a trailing comment on the node was silently dropped — the `defer` statement +/// comment-loss bug (B-629), and the same class of bug for a braceless +/// `return`/`break`/`continue` arm. A whole-node span can also begin inside leading trivia (the parser🤖 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 `@baml_language/crates/baml_fmt/src/ast/expressions.rs` around lines 69 - 84, Update the `VerbatimSpan` doc comment in `baml_fmt::ast::expressions` so it also mentions the new `Expression::Break` and `Expression::Continue` variants alongside `Expression::Unknown` and `Expression::Return`; keep the explanation about token-range-based printing and leading/trailing trivia handling, but make the list of backed variants complete so it matches the current `Expression` enum.
🤖 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.
Nitpick comments:
In `@baml_language/crates/baml_fmt/src/ast/expressions.rs`:
- Around line 69-84: Update the `VerbatimSpan` doc comment in
`baml_fmt::ast::expressions` so it also mentions the new `Expression::Break` and
`Expression::Continue` variants alongside `Expression::Unknown` and
`Expression::Return`; keep the explanation about token-range-based printing and
leading/trailing trivia handling, but make the list of backed variants complete
so it matches the current `Expression` enum.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1f35fe7b-2ef2-4a72-97c0-03aebc38e433
⛔ Files ignored due to path filters (2)
baml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/match_arm_break_continue.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler_parser/src/parser.rsbaml_language/crates/baml_compiler_syntax/src/syntax_kind.rsbaml_language/crates/baml_fmt/src/ast/expressions.rsbaml_language/crates/baml_fmt/src/lib.rsbaml_language/crates/baml_tests/baml_src/ns_match_arm_break_continue/match_arm_break_continue.baml
🚧 Files skipped from review as they are similar to previous changes (5)
- baml_language/crates/baml_compiler_syntax/src/syntax_kind.rs
- baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs
- baml_language/crates/baml_tests/baml_src/ns_match_arm_break_continue/match_arm_break_continue.baml
- baml_language/crates/baml_compiler_parser/src/parser.rs
- baml_language/crates/baml_fmt/src/lib.rs
`break`/`continue` were rejected in expression position (e.g. a bare
`match`/`catch` arm value like `0 => break`) with E0010 "Expected
expression, found break", even though `return` is accepted there and the
braced form `0 => { break; }` already works.
Root cause: the expression parser (`parse_primary_expr`) recognizes
`return` and `throw` as diverging expressions but had no case for
`break`/`continue`, so they fell through to the generic "Expected
expression" error. Statement position was fine, which is why the braced
arm worked.
Fix — make break/continue symmetric with return:
- Parser: new BREAK_EXPR/CONTINUE_EXPR CST nodes, parsed in
`parse_primary_expr` mirroring RETURN_EXPR; like `parse_return_expr`
they don't consume a trailing `;` (that stays owned by the enclosing
statement).
- AST lowering: each desugars to a single-statement block
(`{ break; }` / `{ continue; }`), reusing the tested
Stmt::Break/Stmt::Continue machinery (divergence-to-never typing,
defer replay/unwatch, defer-escape diagnostics). They carry no value,
so no dedicated Expr variant is needed and the braceless form behaves
identically to the already-accepted braced arm.
- Formatter: break/continue arms wrap into a block with a trailing `;`,
exactly as return arms do, so output round-trips through
BREAK_STMT/CONTINUE_STMT and stays idempotent.
Tests: parser unit test for BREAK_EXPR/CONTINUE_EXPR in match arms; a
formatter idempotency test; and a new baml_src/ns_match_arm_break_continue
end-to-end fixture whose test blocks execute the loop-control semantics in
both statement and value arm positions.
Fixes B-619
antoniosarosi
force-pushed
the
antonio/b-619
branch
from
July 6, 2026 21:15
1d62105 to
fcbb6f4
Compare
This branch was previously 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.
breakandcontinuewere rejected in expression position — e.g. a barematch/catcharm value like0 => break— withE0010 "Expected expression, found break"(andfound continue), even thoughreturnis accepted there and the braced form0 => { break; }already works. Thereturn-vs-breakasymmetry surfaced during a hackathon app.Root cause
The expression parser (
parse_primary_expr) recognizesreturnandthrowas diverging expressions but had no case forbreak/continue, so they fell through to the generic "Expected expression" error. Statement position was fine (parse_break_stmt/parse_continue_stmt), which is why the braced arm worked.Fix — make
break/continuesymmetric withreturnBREAK_EXPR/CONTINUE_EXPRCST nodes, parsed inparse_primary_exprmirroringRETURN_EXPR. Likeparse_return_expr, they do not consume a trailing;— that stays owned by the enclosing statement.{ break; }/{ continue; }), reusing the fully-testedStmt::Break/Stmt::Continuemachinery (divergence-to-nevertyping, defer replay/unwatch, defer-escape diagnostics).break/continuecarry no value, so — unlikereturn— no dedicatedExprvariant is needed, and the braceless form is guaranteed to behave identically to the already-accepted braced arm.break/continuearms wrap into a block with a trailing;, exactly asreturnarms already do, so output round-trips throughBREAK_STMT/CONTINUE_STMTand stays idempotent.Tests
break/continueproduceBREAK_EXPR/CONTINUE_EXPRin match arms with no parse errors.baml_src/ns_match_arm_break_continueend-to-end fixture whosetestblocks execute the loop-control semantics (break exits the loop, continue re-enters the condition) in both statement and value arm positions.Fixes B-619
Summary by CodeRabbit
New Features
breakandcontinuein expression position, includingmatch/catcharms.Bug Fixes
break/continueuse proper jump semantics instead of recovery behavior.Tests
break/continuein match-arm expression bodies and end-to-end behavior in loop/value scenarios.