Repository navigation
Conversation
The virtual call currently cannot handle the sysop for the system randomness source
This will allow indirect/virtual calls to execute sysops (WIP)
- Allocation failure on large RNG buffers now raises `AllocFailure` BAML panic - Negative buffer size now consistentily raises a user panic - Doc fixes - Bump diagnostic number to deconflict
Native `$rust_function`s currently don't support default args. For now we have to make the `new` static method a bytecode function that just wraps the native `_new`.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
2kai2kai2
enabled auto-merge
July 20, 2026 23:29
Contributor
📝 WalkthroughWalkthroughAdds the ChangesRandom namespace and dispatch
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BamlProgram
participant IoSysOpsBuilder
participant RuntimeProvider
participant BexVm
BamlProgram->>BexVm: call baml.random operation
BexVm->>IoSysOpsBuilder: resolve random sys-op
IoSysOpsBuilder->>RuntimeProvider: request bytes or random_int
RuntimeProvider-->>BexVm: return random result
BexVm-->>BamlProgram: deliver result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Binary size checks passed✅ 7 passed
Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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_builtins2_codegen/src/codegen_io.rs (1)
1334-1354: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDisambiguate clean-trait method names for
implements-scoped IO builtins
emit_one_class_traitstill derives the clean method name fromio_method_name(m)only, so two interfaces that declare the same$rust_io_functionon one class will emit duplicatefn foo(...)items in the same Rust trait. The interface ambiguity is already handled at call sites; the generated trait needs the same interface qualifier thatio_class_dispatch_keyand the glue names already use. [codegen_io.rs:1338-1354, 1442-1449]🤖 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_builtins2_codegen/src/codegen_io.rs` around lines 1334 - 1354, Update emit_one_class_trait’s clean-method name generation to include the implements-interface qualifier for methods sharing the same $rust_io_function, matching the naming scheme used by io_class_dispatch_key and the generated glue names. Replace the io_method_name-only derivation near method_ident while preserving receiver handling and return-type generation.
🧹 Nitpick comments (2)
baml_language/crates/bex_engine/tests/random.rs (1)
1-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for the pure helpers alongside this e2e suite.
This file is an integration suite exercising the full VM↔SysOps wiring, which is reasonable for validating cross-crate dispatch, but the pure helpers introduced in
bex_vm::package_baml::random(seed_array,byte_count,next_i63) have no dedicated unit tests and are easy to test in isolation (e.g. seed-too-short panic, negative byte-count panic,next_i63's range/determinism given a fixed seed).As per path instructions: "
**/*.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/tests/random.rs` around lines 1 - 225, The pure helpers seed_array, byte_count, and next_i63 in bex_vm::package_baml::random lack isolated unit coverage. Add Rust unit tests alongside their implementation for short-seed panic behavior, negative byte-count panic behavior, and deterministic next_i63 results within the signed range for a fixed seed, while keeping this file focused on end-to-end VM↔SysOps dispatch.Source: Path instructions
baml_language/crates/sys_native/src/io_impls.rs (1)
165-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
SystemRandom.random/random_intlogic across the native and wasm backends. Both implementations repeat byte-count validation, fallible allocation, entropy fill, andHostUnavailableerror mapping verbatim, differing only in whichgetrandomcrate/function performs the fill.
baml_language/crates/sys_native/src/io_impls.rs#L165-L220: extract the shared byte-count-validate + fallible-alloc + fill + map-to-HostUnavailablesequence into a helper (e.g. insys_ops) parameterized over afill(&mut [u8]) -> Result<(), impl Display>closure, and call it here withgetrandom::getrandom.baml_language/crates/bridge_wasm/src/wasm_random.rs#L1-L67: call the same shared helper withgetrandom_03::fillinstead of duplicating the sequence.🤖 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/sys_native/src/io_impls.rs` around lines 165 - 220, Extract the shared random-byte validation, fallible allocation, entropy fill, and HostUnavailable mapping into a helper in sys_ops, parameterized by a fill closure returning a displayable error. Update baml_language/crates/sys_native/src/io_impls.rs lines 165-220 to use it with getrandom::getrandom, and update baml_language/crates/bridge_wasm/src/wasm_random.rs lines 1-67 to use the same helper with getrandom_03::fill; preserve each backend’s existing random and random_int 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/crates/baml_compiler2_tir/src/interfaces/impl_rules.rs`:
- Around line 1204-1231: Move the generic sys-op validation in the impl-method
loop to after the `iface_spec` lookup succeeds, or gate it on a successful
interface-member resolution. Ensure `GenericSysOpMethodInInterfaceImpl` is
emitted only for methods that actually override an interface member, while
unknown methods still produce the existing `UnknownInterfaceMember` diagnostic.
---
Outside diff comments:
In `@baml_language/crates/baml_builtins2_codegen/src/codegen_io.rs`:
- Around line 1334-1354: Update emit_one_class_trait’s clean-method name
generation to include the implements-interface qualifier for methods sharing the
same $rust_io_function, matching the naming scheme used by io_class_dispatch_key
and the generated glue names. Replace the io_method_name-only derivation near
method_ident while preserving receiver handling and return-type generation.
---
Nitpick comments:
In `@baml_language/crates/bex_engine/tests/random.rs`:
- Around line 1-225: The pure helpers seed_array, byte_count, and next_i63 in
bex_vm::package_baml::random lack isolated unit coverage. Add Rust unit tests
alongside their implementation for short-seed panic behavior, negative
byte-count panic behavior, and deterministic next_i63 results within the signed
range for a fixed seed, while keeping this file focused on end-to-end VM↔SysOps
dispatch.
In `@baml_language/crates/sys_native/src/io_impls.rs`:
- Around line 165-220: Extract the shared random-byte validation, fallible
allocation, entropy fill, and HostUnavailable mapping into a helper in sys_ops,
parameterized by a fill closure returning a displayable error. Update
baml_language/crates/sys_native/src/io_impls.rs lines 165-220 to use it with
getrandom::getrandom, and update
baml_language/crates/bridge_wasm/src/wasm_random.rs lines 1-67 to use the same
helper with getrandom_03::fill; preserve each backend’s existing random and
random_int behavior.
🪄 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: 23c890c3-03b7-4687-abdf-8fcdfea43a0d
⛔ Files ignored due to path filters (9)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.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____04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
baml_language/Cargo.tomlbaml_language/crates/baml_builtins2/baml_std/baml/ns_random/random.bamlbaml_language/crates/baml_builtins2/src/lib.rsbaml_language/crates/baml_builtins2_codegen/src/codegen.rsbaml_language/crates/baml_builtins2_codegen/src/codegen_io.rsbaml_language/crates/baml_builtins2_codegen/src/extract.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler_diagnostics/src/diagnostic.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/bex_engine/tests/random.rsbaml_language/crates/bex_vm/Cargo.tomlbaml_language/crates/bex_vm/src/package_baml/mod.rsbaml_language/crates/bex_vm/src/package_baml/random.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bridge_wasm/Cargo.tomlbaml_language/crates/bridge_wasm/src/lib.rsbaml_language/crates/bridge_wasm/src/wasm_random.rsbaml_language/crates/sys_native/Cargo.tomlbaml_language/crates/sys_native/src/io_impls.rsbaml_language/crates/sys_ops/src/lib.rs
Comment on lines
1204
to
+1231
| for &method_loc in &data.methods { | ||
| let method_name = function_data(db, method_loc).name.clone(); | ||
|
|
||
| // A `$rust_io_function` (sys-op) override with method-level generics can't | ||
| // be dispatched through interface (virtual) dispatch — the only way an | ||
| // impl-block method is reached. Virtual dispatch does not reconstruct the | ||
| // synthetic type-argument slots a generic sys-op's glue reads off the | ||
| // stack, so such a call would fail at runtime; reject it at declaration. | ||
| // (A generic sys-op declared directly on a class lowers to a direct | ||
| // `SysOp` instruction, which supplies those slots, so it is fine.) | ||
| if !function_data(db, method_loc).generic_params.is_empty() | ||
| && matches!( | ||
| baml_compiler2_ppir::function_body(db, method_loc).as_ref(), | ||
| baml_compiler2_hir::body::FunctionBody::Builtin( | ||
| baml_compiler2_ast::BuiltinKind::Io | ||
| ) | ||
| ) | ||
| { | ||
| diags.push(( | ||
| crate::infer_context::TirTypeError::GenericSysOpMethodInInterfaceImpl { | ||
| interface: iface_qtn.clone(), | ||
| method: method_name.clone(), | ||
| }, | ||
| ImplDiagnosticLocation::Method(method_name.clone()), | ||
| )); | ||
| continue; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
New generic-sysop check fires before confirming the method is actually an interface member.
The check at lines 1207-1230 runs and continues before the iface_spec lookup at 1233-1247. For a method name that isn't a real member of the interface, this can emit a misleading GenericSysOpMethodInInterfaceImpl (naming the interface as if it declares this method) alongside the correct UnknownInterfaceMember diagnostic from impl_data(). Consider moving this check after (or gating it on) successful iface_spec resolution so it only fires for methods that genuinely override an interface member.
🔧 Sketch of a fix
- if !function_data(db, method_loc).generic_params.is_empty()
- && matches!(
- baml_compiler2_ppir::function_body(db, method_loc).as_ref(),
- baml_compiler2_hir::body::FunctionBody::Builtin(
- baml_compiler2_ast::BuiltinKind::Io
- )
- )
- {
- diags.push((...));
- continue;
- }
-
// The interface method this override targets: a required sig or a default's function.
let iface_spec = if let Some(sig) = iface_data.required_methods.iter().find(...) {
...
} else if let Some(&default_loc) = ... {
...
} else {
continue; // unknown member → E0115 (impl_data)
};
+
+ if !function_data(db, method_loc).generic_params.is_empty()
+ && matches!(..., FunctionBody::Builtin(BuiltinKind::Io))
+ {
+ diags.push((...));
+ continue;
+ }🤖 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/interfaces/impl_rules.rs` around
lines 1204 - 1231, Move the generic sys-op validation in the impl-method loop to
after the `iface_spec` lookup succeeds, or gate it on a successful
interface-member resolution. Ensure `GenericSysOpMethodInInterfaceImpl` is
emitted only for methods that actually override an interface member, while
unknown methods still produce the existing `UnknownInterfaceMember` diagnostic.
Merged
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the
baml.random.Rnginterface which represents a randomness source.Includes three standard library implementors:
baml.random.SystemRandomfetches a secure random value from the OS/browser/etc (typically requires leaving the process but is secure; good for seeding PRNGs)baml.random.Xoshiro256PlusPlusis a widely used fast PRNG (not suitable for security-dependent uses)baml.random.ChaCha20is a widely used cryptographically secure PRNGAlso makes sysop
$rust_io_functions work inimplementsblocks including for indirect/virtual calls.Summary by CodeRabbit
New Features
Bug Fixes
Tests