Repository navigation
fix(compiler2): subtype-reduce control-flow joins and establish empty collections (B-236) - #4030
codeshaunted wants to merge 2 commits into
Conversation
… collections (B-236)
Port of typescript-go's flow-join simplification (getUnionOrEvolvingArrayType +
removeSubtypes). At a control-flow join (if/else, match arms, array/map element
joins), union members that are subtypes of other members are now dropped
(1 | int -> int, Color.Red | Color -> Color), and an unestablished empty [] / {}
is absorbed by any same-category container member: the join is an establishing
use exactly like a push, so 'if c { xs.slice(0, m) } else { [] }' types as
string[] instead of the dead-end union 'string[] | _[]' (E0007 on .join, and
before that a VM abort: expected map, got array).
Unlike the earlier attempt (#3781), two committed lists of different element
types do NOT element-join to (a | b)[], which would let a write through the
joined view corrupt an aliased a[]; they stay a union, as in TypeScript. Two
still-evolving containers join element-wise and stay evolving so a later push
can still establish the result.
Also fixes the empty Expr::Map literal committing to map<never, never> instead
of an evolving map (the Expr::Array arm already had the special case).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (15)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe TIR builder now performs subtype-aware joins, preserves evolving list/map inference, and uses instance-based join helpers across control-flow, throws, and pattern typing. New B-236 tests cover container establishment, incompatible unions, and literal subtype reduction. ChangesType joining and regression coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
baml_language/crates/baml_tests/tests/type_error_repro.rs (2)
258-378: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegression tests are internally consistent with the described join semantics; all six manually traced correctly.
One nit: per coding guidelines, unit tests are preferred over integration tests where possible. Several of these (esp. the two
if_else_literal_and_base_reduce_to_base/ subtype-reduction-only cases) test a pure function (join_types) and could additionally live as fast#[cfg(test)]unit tests directly againstTyvalues inbuilder.rs, rather than going through full compile+VM execution — though the runtime-behavior tests (branch-taken, evolving-establish-then-push, panic-on-push) do need the integration harness. Given this mirrors the codebase's existing convention for TIR regression coverage, treat this as optional.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_tests/tests/type_error_repro.rs` around lines 258 - 378, Optionally add focused #[cfg(test)] unit coverage in builder.rs for the pure join_types behavior, especially subtype reduction such as literal-plus-base and other type-only cases, using direct Ty values. Keep the existing integration tests for runtime branch execution, evolving container inference, and mutation rejection, since those require the full harness.
343-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a Map-typed analog of this test for symmetry.
This covers "committed lists of different element types stay a union, mutation rejected" for
List.join_establishes/remove_subtype_memberstreatListandMapsymmetrically inbuilder.rs, so an equivalentmap<string,int> | map<string,string>case (mutation via index-assign rejected) would round out coverage for the map side of the same guarantee.🤖 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_tests/tests/type_error_repro.rs` around lines 343 - 354, Add a map-typed counterpart to if_else_committed_lists_of_different_elements_stay_a_union, using an if/else expression that produces map<string, int> and map<string, string> values. Attempt mutation through index assignment and assert the compilation panic reports the appropriate rejected map mutation/member behavior, preserving the existing union-typing coverage pattern.
🤖 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.
Nitpick comments:
In `@baml_language/crates/baml_tests/tests/type_error_repro.rs`:
- Around line 258-378: Optionally add focused #[cfg(test)] unit coverage in
builder.rs for the pure join_types behavior, especially subtype reduction such
as literal-plus-base and other type-only cases, using direct Ty values. Keep the
existing integration tests for runtime branch execution, evolving container
inference, and mutation rejection, since those require the full harness.
- Around line 343-354: Add a map-typed counterpart to
if_else_committed_lists_of_different_elements_stay_a_union, using an if/else
expression that produces map<string, int> and map<string, string> values.
Attempt mutation through index assignment and assert the compilation panic
reports the appropriate rejected map mutation/member behavior, preserving the
existing union-typing coverage pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c5d5db46-2b92-4d66-b861-26e1af477bb5
📒 Files selected for processing (2)
baml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_tests/tests/type_error_repro.rs
…btype reduction All 15 diffs are the intended reduction from the previous commit: - 14 x 04_tir: literal-vs-base unions at if/match/catch joins collapse to the base type (1 | int -> int, true | bool -> bool, 0n | bigint -> bigint, string | "caught" -> string). - patterns_new_runtime bytecode: int[] | never[] -> int[] in load_type and baml.iter.Iterable/Iterator instantiations - the B-236 shape occurring in corpus code.
Fixes B-236. Supersedes #3781.
The bug
The if/else joined
string[]with the empty-arrayEvolvingList(never)into a union. The unestablished evolving sentinel is a dead end inside a union (no later use can establish a union arm), so member resolution fails - and before the E0007 guard existed,.joinmis-dispatched to the Map impl and aborted the VM withexpected map, got array.The fix
Port of typescript-go's flow-join simplification (
getUnionOrEvolvingArrayType+removeSubtypes, theUnionReduction.Subtypemode applied to unions synthesized at expression joins), adapted to BAML's invariant containers:1 | int->int,Color.Red | Color->Color,int | unknown->unknown. Declared (user-written) unions are untouched, as in TS.[]/{}branch is absorbed by any same-category container member. It cannot be a subtype of one (lists/maps are invariant), but the join is an establishing use exactly like apush, and theEvolvingwrapper proves the value is a fresh literal no committed alias can observe. This is what TS gets fromnever[]covariance, restricted to the sound fresh-literal case: a user-annotated committednever[]is NOT absorbed.let x = if c { [] } else { [] }; x.push(5)still establishes.(a | b)[]- that would let a write through the joined view corrupt an aliaseda[]. They stay a union, matching TS (which keepsstring[] | number[]and rejects mutation through it).Expr::Mapliterals committing tomap<never, never>instead of an evolving map (theExpr::Arrayarm already had the special case).Tests
Six regression tests in
type_error_repro.rs: the B-236 repro (both runtime paths, including the else-branch's actual empty array flowing through thestring[]binding), the map analogue, both-empty-stays-establishable, a soundness guard that committedint[]/string[]branches stay a union with mutation rejected, and literal-base reduction.Local:
type_error_reprosuite green (18/18); full workspace suite left to CI. TIR/corpus snapshot diffs may surface there (e.g. stdlibOrchestrationStep[] | never[]->OrchestrationStep[]); flagging for review rather than pre-accepting.Summary by CodeRabbit
Bug Fixes
Tests