Repository navigation
Type algebra unification and runtime interface objects - #3896
Merged
Merged
Conversation
|
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (31)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (28)
📝 WalkthroughWalkthroughThe compiler, VM, engine, heap, and tooling now represent interfaces and impl rules as per-package objects. Runtime lookup, type normalization, and object handling were updated to use package-indexed resolution and the new ChangesPackage-based interface impl registry
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
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 |
Binary size checks passed✅ 7 passed
Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/bex_vm/src/package_baml/resolve.rs (1)
124-139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScan the declaring packages when resolving candidate impl rules.
candidate_rulesonly checks the concrete type’s package and the interface package, but impl rules are stored in the package where the impl is declared. A package-level blanket/primitive/container impl for an imported interface can therefore be visible inimplementor_entriesbut missed by runtime dispatch/type membership here.Potential fix
- let mut pkgs: Vec<&Name> = Vec::with_capacity(2); - if let Some(p) = type_package(concrete_ty) { - pkgs.push(p); - } - let iface_pkg = iface.package(); - if !pkgs.contains(&iface_pkg) { - pkgs.push(iface_pkg); - } let mut out = Vec::new(); - for pkg in pkgs { - let Some(&pkg_ptr) = vm.packages.get(pkg) else { - continue; - }; + for &pkg_ptr in vm.packages.values() { collect_package_rules(vm, deref_package(vm, pkg_ptr), iface_ptr, &mut out); } out🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bex_vm/src/package_baml/resolve.rs` around lines 124 - 139, candidate_rules currently only searches the concrete type package and iface.package(), but impl rules live in the package where the impl is declared, so runtime dispatch can miss imported-interface impls. Update candidate_rules in resolve.rs to also scan the declaring package(s) for the interface/impl relationship, using the existing helpers like collect_package_rules, deref_package, and vm.packages lookups, so package-level blanket/primitive/container impls are included in the candidate set.
🧹 Nitpick comments (3)
baml_language/crates/bex_vm/src/type_context.rs (1)
1-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a unit test suite for
RuntimeTypeContextonce wired.Per coding guidelines, unit tests are preferred over integration tests for
*.rsfiles. This file currently has none, though it is intentionallydead_codeand unwired per the module doc, so a test fixture would need a constructedBexVm. Worth tracking for the follow-up PR that flips call sites onto this context.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/bex_vm/src/type_context.rs` around lines 1 - 110, Add Rust unit tests for RuntimeTypeContext instead of leaving this module untested; the review asks to cover it once it is wired, and the best place is a #[cfg(test)] module alongside RuntimeTypeContext::new and the TypeContext impl methods. Focus on small unit-level checks for alias_def, implements_interface, interface_requires, and enum_variants, using a minimal constructed BexVm test fixture so the tests stay local to this file and avoid integration coverage.Source: Coding guidelines
baml_language/crates/bex_heap/src/gc.rs (1)
550-563: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the GC invariant comment to cover
Interface,Package, andImplRule. The leaf-arm shortcut still holds, but the doc should say these variants have no runtimeHeapPtrs, not noHeapPtrs at all.PackageandImplRuledo carryHeapPtrfields, so the invariant needs that wording to stay precise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bex_heap/src/gc.rs` around lines 550 - 563, Update the GC invariant comment in gc.rs so it accurately describes the leaf-arm shortcut for Object::Interface, Object::Package, and Object::ImplRule. The current wording implies these variants have no HeapPtr fields at all, but Package and ImplRule do contain HeapPtr members; revise the comment to say they have no runtime HeapPtrs that need tracing, while keeping the existing Object match arm unchanged. Use the Object enum variant names in the comment to make the invariant precise.baml_language/crates/bex_vm/src/package_baml/future.rs (1)
95-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the stale unsafe lookup comment.
The pointer no longer comes from
resolved_class_names; usevm.get_object(enm_ptr)here and drop the outdated safety note.Proposed cleanup
let Some(enm_ptr) = vm.lookup_type_by_fqn("baml.future.FutureState") else { return Value::NULL; }; - // SAFETY: enm_ptr came from resolved_class_names which holds - // compile-time-registered HeapPtrs valid for the program's lifetime. - let idx = match unsafe { enm_ptr.get() } { + let idx = match vm.get_object(enm_ptr) { Object::Enum(e) => e.variants.iter().position(|v| v.name == variant_name), _ => None, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bex_vm/src/package_baml/future.rs` around lines 95 - 100, The lookup in future.rs is outdated: the pointer is no longer sourced from resolved_class_names, so the SAFETY comment is stale. Update the FutureState handling in the code path around vm.lookup_type_by_fqn and enm_ptr to use vm.get_object(enm_ptr) instead of unsafe access, and remove the obsolete safety note so the implementation matches the current ownership model.
🤖 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 287-298: The impl method dispatch table is currently allowed to
silently drop entries when resolve_fqn cannot find a function index, which can
serialize incomplete metadata. Update resolve_fqn and its callers in the emit
flow to fail hard during emission (or use an explicit expect with the missing
FQN) instead of returning None and skipping methods/defaults, so invalid FQNs in
the emission path surface as a compiler error. Apply the same treatment to the
related call sites around the other dispatch table builders to keep emitted impl
tables complete and consistent.
- Around line 89-93: The runtime lowering in lower_rt is dropping valid
InterfaceDef members whenever a signature mentions Self or Self.Assoc, which
then causes callers in the interface param/field/return/throws handling paths to
omit members or substitute Void. Update the signature lowering path used by
InterfaceDef-related code so it is interface-aware and can preserve arity for
Self-based and associated-type references, either by projecting them explicitly
or by carrying an unresolved/runtime placeholder instead of failing closed. Make
the change in the lower_rt flow and the callers that build interface metadata so
valid interface members are never silently removed.
In `@baml_language/crates/bex_vm/src/package_baml/csv.rs`:
- Around line 1405-1408: The CSV enum resolution in Target::Enum is using
lookup_type_by_fqn on a rendered name, which can fail for user-package enum
fields. Update the Target::Enum handling in csv.rs to keep and use the enum
TypeName and resolve it through vm.lookup_type(qtn) instead of
vm.lookup_type_by_fqn, while preserving the existing bad-conversion error path
when the type cannot be found.
In `@baml_language/crates/bex_vm/src/type_context.rs`:
- Around line 6-10: Update the stale doc comments in `type_context.rs` to match
the new per-package runtime model: revise the module and struct docs for
`TypeContext`, `implements_interface`, and `enum_variants` so they describe
delegation to `package_baml::type_implements` and
`self.vm.lookup_type(name)`/the `packages` index instead of the old baked
`interface_impls` and `resolved_class_names` terminology. Keep the wording
aligned with the actual implementations of `implements_interface`, `alias_def`,
and `enum_variants`, and remove references to the pre-refactor registry/heap
model.
In `@baml_language/crates/bex_vm/tests/interface_registry.rs`:
- Around line 23-30: The baked-rule matcher in the interface registry helper is
too loose because it compares `iface` by short name and `for_type` by substring,
which can pick the wrong rule when names overlap. Update the logic around the
`find` predicate in `interface_registry.rs` to use qualified names or a
structured `TyTemplate`/`TypeName` comparison, and keep the existing
`as_interface()` lookup aligned with the fully qualified interface identity
instead of string containment.
---
Outside diff comments:
In `@baml_language/crates/bex_vm/src/package_baml/resolve.rs`:
- Around line 124-139: candidate_rules currently only searches the concrete type
package and iface.package(), but impl rules live in the package where the impl
is declared, so runtime dispatch can miss imported-interface impls. Update
candidate_rules in resolve.rs to also scan the declaring package(s) for the
interface/impl relationship, using the existing helpers like
collect_package_rules, deref_package, and vm.packages lookups, so package-level
blanket/primitive/container impls are included in the candidate set.
---
Nitpick comments:
In `@baml_language/crates/bex_heap/src/gc.rs`:
- Around line 550-563: Update the GC invariant comment in gc.rs so it accurately
describes the leaf-arm shortcut for Object::Interface, Object::Package, and
Object::ImplRule. The current wording implies these variants have no HeapPtr
fields at all, but Package and ImplRule do contain HeapPtr members; revise the
comment to say they have no runtime HeapPtrs that need tracing, while keeping
the existing Object match arm unchanged. Use the Object enum variant names in
the comment to make the invariant precise.
In `@baml_language/crates/bex_vm/src/package_baml/future.rs`:
- Around line 95-100: The lookup in future.rs is outdated: the pointer is no
longer sourced from resolved_class_names, so the SAFETY comment is stale. Update
the FutureState handling in the code path around vm.lookup_type_by_fqn and
enm_ptr to use vm.get_object(enm_ptr) instead of unsafe access, and remove the
obsolete safety note so the implementation matches the current ownership model.
In `@baml_language/crates/bex_vm/src/type_context.rs`:
- Around line 1-110: Add Rust unit tests for RuntimeTypeContext instead of
leaving this module untested; the review asks to cover it once it is wired, and
the best place is a #[cfg(test)] module alongside RuntimeTypeContext::new and
the TypeContext impl methods. Focus on small unit-level checks for alias_def,
implements_interface, interface_requires, and enum_variants, using a minimal
constructed BexVm test fixture so the tests stay local to this file and avoid
integration coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 84edd0b0-e63b-4734-b736-70ea7be39fc4
⛔ Files ignored due to path filters (2)
baml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snap
📒 Files selected for processing (30)
baml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rsbaml_language/crates/baml_type/src/normalize.rsbaml_language/crates/baml_type/src/normalize/tests.rsbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/src/trace_heap.rsbaml_language/crates/bex_heap/src/accessor.rsbaml_language/crates/bex_heap/src/gc.rsbaml_language/crates/bex_heap/src/heap.rsbaml_language/crates/bex_heap/src/heap_debugger/real.rsbaml_language/crates/bex_vm/src/lib.rsbaml_language/crates/bex_vm/src/package_baml/csv.rsbaml_language/crates/bex_vm/src/package_baml/future.rsbaml_language/crates/bex_vm/src/package_baml/json.rsbaml_language/crates/bex_vm/src/package_baml/mod.rsbaml_language/crates/bex_vm/src/package_baml/ops.rsbaml_language/crates/bex_vm/src/package_baml/resolve.rsbaml_language/crates/bex_vm/src/package_baml/root.rsbaml_language/crates/bex_vm/src/package_load.rsbaml_language/crates/bex_vm/src/type_context.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm/tests/interface_registry.rsbaml_language/crates/bex_vm_types/src/types.rsbaml_language/crates/bex_vm_types/src/types/interface.rsbaml_language/crates/bex_vm_types/src/types/object.rsbaml_language/crates/bex_vm_types/src/types/package.rsbaml_language/crates/tools_onionskin/src/compiler.rsbaml_language/crates/tools_sap_visualizer/src/compile.rs
If we don't know the resolved base type and/or interface types, we can still do subtyping based on the associated type's interface bounds.
The runtime implementor of `baml_type::normalize::TypeContext` for dynamically evaluating type algebra such as subtyping and equivalence. WIP: `<RuntimeTypeContext as TypeContext>::interface_requires`
Similar to `Object::Class` and other program objects, this makes packages, interfaces, and impl rules heap objects. This enables us to have pointer-based identity where possible instead of always doing name-based lookup. - `Object::Package` is now the main path for looking up types/functions/interfaces by name, as well as any impl rules defined in the package. - `Object::Interface` represents an interface definition and holds metadata for reflection. - `Object::ImplRule` represents an `implement` block and can be used for dynamic type resolution. WIP: emitting and using the new package-based lookup paths
- By-name lookups in the runtime should use the package lookup path then find the correct pointers off that. - Removes now-redundant vm fields
We no longer need to load `resolved_class_names` to populate the error/panic name-to-pointer registries since that is handled by the package system now.
- Error/panic class ptrs are now `Arc<[...]>` instead of copied `Vec` - Don't do an `O(n)` walk to find the interface for `impl_rules`, just use package interface lookup - Delete some now-unused code - Docs cleanup
2kai2kai2
force-pushed
the
kai/unified-type-algebra
branch
from
July 1, 2026 21:34
f0e9b4d to
8c9c7c6
Compare
2kai2kai2
enabled auto-merge
July 1, 2026 21:41
This was referenced Jul 6, 2026
Merged
This was referenced Jul 15, 2026
This was referenced Jul 28, 2026
Merged
This was referenced Aug 5, 2026
feat(tir): enrich PackageInterface export schema for source-less deps (BEP-066 slice 6a, PR 1)
#4326
Closed
This was referenced Aug 24, 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.
Object::Interface,Object::ImplRule, andObject::Packageas new heap object types. LikeClass, these are not user values but instead allow program elements to be addressable by heap pointer.Object::Packageis now the unified way to lookup items by name.This PR is part of an ongoing process so further PRs will be forthcoming.
Summary by CodeRabbit
New Features
Bug Fixes
Tests