Repository navigation
Preserve structural VM values in diagnostics and JSON - #4143
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughStructural error rendering now uses bounded, cycle-aware diagnostics and preserves qualified class identities. Typed union JSON serialization selects a matching declared member before serialization and reports mismatches. Regression tests cover rendering, truncation, supported union values, and serialization errors. ChangesStructural error rendering
Typed union JSON serialization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ErrorContext
participant render_value_structural
participant render_to_string
ErrorContext->>render_value_structural: render non-string thrown value
render_value_structural->>render_to_string: render with diagnostic state
render_to_string->>render_to_string: apply depth, node, and cycle limits
sequenceDiagram
participant TypedSerializer
participant TypeMatcher
participant MemberSerializer
TypedSerializer->>TypeMatcher: match value against declared union members
TypeMatcher-->>TypedSerializer: return first matching member
TypedSerializer->>MemberSerializer: serialize using selected member type
Possibly related PRs
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 |
aaronvg
left a comment
aaronvg
left a comment
There was a problem hiding this comment.
Both fixes are real fidelity improvements and strictly better than the old behavior (null union members, <error> placeholders). Leaving a few comments on where the fixpoint is — none of these block the patch.
Behavior change worth documenting: the union arm is now fallible where it was total. value_to_serde never raised (unrepresentable → null); render_to_serde raises JsonSerializationError for uint8arrays without encoding, functions, futures, etc. So a union member value that previously serialized (badly, as null) can now throw from baml.json.to_string<T>(). That's arguably the right semantics, but it deserves a line in the PR description and a regression test (e.g. a union member with a uint8array field).
Test suggestion: an assertion that to_string<Record>(r) == to_string<Record | Phase>(r) for a class with a nested typed field would lock in the intended equivalence between the typed-class path and the union path (see inline comment on the union arm for why they can diverge).
| // Empty override tables preserve typed `to_string`'s canonical | ||
| // behavior: custom `ToJson` implementations are not invoked. | ||
| let mut counter = 0; | ||
| render_to_serde(vm, value, &[], &[], &mut counter, path) |
There was a problem hiding this comment.
This is the right patch, but I think the fixpoint is type-directed union serialization via membership testing, not a shape-directed structural walk:
RealizedTy::Union(members, _) =>
// first member m where runtime value ∈ m (same ordered, decidable
// check BEP-015 match uses — not speculative try-and-backtrack)
match first_matching_member(vm, value, members) {
Some(m) => ty_value_to_serde(vm, value, m, path),
None => Err(raise_serialize(vm, "value is not a member of the union", path, "union")),
}Why this is more correct than routing through render_to_serde:
- Coherence by construction:
to_string<Record>(r) == to_string<Record | Phase>(r)always, because both go through the sameRealizedTy::Classarm. As written, the typed-class path (serialize_class_instance, declared-type-driven) and the union path (runtime-shape-driven) can diverge for the same value wherever the class path applies type-directed treatment to fields. - Type-directed behavior composes: media tagging, uint8array encoding rules, alias handling, and any future per-field attributes automatically apply inside unions; the structural walker has to re-implement or silently miss each of these forever.
- Honest errors: a value outside every member is a type-contract violation — raising beats serializing it as something.
The existing comment's objection ("dispatch on runtime shape rather than trying each member") conflates speculative try-and-backtrack (bad: error-swallowing, non-deterministic) with membership testing, which is the same ordered, decidable check match lowering already uses. Overlapping members resolve first-match, exactly like match arms — a semantics users already hold. TYPE_SYSTEM.md's subset-based unions give you this primitive for free.
The json alias correctly stays on value_to_serde — it's defined as untyped. Fine as a follow-up; this patch is strictly better than what it replaces.
| return format!("{} {{{}}}", class.name, fields.join(", ")); | ||
| } | ||
| "<error>".to_string() | ||
| super::root::render_value_structural(vm, value, false) |
There was a problem hiding this comment.
For this diagnostic path the governing principle should be totality over fidelity: it must never throw, never dispatch user code, and never diverge. The no-user-dispatch invariant is handled (empty override tables — and the test asserting the override is not invoked is exactly right), but render_to_string has no depth/cycle guard, and class fields are mutable (AtomicValueSlot), so a self-referential thrown value would stack-overflow while rendering an error context. The old class-name-only rendering was total by construction.
Suggested follow-up: a depth/node budget on render_value_structural that degrades to … truncation. (This weakness pre-exists in user-facing to_string, but ErrorContext is exactly the path that should be crash-proof.)
Two cosmetic notes while here: the top level renders as user.AppError {message: ...} (qualified, unpadded braces) while nested instances render as Detail { label: ... } (short name, padded braces) — both spellings now appear in one output line and are asserted verbatim in the tests. A qualified: bool knob on the walker would remove both the duplication of the instance-rendering loop and the spacing mismatch.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
baml_language/crates/bex_vm/src/package_baml/root.rs (1)
622-650: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake diagnostic collection stop at the budget.
The node guard only makes each over-budget child return
…; these loops still visit every remaining element and append one ellipsis per item. Also, depth rejection occurs before decrementing the node budget, so a wide container at the depth boundary remains unbounded.DisplaySnap::Seq/Entriesalready clone the full container before this loop, too.A thrown huge array/map can therefore still allocate and emit O(n) diagnostic output. Limit snapshots and sibling iteration to the remaining budget, then append one truncation marker. Extend the wide-value test to assert a bounded result length/count.
🤖 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/package_baml/root.rs` around lines 622 - 650, Bound diagnostic traversal in render_to_string and the DisplaySnap::Seq, DisplaySnap::Entries, and Instance branches by consuming only the remaining node budget, then append a single truncation marker when additional siblings remain. Ensure the node budget is decremented before depth-based rejection so wide containers at the depth boundary are also bounded. Avoid materializing full snapshots where possible, and extend the wide-value test to verify bounded output length/count.
🤖 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_tests/tests/exceptions.rs`:
- Line 243: Update the expected DivisionByZero snapshot in the exceptions test
to include spaces inside the braces, matching render_to_string’s structural
instance formatting.
---
Outside diff comments:
In `@baml_language/crates/bex_vm/src/package_baml/root.rs`:
- Around line 622-650: Bound diagnostic traversal in render_to_string and the
DisplaySnap::Seq, DisplaySnap::Entries, and Instance branches by consuming only
the remaining node budget, then append a single truncation marker when
additional siblings remain. Ensure the node budget is decremented before
depth-based rejection so wide containers at the depth boundary are also bounded.
Avoid materializing full snapshots where possible, and extend the wide-value
test to verify bounded output length/count.
🪄 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: d2e0c29f-71b5-46bf-b4c4-6965ad9b9e11
📒 Files selected for processing (6)
baml_language/crates/baml_tests/tests/errorcontext.rsbaml_language/crates/baml_tests/tests/exceptions.rsbaml_language/crates/baml_tests/tests/json_typed_union.rsbaml_language/crates/bex_vm/src/package_baml/error_context.rsbaml_language/crates/bex_vm/src/package_baml/json.rsbaml_language/crates/bex_vm/src/package_baml/root.rs
Root cause
Two VM rendering paths used incomplete fallbacks for values whose runtime shape carried more information than the fallback understood:
ErrorContext.to_string()executes inside a native call and cannot yield to userToStringimplementations. It therefore rendered class errors as only their class name and replaced other non-string values with<error>, losing the values needed to diagnose caught failures.uint8arraycould behave differently inside versus outside a union.Scope
<error>ToStringoverrides are not invoked from the native callJsonSerializationErrorwhen a value matches no member, or when the selected member itself is not JSON-serializable (includinguint8arraywithout explicit encoding)User impact
Caught errors retain actionable structural detail without risking unbounded diagnostic recursion.
baml.json.to_string<T>()now preserves typed-member behavior inside unions; consequently, union serialization can fail when the runtime value matches no member or the selected member is not JSON-serializable.Validation
cargo nextest run -p baml_tests --test errorcontext --test json_typed_union(19 passed)cargo nextest run -p bex_vm(61 passed)cargo clippy -p bex_vm --all-targets --all-features -- -D warningscargo fmt --all --checkgit diff --checkNote
Medium Risk
Touches core VM error rendering and typed JSON serialization paths used in diagnostics and
baml.json.to_string<T>(), with behavior changes for union members and non-class thrown values.Overview
Improves how thrown values appear in ErrorContext cause chains and stack traces, and fixes typed union JSON serialization so it follows declared member types instead of a loose runtime-shape walk.
ErrorContext / diagnostics: Non-string errors no longer collapse to a class name or
<error>. Native rendering uses a new bounded structural printer (render_value_structural) with qualified class names, recursive fields, depth/node limits, and cycle truncation—without calling userToStringoverrides (native paths cannot yield). Panic snapshots now include payload fields (e.g.DivisionByZero { dividend: 42 }).JSON:
baml.json.to_string<T>()for unions picks the first declared member that contains the value viavalue_matches_template(same asis/ match), then serializes through that member’s typed path. Class, enum, and media values are preserved; values outside the union or non-JSON members (e.g.uint8array) raiseJsonSerializationErrorconsistently inside and outside unions.Regression tests cover ErrorContext rendering, truncation budgets, and typed union JSON (including equivalence with direct class serialization).
Reviewed by Cursor Bugbot for commit 94b78bd. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
ToStringoverrides for class-thrown values.JsonSerializationError.