TIR + MIR should use ItemTree firewall queries - #4092
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):
|
📝 WalkthroughWalkthroughThis PR migrates compiler2 lowering and resolution from HIR item trees and AST type expressions to PPIR item data and TypeRef arenas. It adds TypeRef display and signature contracts, updates interface and MIR handling, and revises diagnostics, throw inference, LSP checks, and metadata tests. ChangesCompiler2 PPIR and TypeRef migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Source as BAML source
participant PPIR as PPIR item_data
participant TIR as TIR resolution
participant MIR as MIR lowering
Source->>PPIR: parse functions, interfaces, impls, and TypeRefs
PPIR->>TIR: provide item data and source-map spans
TIR->>MIR: provide lowered types, bounds, and interface targets
MIR-->>Source: produce lowered schemas, dispatch, and diagnostics
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 |
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_mir/src/lower.rs (1)
9145-9196: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAppend lambda generics on the early-return paths
enclosing_generic_params()is still used while lowering lambda bodies, withself.func_locpointing at the enclosing method. Returning early here dropsself.lambda_generic_params, so generic lambdas nested inFreeImplandInterfacemethods resolve their own type vars against the wrong frame slots.Patch
params.extend(function_data(self.db, fl).generic_params.iter().cloned()); + params.extend(self.lambda_generic_params.iter().cloned()); return params; }Apply the same append before the
Interfacereturn too.🤖 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_mir/src/lower.rs` around lines 9145 - 9196, Update enclosing_generic_params() so both the FreeImpl and Interface early-return paths append self.lambda_generic_params after the enclosing and function generic parameters, matching the normal path and preserving correct lambda type-variable frame slots.
🧹 Nitpick comments (1)
baml_language/crates/baml_compiler2_hir/src/type_ref.rs (1)
244-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a paren-sensitive projection case to the round-trip test. The unqualified projection path already parenthesizes
Union/Functionbases the same way asast::TypeExpr, but the current byte-identical test only coversT.Item. Add a case like(int | string).Itemto lock that behavior in.🤖 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_hir/src/type_ref.rs` around lines 244 - 247, Add a `(int | string).Item` case to the existing byte-identical type round-trip test covering projection formatting. Ensure it exercises the unqualified projection path around `write_postfix_base` and verifies that union/function bases retain the required parentheses.
🤖 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_mir/src/lower.rs`:
- Around line 9145-9196: Update enclosing_generic_params() so both the FreeImpl
and Interface early-return paths append self.lambda_generic_params after the
enclosing and function generic parameters, matching the normal path and
preserving correct lambda type-variable frame slots.
---
Nitpick comments:
In `@baml_language/crates/baml_compiler2_hir/src/type_ref.rs`:
- Around line 244-247: Add a `(int | string).Item` case to the existing
byte-identical type round-trip test covering projection formatting. Ensure it
exercises the unqualified projection path around `write_postfix_base` and
verifies that union/function bases retain the required parentheses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0ae75c16-8604-404c-86c1-80830bb8a105
📒 Files selected for processing (18)
baml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/type_ref.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/builder/associated_projection.rsbaml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rsbaml_language/crates/baml_compiler2_tir/src/callable.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/interfaces.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/coherence.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler2_tir/src/resolve.rsbaml_language/crates/baml_compiler2_tir/src/signature.rsbaml_language/crates/baml_compiler2_tir/src/throw_inference.rsbaml_language/crates/baml_lsp2_actions/src/check.rs
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_mir/src/lower.rs (1)
11074-11089: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the interface package items for associated-type defaults.
assoc.defaultlowerstarget_data.type_refswithtarget_iface_pkg.namespace_pathbut still passesclass_pkg_items, so bare or relative names in an interface default resolve from the implementing class’s package instead of the interface’s.baml_language/crates/baml_compiler2_mir/src/lower.rs:11074-11089🤖 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_mir/src/lower.rs` around lines 11074 - 11089, Update the assoc.default lowering in the surrounding interface implementation flow to pass the interface package’s items instead of class_pkg_items, while retaining target_data.type_refs and target_iface_pkg.namespace_path. Ensure bare and relative names in associated-type defaults resolve within the interface package.
🤖 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_mir/src/lower.rs`:
- Around line 11074-11089: Update the assoc.default lowering in the surrounding
interface implementation flow to pass the interface package’s items instead of
class_pkg_items, while retaining target_data.type_refs and
target_iface_pkg.namespace_path. Ensure bare and relative names in
associated-type defaults resolve within the interface package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f5ce8c6d-806d-4363-9e8f-c5cd7be70468
📒 Files selected for processing (18)
baml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/type_ref.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/builder/associated_projection.rsbaml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rsbaml_language/crates/baml_compiler2_tir/src/callable.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/interfaces.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/coherence.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler2_tir/src/resolve.rsbaml_language/crates/baml_compiler2_tir/src/signature.rsbaml_language/crates/baml_compiler2_tir/src/throw_inference.rsbaml_language/crates/baml_lsp2_actions/src/check.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- baml_language/crates/baml_compiler2_tir/src/callable.rs
- baml_language/crates/baml_compiler2_tir/src/signature.rs
- baml_language/crates/baml_compiler2_tir/src/resolve.rs
- baml_language/crates/baml_compiler2_hir/src/type_ref.rs
- baml_language/crates/baml_compiler2_emit/src/lib.rs
- baml_language/crates/baml_lsp2_actions/src/check.rs
- baml_language/crates/baml_compiler2_tir/src/throw_inference.rs
- baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs
- baml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rs
- baml_language/crates/baml_compiler2_tir/src/interfaces.rs
- baml_language/crates/baml_compiler2_tir/src/package_interface.rs
- baml_language/crates/baml_compiler2_tir/src/builder/associated_projection.rs
- baml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rs
- baml_language/crates/baml_compiler2_tir/src/infer_context.rs
- baml_language/crates/baml_compiler2_tir/src/inference.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
Instead of querying the
ItemTreedirectly (which will force consumers to fully recompute whenever anything changes), we should use the firewall queries for granular salsa cache invalidation.This builds on #4064 and will be followed by further PR(s) to complete this migration (eventually making
ItemTreeprivate to consumers)Summary by CodeRabbit
Improvements
Tests