Repository navigation
feat(compiler2): BEP-042 cleanup finalizer (magic method) - #3835
Merged
Merged
Conversation
Implements the foundation of BEP-042 `cleanup` — but, unlike the BEP's
`implements baml.gc.Cleanup` interface, `cleanup` is a **magic method
recognized by name**, exactly like `to_json` (not `to_string`, which is
interface-gated). A user class that declares
function cleanup(self) -> void { ... }
gets a finalizer whose body runs **at most once per instance**, regardless
of how it is triggered.
This commit covers the explicit and `defer` trigger paths; the GC-driven
path (the "you forgot to defer" safety net) is the follow-up commit.
Run-once mechanism — a per-instance atomic latch, the analogue of .NET's
`GC.SuppressFinalize` bit:
- `bex_vm_types`: `CleanupLatch(AtomicBool)` on `Instance` (manual
Clone/Debug/Borsh, like `AtomicValueSlot`). Stored inline so it is correct
by construction across full *and* minor GC — `copy_object_to_inactive`
clones the object and `Clone` preserves the bit, and a reclaimed slot
reused for a fresh instance starts uncleaned (no stale-pointer aliasing a
heap-side set would risk). `Object` is at its 64-byte budget under
`heap_debug`, so the byte is reclaimed by boxing `class_type_args`
(`Vec` -> `Box<[RuntimeTy]>`, immutable after construction; matches the
existing `Closure::captured_type_args` / `BoundMethod::type_args`
convention).
- `bex_vm` / stdlib: `baml._cleanup_begin(value)` native + its
`conversions.baml` declaration — atomically test-and-sets the latch and
returns whether this is the first call. Set on entry, so a `cleanup` that
throws is still considered cleaned.
Recognition + guard (compile-time, hot dispatch loop untouched):
- `baml_compiler2_ast::cleanup_guard`: a pure AST pass (beside
`auto_derive_json`) wraps a recognized `cleanup` body in
`if (baml._cleanup_begin(self)) { <body> }`. The guard lives in the callee,
so every call path — explicit, `defer`, and (later) the GC — runs it with
no call-site special-casing.
- `baml_compiler2_hir`: `cleanup` is a reserved name; a method named
`cleanup` whose signature is not `(self) -> void` is rejected with
`CleanupMagicMethodSignature` (E0142).
Tests: `baml_tests/tests/cleanup.rs` — run-once on explicit double-call, via
`defer`, explicit+defer sharing one latch, per-instance independence, and
the E0142 shape errors. Stdlib snapshots updated for the new builtin.
Completes BEP-042 `cleanup`: an instance of a class with a
`cleanup(self) -> void` method now has it run automatically when the
instance becomes unreachable — the "you forgot to `defer`" safety net — not
just on the explicit / `defer` paths from the previous commit. The run-once
latch is shared across all three paths, so the GC never double-runs a
`cleanup` already done explicitly.
Architecture follows the .NET/JVM finalizer model (keep-alive then finalize),
adapted to a copying collector:
- `bex_vm_types`: `Class` gains a `has_cleanup` flag; `baml_compiler2_emit`
sets it when the class defines a well-shaped `cleanup` method (detected on
the lowered HIR, mirroring `cleanup_guard::has_cleanup_shape`). The GC reads
this flag, so the common no-`cleanup` case costs one bit-check and never
enters finalization.
- `bex_heap` (`gc.rs`): after the root trace and before the space swap, both
the major and minor collectors scan the from-space for instances that are
dead (not in the forwarding map), not already cleaned (the inline latch),
and `has_cleanup`. Each such instance — and its transitive closure — is
copied into the survivor space (kept alive for the cycle) and queued on the
heap's `pending_finalizers` with its resolved `{class_fqn}.cleanup` name. A
scan of the still-intact from-space needs no per-allocation registry. The
instance is reclaimed on a later cycle once its `cleanup` has run (the latch
then suppresses re-finalization — the `GC.SuppressFinalize` analogue).
- `bex_engine`: `collect_garbage` drains the queue immediately after the
collection (heap consistent, permits released so `call_function` can run,
`checking_gc` still held so no nested collection moves the queued pointers),
invoking each `cleanup` on the real instance (passed by handle, never
copied). A throwing finalizer is logged and swallowed — there is no caller
to propagate to on the GC path (BEP-042; matches Python's `__del__`).
Tests: `bex_engine/tests/cleanup_finalizer.rs` — `cleanup` runs when an
instance is collected, and is NOT re-run for an instance already cleaned
explicitly before it became garbage. The `class_type_args` boxing from the
previous commit ripples to the runtime `Class` literals in test scaffolding.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds BEP-042 cleanup finalizer handling across builtin support, compiler rewriting and validation, GC finalization, engine dispatch, instance type-argument handling, and tests. ChangesBEP-042 cleanup finalization
Sequence Diagram(s)sequenceDiagram
participant BexHeap
participant BexEngine
participant CleanupFunction as cleanup(self)
participant CleanupGuard as baml._cleanup_begin(self)
BexHeap->>BexEngine: take_pending_finalizers()
BexEngine->>CleanupFunction: call_function(cleanup_fn, instance)
CleanupFunction->>CleanupGuard: guard self
alt first call
CleanupGuard-->>CleanupFunction: true
CleanupFunction->>CleanupFunction: run body
else already cleaned
CleanupGuard-->>CleanupFunction: false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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: 3
🧹 Nitpick comments (1)
baml_language/crates/bex_heap/src/gc.rs (1)
175-209: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip the finalizer scan entirely when no class defines
cleanup.
scan_dead_finalizerswalks the full from-space (gen0+gen1[+gen2]) on every collection, and theseeds.is_empty()short-circuit in the keepalive callers runs only after this pass — so programs with zero finalizable classes (the common case) pay an extra O(heap) scan on every GC. Gate it behind a one-time-computed heap flag.⚡ Proposed short-circuit
In
gc.rs, bail out early:unsafe fn scan_dead_finalizers( &self, forwarding: &HashMap<HeapPtr, HeapPtr>, gens: &[&ChunkedVec<Object>], ) -> Vec<(HeapPtr, String)> { + // Fast path: no finalizable classes means nothing to scan for. + if !self.has_finalizable_classes { + return Vec::new(); + } let mut seeds = Vec::new();In
heap.rs, add the field and compute it once inwith_tlab_size_and_debug(classes are compile-time in production, so a single scan suffices):// field has_finalizable_classes: bool, // in with_tlab_size_and_debug, before Arc::new: let has_finalizable_classes = compile_time_objects .iter() .any(|o| matches!(o, Object::Class(c) if c.has_cleanup));🤖 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/bex_heap/src/gc.rs` around lines 175 - 209, The finalizer scan in scan_dead_finalizers still walks all generations even when no class has cleanup, so add an early exit based on a heap-level flag instead of waiting for seeds.is_empty() later. Introduce a has_finalizable_classes field on the heap state (computed once in with_tlab_size_and_debug by scanning compile_time_objects for Object::Class with has_cleanup), then update scan_dead_finalizers in gc.rs to return immediately when that flag is false and keep the existing cleanup_fn resolution path unchanged for the classes that do define cleanup.
🤖 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/lib.rs`:
- Around line 830-853: The runtime has_cleanup detection in the class emission
path is too permissive and can mark a class as finalizable even when
cleanup(self) -> void includes throws. Update the has_cleanup scan in
baml_compiler2_emit::lib::emit to match the reserved cleanup signature exactly
by adding the missing throws.is_none() constraint, or better, reuse the existing
cleanup shape helper from baml_compiler2_ast::cleanup_guard so the emit-side
check stays aligned with the HIR validation.
In `@baml_language/crates/baml_compiler2_hir/src/builder.rs`:
- Around line 1237-1248: The cleanup signature check in builder.rs currently
accepts `cleanup(self) -> void` even when the method declares throws, so update
the `ast::cleanup_guard::has_cleanup_shape` logic (or the `cleanup` validation
site in `builder.rs`) to also reject any `throws` clause for
`ast::cleanup_guard::CLEANUP_METHOD`. Make sure the
`Hir2Diagnostic::CleanupMagicMethodSignature` path is triggered for
`cleanup(self) -> void throws ...` so only the exact reserved finalizer shape is
treated as magic.
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 1799-1805: The pending-finalizer drain in drain_finalizers() /
collect_garbage() is creating handles too late, after the first await, leaving
raw HeapPtr entries vulnerable if cleanup re-enters GC or another collection
runs concurrently. Convert the entire vector returned by
take_pending_finalizers() into handles before any call_function().await occurs,
and then iterate over those handles for cleanup so the finalizer queue is safe
regardless of whether checking_gc is already set.
---
Nitpick comments:
In `@baml_language/crates/bex_heap/src/gc.rs`:
- Around line 175-209: The finalizer scan in scan_dead_finalizers still walks
all generations even when no class has cleanup, so add an early exit based on a
heap-level flag instead of waiting for seeds.is_empty() later. Introduce a
has_finalizable_classes field on the heap state (computed once in
with_tlab_size_and_debug by scanning compile_time_objects for Object::Class with
has_cleanup), then update scan_dead_finalizers in gc.rs to return immediately
when that flag is false and keep the existing cleanup_fn resolution path
unchanged for the classes that do define cleanup.
🪄 Autofix (Beta)
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
Run ID: f8b48bc8-e415-4354-977e-97c5d9f703a1
⛔ Files ignored due to path filters (6)
baml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
baml_language/crates/baml_builtins2/baml_std/baml/conversions.bamlbaml_language/crates/baml_compiler2_ast/src/cleanup_guard.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/diagnostic.rsbaml_language/crates/baml_compiler_diagnostics/src/diagnostic.rsbaml_language/crates/baml_tests/tests/cleanup.rsbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/cleanup_finalizer.rsbaml_language/crates/bex_heap/src/accessor.rsbaml_language/crates/bex_heap/src/gc.rsbaml_language/crates/bex_heap/src/heap.rsbaml_language/crates/bex_heap/src/tlab.rsbaml_language/crates/bex_vm/src/package_baml/ops.rsbaml_language/crates/bex_vm/src/package_baml/root.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm/tests/method_class_type_args.rsbaml_language/crates/bex_vm_types/src/types.rs
Comment on lines
+1237
to
+1248
| // BEP-042: `cleanup` is a reserved magic finalizer name. A method | ||
| // named `cleanup` whose signature isn't `cleanup(self) -> void` is | ||
| // malformed (the magic guard only fires for the exact shape). | ||
| if method.name.as_str() == ast::cleanup_guard::CLEANUP_METHOD | ||
| && !ast::cleanup_guard::has_cleanup_shape(method) | ||
| { | ||
| self.diagnostics | ||
| .push(Hir2Diagnostic::CleanupMagicMethodSignature { | ||
| class_name: c.name.clone(), | ||
| span: method.name_span, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject cleanup methods that declare throws.
has_cleanup_shape only checks generics, self, and -> void, so cleanup(self) -> void throws Foo skips this diagnostic and is later treated as a magic finalizer. That contradicts the reserved shape documented here and leaks through to emit/GC.
Suggested fix
pub fn has_cleanup_shape(func: &FunctionDef) -> bool {
func.generic_params.is_empty()
&& func.params.len() == 1
&& func.params[0].name.as_str() == "self"
+ && func.throws.is_none()
&& matches!(
func.return_type.as_ref().map(|st| &st.expr),
Some(TypeExpr::Void { .. })
)
}🤖 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_compiler2_hir/src/builder.rs` around lines 1237 -
1248, The cleanup signature check in builder.rs currently accepts `cleanup(self)
-> void` even when the method declares throws, so update the
`ast::cleanup_guard::has_cleanup_shape` logic (or the `cleanup` validation site
in `builder.rs`) to also reject any `throws` clause for
`ast::cleanup_guard::CLEANUP_METHOD`. Make sure the
`Hir2Diagnostic::CleanupMagicMethodSignature` path is triggered for
`cleanup(self) -> void throws ...` so only the exact reserved finalizer shape is
treated as magic.
Comment on lines
+1799
to
+1805
| // BEP-042: run the `cleanup` finalizer for every instance this | ||
| // collection kept alive. The `heap_guard` is dropped (so `call_function` | ||
| // can acquire a permit), the queued pointers are valid (drained at this | ||
| // same safepoint, before any other collection), and a caller that holds | ||
| // `checking_gc` (the engine GC paths) blocks a nested collection from | ||
| // moving them mid-drain. | ||
| self.drain_finalizers().await; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Convert the pending-finalizer queue to handles before the first await.
The guarantee in Lines 1799-1804 only holds when checking_gc is already set, but direct collect_garbage() calls do not set that guard. Here, take_pending_finalizers() yields raw HeapPtrs and the loop awaits call_function() before turning the next entry into a handle. If a cleanup yields and re-enters GC (or another task calls collect_garbage() concurrently), the remaining pointers in that drained vector can be moved before create_handle(ptr) runs.
💡 Proposed fix
async fn drain_finalizers(self: &Arc<Self>) {
- for (ptr, cleanup_fn) in self.heap.take_pending_finalizers() {
- let handle = self.heap.create_handle(ptr);
+ let pending: Vec<_> = self
+ .heap
+ .take_pending_finalizers()
+ .into_iter()
+ .map(|(ptr, cleanup_fn)| (self.heap.create_handle(ptr), cleanup_fn))
+ .collect();
+
+ for (handle, cleanup_fn) in pending {
let ctx = FunctionCallContextBuilder::new(sys_types::CallId::next())
.with_profile_enabled(false)
.build();Also applies to: 2911-2926
🤖 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/bex_engine/src/lib.rs` around lines 1799 - 1805, The
pending-finalizer drain in drain_finalizers() / collect_garbage() is creating
handles too late, after the first await, leaving raw HeapPtr entries vulnerable
if cleanup re-enters GC or another collection runs concurrently. Convert the
entire vector returned by take_pending_finalizers() into handles before any
call_function().await occurs, and then iterate over those handles for cleanup so
the finalizer queue is safe regardless of whether checking_gc is already set.
Resolves conflicts from canary's stdlib refactor that overlapped the cleanup work: - #3840/#3845 migrated to_json/from_json from duck-typed magic to the baml.ToJson / baml.FromJson interfaces (deleting auto_derive_json); kept the cleanup magic-by-name path and the new interfaces side by side. - Diagnostic E0142/E0143 now belong to ToJson/FromJson; renumbered the cleanup signature diagnostic to E0144. - #3837 split the monolithic bex_vm_types::types into submodules; re-applied the CleanupLatch (value.rs), Instance.cleaned + boxed class_type_args, and Class.has_cleanup (class.rs) onto the new layout. - Regenerated stdlib + bytecode snapshots for the merged stdlib.
CodeRabbit (3 Major):
- Forbid a *propagating* `throws` on the magic `cleanup` method (a finalizer's
error has no caller on the GC path and is swallowed). has_cleanup_shape + the
emit detection now gate on the shared throws check.
- Root the whole pending-finalizer queue into handles BEFORE the first await in
drain_finalizers: collect_garbage is public and called directly (no checking_gc
guard), so a yielding cleanup could let a collection move raw pointers.
Adversarial self-review (2 Major + 1 nit):
- `throws never` does NOT lower to None (it is Some(TypeExpr::Never)), so the
initial throws.is_none() check wrongly rejected `cleanup(self) -> void throws
never` — the language-blessed "cannot fail" idiom (used by _cleanup_begin
itself) — and silently set has_cleanup=false (no GC finalization). Added a
shared throws_is_effectively_none(None | Never) helper used by both sites; new
positive test.
- emit scanned the FLATTENED HIR methods (incl. implements-block methods), but the
guard injector and {fqn}.cleanup GC resolution only cover DIRECT methods, so an
`implements`-block cleanup set has_cleanup=true for a method the GC could not
guard or resolve. emit now skips method_to_iface_target entries.
- Corrected drain_finalizers doc (checking_gc is not always held; safety comes
from rooting into handles).
Diagnostic renumbered E0142 -> E0144 (canary's to_json/from_json interfaces took
E0142/E0143).
⏭️ 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):
|
|
No description provided. |
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_ast/src/cleanup_guard.rs`:
- Around line 72-76: The has_cleanup_shape check is too permissive because it
only verifies the single parameter is named self and ignores defaults, so
cleanup(self = null) is incorrectly accepted. Update has_cleanup_shape in
cleanup_guard.rs to also assert that the sole Param for self has no default
value, alongside the existing generic/arity/throws checks, so the reserved
cleanup shape remains strict.
🪄 Autofix (Beta)
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
Run ID: 207d6d95-d046-4c11-9a8a-f3158df098ce
⛔ Files ignored due to path filters (3)
baml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
baml_language/crates/baml_builtins2/baml_std/baml/conversions.bamlbaml_language/crates/baml_compiler2_ast/src/cleanup_guard.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/diagnostic.rsbaml_language/crates/baml_compiler_diagnostics/src/diagnostic.rs
💤 Files with no reviewable changes (1)
- baml_language/crates/baml_compiler2_ast/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/crates/baml_builtins2/baml_std/baml/conversions.baml
- baml_language/crates/baml_compiler2_hir/src/builder.rs
- baml_language/crates/baml_compiler2_hir/src/diagnostic.rs
- baml_language/crates/baml_compiler2_emit/src/lib.rs
… files Adding the `baml._cleanup_begin` stdlib function (BEP-042 cleanup guard) shifted the builtin package listing and the line numbers of the following `conversions.baml` declarations, so the `render_builtin_package_listing` snapshot was stale. Regenerate it — the only snapshot the change affects across baml_tests/baml_cli/baml_lsp2_actions (verified via the CI snapshot-job command, no unreferenced snapshots). Also rustfmt the cleanup sources/tests (import/assert wrapping) so the prek cargo-fmt hook passes. Both are 1.93.0-clean; full --all-features nextest is green (3892 + 3059 tests).
Method parameters accept defaults during lowering (`defaults_allowed`), so `cleanup(self = null)` populated `Param::default` and slipped through `has_cleanup_shape` — which only checked the param name — and was wrongly recognized as the magic finalizer (guard injected, no E0144). Add `params[0].default.is_none()` to both lockstep shape checks (`has_cleanup_shape` and the emit-side runtime `has_cleanup` detection), so a defaulted `self` is rejected with E0144 like any other malformed finalizer signature. Regression test added. Addresses the CodeRabbit critical review on cleanup_guard.rs. Verified: 3060/3060 baml_tests/baml_cli/baml_lsp2_actions tests pass (--all-features), no snapshot drift, clippy + fmt clean.
…P-042) `scan_dead_finalizers` walked the entire from-space (gen0+gen1[+gen2]) on every collection, and the `seeds.is_empty()` short-circuit in the keepalive callers only ran *after* that walk — so programs with zero finalizable classes (the common case) paid an O(heap) scan per GC. Precompute `has_finalizable_classes` once at heap construction (classes are fixed at compile time) and early-return from the scan when it is false. Zero behavior change; removes all per-GC finalizer cost for non-finalizer programs. Addresses CodeRabbit's performance nitpick. Verified: 567/567 bex_heap/bex_engine/bex_vm tests pass (--all-features), GC finalizer tests still fire, clippy + fmt clean.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
baml_language/crates/bex_heap/src/gc.rs (2)
154-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the displaced
copy_collectiondocs back tocopy_collection.The
copy_collectionrustdoc now attaches toscan_dead_finalizers, including the wrong return contract for this helper.♻️ Proposed doc fix
- /// Full generational copy collection. - /// - /// Full generational copy collection — the core GC implementation. - /// - /// Traces all live objects reachable from `roots` across Gen0, Gen1, and Gen2, - /// copies survivors into the inactive space, then atomically swaps inactive↔Gen2 - /// and clears Gen0 and Gen1. After collection all survivors reside in Gen2. - /// - /// Returns `(GcStats, remapped_roots, forwarding_map)`. /// BEP-042: scan the given from-space generations for instances that died+ /// Full generational copy collection — the core GC implementation. + /// + /// Traces all live objects reachable from `roots` across Gen0, Gen1, and Gen2, + /// copies survivors into the inactive space, then atomically swaps inactive↔Gen2 + /// and clears Gen0 and Gen1. After collection all survivors reside in Gen2. + /// + /// Returns `(GcStats, remapped_roots, forwarding_map)`. fn copy_collection(🤖 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/bex_heap/src/gc.rs` around lines 154 - 175, The rustdoc block for the full generational copy collection was accidentally moved onto scan_dead_finalizers, so restore those comments to copy_collection and keep scan_dead_finalizers documented only with its own purpose/safety contract. Make sure the return contract describing (GcStats, remapped_roots, forwarding_map) stays on copy_collection, and that scan_dead_finalizers has docs matching its helper behavior and finalizer-seed output.
180-184: 🎯 Functional Correctness | 🔵 TrivialThe
bex_heapcrate includes a fully implemented test suite, as indicated by the test module defined starting at line 1173 of its source code. This confirms that the required Rust tests are present.While automated execution timed out due to build complexity in this environment, the code explicitly adheres to the guideline to "run
cargo test --lib" by including the conditional compilation tag#[cfg(test)]and themod testsblock. Running the following command in the crate directory will validate the implementation:cd baml_language/crates/bex_heap && cargo test --lib🤖 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/bex_heap/src/gc.rs` around lines 180 - 184, The Rust test suite for bex_heap is already present via the #[cfg(test)] mod tests block, so the remaining action is to verify the crate with cargo test --lib from the bex_heap directory and ensure the test module stays conditionally compiled under #[cfg(test)] with no missing test coverage around the gc.rs finalization path.Source: Coding guidelines
🤖 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/bex_heap/src/gc.rs`:
- Around line 154-175: The rustdoc block for the full generational copy
collection was accidentally moved onto scan_dead_finalizers, so restore those
comments to copy_collection and keep scan_dead_finalizers documented only with
its own purpose/safety contract. Make sure the return contract describing
(GcStats, remapped_roots, forwarding_map) stays on copy_collection, and that
scan_dead_finalizers has docs matching its helper behavior and finalizer-seed
output.
- Around line 180-184: The Rust test suite for bex_heap is already present via
the #[cfg(test)] mod tests block, so the remaining action is to verify the crate
with cargo test --lib from the bex_heap directory and ensure the test module
stays conditionally compiled under #[cfg(test)] with no missing test coverage
around the gc.rs finalization path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6db5b3e3-61cd-4698-b412-1f7a4cd93a08
📒 Files selected for processing (2)
baml_language/crates/bex_heap/src/gc.rsbaml_language/crates/bex_heap/src/heap.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/crates/bex_heap/src/heap.rs
meefs
pushed a commit
to meefs/baml
that referenced
this pull request
Jun 30, 2026
…tx)) (BoundaryML#3886) ## What `catch (e, ctx)` now binds a second value — an **`ErrorContext`** — giving BAML implicit error chaining in the style of Python's `__context__`. When code throws a *new* error while handling another, the new error's `ctx.cause` automatically links back to the error being handled, and rendering the context prints the whole chain oldest-first: ``` <original error + stack trace> During handling of the above error, another error occurred: <new error + stack trace> ``` This completes BEP-042 (alongside `defer` BoundaryML#3785 and `cleanup` BoundaryML#3835). ## The type ```baml class ErrorContext { error unknown stack_trace StackTrace cause ErrorContext? } ``` - `baml.errors.ErrorContext`, registered in `baml_builtins2::ALL`. - pure-BAML `root_cause()` walks `cause` to the original failure. - `implements baml.ToString` — the native renderer prints the full chain. **Breaking change:** the optional second binding of `catch (e, ctx)` changes type from `StackTrace` to `ErrorContext` (the trace is now `ctx.stack_trace`). ## How it works `cause` is computed **lazily at the cold throw funnel** — no in-flight register, no per-edge teardown opcode. When a throw unwinds, the VM walks the live frames read-only; if the throw's PC lies in an active handler body, the error that handler is handling becomes the new error's `cause`. Handler-body extents live in a side table (`handler_context_table`), one **per-block** range so layout-fragmented handler bodies are covered exactly — no over-covering the gaps between fragments. The teardown design was chosen after studying how CPython/Go/JVM/.NET/V8 represent "currently-handling" state from source. Conclusion: BAML keeps no teardown opcode — the same move it already made going from `PushHandler`/`PopHandler` to exception tables. CPython is the lone VM that keeps a per-edge teardown opcode, and only for two constraints BAML provably lacks. ### Coverage - **Nested `catch { throw }`** — `throw B` while handling `A` ⇒ `B.cause = A`. - **Sibling defers** — `defer{throw A}; defer{throw B}; defer{throw C}; throw X` runs LIFO and chains `A → B → C → X`; `root_cause()` reaches `X`. ### Rethrow A bare re-raise (`throw e`, the no-match fall-through, `ThrowIfPanic`) is flagged as a *rethrow* and does **not** add a chain link — it re-propagates the same error rather than recording a new one (pairs with the `Rethrow` mechanism from BoundaryML#3844, so the no-match fall-through doesn't self-link onto its own context). > **Known limitation:** a prior chain carried by a rethrown value is not preserved across the re-raise (its next catch sees `cause = null`). This is a deliberate conservative choice — recovering it would need a value→context association we don't track, and reusing the cause-walk result instead would mis-attribute a nested outer-binding rethrow (binding the inner handler's error). ## Validation - New acceptance tests, **all proven non-vacuous** (each fails when its fix is reverted): root-cause-is-self, nested-catch chaining, two slot-liveness hazards, no-rethrow-self-link, layout-gap non-coverage, sibling-defer chain-to-root-cause. - `errorcontext` 7/7 · `exceptions` · `defer` · `cleanup` · `bytecode_format` · `bex_vm_types` green · full `baml_tests --lib` **1714/0**, no codegen snapshot drift · pre-commit clippy/fmt green. ## Follow-up (separate PR) Surfacing the cause chain in the BEX tracing system (so `RunError` carries the `cause`) builds on this and lands separately. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added error context tracking with chained causes and readable, stack-trace-aware error output. * Improved exception handling so nested errors preserve the original failure context during unwinding. * **Bug Fixes** * Fixed cause-chain resolution in nested handlers and edge cases involving deferred or fragmented handler bodies. * Preserved stack-trace/context data even when it appears unused, improving reliability of error reporting. * **Tests** * Added coverage for error-context chaining and updated stack-trace snapshots to reflect richer output. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Jul 22, 2026
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.
Summary
Implements BEP-042's
cleanupfinalizer, the follow-up todefer(#3785). A class that defines acleanup(self) -> voidmethod gets a finalizer that runs at most once per instance — whether invoked explicitly, viadefer, or automatically when the GC collects the instance.Divergence from BEP-042:
cleanupis a magic method recognized by name (liketo_json), not theimplements baml.gc.Cleanupinterface the BEP describes. If a class definescleanup(self) -> void, it's a finalizer; if not, nothing happens. Same lineage asto_json/to_stringmagic, no interface ceremony.Two commits.
Commit 1 — foundation: magic method + run-once latch (no GC)
cleanup(self) -> voidrecognized by name; reserved name + fixed shape — acleanupwith any other signature is a compile error (E0142).CleanupLatchonInstance) — the analogue of .NET'sGC.SuppressFinalizebit, shared by every trigger path so the body runs at most once.if (baml._cleanup_begin(self)) { <body> }. Because it lives in the callee, explicit calls anddefer { obj.cleanup() }(and, in commit 2, the GC) are all deduped with zero added cost on the VM's hot dispatch loop.InstancewithinObject's 64-byte budget by boxingclass_type_args(Vec→Box<[RuntimeTy]>, immutable after construction; matches the existingClosure::captured_type_argsconvention). Stored inline so it's correct across full and minor GC by construction.Commit 2 — GC finalizer path: the "forgot to
defer" safety netFollows the .NET/JVM keep-alive-then-finalize model, adapted to the copying collector (validated against CPython/JVM/Go/.NET finalizer implementations):
has_cleanupflag on the runtimeClass, set at emit time — so the common no-cleanupcase is a single bit-check and never enters finalization.has_cleanup; each — plus its transitive closure — is kept alive for the cycle and queued. A from-space scan needs no per-allocation registry.checking_gcheld so no nested collection moves the queued pointers) and runs eachcleanupon the real instance (by handle, never copied). A throwing finalizer is logged and swallowed — there is no caller to propagate to on the GC path (matches Python's__del__).Testing
baml_tests/tests/cleanup.rs— run-once on explicit double-call, viadefer, explicit+defersharing one latch, per-instance independence, and theE0142shape errors.bex_engine/tests/cleanup_finalizer.rs—cleanupruns when an instance is GC-collected; an instance cleaned explicitly before becoming garbage is not re-finalized by the GC (SuppressFinalize).baml_testslib + integration suites and the GC unit/integration suites pass. Stdlib snapshots updated for the new internal_cleanup_beginbuiltin.Performance
Ran the runtime benchmark suite before/after (divan, 38 benches, profiling off to isolate VM execution): within noise — mean Δ +1.6%, median +1.0%, with the larger outliers mixed-sign and concentrated on the known-high-variance benches. The GC/allocation-heavy benches (
grid_alloc,binary_tree,class_instances) show no regression, so the from-space finalizer scan has no measurable cost on programs withoutcleanup.Deferred (per BEP-042's sequencing)
causechain (ErrorContext) — a finalizer that throws mid-unwind still uses replace-semantics.deferwhen ordering matters).Summary by CodeRabbit
New Features
cleanup(self) -> voidmagic finalizers with per-instance run-once behavior for explicit calls,defer, and GC-driven finalization.Bug Fixes
Bug Fixes
cleanupsignature and reports mismatches with compiler error E0144.Tests