Repository navigation
Deslopify TIR: A workflow experiment - #3645
Closed
antoniosarosi wants to merge 19 commits into
Closed
antoniosarosi wants to merge 19 commits into
antoniosarosi wants to merge 19 commits into
Conversation
…preserving) Multi-agent de-slopify pass over the TIR crate: removed duplication, dead code, dead params, redundant helpers, magic strings, and comment rot while preserving behavior. 131/131 crate tests and 4,991/4,991 workspace tests pass. Highlights: - Unify the two throws walkers: delete the ~165-line duplicate builder.rs::collect_throw_facts_from_expr and parameterize the shared throws_analysis::collect_from_expr with three behavior toggles (walk_catch_clauses / await_adds_future_error / call_unwraps_optional_member_callee), used by CatchBaseThrowsAnalysis. - Replace hand-written `unsafe impl salsa::Update` with `#[derive(salsa::Update)]` for PackageInterface / PackageResolutionContext. - Remove duplicate ResolvedFunction (reuse the existing ExportedFunction); drop dead PackageResolutionContext::lookup_class_fields / resolve_value_in_own. - Remove dead `_pkg_items` / `namespace_path` params from the public interface_closure_locs[_with_args]; update the MIR cross-crate call sites. - Delete the unused cycle_detector module (CycleDetector had no references). - Fix misattributed/stale doc-comments and consolidate repeated magic strings.
Second automated de-slopify pass over baml_compiler2_tir (after pass 1, 8fb72ec), this time with working call-graph grouping and deep, section-by-section analysis of the large files. Behaviour-preserving: 131/131 crate tests and 4,991/4,991 workspace tests pass, no tests removed. TIR (-1,025 lines; cumulative -2,391 / -7.8% over both passes): - Unify substitute_self_in / substitute_paths_in behind one closure-based substitute_in walker (both public fns and their tests retained). - Remove dead TirTypeError variants (AliasCycle, ClassCycle, CannotInferTypeParameter, WrongNumberOfTypeArgs) that were never constructed anywhere; update the lsp2_actions diagnostic-id mapper. Real cycle diagnostics still emit via baml_compiler_diagnostics (untouched). - builder.rs -508 lines, found by the deep duplicate scan. Clippy (`cargo clippy --workspace --all-targets --all-features -- -D warnings` now passes; the gates only ran `cargo check` + tests, so these slipped in, and pre-existing debt in other crates was masked until TIR went clean): - TIR: Default impl, redundant closure, collapsible-match guards, elided lifetimes, doc-comment placement, &-deref, pass-by-slice, too_many_arguments. - hir, builtins2_codegen, mir, emit, bex_sap, bex_vm, bex_project, baml_tests, tools_sap_visualizer: trailing commas, `?`, sort_by_key, for_kv_map, redundant refs, explicit numeric literal — all mechanical.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughRefactors TIR/Type shapes (removing TyAttr), centralizes and sinkifies type lowering/generics, rewrites throws analysis and callable lowering, simplifies interface-impl registry, restructures inference/diagnostics, updates MIR and downstream consumers, and refreshes many tests and test helpers. ChangesTIR refactor and propagation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
baml_language/crates/baml_compiler2_tir/src/throws_analysis.rs (1)
54-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve optional-member segments in the shared path walker.
expr_to_path_segments()is now reused bythrow_inference::collect_call_targets()andcallable::named_callee_key(), but it only handlesPathandMemberAccess. A plainCallwhose callee isOptionalMemberAccess(obj?.method()) therefore stops producing a dotted target, so the pre-TIR throw graph can miss that callee entirely and later summary lookup falls back toUnknown.🤖 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/throws_analysis.rs` around lines 54 - 63, The current expr_to_path_segments only handles Expr::Path and Expr::MemberAccess, so calls whose callee is an OptionalMemberAccess (e.g., obj?.method()) drop the member and produce no dotted path; update expr_to_path_segments to also recognize OptionalMemberAccess (and Expr::Call whose callee is OptionalMemberAccess) and treat it like MemberAccess for path-building: recurse on the base (using expr_to_path_segments(*base, body)?), push the member name, and return the combined segments so throw_inference::collect_call_targets() and callable::named_callee_key() get the full dotted target; use the existing function name expr_to_path_segments and pattern-match on Expr::OptionalMemberAccess (and/or Expr::Call -> callee) to locate the code to change.baml_language/crates/baml_compiler2_tir/src/package_interface.rs (1)
412-443:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the enclosing class package when synthesizing implicit
self.
build_self_type_for_class()hard-codesbamlfor every non-container class. That means a user or dependency method with an implicitselfparameter exportsselfasbaml.<ns>.MyClassinstead of the class's real package, which then leaks intopackage_interface()andlookup_class_method()signatures.Possible fix
fn build_self_type_for_class( class_data: &baml_compiler2_hir::item_tree::Class, + package: &Name, ns_path: &[Name], ) -> Ty { @@ _ => { let qtn = QualifiedTypeName::new_with_generic_params( - Name::new(baml_base::BAML_PACKAGE), + package.clone(), ns_path.to_vec(), class_data.name.clone(), class_data.generic_params.clone(), ); Ty::Class(qtn, vec![], TyAttr::default())Then pass the actual package from the class file at the two call sites.
🤖 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/package_interface.rs` around lines 412 - 443, build_self_type_for_class currently hardcodes the package to baml when creating the QualifiedTypeName, causing implicit self to be exported under the wrong package; change build_self_type_for_class to accept the class's actual package (e.g. a Name or &Name package parameter) and use that instead of Name::new(baml_base::BAML_PACKAGE) when calling QualifiedTypeName::new_with_generic_params; then update the call sites (e.g. where package_interface() and lookup_class_method() call build_self_type_for_class) to pass the class file's package through so synthesized self uses the enclosing class 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.
Inline comments:
In `@baml_language/crates/baml_compiler2_tir/src/callable.rs`:
- Around line 103-120: enclosing_class and enclosing_class_generic_params only
search item_tree.classes, so interface default methods are missed; update both
to also search item_tree.interfaces (or whatever collection holds interfaces in
ItemTree) for a class/interface that contains the given function_id and return
that interface (for enclosing_class) or its generic_params (for
enclosing_class_generic_params) when found; specifically modify enclosing_class
to check both item_tree.classes.values().find(...) and
item_tree.interfaces.values().find(...), and modify
enclosing_class_generic_params to map the found interface's generic_params as
well, falling back to default if neither is found.
In `@baml_language/crates/baml_compiler2_tir/src/exhaustiveness.rs`:
- Around line 1054-1058: The check in split_ctors uses is_list_ty(ty) on the raw
Ty and therefore misses alias-wrapped list types (Ty::TypeAlias); update the
logic to expand aliases before list detection — e.g., add an alias-aware helper
or use PatCtx to resolve/normalize the column type (resolve_type_alias or
similar) before calling is_list_ty, or change is_list_ty to accept a PatCtx + Ty
and internally expand aliases; ensure split_ctors then calls split_slice_ctors
for resolved List/EvolvingList cases instead of falling through to
enumerate_ctors; apply the same alias-aware detection at the other similar sites
called out (around the uses corresponding to lines ~1099-1107 and ~1130-1132).
In `@baml_language/crates/baml_compiler2_tir/src/inference.rs`:
- Around line 521-539: lambda_enclosing_generics currently only prepends class
generics via enclosing_class_generics, so lambdas inside generic interface
default methods miss the interface type parameters; update
lambda_enclosing_generics to also check item_tree.interfaces (reuse or create an
enclosing_interface_generics helper analogous to enclosing_class_generics) and,
when parent_scope.kind is Interface or when enclosing_class_generics returns
None, retrieve the interface's generics and prepend them with prepend_generics
to gp so interface type parameters (e.g. T) are included for nested lambdas.
In `@baml_language/crates/baml_compiler2_tir/src/interfaces.rs`:
- Around line 426-432: The current `continue` inside the loop for
`InterfaceImplOrigin::OutOfBody` causes out-of-body generic impl rules (e.g.
`implements<T> Container<T> for Box<T>`) to be skipped from `class_implements`,
producing false negatives in `ImplementsRegistry::implements()`; change the
logic so that you still record the rule into `class_implements` for the target
class (e.g. `Box`) even when `rule.origin == InterfaceImplOrigin::OutOfBody` and
`class_args` contain `Ty::TypeVar(..)`, while preserving any special dispatch
behavior tied to out-of-body rules — or alternatively update the public
contract/docs of `ImplementsRegistry::implements()` to state that
`class_implements` does not include out-of-body generic impls; prefer the
former: remove the early `continue` and ensure insertion into `class_implements`
occurs unconditionally (but keep any separate handling needed for dispatch).
In `@baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs`:
- Around line 157-193: resolve_type currently tries ns_context+seg_ns then
immediately goes to cross-package lookup, skipping a lookup in the current
package root (seg_ns) when ns_context is non-empty; update resolve_type so it
tries three lookups in order: 1) package_items.lookup_type(&ns, item) where ns =
ns_context + seg_ns, 2) package_items.lookup_type(seg_ns, item) to allow
package-root types, and only then 3) perform the cross-package fallback (the
existing or_else block that checks path[0]=="root" or uses PackageId::new/db
with baml_compiler2_ppir::package_items and pkg.lookup_type). Keep the existing
variables (seg_ns, ns_context, path, item) and methods
(package_items.lookup_type, PackageId::new, baml_compiler2_ppir::package_items)
to locate and implement the change in resolve_type.
In `@baml_language/crates/baml_compiler2_tir/src/normalize.rs`:
- Around line 239-247: The co-inductive pair (variable pair) is inserted into
assumptions before the match but some arms (e.g., the function-subtyping arm
that does an early `return false`) exit without removing it, causing stale
assumptions to leak; update the logic in normalize.rs so that every early return
removes the inserted pair first (call assumptions.remove(&pair) before
returning) or move the insert to just before the recursive/structural paths and
only insert when you can guarantee a matching removal, and apply the same fix
for the similar block around the other occurrence (the block referenced by the
comment covering lines ~351-358) to ensure no return path leaves assumptions
containing the pair.
In `@baml_language/crates/baml_compiler2_tir/src/resolve.rs`:
- Around line 89-109: The follow-up lookup widens scope by calling
dep.lookup_type(&[], name), causing a root-namespace hit from a different dep to
be chosen even when resolve_type() matched the namespaced item; change the
post-resolve_type re-derivation so it only searches the same namespace path used
earlier (i.e. use pkg_info.namespace_path for dep.lookup_type) or, better, have
resolve_type return the Definition directly so you can return that Definition
instead of performing the second, wider search; update the logic around
resolve_type, own_items.lookup_type and res_ctx.items_for_package /
dep.lookup_type to remove the dep.lookup_type(&[], name) fallback and ensure
ResolvedName::Builtin is only produced when the matching dep export is found
under pkg_info.namespace_path.
In `@baml_language/crates/baml_compiler2_tir/src/throw_inference.rs`:
- Around line 104-114: The method branch currently calls process_callable(...)
with only method_data.generic_params, causing enclosing type generics to be
omitted; update the call in the method-handling path so process_callable
receives the full generic parameter slice that includes the enclosing type's
generics (i.e., merge or prepend the class/type generic params with
method_data.generic_params before passing), referencing process_callable and
method_data.generic_params (and the variable holding the class/type generics) to
ensure sig.throws and parameter function types are lowered with both type and
method generics.
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_tir/src/package_interface.rs`:
- Around line 412-443: build_self_type_for_class currently hardcodes the package
to baml when creating the QualifiedTypeName, causing implicit self to be
exported under the wrong package; change build_self_type_for_class to accept the
class's actual package (e.g. a Name or &Name package parameter) and use that
instead of Name::new(baml_base::BAML_PACKAGE) when calling
QualifiedTypeName::new_with_generic_params; then update the call sites (e.g.
where package_interface() and lookup_class_method() call
build_self_type_for_class) to pass the class file's package through so
synthesized self uses the enclosing class package.
In `@baml_language/crates/baml_compiler2_tir/src/throws_analysis.rs`:
- Around line 54-63: The current expr_to_path_segments only handles Expr::Path
and Expr::MemberAccess, so calls whose callee is an OptionalMemberAccess (e.g.,
obj?.method()) drop the member and produce no dotted path; update
expr_to_path_segments to also recognize OptionalMemberAccess (and Expr::Call
whose callee is OptionalMemberAccess) and treat it like MemberAccess for
path-building: recurse on the base (using expr_to_path_segments(*base, body)?),
push the member name, and return the combined segments so
throw_inference::collect_call_targets() and callable::named_callee_key() get the
full dotted target; use the existing function name expr_to_path_segments and
pattern-match on Expr::OptionalMemberAccess (and/or Expr::Call -> callee) to
locate the code to change.
🪄 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: e834ce64-c973-499c-9e33-976d1e4c4cfc
📒 Files selected for processing (32)
baml_language/crates/baml_builtins2_codegen/src/codegen.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/signature.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/analysis.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/callable.rsbaml_language/crates/baml_compiler2_tir/src/cycle_detector.rsbaml_language/crates/baml_compiler2_tir/src/exhaustiveness.rsbaml_language/crates/baml_compiler2_tir/src/generics.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/lib.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/narrowing.rsbaml_language/crates/baml_compiler2_tir/src/normalize.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler2_tir/src/pattern_lowering.rsbaml_language/crates/baml_compiler2_tir/src/resolve.rsbaml_language/crates/baml_compiler2_tir/src/throw_inference.rsbaml_language/crates/baml_compiler2_tir/src/throws_analysis.rsbaml_language/crates/baml_compiler2_tir/src/ty.rsbaml_language/crates/baml_compiler2_tir/src/user_facing.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_tests/src/compiler2_mir/mod.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/mod.rsbaml_language/crates/bex_sap/src/jsonish/parser/fixing_parser/json_collection.rsbaml_language/crates/bex_sap/src/jsonish/parser/fixing_parser/json_parse_state.rsbaml_language/crates/bex_vm/src/debug.rsbaml_language/crates/tools_sap_visualizer/src/ui.rs
💤 Files with no reviewable changes (2)
- baml_language/crates/baml_compiler2_tir/src/cycle_detector.rs
- baml_language/crates/baml_lsp2_actions/src/check.rs
Comment on lines
+103
to
120
| fn enclosing_class( | ||
| item_tree: &baml_compiler2_hir::item_tree::ItemTree, | ||
| function_id: baml_compiler2_hir::ids::LocalItemId<baml_compiler2_hir::ids::FunctionMarker>, | ||
| ) -> Vec<Name> { | ||
| ) -> Option<&baml_compiler2_hir::item_tree::Class> { | ||
| item_tree | ||
| .classes | ||
| .values() | ||
| .find(|class_data| class_data.methods.contains(&function_id)) | ||
| } | ||
|
|
||
| fn enclosing_class_generic_params( | ||
| item_tree: &baml_compiler2_hir::item_tree::ItemTree, | ||
| function_id: baml_compiler2_hir::ids::LocalItemId<baml_compiler2_hir::ids::FunctionMarker>, | ||
| ) -> Vec<Name> { | ||
| enclosing_class(item_tree, function_id) | ||
| .map(|class_data| class_data.generic_params.clone()) | ||
| .unwrap_or_default() | ||
| } |
Contributor
There was a problem hiding this comment.
Handle interface parents in the enclosing-type helpers.
These helpers only search item_tree.classes. For interface default methods, SignatureLoweringCtx::new() drops interface generic params when lowering declared/cycle-initial throws, and callable_short_name() later collapses Interface.method to bare method, which breaks BEP-044 default-method throws keys.
🤖 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/callable.rs` around lines 103 -
120, enclosing_class and enclosing_class_generic_params only search
item_tree.classes, so interface default methods are missed; update both to also
search item_tree.interfaces (or whatever collection holds interfaces in
ItemTree) for a class/interface that contains the given function_id and return
that interface (for enclosing_class) or its generic_params (for
enclosing_class_generic_params) when found; specifically modify enclosing_class
to check both item_tree.classes.values().find(...) and
item_tree.interfaces.values().find(...), and modify
enclosing_class_generic_params to map the found interface's generic_params as
well, falling back to default if neither is found.
Comment on lines
+521
to
+539
| fn lambda_enclosing_generics( | ||
| index: &baml_compiler2_hir::semantic_index::FileSemanticIndex<'_>, | ||
| item_tree: &baml_compiler2_hir::item_tree::ItemTree, | ||
| func_generic_params: Vec<Name>, | ||
| func_parent_fsi: Option<FileScopeId>, | ||
| ) -> Vec<Name> { | ||
| let mut gp = func_generic_params; | ||
| if let Some(parent_fsi) = func_parent_fsi { | ||
| let parent_scope = &index.scopes[parent_fsi.index() as usize]; | ||
| if matches!(parent_scope.kind, ScopeKind::Class) { | ||
| if let Some(class_name) = &parent_scope.name { | ||
| if let Some(class_generics) = enclosing_class_generics(item_tree, class_name) { | ||
| gp = prepend_generics(class_generics, gp); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| gp | ||
| } |
Contributor
There was a problem hiding this comment.
Include interface generics when seeding nested lambdas.
lambda_enclosing_generics() only looks in item_tree.classes, but the function-scope path below already has to fall back to item_tree.interfaces for BEP-044 default methods. A lambda inside a generic interface default method will now miss the enclosing T and lower it to Unknown.
🤖 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/inference.rs` around lines 521 -
539, lambda_enclosing_generics currently only prepends class generics via
enclosing_class_generics, so lambdas inside generic interface default methods
miss the interface type parameters; update lambda_enclosing_generics to also
check item_tree.interfaces (reuse or create an enclosing_interface_generics
helper analogous to enclosing_class_generics) and, when parent_scope.kind is
Interface or when enclosing_class_generics returns None, retrieve the
interface's generics and prepend them with prepend_generics to gp so interface
type parameters (e.g. T) are included for nested lambdas.
Comment on lines
+157
to
+193
| /// Resolve a type path against `package_items`, trying the `ns_context`-qualified | ||
| /// path first, then the cross-package fallback (`root.*` or a package-named first | ||
| /// segment). Returns the resolved [`Definition`], if any. | ||
| fn resolve_type<'db>( | ||
| db: &'db dyn crate::Db, | ||
| package_items: &PackageItems<'db>, | ||
| ns_context: &[baml_base::Name], | ||
| path: &[baml_base::Name], | ||
| item: &baml_base::Name, | ||
| ) -> Option<Definition<'db>> { | ||
| let seg_ns = &path[..path.len() - 1]; | ||
| // When we have a namespace context, try the qualified path first. | ||
| // e.g. for ns_context=["fs"], path=["File"], try namespace ["fs"] item "File". | ||
| let resolved = if !ns_context.is_empty() { | ||
| let ns: Vec<baml_base::Name> = ns_context.iter().chain(seg_ns.iter()).cloned().collect(); | ||
| package_items.lookup_type(&ns, item) | ||
| } else { | ||
| package_items.lookup_type(seg_ns, item) | ||
| }; | ||
| // Cross-package fallback: if not found in the current package and | ||
| // the first segment is a known package name, look in that package | ||
| // using the remaining segments. This supports synthetic type | ||
| // references like `baml.llm.PromptAst` in companion functions. | ||
| resolved.or_else(|| { | ||
| if path.len() >= 2 { | ||
| if path[0].as_str() == "root" { | ||
| package_items.lookup_type(&path[1..path.len() - 1], item) | ||
| } else { | ||
| let pkg_id = PackageId::new(db, path[0].clone()); | ||
| let pkg = baml_compiler2_ppir::package_items(db, pkg_id); | ||
| pkg.lookup_type(&path[1..path.len() - 1], item) | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Restore the package-root fallback in resolve_type.
When ns_context is non-empty, this only probes ns_context + seg_ns; on a miss, it jumps straight to explicit root.* / dependency lookup. That means a namespaced file can no longer resolve a type that exists only at the current package root, even though lower_type_expr_in_ns still documents that fallback. This will turn previously valid signature references into UnresolvedType.
💡 Suggested fix
- let resolved = if !ns_context.is_empty() {
- let ns: Vec<baml_base::Name> = ns_context.iter().chain(seg_ns.iter()).cloned().collect();
- package_items.lookup_type(&ns, item)
- } else {
- package_items.lookup_type(seg_ns, item)
- };
+ let resolved = if !ns_context.is_empty() {
+ let ns: Vec<baml_base::Name> = ns_context.iter().chain(seg_ns.iter()).cloned().collect();
+ package_items
+ .lookup_type(&ns, item)
+ .or_else(|| package_items.lookup_type(seg_ns, item))
+ } else {
+ package_items.lookup_type(seg_ns, item)
+ };🤖 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/lower_type_expr.rs` around lines
157 - 193, resolve_type currently tries ns_context+seg_ns then immediately goes
to cross-package lookup, skipping a lookup in the current package root (seg_ns)
when ns_context is non-empty; update resolve_type so it tries three lookups in
order: 1) package_items.lookup_type(&ns, item) where ns = ns_context + seg_ns,
2) package_items.lookup_type(seg_ns, item) to allow package-root types, and only
then 3) perform the cross-package fallback (the existing or_else block that
checks path[0]=="root" or uses PackageId::new/db with
baml_compiler2_ppir::package_items and pkg.lookup_type). Keep the existing
variables (seg_ns, ns_context, path, item) and methods
(package_items.lookup_type, PackageId::new, baml_compiler2_ppir::package_items)
to locate and implement the change in resolve_type.
Comment on lines
+239
to
247
| // Co-inductive: if we've assumed this pair, it holds. (Only the | ||
| // structural-recursion cases below can ever populate `assumptions`, so | ||
| // this check is deferred past the scalar early-returns above — none of | ||
| // which can apply to an already-assumed pair.) | ||
| let pair = (self.clone(), other.clone()); | ||
| if assumptions.contains(&pair) { | ||
| return true; | ||
| } | ||
| assumptions.insert(pair.clone()); |
Contributor
There was a problem hiding this comment.
Always clear the co-inductive assumption before returning.
assumptions.insert(pair) now happens before the match, but the function-subtyping arm still exits with return false. That skips the cleanup at Line 364, so the current pair can stay cached and make a later sibling recursive comparison succeed incorrectly.
Suggested fix
(
StructuralTy::Function {
params: params1,
ret: ret1,
throws: throws1,
@@
},
) => {
- if !ret1.is_subtype_of(ret2, assumptions) {
- return false;
- }
- if !throws1.is_subtype_of(throws2, assumptions) {
- return false;
- }
- function_params_subtype(params1, params2, assumptions)
+ ret1.is_subtype_of(ret2, assumptions)
+ && throws1.is_subtype_of(throws2, assumptions)
+ && function_params_subtype(params1, params2, assumptions)
}Also applies to: 351-358
🤖 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/normalize.rs` around lines 239 -
247, The co-inductive pair (variable pair) is inserted into assumptions before
the match but some arms (e.g., the function-subtyping arm that does an early
`return false`) exit without removing it, causing stale assumptions to leak;
update the logic in normalize.rs so that every early return removes the inserted
pair first (call assumptions.remove(&pair) before returning) or move the insert
to just before the recursive/structural paths and only insert when you can
guarantee a matching removal, and apply the same fix for the similar block
around the other occurrence (the block referenced by the comment covering lines
~351-358) to ensure no return path leaves assumptions containing the pair.
Comment on lines
+89
to
+109
| // For types, resolve_type returns Ty — we need Definition, so fall back to direct | ||
| // lookup. resolve_type's success is only a guard; re-derive the Definition by | ||
| // walking own items (Item) then deps (Builtin, also probing the root namespace). | ||
| if res_ctx | ||
| .resolve_type(db, name_path, &pkg_info.namespace_path) | ||
| .is_some() | ||
| { | ||
| if let Some(def) = res_ctx | ||
| .own_items | ||
| .lookup_type(&pkg_info.namespace_path, name) | ||
| { | ||
| return ResolvedName::Item(def); | ||
| } | ||
| for (dep_name, _) in &res_ctx.dep_interfaces { | ||
| if let Some(def) = res_ctx.items_for_package(db, dep_name).and_then(|dep| { | ||
| dep.lookup_type(&pkg_info.namespace_path, name) | ||
| .or_else(|| dep.lookup_type(&[], name)) | ||
| }) { | ||
| return ResolvedName::Builtin(def); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the post-resolve_type() lookup on the same namespace path.
This second walk widens dependency lookup with dep.lookup_type(&[], name). In a namespaced file, that can return the wrong dependency definition when one dep exports ns::Foo and another exports root Foo: resolve_type() picks the namespaced item, but the follow-up loop can stop on the root item from a different dep. Please keep the re-derivation aligned with the original bare-name search here, or return the Definition directly from the first lookup to avoid the double search entirely. A unit test for two deps with colliding Foo names would pin this down.
Possible localized fix
- for (dep_name, _) in &res_ctx.dep_interfaces {
- if let Some(def) = res_ctx.items_for_package(db, dep_name).and_then(|dep| {
- dep.lookup_type(&pkg_info.namespace_path, name)
- .or_else(|| dep.lookup_type(&[], name))
- }) {
+ for (dep_name, _) in &res_ctx.dep_interfaces {
+ if let Some(def) = res_ctx
+ .items_for_package(db, dep_name)
+ .and_then(|dep| dep.lookup_type(&pkg_info.namespace_path, name))
+ {
return ResolvedName::Builtin(def);
}
}🤖 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/resolve.rs` around lines 89 -
109, The follow-up lookup widens scope by calling dep.lookup_type(&[], name),
causing a root-namespace hit from a different dep to be chosen even when
resolve_type() matched the namespaced item; change the post-resolve_type
re-derivation so it only searches the same namespace path used earlier (i.e. use
pkg_info.namespace_path for dep.lookup_type) or, better, have resolve_type
return the Definition directly so you can return that Definition instead of
performing the second, wider search; update the logic around resolve_type,
own_items.lookup_type and res_ctx.items_for_package / dep.lookup_type to remove
the dep.lookup_type(&[], name) fallback and ensure ResolvedName::Builtin is only
produced when the matching dep export is found under pkg_info.namespace_path.
Comment on lines
+104
to
+114
| process_callable( | ||
| db, | ||
| pkg_items, | ||
| func_loc, | ||
| key, | ||
| &method_data.generic_params, | ||
| Some(class_name), | ||
| &mut direct_facts, | ||
| &mut has_declared_contract, | ||
| &mut call_edges, | ||
| ); |
Contributor
There was a problem hiding this comment.
Pass enclosing type generics into method throw lowering.
process_callable() lowers sig.throws and parameter function types with the generic_params slice, but the method branch only passes method_data.generic_params. class Box<T> { function f(self) throws T { ... } } will therefore degrade the declared firewall to Unknown, so callers see the wrong transitive throws.
🤖 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/throw_inference.rs` around lines
104 - 114, The method branch currently calls process_callable(...) with only
method_data.generic_params, causing enclosing type generics to be omitted;
update the call in the method-handling path so process_callable receives the
full generic parameter slice that includes the enclosing type's generics (i.e.,
merge or prepend the class/type generic params with method_data.generic_params
before passing), referencing process_callable and method_data.generic_params
(and the variable holding the class/type generics) to ensure sig.throws and
parameter function types are lowered with both type and method generics.
Contributor
|
i wonder if we can get better reesults if we run And also if we formally design how subtyping rules should be modeled across exhaustiveness checks, match, variable assignments, implicit casts etc. |
Consolidate in-crate test fixtures behind a shared Ty/PatCtx fixture DSL (test-only). Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Delete the derived InterfaceImplRuleIndex in favor of the existing linear scan. Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Replace the 3-toggle ThrowsAnalysisContext decorator with one ThrowsMode field. Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Delete the always-default TyAttr field from all 23 TIR Ty variants (shim-first, then variant-by-variant). Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Make body: &ExprBody a TypeInferenceBuilder field instead of threading it through 63 methods / 209 call sites. Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Smart constructors for Ty/Literal to de-noise the constant folder (post-tyattr scope). Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Collapse throws walk (context, body, out) and HIR-throws accumulators into structs. Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Bundle (generic_params, aliases) into a MatchCtx struct (12 fns / 67 sites). Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Collapse the per-body inference map set (4x duplication + 14-tuple finish + doubled MIR merge) into one InferenceMaps newtype. Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Collapse DefaultParameterInference into ScopeInference via an arena-tagged key inside InferenceMaps (builds on inference-maps-newtype). Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Unify DPat and WitnessPat into one Pat type (type-alias-first). Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Inline the single-instantiation analysis.rs dataflow framework into throw_inference.rs. Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Unify the two Tarjan/NodeState SCC implementations into one generic core + deterministic post-pass. Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Route lowering diagnostics through a sink instead of 38 throwaway Vec drains. Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
Bundle (db, package_items, ns_context) into a LoweringCtx struct (depends on diagnostic sink). Verified: unit + full baml_tests snapshot suite green, zero snapshot drift, workspace clippy clean, adversarial diff review passed.
… lines) diagnostics-dual-channel-collapse — collapse duplicated TEMPLATES (renderer-parameterized), NOT the renderers. Verified: unit + full baml_tests snapshot suite green, 7 wording-only snapshot updates (reviewed), workspace clippy clean, adversarial diff review passed.
model.md: architecture, data-flow map, type clusters, and the 189-entry invariant register built by the survey workflow and verified against the code. invariants.md: the distilled contract — 77 frozen behavioral F-invariants, 26 changeable S-invariants, and corrected/false claims documented as bugs-to-preserve. These doubled as the executor's contract for the 16 semantic refactors and are the starting spec for any future TIR rewrite. Whitelisted in .markdown-whitelist as long-term docs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
4870-4882:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSeed receiver class type args on interface-dispatched calls too.
This new seeding only runs on the plain call path below.
try_lower_interface_dispatch/emit_interface_dispatch_switchreturn before Line 4870, so a generic class method invoked through an interface still enters the callee without the matched receiver's class type args. That mis-specializes overrides/defaults behind nongeneric or partially pinned interface guards. Based on learnings: "when class-owned override arms are dispatched through an interface withInterfaceClassGuard::Anyor a partial guard on a generic class, seed the callee frame from the matched receiver instance by binding the receiver into aBoundMethod; do not use the caller'sframe.type_argsas a fallback."🤖 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 4870 - 4882, The interface-dispatch path isn't seeding class type args like the plain call path does, so generic class methods invoked via try_lower_interface_dispatch/emit_interface_dispatch_switch can miss the receiver's class_type_args; update the interface-dispatch logic to perform the same seeding as the Tir2Ty::Class branch: when a matched receiver has non-empty class_type_args, obtain enclosing_generic_params(), convert each ty_arg via ty_to_template, allocate temps with builder.temp(Ty::type_type()), assign them with builder.assign(Place::local(temp), Rvalue::LoadType(template)) and use Operand::Copy(Place::local(temp)) to populate the callee's bound type args; additionally, when dispatching class-owned overrides through an interface with InterfaceClassGuard::Any or a partial guard, bind the receiver into a BoundMethod and use its seeded type args instead of falling back to frame.type_args.baml_language/crates/baml_compiler2_tir/src/generics.rs (2)
518-544:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't erase function-local generic binders.
This branch recurses through
Ty::Functionwith the outershould_erasepredicate unchanged, so an outer erase ofTwill turnfn<T>(x: T) -> Tinto a function that still declaresTbut usesunknownin its params/return.substitute_ty()already handles this binder-shadowing case correctly;erase_typevars_matching()needs the same treatment.🤖 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/generics.rs` around lines 518 - 544, The Ty::Function arm of erase_typevars_matching currently reuses the outer should_erase predicate when recursing, which incorrectly erases typevars bound by the function's own generic_params; change it so that before recursing into generic_param_bounds, params, ret, and throws you build a new predicate/closure that treats any type variable declared in generic_params as not erasable (i.e., shadow them) and use that new predicate for the recursive erase_typevars_matching calls; mirror how substitute_ty handles binder-shadowing so generic_params remain declared but their uses are turned into unknown only when not shadowed.
318-341:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Ty::Futureis skipped bycontains_typevar, so future-contained type vars never get erased.
erase_typevars_matching()bails out early on Line 477, butcontains_typevar()never looks insideTy::Future. A type likefuture<T, never>will therefore bypass erasure entirely and leak unresolved type vars into the MIR/emit cleanup path.Proposed fix
- Ty::Map(k, v) | Ty::EvolvingMap(k, v) => contains_typevar(k) || contains_typevar(v), + Ty::Map(k, v) | Ty::EvolvingMap(k, v) | Ty::Future(k, v) => { + contains_typevar(k) || contains_typevar(v) + }Also applies to: 476-479
🤖 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/generics.rs` around lines 318 - 341, contains_typevar currently skips Ty::Future so type variables inside futures never get detected; add a match arm for the Ty::Future variant that recurses into its inner type parameters (e.g., check both the output and error type like contains_typevar(output) || contains_typevar(error)) in the contains_typevar function, and make the analogous change in the related erase_typevars_matching logic so futures are considered for erasure as well.baml_language/crates/baml_compiler2_tir/src/inference.rs (1)
1218-1244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't stop the enclosing-generic walk at an intermediate lambda.
The recovery loop breaks on any non-
Function/Letscope, sofunction f<T>() { let outer = || { let inner = || reflect.type_of<T>() } }losesTforinneras soon as it hits the outer lambda scope. This path needs to keep walking upward through lambda scopes until it finds the enclosing function context.Minimal direction
- ScopeKind::Let => { + ScopeKind::Let | ScopeKind::Lambda => { current = scope.parent; } _ => break,🤖 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/inference.rs` around lines 1218 - 1244, The walk up from ancestor_scope.parent stops on any non-Function/Let scope, which incorrectly halts at intermediate lambdas; update the loop in inference.rs (the block using current, ScopeKind::Function, ScopeKind::Let and calling lambda_enclosing_generics) so that lambda/closure scopes are treated like Let (i.e., set current = scope.parent and continue) rather than breaking — only break when you find a Function (handle via the existing Function branch) or when current becomes None; adjust the match arms accordingly so enclosing generics from outer functions (via item_tree.functions, fd.generic_params) are discovered for nested lambdas.baml_language/crates/baml_compiler2_tir/src/throw_inference.rs (1)
376-379:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't fall back bare throw-type lookups to the root namespace.
For a bare type path in a namespaced file, this helper now tries
ns_contextfirst and then falls back tolookup_type([], name). That reintroduces the cross-namespace bare-name fallback the compiler intentionally removed, so throw recovery can attribute a throw to the wrong root type instead of leaving it unresolved. Explicitroot.*paths already have a dedicated branch below.Suggested fix
fn lookup_type_ns_first<'db>( pkg_items: &PackageItems<'db>, ns_context: &[Name], ns: &[Name], name: &Name, ) -> Option<Definition<'db>> { if !ns_context.is_empty() && ns.is_empty() { - pkg_items - .lookup_type(ns_context, name) - .or_else(|| pkg_items.lookup_type(ns, name)) + pkg_items.lookup_type(ns_context, name) } else { pkg_items.lookup_type(ns, name) } }Based on learnings: bare name lookups intentionally use only
package_items.lookup_type(&self.ns_context, name)scoped to the file's local namespace.🤖 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/throw_inference.rs` around lines 376 - 379, The current lookup in throw_inference.rs falls back from pkg_items.lookup_type(ns_context, name) to pkg_items.lookup_type(ns, name) when ns_context is non-empty and ns is empty, reintroducing cross-namespace bare-name resolution; change the logic so bare-name lookups only call pkg_items.lookup_type(ns_context, name) (remove the or_else(|| pkg_items.lookup_type(ns, name)) fallback) and keep explicit root.* handling in the dedicated branch below; update the code path that uses ns_context, ns, and pkg_items.lookup_type to ensure only the ns_context lookup is performed for bare names.
🤖 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`:
- Line 6579: The loop labeled 'blanket_search' currently breaks out on the first
matching impl, which prevents collecting subsequent out-of-body blanket-impl
candidates (see the loop starting with 'blanket_search: for file in
compiler2_all_files(self.db)'). Change the control flow so that finding a match
does not break the outer 'blanket_search' loop — continue scanning files to
collect all blanket-impl candidates (replace the break with a continue or remove
the early-break logic), and ensure any inner-loop short-circuits only skip the
current file/impl rather than exiting the whole search; apply the same fix at
the analogous spots around the other occurrences mentioned (the similar breaks
at the locations corresponding to lines ~6772 and ~6792).
- Around line 1532-1535: The check is erasing class generics by constructing
Ty::Class(class_qtn.clone(), Vec::new()), causing generic classes to appear
monomorphic; instead construct the actual Ty::Class including its original
generic arguments so blanket impl rules that depend on class type parameters can
match. Locate where class_qtn is iterated (registry.class_implements.keys()) and
replace the empty Vec with the class's real type parameters (e.g., fetch the
class's generic args from the registry/type definition or a canonical
instantiation for that class) before calling
registry.type_implements_interface_via_rule so the implementation check uses the
correct generic instantiation.
In `@baml_language/crates/baml_compiler2_tir/src/ty.rs`:
- Around line 480-495: The widen_fresh implementation currently widens literal
type args for Ty::Class but omits similar handling for Ty::Interface and
Ty::Future, allowing narrow Interface/ Future wrappers to remain; update
Ty::widen_fresh to mirror the Class branch by adding arms that recursively call
widen_fresh on Interface's type_args (e.g., Ty::Interface(name, type_args) =>
Ty::Interface(name, widened_type_args)) and on Future's contained types (e.g.,
Ty::Future(ok_ty, err_ty) => Ty::Future(Box::new(ok_ty.widen_fresh()),
Box::new(err_ty.widen_fresh()))), ensuring all inner/type-argument positions are
widened and returned.
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 4870-4882: The interface-dispatch path isn't seeding class type
args like the plain call path does, so generic class methods invoked via
try_lower_interface_dispatch/emit_interface_dispatch_switch can miss the
receiver's class_type_args; update the interface-dispatch logic to perform the
same seeding as the Tir2Ty::Class branch: when a matched receiver has non-empty
class_type_args, obtain enclosing_generic_params(), convert each ty_arg via
ty_to_template, allocate temps with builder.temp(Ty::type_type()), assign them
with builder.assign(Place::local(temp), Rvalue::LoadType(template)) and use
Operand::Copy(Place::local(temp)) to populate the callee's bound type args;
additionally, when dispatching class-owned overrides through an interface with
InterfaceClassGuard::Any or a partial guard, bind the receiver into a
BoundMethod and use its seeded type args instead of falling back to
frame.type_args.
In `@baml_language/crates/baml_compiler2_tir/src/generics.rs`:
- Around line 518-544: The Ty::Function arm of erase_typevars_matching currently
reuses the outer should_erase predicate when recursing, which incorrectly erases
typevars bound by the function's own generic_params; change it so that before
recursing into generic_param_bounds, params, ret, and throws you build a new
predicate/closure that treats any type variable declared in generic_params as
not erasable (i.e., shadow them) and use that new predicate for the recursive
erase_typevars_matching calls; mirror how substitute_ty handles binder-shadowing
so generic_params remain declared but their uses are turned into unknown only
when not shadowed.
- Around line 318-341: contains_typevar currently skips Ty::Future so type
variables inside futures never get detected; add a match arm for the Ty::Future
variant that recurses into its inner type parameters (e.g., check both the
output and error type like contains_typevar(output) || contains_typevar(error))
in the contains_typevar function, and make the analogous change in the related
erase_typevars_matching logic so futures are considered for erasure as well.
In `@baml_language/crates/baml_compiler2_tir/src/inference.rs`:
- Around line 1218-1244: The walk up from ancestor_scope.parent stops on any
non-Function/Let scope, which incorrectly halts at intermediate lambdas; update
the loop in inference.rs (the block using current, ScopeKind::Function,
ScopeKind::Let and calling lambda_enclosing_generics) so that lambda/closure
scopes are treated like Let (i.e., set current = scope.parent and continue)
rather than breaking — only break when you find a Function (handle via the
existing Function branch) or when current becomes None; adjust the match arms
accordingly so enclosing generics from outer functions (via item_tree.functions,
fd.generic_params) are discovered for nested lambdas.
In `@baml_language/crates/baml_compiler2_tir/src/throw_inference.rs`:
- Around line 376-379: The current lookup in throw_inference.rs falls back from
pkg_items.lookup_type(ns_context, name) to pkg_items.lookup_type(ns, name) when
ns_context is non-empty and ns is empty, reintroducing cross-namespace bare-name
resolution; change the logic so bare-name lookups only call
pkg_items.lookup_type(ns_context, name) (remove the or_else(||
pkg_items.lookup_type(ns, name)) fallback) and keep explicit root.* handling in
the dedicated branch below; update the code path that uses ns_context, ns, and
pkg_items.lookup_type to ensure only the ns_context lookup is performed for bare
names.
🪄 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: 12777b09-c11e-423a-8451-b0586aca2ae0
⛔ Files ignored due to path filters (5)
baml_language/crates/baml_tests/snapshots/diagnostic_errors/format_checks/baml_tests__diagnostic_errors__format_checks__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/is_pattern_negative/baml_tests__diagnostic_errors__is_pattern_negative__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/match_exhaustiveness/baml_tests__diagnostic_errors__match_exhaustiveness__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/patterns_class_destructure/baml_tests__diagnostic_errors__patterns_class_destructure__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/patterns_new/baml_tests__diagnostic_errors__patterns_new__04_tir.snapis excluded by!**/*.snap
📒 Files selected for processing (37)
baml_language/.markdown-whitelistbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/docs/invariants.mdbaml_language/crates/baml_compiler2_tir/docs/model.mdbaml_language/crates/baml_compiler2_tir/src/analysis.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/callable.rsbaml_language/crates/baml_compiler2_tir/src/exhaustiveness.rsbaml_language/crates/baml_compiler2_tir/src/generics.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/lib.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/narrowing.rsbaml_language/crates/baml_compiler2_tir/src/normalize.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler2_tir/src/pattern_lowering.rsbaml_language/crates/baml_compiler2_tir/src/test_support.rsbaml_language/crates/baml_compiler2_tir/src/throw_inference.rsbaml_language/crates/baml_compiler2_tir/src/throws_analysis.rsbaml_language/crates/baml_compiler2_tir/src/ty.rsbaml_language/crates/baml_lsp2_actions/src/annotations.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/completions.rsbaml_language/crates/baml_lsp2_actions/src/definition.rsbaml_language/crates/baml_lsp2_actions/src/describe.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_lsp2_actions/src/usages.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_tests/src/compiler2_tir/inference.rsbaml_language/crates/baml_tests/src/compiler2_tir/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase5.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase6.rsbaml_language/crates/tools_onionskin/src/compiler.rs
💤 Files with no reviewable changes (1)
- baml_language/crates/baml_compiler2_tir/src/analysis.rs
✅ Files skipped from review due to trivial changes (3)
- baml_language/.markdown-whitelist
- baml_language/crates/baml_tests/src/compiler2_tir/phase6.rs
- baml_language/crates/baml_compiler2_tir/docs/model.md
🚧 Files skipped from review as they are similar to previous changes (3)
- baml_language/crates/baml_compiler2_tir/src/throws_analysis.rs
- baml_language/crates/baml_compiler2_tir/src/callable.rs
- baml_language/crates/baml_compiler2_tir/src/package_interface.rs
Comment on lines
1532
to
1535
| for class_qtn in registry.class_implements.keys() { | ||
| let actual = baml_compiler2_tir::ty::Ty::Class( | ||
| class_qtn.clone(), | ||
| Vec::new(), | ||
| baml_compiler2_tir::ty::TyAttr::default(), | ||
| ); | ||
| let actual = | ||
| baml_compiler2_tir::ty::Ty::Class(class_qtn.clone(), Vec::new()); | ||
| if !registry.type_implements_interface_via_rule( |
Contributor
There was a problem hiding this comment.
Don't erase class generics when checking bound-interface blanket impls.
Re-wrapping each candidate as Ty::Class(class_qtn.clone(), Vec::new()) makes every generic class look monomorphic here. A rule like implements I for T where T: J<string> will never register Pair<string, _>/Box<string, _> as implementors if the proof of J<string> depends on the receiver's class args, so later interface dispatch has no class arm even for matching concrete instantiations.
🤖 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 1532 -
1535, The check is erasing class generics by constructing
Ty::Class(class_qtn.clone(), Vec::new()), causing generic classes to appear
monomorphic; instead construct the actual Ty::Class including its original
generic arguments so blanket impl rules that depend on class type parameters can
match. Locate where class_qtn is iterated (registry.class_implements.keys()) and
replace the empty Vec with the class's real type parameters (e.g., fetch the
class's generic args from the registry/type definition or a canonical
instantiation for that class) before calling
registry.type_implements_interface_via_rule so the implementation check uses the
correct generic instantiation.
| baml_compiler2_tir::ty::TyAttr::default(), | ||
| ); | ||
| let candidate_class_ty = Tir2Ty::Class(candidate_class_qtn.clone(), Vec::new()); | ||
| 'blanket_search: for file in compiler2_all_files(self.db) { |
Contributor
There was a problem hiding this comment.
Keep collecting blanket-impl candidates after the first match.
The outer break 'blanket_search drops every later out-of-body impl candidate for the same class. That breaks the partial-guard scenario this refactor just enabled: e.g. Pair<L, R> implementing the same interface twice with different pinned params only emits one dispatch arm, so one valid receiver instantiation falls through to unreachable.
Also applies to: 6772-6772, 6792-6792
🤖 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` at line 6579, The loop
labeled 'blanket_search' currently breaks out on the first matching impl, which
prevents collecting subsequent out-of-body blanket-impl candidates (see the loop
starting with 'blanket_search: for file in compiler2_all_files(self.db)').
Change the control flow so that finding a match does not break the outer
'blanket_search' loop — continue scanning files to collect all blanket-impl
candidates (replace the break with a continue or remove the early-break logic),
and ensure any inner-loop short-circuits only skip the current file/impl rather
than exiting the whole search; apply the same fix at the analogous spots around
the other occurrences mentioned (the similar breaks at the locations
corresponding to lines ~6772 and ~6792).
Comment on lines
480
to
495
| pub fn widen_fresh(self) -> Ty { | ||
| match self { | ||
| Ty::Literal(lit, Freshness::Fresh, attr) => { | ||
| Ty::Primitive(PrimitiveType::from_literal(&lit), attr) | ||
| } | ||
| Ty::Union(members, attr) => { | ||
| Ty::Literal(lit, Freshness::Fresh) => Ty::Primitive(PrimitiveType::from_literal(&lit)), | ||
| Ty::Union(members) => { | ||
| let widened: Vec<Ty> = members.into_iter().map(Ty::widen_fresh).collect(); | ||
| dedup_and_collapse(widened, attr) | ||
| dedup_and_collapse(widened) | ||
| } | ||
| Ty::List(inner, attr) => Ty::List(Box::new((*inner).widen_fresh()), attr), | ||
| Ty::Map(k, v, attr) => Ty::Map( | ||
| Box::new((*k).widen_fresh()), | ||
| Box::new((*v).widen_fresh()), | ||
| attr, | ||
| ), | ||
| Ty::Optional(inner, attr) => Ty::Optional(Box::new((*inner).widen_fresh()), attr), | ||
| Ty::Class(name, type_args, attr) => { | ||
| Ty::List(inner) => Ty::List(Box::new((*inner).widen_fresh())), | ||
| Ty::Map(k, v) => Ty::Map(Box::new((*k).widen_fresh()), Box::new((*v).widen_fresh())), | ||
| Ty::Optional(inner) => Ty::Optional(Box::new((*inner).widen_fresh())), | ||
| Ty::Class(name, type_args) => { | ||
| let widened: Vec<Ty> = type_args.into_iter().map(Ty::widen_fresh).collect(); | ||
| Ty::Class(name, widened, attr) | ||
| Ty::Class(name, widened) | ||
| } | ||
| other => other, | ||
| } |
Contributor
There was a problem hiding this comment.
widen_fresh stops too early for Interface and Future wrappers.
This now widens literal type args through Class, but not through the parallel Interface/Future shells. A mutable binding can therefore keep Interface<1> or Future<1, never> narrower than the equivalent class case and trip later assignability checks.
Suggested fix
match self {
Ty::Literal(lit, Freshness::Fresh) => Ty::Primitive(PrimitiveType::from_literal(&lit)),
Ty::Union(members) => {
let widened: Vec<Ty> = members.into_iter().map(Ty::widen_fresh).collect();
dedup_and_collapse(widened)
}
Ty::List(inner) => Ty::List(Box::new((*inner).widen_fresh())),
Ty::Map(k, v) => Ty::Map(Box::new((*k).widen_fresh()), Box::new((*v).widen_fresh())),
Ty::Optional(inner) => Ty::Optional(Box::new((*inner).widen_fresh())),
Ty::Class(name, type_args) => {
let widened: Vec<Ty> = type_args.into_iter().map(Ty::widen_fresh).collect();
Ty::Class(name, widened)
}
+ Ty::Interface(name, type_args) => {
+ let widened: Vec<Ty> = type_args.into_iter().map(Ty::widen_fresh).collect();
+ Ty::Interface(name, widened)
+ }
+ Ty::Future(value, error) => Ty::Future(
+ Box::new((*value).widen_fresh()),
+ Box::new((*error).widen_fresh()),
+ ),
other => other,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn widen_fresh(self) -> Ty { | |
| match self { | |
| Ty::Literal(lit, Freshness::Fresh, attr) => { | |
| Ty::Primitive(PrimitiveType::from_literal(&lit), attr) | |
| } | |
| Ty::Union(members, attr) => { | |
| Ty::Literal(lit, Freshness::Fresh) => Ty::Primitive(PrimitiveType::from_literal(&lit)), | |
| Ty::Union(members) => { | |
| let widened: Vec<Ty> = members.into_iter().map(Ty::widen_fresh).collect(); | |
| dedup_and_collapse(widened, attr) | |
| dedup_and_collapse(widened) | |
| } | |
| Ty::List(inner, attr) => Ty::List(Box::new((*inner).widen_fresh()), attr), | |
| Ty::Map(k, v, attr) => Ty::Map( | |
| Box::new((*k).widen_fresh()), | |
| Box::new((*v).widen_fresh()), | |
| attr, | |
| ), | |
| Ty::Optional(inner, attr) => Ty::Optional(Box::new((*inner).widen_fresh()), attr), | |
| Ty::Class(name, type_args, attr) => { | |
| Ty::List(inner) => Ty::List(Box::new((*inner).widen_fresh())), | |
| Ty::Map(k, v) => Ty::Map(Box::new((*k).widen_fresh()), Box::new((*v).widen_fresh())), | |
| Ty::Optional(inner) => Ty::Optional(Box::new((*inner).widen_fresh())), | |
| Ty::Class(name, type_args) => { | |
| let widened: Vec<Ty> = type_args.into_iter().map(Ty::widen_fresh).collect(); | |
| Ty::Class(name, widened, attr) | |
| Ty::Class(name, widened) | |
| } | |
| other => other, | |
| } | |
| pub fn widen_fresh(self) -> Ty { | |
| match self { | |
| Ty::Literal(lit, Freshness::Fresh) => Ty::Primitive(PrimitiveType::from_literal(&lit)), | |
| Ty::Union(members) => { | |
| let widened: Vec<Ty> = members.into_iter().map(Ty::widen_fresh).collect(); | |
| dedup_and_collapse(widened) | |
| } | |
| Ty::List(inner) => Ty::List(Box::new((*inner).widen_fresh())), | |
| Ty::Map(k, v) => Ty::Map(Box::new((*k).widen_fresh()), Box::new((*v).widen_fresh())), | |
| Ty::Optional(inner) => Ty::Optional(Box::new((*inner).widen_fresh())), | |
| Ty::Class(name, type_args) => { | |
| let widened: Vec<Ty> = type_args.into_iter().map(Ty::widen_fresh).collect(); | |
| Ty::Class(name, widened) | |
| } | |
| Ty::Interface(name, type_args) => { | |
| let widened: Vec<Ty> = type_args.into_iter().map(Ty::widen_fresh).collect(); | |
| Ty::Interface(name, widened) | |
| } | |
| Ty::Future(value, error) => Ty::Future( | |
| Box::new((*value).widen_fresh()), | |
| Box::new((*error).widen_fresh()), | |
| ), | |
| other => other, | |
| } |
🤖 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/ty.rs` around lines 480 - 495,
The widen_fresh implementation currently widens literal type args for Ty::Class
but omits similar handling for Ty::Interface and Ty::Future, allowing narrow
Interface/ Future wrappers to remain; update Ty::widen_fresh to mirror the Class
branch by adding arms that recursively call widen_fresh on Interface's type_args
(e.g., Ty::Interface(name, type_args) => Ty::Interface(name, widened_type_args))
and on Future's contained types (e.g., Ty::Future(ok_ty, err_ty) =>
Ty::Future(Box::new(ok_ty.widen_fresh()), Box::new(err_ty.widen_fresh()))),
ensuring all inner/type-argument positions are widened and returned.
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.
What
An experiment in using multi-agent workflows to shrink
baml_compiler2_tirwhile keeping behaviour identical — first two mechanical de-slopify passes, then a semantic pass that built a verified model of the crate and executed major refactors against it.Result:
baml_compiler2_tirshrank 30,609 → 26,698 lines (−3,911, −12.8%) with no tests removed, 131/131 crate + 4,991/4,991 workspace tests passing, the full ~1,234-snapshot insta suite intact, and the workspacecargo clippy --all-targets --all-features -- -D warningsclean.Stage 1 — Mechanical passes (
8fb72ec6,b9c5d7c0): −2,415 linesA pipelined workflow (per-file → call-graph groups → whole-crate, each analyze→fix, gated by compile + tests after every wave) hunting duplication, dead code, redundant helpers, comment rot, needless abstraction, dead surface, and magic strings. Big files (
builder.rs, ~13k lines) got section-mapped deep analysis so nothing was skimmed. Safety rails: workspace-wide usage check before any deletion (salsa/trait dispatch aware), test-integrity gates that restore any wrongly-deleted test from git.Highlights: unified duplicate throws walkers,
#[derive(salsa::Update)]over hand-written unsafe impls, deleted dead modules/variants/params (cycle_detector, 4 never-constructedTirTypeErrorvariants, dead pub params) with cross-crate call-site updates.Stage 2 — Semantic pass (16 commits,
3a420db2..fafd96de): −1,496 linesA two-phase pipeline:
Snapshot policy: byte-identical snapshots, with one reviewed relaxation — purely wording changes to diagnostics (same kind, same span, same count) could be accepted per-snapshot by the verifier. Exactly 7 snapshots changed across the whole run, all wording-only, all on the final commit (collapsing the two drifted duplicate diagnostic-message channels into one).
Headline refactors: deleted the provably-always-default
TyAttrpayload from all 23Tyvariants (−708 alone);body: &ExprBodyun-threaded from 63 methods/209 call sites into builder state; the per-body inference map set (duplicated across 4 structs + a 14-tuple) collapsed into oneInferenceMaps;DPat/WitnessPatunified; single-use dataflow framework inlined; duplicate Tarjan SCC impls unified; diagnostics routed through a sink.Why this is trustworthy
model.md/invariants.md, kept outside the repo) was verified by falsification — several "invariants" turned out to be corrected/false claims and are documented as bugs-to-preserve rather than silently "fixed".🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation