Repository navigation
feat(compiler): execute calls into mounted packages (BEP-066 slice 6a, PR 4) - #4332
Conversation
|
@coderabbitai review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Action performedReview finished.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMounted packages now expose linkable external functions, methods, implementations, interfaces, generics, fields, and type metadata. TIR and MIR resolve and lower these targets. Tests cover compilation, linking, dispatch, runtime execution, stream companions, errors, and unsupported builtins. ChangesMounted Package Support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant TIR
participant PackageInterface
participant MIR
participant LinkedBytecode
Consumer->>TIR: Resolve mounted function or method
TIR->>PackageInterface: Read exported callable and interface metadata
PackageInterface-->>TIR: Return external target and generic metadata
TIR->>MIR: Lower external or interface-dispatched call
MIR->>LinkedBytecode: Emit linkable call reference
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
baml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rs (1)
3226-3249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
provides_methodfallback branch is unreachable.
get_methodalready resolves the implemented interface's own defaults, in both theSourceand theMountedarm, using the same name predicate as this fallback. It also returnsNonewhenself.data(db)isNone, which the fallback treats asfalse. So thematchafter the early return can never add atruethatget_methoddid not already produce.Reduce the body to the
get_methodprobe, or document a case the fallback is meant to catch.♻️ Proposed simplification
pub fn provides_method(&self, db: &'db dyn crate::Db, method: &Name) -> bool { - use baml_compiler2_ppir::item_data::{function_data, interface_data}; - if self.get_method(db, method).is_some() { - return true; - } - let Some(data) = self.data(db) else { - return false; - }; - // The implemented interface's own default — through whichever - // declaration surface exists (source loc or mounted row). - match &data.interface { - ImplInterfaceTarget::Source(iface_loc) => interface_data(db, *iface_loc) - .default_methods - .iter() - .any(|&fn_loc| function_data(db, fn_loc).name == *method), - ImplInterfaceTarget::Mounted(qtn) => matches!( - crate::package_interface::mounted_type_row(db, qtn), - Some(crate::package_interface::ExportedType::Interface { - default_methods, - .. - }) if default_methods.iter().any(|m| m.name == *method) - ), - } + self.get_method(db, method).is_some() }🤖 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 3226 - 3249, Remove the unreachable fallback match from provides_method and make the method return solely whether get_method(db, method) resolves a result. Preserve the existing method-name lookup behavior without retaining redundant Source or Mounted interface-default handling.baml_language/crates/baml_tests/tests/mounted_package_calls.rs (2)
142-183: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the compiled library artifacts across tests.
consumer_programcallscompile_libraryon every invocation, and 13 tests reach it. Each call builds a freshProjectDatabase, loads every builtin stub, checks the library, derives thePackageInterface, and emits full bytecode. The library fixture is constant, so this repeats the most expensive step in the file 13 times.Both artifacts are deterministic and cloneable. Compute them once with
std::sync::LazyLockand clone per test.♻️ Proposed caching
+static LIBRARY: std::sync::LazyLock<(Vec<u8>, Program)> = + std::sync::LazyLock::new(compile_library); + /// Phase 2: a fresh consumer database — blob mounted as `app`, NO `app` /// source — checked clean, then spliced against the library image. fn consumer_program(user_src: &str) -> Program { - let (blob, lib_program) = compile_library(); + let (blob, lib_program) = (LIBRARY.0.clone(), &LIBRARY.1); let mut db = ProjectDatabase::new(); db.set_project_root(std::path::Path::new("/mounted-calls")); db.set_mounted_packages([("app".to_string(), blob)].into()); db.add_file("main.baml", user_src); assert_no_diagnostic_errors(&db); - generate_project_bytecode_with_stdlib(&db, &compile_options(), OPT, &lib_program) + generate_project_bytecode_with_stdlib(&db, &compile_options(), OPT, lib_program) .expect("consumer compiles against the mounted blob") }
Programmust beSend + SyncforLazyLockin a multi-threaded test runner. If it is not, use athread_local!OnceCellinstead.🤖 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/tests/mounted_package_calls.rs` around lines 142 - 183, Cache the deterministic results of compile_library in a std::sync::LazyLock, ensuring Program and the serialized interface blob satisfy Send + Sync; if Program does not, use a thread_local OnceCell instead. Update consumer_program to initialize the cache once and clone both artifacts for each test, while preserving the existing compilation and consumer behavior.
1-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the check-only tests into the library crate.
mounted_stream_companion_supports_consumer_llm_expansion(line 407) andmounted_builtin_call_stays_reserved(line 505) assert only on diagnostics. They do not run the engine. The sibling modulebaml_tests/src/compiler2_tir/package_interface.rsalready holds equivalent check-level coverage as unit tests. Keep the engine-executing tests here and move the two check-only tests into the library crate.Based on coding guidelines: "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/baml_tests/tests/mounted_package_calls.rs` around lines 1 - 38, Move mounted_stream_companion_supports_consumer_llm_expansion and mounted_builtin_call_stays_reserved from this integration test module into the library crate’s compiler2_tir/package_interface.rs unit-test module, preserving their diagnostic assertions and coverage. Keep only tests that execute the engine, such as the run_compiled-based cases, in this file.Source: Coding guidelines
baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
10494-10498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the emptiness check before the argument lowering.
lower_call_arg_operandsemits statements for every provided argument. Ifarg_opsis empty, this function returnsfalseafter those statements were already emitted, and the caller then re-lowers the same arguments through the ordinary call path. Side-effecting arguments would evaluate twice.The current caller guards on
args.first(), so the branch is not reachable today. The rest of this file keeps every fallible step before any operand is lowered (seevirtual_field_assign_target). Keep that invariant here.♻️ Proposed reordering
let Some((iface_tn, iface_type_args, iface_assoc)) = dispatch_target else { return false; }; - let mut arg_ops = self.lower_call_arg_operands(expr_id, args); - if arg_ops.is_empty() { + if args.is_empty() { return false; } + let mut arg_ops = self.lower_call_arg_operands(expr_id, args); + if arg_ops.is_empty() { + return false; + } let receiver_op = arg_ops.remove(0);🤖 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 10494 - 10498, Update the call-lowering logic around lower_call_arg_operands to check whether args is empty before invoking it, returning false immediately when no arguments exist. Only call lower_call_arg_operands for non-empty args, preserving the invariant that fallible checks occur before lowering operands and preventing side-effecting arguments from being emitted before fallback re-lowering.baml_language/crates/baml_compiler2_tir/src/inference.rs (1)
839-864: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify
user_generic_params()zip invariant is always maintained.The receiver-owner's generic params when they are supplied at the CALL site (the static/UFCS form on a generic mounted class); empty for bound-receiver accesses (the receiver's own class args seed the frame), free functions, and interface targets.
user_generic_params()zipsgeneric_paramswithgeneric_param_boundsand silently truncates to the shorter vector if the two ever diverge in length. The struct doc states they are always parallel, so this is a defensive-coding note rather than a live bug: if a future producer ofExternalCallable(e.g. a new export kind) ever populates one vector without the other, the mismatch would silently drop bound checks instead of panicking or erroring.Consider a debug assertion (
debug_assert_eq!(self.generic_params.len(), self.generic_param_bounds.len())) in the constructor sites, or inuser_generic_params()itself, so a future violation surfaces immediately instead of silently under-enforcing generic bounds.🤖 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/inference.rs` around lines 839 - 864, Add a debug assertion enforcing equal lengths for ExternalCallable::generic_params and generic_param_bounds, preferably in user_generic_params() or every ExternalCallable constructor, before the zip operation. Preserve the existing filtering behavior while ensuring any future producer that violates the documented parallel-vector invariant fails immediately in debug builds.baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs (1)
804-829: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider cloning only the needed fields instead of the whole exported row.
ExportedType::Interfacecarries every required and default method with full parameter types. This path clones the whole row for each external member access, then keeps onlyqtn,self_param,generic_params, and one method. Extracting those four values inside a scoped borrow and cloning them avoids the per-access cost on large interfaces.This is a performance note only; behavior is correct as written.
🤖 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/interface_resolution.rs` around lines 804 - 829, Update the exported-type lookup in the interface member-resolution flow to borrow the mounted or package interface row and clone only qtn, self_param, generic_params, and the matching method, rather than cloning the entire ExportedType row. Preserve the existing lookup precedence and return behavior while avoiding retention or cloning of unrelated methods and parameter types.
🤖 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_mir/src/lower.rs`:
- Around line 10920-10938: Update the mounted-interface fallback in
resolve_interface_method_spec to return only user-declared generic parameters,
excluding synthetic __effect_param_N entries from each matched
required_methods/default_methods signature. Reuse or introduce the existing
user-generic-count helper comparable to ExternalCallable::user_generic_params so
interface_method_generic_count matches the generics stored by
resolve_interface_method_spec.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 10666-10691: Update the bare callable-reference lowering around
the mounted-function handling shown here to detect builtin-kind mounted free
functions and methods before fallback resolution. Record the reference
expression as a reserved foreign callable and emit MountedPackageCallUnsupported
at that expression, ensuring package-qualified and local fallback paths do not
produce Null or unresolved-local errors; preserve normal MemberResolution for
non-builtin callables.
In `@baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs`:
- Around line 432-455: Update the inlined ambiguity handling around the
SelfReceiver match to inspect the mounted Source declarers and preserve
field-vs-method classification, reporting AmbiguousInterfaceField when the
declarers define a field and AmbiguousInterfaceMethod otherwise. Keep the
existing receiver, sources, and unknown-type behavior unchanged.
In `@baml_language/crates/baml_compiler2_tir/src/package_interface.rs`:
- Around line 295-349: Update def_to_item_ref so the owner == raw_owner
equivalence check is enforced in release builds instead of using
debug_assert_eq!. Preserve the existing raw_owner fallback and make mismatches
fail through an active assertion or equivalent non-debug failure path,
maintaining impl_method_symbol_owner as the B-693 symbol contract.
---
Nitpick comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 10494-10498: Update the call-lowering logic around
lower_call_arg_operands to check whether args is empty before invoking it,
returning false immediately when no arguments exist. Only call
lower_call_arg_operands for non-empty args, preserving the invariant that
fallible checks occur before lowering operands and preventing side-effecting
arguments from being emitted before fallback re-lowering.
In `@baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs`:
- Around line 804-829: Update the exported-type lookup in the interface
member-resolution flow to borrow the mounted or package interface row and clone
only qtn, self_param, generic_params, and the matching method, rather than
cloning the entire ExportedType row. Preserve the existing lookup precedence and
return behavior while avoiding retention or cloning of unrelated methods and
parameter types.
In `@baml_language/crates/baml_compiler2_tir/src/inference.rs`:
- Around line 839-864: Add a debug assertion enforcing equal lengths for
ExternalCallable::generic_params and generic_param_bounds, preferably in
user_generic_params() or every ExternalCallable constructor, before the zip
operation. Preserve the existing filtering behavior while ensuring any future
producer that violates the documented parallel-vector invariant fails
immediately in debug builds.
In `@baml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rs`:
- Around line 3226-3249: Remove the unreachable fallback match from
provides_method and make the method return solely whether get_method(db, method)
resolves a result. Preserve the existing method-name lookup behavior without
retaining redundant Source or Mounted interface-default handling.
In `@baml_language/crates/baml_tests/tests/mounted_package_calls.rs`:
- Around line 142-183: Cache the deterministic results of compile_library in a
std::sync::LazyLock, ensuring Program and the serialized interface blob satisfy
Send + Sync; if Program does not, use a thread_local OnceCell instead. Update
consumer_program to initialize the cache once and clone both artifacts for each
test, while preserving the existing compilation and consumer behavior.
- Around line 1-38: Move
mounted_stream_companion_supports_consumer_llm_expansion and
mounted_builtin_call_stays_reserved from this integration test module into the
library crate’s compiler2_tir/package_interface.rs unit-test module, preserving
their diagnostic assertions and coverage. Keep only tests that execute the
engine, such as the run_compiled-based cases, in this file.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a7c7170-b4d1-422d-8e88-49776fb326b3
📒 Files selected for processing (14)
baml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_lsp2_actions/src/definition.rsbaml_language/crates/baml_lsp2_actions/src/tokens/classify.rsbaml_language/crates/baml_project/src/db.rsbaml_language/crates/baml_tests/src/compiler2_tir/package_interface.rsbaml_language/crates/baml_tests/tests/mounted_package_calls.rs
Binary size checks passed✅ 7 passed
Generated by |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_tests/tests/interfaces.rs`:
- Around line 11922-11934: Update the assertion in
conjunction_ambiguous_source_fields_keep_field_diagnostic to require an
ambiguity-specific diagnostic, such as the expected E0131 code or “ambiguous”
wording, rather than only “field `value`”. Preserve the existing fixture and
ensure the test still validates the ambiguous source-field error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 11549a6b-e78e-4206-ae01-7772f684bb41
📒 Files selected for processing (5)
baml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rsbaml_language/crates/baml_tests/tests/interfaces.rsbaml_language/crates/baml_tests/tests/mounted_package_calls.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/crates/baml_tests/tests/mounted_package_calls.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
- baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs
- baml_language/crates/baml_compiler2_mir/src/lower.rs
…review) Needles are verbatim substrings of the AmbiguousInterfaceField format string the fixture already triggers; split into field-pin + wording-pin so an unrelated value-mentioning diagnostic cannot satisfy the suite.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…R 5 — capstone) (#4333) **BEP-066 slice-6a stack, PR 5 of 5 — the capstone.** Chained on #4332. With this green, the slice-6a stack is complete: a package consumed as an interface blob is provably indistinguishable from the same package consumed as source. ## The parity oracle (`mounted_package_parity.rs`) - Source-vs-blob **runtime parity** across the full surface: interfaces + requires + assoc defaults + default methods, attrs, generics/bounds, enums, aliases, in-body/out-of-body/blanket impls, user impls of mounted interfaces, caught-throw behavior. - **Byte-identical diagnostic fingerprints** (code/message/severity/phase/user range) for the misuse variants: wrong arity, unsatisfied bound, non-exhaustive match, missing impl members. - **Byte-identical artifacts**: dependency units, package metadata, consumer import tables raw-identical; consumer units + whole programs identical after normalizing database-local debug FileIds — the sole legitimate difference (the known B-693 §9b wart), with paths/ranges/symbols/executable bytes identical. ## What the oracle caught (fixed in the implementation, never weakened in the oracle) 1. Source auxiliary packages didn't participate in user dependency resolution the way mounted ones do. 2. Mounted interface defaults/assoc-defaults/fields/method tables seeded emission differently from source interfaces. 3. Impl-method exports mishandled synthetic callback-effect parameters vs enclosing impl generics. ## Also in this PR - **Coherence-fails-open hole closed and proven**: user-vs-blob impl overlap yields exactly one primary-only E0132 attributed at the user's impl, dep side rendered structurally. - **Public link seam**: the two-DB compile-and-link harness promoted from test scaffolding to a documented API — slice 6's `Package.compile` consumes this. - **Residue ledger** documented adjacent to the public API (E0158 builtin residue, span-dependent LSP ops, re-aliasing/transitive re-export, live `$stream` loading, artifact versioning, dispatcher synthesis) — slice 6 starts from a ledger, not archaeology. ## Gates Full baml_tests green · all-features mounted call/parity suites 26/26 · HIR/TIR/project · bytecode_cache 43/43 · fmt/clippy/rustdoc all-features `-D warnings` · **zero snapshot churn**. Implemented by a Codex (gpt-5.6-sol) worker under stack-manager review. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for compiling applications against mounted, source-less packages, including their default methods, type defaults, and field definitions. - Added dependency linking for precompiled package artifacts before consumer compilation. - **Bug Fixes** - Improved handling of generic callback-effect metadata in exported functions and implementation methods. - Improved package discovery and dependency resolution for auxiliary and mounted source packages. - **Tests** - Added comprehensive parity coverage for source-based and serialized mounted packages, including runtime behavior, diagnostics, imports, and linking. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…sumption Signed-off-by: Antonio Sarosi <[email protected]>
bfaf132
into
antonio/s6a-type-consumption
BEP-066 slice-6a stack, PR 4 of 5 — chained on #4329. Mounted-blob packages become callable: TIR's loc-free
MemberResolution::Externalflows through MIR to B-693 unit symbols, and a two-DB compile-then-link harness proves it end to end at runtime.What
Externalcallees to fq-name unit symbols; impl-method{iface}$for${target}symbols reconstructed structurally with source/blob agreement.$streampolicy landed + tested.Gates
Focused suite 16/16 · tir + project + bytecode_cache green · full baml_tests green · strict all-features clippy clean on every touched crate · fmt clean · zero snapshot churn. (
--no-verifyonly for the known environmental mise/swift-protobuf hook failure; gates run directly.)Implemented by a Codex (gpt-5.6-sol) worker under stack-manager review.
Summary by CodeRabbit
New Features
Bug Fixes
Tests