Repository navigation
[codex] Implement interface v2 semantics - #3555
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds BEP-044 interfaces end-to-end: keywords and ChangesBEP-044 interface system
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
aaronvg
marked this pull request as ready for review
May 21, 2026 06:50
Binary size checks passed✅ 7 passed
Generated by |
![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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
baml_language/_plan/BEP-044-interfaces/IMPLEMENTATION_PLAN.md (1)
1-690:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis implementation plan is obsolete and contradicts the current design.
IMPLEMENTATIONv2.md(lines 3-4) explicitly states it supersedes this document. The two plans have fundamentally incompatible designs:
- This plan (Gap 3, lines 170-278): "Fields must be redeclared inside
implementsblocks"- IMPLEMENTATIONv2.md (lines 31-32): "Fields inside
implementsblocks. They should become invalid syntax"- README.md (line 422): "Fields live on the class, not inside the
implementsblock"Keeping both documents in the repository will confuse implementers about which design to follow.
📝 Recommended action
Either:
- Delete this file and rely solely on IMPLEMENTATIONv2.md, or
- Add a prominent deprecation notice at the top:
# ⚠️ OBSOLETE - DO NOT USE This implementation plan has been superseded by [IMPLEMENTATIONv2.md](./IMPLEMENTATIONv2.md). The design documented here is not being implemented. --- # BEP-044 Interfaces — Implementation Fix Plan (OBSOLETE)🤖 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/_plan/BEP-044-interfaces/IMPLEMENTATION_PLAN.md` around lines 1 - 690, The IMPLEMENTATION_PLAN.md is obsolete and conflicts with IMPLEMENTATIONv2.md and README.md; remove ambiguity by either deleting BEP-044 Interfaces — Implementation Fix Plan (the file shown) or prepend a clear deprecation banner referencing IMPLEMENTATIONv2.md and indicating it supersedes this document (so readers and implementers only follow IMPLEMENTATIONv2.md); ensure the banner mentions IMPLEMENTATIONv2.md and README.md to make the canonical source explicit.baml_language/crates/baml_lsp2_actions/src/describe.rs (1)
687-701:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInterface describe path is still blocked by item-node filtering.
Line 937 adds interface dependency handling, but
find_item_range/docstring extraction rely onis_item_node, and Line 687’s matcher does not include interface definition nodes. That can causedescribe()for top-level interfaces to returnNonebefore this new dependency logic is used.Suggested fix
fn is_item_node(kind: SyntaxKind) -> bool { matches!( kind, SyntaxKind::CLASS_DEF + | SyntaxKind::INTERFACE_DEF | SyntaxKind::FUNCTION_DEF | SyntaxKind::ENUM_DEF | SyntaxKind::CLIENT_DEF | SyntaxKind::GENERATOR_DEF | SyntaxKind::TEST_DEFAlso applies to: 937-948
🤖 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_lsp2_actions/src/describe.rs` around lines 687 - 701, The item-node filter in is_item_node currently omits interface definition nodes, causing find_item_range and docstring extraction used by describe() to skip top-level interfaces; update is_item_node to include the interface SyntaxKind variant (e.g., add SyntaxKind::INTERFACE_DEF) so interface definitions are treated as item nodes, and verify related logic in find_item_range and describe still applies (also mirror this addition where equivalent item-node checks exist around the dependency handling added at lines ~937-948).baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs (1)
65-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t treat
.as<T>target generics as call-site type arguments.
find_callee_generic_argsnow picksGENERIC_ARGSfromUPCAST_EXPR, which can leak upcast target types intoExpr::Call.type_argsfor calls on upcasted receivers.Suggested fix
fn find_callee_generic_args(callee_node: &SyntaxNode) -> Option<SyntaxNode> { + // `.as<T>` generic args belong to the upcast target, not to a call site. + if callee_node.kind() == SyntaxKind::UPCAST_EXPR { + let base = callee_node.children().next()?; + return find_callee_generic_args(&base); + } + if let Some(args) = callee_node .children() .find(|n| n.kind() == SyntaxKind::GENERIC_ARGS) { return Some(args); } match callee_node.kind() { SyntaxKind::FIELD_ACCESS_EXPR - | SyntaxKind::UPCAST_EXPR | SyntaxKind::OPTIONAL_FIELD_ACCESS_EXPR => { let base = callee_node.children().next()?; find_callee_generic_args(&base) } _ => None, } }🤖 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_expr_body.rs` around lines 65 - 75, find_callee_generic_args currently returns a child GENERIC_ARGS even when callee_node is an UPCAST_EXPR (and thus picks upcast target generics like `.as<T>` into Expr::Call.type_args); change the function so it does not treat GENERIC_ARGS from UPCAST_EXPR (and related upcast/optional-upcast kinds) as call-site type arguments — e.g., check callee_node.kind() first and skip returning a child GENERIC_ARGS when kind() == SyntaxKind::UPCAST_EXPR (and/or SyntaxKind::OPTIONAL_FIELD_ACCESS_EXPR if appropriate), leaving the existing behavior for FIELD_ACCESS_EXPR and other call-site forms; update the logic in find_callee_generic_args to only return GENERIC_ARGS for actual call-site nodes so Expr::Call.type_args no longer receives upcast target types.
🧹 Nitpick comments (7)
baml_language/_plan/BEP-044-interfaces/meta.json (1)
9-9: 💤 Low valueConsider updating the
updateddate to reflect recent changes.
IMPLEMENTATIONv2.mdwas written on 2026-05-20 (as noted in its header), butmeta.jsonshows"updated": "2026-05-05". If the BEP has evolved significantly since creation, the metadata should reflect the latest substantial revision.📅 Proposed update
- "updated": "2026-05-05", + "updated": "2026-05-20",🤖 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/_plan/BEP-044-interfaces/meta.json` at line 9, Update the "updated" field in meta.json to match the latest substantial revision date (e.g., change "updated": "2026-05-05" to "2026-05-20" to align with IMPLEMENTATIONv2.md header); locate the "updated" property in meta.json and set it to the most recent revision date that reflects changes described in IMPLEMENTATIONv2.md so the BEP metadata is accurate.baml_language/_plan/BEP-044-interfaces/README.md (1)
783-783: ⚡ Quick winConsider clarifying the historical reference to
extends.The phrase "earlier drafts of this BEP" might confuse readers scanning the document, since the presence of
IMPLEMENTATION_PLAN.md(which usesextends) in the same directory could make them thinkextendsis still a valid approach.✏️ Suggested clarification
-**Why `requires` and not `extends`.** `extends` (as in TS / Java / earlier drafts of this BEP) makes the *child interface* inherit the parent's signatures, so writing `implements Child` silently fulfilled the parent contract too. +**Why `requires` and not `extends`.** `extends` (as in TS / Java) makes the *child interface* inherit the parent's signatures, so writing `implements Child` silently fulfilled the parent contract too.This removes the potentially confusing self-reference to "earlier drafts" that might lead readers to hunt for those drafts.
🤖 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/_plan/BEP-044-interfaces/README.md` at line 783, Reword the sentence that reads "extends (as in TS / Java / earlier drafts of this BEP)" to remove the ambiguous "earlier drafts of this BEP" reference and explicitly clarify the history: either remove that clause or replace it with a short, clear note such as "extends (as in TS / Java — previously used in IMPLEMENTATION_PLAN.md)" so readers understand that IMPLEMENTATION_PLAN.md used extends and that the document now prefers requires; update the sentence referencing 'requires' and 'extends' accordingly to avoid confusion.baml_language/crates/baml_compiler2_hir/src/builder.rs (1)
1091-1121: 💤 Low valueInterface contribution ordering differs from class pattern.
In
lower_class, the type contribution is made before pushing the class scope (lines 1026-1032), whereas here the contribution is made inside the interface scope (afterpush_scopeat line 1098, beforepop_scopeat line 1148). While this doesn't affect correctness since contributions are just appended to a vector independent of scope state, the inconsistency could be confusing for future maintainers.Consider moving the contribution before
push_scopeto match the class pattern, or add a comment explaining why the ordering is intentional here.🤖 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/builder.rs` around lines 1091 - 1121, The interface contribution ordering is inconsistent with lower_class; update lower_interface to either move the type contribution before calling push_scope to match lower_class or (preferably) add a brief explanatory comment above the push_scope call explaining why the contribution must occur after lowering default methods (because default_method_ids are computed inside the Class-kind scope and the interface's local_id is allocated only after collecting them). Reference lower_interface, push_scope, default_method_ids, item_tree.alloc_interface, and type_contributions when making the change so future maintainers understand the intentional ordering.baml_language/crates/baml_compiler_lexer/src/tokens.rs (1)
48-57: ⚡ Quick winAdd a focused lexer regression test for the new interface keywords.
Please add a unit test that covers
interface,implements,implement,extends, andrequires, and explicitly verifiesimplementslexes as one token (notimplement+Word("s")).Proposed test addition
#[test] +fn test_interface_keywords() { + let tokens = lex_no_whitespace("interface implements implement extends requires"); + assert_eq!( + tokens, + vec![ + TokenKind::Interface, + TokenKind::Implements, + TokenKind::Implement, + TokenKind::Extends, + TokenKind::Requires, + ] + ); + + let tokens2 = lex_no_whitespace("implements"); + assert_eq!(tokens2, vec![TokenKind::Implements]); +} + +#[test] fn test_exception_keywords() {🤖 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_compiler_lexer/src/tokens.rs` around lines 48 - 57, Add a unit test that feeds the lexer the string "interface implements implement extends requires" and asserts the produced token sequence contains the single tokens Interface, Implements, Implement, Extends, and Requires (in that order); specifically assert that "implements" produces one Implements token rather than an Implement token plus a Word/Identifier "s". Locate the test near other lexer tests and call the public tokenize/lex function used in your crate (e.g., tokenize, lex, or Lexer::next_tokens) and compare the token enum values (Token::Interface, Token::Implements, Token::Implement, Token::Extends, Token::Requires); include any necessary whitespace handling so the lexer sees separate lexemes.baml_language/crates/baml_compiler2_ast/src/lower_cst.rs (1)
1444-1457: 💤 Low valueMisleading
class_namein diagnostic for interface field links.The
LoweringDiagnostic::MissingFieldNameusesclass_name: "interface field link"which is semantically incorrect and may produce confusing error messages since this isn't actually a class context. Consider using a more specific diagnostic variant or a clearer context string.🛠️ Suggested improvement
let Some(interface_field) = link.interface_field() else { - diags.push(LoweringDiagnostic::MissingFieldName { - class_name: "interface field link".to_string(), + diags.push(LoweringDiagnostic::MissingInterfaceFieldLinkName { + context: "interface field".to_string(), span: link.syntax().text_range(), }); return None; }; let Some(class_field) = link.class_field() else { - diags.push(LoweringDiagnostic::MissingFieldName { - class_name: "interface field link".to_string(), + diags.push(LoweringDiagnostic::MissingInterfaceFieldLinkName { + context: "class field".to_string(), span: link.syntax().text_range(), }); return None; };This would require adding a new
LoweringDiagnosticvariant, or alternatively just use a clearer string like"implements block field mapping".🤖 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 1444 - 1457, The diagnostic for missing field names in the interface->class mapping uses class_name: "interface field link", which is misleading; update the LoweringDiagnostic usage in the block handling link.interface_field() and link.class_field() to provide a clearer context string (e.g., "implements block field mapping") or add a new LoweringDiagnostic variant specifically for interface-to-class field links and use that variant instead; adjust the diags.push calls around link.interface_field() and link.class_field() and update any matching match arms or consumers of LoweringDiagnostic so the new string/variant is handled consistently.baml_language/crates/baml_compiler2_tir/src/interfaces.rs (1)
169-188: 💤 Low valueRemove unused
dbparameter or document future intent.The
dbparameter is unused in this function (line 183 just silences the warning). Consider removing it from the signature if it's not needed, or add a brief comment explaining why it's kept for future use.♻️ Suggested cleanup
pub fn resolve_path_to_interface<'db>( - db: &'db dyn crate::Db, + _db: &'db dyn crate::Db, // Reserved for future cross-package resolution target: &baml_compiler2_ast::TypeExpr, pkg_items: &baml_compiler2_hir::package::PackageItems<'db>, current_ns: &[Name], ) -> Option<baml_compiler2_hir::loc::InterfaceLoc<'db>> { use baml_compiler2_ast::TypeExpr; let TypeExpr::Path { segments, .. } = target else { return None; }; let (head, name) = segments .split_last() .map(|(last, head)| (head, last.clone()))?; let lookup_ns: &[Name] = if head.is_empty() { current_ns } else { head }; - let _ = db; let Definition::Interface(loc) = pkg_items.lookup_type(lookup_ns, &name)? else { return None; }; Some(loc) }🤖 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/interfaces.rs` around lines 169 - 188, The db parameter of resolve_path_to_interface is unused (silenced by let _ = db); either remove it from the function signature and update all callers to stop passing a Db, or keep it and add a short comment explaining it's intentionally reserved for future use (e.g., "kept for potential lookup needs") and remove the let _ = db line; locate the function by name resolve_path_to_interface and adjust callers/exports accordingly to ensure compilation after removing the parameter or add the explanatory comment if retaining it.baml_language/crates/bex_vm_types/src/types.rs (1)
95-103: ⚡ Quick winAdd
#[serde(default)]toProgram::interface_implementorsfor safer cross-version deserialization.
Programis deserialized bybaml-pack-hostfrom the embeddedPackEnvelopeviabitcode::deserialize, andbitcodeis configured with theserdefeature (so missing fields will error unless they have serde defaults). Packed binaries bundle the matching host, but ifProgram/PackEnvelopeever needs to be deserialized across versions (e.g., via a standalone host path or other cached payloads), this field should behave as “empty when absent” to avoid failures.💡 Proposed fix
/// Empty for programs without interfaces. + #[serde(default)] pub interface_implementors: IndexMap<baml_type::TypeName, Vec<baml_type::TypeName>>,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bex_vm_types/src/types.rs` around lines 95 - 103, Add serde's default attribute to the Program interface_implementors field so deserialization will treat a missing field as an empty map: annotate the field `interface_implementors: IndexMap<baml_type::TypeName, Vec<baml_type::TypeName>>` with `#[serde(default)]` (ensure serde feature is enabled where needed); this makes `Program::interface_implementors` default to empty on older/variant payloads and prevents deserialization errors across versions.
🤖 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_compiler_parser/src/parser.rs`:
- Around line 2511-2514: The parser currently treats leading @@ attributes as
standalone when encountering them before a function in parse_interface, so
parse_atat_attribute() is called and the attribute is not attached to the
subsequent interface method; modify parse_interface to collect/consume leading
@@ attributes and attach them to the method node instead of parsing them as
separate items—i.e., detect @@ before deciding between parse_interface_method(),
gather those attributes (the same way
item_keyword_after_leading_block_attributes preserves metadata for class
methods), and ensure they are attached to the METHOD_SIG/default method node
produced by parse_interface_method() so interface methods retain their metadata.
- Around line 2416-2423: The recovery condition is too permissive: don't exempt
interface/implements from ending a body unless it's the class-specific
implements case; update the guard around p.at_top_level_keyword() so only
TokenKind::Function is exempt in the three non-class loops, and in parse_class
allow TokenKind::Function plus only the class-local implements token
(TokenKind::Implements/Implement) if that was the intent—remove
TokenKind::Interface and the broad implements exemption from the other loops;
locate the checks using at_top_level_keyword(), TokenKind::Function,
TokenKind::Interface, TokenKind::Implements/Implement and adjust each loop
(parse_class and the three other recovery loops mentioned) accordingly so
invalid top-level items remain recovery boundaries.
- Around line 2648-2699: interface_method_has_body currently only skips "basic
trivia" which lets line/block comments between the signature and `{` confuse the
scanner; update the code in interface_method_has_body (the initial while that
advances i and the main loop that currently checks
self.is_basic_trivia(token.kind)) to also treat comment tokens as trivia (skip
TokenKind variants for comments, e.g., line/block comment kinds or via an
existing helper like is_comment_trivia or extend is_basic_trivia to include
comments) so braces inside comments are ignored and do not affect
paren/bracket/angle counting; add a regression test with a default interface
method containing intervening // and /* ... */ comments to validate the fix.
In `@baml_language/crates/baml_compiler_syntax/src/ast.rs`:
- Around line 163-171: The new AST node kinds (InterfaceDef / INTERFACE_DEF and
ImplementsFor / IMPLEMENTS_FOR) are registered but not exposed via Item::cast,
so SourceFile::items() silently drops them; update the Item type to include
variants for InterfaceDef and ImplementsFor (and any other newly added top-level
wrappers like ImplementsBlock/ImplementsTarget/InterfaceFieldLink if intended as
top-level) and extend Item::cast to map INTERFACE_DEF and IMPLEMENTS_FOR to
those Item variants so that SourceFile::items() iterates over and returns
interface declarations and out‑of‑body implements blocks correctly.
In `@baml_language/crates/baml_compiler2_emit/src/lib.rs`:
- Around line 198-222: collect_class_fields_with_implements currently only
iterates class_data.fields and thus omits fields contributed by implemented
interfaces; update this function to also traverse class_data.implements (or use
the TIR/HIR API to resolve each implemented interface/trait) and merge their
public field entries into out while still deduplicating via seen, preserving
field.type_expr, attributes, generic_params and pkg_ns; also apply the same fix
to the sibling helper that merges fields for implemented interfaces (the other
merged-field helper mentioned in the review) so both runtime class field/index
maps include interface-injected fields per BEP-044.
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 5475-5480: The emitted interface-dispatch call currently uses
self.builder.call(callee_op, all_args, dest.clone(), bb_join, None) which omits
the active catch/unwind target and the type-argument info that lower_call
threads through; update this dispatch to use the same ABI path as lower_call by
passing the current unwind target and the type-argument list (not None) into
builder.call (or invoke lower_call directly) so that callee_op, all_args (which
should still start with Operand::Copy(Place::Local(recv_local)) plus arg_ops),
dest, and bb_join get the same unwind and generic/type metadata as
non-dispatched calls.
- Around line 5366-5392: try_lower_interface_dispatch only handles when
self.expr_types[base] is Tir2Ty::Interface, so generic receivers (type variables
with an interface bound) fall through; update try_lower_interface_dispatch to
also detect when the metadata type for base is a type variable whose
trait/bounds include the target interface (qtn / iface_type_args) and treat that
case like Tir2Ty::Interface: lookup the type-var bounds via your type/bound
tables, match an Interface bound for the same qtn and type args, and then
proceed to lower the receiver (using lower_to_operand/expr_ty and
operand_to_local as before) and call emit_interface_dispatch_switch; keep the
same local/operand handling (receiver_op, receiver_ty, recv_local) but add the
type-var-with-interface-bound branch before returning false so generic-bound
receivers use the tag-switch path (refer to try_lower_interface_dispatch,
expr_types, expr_metadata_key, lower_to_operand, expr_ty, operand_to_local, and
emit_interface_dispatch_switch).
- Around line 3694-3718: The code currently builds ItemRef::Method using
package/namespace from self.file and only iface_path.last(), causing
default.<method> to resolve to the current file's interface; instead resolve
target_te.expr to the interface's defining file/definition and take its
package/namespace from that definition. Concretely: in the branch that matches
TypeExpr::Path (target_te.expr) and computes iface_name, call the resolver/DB to
find the interface's definition/file (the declaration that path points to)
rather than using file_package::file_package(self.db, self.file), then call
file_package::file_package(self.db, def_file_or_def) (or equivalent API that
returns package/namespace for the resolved interface) and use that package and
namespace when constructing ItemRef::Method (keep using iface_name and
method_name as before); update implements_block_iface_target()/resolution logic
to obtain the def file if necessary.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 6407-6439: The path-root materialization helpers currently only
create classes/enums/type aliases so qualified names like Iface.method(...)
never become Ty::Interface and thus never hit resolve_interface_member; update
the helper(s) that materialize path roots (the functions that currently produce
class/enum/alias Ty variants) to also detect interface declarations from
package_items.lookup_type and construct a Ty::Interface (including its type_args
and any bound info or invoking try_resolve_generic_bound_alias where applicable)
so that later code reaches the Ty::Interface arm and calls
resolve_interface_member; ensure any related diagnostic/related-note behavior is
preserved when constructing the interface Ty.
- Around line 7460-7466: The current bindings initialization only seeds
interface-level generics when iface_type_args is non-empty, so method-level
generic parameters (sig.user_generic_params) are missing and get reduced to
recovery types during lower_type_expr_with_generics; to fix, ensure bindings
includes entries for the method's own generic params as TypeVar placeholders
before any lowering: when constructing bindings (the block that refers to
iface_type_args, bindings, and iface_data.generic_params), after binding
interface generics also seed the method generics (sig.user_generic_params) into
bindings (mirroring how lookup_class_method seeds method generics) so
lower_type_expr_with_generics sees those names as Ty::TypeVar; update the
bindings creation logic to add method-generic entries when iface_type_args is
present.
- Around line 8016-8024: qualified_interface_field_for_construction currently
returns None so interface-backed extra fields never trigger
InterfaceFieldRequiresQualifiedConstruction; implement it to look up whether
field_name belongs to a qualified interface on the given class and, if so,
return the interface field's declared Name and its Ty instantiated with
class_type_args. Specifically, in qualified_interface_field_for_construction use
class_name to find the class's implemented interfaces (or its metadata/AST
entry), search those interface declarations for a field named field_name,
perform type parameter substitution using class_type_args to produce the
concrete Ty, and return Some((field_name.clone(), instantiated_ty)); otherwise
return None.
- Around line 7451-7611: The code returns a function type for interface methods
without recording a callee resolution, so explicit type-arg handling in
resolve_explicit_type_args never runs; before returning fn_ty in the
interface-default branch (the loop over iface_data.default_methods that
constructs fn_ty) — and likewise in the other interface-method branch around
7614–7774 — record the callee resolution using the same mechanism the rest of
the builder uses to enable resolve_explicit_type_args: construct the appropriate
resolution variant (UnboundMethod if not bound, BoundMethod if bound, including
iface_name, fn_id, member/method name, iface_type_args and any explicit type
args resolved) and store it via the existing callee-resolution API used
elsewhere in this module so the explicit type-arg arity/bound checks run before
returning fn_ty.
In `@baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs`:
- Around line 57-107: The current substitute_paths_in/walk implementation skips
TypeExpr::Function nodes so any Self/bound path inside function parameters,
return type or throws is not substituted; add a match arm for TypeExpr::Function
(match the exact field names used in your enum, e.g., params, return_type,
throws, attrs) and recursively call walk on each parameter type, the return_type
(if present), and the throws type (if present), returning a new
TypeExpr::Function with those transformed fields; make the same change in the
other walker mentioned (the second function with the same walk logic) so
function-typed nodes are consistently handled.
In `@baml_language/crates/baml_compiler2_tir/src/normalize.rs`:
- Around line 425-431: The normalization added handling for Ty::Interface
(producing StructuralTy::Interface) but the recursion/cycle walkers (substitute,
ty_has_cycle, extract_type_alias_deps) were not updated to descend into
interface type arguments; update each of those functions to treat interface
nodes like class nodes by iterating and recursing into the interface's type_args
(the same way you already recurse into class type args), so alias cycles like
type A = I<A> are detected and substitution/cycle checks traverse
interface-contained types as well.
In `@baml_language/crates/baml_lsp2_actions/src/check.rs`:
- Around line 1228-1240: type_exprs_compatible currently compares TypeExprs by
a.to_string() == b.to_string(), which is too strict; update it to perform a
structural/semantic compatibility check instead of string equality by resolving
both baml_compiler2_ast::TypeExpr into their canonical/normalized forms or into
the compiler's TIR/Ty representation and then using the existing
subtype/compatibility logic (e.g., convert TypeExpr -> Ty and call the subtyping
check) so that subtypes and equivalent-but-differently-rendered types are
accepted; modify the function type_exprs_compatible to call the
resolver/normalizer and then use the proper subtype/equals routine rather than
relying on to_string().
- Around line 693-797: The traversal currently truncates parent identities by
using segments.last() and probing with a single-segment TypeExpr, which loses
qualification and causes wrong matches and incorrect visited checks; fix
collect_interface_members_with_subst by preserving the full path: when iterating
parent_te, match TypeExpr::Path { segments, .. } and if segments is non-empty,
use the entire segments.clone() (not just last) to build the probe
TypeExpr::Path (including empty generic_args/attrs as before) and change the
visited set to track the full path identity (e.g., HashSet<Vec<Name>> or another
representation of the full segments) so you call
visited.insert(segments.clone()) and skip only exact full-path repeats; apply
the same pattern used in interface_has_cycle to ensure the graph walk keeps
fully-qualified parent identities and probes resolve_interface_path with the
full segments.
- Around line 898-911: The code records implemented interfaces into
seen_target_names and seen_targets from raw syntax, which allows unresolved or
differently-namespaced references to bypass duplicate and
MissingRequiredInterface checks; instead, after calling resolve_interface_path
for each implements entry, use the resolved identity (e.g., the resolved Name or
InterfaceId returned by resolve_interface_path) as the key when inserting into
seen_target_names and seen_targets, and attach the original span
(block.target.span) to that resolved-key entry; update logic that currently uses
block.interface_name(), block.target.expr, and format!("{}", block.target.expr)
to reference the resolved symbol so duplicates like Foo vs ns.Foo are detected
and missing-required-interface checks (e.g., MissingRequiredInterface) use
resolved identities.
In `@baml_language/crates/baml_lsp2_actions/src/usages_at_tests.rs`:
- Around line 139-143: The assertion is brittle because it pins the fixture
coordinates to "test.baml:8:12"; in the assert! that checks
formatted.iter().any(|usage| usage.ends_with("-> Animal") &&
usage.starts_with("test.baml:8:12")), relax the coordinate check to only verify
the filename (e.g., use starts_with("test.baml:") or contains("test.baml")) so
the test still ensures the "-> Animal" usage is reported from the test.baml
fixture without depending on exact line/column positions; update the predicate
in the assert to use the relaxed check (replace starts_with("test.baml:8:12")
with starts_with("test.baml:") or contains("test.baml")).
---
Outside diff comments:
In `@baml_language/_plan/BEP-044-interfaces/IMPLEMENTATION_PLAN.md`:
- Around line 1-690: The IMPLEMENTATION_PLAN.md is obsolete and conflicts with
IMPLEMENTATIONv2.md and README.md; remove ambiguity by either deleting BEP-044
Interfaces — Implementation Fix Plan (the file shown) or prepend a clear
deprecation banner referencing IMPLEMENTATIONv2.md and indicating it supersedes
this document (so readers and implementers only follow IMPLEMENTATIONv2.md);
ensure the banner mentions IMPLEMENTATIONv2.md and README.md to make the
canonical source explicit.
In `@baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs`:
- Around line 65-75: find_callee_generic_args currently returns a child
GENERIC_ARGS even when callee_node is an UPCAST_EXPR (and thus picks upcast
target generics like `.as<T>` into Expr::Call.type_args); change the function so
it does not treat GENERIC_ARGS from UPCAST_EXPR (and related
upcast/optional-upcast kinds) as call-site type arguments — e.g., check
callee_node.kind() first and skip returning a child GENERIC_ARGS when kind() ==
SyntaxKind::UPCAST_EXPR (and/or SyntaxKind::OPTIONAL_FIELD_ACCESS_EXPR if
appropriate), leaving the existing behavior for FIELD_ACCESS_EXPR and other
call-site forms; update the logic in find_callee_generic_args to only return
GENERIC_ARGS for actual call-site nodes so Expr::Call.type_args no longer
receives upcast target types.
In `@baml_language/crates/baml_lsp2_actions/src/describe.rs`:
- Around line 687-701: The item-node filter in is_item_node currently omits
interface definition nodes, causing find_item_range and docstring extraction
used by describe() to skip top-level interfaces; update is_item_node to include
the interface SyntaxKind variant (e.g., add SyntaxKind::INTERFACE_DEF) so
interface definitions are treated as item nodes, and verify related logic in
find_item_range and describe still applies (also mirror this addition where
equivalent item-node checks exist around the dependency handling added at lines
~937-948).
---
Nitpick comments:
In `@baml_language/_plan/BEP-044-interfaces/meta.json`:
- Line 9: Update the "updated" field in meta.json to match the latest
substantial revision date (e.g., change "updated": "2026-05-05" to "2026-05-20"
to align with IMPLEMENTATIONv2.md header); locate the "updated" property in
meta.json and set it to the most recent revision date that reflects changes
described in IMPLEMENTATIONv2.md so the BEP metadata is accurate.
In `@baml_language/_plan/BEP-044-interfaces/README.md`:
- Line 783: Reword the sentence that reads "extends (as in TS / Java / earlier
drafts of this BEP)" to remove the ambiguous "earlier drafts of this BEP"
reference and explicitly clarify the history: either remove that clause or
replace it with a short, clear note such as "extends (as in TS / Java —
previously used in IMPLEMENTATION_PLAN.md)" so readers understand that
IMPLEMENTATION_PLAN.md used extends and that the document now prefers requires;
update the sentence referencing 'requires' and 'extends' accordingly to avoid
confusion.
In `@baml_language/crates/baml_compiler_lexer/src/tokens.rs`:
- Around line 48-57: Add a unit test that feeds the lexer the string "interface
implements implement extends requires" and asserts the produced token sequence
contains the single tokens Interface, Implements, Implement, Extends, and
Requires (in that order); specifically assert that "implements" produces one
Implements token rather than an Implement token plus a Word/Identifier "s".
Locate the test near other lexer tests and call the public tokenize/lex function
used in your crate (e.g., tokenize, lex, or Lexer::next_tokens) and compare the
token enum values (Token::Interface, Token::Implements, Token::Implement,
Token::Extends, Token::Requires); include any necessary whitespace handling so
the lexer sees separate lexemes.
In `@baml_language/crates/baml_compiler2_ast/src/lower_cst.rs`:
- Around line 1444-1457: The diagnostic for missing field names in the
interface->class mapping uses class_name: "interface field link", which is
misleading; update the LoweringDiagnostic usage in the block handling
link.interface_field() and link.class_field() to provide a clearer context
string (e.g., "implements block field mapping") or add a new LoweringDiagnostic
variant specifically for interface-to-class field links and use that variant
instead; adjust the diags.push calls around link.interface_field() and
link.class_field() and update any matching match arms or consumers of
LoweringDiagnostic so the new string/variant is handled consistently.
In `@baml_language/crates/baml_compiler2_hir/src/builder.rs`:
- Around line 1091-1121: The interface contribution ordering is inconsistent
with lower_class; update lower_interface to either move the type contribution
before calling push_scope to match lower_class or (preferably) add a brief
explanatory comment above the push_scope call explaining why the contribution
must occur after lowering default methods (because default_method_ids are
computed inside the Class-kind scope and the interface's local_id is allocated
only after collecting them). Reference lower_interface, push_scope,
default_method_ids, item_tree.alloc_interface, and type_contributions when
making the change so future maintainers understand the intentional ordering.
In `@baml_language/crates/baml_compiler2_tir/src/interfaces.rs`:
- Around line 169-188: The db parameter of resolve_path_to_interface is unused
(silenced by let _ = db); either remove it from the function signature and
update all callers to stop passing a Db, or keep it and add a short comment
explaining it's intentionally reserved for future use (e.g., "kept for potential
lookup needs") and remove the let _ = db line; locate the function by name
resolve_path_to_interface and adjust callers/exports accordingly to ensure
compilation after removing the parameter or add the explanatory comment if
retaining it.
In `@baml_language/crates/bex_vm_types/src/types.rs`:
- Around line 95-103: Add serde's default attribute to the Program
interface_implementors field so deserialization will treat a missing field as an
empty map: annotate the field `interface_implementors:
IndexMap<baml_type::TypeName, Vec<baml_type::TypeName>>` with
`#[serde(default)]` (ensure serde feature is enabled where needed); this makes
`Program::interface_implementors` default to empty on older/variant payloads and
prevents deserialization errors across versions.
🪄 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: 2c5f69ca-66a5-448f-bd3b-6221d96034d8
⛔ Files ignored due to path filters (5)
baml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/attribute_validation/baml_tests__diagnostic_errors__attribute_validation__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snap
📒 Files selected for processing (117)
baml_language/_plan/BEP-044-interfaces/IMPLEMENTATION_PLAN.mdbaml_language/_plan/BEP-044-interfaces/IMPLEMENTATIONv2.mdbaml_language/_plan/BEP-044-interfaces/README.mdbaml_language/_plan/BEP-044-interfaces/meta.jsonbaml_language/crates/baml_builtins2/baml_std/baml/type_class.bamlbaml_language/crates/baml_builtins2_codegen/src/extract.rsbaml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/auto_derive_json.rsbaml_language/crates/baml_compiler2_ast/src/companions.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_ast/src/lowering_diagnostic.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/contributions.rsbaml_language/crates/baml_compiler2_hir/src/diagnostic.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_ppir/src/lib.rsbaml_language/crates/baml_compiler2_tir/architecture-slides.htmlbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/exhaustiveness.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/normalize.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler2_tir/src/throws_analysis.rsbaml_language/crates/baml_compiler2_tir/src/ty.rsbaml_language/crates/baml_compiler_diagnostics/src/diagnostic.rsbaml_language/crates/baml_compiler_lexer/src/tokens.rsbaml_language/crates/baml_compiler_parser/src/parser.rsbaml_language/crates/baml_compiler_syntax/src/ast.rsbaml_language/crates/baml_compiler_syntax/src/syntax_kind.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/completions.rsbaml_language/crates/baml_lsp2_actions/src/describe.rsbaml_language/crates/baml_lsp2_actions/src/listing_tests.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_lsp2_actions/src/type_info_tests.rsbaml_language/crates/baml_lsp2_actions/src/usages_at_tests.rsbaml_language/crates/baml_lsp2_actions/src/utils.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/completion/top_level_keywords.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/dependency_cycle.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/incomplete_class.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/spelling_error.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/unknown_type.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/valid_classes.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/client/remap_role_invalid_type.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/expr/constructors_invalid.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/hover/out_of_body_implements_class.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/ambiguous_method.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/conflicting_field_types.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/duplicate_implements.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_ambiguous_interface_field_access.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_ambiguous_method_call.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_as_downcast_interface_to_child.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_concrete_field_through_interface_type.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_default_on_required_method.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_default_outside_implements.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_duplicate_interface_field_link.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_field_declared_in_implements_block.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_generic_bound_violation.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_interface_extends_field_conflict.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_invariant_generic_assignment.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_match_without_wildcard.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_missing_field_in_implements_block.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_missing_required_interface.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_multi_self_call_on_interface_receiver.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_old_qualified_projection.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_out_of_body_field_interface.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_qualified_call_non_implementor.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_qualified_call_unknown_interface.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_self_outside_type.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_unknown_class_field_link.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_unknown_interface_field_link.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/extends_cycle.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/field_type_mismatch.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/method_signature_mismatch.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/missing_required_method.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/unknown_interface.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/unknown_interface_member.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_as_upcast_child_interface_to_parent.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_basic.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_class_methods_outside_interface.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_default_call_from_override.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_default_methods.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_diamond_independent_resolution.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_extends_chain_three_levels.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_field_rule_3_merged_same_type.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_field_rule_5_subtype_field.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_generic_bound.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_generic_concrete_type_param.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_generic_different_type_params_on_one_class.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_generic_interface_bound_instantiation.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_implement_singular.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_implements_for_class.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_inheritance.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_interface_field_alias.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_interface_field_via_extends.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_interface_typed_var_method_call.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_match_narrows_to_concrete.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_match_with_wildcard.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_qualified_method_call.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_self_qualified_call_in_implements.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_self_return_interface_receiver.bamlbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_tests/tests/interfaces.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/request.rsbaml_language/crates/bex_vm/src/package_baml/type_class.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm_types/src/types.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
baml_language/crates/baml_compiler2_tir/src/builder.rs (1)
7370-7410:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftInherited generic interfaces are lowered without their instantiated type arguments.
Only
root_iface_locgetsiface_type_args; every parent reached throughinterface_closure_locsis lowered with empty bindings. That meansChild<int> requires Parent<int>will still resolveParent<T>members asThere, so inherited generic fields/methods keep type vars instead of the concrete instantiation. The same raw-closure traversal also backs qualified construction, so the fix should be shared.baml_language/crates/baml_compiler2_tir/src/interfaces.rs (1)
366-369:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve each transitive parent in its own namespace.
interface_closure_locskeeps passing the originalcurrent_nsthrough the whole BFS. Once traversal reaches an interface declared in another namespace, itsextendsclauses are resolved relative to the wrong scope, so ancestors can be missed or mis-resolved.interface_closurealready does the right thing by recomputing the namespace fromloc.file(db).🤖 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/interfaces.rs` around lines 366 - 369, interface_closure_locs incorrectly reuses the original current_ns for all BFS steps causing extends clauses to be resolved in the wrong namespace; update the BFS so that when you discover a parent interface via resolve_path_to_interface(db, &parent.expr, pkg_items, current_ns) you recompute the namespace from the parent's location (use parent_loc.file(db) or the same approach used in interface_closure to derive the new current_ns) and pass that recomputed namespace into subsequent resolve_path_to_interface calls and when pushing into interface_closure_locs, ensuring each transitive parent is resolved relative to its own file/namespace.baml_language/crates/baml_compiler2_tir/src/inference.rs (2)
611-637:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude enclosing interface generics in method inference.
This merge only pulls generic params from a parent
ScopeKind::Class. Forinterface BoxLike<T> { func unwrap(self) -> T },Tnever entersgeneric_params, so interface method signatures lower toUnknownand report bogus unresolved-type diagnostics.🤖 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 611 - 637, The code only merges parent generics when parent.kind is ScopeKind::Class, so interface-level generics are never added; update the parent-kind check to handle ScopeKind::Interface as well and look up the corresponding interface entry in item_tree (e.g. item_tree.interfaces) just like you do for classes: check for name match, report method-level type param shadowing against interface.generic_params using builder.report_at_span and TirTypeError::TypeParamShadowed, then merge interface.generic_params into the existing generic_params (same pattern as class_data.generic_params) before continuing.
696-788:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive
Selfand implicitselffrom interface parents too.
enclosing_class_nameis only populated for class parents, but the same lowering path is used for interface default methods. In an interface method likefunc clone(self) -> Self, bothSelfsubstitution and the unannotatedselffallback currently collapse toUnknown, so the method loses the interface receiver contract during inference.🤖 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 696 - 788, The code only sets enclosing_class_name when the parent scope is ScopeKind::Class, which prevents Self substitution and implicit unannotated `self` handling for interface default methods; update the logic that computes enclosing_class_name (the closure using scope.parent and reading index.scopes[..].kind) to treat ScopeKind::Interface the same as ScopeKind::Class so self_replacement, lower_with_self, and the implicit `self` branch (the param handling that calls pkg_items.lookup_type and matches Definition::Interface) can find and use the enclosing interface name.baml_language/crates/baml_compiler_parser/src/parser.rs (1)
2804-2819:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThese recovery guards make keyword-named members unreachable.
Both loops break on any top-level keyword before
looks_like_interface_field_link()/at_member_name()run. That means entries likeinterface as real_fieldorimplements stringcan never parse insideimplementsbodies, even though the later helpers explicitly allow those tokens there.Also applies to: 2893-2908
🤖 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_compiler_parser/src/parser.rs` around lines 2804 - 2819, The recovery guard that breaks on p.at_top_level_keyword() runs too early and prevents parsing keyword-named members; move or narrow that break so the parser first checks for function, block-attribute function (item_keyword_after_leading_block_attributes()), looks_like_interface_field_link()/parse_interface_field_link() and at_member_name()/parse_field() before deciding to break. Concretely, reorder the checks in the loop so the branches that call parse_function(), parse_interface_field_link(), and parse_field() run before invoking p.at_top_level_keyword() (or restrict the break condition to only apply when not inside a implements/interface body), keeping the existing helpers item_keyword_after_leading_block_attributes(), looks_like_interface_field_link(), at_member_name(), parse_function(), parse_interface_field_link(), and parse_field() intact.baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
3962-3976:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate type arguments through
default.<method>(...).This early-return path still emits a plain
builder.call(...)with onlyselfplus value args, sodefault.foo<T>()drops explicit type arguments and diverges from the normallower_callABI. Please route this through the samecall_with_type_argsflow before returning.🤖 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 3962 - 3976, The early-return path that handles `default.<method>(...)` currently constructs a callee and args then calls builder.call(...) which drops explicit type arguments; instead, route this path through the existing call_with_type_args flow. Replace the builder.call(...) invocation in this branch with a call to the same helper used for normal lowers (call_with_type_args) passing the constructed callee_op (ItemRef::Method wrapped in Constant::Function), the assembled all_args (including Operand::Copy(Place::Local(self_local)) and lower_call_arg_operands(...)), the dest, the created target, the unwind from catch_context, and the type-argument list used by normal lower_call so the explicit type parameters are preserved. Ensure you reuse the same target/unwind/dest semantics as the original builder.call invocation.
♻️ Duplicate comments (1)
baml_language/crates/baml_compiler_parser/src/parser.rs (1)
2464-2473:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't exempt top-level
implements ... for ...from class-body recovery.This still treats every
implementstoken as class-local. If a class is missing its closing}, the next top-levelimplements Named for int { ... }will enterparse_implements_block(), fail onfor, and corrupt recovery for the rest of the file. Only the in-class form should bypass the recovery boundary here. A small unit test next tointerface_keyword_recovers_as_top_level_after_missing_class_bracewould pin this down.🤖 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_compiler_parser/src/parser.rs` around lines 2464 - 2473, The current recovery condition incorrectly exempts TokenKind::Implements/Implement as if they were class-local, allowing a top-level "implements ... for ..." to be parsed inside a missing-class body and break recovery; update the condition around recover_top_level_item so only the in-class form bypasses the recovery boundary — remove the special-case exemptions for p.at(TokenKind::Implements) and p.at(TokenKind::Implement) (i.e., do not treat those tokens as class-local here) and rely on the existing looks_like_interface_declaration_start()/p.at(TokenKind::Interface) check to permit in-class behavior; add a unit test (next to interface_keyword_recovers_as_top_level_after_missing_class_brace) that ensures a missing class '}' followed by a top-level "implements Named for int { ... }" is recovered as a top-level item rather than entering parse_implements_block().
🧹 Nitpick comments (3)
baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs (1)
73-79: ⚡ Quick winAdd a unit test for upcasted receiver calls.
The
UPCAST_EXPRbailout here is subtle enough that a small lowering unit test coveringType<T>.method()vsexpr.as<I>.method()would make regressions in call-site generic extraction much harder to miss.As per coding guidelines
**/*.rs: Prefer writing Rust unit tests over integration tests where possible.🤖 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_expr_body.rs` around lines 73 - 79, Add a Rust unit test in the same module to cover upcasted receiver calls so the UPCAST_EXPR bailout isn't regressed: write a #[cfg(test)] mod tests with cases that parse & lower a field call where the receiver is a generic type (e.g. "Type<T>.method()" or "value.method()" that produces a callee_node whose first child is not UPCAST_EXPR) and a case where the receiver is an explicit upcast (e.g. "expr.as<I>.method()") and assert the lowering behavior differs (the former extracts GENERIC_ARGS from the base while the latter follows the current code path that returns None for UPCAST_EXPR); reference SyntaxKind::FIELD_ACCESS_EXPR, SyntaxKind::OPTIONAL_FIELD_ACCESS_EXPR, callee_node and SyntaxKind::UPCAST_EXPR to locate the matching code and invoke the lowering function exercised by lower_expr_body.rs.baml_language/crates/baml_compiler2_emit/src/lib.rs (1)
340-342: ⚡ Quick winAdd an emit unit test for the new runtime-field contract.
These lines change the intended semantics from “merge interface fields” to “emit only concrete class storage”. A focused unit test here would lock that behavior down and prevent future regressions back to interface-backed runtime slots.
As per coding guidelines
**/*.rs: Prefer writing Rust unit tests over integration tests where possible.🤖 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_emit/src/lib.rs` around lines 340 - 342, Add a Rust unit test in this crate's lib.rs (under #[cfg(test)]) that verifies the BEP-044 runtime-field contract: construct a small IR where a class implements an interface that declares fields, call the emitter path that collects/returns runtime fields (the code guarded by the "collect only the class's actual runtime fields" comment), and assert that only the concrete class storage fields are emitted and interface-declared fields are not present; name it something like test_emit_only_concrete_class_storage_runtime_fields and use existing IR/test helpers in this module to build the class/interface and invoke the emitter function responsible for collecting/emitting runtime fields.baml_language/crates/baml_compiler2_hir/src/builder.rs (1)
1036-1076: 💤 Low valueDuplicate detection may miss methods from
implementsblocks.The duplicate detection at lines 1036-1053 iterates over
c.fieldsandc.methods, but lines 1065-1076 lower additional methods fromc.implements[].methodsthat are added tomethod_idswithout being checked for name collisions.If a class declares
fn foo()at the class level and also providesfn foo()inside animplements I { }block, no duplicate diagnostic would be emitted here.♻️ Suggested addition to include implements-block methods in duplicate detection
for method in &c.methods { seen.entry(method.name.clone()) .or_default() .push(MemberSite { range: method.name_span, kind: DefinitionKind::Method, }); } + for impl_block in &c.implements { + for method in &impl_block.methods { + seen.entry(method.name.clone()) + .or_default() + .push(MemberSite { + range: method.name_span, + kind: DefinitionKind::Method, + }); + } + } self.emit_duplicate_diagnostics(seen);🤖 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/builder.rs` around lines 1036 - 1076, The duplicate-detection map `seen` is only populated from `c.fields` and `c.methods`, so methods coming from `c.implements[].methods` are never checked; before calling self.emit_duplicate_diagnostics(seen) add entries for each method in each impl block (using the same MemberSite with range = method.name_span and kind = DefinitionKind::Method) so impl-block methods are recorded and will trigger duplicates; keep the existing lowering (lower_function -> fid, item_tree.method_to_iface_target insert, and pushing to method_ids) but ensure the impl methods are also inserted into `seen` first.
🤖 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_compiler_parser/src/parser.rs`:
- Around line 912-956: The helper looks_like_interface_declaration_start
incorrectly stops on TokenKind::Newline; update it to keep scanning across
newlines the same way parse_interface allows trivia so multiline declarations
recover properly—locate looks_like_interface_declaration_start and remove the
early-return branch that maps TokenKind::Newline to false, instead treat Newline
like Whitespace (i.e., advance i and continue) and keep the existing handling
for LBrace, RBrace, Semicolon and other tokens; ensure the
skip_trivia_and_comments_from / skip_comment_at / skip_header_comment_at logic
remains used as before.
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 5371-5381: The call to try_lower_interface_field_access is
currently ignoring its bool result, which causes the branch to short-circuit
when interface dispatch fails; change the code to check the returned bool from
try_lower_interface_field_access (using its result) and only return/skip further
handling if it returns true, otherwise allow execution to fall through to the
existing union/map/error-recovery code paths so dest can still be assigned;
locate the branch that uses interface_receiver_for_field_access and use the bool
from try_lower_interface_field_access (passing expr_id, base_local, &iface_tn,
&iface_type_args, field, &dest) to decide whether to stop or continue.
- Around line 5733-5741: The current loop skips implementors when
type_tag_for_ty(impl_ty) returns None, so non-class concrete types (uint8array,
enums, lists, maps) get omitted from dispatch; instead, keep those implementors
by recording the None tag and emitting alternative dispatch arms later: change
the push to include the tag option (or otherwise record impl_ty.to_string()) for
every implementor found by resolve_type_implementor_method, and update the
switch-emission logic that consumes interface_type_implementors/resolved to
generate runtime type-name or structural checks for entries with a None tag
(i.e., emit branches that match by impl_ty string or appropriate is-X runtime
test when tag is None) so non-class implementors are covered.
- Around line 1018-1022: The code currently drops instantiated type arguments
for non-root interfaces by setting iface_args = Vec::new() whenever iface_loc !=
root_iface_loc; instead ensure you preserve the actual instantiated type
arguments for that interface (e.g. look them up from whatever instantiation
mapping or the interface's metadata) rather than using an empty Vec: replace the
Vec::new() branch with a call that retrieves the instantiated args for iface_loc
(or clone the existing instantiation entry) so iface_args reflects the concrete
type parameters; apply the same fix to the other occurrences that use Vec::new()
in this walk (the similar blocks around the other noted spots).
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 467-470: interface_method_generic_params is an arena-local side
table keyed by ExprId but is not being saved/restored when swapping inference
state in check_function_parameter_defaults and infer_lambda_body, which lets
stale ExprId entries leak across nested/default bodies; update the same
save/restore logic that handles expressions and resolutions to include
interface_method_generic_params so it is pushed before swapping and restored
after (i.e., treat interface_method_generic_params as part of the arena-local
inference state used by resolve_explicit_type_args).
- Around line 1395-1448: Unbound interface calls ignore the interface's generic
args; when you use self.interface_method_generic_params to build (callee_name,
declared_params) also fetch any stored interface-level generic params (e.g. from
a map like self.interface_instantiation_generic_params or similar) and prepend
them to declared_params just like class_params is prepended for static methods,
keeping func_loc_for_bound_checks as None; update the branch that returns
(callee_name, declared_params, None) so declared_params =
[interface_generic_params..., method_user_generic_params...] using the same Name
type.
In `@baml_language/crates/baml_compiler2_tir/src/interfaces.rs`:
- Around line 75-105: implementation_key_for_ty currently preserves union member
order so Ty::Union keys are order-sensitive; canonicalize union members by
mapping each member with implementation_key_for_ty, filtering out None results,
deduping and sorting the resulting Vec (stable ordering) before building
Ty::Union so that unions like int|string and string|int produce the same
implementation key; update the Ty::Union arm to collect Option<Vec<_>> into a
Vec, sort and dedupe it (using a deterministic comparison of the element Ty
values or their serialized/key form) and then wrap into
Some(Ty::Union(sorted_deduped_vec, TyAttr::default())).
In `@baml_language/crates/baml_lsp2_actions/src/check.rs`:
- Around line 941-1079: validate_implements_for currently only checks
imp.methods and flags any other required interface methods as missing; instead
resolve the target type from imp.for_target (using the same resolution used
elsewhere), locate the target class/impl block and include its declared methods,
inherited/sibling implementations and class-wide `requires` logic when building
the set of provided methods, and then run the same satisfaction checks used by
validate_class_implements (or extract its core check into a shared helper)
against the members returned by collect_interface_members_with_subst; update
validate_implements_for to consult the target's existing method set (not only
imp.methods) before emitting MissingInterfaceMethod or signature-mismatch
diagnostics.
In `@baml_language/crates/baml_lsp2_actions/src/definition.rs`:
- Around line 410-412: The go-to-definition path adds InterfaceDefaultMethod to
the MemberResolution match but definition_at still returns early for any token
that is not a WORD, so keyword-named members (lexed as KW_*) never reach this
resolution. Update definition_at to treat keyword tokens used as identifiers the
same as WORD tokens (e.g., accept KW_* or normalize KW_* to WORD before lookup)
so reflection-style members like "implements" are allowed through; change the
early bail-out that checks token kind to include KW_* (or add a helper like
token_is_identifier_or_keyword) and keep the MemberResolution match
(MemberResolution::BoundMethod, ::UnboundMethod, ::InterfaceDefaultMethod) as-is
to resolve keyword-named members.
---
Outside diff comments:
In `@baml_language/crates/baml_compiler_parser/src/parser.rs`:
- Around line 2804-2819: The recovery guard that breaks on
p.at_top_level_keyword() runs too early and prevents parsing keyword-named
members; move or narrow that break so the parser first checks for function,
block-attribute function (item_keyword_after_leading_block_attributes()),
looks_like_interface_field_link()/parse_interface_field_link() and
at_member_name()/parse_field() before deciding to break. Concretely, reorder the
checks in the loop so the branches that call parse_function(),
parse_interface_field_link(), and parse_field() run before invoking
p.at_top_level_keyword() (or restrict the break condition to only apply when not
inside a implements/interface body), keeping the existing helpers
item_keyword_after_leading_block_attributes(),
looks_like_interface_field_link(), at_member_name(), parse_function(),
parse_interface_field_link(), and parse_field() intact.
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 3962-3976: The early-return path that handles
`default.<method>(...)` currently constructs a callee and args then calls
builder.call(...) which drops explicit type arguments; instead, route this path
through the existing call_with_type_args flow. Replace the builder.call(...)
invocation in this branch with a call to the same helper used for normal lowers
(call_with_type_args) passing the constructed callee_op (ItemRef::Method wrapped
in Constant::Function), the assembled all_args (including
Operand::Copy(Place::Local(self_local)) and lower_call_arg_operands(...)), the
dest, the created target, the unwind from catch_context, and the type-argument
list used by normal lower_call so the explicit type parameters are preserved.
Ensure you reuse the same target/unwind/dest semantics as the original
builder.call invocation.
In `@baml_language/crates/baml_compiler2_tir/src/inference.rs`:
- Around line 611-637: The code only merges parent generics when parent.kind is
ScopeKind::Class, so interface-level generics are never added; update the
parent-kind check to handle ScopeKind::Interface as well and look up the
corresponding interface entry in item_tree (e.g. item_tree.interfaces) just like
you do for classes: check for name match, report method-level type param
shadowing against interface.generic_params using builder.report_at_span and
TirTypeError::TypeParamShadowed, then merge interface.generic_params into the
existing generic_params (same pattern as class_data.generic_params) before
continuing.
- Around line 696-788: The code only sets enclosing_class_name when the parent
scope is ScopeKind::Class, which prevents Self substitution and implicit
unannotated `self` handling for interface default methods; update the logic that
computes enclosing_class_name (the closure using scope.parent and reading
index.scopes[..].kind) to treat ScopeKind::Interface the same as
ScopeKind::Class so self_replacement, lower_with_self, and the implicit `self`
branch (the param handling that calls pkg_items.lookup_type and matches
Definition::Interface) can find and use the enclosing interface name.
In `@baml_language/crates/baml_compiler2_tir/src/interfaces.rs`:
- Around line 366-369: interface_closure_locs incorrectly reuses the original
current_ns for all BFS steps causing extends clauses to be resolved in the wrong
namespace; update the BFS so that when you discover a parent interface via
resolve_path_to_interface(db, &parent.expr, pkg_items, current_ns) you recompute
the namespace from the parent's location (use parent_loc.file(db) or the same
approach used in interface_closure to derive the new current_ns) and pass that
recomputed namespace into subsequent resolve_path_to_interface calls and when
pushing into interface_closure_locs, ensuring each transitive parent is resolved
relative to its own file/namespace.
---
Duplicate comments:
In `@baml_language/crates/baml_compiler_parser/src/parser.rs`:
- Around line 2464-2473: The current recovery condition incorrectly exempts
TokenKind::Implements/Implement as if they were class-local, allowing a
top-level "implements ... for ..." to be parsed inside a missing-class body and
break recovery; update the condition around recover_top_level_item so only the
in-class form bypasses the recovery boundary — remove the special-case
exemptions for p.at(TokenKind::Implements) and p.at(TokenKind::Implement) (i.e.,
do not treat those tokens as class-local here) and rely on the existing
looks_like_interface_declaration_start()/p.at(TokenKind::Interface) check to
permit in-class behavior; add a unit test (next to
interface_keyword_recovers_as_top_level_after_missing_class_brace) that ensures
a missing class '}' followed by a top-level "implements Named for int { ... }"
is recovered as a top-level item rather than entering parse_implements_block().
---
Nitpick comments:
In `@baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs`:
- Around line 73-79: Add a Rust unit test in the same module to cover upcasted
receiver calls so the UPCAST_EXPR bailout isn't regressed: write a #[cfg(test)]
mod tests with cases that parse & lower a field call where the receiver is a
generic type (e.g. "Type<T>.method()" or "value.method()" that produces a
callee_node whose first child is not UPCAST_EXPR) and a case where the receiver
is an explicit upcast (e.g. "expr.as<I>.method()") and assert the lowering
behavior differs (the former extracts GENERIC_ARGS from the base while the
latter follows the current code path that returns None for UPCAST_EXPR);
reference SyntaxKind::FIELD_ACCESS_EXPR, SyntaxKind::OPTIONAL_FIELD_ACCESS_EXPR,
callee_node and SyntaxKind::UPCAST_EXPR to locate the matching code and invoke
the lowering function exercised by lower_expr_body.rs.
In `@baml_language/crates/baml_compiler2_emit/src/lib.rs`:
- Around line 340-342: Add a Rust unit test in this crate's lib.rs (under
#[cfg(test)]) that verifies the BEP-044 runtime-field contract: construct a
small IR where a class implements an interface that declares fields, call the
emitter path that collects/returns runtime fields (the code guarded by the
"collect only the class's actual runtime fields" comment), and assert that only
the concrete class storage fields are emitted and interface-declared fields are
not present; name it something like
test_emit_only_concrete_class_storage_runtime_fields and use existing IR/test
helpers in this module to build the class/interface and invoke the emitter
function responsible for collecting/emitting runtime fields.
In `@baml_language/crates/baml_compiler2_hir/src/builder.rs`:
- Around line 1036-1076: The duplicate-detection map `seen` is only populated
from `c.fields` and `c.methods`, so methods coming from `c.implements[].methods`
are never checked; before calling self.emit_duplicate_diagnostics(seen) add
entries for each method in each impl block (using the same MemberSite with range
= method.name_span and kind = DefinitionKind::Method) so impl-block methods are
recorded and will trigger duplicates; keep the existing lowering (lower_function
-> fid, item_tree.method_to_iface_target insert, and pushing to method_ids) but
ensure the impl methods are also inserted into `seen` first.
🪄 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: 6bec940c-22f7-4d20-a14b-6865182a44e7
⛔ Files ignored due to path filters (16)
baml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__testing_std__/baml_tests__compiles____testing_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/comment_after_string_in_config/baml_tests__compiles__comment_after_string_in_config__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/comment_in_type/baml_tests__compiles__comment_in_type__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/config_dictionary/baml_tests__compiles__config_dictionary__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/config_model_string/baml_tests__compiles__config_model_string__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/json_llm_return_type/baml_tests__compiles__json_llm_return_type__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/llm_image_outputs/baml_tests__compiles__llm_image_outputs__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/o1_allowed_roles/baml_tests__compiles__o1_allowed_roles__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/retry_policy/baml_tests__compiles__retry_policy__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/stream_llm_inferred_typeargs/baml_tests__compiles__stream_llm_inferred_typeargs__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/generics/baml_tests__diagnostic_errors__generics__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/generics/baml_tests__diagnostic_errors__generics__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/type_aliases/baml_tests__diagnostic_errors__type_aliases__04_tir.snapis excluded by!**/*.snap
📒 Files selected for processing (29)
baml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/auto_derive_json.rsbaml_language/crates/baml_compiler2_ast/src/companions.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/item_tree.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/interfaces.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/normalize.rsbaml_language/crates/baml_compiler_parser/src/parser.rsbaml_language/crates/baml_compiler_syntax/src/ast.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/definition.rsbaml_language/crates/baml_lsp2_actions/src/usages_at_tests.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_default_method_non_exhaustive.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_field_rule_invariant_union.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_generic_bound_alias_not_supported.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_method_throws_mismatch.bamlbaml_language/crates/baml_tests/src/compiler2_tir/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/baml_tests/tests/interfaces.rsbaml_language/crates/tools_onionskin/src/compiler.rs
💤 Files with no reviewable changes (4)
- baml_language/crates/baml_compiler2_ppir/src/lib.rs
- baml_language/crates/baml_compiler2_ast/src/ast.rs
- baml_language/crates/baml_compiler2_ast/src/auto_derive_json.rs
- baml_language/crates/baml_compiler2_ast/src/companions.rs
✅ Files skipped from review due to trivial changes (2)
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_field_rule_invariant_union.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/error_generic_bound_alias_not_supported.baml
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | e2e_arithmetic |
96.3 ms | 113.8 ms | -15.37% |
| ⚡ | WallTime | vm_mixed_ops |
10.6 ms | 7 ms | +50.25% |
| ❌ | WallTime | compile_to_engine |
97.9 ms | 114.5 ms | -14.47% |
| ❌ | WallTime | e2e_100_functions |
157.5 ms | 179.7 ms | -12.32% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing aaron/interfacs (5de26df) with canary (1c9c296)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
baml_language/crates/baml_lsp2_actions/src/check.rs (2)
1043-1050:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve inherited interface members from the interface's namespace.
Both call sites still pass the caller's
namespace_pathintocollect_interface_members_with_subst. That walker uses the supplied namespace to resolve relativeextends, so implementing an interface from another namespace can traverse the wrong parent chain and report bogus missing members. The walk needs to use the namespace of each interface being visited, not the namespace of the class or file doing the validation.Also applies to: 1240-1247
🤖 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_lsp2_actions/src/check.rs` around lines 1043 - 1050, The call to collect_interface_members_with_subst is using the caller's namespace_path which makes the walker resolve relative extends incorrectly; change the call sites that build members (where iface, iface_file, pkg_items, namespace_path, &subst are passed) to pass the namespace belonging to the interface being visited (e.g., iface.namespace or resolve namespace from iface_file/iface) instead of the caller's namespace_path so the walker uses the interface's own namespace when resolving extends; make the same change at the other similar call site later in the file (the second collect_interface_members_with_subst invocation).
894-935:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep inherited field types in their original namespace.
seenonly retains the rawTypeExpr, thentype_exprs_compatiblere-lowers both sides as if they were declared in the currentnamespace_path. If a parent interface comes from another namespace and uses relative type names, this will compare the wrong types and can invent or missInterfaceExtendsFieldConflicts. Store each inherited field's originating namespace (or its already-loweredTy) alongside the entry before comparing.🤖 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_lsp2_actions/src/check.rs` around lines 894 - 935, The seen map currently stores raw TypeExpr strings which get re-lowered against the current namespace when calling type_exprs_compatible, causing incorrect comparisons for inherited fields from other namespaces; change the seen entries (where you insert in the iface.fields seed and in the parent-members loop) to also include the originating namespace/file or—better—store the already-lowered type representation (the Ty or lowered form) along with the origin (use parent_loc.file(db) or equivalent) so comparisons use the original namespace context; update the places that call seen.get(field_name) and the type_exprs_compatible invocation to pass the stored origin/lowered Ty instead of re-lowering the raw TypeExpr to avoid spurious InterfaceExtendsFieldConflict results.baml_language/crates/baml_compiler2_ast/src/lower_cst.rs (1)
160-198:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly merge bare local class targets here.
This matches
for_targetbysegments.last(), so a qualified or specialized target likedep.DogorDog<int>will be attached to the localDogclass if one exists. That changes the target before package-level resolution and loses the originalImplementsForidentity. Restrict this eager merge to an unqualified, non-generic local class name; leave everything else asItem::ImplementsFor.Suggested narrowing
- let target_name = match &imp.for_target.expr { - crate::ast::TypeExpr::Path { segments, .. } => segments.last().cloned(), + let target_name = match &imp.for_target.expr { + crate::ast::TypeExpr::Path { + segments, + generic_args, + .. + } if segments.len() == 1 && generic_args.is_empty() => segments.first().cloned(), _ => None, };🤖 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 160 - 198, The current merge finds a local class by using segments.last() on impl.for_target.expr and wrongly merges qualified or generic targets; change the check to only merge when for_target is an unqualified, non-generic simple path: match crate::ast::TypeExpr::Path { segments, .. } and require segments.len() == 1 AND that the single segment has no type/generic arguments (e.g., its generics/parameters list is empty); only then look up Item::Class by comparing class.name to that single segment and merge into class.implements, otherwise retain the original Item::ImplementsFor(imp) in items.
♻️ Duplicate comments (1)
baml_language/crates/baml_compiler2_tir/src/interfaces.rs (1)
256-266:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnion target keys are still order-sensitive here.
This new insert path relies on
implementation_key_for_ty(&target_ty), but that helper still preserves union member order.implements I for int | stringand a later lookup forstring | intwill land in different buckets, so equivalent union targets can still miss nominal matches.🤖 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/interfaces.rs` around lines 256 - 266, The insertion path uses implementation_key_for_ty(&target_ty) which preserves union member order, causing keys like `int|string` and `string|int` to differ; modify implementation_key_for_ty (or add a canonicalize_union_key helper) to produce an order-independent canonical key for union types (e.g., sort or otherwise deterministically normalize union members) and then use that canonical key when inserting into type_implements_type_args and type_implements so equivalent unions map to the same bucket; update references to implementation_key_for_ty/target_key in this block to use the canonicalized key.
🤖 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/normalize.rs`:
- Around line 31-39: The canonicalization currently preserves parameter names
and positional order of named optional parameters causing equivalent function
shapes to compare unequal; update the function-type canonicalization logic (used
by is_same_normalized_type which calls normalize(...).canonicalize()) so that
required parameters are compared only by their types in positional order (ignore
parameter names) and optional/keyword parameters are compared in an
order-insensitive way (treat them as a multiset keyed by parameter type and
optional flag, ignoring the parameter name), and ensure this change is applied
in the StructuralTy / function-canonicalize code paths that produce
canonicalized Ty values (the same logic to fix in any canonicalize
implementation referenced by normalize and StructuralTy) as well as other
similar canonicalize usages mentioned in the review.
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_ast/src/lower_cst.rs`:
- Around line 160-198: The current merge finds a local class by using
segments.last() on impl.for_target.expr and wrongly merges qualified or generic
targets; change the check to only merge when for_target is an unqualified,
non-generic simple path: match crate::ast::TypeExpr::Path { segments, .. } and
require segments.len() == 1 AND that the single segment has no type/generic
arguments (e.g., its generics/parameters list is empty); only then look up
Item::Class by comparing class.name to that single segment and merge into
class.implements, otherwise retain the original Item::ImplementsFor(imp) in
items.
In `@baml_language/crates/baml_lsp2_actions/src/check.rs`:
- Around line 1043-1050: The call to collect_interface_members_with_subst is
using the caller's namespace_path which makes the walker resolve relative
extends incorrectly; change the call sites that build members (where iface,
iface_file, pkg_items, namespace_path, &subst are passed) to pass the namespace
belonging to the interface being visited (e.g., iface.namespace or resolve
namespace from iface_file/iface) instead of the caller's namespace_path so the
walker uses the interface's own namespace when resolving extends; make the same
change at the other similar call site later in the file (the second
collect_interface_members_with_subst invocation).
- Around line 894-935: The seen map currently stores raw TypeExpr strings which
get re-lowered against the current namespace when calling type_exprs_compatible,
causing incorrect comparisons for inherited fields from other namespaces; change
the seen entries (where you insert in the iface.fields seed and in the
parent-members loop) to also include the originating namespace/file
or—better—store the already-lowered type representation (the Ty or lowered form)
along with the origin (use parent_loc.file(db) or equivalent) so comparisons use
the original namespace context; update the places that call seen.get(field_name)
and the type_exprs_compatible invocation to pass the stored origin/lowered Ty
instead of re-lowering the raw TypeExpr to avoid spurious
InterfaceExtendsFieldConflict results.
---
Duplicate comments:
In `@baml_language/crates/baml_compiler2_tir/src/interfaces.rs`:
- Around line 256-266: The insertion path uses
implementation_key_for_ty(&target_ty) which preserves union member order,
causing keys like `int|string` and `string|int` to differ; modify
implementation_key_for_ty (or add a canonicalize_union_key helper) to produce an
order-independent canonical key for union types (e.g., sort or otherwise
deterministically normalize union members) and then use that canonical key when
inserting into type_implements_type_args and type_implements so equivalent
unions map to the same bucket; update references to
implementation_key_for_ty/target_key in this block to use the canonicalized key.
🪄 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: d668d561-160a-4e1e-a87b-c9022f57ab8e
📒 Files selected for processing (9)
baml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/interfaces.rsbaml_language/crates/baml_compiler2_tir/src/normalize.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_field_rule_equivalent_union_order.bamlbaml_language/crates/baml_tests/src/compiler2_tir/inference.rsbaml_language/crates/baml_tests/tests/interfaces.rs
✅ Files skipped from review due to trivial changes (1)
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/interface/valid_field_rule_equivalent_union_order.baml
…r dedup
Type rendering was scattered across ~3 duplicated `Ty` recursions plus
magic-string handling of the implicit `user` package, which leaked into
diagnostics (`user.Dog` instead of `Dog`).
- Collapse all `Ty` rendering into one `Ty::render_with(&dyn TyRenderStrategy)`
in `baml_compiler2_tir`. Canonical/user-facing, the LSP's context-aware hover,
and context-free rendering are now thin strategy impls. Consolidate qualified-
name rendering into `render_dotted`/`render_qualified`; retire the string-level
`humanize_type_string` from all `Ty` paths (effect-param -> `callback` now in
the strategy's `type_var`).
- Route the remaining HIR diagnostic name paths through `render_user_facing`,
removing the last compile-time `user.` leaks (LSP/interface snapshots updated;
every diff is a pure `user.` removal).
- Represent a qualified name's package as `enum Package { Local, Dep(Name) }`
instead of a bare `Name`; the `"user"` literal now lives only in
`Package::from_name`. `package()`/`new()` signatures are unchanged
(`Local` resolves to a `Name::new_inline` static), and `Package` orders by
name to preserve `QualifiedTypeName`'s `Ord`. Convert the MIR `== "user"`
check to `is_local()`.
- Fix the interface-implementor dedup in emit Pass 7.6: the `seen_pairs` key
used `format!("{iface_args:?}")` (the `Debug` of `Vec<baml_type::Ty>`), which
is not contractually unique across distinct types and could silently drop a
legitimate implementor. Key the args structurally (`Vec<Ty>` derives Eq/Hash).
- Refresh the wasm size-gate baseline.
Suites green: interfaces 336, baml_tests, LSP 112 + 369; rustdoc -D warnings clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Add 5 BAML benchmark workloads exercising methods, interfaces, and dispatch (mirrors verified runtime patterns from interfaces.rs): - classes/method-call, classes/method-chain (run on canary too) - interfaces/polymorphic-dispatch, match-dispatch, default-method (interfaces-only; canary fails to compile, gets skipped) All return int for clean cross-language output verification. Co-Authored-By: Claude Opus 4.8 <[email protected]>
# Conflicts: # baml_language/.cargo/size-gate.toml # baml_language/.ci/size-gate/aarch64-apple-darwin.toml # baml_language/.ci/size-gate/x86_64-pc-windows-msvc.toml # baml_language/.ci/size-gate/x86_64-unknown-linux-gnu.toml
# Conflicts: # baml_language/crates/bex_engine/src/lib.rs
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b7989d5. Configure here.
This was referenced Jun 2, 2026
This was referenced Jun 25, 2026
This was referenced Jul 20, 2026
Merged
This was referenced Jul 30, 2026
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.
Summary
Implements BEP-044 interface v2 semantics across the compiler, runtime, tests, and LSP fixtures.
as,.as<I>upcasts,requireschains, generic interface bounds, out-of-body implementation rules, and staticSelfsemantics.origin/canaryinto the branch and preserves the generatedrig_testsPython output as untracked local noise.Validation
UPDATE_EXPECT=1 cargo test -p baml_lsp2_actions_tests -- --nocapturecargo test -p baml_tests --test interfaces -- --nocapturecargo test -p baml_lsp2_actions_tests -- --nocaptureNote
High Risk
Large changes across parsing, HIR validation, MIR emit, and runtime type reflection affect core typing and dispatch; incorrect implementor registry or bound handling could cause subtle runtime misbehavior despite broad test coverage.
Overview
This PR lands BEP-044 interface v2 end-to-end: new surface syntax (
interface, in-classimplements I { … }, top-levelimplements I for T,requires, genericextendsbounds,expr.as<T>upcasts, and contextualdefaultcalls), wired through AST lowering, HIR item trees, and a large set of interface-specific diagnostics.The compiler now models interfaces, implementation blocks, and field links (
iface_field as class_field), merges simple localimplements … forinto classes, and emits a runtimeinterface_implementorsmap so reflection ontypecan answerimplements/implemented_by/implementors. BuiltinTypeValuegains those VM methods; codegen accepts interface-related keywords as method names (e.g.implements).Binary size baselines for
baml-cli,bridge_wasm, and related CI size-gate configs are bumped (~3–4%) to match the larger artifact after this work.Reviewed by Cursor Bugbot for commit 74c1f31. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Editor / LSP
Tests