[codex] Add class generic bounds - #3672
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds BEP-044 per-class generic parameter bounds and enforces them across lowering (CST→AST→HIR), MIR propagation, TIR validation/subtyping, type-inference (GenericEnv), interface-rule matching, and comprehensive tests. ChangesGeneric Parameter Bounds Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
aaronvg
marked this pull request as ready for review
June 3, 2026 20:50
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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)
1739-1775:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror interface-owned generics in
enclosing_generic_params().This constructor now pulls interface generic params/bounds into
generic_param_bounds, butenclosing_generic_params()at Lines 5530-5550 still only returnsimplements_foror class + function generics. In a generic interface default method, any interface type var that later flows throughemit_interface_class_guard_branch,reflect.type_of<T>(), or otherTyTemplatelowering still resolves with the wrong param list and degrades toVoidinstead of aTypeArgRef, so the runtime dispatch/materialization path stays incorrect.Suggested fix
fn enclosing_generic_params(&self) -> Vec<baml_base::Name> { let Some(fl) = self.func_loc else { return Vec::new(); }; let item_tree = file_item_tree(self.db, fl.file(self.db)); let func_id = fl.id(self.db); if let Some(imp) = item_tree .implements_for .iter() .find(|imp| imp.methods.contains(&func_id)) { return imp.generic_params.clone(); } + if let Some(iface_data) = item_tree + .interfaces + .values() + .find(|iface_data| iface_data.default_methods.contains(&func_id)) + { + let mut params = iface_data.generic_params.clone(); + params.extend(item_tree[func_id].generic_params.iter().cloned()); + return params; + } let mut params: Vec<baml_base::Name> = item_tree .classes .values() .find(|class_data| class_data.methods.contains(&func_id)) .map(|class_data| class_data.generic_params.clone()) .unwrap_or_default(); params.extend(item_tree[func_id].generic_params.iter().cloned()); params }🤖 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 1739 - 1775, enclosing_generic_params() currently returns only implements_for or class+function generics, causing interface-owned generic params/bounds added earlier (in the bound_param_names/generic_param_bounds construction) to be omitted; update enclosing_generic_params() to mirror the same logic used when collecting bound_param_names—i.e., if the function scope's parent is a ScopeKind::Class or an Interface, include that parent type's generic_params (and their bounds) when building the returned Name list so interface type variables are visible to TyTemplate lowering and emit_interface_class_guard_branch/reflect.type_of<T>() paths; locate enclosing_generic_params(), extend it to check the parent scope for interface entries (similar to the bound_param_names/ bound_exprs collection) and append interface.generic_params before appending the function's generic_params so runtime materialization uses TypeArgRef instead of collapsing to Void.
🤖 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 11204-11215: The List and Map branches in is_subtype currently
treat containers as covariant by checking only one direction; change them to
enforce invariance by requiring mutual subtyping (i.e., both
self.is_subtype(sub_inner, sup_inner) && self.is_subtype(sup_inner, sub_inner))
for Ty::List/Ty::EvolvingList and similarly require mutual checks for both key
and value in Ty::Map/Ty::EvolvingMap so that List<T> and Map<K,V> are only
considered subtypes when their element/key/value types are equivalent under
mutual subtyping; update those match arms in builder.rs (the match handling
Ty::List/Ty::EvolvingList and Ty::Map/Ty::EvolvingMap) to use the bidirectional
checks instead of a single is_subtype call.
In `@baml_language/crates/baml_compiler2_tir/src/inference.rs`:
- Around line 1323-1330: The generic-bound diagnostics are currently anchored to
the enclosing scope span (func_data.span / ancestor_scope.range) when calling
apply_generic_env after extend_env_with_lambda_generics; change those calls to
pass the lambda's span instead—use the span returned or provided by
extend_env_with_lambda_generics (the lambda_span/local lambda_site span) as the
last argument to apply_generic_env so diagnostics originate at the lambda site;
update both occurrences (the call around apply_generic_env at the shown location
and the similar call around lines 1391-1398) to use that lambda span rather than
func_data.span/ancestor_scope.range.
- Around line 243-245: The ancestor traversal currently stops on the first
non-Let scope (the `break` in inference.rs when matching `ScopeKind::Let` vs
`_`), which prevents finding an enclosing `Function` if an intermediate `Block`
exists; update the loop so that instead of breaking on `_` you continue climbing
(e.g., set `current = scope.parent` and keep looping) until you either hit a
`ScopeKind::Function` or `None`, ensuring the enclosing function's
generics/bounds are discovered; adjust any handling around `current` and `scope`
so the loop exits only on `Function` or end-of-chain rather than any non-Let.
In `@baml_language/crates/baml_compiler2_tir/src/interfaces.rs`:
- Around line 1024-1035: The compatibility path currently bypasses generic bound
checks: update derive_compatibility_views and the compatibility recording so
that class-shaped rules are not blindly recorded in class_implements by
QualifiedTypeName alone; instead ensure ImplementsRegistry::type_implements
consults type_implements_interface_via_rule (or make the compatibility-recording
logic type-arg aware) so that queries for Class<BadArg> are validated against
the InterfaceImplRule.generic_param_bounds created earlier (reference
derive_compatibility_views, class_implements,
ImplementsRegistry::type_implements, QualifiedTypeName, and
type_implements_interface_via_rule).
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 1739-1775: enclosing_generic_params() currently returns only
implements_for or class+function generics, causing interface-owned generic
params/bounds added earlier (in the bound_param_names/generic_param_bounds
construction) to be omitted; update enclosing_generic_params() to mirror the
same logic used when collecting bound_param_names—i.e., if the function scope's
parent is a ScopeKind::Class or an Interface, include that parent type's
generic_params (and their bounds) when building the returned Name list so
interface type variables are visible to TyTemplate lowering and
emit_interface_class_guard_branch/reflect.type_of<T>() paths; locate
enclosing_generic_params(), extend it to check the parent scope for interface
entries (similar to the bound_param_names/ bound_exprs collection) and append
interface.generic_params before appending the function's generic_params so
runtime materialization uses TypeArgRef instead of collapsing to Void.
🪄 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: d1160320-790a-4385-971e-6de7941286a1
📒 Files selected for processing (8)
baml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_hir/src/item_tree.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/interfaces.rsbaml_language/crates/baml_tests/tests/interfaces_class_generics.rs
…bound-generics # Conflicts: # baml_language/crates/baml_compiler2_tir/src/inference.rs
⏭️ 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):
|
Binary size checks passed✅ 7 passed
Generated by |
Contributor
There was a problem hiding this comment.
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_tir/src/interfaces.rs (1)
567-593:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExclude bounded blanket rules from the compatibility view.
The new guard only protects
class_implements/implements_type_args. This branch still records bounded blanket rules inblanket_class_implements, butblanket_class_implements_interface()only checks class QTN + arity, so a rule likeimplements<T extends Named> Printable for Box<T>can still makeBox<int>look compatible through the legacy fast path.Suggested fix
match &rule.for_ty_pattern { Ty::Class(class_qtn, class_args, _) if matches!(rule.origin, InterfaceImplOrigin::OutOfBody) && class_args.iter().any(|arg| matches!(arg, Ty::TypeVar(..))) => { - views.blanket_class_implements.push(BlanketClassImpl { - class_qtn: class_qtn.clone(), - generic_params: rule.generic_params.clone(), - generic_param_bounds: rule.generic_param_bounds.clone(), - interface_qtn: iface_qtn.clone(), - interface_type_args: interface_type_args.clone(), - for_target_ty: rule.for_ty_pattern.clone(), - }); + if rule.generic_param_bounds.iter().all(Option::is_none) { + views.blanket_class_implements.push(BlanketClassImpl { + class_qtn: class_qtn.clone(), + generic_params: rule.generic_params.clone(), + generic_param_bounds: rule.generic_param_bounds.clone(), + interface_qtn: iface_qtn.clone(), + interface_type_args: interface_type_args.clone(), + for_target_ty: rule.for_ty_pattern.clone(), + }); + } }🤖 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 567 - 593, The branch that pushes into views.blanket_class_implements should skip rules with bounded generic params: when matching Ty::Class(...) in the rule.for_ty_pattern arm, check rule.generic_param_bounds (same condition used for class_implements) and only push the BlanketClassImpl into views.blanket_class_implements if all generic_param_bounds are None; this prevents bounded blanket rules (e.g., implements<T extends Named> ...) from being recorded and subsequently considered by blanket_class_implements_interface(), so update the logic around where BlanketClassImpl is created/inserted to include that guard.
🤖 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.
Outside diff comments:
In `@baml_language/crates/baml_compiler2_tir/src/interfaces.rs`:
- Around line 567-593: The branch that pushes into
views.blanket_class_implements should skip rules with bounded generic params:
when matching Ty::Class(...) in the rule.for_ty_pattern arm, check
rule.generic_param_bounds (same condition used for class_implements) and only
push the BlanketClassImpl into views.blanket_class_implements if all
generic_param_bounds are None; this prevents bounded blanket rules (e.g.,
implements<T extends Named> ...) from being recorded and subsequently considered
by blanket_class_implements_interface(), so update the logic around where
BlanketClassImpl is created/inserted to include that guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7bcf8930-835a-4a29-8b6d-f7e32f52575f
📒 Files selected for processing (4)
baml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/interfaces.rsbaml_language/crates/baml_tests/tests/interfaces_class_generics.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- baml_language/crates/baml_compiler2_tir/src/inference.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
baml_language/crates/baml_compiler2_mir/src/lower.rs (2)
7017-7032:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPropagate interface type args for non-class implementor defaults.
This path still hard-codes
frame_seed: CalleeFrameSeed::Static(Vec::new()), so an inherited generic interface default dispatched to a primitive / other non-class implementor still enters with an emptyframe.type_args. A default body that reads its enclosing interface generics (reflect.type_of<T>(), bounded member access onT, etc.) will still resolve them tounknownhere. Please seed this fromimplementor.iface_argswhen they are concrete, mirroring the class-implementor default path.🤖 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 7017 - 7032, The branch that builds InterfaceMethodCandidate for non-class implementors currently uses CalleeFrameSeed::Static(Vec::new()), which drops interface type args and causes interface-default bodies to see unknown generics; update the code in the block that calls resolve_type_implementor_method and constructs InterfaceMethodCandidate so that frame_seed is seeded from implementor.iface_args when those args are concrete (mirror the class-implementor default path), e.g. construct CalleeFrameSeed::Static with implementor.iface_args.clone() (or an empty Vec only if iface_args are not present), keeping guard as InterfaceDispatchGuard::Type(implementor.runtime_ty.clone()) and preserving item_ref.
7649-7708:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBlanket override methods still drop
implements_forgeneric args.The new
frame_seedplumbing stops at inherited defaults; dispatched out-of-body overrides still useStatic(Vec::new()). Those bodies lower againstenclosing_generic_params() == imp.generic_params, so any override that references its rule generics or their bounds still runs with no seededframe.type_argsand loses that runtime contract. This needs a seed inimp.generic_paramsorder rather than the class/interface order.🤖 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 7649 - 7708, The blanket-override path is leaving CalleeFrameSeed::Static(Vec::new()) causing out-of-body overrides to miss impl-level generic args; update the branch that constructs InterfaceMethodCandidate (both the earlier override case and this inherited default case) to compute frame_seed using the implementing impl's generic-parameter order (imp.generic_params / enclosing_generic_params() for the impl) instead of class/interface order: gather the resolved frame type args (currently in current_iface_args or the resolved closure view args) and reorder/map them into the impl's generic parameter order (use the impl/type-name info available via impl_tn or the impl's definition lookup) and pass CalleeFrameSeed::Static(mapped_args) when fully concrete, otherwise Vec::new(); keep using InterfaceMethodCandidate, InterfaceDispatchGuard::Class, and frame_seed = CalleeFrameSeed::Static(...) as the insertion point.
🤖 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.
Outside diff comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 7017-7032: The branch that builds InterfaceMethodCandidate for
non-class implementors currently uses CalleeFrameSeed::Static(Vec::new()), which
drops interface type args and causes interface-default bodies to see unknown
generics; update the code in the block that calls
resolve_type_implementor_method and constructs InterfaceMethodCandidate so that
frame_seed is seeded from implementor.iface_args when those args are concrete
(mirror the class-implementor default path), e.g. construct
CalleeFrameSeed::Static with implementor.iface_args.clone() (or an empty Vec
only if iface_args are not present), keeping guard as
InterfaceDispatchGuard::Type(implementor.runtime_ty.clone()) and preserving
item_ref.
- Around line 7649-7708: The blanket-override path is leaving
CalleeFrameSeed::Static(Vec::new()) causing out-of-body overrides to miss
impl-level generic args; update the branch that constructs
InterfaceMethodCandidate (both the earlier override case and this inherited
default case) to compute frame_seed using the implementing impl's
generic-parameter order (imp.generic_params / enclosing_generic_params() for the
impl) instead of class/interface order: gather the resolved frame type args
(currently in current_iface_args or the resolved closure view args) and
reorder/map them into the impl's generic parameter order (use the impl/type-name
info available via impl_tn or the impl's definition lookup) and pass
CalleeFrameSeed::Static(mapped_args) when fully concrete, otherwise Vec::new();
keep using InterfaceMethodCandidate, InterfaceDispatchGuard::Class, and
frame_seed = CalleeFrameSeed::Static(...) as the insertion point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 546ba8d6-8f9d-4598-87d4-0ddea5f40514
📒 Files selected for processing (1)
baml_language/crates/baml_compiler2_mir/src/lower.rs
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 196e71a. Configure here.
…bound-generics # Conflicts: # baml_language/crates/baml_compiler2_tir/src/builder.rs # baml_language/crates/baml_compiler2_tir/src/inference.rs
aaronvg
enabled auto-merge
June 3, 2026 23:56
This was referenced Jun 25, 2026
This was referenced Jul 14, 2026
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
1 participant
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.
Summary
Adds generic bounds to class generic parameters, including parsing/lowering support, TIR validation, and runtime lowering support for class methods that rely on bounded type variables.
What changed
GenericEnvpath.Root cause
Class generic parameters could be parsed as names, but their bounds were not represented consistently through the AST/HIR/TIR/MIR pipeline. Several TIR annotation paths also lowered types without validating class/interface generic argument bounds, and MIR method lowering did not carry enclosing class bounds when erasing bounded type variables for runtime field/member access.
Validation
cargo test -p baml_tests --test interfaces_class_genericscargo test -p baml_tests --test interfaces generic_boundcargo test -p baml_tests --test interfaces boundedcargo test -p baml_compiler2_tircargo test -p baml_tests --test interfacescargo test -p baml_compiler2_mirNote: the working tree still has unrelated pre-existing generated Go changes and
rig_tests/; they are not included in this PR.Note
High Risk
Large changes to generic bound propagation, subtyping, interface impl indexing, and MIR method lowering in the core compiler pipeline; mistakes could cause false positives/negatives in type-checking or runtime dispatch.
Overview
Class generic parameters now carry
extendsbounds end-to-end (AST/HIRgeneric_param_bounds, class lowering viaextract_generic_params_with_bounds), and TIR enforces those bounds wherever class/interface type arguments appear—not only at explicit call-site type args.TIR introduces a shared
GenericEnvfor merging enclosing class/interface/implementsgenerics with function and lambda scopes, validates bounds on resolved types (constructors, aliases, patterns, throws, interface signatures), and tightens subtyping (type-var reflexivity, structural checks through lists/maps/futures/functions, containerArray/Mapargs without spurious nominal rules). Interface compatibility now evaluates impl rules against lowered class bounds and only caches nominalclass_implementswhen bounds are empty.MIR accumulates generic bounds from
implementsblocks, parent class/interface scopes, and the function when lowering methods; interface impl inference also unifies associated-type bindings on interface-to-interface matches.New
interfaces_class_genericstests and extensions to associated-type / runtime dispatch tests cover bounded class type args and cross-namespace defaults.Reviewed by Cursor Bugbot for commit 4c7031a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Behavior Changes / Bug Fixes
Tests