Repository navigation
Virtual field accesses - #4278
Merged
Merged
Conversation
And some other fixes: - A field declaration (both class and interface) without a type in AST should emit a diagnostic and get `TypeExpr::Error` instead of being optional. This ensures a unified failure path instead of every downstream consumer having a different way of handling, erasing, or ignoring the problem.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR makes field type expressions and references non-optional, replaces implementor enumeration with open-world interface field dispatch, emits positional interface-to-class field links, and adds virtual field bytecode operations with VM-side slot resolution. ChangesInterface field dispatch
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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: 5
🧹 Nitpick comments (2)
baml_language/crates/bex_vm/src/vm.rs (1)
3412-3435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew helper duplicates existing inline logic at
VirtualCall/MakeVirtualBoundMethod.
pop_interface_operandis a clean extraction, but the identical "popType, matchRealizedTy::Interface,unreachable!otherwise" block is still inlined separately in the (unchanged)OpCode::VirtualCallandOpCode::MakeVirtualBoundMethodhandlers further down this file. Now that the helper exists, those two call sites could route through it to remove the duplication.♻️ Example consolidation at the `VirtualCall` site
- let (iface_qtn, iface_args) = { - let iface_ptr = self.as_object_ptr(iface_value, ObjectType::Type)?; - match self.get_object(iface_ptr) { - Object::Type(ty) => match ty.as_ref() { - baml_type::RealizedTy::Interface(qtn, args, _assoc, _attr) => { - (qtn.clone(), args.clone()) - } - other => unreachable!( - "VirtualCall interface operand must be an Interface type, found {other:?}" - ), - }, - other => unreachable!( - "as_object_ptr(Type) guarantees a Type object, found {:?}", - ObjectType::of(other) - ), - } - }; + let (iface_qtn, iface_args) = self.pop_interface_operand(iface_value)?;🤖 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/src/vm.rs` around lines 3412 - 3435, Replace the duplicated interface-operand extraction logic in the OpCode::VirtualCall and OpCode::MakeVirtualBoundMethod handlers with calls to pop_interface_operand. Preserve the existing interface name and argument handling, including the current unreachable behavior for non-interface types.baml_language/crates/baml_compiler2_ast/src/lib.rs (1)
1144-1147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the new
Errorrecovery sentinel.The invariant introduced in
lower_cst.rs(a typeless field now yieldsTypeExprKind::Errorrather thanNone) has no test in this module. A small case parsingclass C { f }and assertingfield.type_expr.kindisErrorwould pin the new contract, since every other assertion here only exercises the well-typed path.🤖 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/lib.rs` around lines 1144 - 1147, Add a focused test alongside the existing AST parsing tests that parses `class C { f }`, retrieves the field through the relevant class/field symbols, and asserts its `field.type_expr.kind` is `TypeExprKind::Error`. Keep the existing well-typed assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_compiler2_mir/src/ir.rs`:
- Around line 324-337: Update baml_compiler2_mir::optimize to handle
VirtualFieldStore in every statement traversal: count its receiver/value
operands in count_in_statement, include it in scan_statement and count_defs,
preserve it during dead-assignment removal and assignee finding, and rewrite
both operands through apply_subst_to_statement and rewrite_locals_in_statement.
Keep its field metadata unchanged while applying existing operand
substitution/local-rewrite behavior.
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 10277-10365: Update virtual_field_assign_target to resolve and
return the declaring interface view and field index via
interface_view_declaring_field, returning None before lowering the receiver or
value when resolution fails. Adjust try_lower_virtual_field_assign and
try_lower_virtual_field_assign_op to consume that resolved data and pass it to
the interface access/store helpers, preventing side effects before a possible
fall-through.
In `@baml_language/crates/baml_compiler2_mir/src/optimize.rs`:
- Around line 1600-1601: Update verify_mir’s statement match to add an explicit
VirtualFieldStore arm, validating both its receiver and value operands instead
of relying on the wildcard branch. Keep the existing VirtualFieldAccess receiver
validation unchanged.
- Around line 696-701: Update collect_place_index_locals in
baml_language/crates/baml_compiler2_mir/src/optimize.rs:696-701 to scan both
receiver and value for VirtualFieldStore, and update the debug verification
visitor at baml_language/crates/baml_compiler2_mir/src/optimize.rs:1600-1601 to
explicitly verify both operands rather than relying on the wildcard arm.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 12453-12507: Update qualified_interface_field_for_construction so
both lower_type_ref ScopeCtx branches provide the same rigid Self type variable
bounded by the current interface, matching interface_member_lowering_env in
interface_resolution.rs, instead of passing self_ty: None. Add a regression test
covering an interface field typed with Self.Assoc through the
construction-qualification path, verifying it resolves as a type variable or
associated-type projection without cascading recovery errors.
---
Nitpick comments:
In `@baml_language/crates/baml_compiler2_ast/src/lib.rs`:
- Around line 1144-1147: Add a focused test alongside the existing AST parsing
tests that parses `class C { f }`, retrieves the field through the relevant
class/field symbols, and asserts its `field.type_expr.kind` is
`TypeExprKind::Error`. Keep the existing well-typed assertions unchanged.
In `@baml_language/crates/bex_vm/src/vm.rs`:
- Around line 3412-3435: Replace the duplicated interface-operand extraction
logic in the OpCode::VirtualCall and OpCode::MakeVirtualBoundMethod handlers
with calls to pop_interface_operand. Preserve the existing interface name and
argument handling, including the current unreachable behavior for non-interface
types.
🪄 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 Plus
Run ID: 62035413-6cc6-4376-b45f-f72513042102
⛔ Files ignored due to path filters (41)
baml_language/crates/baml_tests/snapshots/baml_src/array_rest_binding.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/comparable_sort.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/csv.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/defer.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/for_loops.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/interfaces.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/interfaces_associated_types.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/interfaces_class_generics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/iter.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/iter_closure_elements.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lambdas.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lexical_scoping.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/null_handling.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/patterns_new_runtime.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/promptast_accessors.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/streaming_parsing.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/string_regressions.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/typed_inputs.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/compiles/__testing_std__/baml_tests__compiles____testing_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__testing_std__/baml_tests__compiles____testing_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/anyfunction_reflect/baml_tests__compiles__anyfunction_reflect__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/anyfunction_reflect/baml_tests__compiles__anyfunction_reflect__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/backtick_dedent/baml_tests__compiles__backtick_dedent__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/backtick_dedent/baml_tests__compiles__backtick_dedent__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/backtick_strings/baml_tests__compiles__backtick_strings__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/backtick_strings/baml_tests__compiles__backtick_strings__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closure_loop_variable/baml_tests__compiles__closure_loop_variable__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closure_loop_variable/baml_tests__compiles__closure_loop_variable__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lexical_scoping/baml_tests__compiles__lexical_scoping__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lexical_scoping/baml_tests__compiles__lexical_scoping__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_statements/baml_tests__compiles__parser_statements__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_statements/baml_tests__compiles__parser_statements__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/patterns_new/baml_tests__compiles__patterns_new__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/patterns_new/baml_tests__compiles__patterns_new__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_dynamic/baml_tests__compiles__testset_dynamic__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/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_textual.snapis excluded by!**/*.snap
📒 Files selected for processing (48)
baml_language/crates/baml_builtins2_codegen/src/extract.rsbaml_language/crates/baml_cli/src/bytecode_cache.rsbaml_language/crates/baml_cli/src/file_signature.rsbaml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/disambiguate.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_emit/src/analysis.rsbaml_language/crates/baml_compiler2_emit/src/emit.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_emit/src/pull_semantics.rsbaml_language/crates/baml_compiler2_emit/src/stack_carry.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/classes.rsbaml_language/crates/baml_compiler2_mir/src/builder.rsbaml_language/crates/baml_compiler2_mir/src/ir.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_mir/src/optimize.rsbaml_language/crates/baml_compiler2_mir/src/pretty.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/classes.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/common.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/interfaces.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/definition.rsbaml_language/crates/baml_lsp2_actions/src/describe.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_tests/baml_src/ns_interfaces/interfaces.bamlbaml_language/crates/baml_tests/src/compiler2_emit/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/mod.rsbaml_language/crates/baml_type/src/template.rsbaml_language/crates/bex_vm/src/debug.rsbaml_language/crates/bex_vm/src/package_load.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm_types/src/bytecode.rsbaml_language/crates/bex_vm_types/src/errors.rsbaml_language/crates/bex_vm_types/src/link.rsbaml_language/crates/bex_vm_types/src/relink.rsbaml_language/crates/bex_vm_types/src/types/interface.rsbaml_language/crates/bex_vm_types/src/types/package.rsbaml_language/crates/bex_vm_types/src/unit.rsbaml_language/tools/speedtest/workloads/interfaces/field-access-100k.md
- Fix missing `match` arms and make exhaustive - Fix emit ordering to avoid producing incomplete MIR if lowering fails
2kai2kai2
enabled auto-merge
July 29, 2026 23:57
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.
Previously we emitted a type tag-based switch when accessing interface fields on a non-concrete type. This was unsound and produced unnecessary overhead, so similar to the already-implemented virtual function calls, this PR adds virtual field load/stores.
Additionally:
TypeExprKind::Errorand emit a diagnostic in the AST, instead of being optional (since downstream consumers handled theNonein different ways)TyTemplateInterfacewhere possible as the representation for the interface position (this still doesn't work on the stack but it does everywhere else)Summary by CodeRabbit
New Features
Bug Fixes
Tests