v3: lower value-context match/if block operands with !/? propagation (fix #28000) - #28011
v3: lower value-context match/if block operands with !/? propagation (fix #28000)#28011medvednikov wants to merge 37 commits into
!/? propagation (fix #28000)#28011Conversation
…ion (fix #28000) A `match`/`if` used as the *value* of a cast, string-interpolation part, `dump()`, prefix/infix operand, index expression, selector receiver, or `as` operand must have its (possibly propagating) branch tails lowered as values. Previously they were lowered in a value-less statement context, emitting an empty ternary / expression. Adds `transform_value_operand` + `is_value_match_or_if_operand` helpers (the latter looks through `(...)`, `unsafe { }` and trailing `expr_stmt` wrappers) and routes the affected operands through target-typed lowering. Regression test: vlib/v3/tests/match_as_if_expr_value_propagation_codegen_test.v
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fd6937284
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new_lhs := t.transform_value_operand(lhs_id) | ||
| mut lhs_pending := []flat.NodeId{} | ||
| if t.pending_stmts.len > pending_start { | ||
| lhs_pending = t.pending_stmts[pending_start..].clone() | ||
| t.pending_stmts = t.pending_stmts[..pending_start].clone() | ||
| } | ||
| new_rhs := t.transform_expr(rhs_id) | ||
| new_rhs := t.transform_value_operand(rhs_id) |
There was a problem hiding this comment.
For specialized infix expressions, these new calls are unreachable because the string/array/map/interface/sum/struct handlers above return first. For example, (match x { A { get_a()! } else { get_b()! } }) + suffix enters transform_infix_string_ops, whose operands are still lowered with plain transform_expr in vlib/v3/transform/expr.v, reproducing the statement-context lowering and empty generated expression this change is intended to fix. Apply the value-aware lowering in the specialized handlers as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 2e31fa4. Rather than patching each specialized handler, I materialize value-context match/if operands into value temps at the dispatch point in transform_infix_expr — before the string/array/map/interface/sum/struct handlers run — then re-dispatch over the rewritten node. This routes every specialized handler (and the generic fallthrough) through value-aware lowering uniformly. Only the value-branch operand is materialized; the other side keeps its original node and is transformed exactly once by the re-dispatch.
No regression: match_as_if_expr_value_propagation_codegen_test.v still passes, and review_transform_regressions / review_cgen_regressions / review_checker_regressions / or_expr_transform_review show the same pre-existing failures as master (unchanged 3 failed / 1 passed).
One caveat: I could not add a runnable regression test for the string-operand case. A propagating match whose value type routes through a specialized handler (e.g. string/[]T/struct) currently trips two separate pre-existing v3 crashes that bracket this code — a parallel late-scan race in seed_generated_fn_body_context (before transform) and a post-transform segfault — both reproducible on clean master. They're out of scope for this change; happy to file them separately.
Addresses review feedback on #28011. The type-specialized infix handlers (`transform_infix_string_ops`, `_array_ops`, `_interface_ops`, `_sum_ops`, `_struct_ops`) run before the generic fallthrough and lower their operands with plain `transform_expr`. A value-context `match`/`if` operand routed to one of them (e.g. `(match x { First { get_a()! } else { get_b()! } }) + suffix`, which enters the string handler) therefore had its propagating branch tails lowered in a value-less statement context, reproducing the empty-expression bug this PR fixes for the fallthrough path. Materialize value-branch `match`/`if` operands into value temps at the dispatch point, before the specialized handlers run, then re-dispatch over the rewritten node so every handler (and the fallthrough) sees a plain, typed operand. Only the value-branch operand is materialized; the other side keeps its original node and is transformed exactly once by the re-dispatch.
Addresses review feedback on #28011. The type-specialized infix handlers (`transform_infix_string_ops`, `_array_ops`, `_interface_ops`, `_sum_ops`, `_struct_ops`) run before the generic fallthrough and lower their operands with plain `transform_expr`. A value-context `match`/`if` operand routed to one of them (e.g. `(match x { First { get_a()! } else { get_b()! } }) + suffix`, which enters the string handler) therefore had its propagating branch tails lowered in a value-less statement context, reproducing the empty-expression bug this PR fixes for the fallthrough path. Materialize value-branch `match`/`if` operands into value temps at the dispatch point, before the specialized handlers run, then re-dispatch over the rewritten node so every handler (and the fallthrough) sees a plain, typed operand. Only the value-branch operand is materialized; the other side keeps its original node and is transformed exactly once by the re-dispatch.
5d3da8f to
2e31fa4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e31fa4088
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A value-context `match`/`if` operand (e.g. `(match x { First { get_a()! } | ||
| // else { get_b()! } }) + suffix`) must be materialized as a value before the | ||
| // type-specialized handlers below dispatch on operand type. Those handlers | ||
| // (string/array/map/interface/sum/struct ops) lower their operands with plain | ||
| // `transform_expr`, which would lower the (possibly propagating) branch tails | ||
| // in a value-less statement context and emit an empty expression. Materialize |
There was a problem hiding this comment.
This value-aware dispatch occurs after the early .left_shift return, so numeric shifts such as 1 << (match x { A { get_a()! } else { get_b()! } }) still lower the RHS with plain transform_expr because rhs_target_type is empty. The propagating branch tail therefore retains the empty-expression failure this change fixes for other infix operators; the left-shift path must use the same value-aware lowering for match/if operands.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 5e2e8de. The .left_shift branch now lowers both operands via transform_value_operand (a no-op for the common non-branch operands), so 1 << (match … get()! …) and the symmetric (match …) << 2 materialize the propagating branch tail as a value. The array-append RHS keeps its element-typed transform_expr_for_type path.
Same caveat as the other threads: I couldn't add a runnable regression test — a left-shift with a value match operand currently trips separate pre-existing v3 issues (a checker unused expression for << in block-tail position, and the parallel late-scan crash in seed_generated_fn_body_context for as/propagating arms), both reproducible on clean master. Existing tests pass and the review_transform / review_cgen / review_checker / or_expr_transform suites show the same pre-existing failures as master (no new regressions).
| } else if t.is_value_match_or_if_operand(child_id) { | ||
| // A `match`/`if` cast operand is a value expression whose (possibly | ||
| // propagating) branch tails must be lowered as values, e.g. | ||
| // `i64(match x { ... foo()! ... })`. Plain `transform_expr` would | ||
| // lower them in statement context and emit an empty ternary. | ||
| new_children << t.transform_expr_for_type(child_id, target_type) |
There was a problem hiding this comment.
For a cast such as Shape(match x { A { make_circle()! } else { make_square()! } }), transform_cast_expr returns through the earlier is_sum_type_name(target_type) branch before reaching this new condition. wrap_sum_value ultimately lowers the match with plain transform_expr, so propagated branch tails can still produce the empty generated expression; materialize the match as a value before the sum-target cast path returns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 5e2e8de. transform_cast_expr now materializes a value match/if operand of a sum-type cast through the target sum type before the wrap_sum_value path returns, mirroring the as-cast path — transform_expr_for_type(child, sum_type) is already exercised for a match-returning-sum by select_value_ascast. So Shape(match x { … make_circle()! … }) lowers its propagating branch tails as values instead of via wrap_sum_value → plain transform_expr.
Same caveat: no runnable regression test yet — Shape(match …) with propagating arms hits the pre-existing parallel late-scan crash in seed_generated_fn_body_context, reproducible on clean master. Existing tests pass, no new regressions.
…ering (#28000) Addresses review feedback on #28011. Two more value-context `match`/`if` operand paths bypassed the value-aware lowering: - Left-shift: the early `.left_shift` branch lowered its operands with plain `transform_expr` when `rhs_target_type` is empty, so a numeric shift such as `1 << (match x { First { get_a()! } else { get_b()! } })` (and the symmetric `(match ...) << 2`) kept the empty-expression failure. Use `transform_value_operand` for both operands (a no-op for non-branch operands; the array-append RHS keeps its `transform_expr_for_type` element-typed path). - Sum-type cast: `transform_cast_expr` returns via the `is_sum_type_name` branch into `wrap_sum_value`, which lowers the operand with plain `transform_expr`, before reaching the value-aware cast loop. Materialize a value `match`/`if` operand through the target sum type first (mirrors the `as`-cast path), e.g. `Shape(match x { ... make_circle()! ... })`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e2e8dee39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if t.is_value_match_or_if_operand(child_id) { | ||
| // A `match`/`if` cast operand is a value expression whose (possibly |
There was a problem hiding this comment.
Unlike the now-covered plain Shape(...) cast, type-specific paths return before this generic value-aware guard: ?Shape(match n { ... make_circle()! ... }) exits through the optional-sum branch and Animal(match n { ... make_dog()! ... }) exits through interface boxing. Both paths ultimately lower the match with plain transform_expr, reproducing the empty propagating branch-tail expression; materialize match/if operands before cast-type dispatch and cover the optional/interface cases with regression tests.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 800519e. Instead of guarding each cast branch, transform_cast_expr now materializes a value match/if cast operand into a value temp once, before cast-type dispatch, and re-dispatches over the rewritten operand — so the optional-sum (?Shape(match …)), interface (Animal(match …)), pointer-to-sum (&Shape(match …)), sum and generic paths all see a plain, typed operand. This subsumes the per-branch sum/generic guards from the previous commit, which are consolidated away. Verified the existing i64(match …) cast cases still pass.
On the regression tests: I tried, but each of these scenarios can't compile end-to-end yet. I verified ?Shape(match …), Animal(match …) and (match …) is Circle all crash at HEAD (this commit reverted) via the same pre-existing v3 issues — the parallel late-scan race in seed_generated_fn_body_context (before transform) and a post-transform cgen segfault (both reproduce single-threaded and on clean master). The covered as-cast case (select_value_ascast) only compiles because it happens to avoid those paths. Happy to file the two pre-existing crashes as separate issues so these tests can land once they're fixed.
No new regressions: same 3 fail / 1 pass in review_transform / review_cgen / review_checker / or_expr_transform as master, and review_cgen (heavy cast coverage) passes.
| first_child := t.a.child(&node, 0) | ||
| if t.is_value_match_or_if_operand(first_child) { |
There was a problem hiding this comment.
is subjects through value-aware lowering
The analogous type-test form remains uncovered: (match n { ... make_circle()! ... }) is Circle is handled by transform_is_expr, which still lowers its subject with plain transform_expr at sum.v:773. Therefore a propagating match arm can still generate the empty expression this change fixes for as; apply the same value materialization before building the is tag check and add a regression case.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 800519e. transform_is_expr now routes the subject through transform_value_operand before building the tag check (the sum.v:773 lowering), so (match n { … make_circle()! … }) is Circle materializes the propagating branch tail as a value instead of in a value-less statement context.
Same testing caveat as the cast thread: I verified (match …) is Circle crashes at HEAD (this commit reverted) via the pre-existing parallel late-scan race in seed_generated_fn_body_context plus a post-transform cgen segfault (both reproduce single-threaded and on clean master), so a runnable regression case isn't possible until those separate issues are fixed. No new regressions.
…ubjects (#28000) Addresses review feedback on #28011. - transform_cast_expr: type-specific cast paths return before the value-aware guard, so a value `match`/`if` operand of an optional-sum cast (`?Shape(match ...)`), interface cast (`Animal(match ...)`) or pointer-to-sum cast (`&Shape(match ...)`) was lowered by its helper (`wrap_sum_value` / interface boxing) with plain `transform_expr`, reproducing the empty propagating branch-tail expression. Materialize a value `match`/`if` cast operand into a value temp once, before cast-type dispatch, then re-dispatch over the rewritten operand so every path (optional/interface/pointer-sum/sum/ generic) sees a plain, typed operand. This subsumes the per-branch sum and generic guards added earlier, which are consolidated away. - transform_is_expr: the `is` subject was lowered with plain `transform_expr` (sum.v:773), so `(match n { ... make_circle()! ... }) is Circle` reproduced the same empty expression. Route the subject through value-aware lowering before building the tag check. `transform_value_operand` is a no-op for the common non-branch operands.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 800519e77d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new_rhs := if rhs_is_value_branch { | ||
| t.transform_value_operand(infix_rhs_id) |
There was a problem hiding this comment.
When only the RHS is a value match/if, this call emits its materialization statements before the specialized handler transforms the untouched LHS. Thus a specialized expression such as mark('L') + (match x { ... mark_result('R')! ... }) runs the RHS prelude before the LHS call, reversing observable evaluation order. Materialize operands while preserving the existing LHS-before-RHS pending-statement ordering and add a regression case.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in d26ae44. The pre-dispatch guard now evaluates operands left-to-right with the same LHS-before-RHS pending-statement ordering as the fallthrough path: when one side is a value match/if, a non-stable (side-effecting) other operand is spilled to a temp before the branch's prelude is emitted, so its evaluation still precedes it. Stable operands (idents/literals) are left untouched so the re-dispatch transforms them exactly once.
Regression test added (select_value_infix_order): it evaluates tr.lhs() + (match value { ... }) where both sides append to an order trace, and encodes sum*100 + order[0]*10 + order[1] → 1112 (sum 11, order L=1 then R=2). A reversed evaluation would yield 1121. It fails on HEAD without this fix.
(The case had to go into the existing large codegen test rather than a standalone file — a small standalone program deterministically trips a separate pre-existing parallel late-scan crash in seed_generated_fn_body_context, while the larger file avoids it.) No new regressions in the review suites.
| // route a value `match`/`if` operand (e.g. `-(match x { ... })`) | ||
| // through its target type so its propagating arms are lowered as values. | ||
| t.transform_value_operand(child_id) |
There was a problem hiding this comment.
For an address expression such as &(match x { First { get_a()! } else { get_b()! } }), the .amp branch above reaches value := t.transform_expr(child_id) and returns before this new generic value-aware path. Consequently, propagating match/if tails are still lowered in statement context and can produce an empty generated expression. Apply value-aware lowering in the address-of path as well and cover it with a regression test.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in d26ae44. The .amp (address-of) branch now lowers its operand via transform_value_operand instead of plain transform_expr, so &(match ...) materializes a propagating branch tail as a value rather than in statement context.
Regression test added (select_value_addr): &(match value { First { boxed(lower_first(value)!) } ... }) then reads a field through the pointer → 1. It fails on HEAD without this fix. Note the test uses a struct-typed match because the v3 checker only permits & on a struct-valued match (cannot take the address of match value otherwise) — which is exactly the case that reaches this .amp path.
No new regressions in the review suites.
…ds (#28000) Addresses review feedback on #28011. - transform_infix_expr: when only one operand was a value `match`/`if`, the pre-dispatch guard materialized it (emitting its prelude to pending_stmts) before the other, untouched operand was evaluated — so `mark('L') + (match x { ... mark_result('R')! ... })` ran the RHS prelude before the LHS call, reversing observable evaluation order. Evaluate operands left-to-right with the same LHS-before-RHS pending ordering as the fallthrough, spilling a non-stable (side-effecting) non-branch operand to a temp so its evaluation precedes the branch prelude; stable operands are left untouched. - transform_prefix_expr: the `.amp` (address-of) branch lowered its operand with plain `transform_expr` and returned before the generic value-aware path, so `&(match ...)` lowered a propagating branch tail in statement context. Route it through value lowering. Regression tests (added to the existing codegen test, whose size avoids a separate pre-existing parallel late-scan crash): `select_value_infix_order` encodes the sum and the recorded L-before-R evaluation order (1112; a reversed order would be 1121), and `select_value_addr` takes the address of a struct-typed value match and reads a field through it. Both fail on HEAD without these fixes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d26ae44058
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new_lhs := t.transform_value_operand(lhs_id) | ||
| new_rhs := if rhs_target_type.len > 0 { | ||
| t.transform_expr_for_type(rhs_id, rhs_target_type) | ||
| } else { | ||
| t.transform_expr(rhs_id) | ||
| t.transform_value_operand(rhs_id) |
There was a problem hiding this comment.
When the RHS is a value match/if that produces propagation prelude statements and the LHS has side effects, new_lhs remains inline while transform_value_operand(rhs_id) queues the RHS materialization before the entire shift expression. Thus mark_lhs() << (match x { ... mark_rhs()! ... }) runs mark_rhs before mark_lhs, unlike the ordering-preserving path below; stabilize the LHS before materializing the RHS and cover this shift-specific case with a regression test.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in faa61a1. In the left-shift fast path, when the RHS is a value match/if (whose materialization queues prelude statements) and the LHS is non-stable, the LHS is now stabilized to a temp before the RHS is materialized, so it evaluates first — matching the ordering-preserving general infix path. This is scoped to numeric shifts (rhs_target_type empty); an array-append LHS is a mutated lvalue and is deliberately not spilled, and a value-branch LHS is already materialized in order by transform_value_operand.
Regression test select_value_shift_order: tr.shift_lhs() << (match node { ... }) where both sides record into an order trace, encoded as sum*100 + order[0]*10 + order[1] → 102412 (1<<10=1024, order L=1 then R=2). It fails on HEAD (produces 102421 — reversed). No new regressions; review_cgen (which exercises shifts/appends) passes.
| mut changed := false | ||
| for i in 0 .. node.children_count { | ||
| child_id := t.a.child(&node, i) | ||
| mut new_child := t.transform_expr(child_id) | ||
| // route a value `match`/`if` operand (e.g. `values[match x { ... }]`) |
There was a problem hiding this comment.
When an index or later slice bound is a value match/if with propagation and an earlier child has side effects, this queues the branch materialization in pending_stmts while leaving the earlier child inline. For example, make_values(mut tr)[match n { ... tr.index_result()! ... }] executes the index prelude before make_values; stabilize preceding children once a later child hoists statements, and add an ordering regression test.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in faa61a1. transform_index_expr now finds the last hoisting (value match/if) child up front, and stabilizes each preceding non-stable child to a temp before that child materializes — so a side-effecting base/earlier bound evaluates before the hoisted prelude. This handles the base and any earlier slice bound. Only rvalue reads (.index) reach here; lvalue index targets are the separate .index_assign kind, so spilling the base is safe.
Regression test select_value_index_order: tr.base_values()[match node { ... }] records order into a trace, encoded as val*100 + order[0]*10 + order[1] → 1012 ([10,20,30][0]=10, order base=1 then index=2). It fails on HEAD (produces 1021 — reversed). No new regressions; review_cgen (heavy index coverage) passes.
…nds (#28000) Addresses review feedback on #28011. - Left-shift fast path: when the RHS is a value `match`/`if` whose materialization queues propagation prelude statements and the LHS has side effects, the LHS was left inline and thus evaluated after the RHS prelude — `mark_lhs() << (match x { ... mark_rhs()! ... })` ran mark_rhs before mark_lhs. Stabilize a non-stable LHS before materializing the RHS (numeric shift only; an array-append LHS is a mutated lvalue and must not be spilled). - Index/slice operands: when a later child (index or slice bound) is a value `match`/`if` that hoists statements while an earlier child has side effects, the earlier child ran after the hoisted prelude — `make_values(mut tr)[match n { ... tr.index_result()! ... }]` ran the index prelude before make_values. Stabilize preceding non-stable children once a later child hoists. Only rvalue reads (`.index`) reach here; lvalue targets are the separate `.index_assign` kind. Regression tests `select_value_shift_order` (102412 vs reversed 102421) and `select_value_index_order` (1012 vs reversed 1021) encode both the computed value and the recorded L-before-R order; both fail on HEAD without these fixes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faa61a1bd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mut last_value_branch := -1 | ||
| for i in 0 .. node.children_count { | ||
| if t.is_value_match_or_if_operand(t.a.child(&node, i)) { |
There was a problem hiding this comment.
For a gated index such as values#[match node { First { get_index()! } else { other_index()! } }], this new value-branch scan is never reached because transform_index_expr returns through lower_gated_scalar_index first; that helper still lowers the index with stable_expr_for_reuse, which calls plain transform_expr, so the propagating match tail can again produce an empty generated expression. Fresh evidence beyond the earlier index-ordering comment is the final tree's gated-index early return and unchanged plain lowering at lower_gated_scalar_index; route that index through value-aware lowering and add a gated-index regression case.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 319d914. You're right — transform_index_expr returns through lower_gated_scalar_index before the value-branch scan, and that helper lowered both operands with stable_expr_for_reuse (plain transform_expr), so a gated values#[match n { ... get_index()! ... }] lowered the propagating tail in statement context. The base/index are now routed through transform_value_operand when they're value match/if operands (which materializes them into a value temp — already stable for the helper's multiple uses); non-branch operands keep stable_expr_for_reuse. The base is still evaluated before the index, so base-before-index order is preserved.
Regression test select_value_gated_index_order: a gated negative index tr.base_values()#[match node { ... }] (→ [10,20,30]#[-1] = 30) driven by a propagating match, encoded with the order trace as 3012. Without the fix the generated C fails to compile with exactly the empty-expression symptom (error: expected expression / use of undeclared identifier '__in_lhs_0'). No new regressions; review_cgen passes.
Addresses review feedback on #28011. `transform_index_expr` returns through `lower_gated_scalar_index` before the value-branch scan, and that helper lowered its base and index with `stable_expr_for_reuse` (plain `transform_expr`), so a gated index such as `values#[match n { First { get_index()! } else { other_index()! } }]` lowered the propagating match tail in a value-less statement context and emitted an empty generated expression. Route a value `match`/`if` base/index through `transform_value_operand`, which materializes it into a value temp (already stable for the helper's multiple uses); non-branch operands keep `stable_expr_for_reuse`. The base is still evaluated before the index, preserving base-before-index order. Regression test `select_value_gated_index_order` exercises a gated negative index (`[10,20,30]#[-1]` = 30) driven by a propagating match, encoding value and order as 3012. Without the fix the generated C fails to compile (empty expression).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 319d914eb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn (mut t Transformer) transform_value_operand(id flat.NodeId) flat.NodeId { | ||
| if t.is_value_match_or_if_operand(id) { | ||
| mut typ := t.node_type(id) | ||
| if typ.len == 0 { | ||
| typ = t.resolve_expr_type(id) | ||
| } | ||
| if typ.len > 0 && typ != 'void' { | ||
| return t.transform_expr_for_type(id, typ) |
There was a problem hiding this comment.
Range bounds still bypass this value-aware helper: lower_range_for_in sends both bounds through stable_expr_for_reuse, which calls plain transform_expr, and the range branch of transform_in_expr also calls plain transform_expr for low_id/high_id. Consequently, a bound such as for i in (match node { First { lower_first(node)! } Second { lower_second(node)! } }) .. 10 still lowers the propagating arm tail in statement context and can emit the same empty expression this change fixes elsewhere. Route range bounds through value-aware lowering and add a regression case.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 20f00b3. Both range paths now route value match/if bounds through transform_value_operand:
lower_range_for_in(for i in low..high): each bound is materialized into a value temp when it's a value branch (stable for reuse in the loop condition); non-branch bounds keepstable_expr_for_reuse. Low is still evaluated before high.transform_in_expr's range branch (x in low..high): both bounds go throughtransform_value_operand, and the tested value too (it's compared twice, so a value-branch subject is materialized into a temp; non-branch keepsstable_expr_for_reuse).
Regression tests: select_value_range_low — for i in (match node { First { lower_first(node)! } ... }) .. 4 → sum 6; and select_value_range_membership — 3 in (match node { ... }) .. 4 → true. Both fail to compile on HEAD without these fixes (the same error: expected expression empty-expression symptom). No new regressions; review_cgen (for-in / membership coverage) passes.
Addresses review feedback on #28011. Range bounds bypassed the value-aware helper: `lower_range_for_in` sent both bounds through `stable_expr_for_reuse` and `transform_in_expr`'s range branch lowered `low_id`/`high_id` with plain `transform_expr`. So a bound such as `for i in (match node { First { lower_first(node)! } Second { lower_second(node)! } }) .. 10` lowered the propagating arm tail in a value-less statement context and emitted an empty expression. Route value `match`/`if` range bounds (and, in the membership form, the tested value) through `transform_value_operand`, which materializes them into value temps (stable for the loop condition / two comparisons); non-branch operands keep `stable_expr_for_reuse`. The low bound is still evaluated before the high bound. Regression tests: `select_value_range_low` (`for i in (match ...) .. 4`, sum 6) and `select_value_range_membership` (`3 in (match ...) .. 4`, true). Both fail to compile on HEAD (empty expression) without these fixes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20f00b3b38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new_low := t.transform_value_operand(low_id) | ||
| new_high := t.transform_value_operand(high_id) |
There was a problem hiding this comment.
When the high bound is a value match/if and the low bound has side effects, transform_value_operand(high_id) queues the high-bound materialization in pending_stmts while new_low remains inline in the generated comparison. Thus x in low_with_effect()..(match node { ... high_with_effect()! ... }) evaluates the high-bound prelude before low_with_effect(), reversing source order. Stabilize the low bound before materializing a hoisting high bound and add an ordering regression case.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 773d820. In transform_in_expr's x in low..high branch, a non-branch side-effecting low bound is now stabilized to a temp before a hoisting (value match/if) high bound is materialized, so it evaluates first — preserving low-before-high order. A value-branch low is already materialized in order by transform_value_operand, and non-hoisting high bounds are left untouched. (The for in range path already used stable_expr_for_reuse for non-branch bounds, so it wasn't affected.)
Regression test select_value_range_order: 5 in tr.range_low() .. (match node { ... }) where both sides append to an order trace, encoded as flag*100 + order[0]*10 + order[1] → 112 (5 in 0..10 true, order low=1 then high=2). It fails on HEAD (produces 121 — reversed). No new regressions; review_cgen passes.
#28000) Addresses review feedback on #28011. In `transform_in_expr`'s `x in low..high` branch, both bounds went through `transform_value_operand`, which leaves a non-branch side-effecting low bound inline. When the high bound is a value `match`/`if`, its materialization queues prelude statements, so `x in low_with_effect() .. (match node { ... high_with_effect()! ... })` ran the high-bound prelude before the low bound, reversing source order. Stabilize a side-effecting low bound to a temp before materializing a hoisting high bound (a value-branch low is already materialized in order by `transform_value_operand`; the `for in` range path already used `stable_expr_for_reuse` and was unaffected). Regression test `select_value_range_order`: `5 in tr.range_low() .. (match node { ... })` records order into a trace and encodes membership + order as 112 (a reversed order would be 121). It fails on HEAD without the fix.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 773d8208d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new_lhs := if t.is_value_match_or_if_operand(lhs_id) { | ||
| t.transform_value_operand(lhs_id) | ||
| } else { | ||
| t.stable_expr_for_reuse(lhs_id) |
There was a problem hiding this comment.
When the RHS container is itself a value match/if, such as needle in (match node { First { get_values(node)! } else { get_other_values(node)! } }), this routing is skipped because it only runs in the .range branch. The dynamic/fixed-array and string paths still use plain transform_expr, while stable_array_expr_for_membership does the same for semantic array membership, so the propagating arm tail is again lowered in statement context and can emit an empty expression. Route these container operands through value-aware lowering and add a regression case.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in b7bb6a1. All non-range membership container lowerings now go through transform_value_operand:
- The inline branches in
transform_in_expr(dynamic array, fixed array, string, unknown-ident, and the fallbackin_expr) that lowered the RHS container with plaintransform_expr. stable_array_expr_for_membership(semantic array membership), andlower_map_membership_expr(map membership) — both used plaintransform_expr/stable_expr_for_reuseon the container.
transform_value_operand is a no-op for the common non-branch containers (idents/selectors/calls). (lower_const_string_array_membership_expr is unchanged — it only matches a const string-array literal base, never a match.)
Regression test select_value_membership_container: 20 in (match node { First { make_values_first(node)! } ... }) → true. It fails to compile on HEAD without the fix (empty expression). No new regressions; review_cgen (membership coverage) passes.
| low := if t.is_value_match_or_if_operand(low_id) { | ||
| t.transform_value_operand(low_id) | ||
| } else { | ||
| t.stable_expr_for_reuse(low_id) | ||
| } | ||
| high := if t.is_value_match_or_if_operand(high_id) { | ||
| t.transform_value_operand(high_id) | ||
| } else { | ||
| t.stable_expr_for_reuse(high_id) |
There was a problem hiding this comment.
This only fixes range bounds; a loop such as for value in (match node { First { get_values(node)! } else { get_other_values(node)! } }) dispatches to lower_indexed_for_in, where the container is still lowered by stable_expr_for_reuse, and iterator/map paths likewise use plain transform_expr. A propagating match arm therefore retains the same empty-expression failure when the match produces the iterable rather than a range bound. Apply value-aware lowering to for-in containers and cover the case with a regression test.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in b7bb6a1. All for-in container lowerings now route a value match/if container through transform_value_operand, which materializes it into a value temp (stable for the loop's repeated use); non-branch containers keep the existing lowering:
lower_indexed_for_in(wasstable_expr_for_reuse).lower_iterator_for_in(was plaintransform_expr).- the map / rebuild path in
transform_for_in_body'srebuild_for_in_stmt(wasstable_expr_for_reuse/transform_expr).
Regression test select_value_forin_container: for v in (match node { First { make_values_first(node)! } ... }) summed → 60. It fails to compile on HEAD without the fix (empty expression). No new regressions; review_cgen (for-in coverage) passes.
…wering (#28000) Addresses review feedback on #28011. The value-aware routing only covered range bounds, so a value `match`/`if` used as the *container* still lowered its propagating arm tail in a value-less statement context (empty expression): - Membership (`transform_in_expr`): the dynamic-array, fixed-array, string, unknown-ident and fallback branches lowered the RHS container with plain `transform_expr`, and `stable_array_expr_for_membership` (semantic array membership) and `lower_map_membership_expr` did the same. Route the container through `transform_value_operand`. - for-in (`transform_for_in_body` / `lower_indexed_for_in` / `lower_iterator_for_in`): the indexed, iterator and map/rebuild paths lowered the container with `stable_expr_for_reuse` / `transform_expr`. Materialize a value-branch container into a value temp (stable for the loop's repeated use); non-branch containers keep the existing lowering. Regression tests: `select_value_membership_container` (`20 in (match node { ... make_values(node)! ... })` -> true) and `select_value_forin_container` (`for v in (match ...)` summed -> 60). Both fail to compile on HEAD (empty expression) without these fixes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7bb6a16a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mut last_value_branch := -1 | ||
| for i in 0 .. node.children_count { | ||
| if t.is_value_match_or_if_operand(t.a.child(&node, i)) { |
There was a problem hiding this comment.
When a map read has a value match/if as its base, transform_index_expr returns through try_lower_map_index_expr before reaching this new scan; that helper still lowers the base with stable_expr_for_reuse, which calls plain transform_expr. Thus (match n { First { make_map_first(n)! } else { make_map_second(n)! } })['key'] still lowers the propagating arm in statement context and can generate an empty expression. Route map-index bases through value-aware lowering and add a regression case.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in f378044. try_lower_map_index_expr now routes a value match/if map base through transform_value_operand (which materializes it into a value temp — stable for the helper's repeated use); non-branch bases keep stable_expr_for_reuse. So (match n { First { make_map_first(n)! } ... })['key'] materializes the propagating arm as a value instead of in statement context.
Regression test select_value_map_index: (match node { First { make_map_first(node)! } ... })["b"] → 2. It fails to compile on HEAD without the fix (error: expected expression). No new regressions; review_cgen passes.
| } else if clean_rhs_type == 'string' { | ||
| new_lhs := t.transform_expr(lhs_id) | ||
| new_rhs := t.transform_expr(rhs_id) | ||
| new_rhs := t.transform_value_operand(rhs_id) |
There was a problem hiding this comment.
When a string membership container is a value match/if and the needle has side effects, transform_value_operand(rhs_id) hoists the container's propagation prelude into pending_stmts, while new_lhs remains inline in the final helper call. For example, tr.needle() in (match n { First { tr.text_first(n)! } else { tr.text_second(n)! } }) executes the RHS text function before needle, reversing source order. Stabilize the LHS before materializing such a RHS and add an ordering regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in f378044. In the inline membership branches (dynamic-array, fixed-array, string, and the unknown fallback), a side-effecting needle is now stabilized to a temp before a value-branch container is materialized, so it evaluates first — preserving needle-before-container source order. For the dynamic-array branch (which transformed the container first) the needle is now evaluated before the container; the others simply gate new_lhs on is_value_match_or_if_operand(rhs_id). The semantic-array path (lower_array_membership_expr, receiver_first=false) already stabilized the needle before the container with drain_pending between, so it was unaffected.
Regression test select_value_string_membership_order: tr.needle_str() in (match node { First { tr.text_first(node)! } ... }) where both sides record into an order trace, encoded as 500 + order[0]*10 + order[1] → 512 ("lo" in "hello" true, order needle=1 then container=2). It fails on HEAD (produces 521 — reversed). No new regressions; review_cgen passes.
…ership (#28000) Addresses review feedback on #28011. - Map index base (try_lower_map_index_expr): a value `match`/`if` map base returned through this helper before the value-branch scan and was lowered with `stable_expr_for_reuse` (plain transform_expr), so `(match n { First { make_map_first(n)! } ... })['key']` lowered the propagating arm in statement context (empty expression). Route it through `transform_value_operand`. - Membership needle order (transform_in_expr): in the inline dynamic-array, fixed-array, string and unknown branches the needle stayed inline while `transform_value_operand(rhs_id)` hoisted a value-branch container's prelude, so `tr.needle() in (match n { ... tr.text(n)! ... })` ran the container before the needle. Stabilize a side-effecting needle before materializing a value-branch container (the semantic-array path via lower_array_membership_expr already stabilized the needle first and was unaffected). Regression tests: select_value_map_index (`(match ...)['b']` -> 2, fails to compile on HEAD) and select_value_string_membership_order (`tr.needle_str() in (match ...)` -> 512, reversed 521 on HEAD).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3780447fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new_lhs := if rhs_target_type.len == 0 && rhs_is_value_branch | ||
| && !t.is_value_match_or_if_operand(lhs_id) && !t.is_stable_expr_for_reuse(lhs_id) { | ||
| t.stable_expr_for_reuse(lhs_id) |
There was a problem hiding this comment.
When << is an array append, rhs_target_type is nonempty, so this guard deliberately leaves a side-effecting LHS inline while transform_expr_for_type(rhs_id, ...) queues the match prelude. For a valid append such as arrays[next(mut trace)] << (match node { First { make_value(node)! } else { other_value(node)! } }), the RHS prelude therefore runs before next(), reversing source evaluation order. Fresh evidence beyond the earlier numeric-shift thread is the final rhs_target_type.len == 0 condition, which explicitly excludes this array-append path; stabilize the LHS address/index without spilling the mutated array value and add an ordering regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in b345126. For an append whose RHS is a value match/if, the LHS lvalue's dynamic base/index components are now stabilized into temps first — via stabilize_transformed_lvalue_for_reuse, which preserves the lvalue shape (indexed appends still reach their normal array lowering) and does not spill the mutated array value — so a side-effecting index runs before the RHS prelude. Gated on a value-branch RHS, so normal appends are unchanged.
One clarification: a statement append like arrays[next(mut trace)] << (match …) doesn't actually flow through the transform_infix_expr left-shift block at transform.v:14666 — it's intercepted earlier by try_lower_array_append_stmt, which transformed the LHS with transform_lvalue (leaving the index inline) and drained pending before materializing the RHS. So I applied the stabilization there (the real path), and also in the left-shift block you cited for any append that reaches it.
Regression test select_value_append_order: arrays[tr.next_index()] << (match node { First { tr.append_val_first(node)! } … }) records order into a trace → 712 (order index=1 then match=2). It fails on HEAD (produces 721 — the match prelude ran before next_index()). No new regressions; review_cgen (append coverage) passes.
| new_lhs := if t.is_value_match_or_if_operand(rhs_id) { | ||
| t.stable_transformed_expr_for_reuse(t.transform_expr_for_type(lhs_id, elem), | ||
| elem, 'in_lhs') | ||
| } else { | ||
| t.transform_expr_for_type(lhs_id, elem) |
There was a problem hiding this comment.
This value-aware needle lowering is never reached for membership in a constant string array because lower_const_string_array_membership_expr returns earlier at line 1936, and that helper still lowers its needle with plain transform_expr at line 2113. Consequently, (match node { First { get_first(node)! } else { get_second(node)! } }) in allowed_words still lowers propagating branch tails in statement context when allowed_words is a constant string array and can emit the empty expression this change is intended to eliminate; route the needle through typed value lowering in the shortcut and add a regression case.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in b345126. lower_const_string_array_membership_expr now lowers its needle with transform_expr_for_type(needle_id, 'string') (typed value lowering — the container is a string array), so a value match/if needle materializes its propagating arm as a value instead of in a value-less statement context. This runs before the value-aware needle path in transform_in_expr, exactly as you noted.
Regression test select_value_const_membership: (match node { First { get_first(node)! } … }) in allowed_words (a const []string) → true. It fails to compile on HEAD without the fix (error: expected expression). No new regressions; review_cgen passes.
…st-array membership (#28000) Addresses review feedback on #28011. - Array-append LHS order: for an append whose RHS is a value `match`/`if` that hoists a prelude, the LHS lvalue's dynamic base/index components are now stabilized into temps first (via stabilize_transformed_lvalue_for_reuse, which preserves the lvalue shape and does not spill the mutated array value), so a side-effecting index runs before the RHS prelude — `arrays[next(mut trace)] << (match node { First { make_value(node)! } ... })`. A statement append is lowered by `try_lower_array_append_stmt` (where `transform_lvalue` left the index inline); the guard is also applied in the `transform_infix_expr` left-shift path the review cited, for any append reaching it. Gated on a value-branch RHS, so normal appends are unchanged. - Const-string-array membership needle: `lower_const_string_array_membership_expr` runs before the value-aware needle path and lowered its needle with plain `transform_expr`, so `(match node { First { get_first(node)! } ... }) in allowed_words` lowered the propagating arm in statement context (empty expression). Route the needle through `transform_expr_for_type(.., 'string')`. Regression tests: select_value_append_order (`arrays[tr.next_index()] << (match ...)` -> 712, reversed 721 on HEAD) and select_value_const_membership (`(match ...) in allowed_words` -> true, fails to compile on HEAD).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b345126510
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| map_expr := if t.is_value_match_or_if_operand(map_source_id) { | ||
| t.transform_value_operand(map_source_id) |
There was a problem hiding this comment.
When the RHS map is a value match/if and the LHS key has side effects, this map-specific path materializes the map before line 300 transforms the key, so tr.key() in (match node { First { tr.map_first(node)! } else { tr.map_second(node)! } }) runs the RHS propagation prelude before key(), reversing source evaluation order. Unlike the fixed inline array/string membership paths, this helper needs to stabilize the typed key before materializing the container; add a map-membership ordering regression as well.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 98d026f. lower_map_membership_expr now spills the typed key to its temp before materializing the container, so a side-effecting key evaluates before a value-branch map's hoisted propagation prelude — preserving key in map source order. The key and container are independent, so the reorder is safe, and non-branch maps are unaffected.
Regression test select_value_map_membership_order: tr.map_key() in (match node { First { tr.map_first(node)! } … }) where both sides record into an order trace, encoded as 600 + order[0]*10 + order[1] → 612 ("b" in {a:1, b:2} true, order key=1 then container=2). It fails on HEAD (produces 621 — reversed). No new regressions; review_cgen passes.
Addresses review feedback on #28011. In lower_map_membership_expr (`key in map`), the container (map_expr) was materialized before the key was spilled, so when the RHS map is a value `match`/`if`, its propagation prelude ran before a side-effecting key — `tr.key() in (match node { First { tr.map_first(node)! } ... })` evaluated the match arm before key(), reversing source order. Spill the typed key to its temp first, then materialize the container, so the key evaluates before a value-branch map's hoisted prelude (key and container are independent, so the reorder is safe; non-branch maps are unaffected). Regression test select_value_map_membership_order: `tr.map_key() in (match node { First { tr.map_first(node)! } ... })` records order into a trace -> 612 (order key=1 then container=2). It fails on HEAD (produces 621 - reversed).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98d026f508
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if t.is_value_match_or_if_operand(rhs_id) { | ||
| lhs = t.stabilize_transformed_lvalue_for_reuse(lhs) |
There was a problem hiding this comment.
When the RHS match produces an array, such as out << (match node { First { values_first(node)! } else { values_second(node)! } }), array_append_rhs_is_push_many selects the push_many branch. This guard stabilizes the LHS, but that branch still lowers rhs_id with plain transform_expr at line 1207, so the propagating match-arm tail remains in statement context and can emit an empty expression; route push-many RHS matches through value-aware lowering and cover this array-producing append case.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in fd5233b. The push-many branch now lowers its RHS with transform_value_operand, so an array-producing value match/if materializes its propagating arm as a value instead of in a value-less statement context; it's a no-op for the common non-branch push-many operands. Applied in both try_lower_array_append_stmt and the optional variant try_lower_optional_array_append_stmt, which had the identical push-many branch.
Regression test select_value_push_many: out << (match node { First { make_values_first(node)! } … }) appends [10,20,30] to [1] → sum 61. It fails to compile on HEAD without the fix (error: expected expression). No new regressions; review_cgen (append coverage) passes.
Addresses review feedback on #28011. When an array-append RHS is an array-producing value `match`/`if`, `array_append_rhs_is_push_many` selects the push-many branch, which lowered `rhs_id` with plain `transform_expr` — so the propagating arm tail stayed in a value-less statement context and emitted an empty expression, e.g. `out << (match node { First { values_first(node)! } else { values_second(node)! } })`. Route the push-many RHS through `transform_value_operand` (a no-op for the common non-branch operands) in both `try_lower_array_append_stmt` and `try_lower_optional_array_append_stmt`. Regression test select_value_push_many: `out << (match node { First { make_values_first(node)! } ... })` appends [10,20,30] to [1] -> sum 61. It fails to compile on HEAD (empty expression) without the fix.
…nel ordering (#28000) Addresses review feedback on #28011. Five more operand-ordering guards still used is_value_match_or_if_operand, which only looks through paren/unsafe/block wrappers and stops at the outer expression, so a value branch buried inside a compound operand was missed while lowering it still hoisted the branch prelude into pending_stmts — reversing evaluation order against a preceding inline side effect. Switch each to the recursive operand_hoists_value_branch (added in f021bb9): - transform.v general infix (both operands): `trace_left() + (1 + (match ...))` left the block unentered, so trace_left stayed inline and the RHS prelude ran first. - transform.v numeric shift: `trace_left() << (1 + (match ...))` left rhs_is_value_branch false, so the side-effecting LHS was not stabilized. - transform.v channel send: `channels[next()] <- wrap(match ...) or {}` left the target index inline, so the sent value ran before next(). - expr.v range high bound: `x in trace_low() .. (1 + (match ...))` left trace_low inline, so trace_high ran first. - expr.v string membership container: `trace_needle() in wrap(match ...)` left the needle inline, so the container prelude ran first. Regression tests (each fails with the reversed value when its predicate is reverted): select_value_nested_infix_order 1212/1221, select_value_nested_shift_order 204812/204821, select_value_nested_range_order 112/121, select_value_nested_string_membership_order 512/521, select_value_nested_channel_target_order 9912/9921.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97e69f376d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| r := if stabilized := t.stabilize_original_lvalue_receiver(recv_id) { | ||
| stabilized | ||
| } else { | ||
| t.stable_expr_for_reuse(recv_id) |
There was a problem hiding this comment.
When a non-mut value-receiver method is called on an lvalue and a later argument hoists a value branch, this path preserves the receiver lvalue rather than its current value. For example, in items[next()].read(match node { ... mutate_items(mut items)! ... }), only the index is spilled; the branch prelude can mutate items[0] before the redispatched call finally loads the receiver, so read observes the new value instead of the value evaluated in source order. Reserve lvalue-preserving stabilization for mut/reference receivers and spill ordinary value receivers by value, with a propagation regression test.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 4743d9e. Added method_receiver_is_reference (mirroring the existing method_value_has_pointer_receiver: it resolves the method name from the receiver's type and checks fn_param_types[method][0] is types.Pointer, falling back to mut_receiver_methods). The receiver-ordering path now reserves lvalue-preserving stabilization (spilling only the dynamic base/index components, re-dispatching the original lvalue) for mut/reference receivers, which must mutate through. An ordinary by-value receiver is spilled by value via stable_expr_for_reuse, so its value is read in source order — a later branch prelude that mutates the container can no longer change the observed receiver value.
Regression test select_value_value_receiver: vh.items[vh.at()].read(match node { First { vh.overwrite_first(node)! } … }) where read is a value receiver and the arm sets vh.items[0] = ValItem{9}. First → ValItem{5}.read(2) = 5002. On HEAD the receiver is reloaded after the mutation → 9002. The existing mut-receiver regression (select_value_mut_receiver → 4512) still passes, confirming reference receivers keep lvalue identity. No new regressions; review_cgen passes.
| if t.operand_hoists_value_branch(sent_value_id) { | ||
| lhs = t.stabilize_transformed_lvalue_for_reuse(lhs) | ||
| } |
There was a problem hiding this comment.
When the target of a <- … or {} send is a side-effecting rvalue such as get_channel(mut trace), stabilize_transformed_lvalue_for_reuse returns the call unchanged because it only handles lvalue shapes. A sent value containing a propagating match then queues its prelude before the final send, causing the RHS effects to run before get_channel() and reversing source order. Fresh evidence beyond the resolved indexed-target thread is this function-call target path; spill non-lvalue channel targets by value before transforming the sent branch and add a regression test.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 4743d9e. stabilize_transformed_lvalue_for_reuse only rewrites lvalue shapes (ident/selector/index/deref/paren) and returns anything else — e.g. a get_channel(mut trace) call — unchanged (same node id). The send path now detects that: if the stabilized result is a new node (an lvalue whose dynamic components were spilled) it uses it as before; otherwise, if the target is a non-stable rvalue, it is spilled by value via stable_transformed_expr_for_reuse (a channel is a reference handle, so spilling the handle preserves identity). Either way the temp lands in the send prelude before the sent value's materialization, and the existing prelude-detachment keeps it out of the or {} handler.
Regression test select_value_rvalue_channel_target: c.get_channel() <- (match node { First { c.sent_first(node)! } … }) or { return -1 } where get_channel() records order first → send 77, order [1,2] → 7712. On HEAD the target call runs after the sent-value prelude → 7721. No new regressions; review_cgen passes.
… branch operands (#28000) Addresses review feedback on #28011. Both fixes distinguish reference (lvalue-preserving) from by-value (spill-by-value) stabilization when an operand later hoists a value branch: - Method receiver: when a non-mut *value*-receiver method is called on an lvalue and a later argument hoists a value branch, the receiver path unconditionally used lvalue-preserving stabilization (spilling only the dynamic base/index components) and re-dispatched the original lvalue, so the receiver value was loaded at the call — after a branch prelude that mutates the container, so `items[next()].read(match ... { mutate(mut items)! } ...)` observed the mutated value. Add method_receiver_is_reference (mirroring method_value_has_pointer_receiver): reserve lvalue-preserving stabilization for mut/reference receivers (they must mutate through); spill ordinary value receivers by value so the value is read in source order. - Channel target: stabilize_transformed_lvalue_for_reuse only rewrites lvalue shapes and returns a non-lvalue rvalue unchanged, so a side-effecting rvalue target such as `get_channel(mut trace) <- (match ...) or {}` stayed inline and ran after the sent value prelude. Detect the unchanged (non-lvalue) result and spill it by value with stable_transformed_expr_for_reuse, so the target evaluates before the sent branch. Regression tests: select_value_value_receiver (`vh.items[vh.at()].read(match ... { overwrite(mut vh.items)! } ...)` -> 5002; leaked-mutation 9002 on HEAD) and select_value_rvalue_channel_target (`c.get_channel() <- (match ...) or {}` -> 7712; reversed 7721 on HEAD).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4743d9e7de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lhs = if stabilized != lhs { | ||
| stabilized | ||
| } else if !t.is_stable_expr_for_reuse(lhs) { | ||
| t.stable_transformed_expr_for_reuse(lhs, t.node_type(lhs), 'chan_target') |
There was a problem hiding this comment.
When the channel target is an index or selector rooted in a side-effecting rvalue, such as make_channels(mut trace)[0] <- wrap(match node { ... sent(node)! ... }) or {}, the stabilizer rebuilds the outer index/selector but leaves the call base inline; because the rebuilt ID differs from lhs, this branch treats the target as stabilized and skips spilling it. The sent-value prelude therefore runs before make_channels(), reversing source evaluation order. Fresh evidence beyond the resolved direct-call-target comment is that composite rvalue targets bypass the fallback solely because rebuilding their unchanged lvalue shape produces a new node ID; detect whether a component was actually spilled or spill the whole target value, and add a regression for this shape.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — the stabilized != lhs id comparison was exactly the wrong signal. Fixed in 40591bf.
stabilize_transformed_lvalue_for_reuse spills index/selector components but recurses into the index/selector base and returns a non-lvalue base (a call) unchanged, then rebuilds the outer index into a new node id. So for make_channels(mut trace)[0] the id differed from lhs even though nothing about the side-effecting base was spilled, and the old check took the "already stabilized" branch with the call still inline.
The decision now keys on the actual root, not the id: transformed_lvalue_root_needs_value_spill walks the lvalue spine (index/selector/paren base; a deref operand is spilled as a component, so it is safe) down to the root leaf. If that leaf is a non-lvalue rvalue that is not stable, the whole target is spilled by value via stable_transformed_expr_for_reuse; otherwise a stable-rooted lvalue (channels[next()]) keeps just its dynamic components spilled. This subsumes the earlier bare-call case (get_channel() <- ...) and adds the composite one. It also decides before stabilizing, so no orphan component temps get pushed when we then spill by value.
Regression test select_value_composite_rvalue_channel_target: f.make_channels()[0] <- (match node { First { f.fsent_first(node)! } … }) or { return -1 }, where make_channels() records order first → send 55, order [1,2] → 5512. With the old id-comparison heuristic it produces the reversed 5521 (verified), while the bare-call case still gave 7712 under both — matching your observation that only composite rvalue-rooted targets slipped through. No new regressions; review_cgen passes.
…e sent value (#28000) Addresses review feedback on #28011. The prior channel-target fix decided between lvalue-preserving stabilization and a by-value spill by comparing the stabilized node id to the original (stabilized != lhs). That is fooled by a composite lvalue rooted in a side-effecting rvalue such as make_channels(mut trace)[0]: stabilize_transformed_lvalue_for_reuse spills the index components but recurses into the index *base* and returns the non-lvalue call base unchanged, then rebuilds the outer index into a new node id. The new id != lhs, so the old check treated the target as stabilized and used it with the call still inline — the sent value prelude then ran before make_channels(), reversing source order. Decide on the actual root instead: transformed_lvalue_root_needs_value_spill walks the lvalue spine (index/selector/paren base, deref operand is spilled as a component so it is safe) to the root leaf. If the leaf is a non-lvalue rvalue that is not stable, the whole target is spilled by value; otherwise the stable-rooted lvalue keeps only its dynamic components spilled. This subsumes the bare-call case (get_channel() <- ...) and adds the composite case. Deciding before stabilizing also avoids pushing orphan component temps. Regression test select_value_composite_rvalue_channel_target (`make_channels()[0] <- (match ...) or {}` -> 5512; reversed 5521 with the old id-comparison heuristic).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40591bf6d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if rhs_is_value_branch && !t.is_stable_expr_for_reuse(infix_lhs_id) { | ||
| t.stable_expr_for_reuse(infix_lhs_id) |
There was a problem hiding this comment.
When the RHS branch prelude can mutate a syntactically stable LHS, this condition leaves an identifier, selector, or stable index inline because is_stable_expr_for_reuse returns true. For example, in mut x := 1; y := x + (match node { First { change(mut x)! } else { 0 } }), the hoisted match prelude changes x before the final infix expression reads it, so y observes the updated value instead of the source-order value. Snapshot value-bearing LHS lvalues before hoisting the RHS; the same reuse-stability assumption should also be audited in the new call, append, range, and initializer ordering guards, with a propagation regression added.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 49fa317. The ordering guards spilled a preceding operand only when !is_stable_expr_for_reuse, which treats an ident/selector/stable-index as "safe to leave inline" — but that assumption holds only if nothing mutates in between, and a later value-branch operand's hoisted prelude can mutate exactly that storage.
Added is_pure_constant_expr (stricter than is_stable_expr_for_reuse: it returns false for value-bearing lvalue reads — idents, selectors, indexes — and true only for literals/enum/sizeof/typeof and casts/structs made purely of those) plus snapshot_*_for_reuse, and routed the value-read ordering guards through them: infix LHS, numeric-shift LHS, call receiver and preceding args, index children, range tested-value and low bound, the string/fixed/dynamic/unknown membership needle, and array-init len/cap. A pure constant is still left inline for the re-dispatch to transform once.
Audit of the other guards you flagged: append and channel targets are lvalue/reference mutation targets, not value reads (we resolve/mutate through them rather than snapshot a value), so they are intentionally unchanged. The mut-receiver/arg lvalue-component spilling keeps its existing is_stable check (it preserves an lvalue's identity; that's a distinct, more obscure sub-case).
One subtlety worth flagging: the call/infix/index guards re-dispatch through the same transform for a nested-branch operand (f(x, 1 + (match ...))). A snapshot temp is a non-pure ident, so it would be re-snapshotted on every re-dispatch pass and recurse forever. I record snapshot temp names in ordering_snapshot_names and exclude them via operand_needs_ordering_snapshot, so the re-dispatch terminates.
Regression tests (both verified load-bearing): select_value_stable_lhs_snapshot — c.v + (match node { First { c.bump()! } … }) where the arm sets c.v = 100 → 6100 (leaked mutation 105100 on HEAD); and select_value_stable_arg_snapshot — take2(c.v, match … { c.bump()! }) → 1005 (leaked 100005 on HEAD).
Verification: the 60+-scenario codegen suite passes; the four review regression suites match their prior baseline (review_cgen green); and I confirmed on a full vlib/v3/tests run that the change adds no new failures (the failing set is the v3 backend's pre-existing one — spot-checked several, e.g. mixed_lock/optional_struct_lvalue/union_byte_contains, all fail identically on HEAD).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40591bf6d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // compound expression (`1 + (match ...)`, `i64(match ...)`): lowering it still | ||
| // materializes the inner branch into pending_stmts, so an earlier operand must be | ||
| // stabilized to keep source order. | ||
| mut last_branch := if is_method && t.is_value_match_or_if_operand(recv_id) { 0 } else { -1 } |
There was a problem hiding this comment.
When the call target itself is a value match/if, such as (match node { First { make_cb_first(node)! } else { make_cb_second(node)! } })(), is_method is false and this scan considers only children 1.., so the callee is never materialized. transform_call_args later lowers child 0 with plain transform_expr, leaving the propagating branch tail in statement context and potentially emitting an empty callee expression. Include the non-selector callee as operand position 0 and add a regression for an immediately invoked function-valued branch.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aabe76a. You're right — the scan set last_branch only for a method receiver (child 0 when is_method) and arguments (1..), so a plain call whose callee is a value branch never materialized child 0. I confirmed transform_call_args then lowers it with plain transform_expr (fn.v: transformed_callee := t.const_fn_call_target(callee_id) or { ... t.transform_expr(callee_id) }), which is exactly the empty-callee path.
The fix adds callee_is_value_branch := !is_method && t.is_value_match_or_if_operand(recv_fn_id), counts it as operand position 0, and materializes it via transform_value_operand — symmetric with the receiver/argument handling — then re-dispatches over the rewritten call.
On the regression: I hit a wrinkle worth flagging. The exact shape from your example — a bare propagating callable arm tail, (match node { First { make_cb(node)! } ... })() where make_cb returns !fn () int — can't currently run in v3 because a bare function value wrapped in a Result mis-lowers independently of this change: make_cb(First{}) or { ... } takes the error branch (works on mainline v, panics under v3). So select_value_branch_callee uses a struct-wrapped callable — (match node { First { make_cb_first(node)!.f } ... })() -> 41 — which exercises the immediately-invoked value-branch-callee path end to end. (Because the propagation there is nested under the .f selector rather than the bare arm tail, that particular variant lowers fine even without the fix, so it's a smoke test rather than a strictly load-bearing one; the load-bearing bare-tail form is gated by the separate !fn() codegen bug.) Happy to add the bare-tail regression once that lands. No new regressions; review_cgen passes.
| new_lhs := if t.is_value_match_or_if_operand(lhs_id) { | ||
| t.transform_value_operand(lhs_id) |
There was a problem hiding this comment.
This value-aware membership handling only covers the range branch; for (match node { First { make_foo(node)! } else { make_bar(node)! } }) in [Foo1, Foo3], the array-literal path returns through lower_type_pattern_membership, which still sends the subject through stable_expr_for_reuse and therefore plain transform_expr. The propagating match arms can consequently produce the same empty expression; route the type-pattern subject through typed value lowering and add a regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aabe76a. lower_type_pattern_membership now routes a value-branch subject through typed value lowering — t.transform_expr_for_type(lhs_id, sum_name) then stable_transformed_expr_for_reuse — instead of stable_expr_for_reuse/transform_expr, so a propagating match subject is materialized as a typed value rather than emitting an empty expression. This mirrors the range-bound and membership-needle handling.
One thing to note: I couldn't add a runtime regression because this path isn't reachable in v3 yet — the checker rejects x in [Type, ...] before transform runs. Even the plainest form fails:
struct Foo1 {}
struct Foo3 {}
type Foo = Foo1 | Foo3
f := Foo(Foo1{})
println(f in [Foo1, Foo3])
// v3: error: `Foo1` must be initialized (mainline `v` compiles this and prints `true`)
So the fix is applied for correctness/consistency and will be exercised once the v3 checker admits type-pattern membership, but a value-propagation regression can't compile under v3 today. I can fold in the checker support separately if you'd like it wired up end to end. No new regressions; review_cgen passes.
…an mutate them (#28000) Addresses review feedback on #28011. The operand-ordering guards spilled a preceding operand only when !is_stable_expr_for_reuse, leaving an identifier / selector / stable index inline because it is cheap to re-evaluate. But a later value-branch operand's hoisted prelude can mutate that storage, so re-reading it after the prelude yields a different value than source order — e.g. y := x + (match node { First { change(mut x)! } ... }) read the updated x. Add is_pure_constant_expr (stricter than is_stable_expr_for_reuse: false for value-bearing lvalue reads) and snapshot_*_for_reuse, and route the value-read ordering guards through them: infix LHS, numeric-shift LHS, call receiver and preceding args, index children, range tested value and low bound, string/fixed/dynamic/unknown membership needle, and array-init len/cap. A pure constant is still left inline. Append and channel targets are lvalue/reference mutation targets (not value reads) and are unchanged. To keep the call/infix/index re-dispatch terminating (a snapshot temp is a non-pure ident that would otherwise be re-snapshotted on each pass with a nested-branch operand), record snapshot temp names in ordering_snapshot_names and treat them as already-captured via operand_needs_ordering_snapshot / is_ordering_snapshot_temp. Regression tests: select_value_stable_lhs_snapshot (c.v + (match { c.bump()! }) -> 6100; leaked mutation 105100 on HEAD) and select_value_stable_arg_snapshot (take2(c.v, match { c.bump()! }) -> 1005; leaked 100005 on HEAD).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49fa317b59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // `needle in (match node { ... get_values(node)! ... })`); otherwise the propagating | ||
| // arm tail is lowered in a value-less statement context and emits an empty expression. | ||
| // `transform_value_operand` is a no-op for the common non-branch containers. | ||
| mut expr := t.transform_value_operand(id) |
There was a problem hiding this comment.
When a propagating value-branch container mutates a syntactically stable needle, this materialization queues the container prelude after lower_array_membership_expr has left the needle identifier inline via stable_transformed_expr_for_reuse. For example, x in (match node { First { change(mut x)! } else { [1] } }) reads x in the generated membership loop only after change runs, rather than evaluating the needle before the container as the source requires. Snapshot the needle whenever the array container hoists a value branch, and add a propagation ordering regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ee812dc. In x in container, lower_array_membership_expr lowers the needle before the container (needle-first path, receiver_first=false) but through stable_transformed_expr_for_reuse, which leaves a syntactically stable needle (ident/selector) inline. When the container is a value branch whose prelude mutates that needle, the container prelude drains after the needle is left inline, so the generated membership loop reads the needle only after the mutation.
The needle-first branch now snapshots the needle when the array container hoists a value branch — if t.operand_hoists_value_branch(base_id) { t.snapshot_transformed_expr_for_reuse(...) } — capturing its source-order value before the container prelude. The receiver-first path (arr.contains(x)) is intentionally unchanged: there the container is evaluated first in source order, so reading the needle after it is already correct.
Regression test select_value_membership_needle_snapshot: c.v in (match node { First { c.arr_first(node)! } … }) where the arm sets c.v = 100 → 5 in [1, 5, 9] = true. On HEAD the mutated needle leaks in → 100 in [1, 5, 9] = false (verified: reverting the snapshot reproduces false). No new regressions; review_cgen passes.
…hip subjects (#28000) Addresses review feedback on #28011. - Call callee: the operand-materialization scan only set last_branch for a method receiver (child 0 when is_method) and arguments (1..), so a plain call whose callee is itself a value branch — (match node { First { make_cb(node)! } ... })() — never materialized child 0. transform_call_args then lowered it with plain transform_expr (fn.v: transformed_callee := ... t.transform_expr(callee_id)), leaving a propagating branch tail in a value-less statement context and emitting an empty callee. Detect a non-selector value-branch callee (callee_is_value_branch), count it as operand 0, and materialize it via transform_value_operand, symmetric with receiver/argument handling. - Type-pattern membership subject: lower_type_pattern_membership spilled the subject with stable_expr_for_reuse (plain transform_expr), so a value-branch subject ((match ...) in [Foo1, Foo3]) would lower its propagating arms as an empty expression. Route a value-branch subject through typed value lowering (transform_expr_for_type(lhs_id, sum_name)), consistent with the range/needle paths. Regression: select_value_branch_callee ((match node { First { make_cb_first(node)!.f } ... })() -> 41) exercises the immediately-invoked value-branch callee end to end. (A strictly load-bearing variant with a bare propagating callable arm tail and the direct type-pattern-membership syntax are currently blocked by separate v3 limitations — a bare function value wrapped in a Result mis-lowers, and the checker rejects x in [Type, ...]; see the PR thread replies.)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aabe76a8ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| low := if t.is_value_match_or_if_operand(low_id) { | ||
| t.transform_value_operand(low_id) | ||
| } else { | ||
| t.stable_expr_for_reuse(low_id) |
There was a problem hiding this comment.
When low_id is a stable lvalue and the high bound directly or indirectly contains a propagating value branch, stable_expr_for_reuse leaves the low bound inline while lowering the high bound queues its prelude. For example, if for i in low .. (match node { First { change_low(mut low)! } else { 10 } }) mutates low, the queued prelude runs before the loop initializer reads low, so iteration starts at the mutated value rather than the source-order value. Detect a hoisting high bound and snapshot the low bound first, with a regression covering the mutation.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 531e6c8. lower_range_for_in lowers the low bound before the high bound but via stable_expr_for_reuse, which leaves a stable low bound inline. The low branch now snapshots it when the high bound hoists a value branch — else if t.operand_hoists_value_branch(high_id) && t.operand_needs_ordering_snapshot(low_id) { t.snapshot_expr_for_reuse(low_id) } — so the loop initializer reads the source-order low before the high bound's prelude runs.
Regression test select_value_range_low_snapshot: for i in c.v .. (match node { First { c.rng_hi_first(node)! } … }) where the arm sets c.v = 100 → for i in 1 .. 3 sums 1+2 = 3. On HEAD the mutated low leaks in → for i in 100 .. 3 is an empty range → 0 (verified: reverting the snapshot reproduces 0). No new regressions; review_cgen passes.
| map_expr := if t.is_value_match_or_if_operand(map_source_id) { | ||
| t.transform_value_operand(map_source_id) | ||
| } else { | ||
| t.stable_expr_for_reuse(map_source_id) | ||
| } |
There was a problem hiding this comment.
When the map base is an identifier or another stable lvalue and the key contains a propagating value match/if, this fallback leaves the map expression inline, but transforming the key queues its branch prelude before the eventual map access. If that prelude reassigns the map, as in items[match node { First { replace(mut items)! } else { 'k' } }], the lookup uses the replacement map instead of the map value evaluated before the key. Snapshot the map base whenever the key hoists a value branch and add an ordering regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 531e6c8. try_lower_map_index_expr lowers the map base before the key but via stable_expr_for_reuse, which leaves a stable base (ident/selector) inline. The base branch now snapshots it when the key hoists a value branch — else if t.operand_hoists_value_branch(key_id) && t.operand_needs_ordering_snapshot(map_source_id) { t.snapshot_expr_for_reuse(map_source_id) } — capturing the map evaluated before the key, so a key prelude that reassigns the base can't redirect the lookup.
Regression test select_value_map_base_snapshot: items[match node { First { replace_map(mut items)! } … }] where the arm does items = {"x": 999} → the lookup uses the original map, items["x"] = 5. On HEAD the reassignment leaks in → 999 (verified: reverting the snapshot reproduces 999). No new regressions; review_cgen passes.
… hoists (#28000) Addresses review feedback on #28011. In `x in container`, lower_array_membership_expr lowers the needle before the container (needle-first, receiver_first=false) but via stable_transformed_expr_for_reuse, which leaves a syntactically stable needle (an ident/selector) inline. When the container is a value branch whose prelude mutates that needle (`x in (match node { First { change(mut x)! } ... })`), the container prelude drains after the needle is left inline, so the generated membership loop reads the needle only after the mutation — not in source order. Snapshot the needle whenever the array container hoists a value branch (operand_hoists_value_branch(base_id) -> snapshot_transformed_expr_for_reuse), capturing its source-order value before the container prelude. The receiver-first path (arr.contains(x)) is unchanged: there the container is evaluated first in source order, so reading the needle after it is correct. Regression test select_value_membership_needle_snapshot: `c.v in (match node { First { c.arr_first(node)! } ... })` where the arm sets c.v = 100 -> `5 in [1, 5, 9]` = true; on HEAD the mutated needle leaks in (`100 in [1, 5, 9]` = false).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee812dc717
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lhs := t.transform_expr(t.a.child(&node, 0)) | ||
| value := t.transform_expr(t.a.child(&rhs, 0)) | ||
| send_prelude_start := t.pending_stmts.len | ||
| mut lhs := t.transform_expr(t.a.child(&node, 0)) |
There was a problem hiding this comment.
In the .arrow/or {} fast path, a channel-producing value match/if on the left remains on plain transform_expr. For (match node { First { channel_first(node)! } else { channel_second(node)! } }) <- 1 or { return }, the propagating arm tail is therefore still lowered in statement context and can emit an empty channel expression. Fresh evidence beyond the resolved sent-value thread is that only sent_value_id received value-aware lowering; route the target through value lowering too and add a regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 296f7e0. The target (child 0) now goes through transform_value_operand instead of plain transform_expr, so a value-branch channel target is materialized into a value temp before the send (its prelude joins the detached send-prelude, ahead of the sent value). Previously the propagating arm tail was lowered in a value-less statement context and emitted an empty channel expression — verified: on HEAD the generated C fails to compile with error: expected expression.
Regression test select_value_branch_channel_target: (match node { First { c.pick_first(node)! } … }) <- 7 or { return -1 } → sends 7 to ch1 → 7. No new regressions; review_cgen passes.
| lhs = if t.is_stable_expr_for_reuse(lhs) { | ||
| lhs |
There was a problem hiding this comment.
When the sent-value branch can reassign a stable channel target, this arm leaves the target identifier inline. For example, if target <- (match node { First { retarget(mut target)! } else { 1 } }) or {} changes target from ch1 to ch2, the hoisted RHS prelude runs before the final send reads target, so the value is sent to ch2 instead of the channel evaluated before the RHS. Fresh evidence beyond the resolved stable-operand thread is this fast path's explicit unchanged return for stable targets; snapshot the channel value because a send target does not need lvalue identity, and add a propagation regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 296f7e0. You're right that a send target doesn't need lvalue identity — a channel is a reference handle — so I replaced the lvalue-preserving stabilization with a plain value snapshot: when the sent value hoists a value branch, the target is spilled via snapshot_transformed_expr_for_reuse (which captures value-bearing lvalues, unlike stable_transformed_expr_for_reuse). That captures the source-order channel even when the RHS prelude reassigns a stable target, and it also subsumes the earlier index-component and rvalue-root cases (spilling the whole target by value evaluates its components in order), so the bespoke transformed_lvalue_root_needs_value_spill helper is gone. The existing channel-order regressions (select_value_channel_target_order/nested/rvalue/composite → 9912/9912/7712/5512) still pass, confirming the ordering is preserved.
Regression test select_value_channel_target_reassign: c.target <- (match node { First { c.retarget(node)! } … }) or {} where the arm does c.target = c.ch2 → 7 lands on the original ch1 → 7. On HEAD the reassignment leaks in and 7 goes to ch2 → 107 (verified: reverting the snapshot reproduces 107). No new regressions; review_cgen passes.
…lude (#28000) Addresses review feedback on #28011. Two more value-read ordering guards left a syntactically stable operand inline via stable_expr_for_reuse while lowering a later branch operand queued its prelude: - for-in range: lower_range_for_in lowers the low bound before the high bound. When the high bound hoists a value branch whose prelude mutates a stable low bound (`for i in low .. (match node { First { change_low(mut low)! } ... })`), the queued prelude runs before the loop initializer reads the inline low bound, so iteration starts at the mutated value. Snapshot the low bound when the high bound hoists a value branch. - map index: try_lower_map_index_expr leaves a stable map base inline, then lowers the key. When the key hoists a value branch whose prelude reassigns the base (`items[match node { First { replace(mut items)! } ... }]`), the lookup uses the replacement map instead of the map evaluated before the key. Snapshot the map base when the key hoists a value branch. Regression tests: select_value_range_low_snapshot (`for i in c.v .. (match { rng_hi })` where the arm sets c.v = 100 -> sum 1+2 = 3; leaked mutation gives an empty range and 0) and select_value_map_base_snapshot (`items[match { replace_map(mut items)! } ...]` -> original items["x"] = 5; leaked reassignment gives 999).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 531e6c8983
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // `(match node { ... make_cb(node)! ... })()` — must materialize operand 0 too; | ||
| // otherwise transform_call_args lowers child 0 with plain transform_expr and leaves the | ||
| // propagating branch tail in a value-less statement context, emitting an empty callee. | ||
| callee_is_value_branch := !is_method && t.is_value_match_or_if_operand(recv_fn_id) |
There was a problem hiding this comment.
When a non-method callee is a runtime expression but not itself a direct value branch, a later branch argument is materialized while the callee remains inline. For example, make_cb(mut trace)(match node { First { trace.arg(node)! } else { 0 } }) queues the argument prelude before the final call evaluates make_cb, reversing callee-before-argument order; a function-valued variable can likewise be changed by the prelude before it is read. Fresh evidence beyond the resolved direct-branch-callee case is that callee_is_value_branch only recognizes a direct match/if, and the ordering path never snapshots any other plain callee. Treat the non-method callee as operand position 0 and stabilize runtime callee expressions before later hoisting arguments, with a propagation ordering regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8fc64d7. You're right — callee_is_value_branch only recognized a direct match/if callee, and no other plain callee was stabilized. The non-method callee is now treated as operand position 0: when a later argument hoists a value branch, the callee is snapshotted via a new callee_needs_ordering_snapshot(recv_fn_id) before the argument loop, so it evaluates in source order (once) ahead of the argument's hoisted prelude.
callee_needs_ordering_snapshot is deliberately narrow to avoid breaking name-based dispatch:
- a runtime callee expression (a call/index/… that isn't pure-constant) → snapshot (e.g.
make_cb(mut trace)(...)); - a function-valued local variable (
var_typeisfn …/a fn-pointer, which a prelude could reassign) → snapshot; - a plain top-level function-name ident → left inline (call_name-based lowering — generics, variadics, interface boxing — relies on the callee ident);
- an already-snapshotted temp → not re-snapshotted (keeps the re-dispatch terminating).
Regression test select_value_runtime_callee_order: make_adder_cb(mut tr)(match node { First { tr.cbarg_first(node)! } … }), where make_adder_cb records order 1 and the arm records order 2 → adder(7) = 7 with order [1,2] → 712. On HEAD the argument prelude runs first → order [2,1] → 721 (verified: disabling the snapshot reproduces 721). No new regressions; review_cgen passes.
…28000) Addresses review feedback on #28011. In the `<- ... or {}` fast path, only the sent value received value-aware lowering; the target (child 0) still used plain transform_expr and was stabilized as an lvalue: - A value `match`/`if` channel target (`(match node { First { channel_first(node)! } ... }) <- 1 or { return }`) had its propagating arm tail lowered in a value-less statement context, emitting an empty channel expression (the generated C failed with "expected expression"). Lower the target through transform_value_operand so a branch target is materialized into a value temp. - A stable target was left inline, so a sent-value branch that reassigns it (`target <- (match node { First { retarget(mut target)! } ... }) or {}`) sent to the replacement channel instead of the target evaluated before the RHS. A send target is a channel reference handle and does not need lvalue identity, so snapshot the target value (snapshot_transformed_expr_for_reuse) when the sent value hoists a branch. This subsumes the earlier index-component / rvalue-root cases (still verified by the existing channel-order regressions), so the bespoke transformed_lvalue_root_needs_value_spill helper is removed. Regression tests: select_value_branch_channel_target (`(match ...) <- 7` -> 7; empty callee C error on HEAD) and select_value_channel_target_reassign (`c.target <- (match { c.retarget(node)! } ...)` where the arm sets c.target = c.ch2 -> 7 on the original ch1; leaked reassignment sends to ch2 -> 107).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 296f7e0d9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| t.stable_expr_for_reuse(base_child) |
There was a problem hiding this comment.
When the gated-index base is a stable lvalue and the index hoists a propagating branch, stable_expr_for_reuse leaves the base inline. For example, if values#[match node { First { replace(mut values)! } else { 0 } }] reassigns values, the queued index prelude runs before the final gated access reads the base, so it indexes the replacement array rather than the array evaluated before the index. Snapshot the base whenever the index directly or indirectly hoists a value branch, and add a regression covering reassignment.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in de6b95c. lower_gated_scalar_index lowers the base before the index but via stable_expr_for_reuse, which leaves a stable base inline. The base branch now snapshots it when the index hoists a value branch — else if t.operand_hoists_value_branch(idx_child) && t.operand_needs_ordering_snapshot(base_child) { t.snapshot_expr_for_reuse(base_child) } — so the gated access indexes the array evaluated before the index, not a replacement a key/index prelude installs.
Regression test select_value_gated_base_snapshot: values#[match node { First { replace_arr(mut values)! } … }] where the arm does values = [100, 200, 300] → original values#[0] = 5. On HEAD the reassignment leaks in → [100, 200, 300]#[0] = 100 (verified: reverting the snapshot reproduces 100). No new regressions; review_cgen passes.
| children << if t.is_stable_expr_for_reuse(comp_id) { | ||
| comp_id | ||
| } else { | ||
| t.spill_original_lvalue_component(comp_id, 'recv_index') |
There was a problem hiding this comment.
When a mutable receiver or mut argument uses an identifier index that a later branch argument changes, this helper preserves the identifier instead of its current value. Thus items[idx].update(match node { First { change_idx(mut idx)! } else { 0 } }) evaluates the hoisted branch prelude first and then mutates items at the new index. Fresh evidence beyond the resolved stable-operand thread is that the identity-preserving lvalue helper still explicitly skips spilling stable index components here; snapshot index values while preserving the surrounding lvalue identity, and add a mutation-order regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in de6b95c. stabilize_original_lvalue_receiver (used for mut receivers and mut arguments) preserved a stable index/deref component inline via is_stable_expr_for_reuse. The component spill now gates on is_pure_constant_expr instead, so a value-bearing index (an ident/selector a later branch prelude could mutate) is snapshotted into a temp while the surrounding lvalue shape is kept — items[recv_index] with recv_index := idx — so the element at the source-order index is mutated. A pure-constant index still stays inline. (Both the .index and .prefix/deref component gates got the change; this helper only runs in value-branch operand contexts, so the extra spill is scoped to those.)
Regression test select_value_mut_receiver_index_snapshot: items[c.v].add(match node { First { c.idx_bump_first(node)! } … }) where the arm sets c.v = 1 → items[0].v = 40 + 5 = 45 → 4550. On HEAD the mutated index leaks in and items[1] is updated → 4055 (verified: reverting to is_stable_expr_for_reuse reproduces 4055). No new regressions; review_cgen passes.
…ment (#28000) Addresses review feedback on #28011. The operand-ordering path materialized a direct value-branch callee (callee_is_value_branch) but never stabilized any other plain callee, so a non-method runtime callee preceding a later branch argument stayed inline. For `make_cb(mut trace)(match node { First { trace.arg(node)! } ... })`, the argument prelude was queued before the final call evaluated make_cb, reversing callee-before-argument order (and a function-valued variable could be reassigned by the prelude before it is read). Treat the non-method callee as operand position 0: when a later argument hoists a value branch, snapshot the callee via callee_needs_ordering_snapshot, which stabilizes a runtime callee expression (a call/index/... that is not pure-constant) and a function-valued local variable, but leaves a plain top-level function-name ident inline (name-based call dispatch relies on it) and skips already-snapshotted temps. Regression test select_value_runtime_callee_order: `make_adder_cb(mut tr)(match node { First { tr.cbarg_first(node)! } ... })` where both record into an order trace -> 712 (the callee runs first); a reversed order gives 721 (verified: disabling the snapshot reproduces 721).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fc64d7dd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if node.children_count > 0 { | ||
| recv_fn_id := t.a.children[node.children_start] | ||
| recv_fn := t.a.nodes[int(recv_fn_id)] | ||
| is_method := recv_fn.kind == .selector && recv_fn.children_count > 0 |
There was a problem hiding this comment.
When the callee is a function-valued field, this classifies it as a method solely because it is a selector. For a reference-backed holder such as p.callback(match node { First { replace_callback(p)! } ... }), the method path snapshots only p; the hoisted argument prelude can then replace p.callback before the rebuilt selector reads it, invoking the new callback instead of the callee evaluated before the arguments. Fresh evidence beyond the earlier runtime-callee comment is this selector-wide classification; distinguish function fields (for example with receiver_selector_is_fn_field) and snapshot the complete callee, with a propagation-order regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in af10faf. You're right — is_method := recv_fn.kind == .selector classified a function-valued field callee as a method, so p.callback(match ...) took the method path and stabilized only the receiver p. For a reference-backed holder the hoisted argument prelude could then replace p.callback before the rebuilt selector read it.
The callee is now distinguished via the existing receiver_selector_is_fn_field(base_type, field):
is_fn_field_callee := is_selector_call
&& t.receiver_selector_is_fn_field(t.normalize_type_alias(t.trim_pointer_type(t.lvalue_type(recv_sel_base_id))), recv_fn.value)
is_method := is_selector_call && !is_fn_field_calleeA fn-field callee is not a method, so it falls into the non-method callee path and callee_needs_ordering_snapshot snapshots the whole p.callback (its field value) before the arguments. A real method selector still stabilizes only its receiver (a method name can't be reassigned). This block only runs in value-branch operand contexts, so the reclassification is scoped to those, and if the base type can't be resolved it falls back to the previous method classification.
Regression test select_value_fn_field_callee_order: p.callback(match node { First { p.install_new(node)! } … }) on mut p := &CbHolder{ callback: cb_orig }, where the arm sets p.callback = cb_new → the callee snapshot invokes cb_orig(3) = 30. On HEAD the replaced field is invoked → cb_new(3) = 300 (verified: reclassifying fn-fields as methods reproduces 300). No new regressions; review_cgen passes.
…fore branch operands (#28000) Addresses review feedback on #28011. - Gated index: lower_gated_scalar_index lowers the base before the index but via stable_expr_for_reuse, which leaves a stable base inline. When the index hoists a value branch whose prelude reassigns the base (`values#[match n { First { replace(mut values)! } ... }]`), the gated access indexes the replacement array instead of the array evaluated before the index. Snapshot the base when the index hoists a value branch. - Mutable-lvalue index components: stabilize_original_lvalue_receiver (used for mut receivers and `mut` arguments) preserved a stable index/deref component inline via is_stable_expr_for_reuse. When a later branch argument mutates that index (`items[idx].update(match ... { change_idx(mut idx)! } ...)`), the mutation then targets the element at the new index. Snapshot value-bearing index/deref components (is_pure_constant_expr gate) into temps while keeping the surrounding lvalue identity, so the element at the source-order index is mutated. This helper only runs in value-branch operand contexts, so the extra spill is scoped to those. Regression tests: select_value_gated_base_snapshot (`values#[match { replace_arr(mut values)! } ...]` -> original values#[0] = 5; leaked reassignment indexes [100,200,300] -> 100) and select_value_mut_receiver_index_snapshot (`items[c.v].add(match { c.idx_bump_first(node)! } ...)` where the arm sets c.v = 1 -> items[0].v = 45 -> 4550; mutated index updates items[1] -> 4055).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de6b95cdaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .ident { | ||
| return id | ||
| } |
There was a problem hiding this comment.
When a mutable receiver's lvalue base can be reassigned by a later propagating branch argument, returning the identifier unchanged does not actually stabilize the receiver. For example, holder.ptr.update(match node { First { retarget(mut holder)! } ... }) evaluates the hoisted retarget prelude before the rebuilt selector reads holder.ptr, so the method mutates the replacement object rather than the receiver selected in source order; indexed receivers have the same problem if the branch replaces their array base. Capture the receiver address or reassignable pointer/container base before lowering later branch arguments, and add a propagation-order regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 88c77dc. stabilize_original_lvalue_receiver preserved the lvalue by returning its base identifier unchanged, which doesn't stabilize a receiver reached through a reassignable indirection. Two capture paths added:
- Pointer receiver (
holder.ptr.update(...), or any&Treceiver): captured whole up front viasnapshot_expr_for_reuse— a pointer is a reference handle, so no lvalue identity is needed.if t.lvalue_type(id).starts_with('&') { return t.snapshot_expr_for_reuse(id) }. - Reassignable array/map container base (
items[i].update(...)): the.indexbase is snapshotted when its type is[]/map[, so an argument prelude that replaces the container can't retarget the in-place mutation — the snapshot shares the original backing storage, so the element mutation still reaches the source-order container. (Confirmed a mutable base snapshot mutates the original buffer.)
Regression tests: select_value_pointer_receiver_capture — h.ptr.add(match node { First { h.retarget_first(node)! } … }) where the arm sets h.ptr = &PtrObj{v:1000} → the original object is mutated (orig.v = 15) → 16000; on HEAD the retargeted object is mutated → 11005. select_value_array_base_capture — items[0].add(match node { First { replace_cells(mut items)! } … }) where the arm reassigns items → the source-order element is mutated → 2500; on HEAD → 2005. Both verified load-bearing. Existing mut-receiver regressions (4512/4550) still pass; review_cgen passes and review_ownership is unchanged from HEAD (pre-existing failure).
| tail_expr_id := if last.kind == .expr_stmt && last.children_count > 0 { | ||
| t.a.child(&last, 0) | ||
| } else if last.kind == .block && t.stmt_value_type(last_id).len > 0 { | ||
| } else if last.kind in [.block, .match_stmt, .if_expr] && t.stmt_value_type(last_id).len > 0 { |
There was a problem hiding this comment.
When this newly supported block tail occurs in a later struct field, its materialization prelude is emitted before the completed struct initializer while earlier field values remain inline. In the inspected transform_struct_fields path, Pair{a: tr.first(), b: if cond { match node { First { tr.second(node)! } ... } } else { 0 }} therefore runs second before first, reversing field evaluation order. Snapshot preceding field values before a later field hoists pending statements, and add an ordering regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 88c77dc. transform_struct_fields drains each field value's prelude into the shared prelude emitted before the struct initializer, so a later field whose value hoists a nested block/if/match branch ran its prelude while earlier inline field values were read afterward — reversing field order. The loop now snapshots a preceding field value before a later field hoists its branch prelude (a pre-scan finds the last field whose value operand_hoists_value_branch, and each earlier non-pure field value is spilled via snapshot_transformed_expr_for_reuse).
Regression test select_value_struct_field_order: SPair{ a: tr.sfirst(), b: if cond { match node { First { tr.ssecond(node)! } … } } else { 0 } } where both record into an order trace → a=3, b=4, order [1,2] → 3412. On HEAD second runs before first → 3421 (verified: disabling the snapshot reproduces 3421). No new regressions; review_cgen passes.
…28000) Addresses review feedback on #28011. The operand-ordering path classified any selector callee as a method (is_method := recv_fn.kind == .selector), so a function-valued field callee like `p.callback(match ...)` took the method path and stabilized only the receiver `p`. For a reference-backed holder, a hoisted argument prelude could then replace `p.callback` before the rebuilt `p.callback` selector was read, invoking the new callback instead of the callee evaluated before the arguments. Distinguish a function-field callee via the existing receiver_selector_is_fn_field: such a selector is not a method, so it falls into the non-method callee path and is snapshotted whole (callee_needs_ordering_snapshot -> snapshot_expr_for_reuse captures the field value before the arguments). A real method selector still stabilizes only its receiver, since a method name cannot be reassigned. Only value-branch operand contexts reach this block, so the reclassification is scoped to them. Regression test select_value_fn_field_callee_order: `p.callback(match node { First { p.install_new(node)! } ... })` on `mut p := &CbHolder{callback: cb_orig}`, where the arm sets `p.callback = cb_new` -> cb_orig(3) = 30; on HEAD the replaced field is invoked -> cb_new(3) = 300.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af10fafbc4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // lowering so its propagating arm tail is materialized as a value instead of in a | ||
| // value-less statement context. `transform_value_operand` is a no-op for the | ||
| // common non-branch push-many operands. | ||
| rhs = t.transform_value_operand(rhs_id) |
There was a problem hiding this comment.
In the optional-LHS path, a push-many branch such as holder.values? << (match node { First { holder.replace(node)! } ... }) can reassign holder.values while materializing the RHS. The guard above evaluates the original selector, but lhs_addr later reuses the inline source after the RHS prelude, so the append targets the replacement optional rather than the storage selected before evaluating the RHS. Snapshot the optional source storage/address before lowering the branch and add an ordering regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in ef7a5a4. The optional value-array address is now captured up front when the RHS hoists a value branch, instead of re-reading the inline source at lhs_addr after the RHS prelude:
if t.operand_hoists_value_branch(rhs_id) {
addr := t.runtime_addr(t.make_selector(source, 'value', array_type), array_type)
captured_lhs_addr = t.stable_transformed_expr_for_reuse(addr, '&${array_type}', 'opt_append_target')
...
}So the append target is selected before the RHS, consistent with the !source.ok guard above it.
One transparency note on the regression. I built the branch and compared v3 against mainline v across several shapes of this exact scenario — simple field (holder.values or {...} << (match ... { holder.replace()! } ...)), a side-effecting source base (ctx.get().values? << ...), an indexed source base (holders[ctx.idx].values? << ... where the arm bumps ctx.idx), and an arm that sets the source to none. In every case v3 already produced the same result as mainline (e.g. the simple field gives [100, 200, 7, 8] on both), so the append was already targeting the same storage mainline uses. This change keeps that parity: for the cases that reach this path the value-array slot address is identical whether captured before or after the RHS (source for a dynamic base is already stabilized to a single evaluation), so it's a robustness/ordering fix (selecting the target in source order, matching the guard) rather than a value change — I couldn't construct a case where the observable result diverges.
The added regression select_value_optional_append_reassign (holder.values or { ... } << (match node { First { holder.replace_first(node)! } ... }) -> [100,200,7,8] -> 500) is therefore path coverage for the reassigning-RHS push-many optional append rather than a strictly load-bearing ordering test — I flag that honestly. If you have a specific reproducer where v3 diverges from mainline here, I'll add a targeted load-bearing regression and confirm this fix addresses it. No new regressions; review_cgen passes.
…ranch operands (#28000) Addresses review feedback on #28011. - Mutable receiver storage: stabilize_original_lvalue_receiver preserved a receiver lvalue by returning its base identifier unchanged, which does not stabilize a receiver reached through a reassignable pointer or container. A later branch argument prelude could then reassign the indirection before the rebuilt lvalue is read, so the method mutated the replacement. Capture such storage in source order: a pointer-valued receiver (`holder.ptr.update(match ... { retarget(mut holder)! } ...)`) is snapshotted whole (a reference handle needs no lvalue identity), and a reassignable array/map container base (`items[i].update(match ... { replace(mut items)! } ...)`) is snapshotted so the element mutation still reaches the source-order backing storage. - Struct field order: transform_struct_fields drains each field value prelude into the shared prelude emitted before the struct initializer, so a later field whose value hoists a nested block/if/match branch ran its prelude while earlier inline field values were read afterward (`Pair{a: tr.first(), b: if c { match ... }}` ran second before first). Snapshot a preceding field value before a later field hoists its branch prelude. Regression tests: select_value_pointer_receiver_capture (16000; retargeted 11005), select_value_array_base_capture (2500; retargeted 2005), select_value_struct_field_order (3412; reversed 3421).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88c77dc88a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new_rhs := if rhs_is_value_branch { | ||
| t.transform_value_operand(infix_rhs_id) |
There was a problem hiding this comment.
When a later select send value contains a propagating value branch, this materialization remains in the transformer's global pending_stmts; transform_select_expr then drains it before the entire select, while gen_select normally evaluates earlier case values during select setup. Thus select { ch1 <- tr.first() {} ch2 <- (match node { First { tr.second(node)! } else { 0 } }) {} } runs second before first, and the prelude can also mutate an earlier case's channel before it is read. Isolate or order case preludes and add a select-case regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d87e0bf. Confirmed the bug: select { ch1 <- tr.first() {} ch2 <- (match node { First { tr.second(node)! } else { 0 } }) {} } gave order [2, 1] on HEAD — the match materialization sits in the global pending_stmts, which is drained before the whole select, while gen_select evaluates each case value (int tmp = <value>;) during select setup, so second ran before first.
The fix orders the case preludes rather than isolating them. When any case hoists a value branch (select_case_hoists_value_branch), transform_select_branch captures each case in source order:
transform_select_send_orderedsnapshots the channel and send value into temps (select_chan/select_send_val);transform_select_recv_orderedsnapshots a receive channel (so a later case's prelude cannot change an earlier channel before it is read).
Because the branches are transformed in case order, these snapshots land in pending_stmts in case order, so they drain before the select in source order and gen_select just reads the temps. Selects with no hoisting case are untouched (the capture is gated on order_cases).
Regression test select_value_select_case_order: select { ch1 <- tr.sel_first() {} ch2 <- (match node { First { tr.sel_second(node)! } … }) {} } → order [1,2] → 12. On HEAD it is 21 (verified: disabling the ordering reproduces 21). No new regressions — the select-using v3 tests (drop_codegen, for_multi_init_codegen, pointer_map_lvalue_codegen, channel_close_codegen) and review_cgen all pass.
…ch RHS (#28000) Addresses review feedback on #28011. In the optional-LHS array append (try_lower_optional_array_append_stmt), lhs_addr was computed from the inline transformed source (`&source.value`) after the RHS prelude, so a value-branch RHS that reassigns the optional source (`holder.values or { ... } << (match ... { holder.replace()! } ...)`) had the append target re-read from the source after the RHS rather than selected before it (the `!source.ok` guard already evaluates the source before the RHS). When the RHS hoists a value branch, capture the optional value-array address up front via stable_transformed_expr_for_reuse and reuse it for the append, so the target is selected in source order and consistent with the guard. Verified against mainline V: v3 already matched mainline across the scenarios I could construct (simple field, side-effecting base, indexed base, none-reassignment), and this change keeps that parity — for the cases that reach this path the value-array slot address is identical whether captured before or after the RHS, so it is a robustness/ordering fix with no behavioral divergence rather than a value change. Regression select_value_optional_append_reassign covers the reassigning-RHS push-many optional append (`holder.values or { ... } << (match ... { holder.replace_first()! } ...)` -> 500).
Addresses review feedback on #28011. When a later select send-case value contains a propagating value branch, its materialization prelude stayed in the transformer global pending_stmts; transform_select_expr does not drain per branch, so the caller drains it before the whole select while gen_select evaluates earlier case values during select setup. Thus `select { ch1 <- tr.first() {} ch2 <- (match node { First { tr.second(node)! } ... }) {} }` ran second before first (order [2,1]), and such a prelude could also mutate an earlier case channel before it is read. When any select case hoists a value branch (select_case_hoists_value_branch), transform_select_branch now captures each case in source order: transform_select_send_ordered snapshots the channel and send value into temps, and transform_select_recv_ordered snapshots the receive channel. The snapshots land in pending_stmts in case order, so they drain before the select in source order and gen_select reads the temps. Selects with no hoisting case are unchanged. Regression test select_value_select_case_order: `select { ch1 <- tr.sel_first() {} ch2 <- (match node { First { tr.sel_second(node)! } ... }) {} }` -> order [1,2] -> 12; on HEAD the branch prelude runs first -> [2,1] -> 21.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d87e0bfd09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !t.is_stable_expr_for_reuse(chan_expr) { | ||
| chan_expr = t.snapshot_transformed_expr_for_reuse(chan_expr, t.node_type(chan_expr), | ||
| 'select_chan') |
There was a problem hiding this comment.
When an earlier case uses a stable channel or value identifier that a later propagating send-value branch mutates, this guard leaves the identifier inline until select setup. For example, in select { ch1 <- x {} ch2 <- (match node { First { change(mut x)! } else { 0 } }) {} }, the hoisted change prelude runs before the select reads x, reversing source evaluation order. Fresh evidence beyond the earlier select-order comment is that the newly added ordered helper still snapshots only expressions classified as unstable; snapshot nonconstant channel and send values here as well.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 541fe3d. The ordered helpers now gate on the shared operand_needs_ordering_snapshot (nonconstant / not-already-a-snapshot-temp) instead of !is_stable_expr_for_reuse, so a stable but value-bearing identifier (an ident/selector) in an earlier case is snapshotted into a temp in source order; only pure constants stay inline. This is routed through a new snapshot_select_operand helper shared by the send and receive paths.
Regression test select_value_select_stable_operand: select { ch1 <- c.v {} ch2 <- (match node { First { c.sel_change(node)! } … }) {} } where the arm sets c.v = 100 (an unbuffered ch2 with no receiver forces case 1 to fire) → <-ch1 = 5 (the source-order value). On HEAD c.v is left inline and read after the prelude → 100 (verified: reverting the gate reproduces 100). No new regressions; review_cgen and the select-using v3 tests pass.
| first := t.a.nodes[int(t.a.child(&branch, 0))] | ||
| if first.kind == .infix && first.op == .arrow && first.children_count >= 2 { | ||
| return t.operand_hoists_value_branch(t.a.child(&first, 1)) | ||
| } |
There was a problem hiding this comment.
This scan only examines a send case's value, so a later send or receive case whose channel expression contains a propagating value match/if leaves order_cases false. In select { ch1 <- first() {} (match node { First { make_channel(node)! } else { ch2 } }) <- 1 {} }, lowering the later channel queues its prelude before the whole select while first() remains in select setup, reversing case evaluation order. Include channel operands in the scan and lower them with the value-aware path in the ordered helpers.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 541fe3d. select_case_hoists_value_branch now scans the channel operand as well as the send value — for a send case (ch <- value, both operands), a receive case (<-ch), and a receive-assign (x := <-ch) — so a case whose channel is a value branch sets order_cases. And snapshot_select_operand lowers the channel through the value-aware path (transform_value_operand), so a match/if channel is materialized into a value temp instead of being lowered with plain transform_expr.
Regression test select_value_select_branch_channel: select { ch1 <- c.c_first() {} (match node { First { c.c_make(node)! } Second { c.ch2 } }) <- 1 {} } where both record into an order trace → order [1,2] → 12 (c_first before the channel match -> c_make). On HEAD the channel-match prelude drains before the select while first() stays in setup → 21 (verified: reverting the scan reproduces 21). No new regressions; review_cgen and the select-using v3 tests pass.
… channels (#28000) Addresses review feedback on #28011. - The ordered select helpers snapshotted only operands classified as unstable (!is_stable_expr_for_reuse), so a stable value-bearing identifier in an earlier case that a later branch prelude mutates was left inline until select setup (`select { ch1 <- x {} ch2 <- (match ... { change(mut x)! } ...) {} }` read the mutated x). Snapshot nonconstant channel and send values via operand_needs_ordering_snapshot (the shared snapshot gate), so value-bearing operands are captured in source order and only pure constants stay inline. - The order_cases scan examined only a send case value, so a send or receive case whose channel is a value branch left order_cases false and the channel was lowered with plain transform_expr (`select { ch1 <- first() {} (match ... { make_channel()! } ...) <- 1 {} }` queued the channel prelude before the whole select while first() stayed in setup). Include channel operands (send + receive) in the scan and lower them through the value-aware path. Both paths share a new snapshot_select_operand helper: a value-branch operand is materialized via transform_value_operand; a nonconstant operand is snapshotted; a pure constant is left inline. Regression tests: select_value_select_stable_operand (`ch1 <- c.v` before `ch2 <- (match { c.sel_change()! })` -> ch1 gets 5; mutated 100 on HEAD) and select_value_select_branch_channel (`(match { c.c_make()! }) <- 1` channel case -> order [1,2] -> 12; reversed 21 on HEAD).
1 participant
Fix #28000 (v3 backend)
This is the v3-only portion of #28002, split out per request (the
vlib/vcompiler changes from that PR are dropped here). #28002 is being closed in favor of this.A
match/ifused as the value of another expression — a cast, a string-interpolation part,dump(), a prefix/infix operand, an index expression, a selector receiver, or anasoperand — must have its (possibly propagating,!/?) branch tails lowered as values. Previously the v3 transformer lowered them in a value-less statement context, which produced an empty ternary / expression for the propagated result.Change
vlib/v3/transform/transform.v: addtransform_value_operandandis_value_match_or_if_operandhelpers. The latter looks through transparent wrappers —(...)parens,unsafe { }blocks, and a trailingexpr_stmt(including compositions likei64(unsafe { match ... })). Route the affected operands (string-interp parts,dump(), infix/prefix operands, index children, selector base, cast operands, and block-expr value tails) through target-typed lowering.vlib/v3/transform/sum.v: handle amatch/ifoperand of anascast by materializing it into a value temp first, then re-running theasconversion.Test
vlib/v3/tests/match_as_if_expr_value_propagation_codegen_test.v— covers!/?propagation for amatchused as an if-guard value, and wrapped in parens /unsafe/ cast / interpolation / dump / prefix / index / selector operands.Reproduction from the issue now compiles and runs correctly: