Repository navigation
Improved implements rules representation in compiler - #3852
Merged
Merged
Conversation
Makes an referenceable representation of an `implement` block. In this commit it is unused.
It is a bandaid to handle the unfortunate fact that we type erase in many cases. Once we eliminate type erasure, the variant will be removed.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR changes ChangesImplements pipeline
Sequence Diagram(s)sequenceDiagram
participant lower_implements_for
participant ItemTree
participant package_implements_registry
participant impl_data
participant check_interfaces
participant get_implements_block
participant ResolvedImpl
lower_implements_for->>ItemTree: alloc_impl(...) stores ImplBlock
package_implements_registry->>impl_data: build InterfaceImplRule entries
check_interfaces->>impl_data: surface impl diagnostics
get_implements_block->>impl_data: resolve coherent impl and bindings
get_implements_block->>ResolvedImpl: return ResolvedImpl
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ 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):
|
|
No description provided. |
![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
🧹 Nitpick comments (2)
baml_language/crates/baml_type/src/template.rs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the file-level deprecation expectation.
The blanket
#![expect(deprecated)]is broader than the actual deprecated use sites and can mask unrelated deprecation warnings in this module. The arm-level expectations already localize the exception.Suggested change
-#![expect(deprecated, reason = "TypeArgRefOrWildcard")]🤖 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/template.rs` at line 1, The file-level deprecated expectation in template.rs is too broad and can hide unrelated warnings; narrow or remove the module-level `#![expect(deprecated, reason = "TypeArgRefOrWildcard")]` and rely on the existing localized arm-level expectations instead. Update the `template.rs` module so only the specific deprecated use sites around `TypeArgRefOrWildcard` are covered, keeping the exception scoped to the relevant match arms or items.baml_language/crates/baml_compiler2_ast/src/lower_cst.rs (1)
1228-1276: 📐 Maintainability & Code Quality | 🔵 TrivialOptional: collapse the two bound extractors to reduce duplication.
extract_generic_params_with_all_boundsandextract_generic_params_with_boundsshare nearly all traversal logic, differing only in whether they keep all bounds or the first. The single-bound variant could delegate (...with_all_bounds(node).into_iter().map(|(n, b)| (n, b.into_iter().next()))), keeping one source of truth for the CST walk. Deferable.🤖 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_cst.rs` around lines 1228 - 1276, `extract_generic_params_with_all_bounds` and `extract_generic_params_with_bounds` duplicate almost the same CST traversal logic. Refactor the single-bound helper to delegate to `extract_generic_params_with_all_bounds` and then keep only the first bound per param, while preserving the existing behavior for empty bounds and names. Keep the shared node-walking and name extraction in one place so both helpers stay consistent.
🤖 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_hir/src/item_tree.rs`:
- Around line 599-607: The legacy `ImplementsFor.generic_param_bounds`
population in `item_tree.rs` is truncating each generic parameter to only the
first `&`-bound, so downstream users can miss additional constraints. Update the
`ImplementsFor` construction to preserve the full bound set, or first migrate
all consumers that still read `implements_for.generic_param_bounds` to the new
`ImplBlock`/`GenericParam.bounds` source before keeping this legacy field
truncated. Use the `ImplementsFor` builder in `item_tree.rs` and any
MIR/lowering code paths that depend on `implements_for.generic_param_bounds` to
ensure they see every bound.
In `@baml_language/crates/baml_compiler2_tir/src/interfaces.rs`:
- Around line 1381-1397: The generic-bound lowering in the impl registration
path is only using g.bounds.first(), so multi-bound generics are being treated
as single-bound rules. Update the logic around the names/bounds collection in
this section to use all bounds carried by ImplData::generic_params instead of
reconstructing one lowered Ty from the first bound only, and make sure the free
impl rule registration preserves the full bound list for lookup/compatibility.
---
Nitpick comments:
In `@baml_language/crates/baml_compiler2_ast/src/lower_cst.rs`:
- Around line 1228-1276: `extract_generic_params_with_all_bounds` and
`extract_generic_params_with_bounds` duplicate almost the same CST traversal
logic. Refactor the single-bound helper to delegate to
`extract_generic_params_with_all_bounds` and then keep only the first bound per
param, while preserving the existing behavior for empty bounds and names. Keep
the shared node-walking and name extraction in one place so both helpers stay
consistent.
In `@baml_language/crates/baml_type/src/template.rs`:
- Line 1: The file-level deprecated expectation in template.rs is too broad and
can hide unrelated warnings; narrow or remove the module-level
`#![expect(deprecated, reason = "TypeArgRefOrWildcard")]` and rely on the
existing localized arm-level expectations instead. Update the `template.rs`
module so only the specific deprecated use sites around `TypeArgRefOrWildcard`
are covered, keeping the exception scoped to the relevant match arms or items.
🪄 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: d89d1862-dfcc-4d0c-912d-b3c1e847d21b
📒 Files selected for processing (17)
baml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/ids.rsbaml_language/crates/baml_compiler2_hir/src/item_tree.rsbaml_language/crates/baml_compiler2_hir/src/loc.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/interfaces.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rsbaml_language/crates/baml_compiler_diagnostics/src/diagnostic.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_tests/src/compiler2_hir.rsbaml_language/crates/baml_tests/tests/interfaces.rsbaml_language/crates/baml_type/src/template.rsbaml_language/crates/bex_vm/src/package_baml/resolve.rsbaml_language/crates/bex_vm/src/vm.rs
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.
This PR is part of a larger effort to improve the soundness of interfaces. The code it adds is currently mostly unused, but is intended to enable a unified way to represent and lookup interface
implementsrules in the compiler.ImplBlockImplData(realized concrete type, realized interface)so we can now lookup theImplDatathat represents the implementation of that interface for that concrete type.T, does some implements rule exist whereC<T>implementsI<T>?" and on top of that, if we can narrow down at compile time which implements rule is used, then that may be able to be baked at compile type instead of dynamic lookup.get_implements_blocknow uses the lookupSummary by CodeRabbit
New Features
Bug Fixes
Tests