Repository navigation
Fix interface Self bugs - #3647
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2kai2kai2
enabled auto-merge
June 2, 2026 23:01
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughImplements BEP-044 Self-pinned interface methods across TIR generics/inference, member resolution/call checking, out-of-body signature lowering, MIR bound-method lowering, diagnostics, and tests. ChangesBEP-044 Self-Pinned Interface Methods
Sequence Diagram(s)sequenceDiagram
participant Source as Source/Parser
participant TIR as TIR (generics/inference)
participant Builder as CallChecker/MemberResolver
participant LSP as SignatureChecker
participant MIR as MIR Lowering
participant Runtime as Runtime Guard
Source->>TIR: add rigid Self handling, seed interface Self bounds
TIR->>Builder: propagate rigid_self_var, resolve members with SelfReceiver
Builder->>LSP: validate out-of-body impl `for` targets
Builder->>MIR: record pinned Self & bound-method candidates
MIR->>Runtime: emit guard-switch -> MakeBoundMethod
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/baml_compiler2_tir/src/builder.rs (1)
2519-2527:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let Phase 0 rebind the pinned
Self.This reverse-inference step still uses
infer_bindings, so a contextual expected type can bind the rigidSelfbefore argument checking runs. For a bound zero-arg method likeclone() -> Self,let y: U = x.clone()can inferSelf -> U, suppress the later result mismatch, and accept an invalid projection.Suggested fix
- crate::generics::infer_bindings(ret, phase0_expected, &mut bindings); + crate::generics::infer_bindings_rigid_self( + ret, + phase0_expected, + &mut bindings, + rigid_self_var.as_ref(), + );🤖 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 2519 - 2527, Phase 0 reverse-inference is allowed to rebind a pinned Self via the call to crate::generics::infer_bindings(ret, phase0_expected, &mut bindings); avoid that by skipping infer_bindings when the return type `ret` (or the current bindings) contains the pinned/rigid Self typevar: before calling infer_bindings in the block that checks phase0_expected and crate::generics::contains_typevar(ret), add a guard that detects the pinned Self (e.g., via an existing helper or a new crate::generics::contains_pinned_self(ret) check or by inspecting bindings for Self) and only call infer_bindings when Self is not pinned so `Self` cannot be rebound during Phase 0.
🤖 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 3691-3737: The dispatch misses captured receiver roots because the
recv_root_local lookup only checks self.locals; change the lookup for
recv_root_local to also consult captured roots (e.g. use
self.locals.get(&segments[0]).or_else(|| self.captured_roots.get(&segments[0])))
so captured `x` in closures will be treated the same as local roots, then
proceed to use lower_path_receiver_to_local,
interface_dispatch_target_for_tir_ty, and emit_bound_method_candidate_switch as
before; ensure the rest of the branch logic (receiver_segments, recv_seg_idx,
path_segment_types lookup) is unchanged so bound-method-value lowering works for
captured receivers.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 2667-2673: The defers_typevar predicate currently only checks
self.generic_param_bounds so outer caller generics (e.g. a bare T) are treated
as deferable; update the closure used in contains_typevar_where to also consult
the caller generic list (generic_params) and treat any name present in
generic_params as non-deferable/rigid. Concretely, in the closure for
defers_typevar (the one referencing rigid_self_var, self.generic_param_bounds
and generic_params) add a check that if name is in generic_params it sets rigid
= true (or otherwise prevents deferral), so only names that are in
generic_param_bounds but not in generic_params remain deferable.
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 2519-2527: Phase 0 reverse-inference is allowed to rebind a pinned
Self via the call to crate::generics::infer_bindings(ret, phase0_expected, &mut
bindings); avoid that by skipping infer_bindings when the return type `ret` (or
the current bindings) contains the pinned/rigid Self typevar: before calling
infer_bindings in the block that checks phase0_expected and
crate::generics::contains_typevar(ret), add a guard that detects the pinned Self
(e.g., via an existing helper or a new
crate::generics::contains_pinned_self(ret) check or by inspecting bindings for
Self) and only call infer_bindings when Self is not pinned so `Self` cannot be
rebound during Phase 0.
🪄 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: 3e9fcb5c-76bf-4431-a8b8-07079fde81bd
📒 Files selected for processing (7)
baml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/generics.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler_diagnostics/src/diagnostic.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_tests/tests/interfaces.rs
Binary size checks passed✅ 7 passed
Generated by |
The original interfaces implementation made `Self` an interface type, instead of a pinned type variable. For example, this is now invalid:
```baml
interface Equatable {
function eq(self, other: Self) -> bool
}
function cmp<S extends Equatable, U extends Equatable>(x: S, y: U) -> bool {
return x.eq(y)
}
```
Since while both `S` and `U` extend `Equatable`, they are *not* necessarily the same type and thus `eq` should not be able to be called on them. This test case now produces a compiler error.
We now use an enum for the `Self` reciever-type: interface-typed, typearg, or concrete. Each has slightly different semantics (especially interface-typed which cannot be called on methods with multiple `Self` parameters)
You cannot implement an interface for an abstract (union/interface/`unknown`) type, as they are abstract.
- Prevent implementation blocks for type aliases which refer to abstract types - Fix `self` reciever-type resolution on out-of-body implements blocks - Better docs - Better diagnostics
Previously `self` would lose the generics and be the bare `Foo` instead of `Foo<int, string>`
It is an abstract type.
⏭️ 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):
|
This was referenced Jun 4, 2026
This was referenced Jun 17, 2026
This was referenced Jul 20, 2026
This was referenced Jul 28, 2026
Merged
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Selfshould behave like a pinned type variable, not an dyn-interface-type. Makes this (correctly) produce a compiler error:implement Foo for int | string. This prohibition applies to all abstract types: unions, optionals, interfaces,unknownSummary by CodeRabbit
implements ... for ...targets (error E0138).implements ... for ...constraints.