Repository navigation
Solve _ inference holes in expression-context type positions - #3910
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):
|
|
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 selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR refines how ChangesExpression-context wildcard hole resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
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/builder.rs (1)
4895-4953: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the
_-hole guard before associated-type completion.resolve_deepcan recurse into the target’s interface args, andTy::Inferstill hits normalization’s unreachable path, so.as<Show<_>>can panic beforereport_uninferable_type_holeruns.🤖 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 4895 - 4953, The `_`-hole check in the target handling path is happening too late, after associated-type completion via AssociatedProjectionResolver::resolve_deep can recurse into interface args and trigger normalization on Ty::Infer. Move the ty_has_infer_hole guard in the builder logic before the interface associated-binding completion block so expressions like `.as<Show<_>>` are rejected by report_uninferable_type_hole before any deep resolution runs. Keep the change localized to the target_ty handling around lower_type_expr_in_ns and the associated type projection completion branch.
🤖 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/builder.rs`:
- Around line 4895-4953: The `_`-hole check in the target handling path is
happening too late, after associated-type completion via
AssociatedProjectionResolver::resolve_deep can recurse into interface args and
trigger normalization on Ty::Infer. Move the ty_has_infer_hole guard in the
builder logic before the interface associated-binding completion block so
expressions like `.as<Show<_>>` are rejected by report_uninferable_type_hole
before any deep resolution runs. Keep the change localized to the target_ty
handling around lower_type_expr_in_ns and the associated type projection
completion branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 44c36d0f-fdeb-421b-a64e-df4fe651bf40
📒 Files selected for processing (4)
baml_language/crates/baml_compiler2_ast/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_tests/tests/wildcard_expression_holes.rs
`_` was firewalled to `Ty::Error` only at declaration sites. The four expression positions that admit a written type (call turbofish, object construction, generic-apply value, upcast target) bypassed that firewall, so a `_` there survived as a raw `Ty::Infer` and hit an `unreachable!` at structural normalization or runtime lowering. Handle the hole at each site instead: solve it from local context where a source exists (call arguments, object field values), reject it cleanly where none does (bare generic value, upcast ascription). Centralize the fill-or-reject rule in one documented policy (`fill_expression_type_arg_hole` / `report_uninferable_type_hole`) that the sites share, and correct the now stale `check_wildcard_type` invariant docs. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
5d7c683 to
9e70aa9
Compare
Summary
Extends the
_wildcard feature from #3887 (B-230/B-247) to the expression-context type positions. Before this,_was solvable in aletannotation and athrowsclause, but writing it anywhere in an expression (a call turbofish, an object literal, a generic value, an upcast) either panicked the compiler or was silently un-handled.cc @antoniosarosi, since this builds directly on your
_inference work and changes the invariant your firewall documented.Root cause
Your
check_wildcard_typefirewall rewrites_toTy::Errorat every declaration site (params, returns, fields, aliases, bounds), which is why the doc there said "aTy::Inferonly ever originates from the two hole-handling contexts."That invariant was actually already violated: there are four expression positions that carry a written type and never pass through the firewall, so a
_there reached type checking as a liveTy::Infer:foo<T, _>(x)Box<_> { ... }id<_>c.as<Show<_>>A surviving
Ty::Inferthen tripped anunreachable!at one of the two boundaries that deliberately refuse to guess a meaning for a hole:baml_type/src/runtime_ty.rs(MIR runtime lowering):`Infer` is not a valid `RuntimeTy`baml_compiler2_tir/src/normalize.rs:566(structural normalization):inference hole_reached structural normalizationThese were latent panics, not something a user had to do anything exotic to hit.
Approach
Same principle as your
let/throwshandling: fill the hole from a local source where one exists, reject it cleanly where none does. Never let aTy::Inferescape the site.race<T, _>(...)Box<_> { v: 5 }id<_>c.as<Show<_>>For the two solve sites, a hole that still cannot be resolved from its source (e.g.
pick<int, _>(5)whereUis in neither an argument nor the return) is reported ascannot infer type parameter, not accepted or panicked.I deliberately left upcast as a reject rather than solving
_from the receiver'simplementslist. That is a bigger feature and matches howis Show<_>already behaves. Happy to revisit if you think upcast should solve.Before / after
1. Call turbofish (the original report)
`Infer` is not a valid `RuntimeTy`(runtime lowering)._(the future's error type) is inferred from[work, timer], whileTstays pinned. Note the no-turbofish formrace([work, timer])cannot type-check here (the two futures do not unify), which is exactly why the partial<T, _>is needed.2. Object construction
3. Generic-apply value (no source, reject)
4. Upcast target (no source, reject)
c.as<Show<_>> // was: panic (normalize.rs:566) -> now: E0002 unresolved type: _ (matches `is Show<_>`)Centralized policy (so a 5th position does not reintroduce this)
The enforcement is necessarily per-site (a hole has no sound normal form, so it must be resolved before that site's own subtype/normalize checks; the
unreachable!is the intentional tripwire). To avoid four ad-hoc copies, the fill-or-reject rule now lives in one documented place inbuilder.rs:report_uninferable_type_hole(param, at): the single reject-and-neutralize path.CannotInferTypeParameterwhen a parameter is named,UnresolvedTypefor a nested ascription hole.fill_expression_type_arg_hole(partial, candidate, param, at): keep-if-concrete, else fill from the site's inferred candidate, else reject.candidate: Nonedegenerates to pure reject, so the same helper serves both solve and reject sites.Call, generic-apply, and upcast all route through these. Object keeps its own field-inference loop (it has a richer policy: an unconstrained phantom param recovers to
BuiltinUnknowninstead of erroring) but is documented as the field-sourced case of the same rule.On the MIR side, the two hole detectors were merged into one (
type_arg_is_infer_hole), which asks the actual conversion boundary (lower_to_runtime(..).is_err()) so it can never drift from what the runtime accepts.I also corrected the
check_wildcard_typedoc, which claimed the two-context invariant that is no longer true.What "handled" means here
Every
_position now either solves or produces a clean diagnostic. It does not mean every theoretically-inferable_is inferred (upcast is a deliberate reject). The guarantee is: noTy::Inferreaches normalization or runtime lowering from a supported syntactic position.Files
baml_compiler2_tir/src/builder.rs: policy helpers + the four sites (call, object, generic-apply, upcast).baml_compiler2_mir/src/lower.rs: substitute the TIR-solved type into hole slots for call and object type-args instead of re-lowering the raw_; single hole detector.baml_compiler2_ast/src/lower_type_expr.rs: correctedcheck_wildcard_typeinvariant docs.baml_tests/tests/wildcard_expression_holes.rs: 9 regression tests covering all four positions (solve, mixed-partial, nested, phantom, and each reject).Testing
New file green (9). No regressions across wildcard (17), interfaces (35 / 150), instantiation, spawn, match/typevar, json interfaces (12 / 11), and tir/mir unit (175). Clean
cargo clippy/cargo fmt.🤖 Generated with Claude Code
Summary by CodeRabbit
_type holes in expression-contexts are now handled more consistently, producing clear compile-time errors instead of runtime panics._reliably using available context.