Repository navigation
Use global impl registry and ban bare Self in associated type defaults - #4251
Merged
Merged
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):
|
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 (14)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThe PR requires explicit operator output types, rejects bare ChangesOperator typing and diagnostics
Program-wide implementation indexing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProgramLoader
participant PackageIndex
participant BexEngine
participant BexVm
participant resolve_implements_rule
ProgramLoader->>PackageIndex: build package and interface rule indexes
ProgramLoader->>BexEngine: return heap and PackageIndex
BexEngine->>BexVm: construct VM with shared PackageIndex
BexVm->>resolve_implements_rule: resolve interface implementation
resolve_implements_rule->>PackageIndex: retrieve rules for interface
PackageIndex-->>resolve_implements_rule: return program-wide rules
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
🔧 Fix failing CI
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: 1
🧹 Nitpick comments (1)
baml_language/crates/bex_vm/src/package_baml/resolve.rs (1)
43-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMissing dedicated regression tests for the new program-wide impl-resolution semantics.
This PR introduces a behavior change — cross-package impl resolution keyed by a program-wide
PackageIndex— but neither the new data structure nor the resolution algorithm it feeds has a unit test in the reviewed files covering the specific scenario the PR targets (an impl anchored in a package that owns neitherSelfnor the interface).
baml_language/crates/bex_vm/src/package_baml/resolve.rs#L43-L56: add a unit test exercisingrules_for/resolve_implements_rulefor an impl declared in a third package (owning neither the receiver type nor the interface).baml_language/crates/bex_vm/src/package_load.rs#L84-L136: add a unit test forPackageIndex.impl_rules/impl_rules_ofaccumulating rules for the same interface across two distinct packages.🤖 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/bex_vm/src/package_baml/resolve.rs` around lines 43 - 56, The new program-wide impl-resolution behavior lacks regression coverage. In baml_language/crates/bex_vm/src/package_baml/resolve.rs:43-56, add a unit test covering rules_for and resolve_implements_rule when the impl is declared in a third package that owns neither the receiver type nor the interface. In baml_language/crates/bex_vm/src/package_load.rs:84-136, add a unit test verifying PackageIndex.impl_rules and impl_rules_of accumulate rules for the same interface from two distinct packages.Source: Path instructions
🤖 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/inference.rs`:
- Around line 2202-2206: The Self detector used by the default validation in
inference.rs around TypeInferenceBuilder::type_ref_contains_bare_self must
recurse into associated-type projection bases, exempting only an exact Self base
so nested forms such as (Self[] as I).Item are rejected. Add a regression test
in lower_type_expr.rs in the existing test area covering a default like type Out
= (Self[] as I).Item and asserting the expected rejection.
---
Nitpick comments:
In `@baml_language/crates/bex_vm/src/package_baml/resolve.rs`:
- Around line 43-56: The new program-wide impl-resolution behavior lacks
regression coverage. In
baml_language/crates/bex_vm/src/package_baml/resolve.rs:43-56, add a unit test
covering rules_for and resolve_implements_rule when the impl is declared in a
third package that owns neither the receiver type nor the interface. In
baml_language/crates/bex_vm/src/package_load.rs:84-136, add a unit test
verifying PackageIndex.impl_rules and impl_rules_of accumulate rules for the
same interface from two distinct packages.
🪄 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: aa59d7ac-2dfc-4056-905b-ca6d8f3c068d
⛔ Files ignored due to path filters (1)
baml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
baml_language/crates/baml_builtins2/baml_std/baml/ns_ops/math.bamlbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler_diagnostics/src/diagnostic.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_tests/baml_src/ns_operators/operators.bamlbaml_language/crates/baml_tests/tests/interfaces.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_vm/src/package_baml/resolve.rsbaml_language/crates/bex_vm/src/package_baml/type_class.rsbaml_language/crates/bex_vm/src/package_load.rsbaml_language/crates/bex_vm/src/vm.rs
Binary size checks passed✅ 7 passed
Generated by |
Since orphan rule permits impls with certain local generics even if neither target is local we cannot just rely on the two packages when looking up impls with generics. For simplicity we now do a global search.
It breaks the interface-existential since `Self` is meaningless when we don't know the implementor type. This is similar to rust and is the reason they require `Output` to be explicitly declared for ops.
2kai2kai2
force-pushed
the
kai/fix-reflect-implements
branch
from
July 28, 2026 21:36
b8bf7e5 to
1af6896
Compare
2kai2kai2
enabled auto-merge
July 28, 2026 21:36
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.
Does two things:
Selfin associated type defaults (except as an associated type projection's base). This is because inclusion in defaults would cause an unresolvedSelfin interface-existential types which is unsound since we don't know the concrete implementor type.Summary by CodeRabbit
Outputresult types for add/subtract/multiply/divide/remainder and negate across numeric, string, and custom types.E0157) when an invalid bareSelfis used as an associated-type default, while continuing to allow validSelfprojections and concrete defaults.Self-default rules.