Repository navigation
feat(tir): enrich PackageInterface export schema for source-less deps (BEP-066 slice 6a, PR 1) - #4326
Closed
antoniosarosi wants to merge 4 commits into
Closed
antoniosarosi wants to merge 4 commits into
antoniosarosi wants to merge 4 commits into
Conversation
… (BEP-066 slice 6a, PR 1) Grow the loc-free export schema and its derivation so a dependency's compiled surface can later be consumed as a serialized blob with no source present. Derivation only: nothing reads the new rows yet, and today's resolution paths explicitly treat the new interface rows as absent, keeping diagnostics and bytecode byte-identical (full baml_tests suite green, snapshots unchanged). Schema growth (package_interface.rs): - ExportedType::Interface: qtn, symbolic Self param, declared generic params + per-param bounds, the transitive requires closure pre-flattened and pre-realized at identity args with symbolic Self, associated types (realized bound + symbolic-Self default), fields with @alias/@description attrs, required AND default methods with symbolic-Self signatures. - ExportedFunction: generic_param_bounds parallel to generic_params, interface_target (implements-block methods, resolved through the constraint-head lowering), callable_fqn (dotted pkg.ns.[Owner.]name, the ItemRef Display rendering). - ExportedType::Class: field attrs (new ExportedFieldAttrs) and per-param generic_param_bounds. - PackageInterface.namespaces: the FULL namespace-path set from package_items (BTreeSet, borsh-stable). Derivation reuses the interfaces.rs substrate verbatim: the requires closure rides interface_closure_locs_with_args_and_assoc rooted at identity args with fill_associated_defaults=false (the rigid-Self rule), assoc defaults come from interface_associated_type_default, assoc bounds from associated_type_declared_bound, fields from resolve_interface_fields, required methods from resolve_interface_required_methods, and default methods from a new resolve_interface_default_methods twin (shared spine extracted, byte-identical for the existing query). Interface field @alias/@description currently live on the field's outer TypeRef (the class-side AST hoist does not run for interfaces), so the export reads both homes. Tests: new baml_tests compiler2_tir::package_interface suite covering the fixture assertions, borsh round-trip, two-database byte determinism, and stdlib derivation including the 'Iterator requires Iterable<Item = Self.Item, Error = Self.Error>' pinning idiom. B-694 seeding suites and LSP action suites stay green.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Contributor
📝 WalkthroughWalkthroughThe compiler now supports source-less mounted packages. It serializes interfaces, types, implementations, bounds, and callable metadata. Consumers resolve mounted declarations, link dependency units, lower external calls, and validate runtime and diagnostic parity. ChangesMounted package interfaces
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant LibraryCompiler
participant PackageInterface
participant ConsumerDatabase
participant ConsumerCompiler
participant Runtime
LibraryCompiler->>PackageInterface: derive and serialize exported metadata
PackageInterface->>ConsumerDatabase: mount serialized package interface
ConsumerDatabase->>ConsumerCompiler: resolve mounted types and callables
ConsumerCompiler->>Runtime: link dependency units and emit consumer program
Runtime-->>ConsumerCompiler: execute mounted functions and dispatch methods
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
baml_language/crates/baml_tests/src/compiler2_tir/package_interface.rs (1)
340-361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the interface-row resolution guards.
The central behavior claim of this PR is that exported interface rows stay invisible to resolution.
package_interface.rsimplements that with two!matches!(exported, ExportedType::Interface { .. })guards, inresolve_typeand inresolve_type_in_own_then_deps. No test here exercises either guard. A future change that removes a guard would pass this suite.Add a test that references a dependency-package interface by name from user code and asserts the reference does not resolve. The stdlib
baml.iter.Iteratoris a ready target.🤖 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/src/compiler2_tir/package_interface.rs` around lines 340 - 361, Add a test near namespace_set_is_complete that references the dependency-package interface baml.iter.Iterator from user code and asserts resolution fails or remains unresolved. Ensure the test exercises both resolve_type and resolve_type_in_own_then_deps paths where applicable, preserving the expectation that exported interface rows are excluded from type resolution.
🤖 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.rs`:
- Around line 619-629: Update the generic-bound lowering at
baml_language/crates/baml_compiler2_tir/src/interfaces.rs:619-629 and
requirement generic-argument lowering at
baml_language/crates/baml_compiler2_tir/src/interfaces.rs:1411-1420 to pass the
declaration scope’s symbolic Self context, including its interface constraint,
instead of constructing ScopeCtx with self_ty: None; ensure both lowerers
preserve Self and associated-type expressions as symbolic export metadata before
materializing the requirement closure.
---
Nitpick comments:
In `@baml_language/crates/baml_tests/src/compiler2_tir/package_interface.rs`:
- Around line 340-361: Add a test near namespace_set_is_complete that references
the dependency-package interface baml.iter.Iterator from user code and asserts
resolution fails or remains unresolved. Ensure the test exercises both
resolve_type and resolve_type_in_own_then_deps paths where applicable,
preserving the expectation that exported interface rows are excluded from type
resolution.
🪄 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: 8210afd8-f1d9-4e53-9154-4573e8812f50
📒 Files selected for processing (6)
baml_language/crates/baml_cli/src/bytecode_cache.rsbaml_language/crates/baml_compiler2_tir/src/interfaces.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_project/src/param_schema.rsbaml_language/crates/baml_tests/src/compiler2_tir/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/package_interface.rs
Binary size checks passed✅ 7 passed
Generated by |
…(PR #4326 review) CodeRabbit finding 1(b) verified real: the requires-closure walker (interface_closure_locs_with_args_and_assoc) lowered a requirement's generic ARGUMENTS through a ScopeCtx with no Self in scope, so 'requires Parent<Self>' / 'requires Parent<Self.Item>' silently produced Ty::Error arguments — in the checker's own closure and therefore in the exported requires rows. Fixed by threading the requiring interface's rigid Self (bounded by its realized constraint, self_bound) through the argument lowering, exactly mirroring the adjacent associated-type-binding lowering: a member pinned at the walk root collapses to its witness, an unpinned one stays a symbolic projection. Checker and export share the walker, so blob/source parity is preserved by construction; the pre-existing E0132 coherence tests that exercise this exact shape (interfaces_associated_types.rs 'Routed requires Bucket<Self.Item, ...>') are unchanged. Finding 1(a) verified NOT real: interface-level param bounds lower via interface_generic_param_bounds -> lower_env_interface_bounds, whose scope gets a symbolic self_ty from env_concrete_lowering_scope; and method-level bounds resolve Self through the generic-params route (InterfaceDeclScope.generics = env.source_params(), whose first entry IS the Self param — the Self special-case in lower_type_expr falls through to ordinary type-var resolution when self_ty is None). Both shapes already exported Parent<TypeVar(Self)>; now pinned by test. Also adds the missing guard test (finding 2): dependency interface rows must stay invisible to both PackageResolutionContext::resolve_type paths (package-prefixed and own-then-deps) while a dep class in the same namespace still resolves — proving the ExportedType::Interface guards preserve pre-export resolution. Gate: baml_tests lib 1493 passed / 0 failed, full suite exit 0, no snapshot churn; baml_compiler2_tir 324 passed; clippy/fmt clean. (--no-verify: pre-commit cargo hooks fail on the environmental mise config-trust error in this worktree; real fmt/clippy verified clean.)
Contributor
Author
|
Review round addressed in 0e461fd:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_tests/src/compiler2_tir/package_interface.rs (1)
606-611: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the contrast comment wording.
The comment states that the dependency class "resolves through the interface rows". The class resolves through its own
ExportedType::Classrow. The interface guard only skipsExportedType::Interfacerows.📝 Proposed comment fix
- // Contrast: a dep CLASS in the same namespace still resolves through the - // interface rows on both paths — the guard is interface-specific. + // Contrast: a dep CLASS in the same namespace still resolves on both + // paths — the guard skips only `ExportedType::Interface` rows.🤖 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/src/compiler2_tir/package_interface.rs` around lines 606 - 611, Update the contrast comment above the dependency class resolution assertion to state that the class resolves through its own ExportedType::Class row, while the guard only skips ExportedType::Interface rows. Leave the resolve_type test logic 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.
Nitpick comments:
In `@baml_language/crates/baml_tests/src/compiler2_tir/package_interface.rs`:
- Around line 606-611: Update the contrast comment above the dependency class
resolution assertion to state that the class resolves through its own
ExportedType::Class row, while the guard only skips ExportedType::Interface
rows. Leave the resolve_type test logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76287e0b-d667-4658-ac35-7a2b8db49642
📒 Files selected for processing (2)
baml_language/crates/baml_compiler2_tir/src/interfaces.rsbaml_language/crates/baml_tests/src/compiler2_tir/package_interface.rs
This was referenced Aug 6, 2026
…6a, PR 2) (#4327) **BEP-066 slice-6a stack, PR 2 of 5** — chained on #4326. Derivation only, zero behavior change (`impls` is written, never read; consumers land in PR 3/4). ## What `PackageInterface.impls: Vec<ExportedImpl>` — everything a source-less consumer needs for impl matching and conformance without `ImplLoc`s: interface head (args + realized assoc bindings), `for_ty_pattern` as `Ty` over the impl's rigid `ParamTy`s, generic params + bounds, assoc rows in declaration order, field links, origin (doc-carried as diagnostic-metadata-only), and per-method rows with **structural identity** (owner = the row's interface+for-ty pair; MIR's source-text-derived `{iface}$for${target}` naming deliberately not replicated — the consumer PR reconstructs it structurally). ## Key choices - **Impl-method signatures** lower exactly as `validate_impl_signatures` lowers the impl side (`InterfaceMethodSpec` spine + `realize_with_symbolic_self`, `Self → for_ty` substituted last) — NOT `function_signature_ty`, which lowers free-impl methods without a `Self` binding (its own documented deficiency). - **Ordering**: rows sort by their borsh encoding — canonical, file-order-independent. - **Malformed impls**: skipped exactly as existing consumers filter `ImplDataError`; diagnostics stay owned by `impl_data`. - **No spans** (Salsa span-free rule): verified coherence needs none structurally — dep-side attribution renders structurally; noted for the consumer PR. ## Proof - tir: 324 ✓ · full baml_tests: 2511 ✓, **zero snapshot churn** · bytecode_cache (B-694): 43 ✓ · new impl suite: 8 tests incl. generic-impl bounds, assoc pins + filled defaults, blanket impl, and the stdlib `implement Equals for int` spot-check (`other: Self` → `int`). - Only non-tir change: `realize_with_symbolic_self` visibility (`pub(crate)`) + a test-sentinel literal. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Package interfaces now export comprehensive implementation metadata, including interface relationships, generics, associated types, field links, origins, and method signatures. * Exported implementation data is serialized consistently and included when packages are combined. * **Bug Fixes** * Improved handling of implementation metadata across in-body, out-of-body, generic, and standard-library implementations. * **Tests** * Added extensive coverage for implementation exports, type resolution, associated types, method metadata, serialization, completeness, and deterministic output. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Antonio Sarosi <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (10)
baml_language/crates/baml_compiler2_hir/src/package.rs (2)
471-487: 🚀 Performance & Scalability | 🔵 TrivialNote the new whole-project scan inside
package_dependencies.The
userarm now readscompiler2_all_files(db)and callsfile_packagefor every file.package_dependencieswas previously a name-only match with no tracked inputs. It is now invalidated by any change to the project file set, and it re-executes O(files) work per revision.package_dependency_closurecalls it once per package, so the scan repeats per closure walk.Salsa backdating limits downstream recomputation while the derived package-name set is stable, so this is a cost concern rather than a correctness one. If the LSP path shows regression, extract the auxiliary source-package set into its own memoized query and read that from both arms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_hir/src/package.rs` around lines 471 - 487, Reduce the whole-project scan introduced in the user arm of package_dependencies by extracting the auxiliary source-package set into a separate memoized Salsa query. Have package_dependencies read that query, and reuse it from both relevant arms while preserving the existing filtering and dependency behavior.
371-386: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the stdlib part of
RESERVED_PACKAGE_NAMESfrombaml_builtins2.The constant hardcodes
baml,boundary,testing,assert,log, andreflect.baml_builtins2::stdlib_package_names()already publishes that same set, and this PR's tests iterate it. If a stdlib package is added there and not here, a mounted blob can claim its name, become a dependency of every user package, and shadow the stdlib arm below. The doc comment states the lockstep requirement, but nothing enforces it.Consider composing the reserved check from
baml_builtins2::stdlib_package_names()plus the three non-stdlib names (user,root,env), or add a unit test asserting that every name instdlib_package_names()is reserved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_hir/src/package.rs` around lines 371 - 386, Update RESERVED_PACKAGE_NAMES and is_reserved_package_name to derive the stdlib names from baml_builtins2::stdlib_package_names() rather than duplicating baml, boundary, testing, assert, log, and reflect. Preserve the additional non-stdlib reservations user, root, and env, and ensure every name returned by stdlib_package_names() is rejected by the reserved-name check.baml_language/crates/baml_tests/tests/mounted_package_calls.rs (1)
145-177: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBoth mounted-package harnesses recompile the library once per test. Neither helper memoizes its artifacts, so each test rebuilds a
ProjectDatabase, compiles the stdlib plus the fixture, derives thePackageInterface, and emitsCompilationUnits. Across both files that is roughly twenty identical library compiles per suite run.
baml_language/crates/baml_tests/tests/mounted_package_calls.rs#L145-L177: move the body ofcompile_libraryinto a private builder and return a clone from astd::sync::LazyLockholding the(blob, units)pair.baml_language/crates/baml_tests/tests/mounted_package_parity.rs#L168-L185: apply the same treatment tolibrary_artifacts, caching theLibraryArtifactsvalue and cloningblob,interface, andunitsper test.🤖 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 145 - 177, The mounted-package test harnesses recompile identical library artifacts for every test. In baml_language/crates/baml_tests/tests/mounted_package_calls.rs lines 145-177, move compile_library’s database/build logic into a private builder, cache its (blob, units) result in a std::sync::LazyLock, and have compile_library return clones. Apply the same pattern in baml_language/crates/baml_tests/tests/mounted_package_parity.rs lines 168-185 to cache library_artifacts and clone its blob, interface, and units for each test.baml_language/crates/baml_compiler2_tir/src/package_interface.rs (1)
1073-1081: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider surfacing a corrupt mounted blob.
A corrupt or version-mismatched blob falls through silently. Derivation is then empty for a file-less package, so the user sees a cascade of unresolved-name errors with no indication that the mount failed to decode. A
tracing::warn!with the mount alias would make this diagnosable without changing behavior.🤖 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/package_interface.rs` around lines 1073 - 1081, Update the mounted-package decoding path around PackageInterface to emit a tracing::warn! when borsh::from_slice fails for a blob returned by mounted.by_package, including the mount alias or package name in the message. Preserve the existing fallback behavior and successful return path.baml_language/crates/baml_compiler2_tir/src/interfaces.rs (1)
682-698: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the arity guard the sibling mounted oracle already has.
bind_type_vars(generic_params, args)is called without checking that the two lengths agree. The mounted arm ofassociated_type_declared_boundinbaml_language/crates/baml_compiler2_tir/src/builder/associated_projection.rsguards exactly this case and returns empty on a mismatch, and the source path below guards it through the interface environment. On a mismatch here, the realized default keeps unbound rigidTypeVars instead of returningNone.🛡️ Proposed parity fix
}) = crate::package_interface::mounted_type_row(db, qtn) { + if generic_params.len() != args.len() { + return None; + } let assoc = associated_types.iter().find(|a| &a.name == member)?;🤖 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.rs` around lines 682 - 698, Add an arity check in the mounted interface branch before calling realize_associated_default: ensure generic_params and args have equal lengths, returning None on mismatch. Keep the existing associated-type lookup and realization unchanged when the lengths agree, matching the guard used by associated_type_declared_bound.baml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rs (1)
2393-2413: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
package_implsallocates on a recursive hot path.
package_implsbuilds a freshVecper call.get_implements_block_within_depthrecurses toBLANKET_IMPL_BOUND_DEPTH(16) and re-enumerates every package in the dependency closure at each level, so the allocation multiplies with depth and closure size.package_impl_locsandmounted_impl_datasare bothreturns(ref), so the two underlying tables are already memoized; only the joiningVecis new.Consider returning an iterator that chains the two memoized slices, or making the joined table a tracked query.
🤖 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 2393 - 2413, Update package_impls to avoid allocating a new Vec on each call by returning an iterator over the memoized package_impl_locs and mounted_impl_datas results, chaining both sources while preserving ImplRef construction and invalid-entry filtering. Update callers such as get_implements_block_within_depth to consume the iterator without requiring an owned collection.baml_language/crates/baml_compiler2_tir/src/builder.rs (1)
10656-10670: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd in-crate unit coverage for the mounted path resolver.
The new mounted resolution logic is covered only from the
baml_testsintegration crate. The coding guidelines prefer Rust unit tests where possible. The pure parts of this function are testable in-crate: thedottedpath formatting, thens/itemsplit, and the enum-variant and UFCS split loops over a hand-builtPackageInterface.Add a
#[cfg(test)]module in this crate for those parts, and keep the end-to-end cases inbaml_tests.Based on learnings, "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_compiler2_tir/src/builder.rs` around lines 10656 - 10670, Add a #[cfg(test)] module in the compiler crate covering the pure behavior of resolve_mounted_package_item: dotted path formatting, ns/item splitting, and enum-variant and UFCS split loops using a hand-built PackageInterface. Keep end-to-end mounted-resolution coverage in baml_tests and avoid changing production behavior.Source: Coding guidelines
baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs (2)
1955-2026: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert that the realized args match the declared interface params.
Line 1955 and Line 2018 both pair
iface_generic_paramswithrealized.generics.ziptruncates silently when the two lengths differ. A shortowner_type_arg_binding_seedproduces a frame the callee's templates cannot fill, and that failure surfaces at run time rather than here.Add a debug assertion so a row/receiver arity mismatch fails in tests.
🛡️ Proposed guard
+ debug_assert_eq!( + iface_generic_params.len(), + realized.generics.len(), + "exported interface `{iface_qtn:?}` declares {} generic params but the realized \ + receiver supplies {}", + iface_generic_params.len(), + realized.generics.len(), + ); let mut bindings = crate::generics::bind_type_vars(iface_generic_params, &realized.generics);🤖 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 1955 - 2026, In the interface method resolution flow that builds bindings and owner_type_arg_binding_seed, add a debug assertion that iface_generic_params.len() equals realized.generics.len() before either zip-based pairing occurs. Keep the existing binding and seed construction unchanged, while ensuring row/receiver arity mismatches fail immediately in debug and test builds.
2034-2089: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd in-crate unit tests for the pure
Self-walking helpers.
mentions_bare_selfandself_nested_in_invariant_positiontake only aTyand aParamTy. They need no database, so they are testable with a#[cfg(test)]module in this file. Cover the bare receiver, the projection base, a nested invariant constructor, and a union arm.As per 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_compiler2_tir/src/builder/interface_resolution.rs` around lines 2034 - 2089, Add a #[cfg(test)] module in the same file with unit tests for mentions_bare_self and self_nested_in_invariant_position, constructing Ty and ParamTy values without database dependencies. Cover bare Self, an associated-type projection base, Self nested in an invariant constructor, and a union containing Self, asserting the expected results for both helpers.Source: Coding guidelines
baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
5825-5860: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated path-receiver lowering before
MakeBoundMethod. Both new mounted arms copy the receiver-lowering block from the existingBoundMethodarm, so the same "single-segment root throughplace_for_path, otherwise a field chain throughlower_multi_segment_path_as_field_chain" logic now exists three times in this file. The shared root cause is the absence of one helper for that step.
baml_language/crates/baml_compiler2_mir/src/lower.rs#L5825-L5860: replace the inlined block with a call to a new helper, for examplefn emit_bound_method_for_path(&mut self, expr_id, segments, resolution, dest) -> bool.baml_language/crates/baml_compiler2_mir/src/lower.rs#L5943-L5979: call the same helper here, and route the untouchedBoundMethodarm at Lines 5779-5806 through it as well.🤖 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 5825 - 5860, Extract the duplicated receiver-lowering and MakeBoundMethod construction into a shared helper such as emit_bound_method_for_path, returning whether emission succeeded. Update baml_language/crates/baml_compiler2_mir/src/lower.rs:5825-5860 and 5943-5979 to call it, and route the existing BoundMethod arm at 5779-5806 through the same helper; preserve the single-segment place_for_path and multi-segment field-chain 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_emit/src/lib.rs`:
- Around line 558-565: Update merge_defaults and its caller build_packages so
mounted default methods are resolved using the spliced base package/object
indices, rather than caller-local function_indices. When populating
iface_defaults for default_methods, resolve method.callable_fqn through the
linked package indices and preserve the resolved callable FQN for inherited
mounted-interface defaults.
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 855-882: Replace the assert_eq! in the structural impl symbol
handling within the ItemRef::Method path with debug_assert_eq!, preserving the
existing owner fallback and diagnostic message while preventing symbol-spelling
mismatches from aborting release builds.
- Around line 3332-3344: Guard recursion in mir_interface_declares_method when
traversing mounted ExportedType::Interface requires entries by adding a visited
set or bounded traversal. Ensure self-referential and mutually referential
requires rows terminate while still checking the root interface methods and
valid required interfaces; reuse interface_declares_method_directly if the
requires rows are guaranteed to be pre-flattened.
- Around line 11258-11268: Update the parent-view construction in
interface_closure_type_name_views to substitute or propagate the caller’s
iface_assoc bindings into each realized required interface view, rather than
retaining only required.associated_types after generic substitution. Ensure
views selected by interface_view_declaring_field and passed through
virtual_field_wire_target retain pinned associated types; alternatively restrict
this branch to method-narrowing callers if that is the intended scope.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 12865-12882: Update lookup_class_method around the
ExportedType::Class methods lookup to count all entries matching method_name
instead of selecting the first with methods.iter().find. Return the
corresponding ambiguity or inherent-error ClassMethodLookup variants for
duplicate mounted methods, and when mounted impl resolution is unavailable
return the concrete reserved result rather than treating an arbitrary match as
successful; preserve generic binding and substitution for the unique-match path.
- Around line 10180-10188: Remove report_reserved_mounted_call calls from bare
path resolution in infer_path, including local and package/namespace resolution
branches. Invoke the diagnostic only from the Expr::Call and Expr::OptionalCall
handling arms, while preserving foreign_callable_exprs behavior so referenced
functions retain their exported signature types.
In `@baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs`:
- Around line 501-522: Align mounted declarer discovery with
resolve_member_on_mounted_interface: in the helper containing the
required/default method check, include exported fields when determining whether
the mounted root declares member, and fail closed when it does. In the
requires-parent lookup, accept any declaration kind returned by
interface_member_kind instead of restricting the source arm to
UnionMemberKind::Method, matching the single-bound path’s behavior.
- Around line 2038-2078: Update self_nested_in_invariant_position to inspect
associated type projections before delegating to mentions_bare_self: handle
non-TypeVar projection bases through the exempt association path, while
retaining invariant-container handling for other types. Ensure projections such
as Box<Self>.Item are not classified as Self nested in an invariant position.
In `@baml_language/crates/baml_compiler2_tir/src/inference.rs`:
- Around line 226-240: Update the mounted-interface branch in
interface_associated_type_names_for_qtn to collect each required row’s
associated-type names directly from its exported interface data instead of
recursively calling the same helper. Preserve the current names from
associated_types and the one-level requires behavior, ensuring cycles cannot
cause unbounded recursion.
In `@baml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rs`:
- Around line 2148-2212: Filter iface_method.generic_params through
baml_type::is_synthetic_effect_param to derive the user-declared generic
parameter list, matching ExternalCallable::user_generic_params. Use this
filtered list for the arity comparison and iface_method_bindings, and align
iface_method_bounds with the same filtered parameters so synthetic
callback-effect parameters do not affect signature validation or bound
entailment.
In `@baml_language/crates/baml_compiler2_tir/src/package_interface.rs`:
- Around line 1209-1247: Guard the generic-parameter slicing in the surrounding
package-interface derivation before using impl_param_count or declared_count. If
scope_generics is shorter than impl_param_count, or own_params is shorter than
declared_count, stop this row’s derivation and return the same None result used
for invariant breaks later in the function, rather than slicing and panicking.
Preserve the existing bounds-lowering behavior when both counts are within
range.
In `@baml_language/crates/baml_project/src/db.rs`:
- Around line 431-448: Update add_compiler2_virtual_file to detect an existing
path in compiler2_file_map before creating a new SourceFile; return the existing
handle for repeated paths and leave compiler2_extra_files unchanged, while
preserving the current insertion and list-update behavior for new paths.
In `@baml_language/crates/baml_tests/src/compiler2_tir/package_interface.rs`:
- Around line 1233-1275: Update both tests,
user_impl_of_mounted_interface_missing_method_is_rejected and
user_impl_of_mounted_interface_signature_mismatch_is_rejected, to assert the
expected diagnostic codes E0113 and E0120 respectively through the rendered
error message. Keep the existing member-name checks if useful, but ensure each
test independently fails when mounted interface resolution produces a different
error.
---
Nitpick comments:
In `@baml_language/crates/baml_compiler2_hir/src/package.rs`:
- Around line 471-487: Reduce the whole-project scan introduced in the user arm
of package_dependencies by extracting the auxiliary source-package set into a
separate memoized Salsa query. Have package_dependencies read that query, and
reuse it from both relevant arms while preserving the existing filtering and
dependency behavior.
- Around line 371-386: Update RESERVED_PACKAGE_NAMES and
is_reserved_package_name to derive the stdlib names from
baml_builtins2::stdlib_package_names() rather than duplicating baml, boundary,
testing, assert, log, and reflect. Preserve the additional non-stdlib
reservations user, root, and env, and ensure every name returned by
stdlib_package_names() is rejected by the reserved-name check.
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 5825-5860: Extract the duplicated receiver-lowering and
MakeBoundMethod construction into a shared helper such as
emit_bound_method_for_path, returning whether emission succeeded. Update
baml_language/crates/baml_compiler2_mir/src/lower.rs:5825-5860 and 5943-5979 to
call it, and route the existing BoundMethod arm at 5779-5806 through the same
helper; preserve the single-segment place_for_path and multi-segment field-chain
behavior.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 10656-10670: Add a #[cfg(test)] module in the compiler crate
covering the pure behavior of resolve_mounted_package_item: dotted path
formatting, ns/item splitting, and enum-variant and UFCS split loops using a
hand-built PackageInterface. Keep end-to-end mounted-resolution coverage in
baml_tests and avoid changing production behavior.
In `@baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs`:
- Around line 1955-2026: In the interface method resolution flow that builds
bindings and owner_type_arg_binding_seed, add a debug assertion that
iface_generic_params.len() equals realized.generics.len() before either
zip-based pairing occurs. Keep the existing binding and seed construction
unchanged, while ensuring row/receiver arity mismatches fail immediately in
debug and test builds.
- Around line 2034-2089: Add a #[cfg(test)] module in the same file with unit
tests for mentions_bare_self and self_nested_in_invariant_position, constructing
Ty and ParamTy values without database dependencies. Cover bare Self, an
associated-type projection base, Self nested in an invariant constructor, and a
union containing Self, asserting the expected results for both helpers.
In `@baml_language/crates/baml_compiler2_tir/src/interfaces.rs`:
- Around line 682-698: Add an arity check in the mounted interface branch before
calling realize_associated_default: ensure generic_params and args have equal
lengths, returning None on mismatch. Keep the existing associated-type lookup
and realization unchanged when the lengths agree, matching the guard used by
associated_type_declared_bound.
In `@baml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rs`:
- Around line 2393-2413: Update package_impls to avoid allocating a new Vec on
each call by returning an iterator over the memoized package_impl_locs and
mounted_impl_datas results, chaining both sources while preserving ImplRef
construction and invalid-entry filtering. Update callers such as
get_implements_block_within_depth to consume the iterator without requiring an
owned collection.
In `@baml_language/crates/baml_compiler2_tir/src/package_interface.rs`:
- Around line 1073-1081: Update the mounted-package decoding path around
PackageInterface to emit a tracing::warn! when borsh::from_slice fails for a
blob returned by mounted.by_package, including the mount alias or package name
in the message. Preserve the existing fallback behavior and successful return
path.
In `@baml_language/crates/baml_tests/tests/mounted_package_calls.rs`:
- Around line 145-177: The mounted-package test harnesses recompile identical
library artifacts for every test. In
baml_language/crates/baml_tests/tests/mounted_package_calls.rs lines 145-177,
move compile_library’s database/build logic into a private builder, cache its
(blob, units) result in a std::sync::LazyLock, and have compile_library return
clones. Apply the same pattern in
baml_language/crates/baml_tests/tests/mounted_package_parity.rs lines 168-185 to
cache library_artifacts and clone its blob, interface, and units for each test.
🪄 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: 3efd781c-7510-418c-90d8-eca698914a45
📒 Files selected for processing (27)
baml_language/crates/baml_cli/src/bytecode_cache.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/package.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/builder/associated_projection.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.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/coherence.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_compiler2_tir/src/unify.rsbaml_language/crates/baml_compiler_diagnostics/src/diagnostic.rsbaml_language/crates/baml_lsp2_actions/src/check.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_surface/src/export.rsbaml_language/crates/baml_surface/src/handles.rsbaml_language/crates/baml_tests/src/compiler2_tir/package_interface.rsbaml_language/crates/baml_tests/tests/interfaces.rsbaml_language/crates/baml_tests/tests/mounted_package_calls.rsbaml_language/crates/baml_tests/tests/mounted_package_parity.rsbaml_language/crates/baml_workspace/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/crates/baml_cli/src/bytecode_cache.rs
Comment on lines
+558
to
+565
| if !default_methods.is_empty() { | ||
| let entry = iface_defaults.entry(qtn.clone()).or_default(); | ||
| for method in default_methods { | ||
| entry | ||
| .entry(method.name.clone()) | ||
| .or_insert_with(|| method.callable_fqn.clone()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate generate_project_bytecode and every generate_impl call site to see which pass `base: None`.
set -euo pipefail
fd -t f 'lib.rs' baml_language/crates/baml_compiler2_emit -x rg -n -C 6 'fn generate_project_bytecode|generate_impl\s*\(' {}Repository: BoundaryML/baml
Length of output: 6380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== generate_impl signature/body outline =="
wc -l baml_language/crates/baml_compiler2_emit/src/lib.rs
sed -n '2690,3060p' baml_language/crates/baml_compiler2_emit/src/lib.rs | cat -n
echo "== resolve_fqn occurrences =="
rg -n -C 4 'fn resolve_fqn|resolve_fqn\(' baml_language/crates/baml_compiler2_emit/src/lib.rs
echo "== merge_defaults occurrences =="
rg -n -C 8 'merge_defaults|default_methods|iface_defaults|function_indices' baml_language/crates/baml_compiler2_emit/src/lib.rsRepository: BoundaryML/baml
Length of output: 38263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mounted package query definitions/usages =="
rg -n -C 5 'mounted_package_names|mounted|Mounted|dependency_units|ProjectDatabase|get_bytecode|with_stdlib|with_opt' .
echo "== tests for mounted/default methods =="
rg -n -C 8 'default.*method|mounted|dependency_units|generate_project_bytecode_with_mounted_units|generate_project_bytecode_with_opt\(' .Repository: BoundaryML/baml
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== focused references in compiler emit and Db implementations =="
rg -n -C 8 'mounted_package_names|generate_project_bytecode|generate_impl\(|generate_impl\s*\(' \
baml_language/crates/baml_compiler2_emit/src/lib.rs \
baml_language/crates/baml_compiler2_mir/src \
baml_language/crates/baml_compiler2_hir/src
echo "== focused mounted definitions =="
rg -n -C 8 'mounted_package_names|mounted' \
baml_language/crates/baml_compiler2_mir/src \
baml_language/crates/baml_compiler2_hir/src \
baml_language/crates/baml_tests/src/compiler2_tir/package_interface.rs | sed -n '1,260p'
echo "== focused generate helpers and ProgramBytecode db type =="
rg -n -C 6 'struct ProjectDatabase|impl.*Db|ProjectDatabase|get_bytecode|generate_bytecode' \
baml_language/crates/baml_compiler2_mir/src \
baml_language/crates/baml_tests/srcRepository: BoundaryML/baml
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== emit lib imports and early generate_impl passes =="
sed -n '1,130p' baml_language/crates/baml_compiler2_emit/src/lib.rs | cat -n
sed -n '2690,2855p' baml_language/crates/baml_compiler2_emit/src/lib.rs | cat -n
echo "== exact mounted prepass location relative to generate_impl branches =="
sed -n '480,570p' baml_language/crates/baml_compiler2_emit/src/lib.rs | cat -n
echo "== call sites in tested code paths =="
rg -n -C 5 'emit_units|generate_project_bytecode_with_mounted_units|generate_project_bytecode_with_reuse_artifacts|generate_project_bytecode_with_opt\(.*mounted_unit|mounted_unit' \
baml_language/crates/baml_tests/src/compiler2_tir/package_interface.rs \
baml_language/crates/baml_tests/src \
--glob '*.rs' | sed -n '1,260p'Repository: BoundaryML/baml
Length of output: 19392
Resolve mounted default calls through the linked package instead of their missing emitter FQN.
build_packages loads mounted packages from the linked base, so mounted default methods that inherit from mounted interfaces can exist in impl_rules. Let merge_defaults resolve those FQNs via the spliced base package/object indices rather than passing them into the caller-local function_indices lookup.
🤖 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/lib.rs` around lines 558 - 565,
Update merge_defaults and its caller build_packages so mounted default methods
are resolved using the spliced base package/object indices, rather than
caller-local function_indices. When populating iface_defaults for
default_methods, resolve method.callable_fqn through the linked package indices
and preserve the resolved callable FQN for inherited mounted-interface defaults.
Comment on lines
+855
to
+882
| let owner = baml_compiler2_tir::interfaces::impl_data(db, impl_loc) | ||
| .as_ref() | ||
| .ok() | ||
| .and_then(|data| { | ||
| let qtn = data.interface_qtn(db)?; | ||
| let interface = baml_type::Interface::new( | ||
| qtn, | ||
| data.interface_args.clone(), | ||
| data.associated_types.clone(), | ||
| ); | ||
| Some( | ||
| baml_compiler2_tir::package_interface::impl_method_symbol_owner( | ||
| &interface, | ||
| &data.for_ty_pattern, | ||
| &pkg_info.package, | ||
| &pkg_info.namespace_path, | ||
| ), | ||
| ) | ||
| }) | ||
| .unwrap_or_else(|| raw_owner.clone()); | ||
| assert_eq!( | ||
| owner, raw_owner, | ||
| "structural impl symbol must preserve the source-era B-693 spelling" | ||
| ); | ||
| return ItemRef::Method { | ||
| package: pkg_info.package.clone(), | ||
| namespace: pkg_info.namespace_path, | ||
| class: Name::new(format!( | ||
| "{}$for${}", | ||
| block.type_refs.display(block.interface_target), | ||
| block.type_refs.display(*for_target) | ||
| )), | ||
| class: owner, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not abort the compiler on a symbol-spelling mismatch.
Line 875 uses assert_eq!, so it stays active in release builds. If impl_method_symbol_owner ever returns a different spelling than the structural raw_owner, the compiler panics instead of emitting a diagnostic. Line 874 already falls back to raw_owner, so the assertion only fires on a genuine divergence between the two derivations — the case that most needs graceful handling.
Use debug_assert_eq! so the invariant is checked in tests and does not crash a user build.
🛡️ Proposed change
- assert_eq!(
+ debug_assert_eq!(
owner, raw_owner,
"structural impl symbol must preserve the source-era B-693 spelling"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let owner = baml_compiler2_tir::interfaces::impl_data(db, impl_loc) | |
| .as_ref() | |
| .ok() | |
| .and_then(|data| { | |
| let qtn = data.interface_qtn(db)?; | |
| let interface = baml_type::Interface::new( | |
| qtn, | |
| data.interface_args.clone(), | |
| data.associated_types.clone(), | |
| ); | |
| Some( | |
| baml_compiler2_tir::package_interface::impl_method_symbol_owner( | |
| &interface, | |
| &data.for_ty_pattern, | |
| &pkg_info.package, | |
| &pkg_info.namespace_path, | |
| ), | |
| ) | |
| }) | |
| .unwrap_or_else(|| raw_owner.clone()); | |
| assert_eq!( | |
| owner, raw_owner, | |
| "structural impl symbol must preserve the source-era B-693 spelling" | |
| ); | |
| return ItemRef::Method { | |
| package: pkg_info.package.clone(), | |
| namespace: pkg_info.namespace_path, | |
| class: Name::new(format!( | |
| "{}$for${}", | |
| block.type_refs.display(block.interface_target), | |
| block.type_refs.display(*for_target) | |
| )), | |
| class: owner, | |
| let owner = baml_compiler2_tir::interfaces::impl_data(db, impl_loc) | |
| .as_ref() | |
| .ok() | |
| .and_then(|data| { | |
| let qtn = data.interface_qtn(db)?; | |
| let interface = baml_type::Interface::new( | |
| qtn, | |
| data.interface_args.clone(), | |
| data.associated_types.clone(), | |
| ); | |
| Some( | |
| baml_compiler2_tir::package_interface::impl_method_symbol_owner( | |
| &interface, | |
| &data.for_ty_pattern, | |
| &pkg_info.package, | |
| &pkg_info.namespace_path, | |
| ), | |
| ) | |
| }) | |
| .unwrap_or_else(|| raw_owner.clone()); | |
| debug_assert_eq!( | |
| owner, raw_owner, | |
| "structural impl symbol must preserve the source-era B-693 spelling" | |
| ); | |
| return ItemRef::Method { | |
| package: pkg_info.package.clone(), | |
| namespace: pkg_info.namespace_path, | |
| class: owner, |
🤖 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 855 - 882,
Replace the assert_eq! in the structural impl symbol handling within the
ItemRef::Method path with debug_assert_eq!, preserving the existing owner
fallback and diagnostic message while preventing symbol-spelling mismatches from
aborting release builds.
Comment on lines
+3332
to
+3344
| if let Some(baml_compiler2_tir::package_interface::ExportedType::Interface { | ||
| required_methods, | ||
| default_methods, | ||
| requires, | ||
| .. | ||
| }) = baml_compiler2_tir::package_interface::mounted_type_row(self.db, iface_qtn) | ||
| { | ||
| return required_methods.iter().any(|m| m.name == *method) | ||
| || default_methods.iter().any(|m| m.name == *method) | ||
| || requires | ||
| .iter() | ||
| .any(|req| self.mir_interface_declares_method(&req.name, method)); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the mounted requires recursion against cycles.
This function recurses through requires rows with no visited set. The comment relies on the rows being a pre-flattened transitive closure. The rows come from a deserialized interface blob, so a self-referential or mutually-referential requires entry causes unbounded recursion and a stack overflow in the compiler.
Add a depth or visited-set bound, or check only the root row's own methods plus one non-recursive pass over requires if the closure really is pre-flattened.
🛡️ Non-recursive alternative when `requires` is pre-flattened
return required_methods.iter().any(|m| m.name == *method)
|| default_methods.iter().any(|m| m.name == *method)
|| requires
.iter()
- .any(|req| self.mir_interface_declares_method(&req.name, method));
+ .any(|req| self.interface_declares_method_directly(&req.name, method));interface_declares_method_directly already answers both source and mounted rows without recursing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Some(baml_compiler2_tir::package_interface::ExportedType::Interface { | |
| required_methods, | |
| default_methods, | |
| requires, | |
| .. | |
| }) = baml_compiler2_tir::package_interface::mounted_type_row(self.db, iface_qtn) | |
| { | |
| return required_methods.iter().any(|m| m.name == *method) | |
| || default_methods.iter().any(|m| m.name == *method) | |
| || requires | |
| .iter() | |
| .any(|req| self.mir_interface_declares_method(&req.name, method)); | |
| } | |
| if let Some(baml_compiler2_tir::package_interface::ExportedType::Interface { | |
| required_methods, | |
| default_methods, | |
| requires, | |
| .. | |
| }) = baml_compiler2_tir::package_interface::mounted_type_row(self.db, iface_qtn) | |
| { | |
| return required_methods.iter().any(|m| m.name == *method) | |
| || default_methods.iter().any(|m| m.name == *method) | |
| || requires | |
| .iter() | |
| .any(|req| self.interface_declares_method_directly(&req.name, method)); | |
| } |
🤖 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 3332 -
3344, Guard recursion in mir_interface_declares_method when traversing mounted
ExportedType::Interface requires entries by adding a visited set or bounded
traversal. Ensure self-referential and mutually referential requires rows
terminate while still checking the root interface methods and valid required
interfaces; reuse interface_declares_method_directly if the requires rows are
guaranteed to be pre-flattened.
Comment on lines
+11258
to
+11268
| let mut views = vec![( | ||
| iface_tn.clone(), | ||
| iface_type_args.to_vec(), | ||
| iface_assoc.to_vec(), | ||
| )]; | ||
| for required in requires { | ||
| let realized = required | ||
| .map_tys(|ty| baml_compiler2_tir::generics::substitute_ty(ty, &bindings)); | ||
| views.push((realized.name, realized.generics, realized.associated_types)); | ||
| } | ||
| return Some(views); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace which callers of interface_closure_type_name_views consume args/assoc rather than the name.
set -euo pipefail
rg -nP -C 8 'interface_closure_type_name_views' --type=rust baml_language/crates/baml_compiler2_mir/src/lower.rs
rg -nP -C 12 'fn virtual_field_wire_target|fn interface_view_declaring_field' --type=rust baml_language/crates/baml_compiler2_mir/src/lower.rsRepository: BoundaryML/baml
Length of output: 8398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== interface_closure_type_name_views implementation =="
sed -n '11230,11305p' baml_language/crates/baml_compiler2_mir/src/lower.rs
echo "== tir2_interface_to_template definition =="
rg -n "fn tir2_interface_to_template|struct TyTemplateInterface|TyTemplateInterface" baml_language/crates/baml_compiler2_mir/src lower.rs --type-rust 2>/dev/null | head -80 || true
fd -i 'lower.rs|template.*rs|.*template.*' baml_language/crates/baml_compiler2_mir/src -t f | while read -r f; do
if rg -q "tir2_interface_to_template|TyTemplateInterface" "$f"; then
echo "--- $f"
rg -n -C 8 "tir2_interface_to_template|TyTemplateInterface" "$f"
fi
done
echo "== callers passing assoc into interface_closure_type_name_views =="
rg -n "interface_closure_type_name_views\\(" baml_language/crates/baml_compiler2_mir/src/lower.rs
echo "== resolved interface implementation for l1_impls_for_recv and substitute_class_params =="
sed -n '1660,1760p' baml_language/crates/baml_compiler2_mir/src/lower.rs
sed -n '11258,11292p' baml_language/crates/baml_compiler2_mir/src/lower.rsRepository: BoundaryML/baml
Length of output: 18054
Substitute requested associated bindings into parent interface views.
For mounted interfaces, interface_closure_type_name_views keeps the caller’s iface_assoc on the root view, but parent views come from required.associated_types after only substituting generic args. interface_view_declaring_field can select those parent views, and virtual_field_wire_target carries their assoc bindings to TyTemplateInterface; the runtime therefore resolves against an unpinned associated type. Propagate iface_assoc into each parent view, or limit this branch to the method-narrowing callers.
🤖 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 11258 -
11268, Update the parent-view construction in interface_closure_type_name_views
to substitute or propagate the caller’s iface_assoc bindings into each realized
required interface view, rather than retaining only required.associated_types
after generic substitution. Ensure views selected by
interface_view_declaring_field and passed through virtual_field_wire_target
retain pinned associated types; alternatively restrict this branch to
method-narrowing callers if that is the intended scope.
Comment on lines
+10180
to
+10188
| let ty = self.infer_local_rooted_path(segments, expr_id, true); | ||
| self.report_reserved_mounted_call(expr_id, expr_id); | ||
| ty | ||
| } else { | ||
| // Root is not a known local. Try full package/namespace resolution: | ||
| // 1. Package path (e.g. baml.llm.ClientType.Primitive, baml.env.get) | ||
| let pkg_ty = self.infer_multi_segment_path(segments, expr_id); | ||
| if !matches!(pkg_ty, Ty::Unknown { .. }) { | ||
| self.report_reserved_mounted_call(expr_id, expr_id); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find coverage for MountedPackageCallUnsupported on references vs calls.
set -euo pipefail
rg -nP -C 10 'MountedPackageCallUnsupported' baml_language/crates
rg -nP -C 6 'foreign_callable_exprs|report_reserved_mounted_call' baml_language/crates/baml_compiler2_tir/srcRepository: BoundaryML/baml
Length of output: 40038
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== builder.rs expression handling around infer_path and call arms =="
sed -n '938,960p' baml_language/crates/baml_compiler2_tir/src/builder.rs
sed -n '6428,6448p' baml_language/crates/baml_compiler2_tir/src/builder.rs
sed -n '7260,7282p' baml_language/crates/baml_compiler2_tir/src/builder.rs
sed -n '10168,10224p' baml_language/crates/baml_compiler2_tir/src/builder.rs
sed -n '10656,10785p' baml_language/crates/baml_compiler2_tir/src/builder.rs
echo "== deterministic source verifier: call graph and insertion/removal behavior =="
python3 - <<'PY'
from pathlib import Path
p = Path('baml_language/crates/baml_compiler2_tir/src/builder.rs')
text = p.read_text()
def line_no(s):
return text[:text.find(s)].count('\n') + 1
checks = [
("report_reserved_mounted_call definition", "fn report_reserved_mounted_call(&mut self, callee: ExprId, at: ExprId)"),
("foreign_callable_exprs remove", "self.foreign_callable_exprs.remove(&callee)", line_no("fn report_reserved_mounted_call(&mut self, callee: ExprId, at: ExprId)")),
("Expr::Call reports at expr_id", "self.report_reserved_mounted_call(*callee, expr_id);", line_no("Expr::Call")),
("Expr::OptionalCall reports at expr_id", "self.report_reserved_mounted_call(*callee, expr_id);", line_no("Expr::OptionalCall")),
("path bare call removes with callee/expr_id same", "self.report_reserved_mounted_call(expr_id, expr_id);", line_no("if self.locals.contains_key(&segments[0])")),
("infer_multi_segment_path path_report removes with callee/expr_id same", "self.report_reserved_mounted_call(expr_id, expr_id);", line_no("if !matches!(pkg_ty, Ty::Unknown { .. })")),
("infer_local_rooted_path path_report removes with callee/expr_id same", "self.report_reserved_mounted_call(expr_id, expr_id);", line_no("if root_is_value")),
("resolve_package_item mark free builtin", "self.foreign_callable_exprs.insert(expr_id, dotted(path));", line_no("if f.builtin_kind.is_some()")),
("resolve_package_item resolve external non-builtin", "self.resolutions.insert(\n expr_id,\n crate::inference::MemberResolution::External", line_no("else")),
]
for name, needle, ctx in checks:
print(f"{name}: found={needle in text} line={ctx}")
print("foreign_callable_exprs removal count near report function:", text[text.find("fn report_reserved_mounted_call"):text.find("fn check_call_expr", text.find("fn report_reserved_mounted_call"))].count("foreign_callable_exprs.remove"))
print("path report count:", text.count("self.report_reserved_mounted_call(expr_id, expr_id)"))
print("call report count:", text.count("self.report_reserved_mounted_call(*callee, expr_id)"))
print("foreign insert count:", text.count("foreign_callable_exprs.insert"))
print("external resolution count:", text.count("MemberResolution::External("))
PYRepository: BoundaryML/baml
Length of output: 12566
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== builder.rs expression handling around MemberAccess and resolution =="
sed -n '7220,7282p' baml_language/crates/baml_compiler2_tir/src/builder.rs
sed -n '10200,10224p' baml_language/crates/baml_compiler2_tir/src/builder.rs
sed -n '11096,11146p' baml_language/crates/baml_compiler2_tir/src/builder.rs
echo "== exact source verifier for reported behavior =="
python3 - <<'PY'
from pathlib import Path
text = Path('baml_language/crates/baml_compiler2_tir/src/builder.rs').read_text().splitlines()
def contains_range(first, last, needle):
needle = str(needle).strip()
needle = needle.replace('\r\n', '\n').replace('\r', '\n')
for i in range(first - 1, last):
if needle in text[i]:
return True
return False
print("foreign_callable_exprs.remove only in report function:", not contains_range(1, 120, "foreign_callable_exprs.remove") and text.count("foreign_callable_exprs.remove") == 1)
print("report_reserved_mounted_call removes keyed callee:", "self.foreign_callable_exprs.remove(&callee)" in "\n".join(text[text.index("fn report_reserved_mounted_call")-1:text.index("fn check_call_expr")]))
print("path call arms use expr_id both callee and error location:", contains_range(10170,10223, "self.report_reserved_mounted_call(expr_id, expr_id)") and text.count("self.report_reserved_mounted_call(expr_id, expr_id)") == 3)
print("Expr::Call arm uses callee report only:", "self.report_reserved_mounted_call(*callee, expr_id);" in "\n".join(text[7268:7276]))
print("resolved calls insert External resolution:", "self.resolutions.insert(" in "\n".join(text[10673:10687]))
print("builtin-kind calls insert foreign record:", "self.foreign_callable_exprs.insert(expr_id, dotted(path));" in "\n".join(text[10673:10687]))
PYRepository: BoundaryML/baml
Length of output: 7398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate MemberAccess and path expression branches =="
rg -n "Expr::MemberAccess|infer_path|report_reserved_mounted_call\\(" baml_language/crates/baml_compiler2_tir/src/builder.rs
echo "== line ranges around relevant branches =="
line=$(rg -n "Path|Path," baml_language/crates/baml_compiler2_tir/src/builder.rs | head -1 | cut -d: -f1)
start=$(( line > 20 ? line - 20 : 1 ))
end=$(( line + 80 ))
sed -n "${start},${end}p" baml_language/crates/baml_compiler2_tir/src/builder.rs
echo "== report_reserved_mounted_call context =="
line=$(rg -n "fn report_reserved_mounted_call" baml_language/crates/baml_compiler2_tir/src/builder.rs | head -1 | cut -d: -f1)
start=$(( line > 10 ? line - 10 : 1 ))
end=$(( line + 25 ))
sed -n "${start},${end}p" baml_language/crates/baml_compiler2_tir/src/builder.rs
echo "== deterministic source verifier =="
python3 - <<'PY'
from pathlib import Path
lines = Path('baml_language/crates/baml_compiler2_tir/src/builder.rs').read_text().splitlines()
def contains(start, end, needle):
return any(needle in s for s in lines[start:end])
print("contains Path branch around member access: ", contains(6200, 7282, "Expr::Path"))
print("contains MemberAccess branch: ", contains(7200, 7260, "Expr::MemberAccess"))
print("path branch counts report_reserved_mounted_call:", lines.count("self.report_reserved_mounted_call(expr_id, expr_id)"))
print("call branch reports callee only at call location:", "self.report_reserved_mounted_call(*callee, expr_id);" in "\n".join(lines[7273:7276]))
print("report method removes keyed callee exactly:", "self.foreign_callable_exprs.remove(&callee)" in "\n".join(lines[6431:6438]))
PYRepository: BoundaryML/baml
Length of output: 7490
Report mounted-package diagnostics only at call sites.
infer_path calls report_reserved_mounted_call(expr_id, expr_id) for bare Expr::Path resolution. That reports TirTypeError::MountedPackageCallUnsupported for let f = pkg.func; and let g = pkg.Method.func; instead of Expr::Call / Expr::OptionalCall only. Move the reserved diagnostic to the call arms and keep foreign_callable_exprs as documented so references type from the exported signature.
🤖 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 10180 -
10188, Remove report_reserved_mounted_call calls from bare path resolution in
infer_path, including local and package/namespace resolution branches. Invoke
the diagnostic only from the Expr::Call and Expr::OptionalCall handling arms,
while preserving foreign_callable_exprs behavior so referenced functions retain
their exported signature types.
Comment on lines
+226
to
+240
| // A MOUNTED (source-less) interface answers from its exported row; its | ||
| // `requires` rows are the pre-flattened transitive closure, so one level | ||
| // of name collection per entry covers inheritance (BEP-066 slice 6a). | ||
| if let Some(crate::package_interface::ExportedType::Interface { | ||
| associated_types, | ||
| requires, | ||
| .. | ||
| }) = crate::package_interface::mounted_type_row(db, qtn) | ||
| { | ||
| let mut names: FxHashSet<Name> = associated_types.iter().map(|a| a.name.clone()).collect(); | ||
| for required in requires { | ||
| names.extend(interface_associated_type_names_for_qtn(db, &required.name)); | ||
| } | ||
| return names; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the recursion over the mounted requires rows.
The comment states that requires is the pre-flattened transitive closure, so one level of collection is enough. The code still recurses into each entry. Two consequences follow.
- The recursion is redundant work when the closure is already flat: every entry re-expands a set that is already present.
- The recursion has no visited set. A mounted blob whose rows form a
requirescycle (a corrupt or hand-crafted blob, whichpackage_interfaceaccepts as long as it deserializes) drives unbounded recursion and overflows the stack.
Either drop the recursion or add a visited set.
🛡️ Proposed fix using a visited set
- if let Some(crate::package_interface::ExportedType::Interface {
- associated_types,
- requires,
- ..
- }) = crate::package_interface::mounted_type_row(db, qtn)
- {
- let mut names: FxHashSet<Name> = associated_types.iter().map(|a| a.name.clone()).collect();
- for required in requires {
- names.extend(interface_associated_type_names_for_qtn(db, &required.name));
- }
- return names;
- }
+ if let Some(crate::package_interface::ExportedType::Interface {
+ associated_types,
+ requires,
+ ..
+ }) = crate::package_interface::mounted_type_row(db, qtn)
+ {
+ // The row's `requires` is the pre-flattened transitive closure, so a
+ // single pass over it is complete; collecting each entry's OWN
+ // associated types (never re-entering this function) also keeps a
+ // cyclic blob from recursing without bound.
+ let mut names: FxHashSet<Name> = associated_types.iter().map(|a| a.name.clone()).collect();
+ for required in requires {
+ if let Some(crate::package_interface::ExportedType::Interface {
+ associated_types,
+ ..
+ }) = crate::package_interface::mounted_type_row(db, &required.name)
+ {
+ names.extend(associated_types.iter().map(|a| a.name.clone()));
+ } else {
+ names.extend(interface_associated_type_names_for_qtn(db, &required.name));
+ }
+ }
+ return names;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // A MOUNTED (source-less) interface answers from its exported row; its | |
| // `requires` rows are the pre-flattened transitive closure, so one level | |
| // of name collection per entry covers inheritance (BEP-066 slice 6a). | |
| if let Some(crate::package_interface::ExportedType::Interface { | |
| associated_types, | |
| requires, | |
| .. | |
| }) = crate::package_interface::mounted_type_row(db, qtn) | |
| { | |
| let mut names: FxHashSet<Name> = associated_types.iter().map(|a| a.name.clone()).collect(); | |
| for required in requires { | |
| names.extend(interface_associated_type_names_for_qtn(db, &required.name)); | |
| } | |
| return names; | |
| } | |
| // A MOUNTED (source-less) interface answers from its exported row; its | |
| // `requires` rows are the pre-flattened transitive closure, so one level | |
| // of name collection per entry covers inheritance (BEP-066 slice 6a). | |
| if let Some(crate::package_interface::ExportedType::Interface { | |
| associated_types, | |
| requires, | |
| .. | |
| }) = crate::package_interface::mounted_type_row(db, qtn) | |
| { | |
| // The row's `requires` is the pre-flattened transitive closure, so a | |
| // single pass over it is complete; collecting each entry's OWN | |
| // associated types (never re-entering this function) also keeps a | |
| // cyclic blob from recursing without bound. | |
| let mut names: FxHashSet<Name> = associated_types.iter().map(|a| a.name.clone()).collect(); | |
| for required in requires { | |
| if let Some(crate::package_interface::ExportedType::Interface { | |
| associated_types, | |
| .. | |
| }) = crate::package_interface::mounted_type_row(db, &required.name) | |
| { | |
| names.extend(associated_types.iter().map(|a| a.name.clone())); | |
| } else { | |
| names.extend(interface_associated_type_names_for_qtn(db, &required.name)); | |
| } | |
| } | |
| return names; | |
| } |
🤖 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 226 -
240, Update the mounted-interface branch in
interface_associated_type_names_for_qtn to collect each required row’s
associated-type names directly from its exported interface data instead of
recursively calling the same helper. Preserve the current names from
associated_types and the one-level requires behavior, ensuring cycles cannot
cause unbounded recursion.
Comment on lines
+2148
to
+2212
| let method_generic_arity_matches = | ||
| impl_method_params.len() == iface_method.generic_params.len(); | ||
| let mut iface_method_bindings = realize_bindings.clone(); | ||
| if method_generic_arity_matches { | ||
| iface_method_bindings.extend( | ||
| iface_method | ||
| .generic_params | ||
| .iter() | ||
| .zip(impl_method_params) | ||
| .map(|(iface_param, impl_param)| { | ||
| ( | ||
| iface_param.clone(), | ||
| Ty::TypeVar(impl_param.clone(), TyAttr::default()), | ||
| ) | ||
| }), | ||
| ); | ||
| } | ||
| // The throws slot mirrors `lower_signature`'s Missing convention: the | ||
| // declared clause, `unknown` when unwritten (a default method's | ||
| // body-inferred contract is not the declared surface). | ||
| let iface_fn = substitute_ty( | ||
| &Ty::Function { | ||
| params: iface_method.params.clone(), | ||
| ret: Box::new(iface_method.return_type.clone()), | ||
| throws: Box::new(iface_method.declared_throws.clone().unwrap_or(Ty::Unknown { | ||
| attr: TyAttr::default(), | ||
| })), | ||
| attr: TyAttr::default(), | ||
| }, | ||
| &iface_method_bindings, | ||
| ); | ||
|
|
||
| if !method_generic_arity_matches | ||
| || !baml_type::normalize::is_subtype(&impl_fn, &iface_fn, &ctx) | ||
| { | ||
| diags.push(( | ||
| crate::infer_context::TirTypeError::InterfaceMethodSignatureMismatch { | ||
| interface: iface_qtn.clone(), | ||
| method: method_name.clone(), | ||
| expected: iface_fn, | ||
| got: impl_fn, | ||
| }, | ||
| ImplDiagnosticLocation::Method(method_name.clone()), | ||
| )); | ||
| } | ||
|
|
||
| // An override may not add a generic bound the interface method does | ||
| // not declare (the exported rows carry the interface-side bounds). | ||
| let impl_bounds_by_param = method_generic_bound_interfaces( | ||
| db, | ||
| &res_ctx.own_items, | ||
| &pkg_info.namespace_path, | ||
| &impl_scope_generics, | ||
| &impl_spec, | ||
| ); | ||
| let iface_method_bounds: Vec<Vec<baml_type::Interface>> = iface_method | ||
| .generic_param_bounds | ||
| .iter() | ||
| .map(|conjunction| { | ||
| conjunction | ||
| .iter() | ||
| .map(|bound| bound.map_tys(|ty| substitute_ty(ty, &iface_method_bindings))) | ||
| .collect() | ||
| }) | ||
| .collect(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the two generic-parameter lists differ in currency.
set -euo pipefail
# What InterfaceMethodSpec::generic_param_names returns.
rg -n -C12 'fn generic_param_names' --type=rust baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs
# Where ExportedFunction::generic_params is populated, and whether the
# synthetic effect params are included.
rg -n -C10 'fn is_synthetic_effect_param' --type=rust baml_language
rg -n -C8 'generic_params:' --type=rust baml_language/crates/baml_compiler2_tir/src/package_interface.rs
rg -n -C8 'fn function_generic_params' --type=rust baml_language/crates/baml_compiler2_tirRepository: BoundaryML/baml
Length of output: 14467
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the mounted-arm logic and nearby definitions/imports.
sed -n '180,2260p' baml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rs
# Locate ExternalCallable::user_generic_params and callers.
rg -n -C8 'ExternalCallable|user_generic_params|synthetic_effect_param|generic_param_names|generic_bounds' --type=rust baml_language/crates/baml_compiler2_tir/src/inference.rs baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs baml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rsRepository: BoundaryML/baml
Length of output: 50372
Compare impl generics against the method’s user-declared generic params.
iface_method.generic_params includes synthetic callback-effect params, while impl_method_params is only the override’s written declarations. The arity check fails for valid mounted-override methods whose exported signature carries a synthetic callback effect param, and bound entailment can align the wrong interface-param bounds. Use an ExternalCallable::user_generic_params-compatible list that filters baml_type::is_synthetic_effect_param, then apply it to arities, bindings, and 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/interfaces/impl_rules.rs` around
lines 2148 - 2212, Filter iface_method.generic_params through
baml_type::is_synthetic_effect_param to derive the user-declared generic
parameter list, matching ExternalCallable::user_generic_params. Use this
filtered list for the arity comparison and iface_method_bindings, and align
iface_method_bounds with the same filtered parameters so synthetic
callback-effect parameters do not affect signature validation or bound
entailment.
Comment on lines
+1209
to
+1247
| let impl_param_count = impl_param_names.len(); | ||
| let scope_generics = crate::function_generic_params(db, method_loc); | ||
| debug_assert_eq!( | ||
| &scope_generics[..impl_param_count], | ||
| impl_param_names.as_slice(), | ||
| "an impl method's function environment must begin with the impl parameters" | ||
| ); | ||
| let own_params = &scope_generics[impl_param_count..]; | ||
| let mut bounds: crate::lower_type_expr::TypeVarBoundsMap = | ||
| data.generic_params.iter().cloned().collect(); | ||
| // Lowering diagnostics are dropped, matching every export path: the | ||
| // checked diagnostics for an impl are owned by `impl_data` + | ||
| // `validate_impl_signatures`. | ||
| let mut diags = Vec::new(); | ||
| let mut generic_params = Vec::new(); | ||
| let mut generic_param_bounds = Vec::new(); | ||
| let declared_count = spec.generic_bounds().len(); | ||
| for (param, declared) in own_params[..declared_count] | ||
| .iter() | ||
| .zip(spec.generic_bounds()) | ||
| { | ||
| let ifaces = crate::interfaces::lower_generic_param_interface_bounds( | ||
| db, | ||
| spec.bound_store(), | ||
| &declared.bounds, | ||
| pkg_items, | ||
| ns, | ||
| &scope_generics, | ||
| &mut diags, | ||
| ); | ||
| bounds.insert(param.clone(), ifaces.clone()); | ||
| generic_params.push(param.clone()); | ||
| generic_param_bounds.push(ifaces); | ||
| } | ||
| for param in &own_params[declared_count..] { | ||
| bounds.insert(param.clone(), Vec::new()); | ||
| generic_params.push(param.clone()); | ||
| generic_param_bounds.push(Vec::new()); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the generic-parameter slicing against a shorter scope list.
Lines 1212, 1216, 1226, and 1243 slice scope_generics and own_params at counts derived from other sources (impl_param_names.len(), spec.generic_bounds().len()). If either count exceeds the actual length, the slice panics and aborts the whole package_interface derivation. The rest of this function drops a row with None on an invariant break (Line 1287). Apply the same policy here.
🛡️ Proposed defensive fix
let impl_param_count = impl_param_names.len();
let scope_generics = crate::function_generic_params(db, method_loc);
+ if scope_generics.len() < impl_param_count {
+ debug_assert!(
+ false,
+ "an impl method's function environment must begin with the impl parameters"
+ );
+ return None;
+ }
debug_assert_eq!(
&scope_generics[..impl_param_count],
impl_param_names.as_slice(),
"an impl method's function environment must begin with the impl parameters"
);
let own_params = &scope_generics[impl_param_count..];
@@
- let declared_count = spec.generic_bounds().len();
+ let declared_count = spec.generic_bounds().len().min(own_params.len());🤖 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/package_interface.rs` around
lines 1209 - 1247, Guard the generic-parameter slicing in the surrounding
package-interface derivation before using impl_param_count or declared_count. If
scope_generics is shorter than impl_param_count, or own_params is shorter than
declared_count, stop this row’s derivation and return the same None result used
for invariant breaks later in the function, rather than slicing and panicking.
Preserve the existing bounds-lowering behavior when both counts are within
range.
Comment on lines
+431
to
+448
| pub fn add_compiler2_virtual_file( | ||
| &mut self, | ||
| path: impl AsRef<std::path::Path>, | ||
| content: &str, | ||
| ) -> SourceFile { | ||
| let path = path.as_ref().to_path_buf(); | ||
| let file = self.add_file_internal(&path, content); | ||
| let file_id = file.file_id(self); | ||
| Arc::make_mut(&mut self.file_id_to_path).insert(file_id, path.clone()); | ||
| self.compiler2_file_map.insert(path, file); | ||
| let extra = self | ||
| .compiler2_extra_files | ||
| .expect("set_project_root creates the Compiler2ExtraFiles input"); | ||
| let mut files = extra.files(self).clone(); | ||
| files.push(file); | ||
| extra.set_files(self).to(files); | ||
| file | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Guard add_compiler2_virtual_file against a repeated path.
The method always mints a new SourceFile and pushes it onto Compiler2ExtraFiles. compiler2_file_map replaces the old handle, but the extra-files list keeps both. A second call with the same path therefore makes compiler2_all_files return two files with identical content, which duplicates every item that file declares in its package. The method is pub, so a caller outside the fixtures can hit this.
🛠️ Proposed guard for a repeated path
) -> SourceFile {
let path = path.as_ref().to_path_buf();
+ if let Some(&existing) = self.compiler2_file_map.get(&path) {
+ existing.set_text(self).to(content.to_string());
+ return existing;
+ }
let file = self.add_file_internal(&path, content);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn add_compiler2_virtual_file( | |
| &mut self, | |
| path: impl AsRef<std::path::Path>, | |
| content: &str, | |
| ) -> SourceFile { | |
| let path = path.as_ref().to_path_buf(); | |
| let file = self.add_file_internal(&path, content); | |
| let file_id = file.file_id(self); | |
| Arc::make_mut(&mut self.file_id_to_path).insert(file_id, path.clone()); | |
| self.compiler2_file_map.insert(path, file); | |
| let extra = self | |
| .compiler2_extra_files | |
| .expect("set_project_root creates the Compiler2ExtraFiles input"); | |
| let mut files = extra.files(self).clone(); | |
| files.push(file); | |
| extra.set_files(self).to(files); | |
| file | |
| } | |
| pub fn add_compiler2_virtual_file( | |
| &mut self, | |
| path: impl AsRef<std::path::Path>, | |
| content: &str, | |
| ) -> SourceFile { | |
| let path = path.as_ref().to_path_buf(); | |
| if let Some(&existing) = self.compiler2_file_map.get(&path) { | |
| existing.set_text(self).to(content.to_string()); | |
| return existing; | |
| } | |
| let file = self.add_file_internal(&path, content); | |
| let file_id = file.file_id(self); | |
| Arc::make_mut(&mut self.file_id_to_path).insert(file_id, path.clone()); | |
| self.compiler2_file_map.insert(path, file); | |
| let extra = self | |
| .compiler2_extra_files | |
| .expect("set_project_root creates the Compiler2ExtraFiles input"); | |
| let mut files = extra.files(self).clone(); | |
| files.push(file); | |
| extra.set_files(self).to(files); | |
| 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_project/src/db.rs` around lines 431 - 448, Update
add_compiler2_virtual_file to detect an existing path in compiler2_file_map
before creating a new SourceFile; return the existing handle for repeated paths
and leave compiler2_extra_files unchanged, while preserving the current
insertion and list-update behavior for new paths.
Comment on lines
+1233
to
+1275
| #[test] | ||
| fn user_impl_of_mounted_interface_missing_method_is_rejected() { | ||
| // E0113: the blob row's REQUIRED method list drives coverage. | ||
| let db = consumer_db( | ||
| lib_blob(), | ||
| &[( | ||
| "main.baml", | ||
| r#" | ||
| class Card { | ||
| title string | ||
|
|
||
| implements app.Describable { | ||
| } | ||
| } | ||
| "#, | ||
| )], | ||
| ); | ||
| assert_error_containing(&db, "describe"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn user_impl_of_mounted_interface_signature_mismatch_is_rejected() { | ||
| // E0120: the override must conform to the blob row's symbolic-Self | ||
| // signature. | ||
| let db = consumer_db( | ||
| lib_blob(), | ||
| &[( | ||
| "main.baml", | ||
| r#" | ||
| class Card { | ||
| title string | ||
|
|
||
| implements app.Describable { | ||
| function describe(self) -> int throws never { | ||
| 1 | ||
| } | ||
| } | ||
| } | ||
| "#, | ||
| )], | ||
| ); | ||
| assert_error_containing(&db, "describe"); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the diagnostic code, not only the member name.
Both tests assert on the substring "describe". The comments claim E0113 (missing required method) and E0120 (signature mismatch), but the assertions cannot distinguish them. If mounted interface resolution regresses and app.Describable stops resolving, the resulting unresolved-name error still mentions describe, and both tests keep passing.
error_messages already renders the code as [{code}], so the codes are directly assertable. This matches the tightening applied in baml_language/crates/baml_tests/tests/interfaces.rs at lines 11936-11951 in this same PR.
💚 Proposed assertion tightening
- assert_error_containing(&db, "describe");
+ assert_error_containing(&db, "E0113");
+ assert_error_containing(&db, "describe");and for the signature-mismatch case:
- assert_error_containing(&db, "describe");
+ assert_error_containing(&db, "E0120");
+ assert_error_containing(&db, "describe");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[test] | |
| fn user_impl_of_mounted_interface_missing_method_is_rejected() { | |
| // E0113: the blob row's REQUIRED method list drives coverage. | |
| let db = consumer_db( | |
| lib_blob(), | |
| &[( | |
| "main.baml", | |
| r#" | |
| class Card { | |
| title string | |
| implements app.Describable { | |
| } | |
| } | |
| "#, | |
| )], | |
| ); | |
| assert_error_containing(&db, "describe"); | |
| } | |
| #[test] | |
| fn user_impl_of_mounted_interface_signature_mismatch_is_rejected() { | |
| // E0120: the override must conform to the blob row's symbolic-Self | |
| // signature. | |
| let db = consumer_db( | |
| lib_blob(), | |
| &[( | |
| "main.baml", | |
| r#" | |
| class Card { | |
| title string | |
| implements app.Describable { | |
| function describe(self) -> int throws never { | |
| 1 | |
| } | |
| } | |
| } | |
| "#, | |
| )], | |
| ); | |
| assert_error_containing(&db, "describe"); | |
| } | |
| #[test] | |
| fn user_impl_of_mounted_interface_missing_method_is_rejected() { | |
| // E0113: the blob row's REQUIRED method list drives coverage. | |
| let db = consumer_db( | |
| lib_blob(), | |
| &[( | |
| "main.baml", | |
| r#" | |
| class Card { | |
| title string | |
| implements app.Describable { | |
| } | |
| } | |
| "#, | |
| )], | |
| ); | |
| assert_error_containing(&db, "E0113"); | |
| assert_error_containing(&db, "describe"); | |
| } | |
| #[test] | |
| fn user_impl_of_mounted_interface_signature_mismatch_is_rejected() { | |
| // E0120: the override must conform to the blob row's symbolic-Self | |
| // signature. | |
| let db = consumer_db( | |
| lib_blob(), | |
| &[( | |
| "main.baml", | |
| r#" | |
| class Card { | |
| title string | |
| implements app.Describable { | |
| function describe(self) -> int throws never { | |
| 1 | |
| } | |
| } | |
| } | |
| "#, | |
| )], | |
| ); | |
| assert_error_containing(&db, "E0120"); | |
| assert_error_containing(&db, "describe"); | |
| } |
🤖 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/src/compiler2_tir/package_interface.rs`
around lines 1233 - 1275, Update both tests,
user_impl_of_mounted_interface_missing_method_is_rejected and
user_impl_of_mounted_interface_signature_mismatch_is_rejected, to assert the
expected diagnostic codes E0113 and E0120 respectively through the rendered
error message. Keep the existing member-name checks if useful, but ensure each
test independently fails when mounted interface resolution produces a different
error.
Contributor
Author
|
Closing per plan change: stacked PRs aren't available in this repo, so the whole of BEP-066 ships as a single PR — #4325 is now the long-lived feature branch. This PR's content (enriched export schema, plus the rest of the slice-6a track: #4327 #4329 #4332 #4333) is fully contained in #4325 via merge commit b84086b. Review happens there. |
This was referenced Aug 15, 2026
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BEP-066 slice-6a stack, PR 1 of 5 (track B: interface-centric dependency consumption). Derivation only — zero behavior change by construction; consumers land in the follow-up PRs of this stack.
What
Grows the B-694
PackageInterfaceexport schema so a user package can eventually be consumed by the compiler as a serialized blob with no source present:ExportedType::Interface— generic params + symbolicSelf, per-param bounds, pre-flattened transitiverequiresclosure, associated types (bound + default pre-lowered with symbolicSelf), fields with attrs, required+default methods.ExportedFunction:generic_param_bounds(parallel togeneric_params),interface_target, stablecallable_fqn.ExportedType::Class: field attrs, per-methodinterface_target, per-param bounds.PackageInterface.namespaces: the full namespace-path set (interface-only namespaces included).How the high-risk parts are derived (no parallel lowering)
requiresclosure: a thin wrapper (interface_requires_closure_symbolic) over the checker's owninterface_closure_locs_with_args_and_assocwalk — identity args,fill_associated_defaults=falseper the rigid-Selfrule; consumers fill defaults from the exported rows post-substitution.interface_associated_type_defaultoutput stored verbatim.resolve_interface_required_methods(whose output stays bit-identical), becausefunction_signature_tylowers default methods without aSelfbinding.Behavior-neutrality proof
PackageInterface.typesaudited; the two paths where new Interface rows could newly resolve (resolve_type/resolve_type_in_own_then_deps) explicitly skip them pending the dualization PR.cargo test -p baml_tests -- --skip parser_stress: green, no snapshot churn. Also green: tir (324), project (82), cli bytecode_cache incl. B-694 determinism (43), LSP (468).Swapped<A,B> requires Pair<B,A>substitution, borsh round-trip + two-DB byte determinism (user + stdlib), stdlib derivation with thebaml.iter.Iteratorrequires/assoc spot-check.Notes
@alias/@descriptionaren't hoisted intoFieldData.attributesby AST lowering (class-side hoist doesn't run for interfaces); the export reads both homes. Parser fix + snapshot check deferred.thoughts/antonio/bep066-architecture-and-slices.md§2.8a; research:bep066-salsa-research.mdR1/R2.Summary by CodeRabbit
New Features
Bug Fixes
Tests