fix: upstream independent compiler and runtime fixes - #4308
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR updates CLI project discovery, parser and formatter preservation, throw analysis, control-flow dispatch, media wrapper handling, structured value rendering, and test error reporting. It adds regression coverage for explicit source roots, header comments, generic errors, interface refinement, and qualified literals. ChangesCLI project and source resolution
Parser and formatter syntax handling
Throw analysis and reachability
Resolved control-flow graph dispatch
Runtime value and media handling
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ProjectLayout
participant SourceDiscovery
CLI->>ProjectLayout: resolve project and source roots
ProjectLayout->>SourceDiscovery: provide source root
SourceDiscovery-->>CLI: return BAML files
sequenceDiagram
participant Caller
participant TypeInference
participant CFG
Caller->>TypeInference: resolve function or interface target
TypeInference-->>CFG: return location and dispatch bindings
CFG->>CFG: expand concrete callee
CFG-->>Caller: return resolved graph
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Binary size checks passed✅ 7 passed
Generated by |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/baml_compiler2_tir/src/callable.rs (1)
266-276: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDead fallback: the new type-var fallback can never execute.
instantiated_callee_throwsreturns early at line 274-276 whenevercall_plan.instantiated_throwsisSome. Execution reaches the closurefunction_throws(and thus line 319-325) only when that same field isNone. The new checkcall_plan.and_then(|plan| plan.instantiated_throws.as_ref())at line 321 tests the identical field and can never beSomeat this point.This fallback is unreachable. It does not implement the intended behavior described for this change ("falls back to the call plan's instantiated throws when type variables remain unresolved").
🐛 Proposed fix: remove the top-level short-circuit so the per-parameter substitution can run, and only fall back to the recorded call-plan throws for members whose substitution still contains a type variable
pub(crate) fn instantiated_callee_throws( inference: &ScopeInference<'_>, aliases: &HashMap<crate::ty::QualifiedTypeName, Ty>, callee_expr_id: baml_compiler2_ast::ExprId, args: &[baml_compiler2_ast::ExprId], unwrap_optional_callee: bool, call_plan: Option<&CallPlan>, ) -> Option<Ty> { - if let Some(throws) = call_plan.and_then(|plan| plan.instantiated_throws.clone()) { - return Some(throws); - } let callee_ty = inference.expression_type(callee_expr_id)?;If the top-level check is load-bearing for another call path, verify with the codebase whether any caller relies on the early return returning before
callee_ty/typed_calleeare computed (e.g. to avoid a panic on a missing expression type), and adjust the fix accordingly.Also applies to: 319-325
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_tir/src/callable.rs` around lines 266 - 276, Remove or restructure the early return in instantiated_callee_throws so the per-parameter substitution and function_throws logic can execute when needed. In function_throws, use the call plan’s recorded instantiated throws only for members whose substitution still contains unresolved type variables, while preserving any necessary guard for callers that lack callee expression types.
🧹 Nitpick comments (3)
baml_language/crates/baml_project/src/db.rs (1)
1100-1123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a stable key format instead of
{ty:?}.
cfg_expansion_keyrenders each dispatch binding withDebug. The cache key then depends on theTyDebugoutput, which is not a stability contract. Two structurally equal types that print differently produce separate cache entries, and futureDebugchanges silently change key shape. IfTyimplementsDisplayor a canonical rendering, prefer that.This affects cache hit rate only, not correctness.
🤖 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_project/src/db.rs` around lines 1100 - 1123, Update cfg_expansion_key to render dispatch binding types with Ty’s stable Display or canonical representation instead of Debug formatting. Keep the existing binding sorting and function identity composition unchanged, ensuring structurally equal types produce identical cache keys across formatting changes.baml_language/crates/baml_fmt/src/ast/expressions.rs (1)
1568-1602: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a fmtlib unit test for arm header comments.
arm_header_comments/main.bamlhas a generated10_formattersnapshot, but that formatter is covered by the compiler test project. Add abaml_fmtRust unit test that exercises headers before and between match and catch arms and rounds throughformattwice. Runcargo test --libas part of this change.🤖 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_fmt/src/ast/expressions.rs` around lines 1568 - 1602, Add a baml_fmt Rust unit test covering header comments before and between match and catch arms, using the existing arm_header_comments fixture or equivalent input. Assert formatting is stable by running format twice and comparing the results, and place the test with the formatter’s library tests so it runs under cargo test --lib.Source: Coding guidelines
baml_language/crates/sys_ops/src/lib.rs (1)
3070-3111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for all structured media wrappers in the prompt AST test.
This test only uses a
baml.media.Pdfwrapper with anAdt(Media(...))payload. Add cases forImage,Audio, andVideo, and add one missing-path case that constructs the shorthand media primitive directly without the wrapper_datapayload, so regressions in the shorthand and non-Pdf paths are covered.Run
cargo test --libfor this Rust change to confirm the new test passes.🤖 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/sys_ops/src/lib.rs` around lines 3070 - 3111, Extend assemble_prompt_ast_preserves_pdf_as_structural_media to cover baml.media.Image, baml.media.Audio, and baml.media.Video wrappers using Adt(Media(...)) payloads, plus one shorthand media primitive without the wrapper _data field. Assert each case produces the expected PromptAstSimple::Media kind and value, while preserving the existing PDF coverage, then run cargo test --lib.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@baml_language/crates/baml_compiler2_tir/src/callable.rs`:
- Around line 266-276: Remove or restructure the early return in
instantiated_callee_throws so the per-parameter substitution and function_throws
logic can execute when needed. In function_throws, use the call plan’s recorded
instantiated throws only for members whose substitution still contains
unresolved type variables, while preserving any necessary guard for callers that
lack callee expression types.
---
Nitpick comments:
In `@baml_language/crates/baml_fmt/src/ast/expressions.rs`:
- Around line 1568-1602: Add a baml_fmt Rust unit test covering header comments
before and between match and catch arms, using the existing arm_header_comments
fixture or equivalent input. Assert formatting is stable by running format twice
and comparing the results, and place the test with the formatter’s library tests
so it runs under cargo test --lib.
In `@baml_language/crates/baml_project/src/db.rs`:
- Around line 1100-1123: Update cfg_expansion_key to render dispatch binding
types with Ty’s stable Display or canonical representation instead of Debug
formatting. Keep the existing binding sorting and function identity composition
unchanged, ensuring structurally equal types produce identical cache keys across
formatting changes.
In `@baml_language/crates/sys_ops/src/lib.rs`:
- Around line 3070-3111: Extend
assemble_prompt_ast_preserves_pdf_as_structural_media to cover baml.media.Image,
baml.media.Audio, and baml.media.Video wrappers using Adt(Media(...)) payloads,
plus one shorthand media primitive without the wrapper _data field. Assert each
case produces the expected PromptAstSimple::Media kind and value, while
preserving the existing PDF coverage, then run cargo test --lib.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e8bf2e0e-2aca-4620-bce8-3ccb015de828
⛔ Files ignored due to path filters (14)
baml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_namespace_llm.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/arm_header_comments/baml_tests__compiles__arm_header_comments__03_ppir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/arm_header_comments/baml_tests__compiles__arm_header_comments__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/arm_header_comments/baml_tests__compiles__arm_header_comments__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/arm_header_comments/baml_tests__compiles__arm_header_comments__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/arm_header_comments/baml_tests__compiles__arm_header_comments__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/arm_header_comments/baml_tests__compiles__arm_header_comments__10_formatter__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/catch_interface_refinement/baml_tests__compiles__catch_interface_refinement__03_ppir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/catch_interface_refinement/baml_tests__compiles__catch_interface_refinement__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/catch_interface_refinement/baml_tests__compiles__catch_interface_refinement__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/catch_interface_refinement/baml_tests__compiles__catch_interface_refinement__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/catch_interface_refinement/baml_tests__compiles__catch_interface_refinement__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/catch_interface_refinement/baml_tests__compiles__catch_interface_refinement__10_formatter__main.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
baml_language/crates/baml_builtins2/baml_std/baml/ns_llm/llm_types.bamlbaml_language/crates/baml_builtins2/baml_std/testing/registry.bamlbaml_language/crates/baml_cli/src/format.rsbaml_language/crates/baml_cli/src/playground_command.rsbaml_language/crates/baml_cli/src/project_load.rsbaml_language/crates/baml_cli/tests/exit_code_e2e.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/callable.rsbaml_language/crates/baml_compiler_parser/src/parser.rsbaml_language/crates/baml_fmt/src/ast/expressions.rsbaml_language/crates/baml_project/src/db.rsbaml_language/crates/baml_tests/projects/compiles/arm_header_comments/main.bamlbaml_language/crates/baml_tests/projects/compiles/catch_interface_refinement/main.bamlbaml_language/crates/baml_tests/src/compiler2_tir/phase8_exceptions.rsbaml_language/crates/bex_engine/src/trace_value_encode.rsbaml_language/crates/bex_engine/src/value_capture.rsbaml_language/crates/bex_engine/tests/log_intrinsic.rsbaml_language/crates/sys_ops/src/lib.rs
Three fixes surfaced by the ai_agents reference implementation (_plan/reference_notes.md #1, #2, #7), plus a regression pin for #3: - fmt: `client` is a contextual keyword (KW_CLIENT) but a legal identifier; the formatter died on it as a field name, parameter name, object key, or path-expression head ("Expected token/node of kind WORD, but found KW_CLIENT"). Word::from_cst and the WORD-only dispatch sites now accept KW_CLIENT, mirroring the parser. - parser: header comments (`//#`) were only recognized at statement boundaries inside blocks; in class bodies, implements blocks, interface bodies, and between match arms they lexed as `/ / #` and produced cascading parse errors. The member/arm loops now consume them, and the formatter's strong AST carries them (ClassItem, ImplementsItem, MatchArmItem). - parser: a bare identifier as a *top-level* test/testset name can never resolve, and the old failure mode was a misleading E0003 "unresolved name". Now reported at parse time as "test(set) names must be quoted strings: `testset \"foo\"`". Identifiers remain legal in nested position, where names may be computed from loop variables. - pins a regression fixture for qualified struct literals inside array literals (#3 in the notes), which no longer reproduces on canary. The while(true) divergence fix (#5) is parked on divergence-e0029-wip: it exposes a pre-existing full-vs-splice compile mismatch (bare-name class-object fallback in emit resolves `Done` to different classes depending on registration order; see emit.rs class_object_index_for_type_name). Needs an emit-layer decision first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The upstream fix commit added media-type match arms to baml_std/baml/ns_llm/llm_types.baml without regenerating the downstream PPIR/TIR/MIR/codegen snapshots. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # baml_language/crates/baml_builtins2/baml_std/baml/ns_llm/llm_types.baml # baml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_namespace_llm.snap # baml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snap # baml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_ppir.snap # baml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snap # baml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_tir.snap # baml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snap # baml_language/crates/sys_ops/src/lib.rs
…lippy - bytecode_format snapshots: make_closure indices shifted by the merged testing/registry.baml change - rustfmt: drop stray blank line in baml_fmt expressions.rs - clippy: fix redundant closure and redundant clone in baml_project db.rs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The baml.media.{Image,Audio,Video,Pdf} <-> MediaKind mapping was
string-matched independently in bex_engine's conversion layer and the
jinja value converter, and the wrappers' private `_data` field name was
spelled at each consumer. Adding a media type (or renaming the payload
field) meant finding every copy.
- MediaKind::wrapper_class_name / from_wrapper_class_name in baml_base
are now the only place the class names exist, with a round-trip test.
- BexExternalValue::media_wrapper_kind / media_wrapper_inner (plus the
MEDIA_WRAPPER_DATA_FIELD constant) own the wrapper-layout knowledge;
the jinja converter and the engine's media-payload unwrap go through
them instead of matching strings and naming `_data` locally.
- Test fixtures keep literal class names on purpose: they pin the
public contract the registry must keep producing.
(applied from 74fbb6020, branch media-kind-centralize)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI pre-commit runs clippy with --all-targets -D warnings; KW_CLIENT in the contextual-keyword test's doc comment needed backticks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Brings the branch up to date with canary (146 conflicted files) and ports every TIR-side change onto the hir_ty substrate: - #4311 equirecursive types: canary's mu-automaton (phase-typed NormalTy<Named|Canonical>, de Bruijn binders, canonical_bottom_up) merged with this branch's B-1091 co-inductive assumption threading; the interned entry re-phased through the same pipeline. Same-shape recursive aliases now provably overlap (coherence twin updated). - #4320 tooling surface: hir_ty gains callable::function_signature_ty (a view over function_signature; own generics only; Self resolved for interface-default and free-impl methods), interfaces:: {InterfaceDeclScope, resolve_interface_fields, resolve_interface_required_methods, interfaces_declaring_associated_type}, and package_interface::exported_function (one place pairing the signature query with the effective-throws oracle). baml_surface is re-pointed tir -> hir_ty with its facts.rs contract intact. - #4308 catch-arm interface facts: reachability respects implements in both directions (the semantic contract compiles with zero diagnostics); the arm lowering keeps the modular Interface narrow rather than the closed-world concrete residual. - #4291 intersection bounds, consumption side: GenericParamData conjunction shapes adopted end to end; class constructor sites register one Implements obligation per declared bound conjunct (register_class_bounds, rustc's ADT well-formedness discipline); interface-member lookup pools declarers across the bound conjunction, dedupes by realized identity through requires, and reports E0121/E0122 ambiguity (rustc's MethodError::Ambiguity shape) instead of first-conjunct-wins; a failing blanket-impl bound names the unsatisfied conjunct (BlanketBoundNotSatisfied) instead of a bare mismatch. - #4352 @spec/ai builtins compile and emit on hir_ty. Four pre-existing engine gaps they exposed are fixed: * a bare AnyFunction's unpinned members read as their declared `unknown` defaults at the oracle and in the engine's same-interface unification (BEP-062 lazy default; no eager fill at lowering); * binding a bounded inference var by direct unification now REPLAYS its accumulated VarBounds against the solution (take_solved_class_bounds + replay in the finish fixpoint) instead of dropping them - the map-lambda + future.all shape no longer strands its type args; * an empty container literal flowing into a ground `unknown` demand commits its establishment slots to the top type (the demand is a consuming use); a literal with NO demand keeps the strict uninferrable-container error; * required interface methods (bodyless items under the unified method model) are excluded from the codegen symbol pool. - Coherence: canary's PreparedImpl refactor adopted on Facts::with_bounds - the subject-validity gate now judges the same normalized spelling E0138 judges, memoized per impl. - baml_project CFG dispatch (canary's interface virtual-call resolution in the visualization) re-pointed onto hir_ty's InferenceResult and impls_for_type. - check.rs: the legacy jinja prompt checker is deleted with canary's `client<llm>`/jinja removal; the associated-type declarer walk uses the conjunction-deduping interfaces_declaring_associated_type. - Tests: fixtures migrated off removed builtins (baml.deep_equals -> ops.Equals `==`; a local generic pair fn where the fixture probes inference); snapshots re-blessed for the unified required-method model ([missing] items), canary's builtin/std content, and canonical union order.
Summary
//#headersThe experimental
baml_std/aipackage, AI-only compiler/runtime work, provider continuation authentication, temporary AI fixtures, and realtime process APIs are intentionally excluded.Validation
cargo fmt --all -- --checkcargo nextest run -p baml_cli(539 passed)cargo nextest run -p bex_engine(595 passed; one pre-existing leaky test annotation)cargo nextest run -p baml_project(81 passed)cargo nextest run -p sys_ops(12 passed)cargo nextest run -p baml_tests arm_header_comments(6 passed)cargo nextest run -p baml_tests catch_interface_refinement(6 passed)cargo nextest run -p baml_tests generic_bound_associated_error_is_reused_by_throws_analysis(1 passed)Note
Medium Risk
Changes span project discovery (could load unintended trees only when
--fromis explicit), catch/throws typing, and LLM prompt assembly—areas that affect compile correctness and provider requests—but behavior is heavily covered by new e2e and snapshot tests.Overview
Compiler & formatting: Match/catch arm parsing no longer spins on empty recovery or rejects
//#header comments between arms; the formatter AST carries those headers viaArmListItem. Catch-arm reachability now treats interface throw facts and concrete refiners symmetrically (subtype checks), removing false unreachable-arm warnings. Generic calls can reuse instantiatedthrowsfrom the call plan when type variables would otherwise leak.CLI project loading: Introduces
ProjectLayout(resolve_project_layout) splitting settings root vs source root. An explicit--fromloads that tree even withoutbaml.toml/baml_src/, and does not redirect a sibling directory (e.g. alternatebaml_src_temp2) into the primarybaml_src/while still picking up an ancestor manifest when present.fmt,run, and introspection paths share this logic; playground keeps marker-based discovery.LLM prompts & logs:
render_prompt_valuesleaves Role and media types uncooked;assemble_prompt_astbuilds multi-part content (including PDF URLs) instead of stringifying media away. Captured trace values forbaml test --logsrender as BAML-like structural strings, not protobuf debug dumps. Testrun_testusescatch_all (e, ctx)so non-panic failures surface rendered error context in CLI output.Playground CFG: Control-flow graph expansion resolves calls through TIR (methods, interface virtual dispatch with binding-aware cache keys) so generic
runner.run(self)inlines concrete implementer bodies in visualization.Reviewed by Cursor Bugbot for commit 0f29a6c. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Additional fixes (follow-up commits)
clientis used as an identifier (KW_CLIENT treated as a plain identifier in expression positions).//#header comments are now accepted in class bodies,implementsblocks, and interface bodies — extending the existing match/catch-arm header-comment support.test/testsetname is written as a quoted string instead of an identifier (with a broken-syntax project covering it).ns_header_commentsandns_qualified_literals(qualified struct literals).canary(the in-BAMLrender_prompt_valuesmedia handling was superseded by canary's nativeassemble_prompt_ast; this branch keeps canary's version) and regenerated stale std/bytecode snapshots.MediaKind(wrapper_class_name/from_wrapper_class_name); wrapper layout knowledge moved next toBexExternalValue(media_wrapper_kind/media_wrapper_inner,MEDIA_WRAPPER_DATA_FIELD), replacing per-consumer string matches in bex_engine and the jinja value converter.