Repository navigation
fix: thread inferred generic type args through interface dispatch - #3667
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSeeds callee frames' generic type arguments per interface dispatch candidate by adding CalleeFrameSeed, threading frame_seed through candidate generation, emitting per-arm LoadType temps when needed, and updating dispatch and default-method lowering. Adds end-to-end tests exercising inferred/generic dispatch and default forwarding. ChangesInterface Dispatch Frame Type Arguments
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Switch as emit_method_candidate_switch
participant Lowering as emit_frame_type_arg_ops
participant VM
participant Callee
Caller->>Switch: invoke interface call lowering (site, explicit type args)
Switch->>Lowering: for Static frame_seed -> emit LoadType temps (per-Tir2Ty)
Lowering-->>Switch: return frame-type operands
Switch->>VM: call_with_type_args(frame-type-ops + explicit-type-ops, receiver/args)
VM->>Callee: enter callee frame with seeded frame.type_args
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 |
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
7356-7386:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't discard caller generics in blanket default candidates.
current_iface_argshere can already contain the caller's generic type vars afterinstantiate_rule_for_requested_interface()substitutes the blanket impl params. Treating any remainingTypeVaras "unseedable" makes calls likefn f<T>(b: Box<T>) { b.describe() }throughimplements<U> Named<U> for Box<U>still seed no frame args, so the inherited default seesTasunknown.🤖 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 7356 - 7386, The code currently drops all frame type args whenever current_iface_args contains any TypeVar, which loses caller generics produced by instantiate_rule_for_requested_interface(); instead, change the logic that builds frame_type_args so it only discards type variables that originate from the blanket impl’s own parameters (impl-level), not type variables that came from the caller; in other words, when constructing frame_type_args for InterfaceMethodCandidate, inspect each type argument in current_iface_args and retain those that are caller-generic while only removing/ignoring those proven to be impl-level (use the same origin/instantiation info produced by instantiate_rule_for_requested_interface() or equivalent metadata rather than a blanket baml_compiler2_tir::generics::contains_typevar check).
🤖 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 282-296: The helper fully_pinned_class_args currently returns an
empty Vec when the InterfaceClassGuard is Any or partially pinned, which causes
class_type_args to be dropped for generic overrides; change
fully_pinned_class_args to accept a fallback/runtime class args parameter (e.g.
runtime_class_args: Option<&[Tir2Ty]>) and when the guard is not fully pinned
return runtime_class_args cloned (when present) instead of Vec::new(); update
all call sites (including the class-owned override dispatch) to pass the
frame.type_args (or None) so concrete class_type_args are preserved for
reification.
- Around line 5549-5563: The direct call path for default.<method>() isn't
seeding the frame.type_args with the interface generic params, so generic params
from the interface become unknown at runtime; update the lowering that handles
direct default.<method>() invocations to mirror the interface-default lowering
here: when you detect a default method (via iface_data.default_methods and
func_id) seed the call frame's type args with iface_data.generic_params extended
by the function's own generic params (same ordering used by
emit_method_candidate_switch), ensuring frame.type_args is populated before
emitting the default method body.
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 7356-7386: The code currently drops all frame type args whenever
current_iface_args contains any TypeVar, which loses caller generics produced by
instantiate_rule_for_requested_interface(); instead, change the logic that
builds frame_type_args so it only discards type variables that originate from
the blanket impl’s own parameters (impl-level), not type variables that came
from the caller; in other words, when constructing frame_type_args for
InterfaceMethodCandidate, inspect each type argument in current_iface_args and
retain those that are caller-generic while only removing/ignoring those proven
to be impl-level (use the same origin/instantiation info produced by
instantiate_rule_for_requested_interface() or equivalent metadata rather than a
blanket baml_compiler2_tir::generics::contains_typevar check).
🪄 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: 69ddb0bc-6bae-4c9c-b152-443678c84442
⛔ Files ignored due to path filters (2)
baml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/inferred_generic_type_args.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
baml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_tests/baml_src/ns_inferred_generic_type_args/inferred_generic_type_args.baml
hellovai
force-pushed
the
hellovai/fix-generic-typearg-dispatch
branch
from
June 3, 2026 19:31
eb5e4c6 to
5f85c2d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 7712-7729: The assoc matcher interface_tir_assoc_match is too
strict: when a requested associated type is a TypeVar (e.g., Item = T) it must
be treated as a wildcard that matches any implementor binding instead of
requiring the impl assoc to be the same TypeVar; update
interface_tir_assoc_match so that inside the is_some_and closure it first checks
whether requested_ty is a TypeVar and if so returns true, otherwise fall back to
calling baml_compiler2_tir::normalize::is_same_normalized_type(impl_ty,
requested_ty, &self.resolved_aliases.aliases). Ensure you reference the Tir2Ty
representation for the type-var check and keep the rest of the matching logic
unchanged.
- Around line 959-967: The default associated type is being lowered with the
caller's pkg_items which causes package-local names to resolve incorrectly;
update the call to baml_compiler2_tir::generics::lower_type_expr_with_generics
so it passes the target interface's PackageItems (the package items belonging to
target_iface_pkg / the interface's package) instead of the local pkg_items when
lowering assoc.default.expr, and apply the identical change in the duplicated
logic inside resolve_implements_target_view so both sites use the target
interface's package items (keep passing the same namespace_path, bindings, and
diags).
🪄 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: 4e755e68-14e7-430c-b3ad-c939a10e87a2
⛔ Files ignored due to path filters (2)
baml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/inferred_generic_type_args.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
baml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_tests/baml_src/ns_inferred_generic_type_args/inferred_generic_type_args.baml
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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 7712-7729: The assoc matcher interface_tir_assoc_match is too
strict: when a requested associated type is a TypeVar (e.g., Item = T) it must
be treated as a wildcard that matches any implementor binding instead of
requiring the impl assoc to be the same TypeVar; update
interface_tir_assoc_match so that inside the is_some_and closure it first checks
whether requested_ty is a TypeVar and if so returns true, otherwise fall back to
calling baml_compiler2_tir::normalize::is_same_normalized_type(impl_ty,
requested_ty, &self.resolved_aliases.aliases). Ensure you reference the Tir2Ty
representation for the type-var check and keep the rest of the matching logic
unchanged.
- Around line 959-967: The default associated type is being lowered with the
caller's pkg_items which causes package-local names to resolve incorrectly;
update the call to baml_compiler2_tir::generics::lower_type_expr_with_generics
so it passes the target interface's PackageItems (the package items belonging to
target_iface_pkg / the interface's package) instead of the local pkg_items when
lowering assoc.default.expr, and apply the identical change in the duplicated
logic inside resolve_implements_target_view so both sites use the target
interface's package items (keep passing the same namespace_path, bindings, and
diags).
🪄 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: 4e755e68-14e7-430c-b3ad-c939a10e87a2
⛔ Files ignored due to path filters (2)
baml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/inferred_generic_type_args.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
baml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_tests/baml_src/ns_inferred_generic_type_args/inferred_generic_type_args.baml
🛑 Comments failed to post (2)
baml_language/crates/baml_compiler2_mir/src/lower.rs (2)
959-967:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve default associated types with the target interface's package items.
default.exprbelongs to the interface declared attarget_loc, but this path still lowers it with the caller'spkg_items. For imported interfaces, package-local names in the default will resolve against the implementor package, degrade toUnknown, and then assoc-based guard/method matching can silently miss valid implementors. Please use the target interface'sPackageItemshere, and mirror the same fix in the duplicated logic inresolve_implements_target_view.🤖 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 959 - 967, The default associated type is being lowered with the caller's pkg_items which causes package-local names to resolve incorrectly; update the call to baml_compiler2_tir::generics::lower_type_expr_with_generics so it passes the target interface's PackageItems (the package items belonging to target_iface_pkg / the interface's package) instead of the local pkg_items when lowering assoc.default.expr, and apply the identical change in the duplicated logic inside resolve_implements_target_view so both sites use the target interface's package items (keep passing the same namespace_path, bindings, and diags).
7712-7729:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat requested associated
TypeVars like wildcard interface args during matching.This helper is stricter than
interface_tir_type_args_match_preserving_typevars: a requested assoc binding such asItem = Tonly matches an implementor whose assoc is literally the sameTypeVar, so generic callers can lose primitive/out-of-body candidates before dispatch even checks the runtime receiver. The assoc matcher needs the same “requested typevar matches any concrete implementor binding” rule as the type-arg matcher.🤖 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 7712 - 7729, The assoc matcher interface_tir_assoc_match is too strict: when a requested associated type is a TypeVar (e.g., Item = T) it must be treated as a wildcard that matches any implementor binding instead of requiring the impl assoc to be the same TypeVar; update interface_tir_assoc_match so that inside the is_some_and closure it first checks whether requested_ty is a TypeVar and if so returns true, otherwise fall back to calling baml_compiler2_tir::normalize::is_same_normalized_type(impl_ty, requested_ty, &self.resolved_aliases.aliases). Ensure you reference the Tir2Ty representation for the type-var check and keep the rest of the matching logic unchanged.
A generic class instance whose `<T>` is *inferred* (not written explicitly) must carry the correct runtime `class_type_args`, and a method dispatched through a multi-implementor interface must receive the resolved class/interface type args in its frame. Previously the interface-dispatch switch (`emit_method_candidate_switch`) called the matched implementor's method with only the explicit call-site `<...>` type args, never seeding the callee frame. The effect: an interface **default method** (e.g. a lazy iterator's `map` building `Map<T, U>`), or any dispatched method that reads its enclosing `T` at runtime, saw empty `frame.type_args` -> built instances with `class_type_args = [unknown]` -> the downstream `IsType` dispatch guard fell through to the WRONG implementor (a silent wrong answer, or a field-access panic when the sibling has a different layout). This blocked the `baml.iter` lazy-iterator protocol. Fix: - `enclosing_generic_params()` now includes the interface's generic params for interface default methods (lowered as standalone functions whose bodies reference the interface's `T`; without this `TypeVar(T)` lowered to `Concrete(Void)` instead of `TypeArgRef(N)`). - The interface-dispatch switch seeds each candidate's callee frame (De Bruijn order: class/iface params, then explicit call-site args). A class-owned method gets the implementor's class args — statically when the guard pins them, otherwise from the matched runtime instance via a `BoundMethod` (covering `Any`/partial guards, e.g. a generic class behind a non-generic interface). An inherited default gets the resolved interface args. - `default.<method>()` (super-like) forwarding into a generic interface default seeds the default frame with the interface args too. Regression coverage: new in-BAML namespace `crates/baml_tests/baml_src/ns_inferred_generic_type_args` with a lazy-cursor showcase plus focused cases (inferred static ctor, interface default reading T, class-owned method building a generic, `default.<method>()` forwarding, and a generic class behind a non-generic interface). Full suite green; existing bytecode snapshots unchanged (no existing namespace exercised this path). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
hellovai
force-pushed
the
hellovai/fix-generic-typearg-dispatch
branch
from
June 3, 2026 21:18
5f85c2d to
2a53895
Compare
Contributor
Author
|
@coderabbitai — re: the two associated-types findings (the We're intentionally not addressing these in this PR. Both are valid-looking observations, but they belong to the BEP-57 associated-types feature ( Folding an associated-types fix into this PR would expand the diff into a separate feature, blur the change's purpose, and would need dedicated cross-package / generic-caller assoc fixtures to validate properly. We'll track them separately for the associated-types owner. The two findings that were in this PR's scope — the |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Would you like me to open a GitHub issue to track those two associated-types findings for the BEP-57 owner? ✏️ Learnings added
🧠 Learnings used |
Contributor
Author
|
@CodeRabbit, not, but can you share a small repro in BAML code that might trigger the issue? |
hellovai
enabled auto-merge
June 3, 2026 21:23
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find BAML files related to associated types
fd -e baml | xargs grep -l "assoc\|type Item\|associated" 2>/dev/null | head -20
# Look for assoc-related test fixtures
fd -e baml -p "assoc" | head -20
fd "assoc" --type f | head -20Length of output: 745 🏁 Script executed: #!/bin/bash
# Look at the lower.rs context around line 959-967 to understand what assoc.default does
sed -n '940,990p' baml_language/crates/baml_compiler2_mir/src/lower.rs 2>/dev/null || \
fd lower.rs -p baml_compiler2_mir | xargs -I{} sed -n '940,990p' {}Length of output: 2468 🏁 Script executed: #!/bin/bash
# Look at interface_tir_assoc_match around line 7712-7729
fd lower.rs -p baml_compiler2_mir | xargs -I{} sed -n '7700,7740p' {}Length of output: 488 🏁 Script executed: #!/bin/bash
# Read the associated types test file
cat baml_language/crates/baml_tests/tests/interfaces_associated_types.rs | head -200Length of output: 5587 🏁 Script executed: #!/bin/bash
# Find and read the lower.rs file properly
LOWER=$(fd "lower.rs" baml_language/ | grep baml_compiler2_mir | head -1)
echo "Found: $LOWER"
# Look at interface_tir_assoc_match
grep -n "interface_tir_assoc_match" "$LOWER" | head -10
sed -n '7700,7750p' "$LOWER"Length of output: 2454 🏁 Script executed: #!/bin/bash
# Find baml fixtures related to associated types
fd -e baml baml_language/crates/baml_tests/ | xargs grep -l "assoc\|type [A-Z]" 2>/dev/null | head -10
# Also look for any .baml files in projects related to associated types
fd -e baml baml_language/ | xargs grep -l "impl.*assoc\|type [A-Z][a-z]*\b" 2>/dev/null | head -10Length of output: 1373
Issue 1 —
|
This was referenced Jun 5, 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.
Problem
A generic class instance whose
<T>is inferred (not written explicitly) must carry the correct runtimeclass_type_args, and a method dispatched through a multi-implementor interface must receive the resolved class/interface type args in its frame.The interface-dispatch switch (
emit_method_candidate_switch) called the matched implementor's method with only the explicit call-site<...>type args — it never seeded the callee frame. So an interface default method (e.g. a lazy iterator'smapbuildingMap<T, U>), or any dispatched method that reads its enclosingTat runtime (reflect.type_of<T>(), constructingOther<T>{}), saw an emptyframe.type_args. It then built instances withclass_type_args = [unknown], and the downstreamIsTypedispatch guard fell through to the wrong implementor — a silent wrong answer, or a field-access panic when the sibling class has a different layout.This was the core compiler blocker for the
baml.iterlazy-iterator protocol.Minimal reproduction (before this fix)
Before:
take_one()(a default method) had emptyframe.type_args, soSingleCursor<T>was built withclass_type_args = [unknown].single.head()then failed theSingleCursor<int>guard and dispatched toArrayCursor.head, which indexed a non-existentarrfield. After: returns10.Fix (in
baml_compiler2_mir::lower)enclosing_generic_params()now includes the interface's generic params for interface default methods. They are lowered as standalone functions whose bodies reference the interface'sT; without this,TypeVar(T)lowered toConcrete(Void)instead ofTypeArgRef(N).The interface-dispatch switch seeds each candidate's callee frame (De Bruijn order: class/iface params first, then explicit call-site args):
BoundMethod(coversAny/partial guards, e.g. a generic class behind a non-generic interface);implementsblock renames/reorders params).default.<method>()(the super-like form that forwards into the interface default) seeds the default frame with the interface's type args too.Tests
New in-BAML regression namespace
crates/baml_tests/baml_src/ns_inferred_generic_type_args:showcase_lazy_cursor_pipeline— the lazy-cursor example above, end-to-end.inferred_static_ctor_dispatches_to_correct_implementor— inferred static ctor + non-generic interface.inferred_static_ctor_dispatches_through_generic_interface— inferred static ctor + generic interface, mismatched field shapes.interface_default_method_sees_interface_type_var— default method readsTviareflect.type_of<T>()at runtime.class_method_builds_generic_via_class_type_var— class-owned method builds a generic from its classT.default_method_forward_reads_interface_type_var—default.<method>()forwarding into a generic default.generic_class_behind_nongeneric_interface_reads_type_var—Any-guard case, seeded from the runtime instance.Full
baml_testssuite green. Existing bytecode snapshots unchanged — only the new namespace's.snapchanged (no existing namespace exercised this path; theBoundMethodpath fires only for previously-brokenAny/partial-guard generic arms).Known limitation (separate, pre-existing — not addressed here)
A generic class whose only field is
T[], built via a static constructor with inferredT, gets wrongclass_type_argsfrom the producer side (a TIR inference issue in the struct literal, independent of this dispatch fix). Adding any second field (as every real iterator has, e.g.idx: int) avoids it, so it does not affect theArrayIterator-shaped use case. Worth a separate investigation.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests