fix(compiler): don't sink a panicking rvalue to its use site - #4354
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe compiler now prevents cross-block virtual-local inlining for potentially panicking rvalues. New runtime tests cover defer execution and ordering during division-by-zero panic unwinding. ChangesPanic-aware MIR execution
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
⏭️ 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):
|
Binary size checks passed✅ 7 passed
Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@baml_language/crates/baml_compiler2_emit/src/analysis.rs`:
- Around line 1703-1706: Update the Rvalue::BinaryOp handling in
operand_could_be_int so BinOp::Div only returns true when both operands could be
integers, while BinOp::Mod no longer creates an unconditional barrier; preserve
the existing paired-operand checks for the other arithmetic and shift operators.
Run cd baml_language && cargo test --lib.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 605a8d76-40d4-405e-8ab6-095afcdd5f6e
⛔ Files ignored due to path filters (15)
baml_language/crates/baml_tests/snapshots/baml_src/array_rest_binding.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/cancel_cascade.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/classes.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/comparable_sort.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/functions.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/interfaces_associated_types.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/ints.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lambdas.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lexical_scoping.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/optional_function_parameters.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/patterns_new_runtime.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/task_group.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lexical_scoping/baml_tests__compiles__lexical_scoping__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_statements/baml_tests__compiles__parser_statements__06_codegen.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
baml_language/crates/baml_compiler2_emit/src/analysis.rsbaml_language/crates/baml_tests/tests/defer.rs
hellovai
added a commit
that referenced
this pull request
Aug 10, 2026
Addresses CodeRabbit on #4354. `%` has no specialized float opcode: the generic float path does `l % r` and yields NaN, so only `ModInt` (bex_vm/src/vm.rs) throws `DivisionByZero`. Ask `operand_could_be_int` like the other int-only failures. `/` stays unconditional. `OpCode::DivFloat` throws `DivisionByZero` on a zero divisor rather than yielding IEEE infinity, so float division is fallible too. No generated code changes — cross-block `%` virtualization does not occur in the test corpus. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018D2txzPqfLJXdUcTit3uqk
Contributor
Author
|
@coderabbitai review The outstanding comment is resolved: the |
Contributor
|
|
The emitter's `Virtual` classification inlines a single-use local's rvalue at its use site instead of storing it. For a cross-block def-use that *moves* the evaluation past the def block's terminator (a call) and into a possibly different exception region. Arithmetic can panic, so the move is observable. `return 10 / 0` inside a function with a `defer` sank the division below the `return`'s inline defer replay: the defer body ran on the way out, the division then panicked, and the unwind landing pad ran the body a second time. With nested defers the whole chain repeated (`inner,outer,inner,outer`). The same reorder is visible without any `defer` — `let x = 10 / 0` followed by a call ran the call first. Reject cross-block virtualization for rvalues that can panic: `int` add/sub/mul/shift (range-checked), `/` and `%` (zero divisor), and negation (`INT_MIN`). Index reads can panic too but are already rejected cross-block by the projection-read guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018D2txzPqfLJXdUcTit3uqk
Follow-up to the previous commit. Two refinements plus the snapshot updates it implies. `rvalue_can_panic` treated every `+`/`-`/`*`/shift as fallible regardless of operand type, which blocked virtualization for string concatenation — a chain of concats round-tripped through slots instead of staying on the stack. Only `int` arithmetic is range-checked: `float` saturates, `bigint` grows, and `string + string` is concatenation. Ask the operand types, defaulting to "could be int" for anything not pinned down. That drops the affected codegen snapshots from 10 to 3. The predicate also fell through to `_ => false`, asserting that every other `Rvalue` is infallible. Wrong default for a soundness check: a panicking variant added later would silently re-enable the code motion. Match all 19 variants so a new one fails to compile instead. Remaining snapshot churn is arithmetic materialized at its binding site rather than sunk past a later call, which is the fix working. Several cases lose an instruction: the operand that used to need a slot now stays on the stack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018D2txzPqfLJXdUcTit3uqk
Addresses CodeRabbit on #4354. `%` has no specialized float opcode: the generic float path does `l % r` and yields NaN, so only `ModInt` (bex_vm/src/vm.rs) throws `DivisionByZero`. Ask `operand_could_be_int` like the other int-only failures. `/` stays unconditional. `OpCode::DivFloat` throws `DivisionByZero` on a zero divisor rather than yielding IEEE infinity, so float division is fallible too. No generated code changes — cross-block `%` virtualization does not occur in the test corpus. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018D2txzPqfLJXdUcTit3uqk
hellovai
dismissed
coderabbitai[bot]’s stale review
August 10, 2026 19:18
Stale: this review predates d9ae785/46af8cb. Its one comment had two parts — the % half was applied, and the reviewer withdrew the / half in #4354 (comment) ("My original division recommendation incorrectly applied Rust float semantics instead of BAML VM semantics"). Thread resolved; CI green at 61 checks.
aaronvg
added a commit
that referenced
this pull request
Aug 10, 2026
CI reported two bytecode snapshot failures that I could not reproduce locally, which sent me chasing nondeterminism and platform divergence. Neither was right. CI tests the PR MERGED with canary, and canary had landed d4e70d0, "fix(compiler): don't sink a panicking rvalue to its use site" (#4354). That fix changes `can_be_virtual`: a single-use local is no longer virtualized away, because dropping the store at the definition and re-emitting at the use site made the rvalue run a second time when a panic unwound through it. So the emitted code now materializes `store_var _14` at the definition instead of sinking it, which in turn leaves two loads adjacent and lets the peephole fuse them into `load_var2`. My snapshots predated the fix and encoded the old sunk form. The tell was the test count: CI ran 3188 where I ran 3184, and the four extra were that fix's own regression tests. Merging canary reproduces CI's output exactly, byte for byte. Regenerated the four affected snapshots. Local run is now 3188 tests, 3185 passing, no unreferenced snapshots; the 3 failures are the local gofmt shim with no go version set, which is green in CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
antoniosarosi
added a commit
that referenced
this pull request
Aug 11, 2026
Canary #4354 added the exhaustive rvalue_can_panic soundness predicate; BEP-066's RuntimeIsType and CurrentPackage variants are both infallible (a type test like IsType, and a build-time-resolved lookup like LoadType) so they join the non-panicking group. Regenerated the describe listing and bytecode snapshots for the merged stdlib + new Package API methods.
antoniosarosi
added a commit
that referenced
this pull request
Aug 11, 2026
Canary #4354 added the exhaustive rvalue_can_panic soundness predicate; BEP-066's RuntimeIsType and CurrentPackage variants are both infallible (a type test like IsType, and a build-time-resolved lookup like LoadType) so they join the non-panicking group. Regenerated the describe listing and bytecode snapshots for the merged stdlib + new Package API methods.
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.
Fixes B-1184:
deferruns twice when a panic unwinds through it.Root cause
Not in the VM unwinder — in the emitter.
can_be_virtual(baml_compiler2_emit/src/analysis.rs) classifies a single-use local asVirtual, which drops the store at the definition and re-emits the rvalue at the use site. For a cross-block def-use that moves the evaluation past the def block's terminator (a call) and into a possibly different exception region.MIR for the repro is correct:
The emitted bytecode is not —
_0is virtualized to thereturnin bb2, so the division lands after the replay:The defer body runs once on the way out, the division then panics, and the landing pad runs it again. With nested defers the entire chain repeats.
Typed throws are unaffected because
throwis its own terminator — there is nothing to sink past.The bug is not defer-specific
The same reorder is observable with no
deferanywhere:Fix
Reject cross-block virtualization when the rvalue can panic:
intadd/sub/mul/shift (range-checked →IntegerOverflow),/and%(zero divisor →DivisionByZero), and negation (INT_MIN). Bitwise and/or/xor and comparisons stay in range by construction. Index reads can panic too, but every rvalue with a projection read is already rejected cross-block.Same-block virtualization is untouched: exception regions start and end on block boundaries, so the set of covering handlers is constant within a block, and the existing intervening-side-effect check still applies.
Tests
Four new cases in
crates/baml_tests/tests/defer.rs. Verified they fail on the parent commit with exactly the reported symptoms:defer_runs_once_when_a_panic_unwinds_through_it["D", "D"]["D"]nested_defers_run_once_each_when_a_panic_unwinds["inner","outer","inner","outer"]["inner","outer"]defer_runs_once_when_a_call_free_body_sees_a_panic89a_panicking_binding_runs_before_later_side_effects["after"][]The third covers a defer body that compiles to statements rather than a call, so the replay lands in the returning block itself; the fourth is the underlying emitter bug with no
deferinvolved.Local run:
cargo test -p baml_tests --test defer— 17 passed. Leaving the full suite to CI.Draft pending a green CI run — the bytecode-snapshot tests are the ones worth watching, since this makes the emitter strictly less aggressive.
🤖 Generated with Claude Code
https://claude.ai/code/session_018D2txzPqfLJXdUcTit3uqk
Summary by CodeRabbit
Bug Fixes
Tests