Preserve the ErrorContext cause chain across a defer on the unwind path (B-611)#3903
Merged
Conversation
…th (B-611)
A non-throwing `defer` on the frame an error unwinds through wiped the
propagating error's `.cause` chain: `low()` throws A; `mid()` catches A and
throws B (so B.cause == A); with a `defer {}` armed in `mid`, `root()`'s
`ctx.root_cause()` returned B instead of walking past B to A, and
`ctx.to_string()` dropped the "During handling of the above error" section.
Two coupled deficiencies:
1. An expression-position `throw` inside a `defer` region (or a `catch`
arm/base) was lowered as a static `error_local = v; goto pad` via
`catch_context.unwind_target`, bypassing the exception funnel entirely.
The funnel is what computes the BEP-042 cause (`find_cause_context`) and
materializes the destination handler's `ErrorContext`, so the static jump
dropped the cause (and left a bound `ctx` unmaterialized — an expression
`throw` directly in a `catch` base previously crashed reading `ctx`).
`AstExpr::Throw` now routes through the funnel like `AstStmt::Throw`
already does; the exception table reaches the same innermost handler, so
control flow is unchanged (no codegen snapshot churn).
2. The defer landing pad re-raises the in-flight error via `Rethrow`, and
`try_unwind_exception` unconditionally nulled the cause for any rethrow.
The VM now records the cause computed at each value's fresh throw site
(`thrown_value_causes`, keyed by the thrown value, mirroring
`seen_throw_values` with the same GC root/forward hooks and `finalize`
clear) and reuses it on a rethrow instead of nulling. The recorded value
is the superseded error, never the re-raised value's own context, so this
can never self-link; a genuine rethrow that never superseded an error has
no recorded entry and keeps its `cause = null`.
Together these preserve the chain across the pad's transparent re-raise
(root_cause reaches A, to_string regains the chain) while the existing
null-cause guards (no-match fall-through, layout-gap throws) still hold.
Adds e2e tests in errorcontext.rs: the repro (root_cause and to_string),
a non-empty non-throwing defer body, stacked non-throwing defers, and a
defer that itself throws (chain cleanup -> wrap -> origin stays intact).
|
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 selected for processing (3)
📝 WalkthroughWalkthroughThrow expression lowering in the MIR compiler is unified to always use the exception funnel rather than a catch-context-specific path. The bex_vm now tracks a ChangesCause Chain Preservation
Estimated code review effort: 4 (Complex) | ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Binary size checks passed✅ 7 passed
Generated by |
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.
Bug
A non-throwing
defer {}on the frame an error unwinds through silently wiped the propagating error's.causechain.Repro:
low()throws A;mid()catches A and throws B (soB.cause == A). With thedefer {}present:root()'sctx.root_cause()returned B (\"LOST:wrap\") instead of walking past B to A (\"origin\").ctx.to_string()dropped theDuring handling of the above error, another error occurredsection and the A frame.Removing the
defer {}made both correct — the tell that the defer was the culprit.Root cause
Two coupled deficiencies conspired; the plan's suspected VM-only cause was only half of it.
Lowering —
crates/baml_compiler2_mir/src/lower.rsAstExpr::Throw(~5783). An expression-positionthrowinside adeferregion (or acatcharm/base) was lowered as a staticerror_local = v; goto catch_context.unwind_target, bypassing the exception funnel entirely. The funnel (try_unwind_exception) is what computes the BEP-042 cause (find_cause_context) and materializes the destination handler'sErrorContext. Sothrow Bnever hit the funnel — its cause was never computed. (AstStmt::Throwat ~13393 already always funnels, which is why the sibling-defer case worked.) As a side effect, an expressionthrowdirectly in acatchbase left the boundctxunmaterialized and crashed on access.VM —
crates/bex_vm/src/vm.rstry_unwind_exception(~3390). The defer landing pad re-raises the in-flight error viaRethrow, and the funnel unconditionally forcedcause_context = Value::NULLfor any rethrow (the exact gap thevm.rscomment named: "would need a value->context association we don't track"). Even oncethrow Bfunnels and computesB.cause = A, the pad's transparent re-raise then nulled it before it reachedroot.Fix
AstExpr::Thrownow routes through the funnel (throw/rethrow) likeAstStmt::Throw, instead of the static jump. The exception table reaches the same innermost handler thatcatch_context.unwind_targetpointed at (its region covers this PC), so control flow is unchanged — and confirmed by zero codegen snapshot churn.thrown_value_causes: Vec<(Value, Value)>, recording the cause computed at each value's fresh throw site, keyed by the thrown value. On a rethrow the funnel reuses this instead of nulling. It mirrors the existingseen_throw_valuesprecedent exactly — same GCcollect_roots/forward_rootshooks (both key and cause are heap pointers) andfinalizeclear. The recorded value is always the superseded error, never the re-raised value's own context, so it can never self-link; a genuine rethrow that never superseded an error has no entry and keepscause = null.Both are required: (1) gets B's cause computed/recorded; (2) preserves it across the pad's re-raise.
Candidate choice
I evaluated Candidate 1 (VM value→cause map) vs Candidate 2b (a dedicated
RethrowTransparentopcode). Candidate 1 is used for the VM half — it is safe here (no self-link risk, clean GC rooting via theseen_throw_valuespattern, null-cause preserved for genuine rethrows). But investigation against the real code showed Candidate 1 alone is insufficient: thethrowin the repro is an expression throw that takes a static goto and never reaches the funnel, so there is nothing for the VM to record. The minimal correct addition is the one-branch lowering change so the throw funnels — which also has zero snapshot blast radius (unlike 2b, which would churn codegen for every defer fixture). So: Candidate 1 for the VM, plus the funnel-routing lowering fix.Tests
New e2e tests in
crates/baml_tests/tests/errorcontext.rs:defer_on_unwind_preserves_cause_chain— the repro;root_cause().error == \"origin\".defer_on_unwind_preserves_to_string_chain—to_stringregains theDuring handling of...section plus both frames.defer_with_body_preserves_cause_chain— a non-empty, non-throwing defer body (isolates the transparent re-raise from empty-block elimination).nested_defers_nonthrowing_preserve_cause— two stacked non-throwing defers.defer_that_throws_still_chains_through_wrap— negative test: a defer whose body itself throws still builds the full chaincleanup -> wrap -> origin(the pre-existingwrap -> originlink is not dropped).The existing guard tests are unchanged and still pass:
no_match_rethrow_does_not_self_chain,throw_after_catch_in_layout_gap_does_not_chain,sibling_defers_that_throw_chain_to_root_cause,hazard_a,hazard_b.Before / after (repro
root())\"LOST:wrap\"\"origin\"(removing thedefer {}still yields\"origin\", unchanged)to_stringafter the fix regains... origin ... During handling of the above error, another error occurred: ... wrap.Validation (all local, green)
cargo nextest run -p bex_vm -p bex_engine— 565 passed.cargo test -p baml_tests -- --skip parser_stress— green (1724 snapshot + 1987 nextest + all integration groups; 0 failed; no snapshot churn).cargo nextest run --all-features -E 'not package(baml_tests) and not package(/^sdk_test_/) and not binary(=pack_e2e)'— 4137 passed.cargo clippy -p bex_vm -p baml_compiler2_mir— clean.https://linear.app/boundaryml2/issue/B-611
Summary by CodeRabbit
deferblocks during unwinding could previously hide earlier errors in the final message.