Use indexed generic parameter identities - #4185
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR replaces name-based generic type-variable handling with indexed ChangesParamTy generic identity
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
⏭️ 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):
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs (1)
994-1036: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the interface and free-impl scope branches with regression tests.
The supplied regression covers a class-owned method, while this helper also adds interface
Selfand free-impl generics. Add focused cases for those branches, or verify existing tests cover them.🤖 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/lower_type_expr.rs` around lines 994 - 1036, Add regression coverage for function_in_scope_generic_params covering an interface-owned method, asserting interface generics followed by the implicit Self and function generics, and a free-impl method, asserting impl generics followed by function generics. Reuse existing test helpers and verify ordering matches the helper’s contract.baml_language/crates/baml_tests/tests/interfaces_class_generics.rs (1)
781-805: 📐 Maintainability & Code Quality | 🔵 TrivialRun
cargo test --libfrom the Cargo workspace root before merge.This PR touches Rust; running
cargo test --libfrom the appropriateCargo.tomldirectory needs to pass before the PR can land.
cargo test --libmust run from a directory containing the workspaceCargo.toml.🤖 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_class_generics.rs` around lines 781 - 805, Run cargo test --lib from the Rust workspace root containing the workspace Cargo.toml, and ensure the test suite passes before merging the change to method_generic_associated_projection_preserves_enclosing_class_param.Source: Coding guidelines
🤖 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_compiler2_tir/src/lower_type_expr.rs`:
- Around line 994-1036: Add regression coverage for
function_in_scope_generic_params covering an interface-owned method, asserting
interface generics followed by the implicit Self and function generics, and a
free-impl method, asserting impl generics followed by function generics. Reuse
existing test helpers and verify ordering matches the helper’s contract.
In `@baml_language/crates/baml_tests/tests/interfaces_class_generics.rs`:
- Around line 781-805: Run cargo test --lib from the Rust workspace root
containing the workspace Cargo.toml, and ensure the test suite passes before
merging the change to
method_generic_associated_projection_preserves_enclosing_class_param.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0caa1c9-20d7-44ac-92b9-ad0ccaa21898
📒 Files selected for processing (2)
baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_tests/tests/interfaces_class_generics.rs
Binary size checks passed✅ 7 passed
Generated by |
codeshaunted
force-pushed
the
avery/b-896
branch
from
July 24, 2026 22:53
7910dfa to
b4ef530
Compare
codeshaunted
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
baml_language/crates/baml_compiler2_tir/src/inference.rs (1)
1513-1514: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
.clone()onGenericEnv. The function/class/interface constructors returnGenericEnv<'db>by value, and the type derivesClone, so these.clone()calls are unnecessary no-ops.
baml_language/crates/baml_compiler2_tir/src/inference.rs#L1532-L1533baml_language/crates/baml_compiler2_tir/src/inference.rs#L2263-L2263baml_language/crates/baml_compiler2_tir/src/inference.rs#L2432-L2432🤖 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 1513 - 1514, Remove the redundant clone calls on GenericEnv returned by the constructors used in inference.rs. Update the sites at baml_language/crates/baml_compiler2_tir/src/inference.rs:1513-1514, 2245-2245, and 2413-2413 by using the returned value directly while preserving the existing generic environment 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_tir/src/generic_env.rs`:
- Around line 304-318: Update the free impl branch of GenericEnv::from_sources
to preserve every GenericParamData::bounds entry as an interface intersection
bound instead of using only bounds.first(). Reuse the existing multi-bound
lowering path used by the impl resolver, and apply the same correction in the
MIR generic scope builder so free impl generics retain all BEP-044 bounds.
---
Nitpick comments:
In `@baml_language/crates/baml_compiler2_tir/src/inference.rs`:
- Around line 1513-1514: Remove the redundant clone calls on GenericEnv returned
by the constructors used in inference.rs. Update the sites at
baml_language/crates/baml_compiler2_tir/src/inference.rs:1513-1514, 2245-2245,
and 2413-2413 by using the returned value directly while preserving the existing
generic environment behavior.
🪄 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 Plus
Run ID: bf973c19-3ba7-4b20-b740-5ab1478da908
📒 Files selected for processing (5)
baml_language/crates/baml_compiler2_tir/src/generic_env.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/lib.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_tests/tests/interfaces_class_generics.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/crates/baml_tests/tests/interfaces_class_generics.rs
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/sdks/csharp/sdkgen_csharp/src/semantic.rs (1)
797-808: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve
Ty::TypeVaridentity in C# generic resolution.The lookups resolve generated type variables by spelling alone while
Ty::TypeVaralso carries aParamTyindex. Namespaced generic code such asclass Foo<T> { ... class Bar<T> { fn f() -> List<T> } }or receiver-methoditerresults can therefore resolve the innerTto the wrong local type token. KeepParamTy/index identity throughtype_source_forand therender_token/substitution paths, and add a Rust unit test with shadowedTparameters.🤖 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/sdks/csharp/sdkgen_csharp/src/semantic.rs` around lines 797 - 808, Preserve the ParamTy/index identity when resolving Ty::TypeVar instead of matching only by spelling, and carry that identity through type_source_for plus the render_ token/substitution paths; update all affected sites in baml_language/sdks/csharp/sdkgen_csharp/src/semantic.rs at lines 797-808, 2528-2545, 2868-2875, 3179-3192, and 3965-3976. Ensure shadowed generic parameters and receiver-method iter results resolve to the correct C# type token, and add a Rust unit test covering nested or shadowed T parameters.
🧹 Nitpick comments (3)
baml_language/crates/baml_compiler2_tir/src/interfaces.rs (1)
1240-1252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive test params distinct indices so index identity is actually exercised.
param()pins every parameter to index0, so these tests only ever discriminateParamTyby name — the exact dimension this PR introduces (index identity) is untested. Distinct indices would catch a regression whereParamTyequality/hashing drops or mis-uses the index.♻️ Suggested helper
- fn param(name: &str) -> ParamTy { - ParamTy::new(0, Name::new(name)) - } + fn param_at(index: u32, name: &str) -> ParamTy { + ParamTy::new(index, Name::new(name)) + } + + fn param(name: &str) -> ParamTy { + // Stable per-name index so distinct params never share an index. + param_at(u32::from(name.as_bytes()[0]), name) + }As per coding guidelines, "Prefer writing Rust unit tests over integration tests where possible" — these in-crate tests are the right place for that coverage.
🤖 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 1240 - 1252, Update the test helper param and its call sites in the match_ty_pattern tests so each parameter uses a distinct index instead of param() always assigning index 0. Preserve the existing names and assertions while ensuring the tests exercise ParamTy identity by both index and name.Source: Coding guidelines
baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs (1)
1066-1077: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive method-generic
ParamTys from the generic environment instead of minting fresh indices.
ParamTyidentity matters for substitution and bindings, whileGenericEnvDatabuilds method defaults withParamTy::new(next_index, name). Usinginterface_env.param_count() + offsethere can produce differentindex/namepairs for the same method generic, and future interface-relatedParamTyindex changes would silently desynchronize this path. Use the shared helper/construction for all generic params instead.🤖 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 1066 - 1077, Update the method parameter construction in the interface resolution path to derive each generic ParamTy from the method’s GenericEnvData/shared generic-parameter helper rather than calculating indices from interface_env.param_count() and offset. Ensure the resulting index/name pairs exactly match the method defaults and remain synchronized with future ParamTy index changes, while preserving the existing extra_generic_names ordering.baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
3439-3452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSynthesized
Selfslot index has no corresponding runtime frame slot.When
Selfisn't already inenclosing_generic_params, this mintsParamTy::new(max+1, "Self"). That's fine for name resolution insidelower_type_expr, but the resultingTir2Ty::TypeVar(self_param, …)now carries an index that would map to a non-existentTypeArgRefif it ever reaches template lowering. Today it only flows intoconvert_tir_ty_for_runtime; a comment pinning that invariant (or a sentinel index) would keep the next refactor from silently emitting a bogus slot.🤖 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 3439 - 3452, Document the invariant at the synthesized Self parameter in the fallback branch of the generic-parameter lookup: its index is only for name resolution and must never be treated as a runtime frame or TypeArgRef slot during template lowering. Keep the existing index allocation and ensure the comment clearly identifies convert_tir_ty_for_runtime as the boundary where this assumption currently applies.
🤖 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 856-862: Update the Tir2Ty::TypeVar branch in tir2_to_template to
avoid asserting when the parameter is absent from generic_params. Return
TyTemplate::TypeArgRef(param.index()) for in-scope parameters and
TyTemplate::Wildcard for out-of-scope parameters, matching
tir2_to_dispatch_guard_template behavior.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 6757-6761: Update the function-type checking path around
surface_throws_ty so an omitted func_def.throws preserves contextual_throws
unchanged instead of using lambda_effective_throws. Keep the explicit throws_ty
branch intact, while allowing lambda_effective_throws to remain the body’s
effective boundary for inference.
In `@baml_language/crates/baml_type/src/normalize/tests.rs`:
- Around line 69-71: Update the test fixture’s type-variable bounds lookup in
type_var_bound to preserve the full ParamTy identity instead of reducing it to
param.name(). Change var_bounds and the typevar helper to use indexed ParamTy
keys, and construct explicit index/name pairs so same-named parameters with
different indices can be represented and tested.
In `@baml_language/sdks/rust/sdkgen_rust/src/lib.rs`:
- Around line 854-858: The typevar helper currently assigns every generic
variable to ParamTy slot 0, collapsing multi-parameter fixtures. Update typevar
and its call sites to accept and pass distinct slot indices, ensuring T and U
use separate ParamTy slots (for example, U uses 1), then run cargo test --lib
from the sdkgen_rust crate directory.
---
Outside diff comments:
In `@baml_language/sdks/csharp/sdkgen_csharp/src/semantic.rs`:
- Around line 797-808: Preserve the ParamTy/index identity when resolving
Ty::TypeVar instead of matching only by spelling, and carry that identity
through type_source_for plus the render_ token/substitution paths; update all
affected sites in baml_language/sdks/csharp/sdkgen_csharp/src/semantic.rs at
lines 797-808, 2528-2545, 2868-2875, 3179-3192, and 3965-3976. Ensure shadowed
generic parameters and receiver-method iter results resolve to the correct C#
type token, and add a Rust unit test covering nested or shadowed T parameters.
---
Nitpick comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 3439-3452: Document the invariant at the synthesized Self
parameter in the fallback branch of the generic-parameter lookup: its index is
only for name resolution and must never be treated as a runtime frame or
TypeArgRef slot during template lowering. Keep the existing index allocation and
ensure the comment clearly identifies convert_tir_ty_for_runtime as the boundary
where this assumption currently applies.
In `@baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs`:
- Around line 1066-1077: Update the method parameter construction in the
interface resolution path to derive each generic ParamTy from the method’s
GenericEnvData/shared generic-parameter helper rather than calculating indices
from interface_env.param_count() and offset. Ensure the resulting index/name
pairs exactly match the method defaults and remain synchronized with future
ParamTy index changes, while preserving the existing extra_generic_names
ordering.
In `@baml_language/crates/baml_compiler2_tir/src/interfaces.rs`:
- Around line 1240-1252: Update the test helper param and its call sites in the
match_ty_pattern tests so each parameter uses a distinct index instead of
param() always assigning index 0. Preserve the existing names and assertions
while ensuring the tests exercise ParamTy identity by both index and name.
🪄 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 Plus
Run ID: cec392b5-7c67-46ce-a1d5-351695710130
⛔ Files ignored due to path filters (4)
baml_language/sdks/go/baml_go/internal/cffi/baml_type.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/cffi/proto/baml_bridge/cffi/v1/baml_type.pb.gois excluded by!**/*.pb.gobaml_language/sdks/typescript/bridge_typescript/dist/proto/baml_cffi.d.tsis excluded by!**/dist/**baml_language/sdks/typescript/bridge_typescript/dist/proto/baml_cffi.jsis excluded by!**/dist/**
📒 Files selected for processing (61)
baml_language/crates/baml_codegen_types/src/ty.rsbaml_language/crates/baml_compiler2_emit/src/lib.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/callable.rsbaml_language/crates/baml_compiler2_tir/src/exhaustiveness.rsbaml_language/crates/baml_compiler2_tir/src/generic_env.rsbaml_language/crates/baml_compiler2_tir/src/generics.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/lib.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/self_type.rsbaml_language/crates/baml_compiler2_tir/src/throw_inference.rsbaml_language/crates/baml_compiler2_tir/src/ty.rsbaml_language/crates/baml_compiler2_tir/src/type_context.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_type/src/family.rsbaml_language/crates/baml_type/src/lib.rsbaml_language/crates/baml_type/src/normalize.rsbaml_language/crates/baml_type/src/normalize/tests.rsbaml_language/crates/baml_type/src/param.rsbaml_language/crates/baml_type/src/realized_ty.rsbaml_language/crates/baml_type/src/runtime_ty.rsbaml_language/crates/baml_type/src/template.rsbaml_language/crates/baml_type_runtime/src/lib.rsbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/trace_value_encode.rsbaml_language/crates/bex_vm/src/package_baml/resolve.rsbaml_language/crates/bex_vm/src/type_context.rsbaml_language/crates/bex_vm/src/type_match.rsbaml_language/crates/bex_vm_types/src/types/interface.rsbaml_language/crates/bridge_ctypes/src/ty_decode.rsbaml_language/crates/bridge_ctypes/src/ty_encode.rsbaml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_type.protobaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_type.pb.ccbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_type.pb.hbaml_language/sdks/csharp/sdkgen_csharp/src/semantic.rsbaml_language/sdks/go/sdkgen_go/src/lib.rsbaml_language/sdks/go/sdkgen_go/src/types.rsbaml_language/sdks/java/sdkgen_java/src/lib.rsbaml_language/sdks/java/sdkgen_java/src/translate_ty.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/lib.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/translate_ty.rsbaml_language/sdks/python/src/baml_bridge/cffi/v1/baml_type_pb2.pybaml_language/sdks/python/src/baml_bridge/cffi/v1/baml_type_pb2.pyibaml_language/sdks/rust/bridge_rust/src/wire/baml_bridge.cffi.v1.rsbaml_language/sdks/rust/sdkgen_rust/src/effect_rename.rsbaml_language/sdks/rust/sdkgen_rust/src/lib.rsbaml_language/sdks/swift/Sources/BamlBridge/Proto/.generated-frombaml_language/sdks/swift/Sources/BamlBridge/Proto/baml_type.pb.swiftbaml_language/sdks/typescript/bridge_typescript/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/typescript/bridge_typescript/typescript_src/proto/baml_cffi.jsbaml_language/sdks/typescript/sdkgen_typescript_shared/src/translate_ty.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/crates/baml_compiler2_tir/src/inference.rs
codeshaunted
changed the title
# Conflicts: # baml_language/crates/baml_compiler2_emit/src/lib.rs # baml_language/crates/baml_compiler2_mir/src/lower.rs # baml_language/crates/baml_compiler2_tir/src/builder.rs # baml_language/crates/baml_compiler2_tir/src/interfaces.rs # baml_language/crates/baml_compiler2_tir/src/interfaces/coherence.rs # baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs # baml_language/crates/baml_type/src/template.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 (1)
baml_language/crates/baml_compiler2_tir/src/unify.rs (1)
84-120: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTraverse associated-type projections when detecting bound variables.
Ty::AssociatedTypeProjectionfalls through tofalse. Consequently,try_union_set_equalitycan treat a union containing<T as I>::Itemas ground and returnNoagainst a concrete union, bypassingunify_into’s conservative projection-overlap rule. This can incorrectly classify overlapping impl subjects as disjoint.Proposed fix
Ty::Function { params, ret, throws, .. } => { params .iter() .any(|FunctionParamTy { ty, .. }| contains_bound_typevar(ty, generic_params)) || contains_bound_typevar(ret, generic_params) || contains_bound_typevar(throws, generic_params) } + Ty::AssociatedTypeProjection { + base, interface, .. + } => { + contains_bound_typevar(base, generic_params) + || interface + .tys() + .any(|ty| contains_bound_typevar(ty, generic_params)) + } _ => false,🤖 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/unify.rs` around lines 84 - 120, Update contains_bound_typevar to explicitly traverse Ty::AssociatedTypeProjection, checking its projected type and any associated-type arguments or bindings for generic parameters using the same recursive detection. Ensure projections containing a bound type variable return true so try_union_set_equality does not classify them as ground before unify_into applies its conservative overlap handling.
🤖 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/unify.rs`:
- Around line 84-120: Update contains_bound_typevar to explicitly traverse
Ty::AssociatedTypeProjection, checking its projected type and any
associated-type arguments or bindings for generic parameters using the same
recursive detection. Ensure projections containing a bound type variable return
true so try_union_set_equality does not classify them as ground before
unify_into applies its conservative overlap handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4803565c-df5b-4fdd-8c07-d21e2b44f867
📒 Files selected for processing (14)
baml_language/crates/baml_compiler2_emit/src/lib.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/exhaustiveness.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/lib.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/pattern_overlap.rsbaml_language/crates/baml_compiler2_tir/src/unify.rsbaml_language/crates/baml_lsp2_actions/src/check.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- baml_language/crates/baml_compiler2_tir/src/exhaustiveness.rs
- baml_language/crates/baml_compiler2_tir/src/lib.rs
- baml_language/crates/baml_lsp2_actions/src/check.rs
- baml_language/crates/baml_compiler2_tir/src/builder/associated_projection.rs
- baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs
- baml_language/crates/baml_compiler2_tir/src/interfaces.rs
- baml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rs
- baml_language/crates/baml_compiler2_emit/src/lib.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
- baml_language/crates/baml_compiler2_mir/src/lower.rs
- baml_language/crates/baml_compiler2_tir/src/inference.rs
Contributor
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_type/src/param.rs`:
- Around line 81-85: The slot_by_name lookup incorrectly selects the first
matching parameter when flattened frames contain shadowed owner and method
generics. Resolve slots from the indexed ParamTy wherever available; otherwise
make name-based lookup shadowing-aware or reject ambiguous matches, preserving
the inner parameter’s slot. Add a regression test covering outer and inner
parameters with the same name and verifying the inner parameter resolves
correctly.
🪄 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 Plus
Run ID: bc61caac-6f19-4e4a-81bc-da8bcd6a414f
📒 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_compiler2_tir/src/ty.rsbaml_language/crates/baml_type/src/param.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- baml_language/crates/baml_compiler2_tir/src/ty.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
- baml_language/crates/baml_compiler2_mir/src/lower.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 (1)
baml_language/crates/baml_type/src/param.rs (1)
34-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the appended index range against
u32overflow.
first_index + offsetis not validated as a sum. If the frame is nearu32::MAX, release builds can wrap indices and create duplicateParamTyidentities. Validate the complete appended range before extending the frame.Proposed fix
pub fn extend_frame(frame: &mut Vec<Self>, names: &[Name]) { let first_index = u32::try_from(frame.len()).expect("generic parameter count fits in u32"); + let count = u32::try_from(names.len()).expect("generic parameter count fits in u32"); + first_index + .checked_add(count) + .expect("generic parameter index range fits in u32"); frame.extend(names.iter().enumerate().map(|(offset, name)| { Self::new( - first_index + u32::try_from(offset).expect("generic parameter index fits in u32"), + first_index + .checked_add(u32::try_from(offset).expect("generic parameter index fits in u32")) + .expect("generic parameter index fits in u32"), name.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_type/src/param.rs` around lines 34 - 41, Update ParamTy::extend_frame to validate that the entire appended range, from frame.len() through frame.len() + names.len() - 1, fits within u32 before calling frame.extend. Reject overflow explicitly, then preserve the existing index generation for valid ranges.
🧹 Nitpick comments (1)
baml_language/crates/baml_type/src/param.rs (1)
102-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover direct indexed lookup in the shadowing regression test.
The test verifies
slot_by_name, but not thatslot(&outer)andslot(&inner)preserve distinct indexed identities. Add assertions for both direct lookups.🤖 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_type/src/param.rs` around lines 102 - 110, Extend the test frame_index_distinguishes_same_named_parameters to assert that direct layout lookups for both outer and inner parameters return their distinct indexed slots via slot(&outer) and slot(&inner), while preserving the existing slot_by_name shadowing assertion.
🤖 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_type/src/param.rs`:
- Around line 34-41: Update ParamTy::extend_frame to validate that the entire
appended range, from frame.len() through frame.len() + names.len() - 1, fits
within u32 before calling frame.extend. Reject overflow explicitly, then
preserve the existing index generation for valid ranges.
---
Nitpick comments:
In `@baml_language/crates/baml_type/src/param.rs`:
- Around line 102-110: Extend the test
frame_index_distinguishes_same_named_parameters to assert that direct layout
lookups for both outer and inner parameters return their distinct indexed slots
via slot(&outer) and slot(&inner), while preserving the existing slot_by_name
shadowing assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1af84eca-64c2-4893-bced-2d6d9679ad05
📒 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_type/src/param.rsbaml_language/crates/bex_vm/src/vm.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- baml_language/crates/baml_compiler2_mir/src/lower.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 (1)
baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
694-698: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve canonical generic-parameter order instead of iterating
FxHashMapkeys.Both helpers now derive
ScopeCtx.generic_paramsfrombindings.keys(). Hash-map iteration is not declaration/frame order, so nested parameters with the same name can resolve to the wrongParamTy; consumers that build runtime templates from this slice can also receive incorrect slot ordering. Pass the owning scope’s canonical ordered&[ParamTy]instead of reconstructing it from the map, and add a nested-shadowing unit test.Also applies to: 727-731
🤖 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 694 - 698, Update both helpers around the generic-parameter collection to accept and use the owning scope’s canonical ordered &[ParamTy] instead of deriving order from bindings.keys(). Preserve that slice when constructing ScopeCtx and when generating runtime templates so nested shadowed parameters resolve to the correct ParamTy and slot order; add a unit test covering nested generic-parameter shadowing.
🤖 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 694-698: Update both helpers around the generic-parameter
collection to accept and use the owning scope’s canonical ordered &[ParamTy]
instead of deriving order from bindings.keys(). Preserve that slice when
constructing ScopeCtx and when generating runtime templates so nested shadowed
parameters resolve to the correct ParamTy and slot order; add a unit test
covering nested generic-parameter shadowing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0871a30d-970f-48ee-bc61-83c416494f00
📒 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_type/src/param.rsbaml_language/crates/bex_vm/src/vm.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- baml_language/crates/baml_type/src/param.rs
- baml_language/crates/bex_vm/src/vm.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
# Conflicts: # baml_language/crates/baml_compiler2_mir/src/lower.rs
Contributor
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_tests/baml_src/ns_interfaces_class_generics/interfaces_class_generics.baml`:
- Around line 109-111: Update the regression function aem_accept_echo to invoke
the passed AemEcho<int> value’s echo operation, exercising the alpha-equivalent
generic echo<A, B extends AemEq<A>> implementation and its bound instead of
returning a constant. Preserve the function’s int result contract.
🪄 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 Plus
Run ID: 572b6689-b013-46cb-ab67-b0e426b33e59
⛔ Files ignored due to path filters (4)
baml_language/crates/baml_tests/snapshots/baml_src/interfaces_associated_types.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/interfaces_class_generics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lambdas.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
baml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_tests/baml_src/ns_interfaces_associated_types/interfaces_associated_types.bamlbaml_language/crates/baml_tests/baml_src/ns_interfaces_class_generics/interfaces_class_generics.bamlbaml_language/crates/baml_tests/baml_src/ns_lambdas/lambdas.bamlbaml_language/crates/baml_type/src/lib.rs
💤 Files with no reviewable changes (1)
- baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/crates/baml_type/src/lib.rs
- baml_language/crates/baml_lsp2_actions/src/check.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
- baml_language/crates/baml_compiler2_mir/src/lower.rs
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.
Issue Reference
Changes
GenericEnv, modeled after rustc'sty::Generics.GenericParam { index, name, bound }instead of parallel vectors and manual parent merges.Twhile loweringD extends Driver<Task<T>>, fixing the associated projection in B-896.Ty::TypeVarname-based; this PR does not change runtime types or serialization.Testing
cargo fmt --check --package baml_compiler2_tir --package baml_testscargo clippy -p baml_compiler2_tir --tests -- -D warningscargo test -p baml_compiler2_tir --lib(276 passed, 2 ignored)cargo test -p baml_tests --test interfaces_class_generics(34 passed)cargo test -p baml_tests --test interfaces_associated_types(130 passed, 1 ignored)cargo test -p baml_tests --test interfaces(486 passed, 2 ignored)Summary by CodeRabbit
New Features
throws.Bug Fixes
Self/frame handling.throws) inference.Tests