Normalize MIR interface implementors through the canonical TIR registry#4012
Normalize MIR interface implementors through the canonical TIR registry#4012aaronvg wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughMIR lowering now consumes canonical TIR implementation data for interface-implementor registration, propagates associated-type bindings during closure computation, and supports concrete out-of-body implementations. A multi-file runtime test verifies interface narrowing and method dispatch. ChangesInterface implementor registration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 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 |
⏭️ 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):
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_tests/tests/interfaces.rs (1)
5631-5690: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider writing a unit test instead of an integration test.
As per coding guidelines, prefer writing Rust unit tests over integration tests where possible. Given this is a new integration test for cross-file interface matching, please evaluate if this behavior can be adequately validated via a unit test in the
baml_compiler2_mircrate.🤖 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_tests/tests/interfaces.rs` around lines 5631 - 5690, Evaluate whether cross-file interface matching in cross_file_out_of_body_impl_matches_after_parent_interface_erasure can be covered at the MIR level, and if so, move the test into a Rust unit-test module in the baml_compiler2_mir crate using the closest existing MIR test helpers. Preserve coverage of parent-interface erasure and out-of-body implementations across files; retain the integration test only if those compiler-engine behaviors cannot be validated adequately at MIR level.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.
Nitpick comments:
In `@baml_language/crates/baml_tests/tests/interfaces.rs`:
- Around line 5631-5690: Evaluate whether cross-file interface matching in
cross_file_out_of_body_impl_matches_after_parent_interface_erasure can be
covered at the MIR level, and if so, move the test into a Rust unit-test module
in the baml_compiler2_mir crate using the closest existing MIR test helpers.
Preserve coverage of parent-interface erasure and out-of-body implementations
across files; retain the integration test only if those compiler-engine
behaviors cannot be validated adequately at MIR level.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bbc72881-e18f-418b-b350-f51225efd3a2
📒 Files selected for processing (2)
baml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_tests/tests/interfaces.rs
Binary size checks passed✅ 7 passed
Generated by |
abc28ff to
2c96901
Compare
aaronvg
changed the title
1 participant
Summary
ImplDataregistryclass_data.implementsand out-of-bodyfree_implsrequiresclosureRoot cause
TIR already normalizes both forms:
and:
implement I for C {}into one semantic
ImplDatashape containing the resolved interface, implementor pattern, generics, associated bindings, methods, and field links. The syntax origin is diagnostic metadata only.MIR bypassed that invariant. It independently populated runtime interface membership from:
class_data.implementsfor in-body/same-file declarationsfree_implsfor generic out-of-body rulesA non-generic cross-file out-of-body implementation therefore existed in the canonical TIR registry—so type checking and reflection returned true—but was absent from MIR's runtime type-tag candidate set. Matching the erased provider against its child capability selected the wildcard arm.
Fix
MIR now enumerates
package_impl_locs, reads each normalizedimpl_data, and derives runtime class candidates fromfor_ty_pattern. Both source syntaxes therefore take the same path. Blanket bounds are checked from canonical normalized bounds, and the interface closure receives canonical generic and associated-type bindings.Tracks B-884.
Validation
cargo nextest run -p baml_tests --test interfaces --no-fail-fast— 470 passed, 2 skippedcargo nextest run -p baml_compiler2_mir— 5 passedSKIP=no-commit-to-branch prek run --all-files --show-diff-on-failure --hook-stage manual— passedcargo fmt --all -- --check— passedgit diff --check— passedSummary by CodeRabbit
Bug Fixes
implements I for Crules correctly apply when the target is a concrete class without type arguments.Tests