Repository navigation
Fix 17 interface-union bugs (F1–F17) - #3638
Conversation
Found by a union-focused fuzz of BEP-044 interfaces; each was pinned as a
failing `union_fuzz_fNN_*` test in baml_tests/tests/interfaces.rs and is now
green.
- F7/F12: a method present on every member of a union containing an interface
no longer rejects as `unknown | unknown`. The TIR union-member probe resolves
interface members through the real interface machinery (side effects
suppressed), and MIR dispatches the call on the runtime class across all
members' implementors.
- F1/F3/F11: reading a field declared by an interface member of a union no
longer crashes the VM; MIR dispatches the field read on the runtime class.
Conflicting field types across union interfaces read soundly as `T | U`
(misuse is E0001); a genuinely ambiguous field view is E0131.
- F2/F5: a generic-interface match arm (`Slot<int>`) now respects its type
argument at runtime instead of matching every implementor of the bare
interface (which fed a string into integer code).
- F4: `string + <non-object primitive>` (int/float/bigint/bool/null) is a type
error rather than inferring `string` and aborting the VM; `string +
uint8array` stays valid.
- F6: reflection compares generic union arguments as unordered sets, so
`Box<int | string>` and `Box<string | int>` are the same instantiation.
- F8: a bounded type variable is a subtype of itself (`<T extends I>(a: T) -> T
{ return a }` compiles).
- F9: a match arm whose pattern overlaps any member of an optional/union
scrutinee is accepted (`let a: Animal` over `(Dog | Cat)?`).
- F10/F15: out-of-body `implements I for <primitive>` resolves for every
primitive (not just `int`), and union method-not-found blames only the member
that genuinely lacks the method.
- F13/F14/F16/F17: diagnostics no longer leak the internal `user.` package
prefix in match witnesses, name uncovered interface members instead of `_`,
report ambiguous union fields as E0131, and keep generic args in projection
errors (`Cargo<int>`).
Snapshot + LSP expectation updates reflect the corrected diagnostics (no
`user.` leak; `string + int` now E0004). Scratch fuzz repros are gitignored.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
# Conflicts: # baml_language/crates/baml_compiler2_mir/src/lower.rs
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements union/interface runtime dispatch for method calls and field reads, tightens generic-interface runtime/type-test behavior, refactors TIR union-member resolution and diagnostics rollback, adds VM semantic type-arg equivalence, updates witness/diagnostic rendering, and adds regression tests and small gitignore entry. ChangesBEP-044 Interface/Union Dispatch and Generic Interface Type Testing
Sequence Diagram(s)sequenceDiagram
participant Parser as Parser/AST
participant TIR as TypeInferenceBuilder (TIR)
participant MIR as MIR Lowering (lower.rs)
participant VM as Bex VM (type_class)
Parser->>TIR: produce typed TIR nodes (resolve_member, upcasts)
TIR->>MIR: emit lowered calls/field accesses with union/interface-aware annotations
MIR->>VM: emit runtime dispatch guards (emit_method_candidate_switch / interface_implementor_class_guards)
VM->>VM: ty_args_equivalent / ty_equivalent check implementors
VM-->>MIR: runtime dispatch decision (class-tag + type-arg guards)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 9362-9371: Replace the shallow generic-Interface check in
classify_pattern_type_tag with the recursive helper
tir_ty_needs_generic_interface_test so unions/optionals containing generic
interfaces are detected; specifically, where classify_pattern_type_tag currently
rejects only a top-level Tir2Ty::Interface, call
tir_ty_needs_generic_interface_test(ty) to decide to avoid lowering to a flat
type-tag switch. Also apply the same replacement at the other occurrence noted
(the logic around the second classify_pattern_type_tag usage referenced in the
comment) so both locations use the recursive helper.
- Around line 6477-6498: The Class arm currently uses
class_method_item_ref_by_name and thus skips members provided via
interfaces/field_links/implements; instead, treat the concrete class arm like
the interface arm: derive the interface dispatch target for that Tir2Ty (use
self.interface_dispatch_target_for_tir_ty or the same helper used for interface
arms), call self.interface_method_candidates_for(&iface_tn, &iface_type_args,
method) and extend candidates with those results, and only if that candidate
list is empty fall back to the original class_method_item_ref_by_name path.
Apply this change for the Tir2Ty::Class handling here and the analogous block at
6755-6795 so concrete union members do not bypass interface-provided members.
- Around line 7607-7659: The function interface_implementor_class_guards
currently only iterates class_data.implements and then deduplicates by impl_tn
(the out.iter().any check), which drops out-of-body implementors and multiple
valid guards for the same generic class; change it to gather and iterate all
implement blocks for impl_tn (both in-body and out-of-body) instead of just
class_data.implements—e.g. call or add a helper that returns all implement
entries for a given impl_tn—and remove the uniqueness filter that prevents
pushing multiple guards so you can push every guard produced by
interface_class_guard_for_args (keep using resolve_implements_target_view and
interface_closure_type_name_views to compute guards).
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 8053-8107: The class-union path only checks class-owned members
and registry rules (via try_resolve_member_on_ty and try_registry_member inside
resolve_member_suppressing_side_effects) but omits the
inherited-default/interface-member fallback that resolve_member uses; update
this path so that if the registry probe returns None for a Ty::Class arm you
also consult the same inherited/default interface-member resolution used in
resolve_member (the code around lines 7235-7254) and include that result (still
using Self::prepend_self_param_if_method) while keeping diagnostics suppressed
in this union-resolution context. Ensure you call the same helper(s) used by
resolve_member for inherited/interface defaults so class arms relying on a
single implements I {} default are found.
In `@baml_language/crates/baml_tests/tests/interfaces.rs`:
- Around line 9867-9889: The test currently only checks that no diagnostic
contains "user." which falsely passes if there are no errors; modify the test
around the collect_compile_errors call to first assert that the expected
non-exhaustive-match diagnostic (E0062) is present in errors (e.g., find an
entry containing "E0062" or the non-exhaustive-match message) and then verify
that that specific diagnostic's witness text does not contain the "user."
prefix; look for the errors variable and collect_compile_errors invocation to
locate where to add the E0062 presence assertion before the existing no-"user."
check.
- Around line 9568-9583: The test currently only asserts that
collect_compile_errors for the functions plainPlus and main produced some error,
which can hide unrelated failures; update the assertion to pin the expected
diagnostic by checking that the collected errors (the errors variable from
collect_compile_errors) contain a diagnostic/message indicating the
binary-op/type error for "string + int" (e.g. match or assert that one error
text includes "cannot apply binary operation: string + int" or the type-checker
message your compiler emits), and do the same for the other occurrence mentioned
(around the block at lines 9797-9816) so the test fails only if the specific
soundness-related rejection is missing.
🪄 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: 2ef77d7e-c499-4dbc-a8a8-dcaa53eaaf1d
⛔ Files ignored due to path filters (8)
baml_language/crates/baml_tests/snapshots/diagnostic_errors/match_exhaustiveness/baml_tests__diagnostic_errors__match_exhaustiveness__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/match_exhaustiveness/baml_tests__diagnostic_errors__match_exhaustiveness__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/patterns_new/baml_tests__diagnostic_errors__patterns_new__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/patterns_new/baml_tests__diagnostic_errors__patterns_new__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/simple_type_error/baml_tests__diagnostic_errors__simple_type_error__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/simple_type_error/baml_tests__diagnostic_errors__simple_type_error__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_name_type_error/baml_tests__diagnostic_errors__test_expr_name_type_error__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_name_type_error/baml_tests__diagnostic_errors__test_expr_name_type_error__05_diagnostics.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
baml_language/.gitignorebaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/exhaustiveness.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/catch/exhaustiveness_checks.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/invalid_assignment/compound_ops.bamlbaml_language/crates/baml_tests/tests/interfaces.rsbaml_language/crates/bex_vm/src/package_baml/type_class.rs
Binary size checks passed✅ 7 passed
Generated by |
Follow-up to the F1–F17 fixes, resolving CodeRabbit/Cursor review threads.
Each item below reproduced a real bug before the fix; reproducible ones get a
`union_fuzz_pr_*` regression test.
- Class-only union whose members satisfy a method only through an inherited
interface default (`class Dog { implements Greeter {} }`) type-checked but
crashed the VM (`expected map, got instance`): the union method/field dispatch
now resolves concrete-class members through the interfaces they implement
(inherited defaults, field-links, out-of-body impls), not just direct members.
- A function-typed interface *field* in a union no longer gets a phantom `self`
prepended (the self-reattach now applies only to actual methods).
- Reflection generic-arg equivalence recurses through `Optional`/`List`/`Map`,
so a union nested in a wrapper (`Box<(int | string)?>`) is still compared
order-insensitively.
- `classify_pattern_type_tag` disqualifies the flat type-tag switch for any
pattern whose type *contains* a generic interface (recursive check), not just
a top-level one.
- `interface_implementor_class_guards` keeps every matching guard instead of one
per implementor, so a generic class satisfying an interface through multiple
type-arg projections still matches.
- Tightened F4/F11/F13 regression assertions to pin the specific diagnostic
(E0004 / E0001 / E0062) instead of accepting any compile error.
Test plan: cargo test -p baml_tests (2061 passed), -p baml_lsp2_actions_tests
(369 passed), mise run clippy (clean), mise run fmt.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
baml_language/crates/baml_tests/tests/interfaces.rs (1)
9497-9524: ⚡ Quick winAlso pin the nested-order case through
implemented_by()/implementors().This follow-up only exercises
implements(). A nested-union regression in the reverse/registry reflection paths would still pass here because F6 only covers the non-nested shape.🤖 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/interfaces.rs` around lines 9497 - 9524, The test union_fuzz_pr_reflection_nested_union_arg_order_insensitive currently only checks implements(), so add assertions that pin the same nested-union order-insensitivity through the reverse/registry reflection paths by invoking implemented_by() and implementors() for the same types; specifically call reflect.implemented_by(Box<(int | string)?>) and reflect.implemented_by(Box<(string | int)?>) (or their runtime equivalents) and also query reflect.implementors(fwd) and reflect.implementors(rev) to ensure both directions report the UBox implementor, updating the test to assert those results equal as well as the existing implements() checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_tests/tests/interfaces.rs`:
- Around line 9468-9471: Update the PR validation notes in instructions.mdc to
require running and capturing the output of the command "cargo test --lib"
whenever Rust code changes; add a short bullet under the PR validation notes /
"validation notes" section that instructs contributors to run that command
locally, paste the resulting test output into the PR description (or attach it),
and include a brief example of the expected success summary so reviewers can
confirm tests were run.
---
Nitpick comments:
In `@baml_language/crates/baml_tests/tests/interfaces.rs`:
- Around line 9497-9524: The test
union_fuzz_pr_reflection_nested_union_arg_order_insensitive currently only
checks implements(), so add assertions that pin the same nested-union
order-insensitivity through the reverse/registry reflection paths by invoking
implemented_by() and implementors() for the same types; specifically call
reflect.implemented_by(Box<(int | string)?>) and
reflect.implemented_by(Box<(string | int)?>) (or their runtime equivalents) and
also query reflect.implementors(fwd) and reflect.implementors(rev) to ensure
both directions report the UBox implementor, updating the test to assert those
results equal as well as the existing implements() checks.
🪄 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: 03599d01-6c08-488e-9448-bfded83bc22e
📒 Files selected for processing (4)
baml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_tests/tests/interfaces.rsbaml_language/crates/bex_vm/src/package_baml/type_class.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- baml_language/crates/bex_vm/src/package_baml/type_class.rs
- baml_language/crates/baml_compiler2_mir/src/lower.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
- resolve_union_member_ty: the out-of-body / blanket-registry fallback now guards `prepend_self_param_if_method` with an `is_method` check, mirroring the interface branch. A function-typed interface *field* satisfied via `implements I for T` keeps its declared signature instead of gaining a phantom `self`. (Surfaced in PR review; no behavior change today since the registry only resolves methods, but removes the latent asymmetry.) - exhaustiveness unit test `rustc_port_optional_pair_witnesses`: drop the `user.` prefix from the expected `missing:` witnesses, matching the no-`user.`-leak diagnostic fix (caught by the full `cargo test`, which exercises the baml_compiler2_tir crate's own unit tests). Test plan: full `mise exec -- cargo test --no-fail-fast` — 5010 passed, 0 failed across 154 binaries; `mise run clippy` clean; `mise run fmt`. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
While verifying the review note about class-union members backed by an
inherited `implements I {}` default (which already resolve correctly via the
in-body implements registry — not a bug), this surfaced a real adjacent
diagnostic bug: when two interfaces both declare the method, calling it
unqualified on a *union* of implementors collapsed to
`unknown | unknown is not a function` (E0006, leaking the internal sentinel)
instead of the E0121 ambiguity the single-class receiver reports.
- resolve_union_member_ty: detect a class member's method that is contributed by
≥2 interfaces and emit E0121 (with the `.as<I>.m()` hint), mirroring the
existing E0131 field-ambiguity handling.
- check_call_inner: when a union callee arm is Unknown/Error, the real error was
already reported during member resolution, so don't also emit a misleading
`NotCallable` (E0006).
Test plan: full `mise exec -- cargo test --no-fail-fast` — 5011 passed, 0 failed
across 154 binaries; `mise run clippy` clean; `mise run fmt`.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 2746-2761: Current code short-circuits on any Unknown/Error arm
(using expanded.iter().any(...)) which hides NotCallable errors for concrete
non-function arms; change the check to only suppress E0006 when every
non-function arm is already in recovery. Replace the any(...) condition with a
predicate that finds all arms that are not callable (e.g., filter out
function-typed arms) and uses all(...) to ensure each non-function arm matches
Ty::Unknown or Ty::Error before calling infer_args_for_recovery and returning
the CheckedCallInner Unknown; keep references to expanded, Ty::Unknown,
Ty::Error, infer_args_for_recovery, and CheckedCallInner when making this
change.
🪄 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: 7414451f-aedf-4baf-9ba6-51a5bfdf8f57
📒 Files selected for processing (2)
baml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_tests/tests/interfaces.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/crates/baml_tests/tests/interfaces.rs
Two issues from PR review (both reproduced and now covered by tests): - Reading an interface-member field through an *optional* union (`(Dog | Named)?` via `?.`) crashed the VM (`expected map, got instance`) because the union field/method dispatch matched only a bare `Union`. Added a `tir_union_members` helper that transparently unwraps `Optional` and applied it at all four union dispatch hooks (field + method, member-access + path forms). - `check_call_inner` suppressed the `NotCallable` (E0006) diagnostic on *any* Unknown/Error union-callee arm, hiding the genuinely-not-callable concrete arm in `int | <unresolved>`. Now it only suppresses when *every* non-function arm is already in recovery (so an ambiguous-method union still reports just E0121, while a concrete non-function arm is still diagnosed). Test plan: interfaces suite 376 passed; `mise run clippy` clean; `mise run fmt`. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2a26059. Configure here.

Summary
Fixes 17 bugs in BEP-044 interface unions, found by a union-focused fuzz of the language. Each was pinned as a failing
union_fuzz_fNN_*test incrates/baml_tests/tests/interfaces.rsand is now green. A class-only union method-dispatch case is also pinned as a regression guard.Two root causes dominated:
unknown | unknown is not a function— a method present on every member of a union containing an interface was rejected because the union-member probe resolved interface members to theTy::Unknownsentinel.expected map, got instance,tagged_int_add) — reading a field / narrowing a generic-interface match arm on a union dispatched incorrectly at runtime.Fixes
unknown | unknown.T | U(misuse → E0001); a genuinely ambiguous field view → E0131.Slot<int>) respects its type argument at runtime instead of matching every implementor of the bare interface.string + <non-object primitive>(int/float/bigint/bool/null) is a type error, not an inferredstringthat aborts the VM.string + uint8arraystays valid.Box<int|string>==Box<string|int>).<T extends I>(a: T) -> T { return a }compiles).let a: Animalover(Dog | Cat)?).implements I for <primitive>resolves for every primitive (not justint); union method-not-found blames only the genuinely-lacking member.user.package-prefix leak in match witnesses; uncovered interface members named instead of_; ambiguous union field → E0131; projection errors keep generic args (Cargo<int>).Snapshot + LSP expectation updates reflect the corrected diagnostics (no
user.leak;string + intnow E0004).Test plan
cargo test -p baml_tests— 2058 passed, 0 failedcargo test -p baml_lsp2_actions_tests— 369 passed, 0 failedcargo clippy --all-targets -- -D warnings— cleancargo fmt— cleanMerged latest
canary(salsa-memoization hang fix); the one conflict inlower.rswas resolved by keeping the generic-interface pattern routing on top of canary's borrowed-resolved_aliasesAPI.🤖 Generated with Claude Code
Note
High Risk
Touches MIR lowering, TIR inference, VM reflection, and pattern/match codegen for interfaces and unions—areas that previously caused VM crashes and silent wrong dispatch; regressions would affect runtime behavior and type soundness.
Overview
Fixes BEP-044 interface unions end-to-end: TIR no longer collapses interface arms to
unknown | unknownfor callable unions; MIR adds runtime class-tag dispatch for methods and fields when the receiver is a union (including optional-wrapped unions and interface members), including inherited defaults and interface field views. Generic interfaceis/match patterns and fast type-tag switches now respect type arguments soSlot<int>cannot captureSlot<string>values.TIR also gains union-member resolution with diagnostic rollback, E0121/E0131 for ambiguous shared implementors, out-of-body primitive members, bounded-generic
T <: T, match-arm overlap over optional unions, and stricterstring +rules for non-object primitives. VM reflection compares generic args with order-insensitive union equivalence. Diagnostics and exhaustiveness witnesses dropuser.prefixes and show user-facing type names. Largeunion_fuzz_fNN_*regression tests plus snapshot/LSP updates;.gitignoreaddsworkflow_scratch_files/.Reviewed by Cursor Bugbot for commit b535f94. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests
Chores