Repository navigation
Function types must be fully resolved - #3829
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughOops — WalkthroughGeneric parameters and bounds are removed from function value types ( ChangesRealized function values without generic binders
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
⏭️ 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):
|
3762b30 to
79a81af
Compare
|
No description provided. |
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_type/src/lib.rs (1)
819-823: 🎯 Functional Correctness | 🟠 MajorApply the proposed fix to handle
Ty::Voidasneverin Functionthrowsrendering.The
render_with()method for Function types at lines 819-823 passesthrowsthroughrender_with(), which rendersTy::Voidas"void". However, the Display implementation (lines 938-948) correctly special-cases this to render as"never". This inconsistency meansrender_with()(used byrender_canonical()andrender_user_facing()) will producethrows voidwhile Display producesthrows never.The proposed fix is correct and should be applied to ensure consistency between the two rendering paths.
🤖 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/lib.rs` around lines 819 - 823, In the render_with() method for Function types around the format string with parameters and return type, the throws rendering is using throws.render_with(s) which incorrectly renders Ty::Void as "void". Instead, special-case the Ty::Void type to render as "never" (matching the Display implementation behavior), so that throws void consistently renders as "throws never" across all rendering paths including render_canonical() and render_user_facing().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.
Inline comments:
In `@baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs`:
- Around line 2529-2546: The validation logic that checks for non-path bases
with type arguments (the parenthesized_base and path_base checks shown in the
diff) only applies after wrap_generic_apply is called, but keyword-literal bases
like true, false, and null return early from lower_path_expr before reaching
this validation. Ensure that expressions like true<int> properly emit the
TypeArgsOnNonPathBase diagnostic by routing keyword-literal bases through the
same validation logic that checks parenthesized_base and path_base conditions,
either by moving the early returns in lower_path_expr to after the validation
check, or by applying the validation check before those early returns occur.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 6434-6436: Remove the `expected_for_check.is_none()` guard from
the condition that checks
`self.references_unspecialized_generic_function(init)`. The
`references_unspecialized_generic_function` method should be called
unconditionally to reject bare generic function references regardless of whether
an expected type is present, ensuring that all unspecialized generic function
values are caught and properly rejected with the
`GenericFunctionValueNotSpecialized` error.
- Around line 12564-12572: In the code block where
self.interface_method_generic_params is being inserted (around the return of
Some(fn_ty)), this branch is missing the owner type-arg seeding that the earlier
interface-method branch performs. Before the return Some(fn_ty) statement, add a
call to record owner_type_arg_binding_seed (similar to what the earlier
interface-method branch does) to ensure bounds on required interface methods
that reference interface owner params maintain the concrete owner binding during
call-site validation.
- Around line 2116-2123: The bindings used in the subtype check for generic
bounds do not include owner parameter bindings, which are necessary to properly
substitute owner references (such as C in bounds like U extends Eq<C>) before
checking the bounds. Seed the bindings map with owner parameter bindings before
it is used in the substitute_ty call within the loop. Ensure that owner params
are added to bindings first, and then the explicitly supplied method params are
bound on top of those owner bindings, so that when substitute_ty is called on
the bound_ty, all referenced parameters including owner params are properly
substituted.
---
Outside diff comments:
In `@baml_language/crates/baml_type/src/lib.rs`:
- Around line 819-823: In the render_with() method for Function types around the
format string with parameters and return type, the throws rendering is using
throws.render_with(s) which incorrectly renders Ty::Void as "void". Instead,
special-case the Ty::Void type to render as "never" (matching the Display
implementation behavior), so that throws void consistently renders as "throws
never" across all rendering paths including render_canonical() and
render_user_facing().
🪄 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: da160919-94a4-47c0-9bc9-ca0801b75598
⛔ Files ignored due to path filters (25)
baml_language/crates/baml_tests/snapshots/baml_src/instantiation_expr.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__01_lexer__lambda_advanced.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__02_parser__lambda_advanced.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__10_formatter__lambda_advanced.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_basic/baml_tests__compiles__lambda_basic__01_lexer__lambda_basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_basic/baml_tests__compiles__lambda_basic__02_parser__lambda_basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_basic/baml_tests__compiles__lambda_basic__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_basic/baml_tests__compiles__lambda_basic__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_basic/baml_tests__compiles__lambda_basic__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_basic/baml_tests__compiles__lambda_basic__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_basic/baml_tests__compiles__lambda_basic__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_basic/baml_tests__compiles__lambda_basic__10_formatter__lambda_basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__01_lexer__lambda_fat_arrow.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__02_parser__lambda_fat_arrow.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__10_formatter__lambda_fat_arrow.snapis excluded by!**/*.snap
📒 Files selected for processing (38)
baml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_ast/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_ast/src/lowering_diagnostic.rsbaml_language/crates/baml_compiler2_hir/src/signature.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/associated_projection.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/generics.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/interfaces.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/normalize.rsbaml_language/crates/baml_compiler_parser/src/parser.rsbaml_language/crates/baml_exec/src/auto_cli.rsbaml_language/crates/baml_fmt/src/lib.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/utils.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/on_hover/generic_function_and_method_references.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/on_hover/stdlib_iterator_inference.bamlbaml_language/crates/baml_tests/baml_src/ns_instantiation_expr/instantiation_expr.bamlbaml_language/crates/baml_tests/projects/compiles/lambda_advanced/lambda_advanced.bamlbaml_language/crates/baml_tests/projects/compiles/lambda_basic/lambda_basic.bamlbaml_language/crates/baml_tests/projects/compiles/lambda_fat_arrow/lambda_fat_arrow.bamlbaml_language/crates/baml_tests/src/compiler2_tir/explicit_type_args.rsbaml_language/crates/baml_tests/tests/interfaces.rsbaml_language/crates/baml_tests/tests/interfaces_class_generics.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/realized_ty.rsbaml_language/crates/baml_type/src/runtime_ty.rsbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_external_types/src/host_return.rsbaml_language/crates/bex_vm/src/package_baml/resolve.rsbaml_language/crates/bridge_ctypes/src/ty_decode.rs
💤 Files with no reviewable changes (15)
- baml_language/crates/bex_engine/src/conversion.rs
- baml_language/crates/baml_type/src/normalize.rs
- baml_language/crates/bex_external_types/src/host_return.rs
- baml_language/crates/baml_tests/projects/compiles/lambda_basic/lambda_basic.baml
- baml_language/crates/baml_tests/projects/compiles/lambda_advanced/lambda_advanced.baml
- baml_language/crates/baml_compiler2_ast/src/lower_cst.rs
- baml_language/crates/baml_compiler2_ast/src/lib.rs
- baml_language/crates/baml_type/src/realized_ty.rs
- baml_language/crates/baml_tests/projects/compiles/lambda_fat_arrow/lambda_fat_arrow.baml
- baml_language/crates/baml_compiler2_tir/src/associated_projection.rs
- baml_language/crates/baml_exec/src/auto_cli.rs
- baml_language/crates/bridge_ctypes/src/ty_decode.rs
- baml_language/crates/baml_compiler2_mir/src/lower.rs
- baml_language/crates/baml_tests/baml_src/ns_instantiation_expr/instantiation_expr.baml
- baml_language/crates/baml_compiler2_tir/src/normalize.rs
| // Only a bare path reference to a generic function may be specialized into | ||
| // a value (`foo<int>`, `a.b.foo<int>`). A *parenthesized* base (`(foo)<int>`) | ||
| // is rejected even though its inner expression is a path — the paren lowers | ||
| // transparently, so it must be detected on the CST node — as is any other | ||
| // non-path base (`(a + b)<int>`, `g().foo<int>`). | ||
| let parenthesized_base = node.children().any(|n| n.kind() == SyntaxKind::PAREN_EXPR); | ||
| let path_base = matches!(self.exprs[base], Expr::Path(_)); | ||
| if parenthesized_base || !path_base { | ||
| self.diags | ||
| .push(LoweringDiagnostic::TypeArgsOnNonPathBase { span: range }); | ||
| // Recover by still specializing the inner reference when it is a path | ||
| // (`(foo)<int>`), so a downstream "must be specialized" error does not | ||
| // pile on; a genuinely non-path base cannot be specialized. | ||
| if !path_base { | ||
| return base; | ||
| } | ||
| } | ||
| self.alloc_expr(Expr::GenericApply { base, type_args }, range) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Route keyword-literal bases through this validation.
Line 2535 only rejects non-path bases after wrap_generic_apply is called, but lower_path_expr returns early for true, false, and null before that call. A value like true<int> can silently lower to true and drop the type arguments instead of emitting this diagnostic.
🐛 Proposed fix
@@
if segments.len() == 1 {
- match segments[0].0.as_str() {
- "true" => {
- return self.alloc_expr(Expr::Literal(Literal::Bool(true)), node.text_range());
- }
- "false" => {
- return self.alloc_expr(Expr::Literal(Literal::Bool(false)), node.text_range());
- }
- "null" => return self.alloc_expr(Expr::Null, node.text_range()),
- _ => {}
- }
+ let literal = match segments[0].0.as_str() {
+ "true" => Some(Expr::Literal(Literal::Bool(true))),
+ "false" => Some(Expr::Literal(Literal::Bool(false))),
+ "null" => Some(Expr::Null),
+ _ => None,
+ };
+ if let Some(expr) = literal {
+ let id = self.alloc_expr(expr, node.text_range());
+ return self.wrap_generic_apply(node, id, node.text_range());
+ }
}🤖 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_ast/src/lower_expr_body.rs` around lines
2529 - 2546, The validation logic that checks for non-path bases with type
arguments (the parenthesized_base and path_base checks shown in the diff) only
applies after wrap_generic_apply is called, but keyword-literal bases like true,
false, and null return early from lower_path_expr before reaching this
validation. Ensure that expressions like true<int> properly emit the
TypeArgsOnNonPathBase diagnostic by routing keyword-literal bases through the
same validation logic that checks parenthesized_base and path_base conditions,
either by moving the early returns in lower_path_expr to after the validation
check, or by applying the validation check before those early returns occur.
| let bindings = crate::generics::bind_type_vars(&generic_params, &resolved); | ||
|
|
||
| // BEP-044 generic-bound enforcement: each supplied type arg must satisfy | ||
| // its param's bound (mirrors the call-site check in | ||
| // `resolve_explicit_type_args`). The bounds are already lowered on | ||
| // `base_ty`; substitute the bindings so self-referential bounds | ||
| // (`<T: Container<T>>`) resolve before the subtype check. | ||
| // Generic-bound enforcement: each supplied type arg must satisfy its | ||
| // param's declared bound. Substitute the bindings first so self-referential | ||
| // bounds (`<T extends Container<T>>`) resolve before the subtype check. | ||
| for (idx, resolved_arg) in resolved.iter().enumerate() { | ||
| if let Some(Some(bound)) = generic_param_bounds.get(idx) { | ||
| let bound_ty = crate::generics::substitute_ty(bound, &bindings); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Seed owner bindings before checking explicit generic bounds.
For bound methods, generic_param_bounds may reference owner params like C in <U extends Eq<C>>, but bindings currently contains only explicitly supplied method params. That leaves owner params unsubstituted during this subtype check.
Proposed fix
- let bindings = crate::generics::bind_type_vars(&generic_params, &resolved);
+ let mut bindings = crate::generics::bind_type_vars(&generic_params, &resolved);
+ if let Some(seed) = self.owner_type_arg_binding_seed.get(&base) {
+ for (name, ty) in seed {
+ bindings
+ .entry(name.clone())
+ .or_insert_with(|| ty.clone());
+ }
+ }
// Generic-bound enforcement: each supplied type arg must satisfy its🤖 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.rs` around lines 2116 -
2123, The bindings used in the subtype check for generic bounds do not include
owner parameter bindings, which are necessary to properly substitute owner
references (such as C in bounds like U extends Eq<C>) before checking the
bounds. Seed the bindings map with owner parameter bindings before it is used in
the substitute_ty call within the loop. Ensure that owner params are added to
bindings first, and then the explicitly supplied method params are bound on top
of those owner bindings, so that when substitute_ty is called on the bound_ty,
all referenced parameters including owner params are properly substituted.
| let ty = if expected_for_check.is_none() | ||
| && self.references_unspecialized_generic_function(init) | ||
| { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject bare generic function references even with an expected type.
The expected_for_check.is_none() guard lets typed initializers bypass GenericFunctionValueNotSpecialized even though the initializer is still a bare generic free-function value. If explicit specialization is required, check references_unspecialized_generic_function(init) unconditionally here.
Proposed fix
- let ty = if expected_for_check.is_none()
- && self.references_unspecialized_generic_function(init)
- {
+ let ty = if self.references_unspecialized_generic_function(init) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let ty = if expected_for_check.is_none() | |
| && self.references_unspecialized_generic_function(init) | |
| { | |
| let ty = if self.references_unspecialized_generic_function(init) { |
🤖 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.rs` around lines 6434 -
6436, Remove the `expected_for_check.is_none()` guard from the condition that
checks `self.references_unspecialized_generic_function(init)`. The
`references_unspecialized_generic_function` method should be called
unconditionally to reject bare generic function references regardless of whether
an expected type is present, ensuring that all unspecialized generic function
values are caught and properly rejected with the
`GenericFunctionValueNotSpecialized` error.
| self.interface_method_generic_params.insert( | ||
| at, | ||
| ( | ||
| member.clone(), | ||
| function_generic_params, | ||
| function_generic_param_bounds, | ||
| ), | ||
| ); | ||
| return Some(fn_ty); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Mirror owner type-arg seeding in this interface-method path.
The earlier interface-method branch records owner_type_arg_binding_seed before returning, but this parallel branch only stores interface_method_generic_params. Bounds on required interface methods that reference interface owner params can then lose the concrete owner binding during call-site validation.
Proposed fix
self.interface_method_generic_params.insert(
at,
(
member.clone(),
function_generic_params,
function_generic_param_bounds,
),
);
+ let owner_type_arg_bindings = iface_data
+ .generic_params
+ .iter()
+ .filter_map(|param| bindings.get(param).cloned().map(|ty| (param.clone(), ty)))
+ .collect::<Vec<_>>();
+ if !owner_type_arg_bindings.is_empty() {
+ self.owner_type_arg_binding_seed
+ .insert(at, owner_type_arg_bindings);
+ }
return Some(fn_ty);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.interface_method_generic_params.insert( | |
| at, | |
| ( | |
| member.clone(), | |
| function_generic_params, | |
| function_generic_param_bounds, | |
| ), | |
| ); | |
| return Some(fn_ty); | |
| self.interface_method_generic_params.insert( | |
| at, | |
| ( | |
| member.clone(), | |
| function_generic_params, | |
| function_generic_param_bounds, | |
| ), | |
| ); | |
| let owner_type_arg_bindings = iface_data | |
| .generic_params | |
| .iter() | |
| .filter_map(|param| bindings.get(param).cloned().map(|ty| (param.clone(), ty))) | |
| .collect::<Vec<_>>(); | |
| if !owner_type_arg_bindings.is_empty() { | |
| self.owner_type_arg_binding_seed | |
| .insert(at, owner_type_arg_bindings); | |
| } | |
| return Some(fn_ty); |
🤖 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.rs` around lines 12564 -
12572, In the code block where self.interface_method_generic_params is being
inserted (around the return of Some(fn_ty)), this branch is missing the owner
type-arg seeding that the earlier interface-method branch performs. Before the
return Some(fn_ty) statement, add a call to record owner_type_arg_binding_seed
(similar to what the earlier interface-method branch does) to ensure bounds on
required interface methods that reference interface owner params maintain the
concrete owner binding during call-site validation.
The type of function-pointer values may not parameterize themselves:
(int) -> float throws neveris valid, but<T>(T) -> float throws neveris not. At a later date, we may add unbound function types, but at this time we do not as they cause problems with subtyping and equivalence.Summary by CodeRabbit
Breaking Changes
identity<int>instead ofidentity).(foo)<int>is invalid).Bug Fixes