refactor(tracing): trim the event-stream surface for a fresh start#3616
Draft
hellovai wants to merge 10 commits into
Draft
refactor(tracing): trim the event-stream surface for a fresh start#3616hellovai wants to merge 10 commits into
hellovai wants to merge 10 commits into
Conversation
…resh start
Rips out the old observability surface ahead of rebuilding it (see
bex-event-stream-design). No behavior is preserved — tracing is inert
after this change; the goal is a clean slate that still compiles.
Deleted:
- bex_events_native crate (JSONL/stderr EventSink publisher)
- bex_events: event_store (Mutex<CollectorStore>), serialize/JSONL,
RuntimeEvent/EventKind/SpanContext/FunctionLog, SpanId
- bridge_ctypes event_encode.rs (protobuf-FFI event wire)
- baml_lsp_server playground_event_sink.rs + FanOutEventSink
- Rust suites bex_engine/tests/{tracing,event_system}.rs and the
python/node tracing+collector test suites
Rewired to keep the workspace + SDKs compiling:
- EventSink/event_sink param dropped from BexEngine/BexProject/new_lsp
and every caller; C-FFI flush_events() is now a no-op (ABI symbol kept)
- HostSpanManager kept but inert (depth tracker, no sink)
- SDK Collector/FunctionLog/Timing/Usage/LLMCall kept as no-op shims so
the python/node public surfaces still build
VM-layer span plumbing (current_span_context, Function.trace, SpanNotify)
is deliberately left intact and handled as no-ops to avoid touching the
hot exec loop; it will be revisited in the rebuild.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
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:
📝 WalkthroughWalkthroughRemoves tracing/event infrastructure across the workspace. BexEngine/VM drop span notifications and event sinks. Compiler/bytecode eliminate visualization and custom-event instructions. Bridges, SDKs, LSP, CLI, and tests are updated accordingly. Workspace adopts forked AWS/Google crates and adjusts dependencies. ChangesEvent-system removal and related wiring
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
baml_language/crates/bex_engine/src/lib.rs (1)
1109-1125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the rustdoc example to match the new
call_functionsignature.The snippet still shows the removed
Noneargument and omits the requiredFunctionCallContextandcopy_objectsparameters.Suggested doc fix
-/// let result = engine.call_function("get_user", vec![ -/// "Alice".into(), -/// 42i64.into(), -/// ], None).await?; +/// let ctx = +/// bex_engine::FunctionCallContextBuilder::new(bex_engine::CallId::next()).build(); +/// let result = engine +/// .call_function("get_user", vec!["Alice".into(), 42i64.into()], ctx, true) +/// .await?;🤖 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 1109 - 1125, The doc example for call_function is outdated: update the example call to match the new signature of Engine::call_function by supplying a FunctionCallContext and the copy_objects parameter (and remove the old trailing None). Locate the example around the call_function docs and construct a FunctionCallContext (or use an existing helper/context value) and pass an explicit boolean for copy_objects to the call_function invocation so the example compiles with the current call_function(FunctionCallContext, Vec<BexExternalValue>, bool) signature.baml_language/crates/baml_cli/src/run_command.rs (1)
292-409:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject or remove
--log-filenow that the sink is gone.
RunArgsstill exposes--log-fileon Lines 150-152, but this path no longer readsself.log_file, so the flag now succeeds and produces nothing. That's a silent CLI contract break; either remove the option from the parser/help in this PR or fail fast here when it is passed.Suggested guard in the changed path
fn run_with_reporter(&self, reporter: &Reporter) -> Result<crate::ExitCode> { + if let Some(path) = &self.log_file { + anyhow::bail!( + "`--log-file {}` is temporarily unavailable while runtime tracing is disabled.", + path.display() + ); + } + // Dispatch modes are mutually exclusive. Positional target / // `-f` (one or many) / `-e` all replace each other.🤖 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_cli/src/run_command.rs` around lines 292 - 409, The CLI still accepts --log-file via RunArgs but run_with_reporter no longer uses self.log_file, causing a silent no-op; either remove the flag from RunArgs/argument parser and its help text (and delete the self.log_file field) or add a fail-fast guard in run_with_reporter that checks if self.log_file.is_some() and returns an error (anyhow::bail!) explaining that --log-file is no longer supported; locate this in the run_with_reporter method and the RunArgs struct to implement the chosen fix.
🧹 Nitpick comments (4)
baml_language/crates/bex_engine/src/lib.rs (1)
2418-2437: ⚡ Quick winAdd a focused unit test for the inert
Event/SpanNotifypath.
VmExecState::Eventstill depends on pushingnullto keep the VM stack balanced, andSpanNotifystill needs to resume cleanly. That contract is subtle enough to deserve a library test now that the tracing-specific suites are gone.As per coding guidelines,
**/*.rs: Prefer writing Rust unit tests over integration tests where possible.🤖 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 2418 - 2437, Add a focused unit test that exercises the inert Event and SpanNotify branches so the VM stack contract is verified: create a small VM state, invoke the executor path that yields VmExecState::Event and assert that thread.vm.stack has a pushed Value::NULL and execution resumes without altering stack depth; likewise simulate VmExecState::SpanNotify and assert resume is clean (no stack change). Target the executor function handling these states (refer to VmExecState::Event, thread.vm.stack.push(Value::NULL), and VmExecState::SpanNotify) and write the tests as Rust unit tests in the same module to keep them local.baml_language/crates/bex_events/src/collector.rs (1)
29-55: ⚡ Quick winAdd a small unit test for the no-op collector contract.
These methods are now the entire compatibility surface for bindings, so a local unit test should pin the expected empty/
None/0behavior.Based on learnings/coding guidelines,
**/*.rs: Prefer writing Rust unit tests over integration tests where possible.🤖 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_events/src/collector.rs` around lines 29 - 55, Add a unit test in the same file (#[cfg(test)] mod tests) that constructs the collector instance and asserts the no-op contract: call track(&SpanId::default()) to ensure it doesn't panic, assert_eq!(collector.logs(), Vec::new()), assert!(collector.last().is_none()), assert_eq!(collector.usage(), Usage::default()), assert_eq!(collector.clear(), 0), and assert!(collector.id("any").is_none()); use the actual collector type from this file (e.g., Collector) and the SpanId, FunctionLog, and Usage types already in scope.baml_language/sdks/nodejs/bridge_nodejs/src/lib.rs (1)
17-19: 💤 Low valueStale comment references removed event-sink/tracing. This
init()block no longer initializes logging viaBAML_TRACE_FILEandbridge_cffi's event sink (both removed in this PR), so the comment is now misleading. Consider updating it to reflect that only host-dispatch/release callbacks are wired here.🤖 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/sdks/nodejs/bridge_nodejs/src/lib.rs` around lines 17 - 19, The existing comment above the init() block is stale — it mentions BAML_TRACE_FILE, bridge_cffi, and an event sink that were removed; update the comment to accurately describe what init() now does by removing references to tracing/event-sink and stating that it only wires the host-dispatch and release callbacks (e.g., mention init(), host_dispatch wiring and release callbacks registration) so future readers understand the current behavior.baml_language/sdks/python/rust/bridge_python/src/runtime.rs (1)
82-84: 💤 Low valueParam docs no longer match behavior. The
ctx/collectorsdocstrings still claim host-trace nesting and per-call tracking, but both are now accepted-and-ignored no-ops. Same applies to the duplicated block at Lines 137-139. Consider noting they are retained for ABI compatibility only.🤖 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/sdks/python/rust/bridge_python/src/runtime.rs` around lines 82 - 84, Update the doc comments in runtime.rs for the functions referencing `ctx` and `collectors` (the doc block around the `ctx`/`collectors` params near the comment starting at line ~82 and the duplicated block around lines ~137-139) to state that `ctx` and `collectors` are accepted but currently ignored (no-ops) and are retained only for ABI/backward-compatibility; replace the outdated claims about host-trace nesting and per-call tracking with a concise note that these parameters are no-ops and will not affect tracing, and keep the rest of the parameter docs unchanged.
🤖 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.
Outside diff comments:
In `@baml_language/crates/baml_cli/src/run_command.rs`:
- Around line 292-409: The CLI still accepts --log-file via RunArgs but
run_with_reporter no longer uses self.log_file, causing a silent no-op; either
remove the flag from RunArgs/argument parser and its help text (and delete the
self.log_file field) or add a fail-fast guard in run_with_reporter that checks
if self.log_file.is_some() and returns an error (anyhow::bail!) explaining that
--log-file is no longer supported; locate this in the run_with_reporter method
and the RunArgs struct to implement the chosen fix.
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 1109-1125: The doc example for call_function is outdated: update
the example call to match the new signature of Engine::call_function by
supplying a FunctionCallContext and the copy_objects parameter (and remove the
old trailing None). Locate the example around the call_function docs and
construct a FunctionCallContext (or use an existing helper/context value) and
pass an explicit boolean for copy_objects to the call_function invocation so the
example compiles with the current call_function(FunctionCallContext,
Vec<BexExternalValue>, bool) signature.
---
Nitpick comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 2418-2437: Add a focused unit test that exercises the inert Event
and SpanNotify branches so the VM stack contract is verified: create a small VM
state, invoke the executor path that yields VmExecState::Event and assert that
thread.vm.stack has a pushed Value::NULL and execution resumes without altering
stack depth; likewise simulate VmExecState::SpanNotify and assert resume is
clean (no stack change). Target the executor function handling these states
(refer to VmExecState::Event, thread.vm.stack.push(Value::NULL), and
VmExecState::SpanNotify) and write the tests as Rust unit tests in the same
module to keep them local.
In `@baml_language/crates/bex_events/src/collector.rs`:
- Around line 29-55: Add a unit test in the same file (#[cfg(test)] mod tests)
that constructs the collector instance and asserts the no-op contract: call
track(&SpanId::default()) to ensure it doesn't panic,
assert_eq!(collector.logs(), Vec::new()), assert!(collector.last().is_none()),
assert_eq!(collector.usage(), Usage::default()), assert_eq!(collector.clear(),
0), and assert!(collector.id("any").is_none()); use the actual collector type
from this file (e.g., Collector) and the SpanId, FunctionLog, and Usage types
already in scope.
In `@baml_language/sdks/nodejs/bridge_nodejs/src/lib.rs`:
- Around line 17-19: The existing comment above the init() block is stale — it
mentions BAML_TRACE_FILE, bridge_cffi, and an event sink that were removed;
update the comment to accurately describe what init() now does by removing
references to tracing/event-sink and stating that it only wires the
host-dispatch and release callbacks (e.g., mention init(), host_dispatch wiring
and release callbacks registration) so future readers understand the current
behavior.
In `@baml_language/sdks/python/rust/bridge_python/src/runtime.rs`:
- Around line 82-84: Update the doc comments in runtime.rs for the functions
referencing `ctx` and `collectors` (the doc block around the `ctx`/`collectors`
params near the comment starting at line ~82 and the duplicated block around
lines ~137-139) to state that `ctx` and `collectors` are accepted but currently
ignored (no-ops) and are retained only for ABI/backward-compatibility; replace
the outdated claims about host-trace nesting and per-call tracking with a
concise note that these parameters are no-ops and will not affect tracing, and
keep the rest of the parameter docs unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0f17b05e-fc68-4b1d-9a55-74bb308796cf
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
baml_language/Cargo.tomlbaml_language/crates/baml_cli/Cargo.tomlbaml_language/crates/baml_cli/src/pack_command.rsbaml_language/crates/baml_cli/src/run_command.rsbaml_language/crates/baml_cli/src/test_command.rsbaml_language/crates/baml_exec/src/dispatch.rsbaml_language/crates/baml_lsp_server/Cargo.tomlbaml_language/crates/baml_lsp_server/src/lib.rsbaml_language/crates/baml_lsp_server/src/playground_event_sink.rsbaml_language/crates/baml_pack_host/Cargo.tomlbaml_language/crates/baml_pack_host/src/main.rsbaml_language/crates/baml_tests/benches/cache_profile.rsbaml_language/crates/baml_tests/benches/runtime_benchmark.rsbaml_language/crates/baml_tests/src/engine.rsbaml_language/crates/baml_tests/tests/exceptions.rsbaml_language/crates/baml_tests/tests/parallel_scaling.rsbaml_language/crates/baml_tests/tests/spawn_array_race.rsbaml_language/crates/baml_tests/tests/spawn_parallel.rsbaml_language/crates/bex_engine/Cargo.tomlbaml_language/crates/bex_engine/src/function_call_context.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/build_request.rsbaml_language/crates/bex_engine/tests/cancellation.rsbaml_language/crates/bex_engine/tests/collect_tests.rsbaml_language/crates/bex_engine/tests/common/mod.rsbaml_language/crates/bex_engine/tests/concurrent.rsbaml_language/crates/bex_engine/tests/early_yield.rsbaml_language/crates/bex_engine/tests/event_system.rsbaml_language/crates/bex_engine/tests/function_resolution.rsbaml_language/crates/bex_engine/tests/future_cleanup.rsbaml_language/crates/bex_engine/tests/gc.rsbaml_language/crates/bex_engine/tests/gc_during_future_op.rsbaml_language/crates/bex_engine/tests/host_argv.rsbaml_language/crates/bex_engine/tests/host_value_callable.rsbaml_language/crates/bex_engine/tests/llm_render.rsbaml_language/crates/bex_engine/tests/media_roundtrip.rsbaml_language/crates/bex_engine/tests/orchestration.rsbaml_language/crates/bex_engine/tests/registry.rsbaml_language/crates/bex_engine/tests/tracing.rsbaml_language/crates/bex_events/Cargo.tomlbaml_language/crates/bex_events/src/collector.rsbaml_language/crates/bex_events/src/event_store.rsbaml_language/crates/bex_events/src/lib.rsbaml_language/crates/bex_events/src/serialize.rsbaml_language/crates/bex_events/src/types.rsbaml_language/crates/bex_events_native/Cargo.tomlbaml_language/crates/bex_events_native/src/lib.rsbaml_language/crates/bex_project/Cargo.tomlbaml_language/crates/bex_project/src/bex.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/mod.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_project/src/project.rsbaml_language/crates/bridge_cffi/Cargo.tomlbaml_language/crates/bridge_cffi/src/ffi/objects.rsbaml_language/crates/bridge_cffi/src/host_spans.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_ctypes/Cargo.tomlbaml_language/crates/bridge_ctypes/src/event_encode.rsbaml_language/crates/bridge_ctypes/src/lib.rsbaml_language/crates/bridge_wasm/Cargo.tomlbaml_language/crates/bridge_wasm/src/lib.rsbaml_language/crates/bridge_wasm/src/wasm_playground.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/host_span_manager.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.tsbaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/rust/bridge_python/src/runtime.rsbaml_language/sdks/python/rust/bridge_python/src/types/host_span_manager.rsbaml_language/sdks/python/tests/test_collector.pybaml_language/sdks/python/tests/test_engine.pybaml_language/sdks/python/tests/test_tracing.py
💤 Files with no reviewable changes (38)
- baml_language/crates/bex_engine/tests/collect_tests.rs
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/crates/bex_events/Cargo.toml
- baml_language/crates/bex_project/Cargo.toml
- baml_language/crates/bridge_wasm/Cargo.toml
- baml_language/crates/bex_engine/Cargo.toml
- baml_language/crates/baml_pack_host/Cargo.toml
- baml_language/crates/bridge_ctypes/src/event_encode.rs
- baml_language/crates/bex_engine/tests/tracing.rs
- baml_language/crates/bex_events/src/serialize.rs
- baml_language/crates/baml_pack_host/src/main.rs
- baml_language/crates/bex_events_native/Cargo.toml
- baml_language/crates/bex_events/src/event_store.rs
- baml_language/crates/bex_project/src/bex.rs
- baml_language/crates/bridge_cffi/Cargo.toml
- baml_language/crates/bex_engine/tests/event_system.rs
- baml_language/crates/bex_engine/tests/host_argv.rs
- baml_language/crates/bex_engine/tests/function_resolution.rs
- baml_language/crates/baml_lsp_server/Cargo.toml
- baml_language/crates/bex_events/src/types.rs
- baml_language/crates/baml_lsp_server/src/lib.rs
- baml_language/crates/bridge_ctypes/Cargo.toml
- baml_language/crates/bex_events_native/src/lib.rs
- baml_language/crates/bex_engine/tests/orchestration.rs
- baml_language/sdks/python/tests/test_tracing.py
- baml_language/crates/bridge_wasm/src/wasm_playground.rs
- baml_language/crates/baml_cli/Cargo.toml
- baml_language/crates/bex_engine/tests/early_yield.rs
- baml_language/sdks/python/tests/test_collector.py
- baml_language/crates/baml_lsp_server/src/playground_event_sink.rs
- baml_language/crates/bridge_ctypes/src/lib.rs
- baml_language/crates/bex_engine/tests/gc.rs
- baml_language/crates/bex_engine/tests/registry.rs
- baml_language/Cargo.toml
- baml_language/crates/bex_engine/tests/concurrent.rs
- baml_language/crates/bex_engine/src/function_call_context.rs
- baml_language/crates/bex_engine/tests/cancellation.rs
Strips the remaining tracing bias out of the VM so the core executes without any observability hooks (clean slate for the rebuild). Removed: - BexVm::traced_frames and BexVm::current_span_context fields (+ inits) - Function.trace field and every producer, incl. the @trace/LLM lowering (compiled_fn.trace = true) in baml_compiler2_emit - VmExecState::SpanNotify variant and the SpanNotification enum - Call-enter arg snapshot (is_traced/trace_data) + FunctionEnter yield - Return-path span_exit logic + FunctionExit yield - Both traced_frames cleanup loops in the exception-unwind path - SpanNotify match arms in bex_engine ($init loop + dispatch) and tools_onionskin's VM runner - bex_vm's now-unused bex_events dependency The Call/Return hot path now does zero observability work — no branch, no snapshot, no yield. Removing Function.trace changes the Function borsh layout, so packed .baml artifacts must be recompiled. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/bex_vm/src/vm.rs (1)
2358-2451: ⚡ Quick winAdd a unit test for unwind past the interrupt boundary.
This branch mutates
interrupt_frame, which changes how laterReturninstructions are interpreted. A small unit test here that drivesinterrupt()through an unwind path would make regressions obvious; please also runcargo test --libbefore merge.As per coding guidelines,
**/*.rs: Prefer writing Rust unit tests over integration tests where possibleandAlways run cargo test --lib if you changed any Rust code`.🤖 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_vm/src/vm.rs` around lines 2358 - 2451, Add a Rust unit test in the same module as vm.rs (e.g. fn test_unwind_past_interrupt_boundary) that constructs a VM state with at least two frames, sets up a bytecode frame that will throw (so unwinding will pop a frame past the current interrupt boundary), calls the VM interrupt() to set self.interrupt_frame, triggers the exception/unwind path exercised in the shown code, and then asserts that self.interrupt_frame has been cleared and that subsequent Return instruction handling still behaves as expected; use the VM APIs used in vm.rs (self.interrupt_frame, interrupt(), Frame::Bytecode, Return/exception dispatch) to drive the scenario and ensure the test fails on regressions, and run cargo test --lib locally before merging.
🤖 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_vm/src/vm.rs`:
- Around line 2358-2451: Add a Rust unit test in the same module as vm.rs (e.g.
fn test_unwind_past_interrupt_boundary) that constructs a VM state with at least
two frames, sets up a bytecode frame that will throw (so unwinding will pop a
frame past the current interrupt boundary), calls the VM interrupt() to set
self.interrupt_frame, triggers the exception/unwind path exercised in the shown
code, and then asserts that self.interrupt_frame has been cleared and that
subsequent Return instruction handling still behaves as expected; use the VM
APIs used in vm.rs (self.interrupt_frame, interrupt(), Frame::Bytecode,
Return/exception dispatch) to drive the scenario and ensure the test fails on
regressions, and run cargo test --lib locally before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: faf14f70-822c-4e6b-b678-2e5a82a6a1a3
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
baml_language/crates/baml_compiler2_emit/src/emit.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_vm/Cargo.tomlbaml_language/crates/bex_vm/src/lib.rsbaml_language/crates/bex_vm/src/package_baml/mod.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm/tests/load_type.rsbaml_language/crates/bex_vm/tests/method_class_type_args.rsbaml_language/crates/bex_vm_types/src/types.rsbaml_language/crates/tools_onionskin/src/compiler.rs
💤 Files with no reviewable changes (8)
- baml_language/crates/bex_vm/tests/load_type.rs
- baml_language/crates/bex_vm/Cargo.toml
- baml_language/crates/bex_vm/tests/method_class_type_args.rs
- baml_language/crates/bex_vm_types/src/types.rs
- baml_language/crates/bex_vm/src/package_baml/mod.rs
- baml_language/crates/baml_compiler2_emit/src/emit.rs
- baml_language/crates/tools_onionskin/src/compiler.rs
- baml_language/crates/baml_compiler2_emit/src/lib.rs
Binary size checks passed✅ 7 passed
Generated by |
Continues stripping observability bias from the VM core.
Custom events (kept log.*, dropped the event stream):
- Removed the baml.events.send intrinsic: IntrinsicOp::SendEvent (MIR
ir/lower), its emit lowering arm, and the stdlib ns_events/events.baml
declaration. log.debug/info/warn still work (they keep the SendEvent
instruction + VmExecState::Event path).
Block/loop/header "flagging" (VM + bytecode surface only):
- Removed Instruction/OpCode VizEnter/VizExit/NotifyBlock and all their
match arms; the runtime types BlockNotification/BlockNotificationType/
VizNodeMeta/VizNodeType/VizExecEvent/VizExecDelta; the Function
block_notifications/viz_nodes fields; the VM handlers; WatchNotification
Block/Viz variants; and the debug-disassembler arms.
- emit now no-ops the MIR StatementKind::{VizEnter,VizExit,NotifyBlock}.
The MIR compiler keeps its own viz/block IR dormant (not lowered to
bytecode). The variable-watch system (Watch/Notify/Variables) is intact.
cargo check --workspace --all-targets is green. The event_system /
ns_watch test fixtures and bytecode-disasm snapshots break by design and
will be regenerated in the tracing rebuild.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Merging this PR will improve performance by 23.89%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
…han CPython (#3627) Stacked on #3616 (base = `hellovai/trim-events`). Makes the `bex_vm` bytecode interpreter **faster than CPython 3.9** across most workloads, where it started ~2.2× slower. ## Results (Apple M2 Max, `scripts/speedtest run`) - **`fib` (1M × fib(50))**: total instructions retired **−60%**, CPU cycles **−51%** vs the pre-fusion baseline — below the cycle count of an optimal naive hand-written interpreter for the same loop. - **All ~38 speedtest workloads**: BAML now beats Python on ~22 of them (several by 2–4×: parallel-sum 0.23×, spawn-fan-out 0.41×, call-chain 0.45×, class-instances 0.50×, method-call 0.56×). Not overfit to fib — the wins span compute, classes, concurrency, dispatch, and most string ops. ## What changed (each commit validated + clippy-clean) All fusion is an **emit-time peephole** that rewrites instructions in place, confined to the current basic block (mirrors the existing `StoreVarLoadVar` fusion), so jump targets and block addresses are never affected: - **Superinstruction fusion** — fold operand loads (and the store, and the loop condition's branch) into single ops: - `AddIntVar/Const`, `SubIntVar/Const`, `CmpIntLtVar/Const` (fold right operand) - `AddIntVarVar/VarConst`, `SubIntVarVar/VarConst`, `CmpIntLtVarVar/VarConst` (fold both operands — no operand stack pushes) - `AddIntVarVarStore/VarConstStore` (compute + store directly), `MoveLocal` (fused `x = y`) - `CmpIntLt*BrFalse/BrTrue` (fuse the loop condition with its branch; **branch inversion** drops the per-iteration jump-to-body) - **Unchecked local-slot fast paths** for `LoadVar`/`StoreVar`, and a **lazy `faulting_pc`** (track `cur_pc` once per dispatch instead of writing the frame every op). - **Call path**: consolidated redundant per-call `get_object` heap derefs with a plain-`Function` fast path. - **kperf**: an in-process Apple-Silicon PMC probe (cycles + instructions retired, env-gated `BAML_KPERF=1`, behind an off-by-default `kperf` cargo feature) for deterministic, frequency-independent measurement. - **Toolchain**: pinned the nightly toolchain and dropped the MSRV job. ## Key insight Cycle cost is dominated by **branch-mispredictions on the giant opcode dispatch `match`** (an indirect jump). So the real lever is **reducing the number of *dispatched* ops** — that's why fusion keeps paying off (branch inversion: −15% cycles for −9% instructions), while removing the inline per-op counter barely moved cycles. Measure total instructions/cycles, not `instr/op` (fusion shrinks the op-count denominator). ## Validation fib/boundary-loops/if-else correct; `bex_vm`, `interfaces` (337), `exceptions`, `cancellation`, `host_value_callable`, `errors`, `floats`, `gc`, `dispatch`, `optimization` all green. ## Known follow-ups (diagnosed, not in this PR) - `baml pack`'s embedded runtime stub needs rebuilding for the new opcodes (`baml run` is unaffected). - Remaining Python losses: strings (`split` ~2.2× — alloc-bound), deep recursion (~130 cyc/call, frame setup), collatz (`%`/`/`/`*`/`!=`/`==` still unfused). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]> Co-authored-by: Sam Lijin <[email protected]> Co-authored-by: Dhilan Shah <[email protected]> Co-authored-by: aaronvg <[email protected]> Co-authored-by: 2kai2kai2 <[email protected]>
# Conflicts: # baml_language/Cargo.lock # baml_language/crates/bex_project/src/lib.rs # baml_language/crates/bridge_cffi/src/lib.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/baml_compiler2_tir/src/exhaustiveness.rs (1)
99-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude generic args in
Ctor::Classidentity.
PartialEq,Hash, andcoversstill collapseFoo<int>andFoo<string>into the same ctor because they only compare/hash theQualifiedTypeName. That breaksdedup_ctors,present_set, and row specialization for unions containing multiple instantiations of the same generic class, so one arm can incorrectly exhaust or shadow another. Keep the class args in ctor identity as well, preferably viaty_ctor_identity(...)so attrs/freshness stay ignored consistently.Suggested fix
impl PartialEq for Ctor { fn eq(&self, other: &Self) -> bool { use Ctor::{ Class, Interface, Missing, NonExhaustive, Or, Single, Slice, UnionMember, Wildcard, }; match (self, other) { (Single(a), Single(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), (Slice(a), Slice(b)) => a == b, - (Class(a, _), Class(b, _)) => a == b, + (Class(a, a_args), Class(b, b_args)) => { + a == b + && a_args.len() == b_args.len() + && a_args + .iter() + .zip(b_args) + .all(|(a, b)| ty_ctor_identity(a) == ty_ctor_identity(b)) + } (Interface(a), Interface(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), (UnionMember(a), UnionMember(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), (Or, Or) | (Wildcard, Wildcard) | (NonExhaustive, NonExhaustive) @@ std::mem::discriminant(self).hash(state); match self { Single(ty) => ty_ctor_identity(ty).hash(state), Slice(s) => s.hash(state), - Class(qtn, _) => { + Class(qtn, args) => { qtn.hash(state); + for arg in args { + ty_ctor_identity(arg).hash(state); + } } Interface(ty) => ty_ctor_identity(ty).hash(state), UnionMember(ty) => ty_ctor_identity(ty).hash(state), Or | Wildcard | NonExhaustive | Missing => {} } @@ use Ctor::{ Class, Interface, Missing, NonExhaustive, Or, Single, Slice, UnionMember, Wildcard, }; match (self, other) { (Wildcard, _) => true, (_, Wildcard) => false, (Single(a), Single(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), - (Class(a, _), Class(b, _)) => a == b, + (Class(a, a_args), Class(b, b_args)) => { + a == b + && a_args.len() == b_args.len() + && a_args + .iter() + .zip(b_args) + .all(|(a, b)| ty_ctor_identity(a) == ty_ctor_identity(b)) + } (Interface(a), Interface(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), (Slice(a), Slice(b)) => slice_covers(a, b), (UnionMember(a), UnionMember(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), (NonExhaustive, NonExhaustive) => true, (Missing, Missing) => true, @@ fn class_ty_for_ctor(qtn: &QualifiedTypeName, args: &[Ty], fallback: &Ty) -> Ty { match fallback { - Ty::Class(fallback_qtn, _, _) if fallback_qtn == qtn => fallback.clone(), + Ty::Class(fallback_qtn, fallback_args, _) + if fallback_qtn == qtn + && fallback_args.len() == args.len() + && fallback_args + .iter() + .zip(args) + .all(|(a, b)| ty_ctor_identity(a) == ty_ctor_identity(b)) => + { + fallback.clone() + } _ => Ty::Class(qtn.clone(), args.to_vec(), TyAttr::default()), } }Also applies to: 183-199
🤖 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_tir/src/exhaustiveness.rs` around lines 99 - 124, The Class ctor currently ignores generic args when computing identity (PartialEq, Hash, and covers), so Foo<int> and Foo<string> collapse; update all Class handling to include the type+args via ty_ctor_identity instead of only the QualifiedTypeName: in the PartialEq match arm that now reads (Class(a, _), Class(b, _)) => a == b, call ty_ctor_identity on the full class type constructed from the QualifiedTypeName and its args (use the same tuple fields, e.g. Class(qtn, args)) and compare those identities; likewise in std::hash::Hash for Ctor replace hashing qtn in the Class arm with hashing ty_ctor_identity(...) of the full class type; also make the same change in covers and anywhere else referenced (dedup_ctors / present_set logic—see the other Class handling around lines ~183-199) so class generic arguments participate in ctor identity consistently.
🧹 Nitpick comments (8)
.github/workflows/cargo-tests.reusable.yaml (2)
803-822: 💤 Low valueMissing blank line before
sccache-statsjob definition.There's no blank line between the end of
cargo-timingsjob (line 803) and the start ofsccache-statsjob (line 804), unlike other job definitions in this file. Minor formatting inconsistency.Suggested fix
delete-merged: true + sccache-stats: name: "sccache stats (merged)"🤖 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 @.github/workflows/cargo-tests.reusable.yaml around lines 803 - 822, Add a blank line before the sccache-stats job definition to match the file's job separation style: insert one empty line between the end of the previous job (cargo-timings) and the start of the job named "sccache-stats" so the job block beginning with "sccache-stats:" sits on its own paragraph.
83-89: ⚖️ Poor tradeoffConsider pinning third-party actions to commit SHAs for supply-chain security.
Static analysis (zizmor) flags multiple unpinned action references throughout this workflow (e.g.,
Swatinem/rust-cache@v2,actions/upload-artifact@v7,taiki-e/install-action@v2). Pinning to commit SHAs instead of tags prevents silent changes from compromised or force-pushed tags.This is a general hardening measure. If the repo's policy accepts tag-based versioning for trusted actions, this can be deferred.
🤖 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 @.github/workflows/cargo-tests.reusable.yaml around lines 83 - 89, Replace tag-based action references with their corresponding commit SHAs to harden supply-chain security: locate occurrences of Swatinem/rust-cache@v2, actions/upload-artifact@v7, taiki-e/install-action@v2 (and any other third-party actions in the workflow) and update each reference to the full commit SHA (e.g., Swatinem/rust-cache@<sha>) obtained from the action repository, then run the workflow locally or in a branch to verify the SHA pins work as expected.baml_language/crates/baml_compiler2_emit/src/emit.rs (1)
1137-1152: 💤 Low value
emit_store_varloses metadata for the first store when fusingStoreVar2.When fusing
StoreVar(a); StoreVar(slot)intoStoreVar2(a, slot), the metadata for slota(set by the previousemit_store_varcall at line 1150-1151) is overwritten byset_var_operand(n - 1, slot)at line 1146. The operand metadata will only reflect the second slot name, not the first.For debugging/disassembly, both slot names should ideally be preserved. Consider whether
StoreVar2needs a differentOperandMetavariant that holds both names, or if this is acceptable given the instruction semantics.🤖 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_emit/src/emit.rs` around lines 1137 - 1152, emit_store_var currently fuses a previous StoreVar(a) and a new StoreVar(slot) into Instruction::StoreVar2(a, slot) but then calls set_var_operand(n - 1, slot), overwriting the metadata for the first operand; preserve both operand metadata by extending the metadata model and update the fusion logic: add a new OperandMeta variant (or storage) that can hold two slot names for Instruction::StoreVar2, or modify set_var_operand to accept/attach metadata for both operands, and when you replace bytecode.instructions[n - 1] with Instruction::StoreVar2(a, slot) copy the original metadata for operand a into the first-slot metadata and set the second-slot metadata to slot so disassembly/debugging can see both names (symbols: emit_store_var, Instruction::StoreVar2, set_var_operand, bytecode.instructions).baml_language/crates/baml_cli/src/grep_command.rs (1)
447-469: ⚡ Quick winAdd a unit test that locks the method JSON schema.
This changes machine-readable CLI output and line-range serialization, so a focused unit test around
description_to_jsonwould catch silent drift cheaply. As per coding guidelines**/*.rs:Prefer writing Rust unit tests over integration tests where possible.Also applies to: 506-507
🤖 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_cli/src/grep_command.rs` around lines 447 - 469, Add a unit test that locks the JSON schema emitted by description_to_json/method_json: create a small deterministic ProjectDatabase fixture (or use an existing test helper), call description_to_json (which uses method_json) for a known file/class with predictable docstring/signature and line offsets, then assert the produced serde_json::Value includes the exact keys ("name","signature","docstring","file","line_start","line_end") and that "line_start" and "line_end" are the expected integers and that "file" matches the relative path; place the test as a Rust unit test in the same module as method_json so it runs fast and fails on any silent output/schema drift.baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
11133-11147: ⚡ Quick winAdd a regression test for the new union/interface dispatch path.
This helper-only test won't catch the new method/field dispatch branches or the generic-interface runtime checks. A Rust unit test that exercises a concrete generic-class union calling an inherited/default interface method would lock this behavior down. As per coding guidelines,
**/*.rs: Prefer writing Rust unit tests over integration tests where possible, and always runcargo test --libif you changed any Rust code.🤖 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_mir/src/lower.rs` around lines 11133 - 11147, Add a regression unit test exercising the new union/interface dispatch path by creating a concrete generic-class union that implements or inherits an interface with a default method and then invoking that method through the union to ensure generic-interface runtime checks and dispatch work; specifically, add a new #[test] (similar place as interface_tir_type_args_match_preserves_type_var_identity) that constructs a generic class/union instance using concrete type parameters, ensures the interface implementation/default method is available, calls the method via the union dispatch path, and asserts the correct behavior so that interface_tir_type_args_match_preserving_typevars and the new union/interface dispatch are both validated at cargo test --lib time.baml_language/crates/baml_tests/build.rs (1)
216-231: 💤 Low valueConsider guarding against empty slugs.
If a workload name consists entirely of non-alphanumeric characters (e.g.,
"!!!"), theslugifyfunction will produce a single underscore which is then trimmed away at line 230, leaving an empty string. This would causeformat_ident!at line 131 to panic at build time.While workload names are controlled repo content and this scenario is extremely unlikely, a defensive fallback (e.g.,
slug.is_empty()check with a default like"unnamed") would make the build script more robust.🛡️ Proposed defensive fallback
fn slugify(name: &str) -> String { let mut out = String::with_capacity(name.len()); let mut prev_underscore = false; for ch in name.chars() { if ch.is_ascii_alphanumeric() { out.extend(ch.to_lowercase()); prev_underscore = false; } else if !prev_underscore { out.push('_'); prev_underscore = true; } } - out.trim_matches('_').to_string() + let slug = out.trim_matches('_'); + if slug.is_empty() { + "unnamed".to_string() + } else { + slug.to_string() + } }🤖 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_tests/build.rs` around lines 216 - 231, The slugify function can return an empty string for names with no alphanumeric characters; after computing the slug in fn slugify(name: &str) -> String check if the result is empty and return a safe fallback (e.g. "unnamed" or similar) so callers like format_ident! don't panic; update slugify to ensure it never returns an empty string (use the same lowercase/underscore logic but substitute the fallback when slug.is_empty()).baml_language/crates/baml_lsp2_actions/src/describe.rs (1)
1675-1680: ⚡ Quick winLimit comment stripping to the covered CST subtree.
clean_body_source()scanstree.descendants_with_tokens()for every body extraction, so describing several symbols in one file turns into repeated full-file token walks. Each caller already passes a single item/member/function span, so starting from the covering node would keep this proportional to the rendered body instead of the whole file.🤖 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_lsp2_actions/src/describe.rs` around lines 1675 - 1680, clean_body_source() currently walks tree.descendants_with_tokens() for every extraction which causes full-file token scans; change it to locate the CST node that covers the provided span (use the parser/tree covering node API for the given span) and iterate only that node.descendants_with_tokens() (or the subtree root) when building deletions and stripping comments. Update the logic that builds deletions (currently iterating tree.descendants_with_tokens()) to start from the covering node for the span passed into clean_body_source(), keeping the rest of the comment-removal logic intact.baml_language/crates/baml_compiler2_ppir/src/lib.rs (1)
139-161: ⚡ Quick winDrop the custom
salsa::Updateimpl unless you need behavior beyond the default fallback.Salsa already falls back to a
'static + PartialEqcomparison when a tracked return type has noUpdateimpl, andProjectExpansionMapsis already fully owned +PartialEq. Keeping a hand-writtenunsafe implhere adds unsafe maintenance surface without changing the early-cutoff behavior. Please verify this still compiles cleanly after removing the impl. (docs.rs)Suggested simplification
-/// # Safety -/// -/// Mirrors [`baml_compiler2_hir::package::PackageItems`]'s impl. The contained -/// maps hold no Salsa-interned (`'db`) data, so storing them by value is sound; -/// `maybe_update` uses `PartialEq` for proper Salsa early-cutoff. -#[allow(unsafe_code)] -unsafe impl salsa::Update for ProjectExpansionMaps { - unsafe fn maybe_update(old_pointer: *mut Self, new_value: Self) -> bool { - // SAFETY: `old_pointer` is valid, aligned, and Salsa-owned. - #[allow(unsafe_code)] - let old = unsafe { &*old_pointer }; - if old == &new_value { - false - } else { - #[allow(unsafe_code)] - unsafe { - std::ptr::drop_in_place(old_pointer); - std::ptr::write(old_pointer, new_value); - } - true - } - } -}🤖 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_ppir/src/lib.rs` around lines 139 - 161, Remove the custom unsafe impl salsa::Update for ProjectExpansionMaps (including the maybe_update function) because ProjectExpansionMaps is already owned and PartialEq, so Salsa's default fallback handles early-cutoff; delete the entire unsafe impl block that implements salsa::Update and its maybe_update logic, then run a build to confirm it compiles cleanly and relies on the default Update behavior.
🤖 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/Cargo.toml`:
- Around line 169-171: Bump the crypto crate version constraints in Cargo.toml:
leave hex as-is, update the hmac dependency (symbol: hmac) from "0.12" to the
newer "0.13" and update the sha1 dependency (symbol: sha1) from "0.10" to "0.11"
so Cargo can pick the latest compatible minor releases; run cargo update and
cargo build/test to ensure compatibility and fix any API changes if they
surface.
In `@baml_language/crates/baml_cli/src/grep_command.rs`:
- Around line 447-469: The method_json serializer (and the similar serializer at
lines 506-507) drops the previous per-method JSON keys ("kind" and the single
"line") in favor of "line_start"/"line_end", which will break existing
consumers; restore backward compatibility by emitting the old keys alongside the
new ones (e.g., include "kind": m.kind and "line": line_number_at_offset(text,
m.item_range.start().into())) or alternatively gate the richer shape behind a
version flag, updating method_json and the analogous function to populate both
legacy and new fields.
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 6473-6483: The loop over union members currently drops each
member's concrete type arguments (patterning Tir2Ty::Class(qtn, _, _)), so
class_member_method_candidates and later interface-candidate logic lose the
concrete instantiated type; update the match to capture the member's type_args
(e.g., Tir2Ty::Class(qtn, type_args, _)) and thread those concrete args into the
interface-call candidate derivation (so class_member_method_candidates /
candidate construction uses the member's instantiated type), ensuring candidates
include members that arise only via inherited/default/generic interface views or
blanket `implements ... for C` rules; also ensure try_lower_union_iface_dispatch
uses these preserved concrete-member candidates when deciding dispatch.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 12647-12655: The helper pattern_overlaps_scrut_member currently
checks overlap using is_subtype against each member from
flatten_union_optional_members; replace those is_subtype calls with the
runtime-aware types_overlap so pattern dispatch matches runtime behavior (avoid
numeric-tower false positives). Specifically, in pattern_overlaps_scrut_member
change the closure that does self.is_subtype(pat, m) || self.is_subtype(m, pat)
to use self.types_overlap(pat, m) (or an equivalent symmetric types_overlap
check) so the function uses types_overlap for both directions when iterating
union/optional members.
- Around line 13375-13418: The interface destructure branch (matching
Ty::Interface and calling interface_field_infos_ordered) is running before
checking for missing generic args, so type_args can be empty and fields are
lowered with the interface's generic parameters instead of the scrutinee's
concrete args; fix by adding the same missing-type-args guard used for class
destructures or by substituting the scrutinee's concrete interface type
arguments into class_ty before calling interface_field_infos_ordered (i.e., when
handling Ty::Interface(iface_name, type_args, _) use the concrete args from
scrut_ty or bail/report missing args, then proceed to call
interface_field_infos_ordered, analyze_and_lower, build sub_dpats and
DPat::interface, and finally compute matched_ty with
intersect_pattern_flow_types).
- Around line 8099-8137: union_interface_method_ambiguity currently only
considers in-body implements via implemented_interface_method_sources (losing
registry-only rules and distinct instantiations like Getter<int> vs
Getter<string>), so update it to also consult the
package_implements_registry(rule set) when gathering "sources" for a given
class_qtn: for each rule in registry.interface_impl_rules where
rule.for_ty_pattern is the class_qtn and rule.interface_ty is one of the union
interfaces, check whether that rule provides the requested member (reusing
try_registry_member/resolve logic) and add the corresponding (interface QTN +
instantiation args) to the sources list; also stop collapsing members to bare
interface QTN — preserve the interface instantiation (e.g., keep
(QualifiedTypeName, Vec<Ty>) keys) so same-interface different-instantiation
cases are treated as distinct, and deduplicate sources before checking
sources.len() >= 2.
In `@baml_language/crates/baml_lsp2_actions/src/describe.rs`:
- Around line 1507-1523: The function collect_qtn_dep currently drops the
namespace by using qtn.name() and dedupes/looks up by short name, which causes
collisions (e.g., root.a.User vs root.b.User); change it to use the full
qualified path from qtn (e.g., qtn.to_string() or qtn.full_path()) for both
deduplication (the seen HashSet) and for resolving the dependency so you call
resolve_dep_from_outline with the qualified name instead of qtn.name(); update
references to the HashSet key and the resolve_dep_from_outline argument
accordingly in collect_qtn_dep so deps and seen are keyed by the full identity
and duplicates are correctly suppressed.
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_tir/src/exhaustiveness.rs`:
- Around line 99-124: The Class ctor currently ignores generic args when
computing identity (PartialEq, Hash, and covers), so Foo<int> and Foo<string>
collapse; update all Class handling to include the type+args via
ty_ctor_identity instead of only the QualifiedTypeName: in the PartialEq match
arm that now reads (Class(a, _), Class(b, _)) => a == b, call ty_ctor_identity
on the full class type constructed from the QualifiedTypeName and its args (use
the same tuple fields, e.g. Class(qtn, args)) and compare those identities;
likewise in std::hash::Hash for Ctor replace hashing qtn in the Class arm with
hashing ty_ctor_identity(...) of the full class type; also make the same change
in covers and anywhere else referenced (dedup_ctors / present_set logic—see the
other Class handling around lines ~183-199) so class generic arguments
participate in ctor identity consistently.
---
Nitpick comments:
In @.github/workflows/cargo-tests.reusable.yaml:
- Around line 803-822: Add a blank line before the sccache-stats job definition
to match the file's job separation style: insert one empty line between the end
of the previous job (cargo-timings) and the start of the job named
"sccache-stats" so the job block beginning with "sccache-stats:" sits on its own
paragraph.
- Around line 83-89: Replace tag-based action references with their
corresponding commit SHAs to harden supply-chain security: locate occurrences of
Swatinem/rust-cache@v2, actions/upload-artifact@v7, taiki-e/install-action@v2
(and any other third-party actions in the workflow) and update each reference to
the full commit SHA (e.g., Swatinem/rust-cache@<sha>) obtained from the action
repository, then run the workflow locally or in a branch to verify the SHA pins
work as expected.
In `@baml_language/crates/baml_cli/src/grep_command.rs`:
- Around line 447-469: Add a unit test that locks the JSON schema emitted by
description_to_json/method_json: create a small deterministic ProjectDatabase
fixture (or use an existing test helper), call description_to_json (which uses
method_json) for a known file/class with predictable docstring/signature and
line offsets, then assert the produced serde_json::Value includes the exact keys
("name","signature","docstring","file","line_start","line_end") and that
"line_start" and "line_end" are the expected integers and that "file" matches
the relative path; place the test as a Rust unit test in the same module as
method_json so it runs fast and fails on any silent output/schema drift.
In `@baml_language/crates/baml_compiler2_emit/src/emit.rs`:
- Around line 1137-1152: emit_store_var currently fuses a previous StoreVar(a)
and a new StoreVar(slot) into Instruction::StoreVar2(a, slot) but then calls
set_var_operand(n - 1, slot), overwriting the metadata for the first operand;
preserve both operand metadata by extending the metadata model and update the
fusion logic: add a new OperandMeta variant (or storage) that can hold two slot
names for Instruction::StoreVar2, or modify set_var_operand to accept/attach
metadata for both operands, and when you replace bytecode.instructions[n - 1]
with Instruction::StoreVar2(a, slot) copy the original metadata for operand a
into the first-slot metadata and set the second-slot metadata to slot so
disassembly/debugging can see both names (symbols: emit_store_var,
Instruction::StoreVar2, set_var_operand, bytecode.instructions).
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 11133-11147: Add a regression unit test exercising the new
union/interface dispatch path by creating a concrete generic-class union that
implements or inherits an interface with a default method and then invoking that
method through the union to ensure generic-interface runtime checks and dispatch
work; specifically, add a new #[test] (similar place as
interface_tir_type_args_match_preserves_type_var_identity) that constructs a
generic class/union instance using concrete type parameters, ensures the
interface implementation/default method is available, calls the method via the
union dispatch path, and asserts the correct behavior so that
interface_tir_type_args_match_preserving_typevars and the new union/interface
dispatch are both validated at cargo test --lib time.
In `@baml_language/crates/baml_compiler2_ppir/src/lib.rs`:
- Around line 139-161: Remove the custom unsafe impl salsa::Update for
ProjectExpansionMaps (including the maybe_update function) because
ProjectExpansionMaps is already owned and PartialEq, so Salsa's default fallback
handles early-cutoff; delete the entire unsafe impl block that implements
salsa::Update and its maybe_update logic, then run a build to confirm it
compiles cleanly and relies on the default Update behavior.
In `@baml_language/crates/baml_lsp2_actions/src/describe.rs`:
- Around line 1675-1680: clean_body_source() currently walks
tree.descendants_with_tokens() for every extraction which causes full-file token
scans; change it to locate the CST node that covers the provided span (use the
parser/tree covering node API for the given span) and iterate only that
node.descendants_with_tokens() (or the subtree root) when building deletions and
stripping comments. Update the logic that builds deletions (currently iterating
tree.descendants_with_tokens()) to start from the covering node for the span
passed into clean_body_source(), keeping the rest of the comment-removal logic
intact.
In `@baml_language/crates/baml_tests/build.rs`:
- Around line 216-231: The slugify function can return an empty string for names
with no alphanumeric characters; after computing the slug in fn slugify(name:
&str) -> String check if the result is empty and return a safe fallback (e.g.
"unnamed" or similar) so callers like format_ident! don't panic; update slugify
to ensure it never returns an empty string (use the same lowercase/underscore
logic but substitute the fallback when slug.is_empty()).
🪄 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: d0142bf5-233d-4074-acb2-a1a87b42c87d
⛔ Files ignored due to path filters (97)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__control_describe_shallow_namespace_item_works.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__describe_user_method_drill_in_shows_body.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__dispatch_root_prefix_resolves_user_item.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__fixed_describe_deep_namespace_item.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__fixed_describe_deep_namespace_member.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_alias_string.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_builtin_deep_copy.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_builtin_item_by_definition.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_builtin_string.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_class.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_class_with_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_class_with_static_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_enum.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_function_with_docstring.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_generic_class_with_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_member_field.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_ns_item.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_ns_member.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_truncation_hint.snapis excluded by!**/*.snapbaml_language/crates/baml_lsp2_actions/src/snapshots/baml_lsp2_actions__describe_tests__describe_builtin_deep_copy_with_compiler2_visible_files.snapis excluded by!**/*.snapbaml_language/crates/baml_lsp2_actions/src/snapshots/baml_lsp2_actions__describe_tests__describe_builtin_string_with_compiler2_visible_files.snapis excluded by!**/*.snapbaml_language/crates/baml_lsp2_actions/src/snapshots/baml_lsp2_actions__describe_tests__describe_class.snapis excluded by!**/*.snapbaml_language/crates/baml_lsp2_actions/src/snapshots/baml_lsp2_actions__describe_tests__describe_class_with_refs.snapis excluded by!**/*.snapbaml_language/crates/baml_lsp2_actions/src/snapshots/baml_lsp2_actions__grep_tests__grep_semantic_result_snapshot.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/assignments.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/bigints.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/closures.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/control_flow.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/deep_copy.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/exceptions.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/floats.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/for_loops.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/fs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/functions.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/glob.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/ints.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/is_operator.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/json_auto_derive.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/match_basics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/null_handling.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/reflect_type_of.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/spawn_semantics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/type_error_repro.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/type_reflection.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/typed_inputs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/watch.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/while_loops.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiler2_emit/baml_tests__compiler2_emit__optional_defaults_emit_snapshot.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__assert_std__/baml_tests__compiles____assert_std____06_codegen.snapis excluded by!**/*.snapbaml_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/snapshots/compiles/__testing_std__/baml_tests__compiles____testing_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closure_loop_variable/baml_tests__compiles__closure_loop_variable__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closures/baml_tests__compiles__closures__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/comment_in_type/baml_tests__compiles__comment_in_type__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__01_lexer__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__02_parser__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__10_formatter__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/function_call/baml_tests__compiles__function_call__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__06_codegen.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/literal_union_arithmetic/baml_tests__compiles__literal_union_arithmetic__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_expressions/baml_tests__compiles__parser_expressions__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_statements/baml_tests__compiles__parser_statements__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/patterns_new/baml_tests__compiles__patterns_new__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/match_exhaustiveness/baml_tests__diagnostic_errors__match_exhaustiveness__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/match_exhaustiveness/baml_tests__diagnostic_errors__match_exhaustiveness__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/patterns_new/baml_tests__diagnostic_errors__patterns_new__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/patterns_new/baml_tests__diagnostic_errors__patterns_new__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/simple_type_error/baml_tests__diagnostic_errors__simple_type_error__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/simple_type_error/baml_tests__diagnostic_errors__simple_type_error__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_name_type_error/baml_tests__diagnostic_errors__test_expr_name_type_error__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_name_type_error/baml_tests__diagnostic_errors__test_expr_name_type_error__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/engine/baml_tests__engine__tests__optional_dropping_adapter_preserves_source_defaults_bytecode.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/engine/baml_tests__engine__tests__source_call_omitted_default_executes_in_callee_scope_bytecode.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!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_textual.snapis excluded by!**/*.snapbaml_language/forks/aws-config-systest/Cargo.lockis excluded by!**/*.lockbaml_language/forks/google-cloud-auth-systest/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (127)
.envrc.github/actions/setup-mise/action.yml.github/workflows/cargo-tests.reusable.yaml.github/workflows/ci.yaml.github/workflows/size-gate-baseline-refresh.ymlbaml_language/.cargo/size-gate.tomlbaml_language/.ci/size-gate/aarch64-apple-darwin.tomlbaml_language/.ci/size-gate/wasm32-unknown-unknown.tomlbaml_language/.ci/size-gate/x86_64-pc-windows-msvc.tomlbaml_language/.ci/size-gate/x86_64-unknown-linux-gnu.tomlbaml_language/.gitignorebaml_language/Cargo.tomlbaml_language/crates/baml_cli/src/describe_command.rsbaml_language/crates/baml_cli/src/describe_command_tests.rsbaml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_cli/src/grep_command.rsbaml_language/crates/baml_compiler2_emit/src/emit.rsbaml_language/crates/baml_compiler2_hir/src/file_package.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/exhaustiveness.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/ty.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/describe.rsbaml_language/crates/baml_lsp2_actions/src/testing.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_lsp2_actions/src/type_info_tests.rsbaml_language/crates/baml_lsp2_actions/src/utils.rsbaml_language/crates/baml_lsp2_actions_tests/src/runner.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/catch/exhaustiveness_checks.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/hover/class.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/hover/out_of_body_implements_class.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/invalid_assignment/compound_ops.bamlbaml_language/crates/baml_tests/Cargo.tomlbaml_language/crates/baml_tests/baml_src/ns_floats/floats.bamlbaml_language/crates/baml_tests/benches/compiler_benchmark.rsbaml_language/crates/baml_tests/benches/runtime_benchmark.rsbaml_language/crates/baml_tests/build.rsbaml_language/crates/baml_tests/projects/compiles/event_system/main.bamlbaml_language/crates/baml_tests/tests/dispatch.rsbaml_language/crates/baml_tests/tests/interfaces.rsbaml_language/crates/baml_tests/tests/optimization.rsbaml_language/crates/baml_tests/tests/spawn_specialization.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_project/Cargo.tomlbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_vm/Cargo.tomlbaml_language/crates/bex_vm/src/debug.rsbaml_language/crates/bex_vm/src/indexable.rsbaml_language/crates/bex_vm/src/kperf.rsbaml_language/crates/bex_vm/src/lib.rsbaml_language/crates/bex_vm/src/package_baml/type_class.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm_types/src/bytecode.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/sys_llm/Cargo.tomlbaml_language/crates/sys_llm/src/auth_request/bedrock.rsbaml_language/crates/sys_llm/src/auth_request/vertex.rsbaml_language/crates/sys_llm/src/build_request/bedrock.rsbaml_language/crates/sys_llm/src/lib.rsbaml_language/crates/sys_llm/src/wasm.rsbaml_language/crates/tools_size_gate/Cargo.tomlbaml_language/crates/tools_size_gate/src/ceilings.rsbaml_language/crates/tools_size_gate/src/fetch.rsbaml_language/crates/tools_size_gate/src/main.rsbaml_language/crates/tools_size_gate/src/measure.rsbaml_language/crates/tools_size_gate/src/output.rsbaml_language/forks/aws-bedrock/Cargo.tomlbaml_language/forks/aws-bedrock/src/lib.rsbaml_language/forks/aws-config-systest/Cargo.tomlbaml_language/forks/aws-config-systest/README.mdbaml_language/forks/aws-config-systest/run-e2e.pybaml_language/forks/aws-config-systest/src/main.rsbaml_language/forks/aws-config/Cargo.tomlbaml_language/forks/aws-config/src/ini.rsbaml_language/forks/aws-config/src/lib.rsbaml_language/forks/aws-config/src/profile.rsbaml_language/forks/aws-config/src/providers.rsbaml_language/forks/aws-config/tests/chain_precedence.rsbaml_language/forks/aws-config/tests/common/mod.rsbaml_language/forks/aws-config/tests/common_smoke.rsbaml_language/forks/aws-config/tests/credential_process.rsbaml_language/forks/aws-config/tests/ecs_container.rsbaml_language/forks/aws-config/tests/env_credentials.rsbaml_language/forks/aws-config/tests/imds.rsbaml_language/forks/aws-config/tests/ini_parsing.rsbaml_language/forks/aws-config/tests/json_credentials.rsbaml_language/forks/aws-config/tests/profile_static.rsbaml_language/forks/aws-config/tests/region_resolution.rsbaml_language/forks/aws-config/tests/sso_provider.rsbaml_language/forks/aws-sigv4/Cargo.tomlbaml_language/forks/aws-sigv4/src/lib.rsbaml_language/forks/google-cloud-auth-systest/Cargo.tomlbaml_language/forks/google-cloud-auth-systest/README.mdbaml_language/forks/google-cloud-auth-systest/run-e2e.pybaml_language/forks/google-cloud-auth-systest/src/main.rsbaml_language/forks/google-cloud-auth/Cargo.tomlbaml_language/forks/google-cloud-auth/src/lib.rsbaml_language/forks/google-cloud-auth/tests/adc_precedence.rsbaml_language/forks/google-cloud-auth/tests/auth_flows.rsbaml_language/forks/google-cloud-auth/tests/common/mod.rsbaml_language/mise.tomlbaml_language/scripts/bench-comparebaml_language/sdk_tests/harness_setup/Cargo.tomlbaml_language/sdk_tests/harness_setup/src/lib.rsbaml_language/sdk_tests/harness_setup/src/python_pydantic2.rsbaml_language/sdks/python/rust/bridge_python/src/baml_core/baml_py/__init__.pyibaml_language/sdks/python/rust/bridge_python/src/runtime.rsbaml_language/sdks/python/rust/codegen_python/src/lib.rsbaml_language/sdks/python/src/baml_core/baml_py.pyibaml_language/stow.tomlbaml_language/tools/speedtest/export_baml.pybaml_language/tools/speedtest/workloads/compute/mixed-ops-5k.mdbaml_language/tools/speedtest/workloads/compute/wide-nested-class-create-50k.mdbaml_language/tools_sccache/Cargo.tomlbaml_language/tools_sccache/src/main.rsmise.tomltools/baml-bench/.env.exampletools/baml-bench/.gitignoretools/baml-bench/README.mdtools/baml-bench/convex/bamlBuilds.tstools/baml-bench/convex/crons.tstools/baml-bench/convex/issues.tstools/baml-bench/convex/lib.tstools/baml-bench/convex/maintenance.ts
💤 Files with no reviewable changes (1)
- baml_language/crates/baml_tests/projects/compiles/event_system/main.baml
✅ Files skipped from review due to trivial changes (8)
- baml_language/.gitignore
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/catch/exhaustiveness_checks.baml
- baml_language/.ci/size-gate/x86_64-pc-windows-msvc.toml
- baml_language/.ci/size-gate/aarch64-apple-darwin.toml
- baml_language/crates/baml_tests/Cargo.toml
- baml_language/crates/baml_lsp2_actions/src/check.rs
- baml_language/.ci/size-gate/wasm32-unknown-unknown.toml
- baml_language/.ci/size-gate/x86_64-unknown-linux-gnu.toml
hellovai
changed the title
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/baml_compiler2_tir/src/exhaustiveness.rs (1)
99-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude generic args in
Ctor::Classidentity.
PartialEq,Hash, andcoversstill collapseFoo<int>andFoo<string>into the same ctor because they only compare/hash theQualifiedTypeName. That breaksdedup_ctors,present_set, and row specialization for unions containing multiple instantiations of the same generic class, so one arm can incorrectly exhaust or shadow another. Keep the class args in ctor identity as well, preferably viaty_ctor_identity(...)so attrs/freshness stay ignored consistently.Suggested fix
impl PartialEq for Ctor { fn eq(&self, other: &Self) -> bool { use Ctor::{ Class, Interface, Missing, NonExhaustive, Or, Single, Slice, UnionMember, Wildcard, }; match (self, other) { (Single(a), Single(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), (Slice(a), Slice(b)) => a == b, - (Class(a, _), Class(b, _)) => a == b, + (Class(a, a_args), Class(b, b_args)) => { + a == b + && a_args.len() == b_args.len() + && a_args + .iter() + .zip(b_args) + .all(|(a, b)| ty_ctor_identity(a) == ty_ctor_identity(b)) + } (Interface(a), Interface(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), (UnionMember(a), UnionMember(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), (Or, Or) | (Wildcard, Wildcard) | (NonExhaustive, NonExhaustive) @@ std::mem::discriminant(self).hash(state); match self { Single(ty) => ty_ctor_identity(ty).hash(state), Slice(s) => s.hash(state), - Class(qtn, _) => { + Class(qtn, args) => { qtn.hash(state); + for arg in args { + ty_ctor_identity(arg).hash(state); + } } Interface(ty) => ty_ctor_identity(ty).hash(state), UnionMember(ty) => ty_ctor_identity(ty).hash(state), Or | Wildcard | NonExhaustive | Missing => {} } @@ use Ctor::{ Class, Interface, Missing, NonExhaustive, Or, Single, Slice, UnionMember, Wildcard, }; match (self, other) { (Wildcard, _) => true, (_, Wildcard) => false, (Single(a), Single(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), - (Class(a, _), Class(b, _)) => a == b, + (Class(a, a_args), Class(b, b_args)) => { + a == b + && a_args.len() == b_args.len() + && a_args + .iter() + .zip(b_args) + .all(|(a, b)| ty_ctor_identity(a) == ty_ctor_identity(b)) + } (Interface(a), Interface(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), (Slice(a), Slice(b)) => slice_covers(a, b), (UnionMember(a), UnionMember(b)) => ty_ctor_identity(a) == ty_ctor_identity(b), (NonExhaustive, NonExhaustive) => true, (Missing, Missing) => true, @@ fn class_ty_for_ctor(qtn: &QualifiedTypeName, args: &[Ty], fallback: &Ty) -> Ty { match fallback { - Ty::Class(fallback_qtn, _, _) if fallback_qtn == qtn => fallback.clone(), + Ty::Class(fallback_qtn, fallback_args, _) + if fallback_qtn == qtn + && fallback_args.len() == args.len() + && fallback_args + .iter() + .zip(args) + .all(|(a, b)| ty_ctor_identity(a) == ty_ctor_identity(b)) => + { + fallback.clone() + } _ => Ty::Class(qtn.clone(), args.to_vec(), TyAttr::default()), } }Also applies to: 183-199
🤖 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_tir/src/exhaustiveness.rs` around lines 99 - 124, The Class ctor currently ignores generic args when computing identity (PartialEq, Hash, and covers), so Foo<int> and Foo<string> collapse; update all Class handling to include the type+args via ty_ctor_identity instead of only the QualifiedTypeName: in the PartialEq match arm that now reads (Class(a, _), Class(b, _)) => a == b, call ty_ctor_identity on the full class type constructed from the QualifiedTypeName and its args (use the same tuple fields, e.g. Class(qtn, args)) and compare those identities; likewise in std::hash::Hash for Ctor replace hashing qtn in the Class arm with hashing ty_ctor_identity(...) of the full class type; also make the same change in covers and anywhere else referenced (dedup_ctors / present_set logic—see the other Class handling around lines ~183-199) so class generic arguments participate in ctor identity consistently.
🧹 Nitpick comments (8)
.github/workflows/cargo-tests.reusable.yaml (2)
803-822: 💤 Low valueMissing blank line before
sccache-statsjob definition.There's no blank line between the end of
cargo-timingsjob (line 803) and the start ofsccache-statsjob (line 804), unlike other job definitions in this file. Minor formatting inconsistency.Suggested fix
delete-merged: true + sccache-stats: name: "sccache stats (merged)"🤖 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 @.github/workflows/cargo-tests.reusable.yaml around lines 803 - 822, Add a blank line before the sccache-stats job definition to match the file's job separation style: insert one empty line between the end of the previous job (cargo-timings) and the start of the job named "sccache-stats" so the job block beginning with "sccache-stats:" sits on its own paragraph.
83-89: ⚖️ Poor tradeoffConsider pinning third-party actions to commit SHAs for supply-chain security.
Static analysis (zizmor) flags multiple unpinned action references throughout this workflow (e.g.,
Swatinem/rust-cache@v2,actions/upload-artifact@v7,taiki-e/install-action@v2). Pinning to commit SHAs instead of tags prevents silent changes from compromised or force-pushed tags.This is a general hardening measure. If the repo's policy accepts tag-based versioning for trusted actions, this can be deferred.
🤖 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 @.github/workflows/cargo-tests.reusable.yaml around lines 83 - 89, Replace tag-based action references with their corresponding commit SHAs to harden supply-chain security: locate occurrences of Swatinem/rust-cache@v2, actions/upload-artifact@v7, taiki-e/install-action@v2 (and any other third-party actions in the workflow) and update each reference to the full commit SHA (e.g., Swatinem/rust-cache@<sha>) obtained from the action repository, then run the workflow locally or in a branch to verify the SHA pins work as expected.baml_language/crates/baml_compiler2_emit/src/emit.rs (1)
1137-1152: 💤 Low value
emit_store_varloses metadata for the first store when fusingStoreVar2.When fusing
StoreVar(a); StoreVar(slot)intoStoreVar2(a, slot), the metadata for slota(set by the previousemit_store_varcall at line 1150-1151) is overwritten byset_var_operand(n - 1, slot)at line 1146. The operand metadata will only reflect the second slot name, not the first.For debugging/disassembly, both slot names should ideally be preserved. Consider whether
StoreVar2needs a differentOperandMetavariant that holds both names, or if this is acceptable given the instruction semantics.🤖 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_emit/src/emit.rs` around lines 1137 - 1152, emit_store_var currently fuses a previous StoreVar(a) and a new StoreVar(slot) into Instruction::StoreVar2(a, slot) but then calls set_var_operand(n - 1, slot), overwriting the metadata for the first operand; preserve both operand metadata by extending the metadata model and update the fusion logic: add a new OperandMeta variant (or storage) that can hold two slot names for Instruction::StoreVar2, or modify set_var_operand to accept/attach metadata for both operands, and when you replace bytecode.instructions[n - 1] with Instruction::StoreVar2(a, slot) copy the original metadata for operand a into the first-slot metadata and set the second-slot metadata to slot so disassembly/debugging can see both names (symbols: emit_store_var, Instruction::StoreVar2, set_var_operand, bytecode.instructions).baml_language/crates/baml_cli/src/grep_command.rs (1)
447-469: ⚡ Quick winAdd a unit test that locks the method JSON schema.
This changes machine-readable CLI output and line-range serialization, so a focused unit test around
description_to_jsonwould catch silent drift cheaply. As per coding guidelines**/*.rs:Prefer writing Rust unit tests over integration tests where possible.Also applies to: 506-507
🤖 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_cli/src/grep_command.rs` around lines 447 - 469, Add a unit test that locks the JSON schema emitted by description_to_json/method_json: create a small deterministic ProjectDatabase fixture (or use an existing test helper), call description_to_json (which uses method_json) for a known file/class with predictable docstring/signature and line offsets, then assert the produced serde_json::Value includes the exact keys ("name","signature","docstring","file","line_start","line_end") and that "line_start" and "line_end" are the expected integers and that "file" matches the relative path; place the test as a Rust unit test in the same module as method_json so it runs fast and fails on any silent output/schema drift.baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
11133-11147: ⚡ Quick winAdd a regression test for the new union/interface dispatch path.
This helper-only test won't catch the new method/field dispatch branches or the generic-interface runtime checks. A Rust unit test that exercises a concrete generic-class union calling an inherited/default interface method would lock this behavior down. As per coding guidelines,
**/*.rs: Prefer writing Rust unit tests over integration tests where possible, and always runcargo test --libif you changed any Rust code.🤖 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_mir/src/lower.rs` around lines 11133 - 11147, Add a regression unit test exercising the new union/interface dispatch path by creating a concrete generic-class union that implements or inherits an interface with a default method and then invoking that method through the union to ensure generic-interface runtime checks and dispatch work; specifically, add a new #[test] (similar place as interface_tir_type_args_match_preserves_type_var_identity) that constructs a generic class/union instance using concrete type parameters, ensures the interface implementation/default method is available, calls the method via the union dispatch path, and asserts the correct behavior so that interface_tir_type_args_match_preserving_typevars and the new union/interface dispatch are both validated at cargo test --lib time.baml_language/crates/baml_tests/build.rs (1)
216-231: 💤 Low valueConsider guarding against empty slugs.
If a workload name consists entirely of non-alphanumeric characters (e.g.,
"!!!"), theslugifyfunction will produce a single underscore which is then trimmed away at line 230, leaving an empty string. This would causeformat_ident!at line 131 to panic at build time.While workload names are controlled repo content and this scenario is extremely unlikely, a defensive fallback (e.g.,
slug.is_empty()check with a default like"unnamed") would make the build script more robust.🛡️ Proposed defensive fallback
fn slugify(name: &str) -> String { let mut out = String::with_capacity(name.len()); let mut prev_underscore = false; for ch in name.chars() { if ch.is_ascii_alphanumeric() { out.extend(ch.to_lowercase()); prev_underscore = false; } else if !prev_underscore { out.push('_'); prev_underscore = true; } } - out.trim_matches('_').to_string() + let slug = out.trim_matches('_'); + if slug.is_empty() { + "unnamed".to_string() + } else { + slug.to_string() + } }🤖 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_tests/build.rs` around lines 216 - 231, The slugify function can return an empty string for names with no alphanumeric characters; after computing the slug in fn slugify(name: &str) -> String check if the result is empty and return a safe fallback (e.g. "unnamed" or similar) so callers like format_ident! don't panic; update slugify to ensure it never returns an empty string (use the same lowercase/underscore logic but substitute the fallback when slug.is_empty()).baml_language/crates/baml_lsp2_actions/src/describe.rs (1)
1675-1680: ⚡ Quick winLimit comment stripping to the covered CST subtree.
clean_body_source()scanstree.descendants_with_tokens()for every body extraction, so describing several symbols in one file turns into repeated full-file token walks. Each caller already passes a single item/member/function span, so starting from the covering node would keep this proportional to the rendered body instead of the whole file.🤖 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_lsp2_actions/src/describe.rs` around lines 1675 - 1680, clean_body_source() currently walks tree.descendants_with_tokens() for every extraction which causes full-file token scans; change it to locate the CST node that covers the provided span (use the parser/tree covering node API for the given span) and iterate only that node.descendants_with_tokens() (or the subtree root) when building deletions and stripping comments. Update the logic that builds deletions (currently iterating tree.descendants_with_tokens()) to start from the covering node for the span passed into clean_body_source(), keeping the rest of the comment-removal logic intact.baml_language/crates/baml_compiler2_ppir/src/lib.rs (1)
139-161: ⚡ Quick winDrop the custom
salsa::Updateimpl unless you need behavior beyond the default fallback.Salsa already falls back to a
'static + PartialEqcomparison when a tracked return type has noUpdateimpl, andProjectExpansionMapsis already fully owned +PartialEq. Keeping a hand-writtenunsafe implhere adds unsafe maintenance surface without changing the early-cutoff behavior. Please verify this still compiles cleanly after removing the impl. (docs.rs)Suggested simplification
-/// # Safety -/// -/// Mirrors [`baml_compiler2_hir::package::PackageItems`]'s impl. The contained -/// maps hold no Salsa-interned (`'db`) data, so storing them by value is sound; -/// `maybe_update` uses `PartialEq` for proper Salsa early-cutoff. -#[allow(unsafe_code)] -unsafe impl salsa::Update for ProjectExpansionMaps { - unsafe fn maybe_update(old_pointer: *mut Self, new_value: Self) -> bool { - // SAFETY: `old_pointer` is valid, aligned, and Salsa-owned. - #[allow(unsafe_code)] - let old = unsafe { &*old_pointer }; - if old == &new_value { - false - } else { - #[allow(unsafe_code)] - unsafe { - std::ptr::drop_in_place(old_pointer); - std::ptr::write(old_pointer, new_value); - } - true - } - } -}🤖 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_ppir/src/lib.rs` around lines 139 - 161, Remove the custom unsafe impl salsa::Update for ProjectExpansionMaps (including the maybe_update function) because ProjectExpansionMaps is already owned and PartialEq, so Salsa's default fallback handles early-cutoff; delete the entire unsafe impl block that implements salsa::Update and its maybe_update logic, then run a build to confirm it compiles cleanly and relies on the default Update behavior.
🤖 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/Cargo.toml`:
- Around line 169-171: Bump the crypto crate version constraints in Cargo.toml:
leave hex as-is, update the hmac dependency (symbol: hmac) from "0.12" to the
newer "0.13" and update the sha1 dependency (symbol: sha1) from "0.10" to "0.11"
so Cargo can pick the latest compatible minor releases; run cargo update and
cargo build/test to ensure compatibility and fix any API changes if they
surface.
In `@baml_language/crates/baml_cli/src/grep_command.rs`:
- Around line 447-469: The method_json serializer (and the similar serializer at
lines 506-507) drops the previous per-method JSON keys ("kind" and the single
"line") in favor of "line_start"/"line_end", which will break existing
consumers; restore backward compatibility by emitting the old keys alongside the
new ones (e.g., include "kind": m.kind and "line": line_number_at_offset(text,
m.item_range.start().into())) or alternatively gate the richer shape behind a
version flag, updating method_json and the analogous function to populate both
legacy and new fields.
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 6473-6483: The loop over union members currently drops each
member's concrete type arguments (patterning Tir2Ty::Class(qtn, _, _)), so
class_member_method_candidates and later interface-candidate logic lose the
concrete instantiated type; update the match to capture the member's type_args
(e.g., Tir2Ty::Class(qtn, type_args, _)) and thread those concrete args into the
interface-call candidate derivation (so class_member_method_candidates /
candidate construction uses the member's instantiated type), ensuring candidates
include members that arise only via inherited/default/generic interface views or
blanket `implements ... for C` rules; also ensure try_lower_union_iface_dispatch
uses these preserved concrete-member candidates when deciding dispatch.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 12647-12655: The helper pattern_overlaps_scrut_member currently
checks overlap using is_subtype against each member from
flatten_union_optional_members; replace those is_subtype calls with the
runtime-aware types_overlap so pattern dispatch matches runtime behavior (avoid
numeric-tower false positives). Specifically, in pattern_overlaps_scrut_member
change the closure that does self.is_subtype(pat, m) || self.is_subtype(m, pat)
to use self.types_overlap(pat, m) (or an equivalent symmetric types_overlap
check) so the function uses types_overlap for both directions when iterating
union/optional members.
- Around line 13375-13418: The interface destructure branch (matching
Ty::Interface and calling interface_field_infos_ordered) is running before
checking for missing generic args, so type_args can be empty and fields are
lowered with the interface's generic parameters instead of the scrutinee's
concrete args; fix by adding the same missing-type-args guard used for class
destructures or by substituting the scrutinee's concrete interface type
arguments into class_ty before calling interface_field_infos_ordered (i.e., when
handling Ty::Interface(iface_name, type_args, _) use the concrete args from
scrut_ty or bail/report missing args, then proceed to call
interface_field_infos_ordered, analyze_and_lower, build sub_dpats and
DPat::interface, and finally compute matched_ty with
intersect_pattern_flow_types).
- Around line 8099-8137: union_interface_method_ambiguity currently only
considers in-body implements via implemented_interface_method_sources (losing
registry-only rules and distinct instantiations like Getter<int> vs
Getter<string>), so update it to also consult the
package_implements_registry(rule set) when gathering "sources" for a given
class_qtn: for each rule in registry.interface_impl_rules where
rule.for_ty_pattern is the class_qtn and rule.interface_ty is one of the union
interfaces, check whether that rule provides the requested member (reusing
try_registry_member/resolve logic) and add the corresponding (interface QTN +
instantiation args) to the sources list; also stop collapsing members to bare
interface QTN — preserve the interface instantiation (e.g., keep
(QualifiedTypeName, Vec<Ty>) keys) so same-interface different-instantiation
cases are treated as distinct, and deduplicate sources before checking
sources.len() >= 2.
In `@baml_language/crates/baml_lsp2_actions/src/describe.rs`:
- Around line 1507-1523: The function collect_qtn_dep currently drops the
namespace by using qtn.name() and dedupes/looks up by short name, which causes
collisions (e.g., root.a.User vs root.b.User); change it to use the full
qualified path from qtn (e.g., qtn.to_string() or qtn.full_path()) for both
deduplication (the seen HashSet) and for resolving the dependency so you call
resolve_dep_from_outline with the qualified name instead of qtn.name(); update
references to the HashSet key and the resolve_dep_from_outline argument
accordingly in collect_qtn_dep so deps and seen are keyed by the full identity
and duplicates are correctly suppressed.
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_tir/src/exhaustiveness.rs`:
- Around line 99-124: The Class ctor currently ignores generic args when
computing identity (PartialEq, Hash, and covers), so Foo<int> and Foo<string>
collapse; update all Class handling to include the type+args via
ty_ctor_identity instead of only the QualifiedTypeName: in the PartialEq match
arm that now reads (Class(a, _), Class(b, _)) => a == b, call ty_ctor_identity
on the full class type constructed from the QualifiedTypeName and its args (use
the same tuple fields, e.g. Class(qtn, args)) and compare those identities;
likewise in std::hash::Hash for Ctor replace hashing qtn in the Class arm with
hashing ty_ctor_identity(...) of the full class type; also make the same change
in covers and anywhere else referenced (dedup_ctors / present_set logic—see the
other Class handling around lines ~183-199) so class generic arguments
participate in ctor identity consistently.
---
Nitpick comments:
In @.github/workflows/cargo-tests.reusable.yaml:
- Around line 803-822: Add a blank line before the sccache-stats job definition
to match the file's job separation style: insert one empty line between the end
of the previous job (cargo-timings) and the start of the job named
"sccache-stats" so the job block beginning with "sccache-stats:" sits on its own
paragraph.
- Around line 83-89: Replace tag-based action references with their
corresponding commit SHAs to harden supply-chain security: locate occurrences of
Swatinem/rust-cache@v2, actions/upload-artifact@v7, taiki-e/install-action@v2
(and any other third-party actions in the workflow) and update each reference to
the full commit SHA (e.g., Swatinem/rust-cache@<sha>) obtained from the action
repository, then run the workflow locally or in a branch to verify the SHA pins
work as expected.
In `@baml_language/crates/baml_cli/src/grep_command.rs`:
- Around line 447-469: Add a unit test that locks the JSON schema emitted by
description_to_json/method_json: create a small deterministic ProjectDatabase
fixture (or use an existing test helper), call description_to_json (which uses
method_json) for a known file/class with predictable docstring/signature and
line offsets, then assert the produced serde_json::Value includes the exact keys
("name","signature","docstring","file","line_start","line_end") and that
"line_start" and "line_end" are the expected integers and that "file" matches
the relative path; place the test as a Rust unit test in the same module as
method_json so it runs fast and fails on any silent output/schema drift.
In `@baml_language/crates/baml_compiler2_emit/src/emit.rs`:
- Around line 1137-1152: emit_store_var currently fuses a previous StoreVar(a)
and a new StoreVar(slot) into Instruction::StoreVar2(a, slot) but then calls
set_var_operand(n - 1, slot), overwriting the metadata for the first operand;
preserve both operand metadata by extending the metadata model and update the
fusion logic: add a new OperandMeta variant (or storage) that can hold two slot
names for Instruction::StoreVar2, or modify set_var_operand to accept/attach
metadata for both operands, and when you replace bytecode.instructions[n - 1]
with Instruction::StoreVar2(a, slot) copy the original metadata for operand a
into the first-slot metadata and set the second-slot metadata to slot so
disassembly/debugging can see both names (symbols: emit_store_var,
Instruction::StoreVar2, set_var_operand, bytecode.instructions).
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 11133-11147: Add a regression unit test exercising the new
union/interface dispatch path by creating a concrete generic-class union that
implements or inherits an interface with a default method and then invoking that
method through the union to ensure generic-interface runtime checks and dispatch
work; specifically, add a new #[test] (similar place as
interface_tir_type_args_match_preserves_type_var_identity) that constructs a
generic class/union instance using concrete type parameters, ensures the
interface implementation/default method is available, calls the method via the
union dispatch path, and asserts the correct behavior so that
interface_tir_type_args_match_preserving_typevars and the new union/interface
dispatch are both validated at cargo test --lib time.
In `@baml_language/crates/baml_compiler2_ppir/src/lib.rs`:
- Around line 139-161: Remove the custom unsafe impl salsa::Update for
ProjectExpansionMaps (including the maybe_update function) because
ProjectExpansionMaps is already owned and PartialEq, so Salsa's default fallback
handles early-cutoff; delete the entire unsafe impl block that implements
salsa::Update and its maybe_update logic, then run a build to confirm it
compiles cleanly and relies on the default Update behavior.
In `@baml_language/crates/baml_lsp2_actions/src/describe.rs`:
- Around line 1675-1680: clean_body_source() currently walks
tree.descendants_with_tokens() for every extraction which causes full-file token
scans; change it to locate the CST node that covers the provided span (use the
parser/tree covering node API for the given span) and iterate only that
node.descendants_with_tokens() (or the subtree root) when building deletions and
stripping comments. Update the logic that builds deletions (currently iterating
tree.descendants_with_tokens()) to start from the covering node for the span
passed into clean_body_source(), keeping the rest of the comment-removal logic
intact.
In `@baml_language/crates/baml_tests/build.rs`:
- Around line 216-231: The slugify function can return an empty string for names
with no alphanumeric characters; after computing the slug in fn slugify(name:
&str) -> String check if the result is empty and return a safe fallback (e.g.
"unnamed" or similar) so callers like format_ident! don't panic; update slugify
to ensure it never returns an empty string (use the same lowercase/underscore
logic but substitute the fallback when slug.is_empty()).
🪄 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: d0142bf5-233d-4074-acb2-a1a87b42c87d
⛔ Files ignored due to path filters (97)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__control_describe_shallow_namespace_item_works.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__describe_user_method_drill_in_shows_body.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__dispatch_root_prefix_resolves_user_item.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__fixed_describe_deep_namespace_item.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__fixed_describe_deep_namespace_member.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_alias_string.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_builtin_deep_copy.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_builtin_item_by_definition.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_builtin_string.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_class.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_class_with_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_class_with_static_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_enum.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_function_with_docstring.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_generic_class_with_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_member_field.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_ns_item.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_ns_member.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_describe_truncation_hint.snapis excluded by!**/*.snapbaml_language/crates/baml_lsp2_actions/src/snapshots/baml_lsp2_actions__describe_tests__describe_builtin_deep_copy_with_compiler2_visible_files.snapis excluded by!**/*.snapbaml_language/crates/baml_lsp2_actions/src/snapshots/baml_lsp2_actions__describe_tests__describe_builtin_string_with_compiler2_visible_files.snapis excluded by!**/*.snapbaml_language/crates/baml_lsp2_actions/src/snapshots/baml_lsp2_actions__describe_tests__describe_class.snapis excluded by!**/*.snapbaml_language/crates/baml_lsp2_actions/src/snapshots/baml_lsp2_actions__describe_tests__describe_class_with_refs.snapis excluded by!**/*.snapbaml_language/crates/baml_lsp2_actions/src/snapshots/baml_lsp2_actions__grep_tests__grep_semantic_result_snapshot.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/assignments.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/bigints.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/closures.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/control_flow.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/deep_copy.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/exceptions.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/floats.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/for_loops.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/fs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/functions.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/glob.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/ints.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/is_operator.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/json_auto_derive.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/match_basics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/null_handling.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/reflect_type_of.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/spawn_semantics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/type_error_repro.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/type_reflection.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/typed_inputs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/watch.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/while_loops.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiler2_emit/baml_tests__compiler2_emit__optional_defaults_emit_snapshot.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__assert_std__/baml_tests__compiles____assert_std____06_codegen.snapis excluded by!**/*.snapbaml_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/snapshots/compiles/__testing_std__/baml_tests__compiles____testing_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closure_loop_variable/baml_tests__compiles__closure_loop_variable__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closures/baml_tests__compiles__closures__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/comment_in_type/baml_tests__compiles__comment_in_type__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__01_lexer__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__02_parser__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__10_formatter__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/function_call/baml_tests__compiles__function_call__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__06_codegen.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/literal_union_arithmetic/baml_tests__compiles__literal_union_arithmetic__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_expressions/baml_tests__compiles__parser_expressions__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_statements/baml_tests__compiles__parser_statements__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/patterns_new/baml_tests__compiles__patterns_new__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/match_exhaustiveness/baml_tests__diagnostic_errors__match_exhaustiveness__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/match_exhaustiveness/baml_tests__diagnostic_errors__match_exhaustiveness__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/patterns_new/baml_tests__diagnostic_errors__patterns_new__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/patterns_new/baml_tests__diagnostic_errors__patterns_new__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/simple_type_error/baml_tests__diagnostic_errors__simple_type_error__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/simple_type_error/baml_tests__diagnostic_errors__simple_type_error__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_name_type_error/baml_tests__diagnostic_errors__test_expr_name_type_error__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_name_type_error/baml_tests__diagnostic_errors__test_expr_name_type_error__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/engine/baml_tests__engine__tests__optional_dropping_adapter_preserves_source_defaults_bytecode.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/engine/baml_tests__engine__tests__source_call_omitted_default_executes_in_callee_scope_bytecode.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!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_textual.snapis excluded by!**/*.snapbaml_language/forks/aws-config-systest/Cargo.lockis excluded by!**/*.lockbaml_language/forks/google-cloud-auth-systest/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (127)
.envrc.github/actions/setup-mise/action.yml.github/workflows/cargo-tests.reusable.yaml.github/workflows/ci.yaml.github/workflows/size-gate-baseline-refresh.ymlbaml_language/.cargo/size-gate.tomlbaml_language/.ci/size-gate/aarch64-apple-darwin.tomlbaml_language/.ci/size-gate/wasm32-unknown-unknown.tomlbaml_language/.ci/size-gate/x86_64-pc-windows-msvc.tomlbaml_language/.ci/size-gate/x86_64-unknown-linux-gnu.tomlbaml_language/.gitignorebaml_language/Cargo.tomlbaml_language/crates/baml_cli/src/describe_command.rsbaml_language/crates/baml_cli/src/describe_command_tests.rsbaml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_cli/src/grep_command.rsbaml_language/crates/baml_compiler2_emit/src/emit.rsbaml_language/crates/baml_compiler2_hir/src/file_package.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/exhaustiveness.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/ty.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/describe.rsbaml_language/crates/baml_lsp2_actions/src/testing.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_lsp2_actions/src/type_info_tests.rsbaml_language/crates/baml_lsp2_actions/src/utils.rsbaml_language/crates/baml_lsp2_actions_tests/src/runner.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/catch/exhaustiveness_checks.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/hover/class.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/hover/out_of_body_implements_class.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/invalid_assignment/compound_ops.bamlbaml_language/crates/baml_tests/Cargo.tomlbaml_language/crates/baml_tests/baml_src/ns_floats/floats.bamlbaml_language/crates/baml_tests/benches/compiler_benchmark.rsbaml_language/crates/baml_tests/benches/runtime_benchmark.rsbaml_language/crates/baml_tests/build.rsbaml_language/crates/baml_tests/projects/compiles/event_system/main.bamlbaml_language/crates/baml_tests/tests/dispatch.rsbaml_language/crates/baml_tests/tests/interfaces.rsbaml_language/crates/baml_tests/tests/optimization.rsbaml_language/crates/baml_tests/tests/spawn_specialization.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_project/Cargo.tomlbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_vm/Cargo.tomlbaml_language/crates/bex_vm/src/debug.rsbaml_language/crates/bex_vm/src/indexable.rsbaml_language/crates/bex_vm/src/kperf.rsbaml_language/crates/bex_vm/src/lib.rsbaml_language/crates/bex_vm/src/package_baml/type_class.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm_types/src/bytecode.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/sys_llm/Cargo.tomlbaml_language/crates/sys_llm/src/auth_request/bedrock.rsbaml_language/crates/sys_llm/src/auth_request/vertex.rsbaml_language/crates/sys_llm/src/build_request/bedrock.rsbaml_language/crates/sys_llm/src/lib.rsbaml_language/crates/sys_llm/src/wasm.rsbaml_language/crates/tools_size_gate/Cargo.tomlbaml_language/crates/tools_size_gate/src/ceilings.rsbaml_language/crates/tools_size_gate/src/fetch.rsbaml_language/crates/tools_size_gate/src/main.rsbaml_language/crates/tools_size_gate/src/measure.rsbaml_language/crates/tools_size_gate/src/output.rsbaml_language/forks/aws-bedrock/Cargo.tomlbaml_language/forks/aws-bedrock/src/lib.rsbaml_language/forks/aws-config-systest/Cargo.tomlbaml_language/forks/aws-config-systest/README.mdbaml_language/forks/aws-config-systest/run-e2e.pybaml_language/forks/aws-config-systest/src/main.rsbaml_language/forks/aws-config/Cargo.tomlbaml_language/forks/aws-config/src/ini.rsbaml_language/forks/aws-config/src/lib.rsbaml_language/forks/aws-config/src/profile.rsbaml_language/forks/aws-config/src/providers.rsbaml_language/forks/aws-config/tests/chain_precedence.rsbaml_language/forks/aws-config/tests/common/mod.rsbaml_language/forks/aws-config/tests/common_smoke.rsbaml_language/forks/aws-config/tests/credential_process.rsbaml_language/forks/aws-config/tests/ecs_container.rsbaml_language/forks/aws-config/tests/env_credentials.rsbaml_language/forks/aws-config/tests/imds.rsbaml_language/forks/aws-config/tests/ini_parsing.rsbaml_language/forks/aws-config/tests/json_credentials.rsbaml_language/forks/aws-config/tests/profile_static.rsbaml_language/forks/aws-config/tests/region_resolution.rsbaml_language/forks/aws-config/tests/sso_provider.rsbaml_language/forks/aws-sigv4/Cargo.tomlbaml_language/forks/aws-sigv4/src/lib.rsbaml_language/forks/google-cloud-auth-systest/Cargo.tomlbaml_language/forks/google-cloud-auth-systest/README.mdbaml_language/forks/google-cloud-auth-systest/run-e2e.pybaml_language/forks/google-cloud-auth-systest/src/main.rsbaml_language/forks/google-cloud-auth/Cargo.tomlbaml_language/forks/google-cloud-auth/src/lib.rsbaml_language/forks/google-cloud-auth/tests/adc_precedence.rsbaml_language/forks/google-cloud-auth/tests/auth_flows.rsbaml_language/forks/google-cloud-auth/tests/common/mod.rsbaml_language/mise.tomlbaml_language/scripts/bench-comparebaml_language/sdk_tests/harness_setup/Cargo.tomlbaml_language/sdk_tests/harness_setup/src/lib.rsbaml_language/sdk_tests/harness_setup/src/python_pydantic2.rsbaml_language/sdks/python/rust/bridge_python/src/baml_core/baml_py/__init__.pyibaml_language/sdks/python/rust/bridge_python/src/runtime.rsbaml_language/sdks/python/rust/codegen_python/src/lib.rsbaml_language/sdks/python/src/baml_core/baml_py.pyibaml_language/stow.tomlbaml_language/tools/speedtest/export_baml.pybaml_language/tools/speedtest/workloads/compute/mixed-ops-5k.mdbaml_language/tools/speedtest/workloads/compute/wide-nested-class-create-50k.mdbaml_language/tools_sccache/Cargo.tomlbaml_language/tools_sccache/src/main.rsmise.tomltools/baml-bench/.env.exampletools/baml-bench/.gitignoretools/baml-bench/README.mdtools/baml-bench/convex/bamlBuilds.tstools/baml-bench/convex/crons.tstools/baml-bench/convex/issues.tstools/baml-bench/convex/lib.tstools/baml-bench/convex/maintenance.ts
💤 Files with no reviewable changes (1)
- baml_language/crates/baml_tests/projects/compiles/event_system/main.baml
✅ Files skipped from review due to trivial changes (8)
- baml_language/.gitignore
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/catch/exhaustiveness_checks.baml
- baml_language/.ci/size-gate/x86_64-pc-windows-msvc.toml
- baml_language/.ci/size-gate/aarch64-apple-darwin.toml
- baml_language/crates/baml_tests/Cargo.toml
- baml_language/crates/baml_lsp2_actions/src/check.rs
- baml_language/.ci/size-gate/wasm32-unknown-unknown.toml
- baml_language/.ci/size-gate/x86_64-unknown-linux-gnu.toml
🛑 Comments failed to post (7)
baml_language/Cargo.toml (1)
169-171:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check latest versions of the added crypto crates cargo search hex --limit 1 cargo search hmac --limit 1 cargo search sha1 --limit 1Repository: BoundaryML/baml
Length of output: 839
Update crypto crate version constraints (hmac/sha1)
Inbaml_language/Cargo.toml(lines 169-171),hex = "0.4"already allows the latesthex(0.4.3), buthmac = "0.12"(latest 0.13.0) andsha1 = "0.10"(latest 0.11.0) constrain updates to older minor releases. Consider bumping those version constraints if the new forks remain compatible.🤖 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/Cargo.toml` around lines 169 - 171, Bump the crypto crate version constraints in Cargo.toml: leave hex as-is, update the hmac dependency (symbol: hmac) from "0.12" to the newer "0.13" and update the sha1 dependency (symbol: sha1) from "0.10" to "0.11" so Cargo can pick the latest compatible minor releases; run cargo update and cargo build/test to ensure compatibility and fix any API changes if they surface.baml_language/crates/baml_cli/src/grep_command.rs (1)
447-469:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the existing method JSON keys or version this schema change.
grep --jsonis a machine-facing surface. This helper drops the previous per-methodkind/linefields in favor ofline_start/line_end, which will break existing consumers with no compatibility path. Keep the old keys alongside the new metadata, or gate the richer shape behind a versioned flag.Also applies to: 506-507
🤖 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_cli/src/grep_command.rs` around lines 447 - 469, The method_json serializer (and the similar serializer at lines 506-507) drops the previous per-method JSON keys ("kind" and the single "line") in favor of "line_start"/"line_end", which will break existing consumers; restore backward compatibility by emitting the old keys alongside the new ones (e.g., include "kind": m.kind and "line": line_number_at_offset(text, m.item_range.start().into())) or alternatively gate the richer shape behind a version flag, updating method_json and the analogous function to populate both legacy and new fields.baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
6473-6483:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve the concrete union member when deriving interface-call candidates.
Line 6476 drops the member’s concrete
type_args, and Lines 6529-6540 only look at in-bodyimplementsblocks. That means a union member that getsmethodonly through an inherited/default interface view on a generic class, or only throughimplements ... for C/ blanket rules, contributes no usable candidate here, sotry_lower_union_iface_dispatch()falls back to the non-dispatch path for a valid call.Also applies to: 6508-6541
🤖 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_mir/src/lower.rs` around lines 6473 - 6483, The loop over union members currently drops each member's concrete type arguments (patterning Tir2Ty::Class(qtn, _, _)), so class_member_method_candidates and later interface-candidate logic lose the concrete instantiated type; update the match to capture the member's type_args (e.g., Tir2Ty::Class(qtn, type_args, _)) and thread those concrete args into the interface-call candidate derivation (so class_member_method_candidates / candidate construction uses the member's instantiated type), ensuring candidates include members that arise only via inherited/default/generic interface views or blanket `implements ... for C` rules; also ensure try_lower_union_iface_dispatch uses these preserved concrete-member candidates when deciding dispatch.baml_language/crates/baml_compiler2_tir/src/builder.rs (3)
8099-8137:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAmbiguity detection here misses registry-only and same-interface instantiations.
union_interface_method_ambiguity()collapses members to bare interface QTN and then derives sources viaimplemented_interface_method_sources(), which only sees in-body classimplementsblocks. That misses cases likeGetter<int> | Getter<string>on a shared implementor, and any ambiguity introduced solely by out-of-body/blanket or primitive impl rules. In those casesresolve_union_member_ty()still returns callable arms viatry_registry_member(), so the union call gets folded instead of raising E0121.🤖 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_tir/src/builder.rs` around lines 8099 - 8137, union_interface_method_ambiguity currently only considers in-body implements via implemented_interface_method_sources (losing registry-only rules and distinct instantiations like Getter<int> vs Getter<string>), so update it to also consult the package_implements_registry(rule set) when gathering "sources" for a given class_qtn: for each rule in registry.interface_impl_rules where rule.for_ty_pattern is the class_qtn and rule.interface_ty is one of the union interfaces, check whether that rule provides the requested member (reusing try_registry_member/resolve logic) and add the corresponding (interface QTN + instantiation args) to the sources list; also stop collapsing members to bare interface QTN — preserve the interface instantiation (e.g., keep (QualifiedTypeName, Vec<Ty>) keys) so same-interface different-instantiation cases are treated as distinct, and deduplicate sources before checking sources.len() >= 2.
12647-12655:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse runtime overlap here, not subtype.
This helper uses
is_subtype()against each union member, but the surrounding pattern-dispatch code deliberately usestypes_overlap()to avoid numeric-tower false positives. As written, a pattern likefloatagainst anint | stringscrutinee now bypasses the mismatch check becauseint <: float, even though that arm can never match at runtime.🤖 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_tir/src/builder.rs` around lines 12647 - 12655, The helper pattern_overlaps_scrut_member currently checks overlap using is_subtype against each member from flatten_union_optional_members; replace those is_subtype calls with the runtime-aware types_overlap so pattern dispatch matches runtime behavior (avoid numeric-tower false positives). Specifically, in pattern_overlaps_scrut_member change the closure that does self.is_subtype(pat, m) || self.is_subtype(m, pat) to use self.types_overlap(pat, m) (or an equivalent symmetric types_overlap check) so the function uses types_overlap for both directions when iterating union/optional members.
13375-13418:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGeneric interface destructures currently lose the concrete type arguments.
The new interface branch runs before the existing missing-generic-args guard, so
Box { value }on aBox<int>scrutinee reachesinterface_field_infos_ordered()withtype_args == []. That lowersvalueusing the interface's generic parameter instead ofint, and unions likeBox<int> | Box<string>stop discriminating correctly. This branch needs the same missing-type-args protection as class destructures, or it needs to substitute the scrutinee's concrete interface args before lowering fields.🤖 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_tir/src/builder.rs` around lines 13375 - 13418, The interface destructure branch (matching Ty::Interface and calling interface_field_infos_ordered) is running before checking for missing generic args, so type_args can be empty and fields are lowered with the interface's generic parameters instead of the scrutinee's concrete args; fix by adding the same missing-type-args guard used for class destructures or by substituting the scrutinee's concrete interface type arguments into class_ty before calling interface_field_infos_ordered (i.e., when handling Ty::Interface(iface_name, type_args, _) use the concrete args from scrut_ty or bail/report missing args, then proceed to call interface_field_infos_ordered, analyze_and_lower, build sub_dpats and DPat::interface, and finally compute matched_ty with intersect_pattern_flow_types).baml_language/crates/baml_lsp2_actions/src/describe.rs (1)
1507-1523:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve
QualifiedTypeNamedeps by full path, not short name.This helper throws away the namespace/package and then dedupes + looks up by
qtn.name()only. In a project withroot.a.Userandroot.b.User,describecan now either link the dependency to the wrong symbol or drop one entirely, becauseresolve_dep_from_outline()returns the first matching short name. Sinceqtnalready has the exact path, this should resolve from the qualified name directly and dedupe on that full identity instead.🤖 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_lsp2_actions/src/describe.rs` around lines 1507 - 1523, The function collect_qtn_dep currently drops the namespace by using qtn.name() and dedupes/looks up by short name, which causes collisions (e.g., root.a.User vs root.b.User); change it to use the full qualified path from qtn (e.g., qtn.to_string() or qtn.full_path()) for both deduplication (the seen HashSet) and for resolving the dependency so you call resolve_dep_from_outline with the qualified name instead of qtn.name(); update references to the HashSet key and the resolve_dep_from_outline argument accordingly in collect_qtn_dep so deps and seen are keyed by the full identity and duplicates are correctly suppressed.
`SystemTime` is only used in the `#[cfg(not(target_arch = "wasm32"))]` `_default_round_robin_start`, but the import sat at module top level — so on wasm32 (which CI clippy-checks) it was unused, tripping `-D unused-imports`. Host clippy compiles the non-wasm branch, so it slipped through locally. Move it into the function alongside the existing `UNIX_EPOCH` import. Verified clean via clippy --target wasm32-unknown-unknown. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
baml_language/crates/bex_engine/src/lib.rs (2)
400-415:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the rustdoc examples to the new public signatures.
Both examples still show the pre-refactor API shape (
BexEngine::new(bytecode, sys_ops)andcall_function(name, args)), so copying them now produces compile errors for missingargv,FunctionCallContext, andcopy_objects.Also applies to: 1119-1123
🤖 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 400 - 415, The rustdoc examples use the old API (BexEngine::new(bytecode, sys_ops) and call_function(name, args)) which no longer compiles; update examples to the current public signatures by constructing BexEngine with the new constructor signature and calling the updated method that requires a FunctionCallContext, argv, and copy_objects parameters (replace BexEngine::new(...) usage and calls to call_function(...) with the new constructor and calls supplying a FunctionCallContext, the argv vector, and the copy_objects flag), and make the same changes in the other example lines referenced (including the example around lines 1119-1123) so examples match the current public API.
2036-2089:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop the now-dead
_event_resultconversion from the root return path.After the tracing removal, this tuple still forces an extra
vm_value_to_owned(...)/external.clone()on every successful root return. In thecopy_objects = falsehandle path that means we still walk and clone the full object graph before discarding it, which largely erases the fast path this PR is trying to preserve.Suggested cleanup
- let (return_value, _event_result) = if !copy_objects { + if cancelled { + return Err(cancelled_unhandled_throw()); + } + + let return_value = if !copy_objects { if let Some(ptr) = value.as_object_ptr() { // SAFETY: the active thread holds the heap permit // through `thread.proof()`. @@ if matches!(unsafe { ptr.get() }, Object::Float(_)) { let external = self.convert_vm_value_to_external_with_type( value, &return_type, thread.proof(), )?; - (external.clone(), external) + external } else { let handle = self.heap.create_handle(ptr); - ( - BexExternalValue::Handle(handle), - self.vm_value_to_owned(thread.proof(), value), - ) + BexExternalValue::Handle(handle) } } else { let external = self.convert_vm_value_to_external_with_type( value, &return_type, @@ let external = crate::conversion::coerce_return_to_declared_type( external, &return_type, )?; - (external.clone(), external) + external } } else { let external = self.convert_vm_value_to_external_with_type( value, &return_type, @@ let external = crate::conversion::coerce_return_to_declared_type( external, &return_type, )?; - (external.clone(), external) + external }; - - if cancelled { - return Err(cancelled_unhandled_throw()); - }🤖 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 2036 - 2089, The tuple (_event_result) forces an extra vm_value_to_owned/external.clone on every successful root return; remove the unused second element and stop computing it. In the block building (return_value, _event_result) eliminate the secondary computation: compute only return_value (using convert_vm_value_to_external_with_type and, when creating a handle via self.heap.create_handle(ptr), avoid calling vm_value_to_owned or cloning the external), and adjust subsequent code to use that single return_value; update references to _event_result accordingly and remove vm_value_to_owned/external.clone calls to restore the fast path for copy_objects = false and handle creation.
🤖 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.
Outside diff comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 400-415: The rustdoc examples use the old API
(BexEngine::new(bytecode, sys_ops) and call_function(name, args)) which no
longer compiles; update examples to the current public signatures by
constructing BexEngine with the new constructor signature and calling the
updated method that requires a FunctionCallContext, argv, and copy_objects
parameters (replace BexEngine::new(...) usage and calls to call_function(...)
with the new constructor and calls supplying a FunctionCallContext, the argv
vector, and the copy_objects flag), and make the same changes in the other
example lines referenced (including the example around lines 1119-1123) so
examples match the current public API.
- Around line 2036-2089: The tuple (_event_result) forces an extra
vm_value_to_owned/external.clone on every successful root return; remove the
unused second element and stop computing it. In the block building
(return_value, _event_result) eliminate the secondary computation: compute only
return_value (using convert_vm_value_to_external_with_type and, when creating a
handle via self.heap.create_handle(ptr), avoid calling vm_value_to_owned or
cloning the external), and adjust subsequent code to use that single
return_value; update references to _event_result accordingly and remove
vm_value_to_owned/external.clone calls to restore the fast path for copy_objects
= false and handle creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 42cd79aa-6b46-4c20-9ad9-e77457871a17
📒 Files selected for processing (1)
baml_language/crates/bex_engine/src/lib.rs
Brings in the VM perf work that landed on canary via #3646 (CPython-style LoadVar2/StoreVar2 superinstructions, lazy faulting PC, kperf fail-closed, sequence-point fusion guard, StoreVar2 notification merge) plus other canary changes, while preserving trim-events' event/viz stripping. Conflict resolution: - kperf.rs: took canary's Option-based fail-closed version (pure perf). - emit.rs: took canary's sequence-point fusion guard (pure perf). - vm.rs: took canary's consolidated call-classify path and cur_pc reads; kept trim-events' event handling. - Removed duplicate LoadVar2/StoreVar2 match arms that the auto-merge produced in vm.rs (dispatch) and bytecode.rs (Display / lower_to_compact / instruction_to_opcode). - Kept trim-events' stripping of NotifyBlock/VizEnter/VizExit and the deleted node SDK event/tracing tests. Snapshots verified consistent (lib codegen, baml_src, bytecode_format). Co-Authored-By: Claude Opus 4.8 <[email protected]>
⏭️ 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):
|
Brings in 34 commits of canary changes; perf files (vm.rs, bytecode.rs, emit.rs, debug.rs) now auto-merge cleanly since the VM perf set is identical on both sides. Conflict resolution (all event-stripping related): - bex_engine/lib.rs: kept the event-stripped side — dropped canary's VmExecState::SpanNotify handler (trim-events has no SpanNotify variant) and the SpanNotification/TlabHolder imports. InterfaceImplementors now comes from bex_vm_types::types (canary added it there), so the private bex_vm::vm::InterfaceImplementors import was removed; ValueKind dropped (only used by the stripped span handler). - baml_lsp_server/Cargo.toml, bridge_wasm/Cargo.toml: kept baml_version (used by lib.rs) but dropped bex_events/bex_events_native (event system). - Cargo.lock: regenerated against the resolved manifests. Whole workspace compiles; snapshots verified consistent (lib codegen, baml_src, bytecode_format). Co-Authored-By: Claude Opus 4.8 <[email protected]>
…C probe Standalone uv script that packs the same workload with two baml-cli builds, runs each under the in-VM Apple-Silicon kperf probe (BAML_KPERF=1 + sudo), and tables instructions retired / CPU cycles / VM ops plus a per-op IPC summary with a stalls-vs-codegen verdict. Used to compare this branch (events trimmed) against canary; surfaced that the two retire identical VM ops, so cycles — not instructions — are the metric that matters. Co-Authored-By: Claude Opus 4.8 <[email protected]>
hellovai
changed the title
The spec for the unified always-on observability stream that replaces the surface trimmed in this PR (per-worker lock-free ring, always-on enter/exit/ error, .bamlprof artifact), plus the adversarial gap review of that design. Context for whoever builds the replacement. Co-Authored-By: Claude Opus 4.8 <[email protected]>
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.
What this PR does
Trims ("strips") the current BEX event/tracing system down to nothing, intentionally not preserving any existing contract (protobuf-FFI wire,
Collector/EventSinkAPI, JSONL), so a unified always-on observability stream can be built fresh on top of a clean base.Removed / gutted
bex_eventsinternals:event_store.rs,serialize.rs,types.rs, most ofcollector.rs.bex_events_native— entire crate deleted.bridge_ctypes/event_encode.rs;bridge_cffihost-span plumbing;baml_lsp_server/playground_event_sink.rs.SpanNotificationtype +VmExecState::SpanNotifypath, theNotifyBlock/VizEnter/VizExitopcodes, and per-call span emission (gated oncallee.trace).bex_engine: theSpanNotifyhandling arm and collector wiring (BexEngine::new/bex_projectevent-sink params).event_system.rs+tracing.rs, pythontest_tracing.py+test_collector.py, nodejs SDKtest_tracing/test_collector, and theevent_systemcompile fixtures + snapshots.Kept on purpose
baml.events.send()still compiles and returnsnull— theSendEventopcode is now a no-op (pops its two args, pushes null), so existing BAML referencing it doesn't break.Watch/Notify/StoreVarLoadVar) — that's separate from tracing.Why a clean slate: today BEX has two observability shapes and neither can profile a BAML program —
bex_eventsis opt-in per@trace, deep-copies every payload, and funnels every event through one process-globalMutex<CollectorStore>; native CPU samplers see worker frames but never the logical BAML function. The replacement design (a per-worker lock-free ring, always-on enter/exit/error, a.bamlprofartifact) deliberately changes every contract, so the old surface is removed first. Full spec lives inbex-event-stream-design.md(currently a local doc — see "For the next owner").Relationship to the VM perf work (already on canary)
The CPython-aligned VM changes that the original title mentions —
LoadVar2/StoreVar2movement superinstructions, lazy faulting PC, unchecked local-slot indexing, call-path deref consolidation, and the in-processkperfPMC probe — were split out and merged tocanaryin #3646. They are not in this diff; this branch is rebased on top of them and inherits them. This PR is now purely the event trim (+ the perf-comparison tool below).Perf finding — trimming events is also a small runtime win
Measured with the tool below (Apple Silicon, tracing off,
computeworkloads), this branch vscanary:Removing the event arms/opcodes gives LLVM a leaner, higher-ILP dispatch loop, so it completes in fewer cycles despite retiring slightly more instructions. Consistent ~3–9% cycle wins on the heavy, low-variance workloads (closure-apply, fib32-recursive, fib-iterative, collatz, bubble-sort). Lesson baked into the tooling: compare cycles, not instructions — instructions-retired is confounded by codegen when comparing two different binaries.
How to run the perf tests we added
The VM carries an in-process Apple-Silicon PMC probe at
crates/bex_vm/src/kperf.rs(from #3646):BAML_KPERF=1+sudo(PMC access). Printed to stderr at process exit. Works in any normal release build.kperfcargo feature (it gates a per-op counter — a store on the hottest path, hence off by default).scripts/kperf_compare.pyis a standaloneuvscript (stdlib only) that A/Bs twobaml-clibuilds: it packs the same workload with each, runs both under the probe, and prints instructions / cycles / VM ops tables plus a per-op IPC summary that auto-diagnoses stalls vs codegen vs genuinely-faster.Reading the output:
BRANCH_MISPRED_NONSPEC,L1I); more instructions yet fewer cycles ⇒ genuinely faster.--runs 5+ (instructions are exact, fine at 1 run). Rows flagged~exceeded 1% run variance;*are below the probe's ~1e6 print resolution. Workloads live intools/speedtest/workloads/(.mdfiles with a## BAMLsection; the script extracts it).Validation
cargo check --workspaceclean;bex_vm/bex_engine/baml_compiler2_emitclippy clean.baml_src,bytecode_format).canary(merged in twice during this work).For the next owner
bex-event-stream-design.md— and its gap review —bex-event-stream-design-GAPS.md— are committed in this PR (repo root). They define what replaces the trimmed surface (unified lock-free event stream →.bamlprof).SendEventis a deliberate no-op placeholder; the new stream should give it real semantics (or remove the opcode).🤖 Generated with Claude Code