parser: fix match-arm !/? propagation when a match is used as a block value (fix #28000) - #28002
parser: fix match-arm !/? propagation when a match is used as a block value (fix #28000)#28002medvednikov wants to merge 20 commits into
!/? propagation when a match is used as a block value (fix #28000)#28002Conversation
…alue (fix #28000) A `match` whose arms use `!`/`?` error-propagation, when the match is used as the value of an if-expression (or directly as a block value), emitted invalid C: the if-expression result temp was assigned an empty expression (`_t4 = ;`), giving "expected expression". `mark_last_call_return_as_used` recursively descended into `if` branches to flag the last call's return value as used, but had no case for `match`, so calls in match arms kept `is_return_used == false`. cgen then skipped emitting the unwrapped propagated value. Add a `MatchExpr` case symmetric to the existing `IfExpr` one, marking the last statement of each match branch. The v3 backend already handled this case; a regression test is added there too.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
![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: fab69bef16
ℹ️ 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".
| } | ||
| } | ||
| } | ||
| ast.MatchExpr { |
There was a problem hiding this comment.
When the block value is parenthesized, for example result := if value := node { (match value { First { lower_first(value)! } Second { lower_second(value)! } }) } else { 0 }, the parser retains an ast.ParExpr wrapper (vlib/v/parser/expr.v:251), so this MatchExpr arm is never reached because mark_last_call_return_as_used has no ParExpr case. The match-arm calls therefore remain return-unused and the original invalid empty C assignment can still be emitted; recurse through ParExpr and cover this valid spelling in the regression test.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0efc15b. Split the per-expression logic into mark_last_call_expr_return_as_used(mut expr ast.Expr) so it can recurse through ast.ParExpr by reference, and added a ParExpr case. Both the vlib/v and vlib/v3 regression tests now cover the parenthesized spelling ( match .. { .. } ). Verified locally: compiler_errors_test (1617 snapshots), the parser test suite, and the 45 related match/option/result tests all pass.
…ls as return-used
Address PR review: when the block value is parenthesized, e.g.
`result := if value := node { (match value { ... }) } else { 0 }`, the
parser keeps an `ast.ParExpr` wrapper around the inner `match`.
`mark_last_call_return_as_used` had no `ParExpr` case, so the `MatchExpr`
arm was never reached, the match-arm calls stayed return-unused, and cgen
could still emit the invalid empty assignment (`_t = ;`).
Split the per-expression handling into `mark_last_call_expr_return_as_used`
(taking `mut expr ast.Expr`) so it can recurse through `ParExpr` by
reference, and add a `ParExpr` case. Cover the parenthesized spelling in
both the vlib/v and vlib/v3 regression tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0efc15bd24
ℹ️ 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".
| ast.ParExpr { | ||
| // last stmt on block is parenthesized: ( match .. { a { foo() } } ) | ||
| p.mark_last_call_expr_return_as_used(mut expr.expr) |
There was a problem hiding this comment.
When an assignment-RHS branch uses a value such as (unsafe { match value { First { lower_first(value)! } Second { lower_second(value)! } } }), the parser retains an ast.UnsafeExpr around the match, but this helper stops at that wrapper and leaves both propagated calls with is_return_used == false; C generation then reaches the same empty assignment path this change is intended to fix. Fresh evidence beyond the previously covered ParExpr case is that UnsafeExpr is another transparent wrapper whose C generator directly emits its inner expression, so recurse through expr.expr here and add regression coverage for this spelling.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 439a513 — and this one needed fixing in both backends:
- vlib/v: added an
ast.UnsafeExprcase inmark_last_call_expr_return_as_usedthat recurses throughexpr.expr, mirroring theParExprcase. - vlib/v3: the same spelling exposed a matching gap in the v3 backend.
transform_block_expr_for_typeonly treatedexpr_stmt/nested-blocktails as value tails, sounsafe { match ... }(a block whose tail is a barematch_stmt) fell back to the type-lesstransform_block_exprand emitted a malformed empty ternary_ifexpr = (!ok ? : ). It now treats a value-producingmatch_stmt/if_exprtail as the value expression, so the target type reaches the propagating arms.
Both regression tests now cover (unsafe { match .. }). Verified v3 still self-hosts with the transform change, and confirmed (by diffing a pre-change vs post-change v3 build) that the fix is neutral to unrelated tests.
…ion (#28000) Address PR review: an assignment-RHS branch value such as `(unsafe { match value { First { lower_first(value)! } ... } })` keeps an `ast.UnsafeExpr` around the match, another transparent wrapper (like `ParExpr`) whose C generator emits its inner expression directly. Both backends failed to propagate the value context through it: - vlib/v: `mark_last_call_expr_return_as_used` stopped at `UnsafeExpr`, so the match-arm calls stayed `is_return_used == false` and cgen emitted the invalid empty assignment (`_t = ;`). Add an `UnsafeExpr` case that recurses through `expr.expr`, mirroring the `ParExpr` case. - vlib/v3: `transform_block_expr_for_type` only recognized `expr_stmt` and nested `block` tails as value tails, so an unsafe block whose tail is a bare `match`/`if` (`unsafe { match ... }`) fell back to the type-less `transform_block_expr`, lowering the propagating arms in statement context and emitting a malformed empty ternary (`_ifexpr = (!ok ? : )`). Treat a value-producing `match_stmt`/`if_expr` tail as the tail expression so the target type reaches its branch tails. Cover the `(unsafe { match })` spelling in both regression tests. Verified v3 still self-hosts with the transform change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 439a51341a
ℹ️ 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".
| p.mark_last_call_return_as_used(mut or_block_last_stmt) | ||
| } | ||
| } | ||
| else {} |
There was a problem hiding this comment.
When the block tail wraps the match in an ast.CastExpr, for example result := if value := node { i64(match value { First { lower_first(value)! } Second { lower_second(value)! } }) } else { 0 }, this match falls through the default case and its branch calls retain is_return_used == false. C generation then treats the match as a value but emits empty branch assignments, reproducing the invalid C this patch addresses; recurse through CastExpr.expr (and similarly transparent AsCast operands) and add a regression case.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1546c32. Investigating this turned up that the parser recursion alone was necessary but not sufficient — there were two distinct bugs, so both backends needed a checker/transform fix in addition to the parser change:
vlib/v (main compiler)
- checker: a
match/ifoperand of a cast (CastExprorAsCast, possibly parenthesized) was checked with a void expected type when nested in a void context (an if-branch), so it was mistyped asvoid— the case failed at type-checking (expression does not return a value so it cannot be cast/cannot cast non-sum typevoidusingas``) before ever reaching cgen. Fixed by giving such an operand the cast target as its expected type (is_expr). - parser: with the type fixed, cgen then emitted the exact
_t = ;you predicted, becausemark_last_call_expr_return_as_usedhad noCastExpr/AsCastcase. Added both, recursing throughexpr.expr, mirroringParExpr/UnsafeExpr.
vlib/v3
transform_cast_expr(andtransform_as_expr) lowered the operand with plaintransform_expr(statement context), so the propagating arms produced the empty ternary_ifexpr = (!ok ? : ). A valuematch/ifoperand is now routed throughtransform_expr_for_type.
Both regression tests now cover i64(match ...) and (match ...) as Circle. Verified: v3 still self-hosts, compiler_errors_test (1617 snapshots) + 35 cast/sumtype/match tests are unchanged, and that both the checker and parser fixes are individually required (checker-only still emits the empty C).
…s with propagation (#28000) Address PR review: a block-tail value that wraps a `match`/`if` in a cast, e.g. `i64(match value { First { lower_first(value)! } ... })` or `(match value { ... }) as Circle`, needs the same value-context handling as the `ParExpr`/`UnsafeExpr` wrappers. Two distinct bugs had to be fixed for these to compile and run correctly: vlib/v (main compiler): - checker: a `match`/`if` operand of a cast (`ast.CastExpr`/`ast.AsCast`, possibly parenthesized) was checked with a void expected type when nested in a void context (e.g. an if-branch), so it was mistyped as `void` ("expression does not return a value so it cannot be cast" / "cannot cast non-sum type `void` using `as`"). Give such an operand the cast target as its expected type so it is checked as an expression (`is_expr`). - parser: with the type fixed, cgen then emitted the empty assignment (`_t = ;`) because `mark_last_call_expr_return_as_used` had no `CastExpr`/`AsCast` case, leaving the match-arm calls return-unused. Add cases that recurse through the wrapper, mirroring `ParExpr`/`UnsafeExpr`. vlib/v3: - `transform_cast_expr` lowered a non-float/non-pointer cast operand with plain `transform_expr` (statement context), and `transform_as_expr` likewise, so a `match`/`if` operand's propagating arms produced an empty ternary (`_ifexpr = (!ok ? : )`). Route a value `match`/`if` operand through `transform_expr_for_type` (materializing it for the `as` path). Cover `i64(match ...)` and `(match ...) as Circle` in both regression tests. Verified v3 still self-hosts, and that both fixes are required (checker-only still emits the empty C).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1546c320ad
ℹ️ 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 expr is ast.ParExpr { | ||
| return cast_operand_is_value_match_or_if(expr.expr) | ||
| } | ||
| return expr is ast.MatchExpr || expr is ast.IfExpr |
There was a problem hiding this comment.
Although the separate unsafe and cast spellings are covered, composing them—for example i64(unsafe { match value { First { lower_first(value)! } Second { lower_second(value)! } } }) in an if-expression branch—still returns false here because this predicate only peels ParExpr. cast_expr therefore leaves expected_type as void; unsafe_expr forwards that void context to the match, which is checked as a statement rather than a value, so valid code is rejected. The v3 predicate has the equivalent unsafe-block gap; recurse through this wrapper in both implementations and add a composition regression.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b210e50. You're right — the predicates only peeled (...). Recursing through unsafe { } in both, plus one extra fix the composition surfaced:
- vlib/v checker:
cast_operand_is_value_match_or_ifnow also recurses throughast.UnsafeExpr, soi64(unsafe { match ... })(in any paren composition) is checked as a value. - vlib/v cgen: recognizing the composed as-cast spelling exposed a pre-existing bug —
as_cast_operand_needs_tmp_evaldidn't look throughast.UnsafeExpreither, so(unsafe { match ... }) as Variantemitted invalid C (a temp decl inside the__as_castargument). It now recurses through the wrapper like theParExprcase; this also fixes the direct (non-if-branch) spelling that was already broken. - vlib/v3:
is_value_match_or_if_operandnow looks throughunsafe { }(a.blockwhose value tail is the expression) and a trailingexpr_stmt.
Both regression tests now cover i64(unsafe { match ... }) and (unsafe { match ... }) as Circle. Verified: v3 still self-hosts, and compiler_errors_test (1617 snapshots) + the cast/sumtype/as suites are unchanged.
…ped match/if values (#28000) Address PR review: composing the cast and unsafe wrappers, e.g. `i64(unsafe { match value { First { lower_first(value)! } ... } })` in an if-branch, was still rejected because the value-match predicates only peeled `(...)` parens, not `unsafe { }`. `cast_expr` therefore left `expected_type` void, `unsafe_expr` forwarded that void context to the match, and it was checked as a statement ("expression does not return a value so it cannot be cast"). - vlib/v checker: `cast_operand_is_value_match_or_if` now also recurses through `ast.UnsafeExpr`, so a cast operand of `unsafe { match/if ... }` (in any composition with parens) is checked as a value. - vlib/v cgen: recognizing the composed as-cast spelling then surfaced a pre-existing bug — `as_cast_operand_needs_tmp_eval` did not look through `ast.UnsafeExpr`, so `(unsafe { match ... }) as Variant` emitted invalid C (a temp declaration inside the `__as_cast` argument). It now recurses through the wrapper, matching the existing `ParExpr` case. This also fixes the direct (non-if-branch) spelling that was already broken. - vlib/v3: `is_value_match_or_if_operand` now looks through `unsafe { }` (a `.block` whose value tail is the expression) and a trailing `expr_stmt`, so `transform_cast_expr`/`transform_as_expr` route the operand through `transform_expr_for_type`. Cover `i64(unsafe { match ... })` and `(unsafe { match ... }) as Circle` in both regression tests. Verified v3 still self-hosts and compiler_errors_test plus the cast/sumtype/as suites are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b210e50582
ℹ️ 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".
| p.mark_last_call_return_as_used(mut or_block_last_stmt) | ||
| } | ||
| // last stmt has infix expr with CallExpr: foo()? + 'a' | ||
| mut left_expr := expr.left |
There was a problem hiding this comment.
When an assignment-RHS block ends in an expression such as 1 + (match value { First { lower_first(value)! } Second { lower_second(value)! } }), the nested match branches are parsed with inside_assign_rhs cleared, leaving their calls return-unused. This InfixExpr case examines only expr.left and never applies the new recursive helper to the right-hand match; similarly, a wrapped match on the left causes the loop to stop. C generation can therefore omit the propagated values and recreate the invalid empty assignment. Traverse the infix operands through mark_last_call_expr_return_as_used and add regression coverage for this composition.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 77a271a. Replaced the left-only expr.left walk with recursion into both operands via the helper, so a match/if on either side (nested, or wrapped in parens/unsafe/cast) is marked return-used. This also fixes a()! + b()!, where the old loop marked only the left call.
Two extra things the composition surfaced:
- checker: a value
match/ifon the left of an infix ((match ...) + 10) was checked with the (void) surrounding expected type and mistyped as a statement when nested in an if-branch — it failed at type-checking (mismatched types void and int literal) before reaching cgen.infix_exprnow resolves the right operand's type first and uses it as the left operand's expected type (reusing the existing check-right-first path used for short enums). Renamed the shared predicate tooperand_is_value_match_or_if. - vlib/v3:
transform_infix_exprlowered both operands with plaintransform_expr; a valuematch/ifoperand now goes throughtransform_expr_for_type(newtransform_infix_operandhelper).
Both regression tests now cover 1 + (match ...) and (match ...) + 10. Verified: v3 self-hosts, and compiler_errors_test (1617 snapshots) + the enum/infix/match/option/result suites are unchanged.
…values (#28000) Address PR review: an assignment-RHS block ending in an infix expression such as `1 + (match value { First { lower_first(value)! } ... })` (or with the match on the left, `(match ...) + 10`) still recreated the invalid empty C. - vlib/v parser: `mark_last_call_expr_return_as_used`'s `InfixExpr` case only walked a left-associative `expr.left` chain and never looked at `expr.right` (and stopped on a wrapped-match left). It now recurses into both operands via the helper, so nested/wrapped match/if/call values on either side are marked as return-used. This also fixes `a()! + b()!`, where the old loop marked only the left call. - vlib/v checker: a value `match`/`if` on the *left* of an infix was checked with the (void) surrounding expected type and mistyped as a statement when nested in an if-branch. `infix_expr` now resolves the right operand's type first and uses it as the expected type for such a left operand (reusing the existing check-right-first path). Renamed the shared predicate `cast_operand_is_value_match_or_if` -> `operand_is_value_match_or_if`. - vlib/v3: `transform_infix_expr` lowered both operands with plain `transform_expr`; a value `match`/`if` operand now goes through `transform_expr_for_type` (new `transform_infix_operand` helper). Cover `1 + (match ...)` and `(match ...) + 10` in both regression tests. Verified v3 self-hosts and compiler_errors_test plus the enum/infix/match/ option/result suites are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77a271ae2d
ℹ️ 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".
| ast.CallExpr { | ||
| // last stmt on block is CallExpr | ||
| expr.is_return_used = true |
There was a problem hiding this comment.
When an assignment-RHS block ends with a call such as wrap(match value { First { lower_first(value)! } Second { lower_second(value)! } }), the match-arm tails are parsed in statement context, but this case marks only the outer wrap call and never visits its arguments. The propagated inner calls therefore retain is_return_used == false, allowing C generation to reproduce the empty branch assignment this change is intended to prevent; recurse into call arguments containing value match/if expressions and add regression coverage for this composition.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b3b5a13. The CallExpr case now recurses into arguments that hold a block-value match/if, marking their arm calls as return-used. To avoid touching plain arguments, the recursion is gated by a new expr_is_wrapped_match_or_if predicate that looks through (...)/unsafe/cast/as-cast wrappers, so only arguments that actually contain a value match/if are traversed — wrap(match ...), wrap((match ...)), wrap(unsafe { match ... }), add(100, match ...), and wrap(if ... { foo()! } else { ... }) all work now.
No checker fix was needed here (unlike the cast/infix cases): a call argument already gets its expected type from the parameter, so the match is typed correctly and only the parser marking was missing. The v3 backend already routes arguments through the parameter type and handled this composition, so it needed no code change — a regression test is added there too.
Both regression tests now cover wrap(match ...). Verified compiler_errors_test (1617 snapshots) + the call/fn/match/option/result suites are unchanged.
) Address PR review: an assignment-RHS block ending in a call whose argument is a block-value match/if, e.g. `wrap(match value { First { lower_first(value)! } ... })`, recreated the invalid empty C. The `CallExpr` case marked only the outer call and never visited its arguments, so the propagated inner calls kept `is_return_used == false`. Recurse into call arguments that hold a (possibly `(...)`/`unsafe`/cast/as-cast wrapped) value match/if via a new `expr_is_wrapped_match_or_if` predicate that gates the recursion, so only such arguments are traversed (plain arguments are left untouched). The v3 backend already routes call arguments through the parameter type and handled this case; a regression test is added there too. Cover `wrap(match ...)` in both regression tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3b5a13a0f
ℹ️ 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 p.expr_is_wrapped_match_or_if(arg.expr) { | ||
| p.mark_last_call_expr_return_as_used(mut arg.expr) |
There was a problem hiding this comment.
The new direct-argument handling still misses compositions such as wrap(wrap(match value { First { lower_first(value)! } Second { lower_second(value)! } })): the outer argument is an ast.CallExpr, so expr_is_wrapped_match_or_if returns false and the inner call and match are never visited. The propagated match-arm calls therefore retain is_return_used == false, allowing the same empty C assignment this patch fixes; recurse through nested argument expression trees and add a regression for this composition.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3c7894. Extended the gate predicate (renamed expr_is_wrapped_match_or_if -> expr_contains_value_match_or_if) to also look through a nested CallExpr's arguments, so an argument whose subtree contains a value match/if is recursed into and its arm calls are marked return-used. Arbitrary nesting now works — wrap(wrap(match ...)), wrap(wrap(wrap(match ...))), wrap(add(1, match ...)), wrap(int(i64(match ...))).
The predicate still returns false for argument trees that contain no match/if, so plain arguments like wrap(wrap(foo())) are left untouched (no over-marking). No checker change was needed — arguments get their expected type from the parameter — and v3 already handled nested arguments, so it needed no code change (regression test added there too).
Both regression tests now cover wrap(wrap(match ...)). Verified compiler_errors_test (1617 snapshots) + the call/fn/match/option/result suites are unchanged.
#28000) Address PR review: the direct call-argument handling missed nested compositions such as `wrap(wrap(match value { First { lower_first(value)! } ... }))`. The outer argument is itself an `ast.CallExpr`, so the gate predicate returned false and the inner call and match were never visited, leaving the propagated arm calls return-unused and recreating the invalid empty C. Extend the predicate (renamed `expr_is_wrapped_match_or_if` -> `expr_contains_value_match_or_if`) to also look through a nested `CallExpr`'s arguments, so a call argument whose subtree contains a value match/if is recursed into and its arm calls are marked as return-used. The gate still skips argument trees that contain no match/if, so plain call arguments are untouched. The v3 backend already routes call arguments through the parameter type and handled this; a regression test is added there too. Cover `wrap(wrap(match ...))` in both regression tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3c78948cf
ℹ️ 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 { | ||
| false | ||
| } |
There was a problem hiding this comment.
When a block-value call argument contains the match inside an infix expression, such as wrap(1 + (match value { First { lower_first(value)! } Second { lower_second(value)! } })), this predicate falls through to false, so the CallExpr handler never traverses that argument even though the marking helper supports InfixExpr. The match-arm calls retain is_return_used == false, allowing the same empty C assignment this change addresses; recognize and recurse through infix operands here and add composition coverage.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f46183f. Added an ast.InfixExpr case to expr_contains_value_match_or_if that recurses through both operands, so a call argument whose subtree reaches a value match/if through infix operators is recognized and traversed (the marking helper already handles InfixExpr).
Verified compositions: wrap(1 + (match ...)), wrap((match ...) + 10), add(1 + (match ...), 100), and wrap(wrap(1 + (match ...))). The gate still returns false for infix trees with no match/if, so plain arguments are untouched. No checker change needed, and v3 already handled this composition (regression test added there too).
Both regression tests now cover wrap(1 + (match ...)). Verified compiler_errors_test (1617 snapshots) is unchanged.
…k-value match/if (#28000) Address PR review: a call argument that wraps the match inside an infix expression, e.g. `wrap(1 + (match value { First { lower_first(value)! } ... }))`, fell through `expr_contains_value_match_or_if` to false, so the `CallExpr` handler never traversed it and the match-arm calls stayed return-used == false, recreating the invalid empty C. Recognize `ast.InfixExpr` in the predicate, recursing through both operands, so an argument whose subtree reaches a value match/if through infix operators is marked. The marking helper already handles `InfixExpr`. The v3 backend already handled this composition; a regression test is added there too. Cover `wrap(1 + (match ...))` in both regression tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f46183fdd8
ℹ️ 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 p Parser) mark_last_call_expr_return_as_used(mut expr ast.Expr) { |
There was a problem hiding this comment.
Fresh evidence beyond the already-fixed call/infix spellings is an assignment-RHS branch ending in [match value { First { lower_first(value)! } Second { lower_second(value)! } }]: this visitor has no ast.ArrayInit case, so it never reaches the match or marks its arm calls as return-used. Because match-arm blocks are parsed after inside_assign_rhs is cleared, C generation treats those propagated calls as discarded while assigning the match temporary and can emit the same empty assignment this change is intended to prevent; recurse through array elements and add this composition to the regression test.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb4b179. This one needed both a parser and a checker change:
- parser: added an
ast.ArrayInitcase tomark_last_call_expr_return_as_used(recurse through elements that contain a value match/if) and toexpr_contains_value_match_or_if(so array literals nested in call arguments are covered). - checker: recognizing the array element then surfaced a typing gap — in a void context (nested in an if-branch) the element match/if was checked with a void expected type and mistyped as a statement (
invalid void array element type). An expected-type override doesn't work here since the element type is inferred and any concrete type mistypes the arms (I confirmednone/array-type both break it), so I added aninside_array_init_value_elemflag set around the element check inarray_initand honored bymatch_expr/if_exprto forceis_exprwhile leaving the expected type void so the arms infer their own type.
Verified: [match ...], [(match ...)], [unsafe { match ... }], [i64(match ...)], and [100, match ...] all compile+run on both backends. v3 already handled array elements, so it needed no code change (regression test added there too). The flag is only set for value match/if array elements in a void context, so all other matches are unaffected — compiler_errors_test (1617 snapshots) + the array/match/if/option/result/sumtype suites are unchanged.
Both regression tests now cover [match ...].
…agation (#28000) Address PR review: an assignment-RHS branch ending in an array literal whose element is a value match/if, e.g. `[match value { First { lower_first(value)! } ... }]`, was rejected and could recreate the invalid empty C. Two fixes were needed: - checker: in a void context (e.g. nested in an if-branch) the array element match/if was checked with a void expected type and mistyped as a statement ("invalid void array element type"). An expected-type override is unusable here (the element type is inferred and any concrete type mistypes the arms), so a new `inside_array_init_value_elem` flag is set around the element check in `array_init` and honored by `match_expr`/`if_expr` to force `is_expr` while leaving the expected type void so the arms infer their own type. - parser: with the type fixed, cgen then emitted the empty assignment (`_t = ;`) because `mark_last_call_expr_return_as_used` had no `ArrayInit` case. Recurse through array elements that contain a value match/if, and recognize `ArrayInit` in `expr_contains_value_match_or_if` so array literals nested in call arguments are covered too. The v3 backend already handled array-literal elements; a regression test is added there too. Cover `[match ...]` in both regression tests. Verified compiler_errors_test plus the array/match/if/option/result suites are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb4b179c58
ℹ️ 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".
| } | ||
| } | ||
| } | ||
| ast.ArrayInit { |
There was a problem hiding this comment.
When an assignment-RHS branch ends in a struct literal such as Holder{value: match node { First { lower_first(node)! } Second { lower_second(node)! } }}, the checker supplies the field type so the nested match is a value, but this visitor handles only array literals and never traverses StructInit.init_fields. The match-arm calls therefore remain return-unused and C generation can emit the same empty temporary assignment this change is intended to prevent; recurse through struct field expressions and add regression coverage for this composition.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 353a90a. Added an ast.StructInit case to mark_last_call_expr_return_as_used (recurse through init_fields) and to expr_contains_value_match_or_if (so struct literals nested in call arguments are covered too). As you noted, the checker already supplies the field type, so no checker change was needed — only the parser marking.
Verified: Holder{ value: match ... }, Holder{ value: (match ...), other: 100 }, and take(Holder{ value: match ... }) (struct as a call arg) all compile+run on both backends. v3 already handled struct fields (regression test added there too). Both regression tests now cover Holder{ value: match ... }.
| if c.expected_type == ast.void_type && !c.inside_array_init_value_elem | ||
| && operand_is_value_match_or_if(expr) { | ||
| c.inside_array_init_value_elem = true | ||
| restore_array_elem_flag = true |
There was a problem hiding this comment.
When [match ...] is checked in a void context and one of its arms contains a statement-level match or if before the final propagated value, this flag remains set throughout the recursive c.expr call, so the nested statement is also reclassified as a value expression. For example, a nested statement match whose first arm calls an int-returning side-effect function and whose second arm is empty is valid as a statement, but now fails because the checker requires every branch to produce a value; consume the flag at the intended outer node or otherwise bind it to that exact expression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 353a90a. The flag now lives only for the outer element node: match_expr and if_expr capture inside_array_init_value_elem into a local and immediately clear c.inside_array_init_value_elem before checking their arms/branches, so a nested statement-level match/if no longer inherits it.
Confirmed with your exact scenario: an array-element match arm containing a nested statement match cond { true { side_effect() /* int */ } else {} } (and a statement if) before the propagated value now compiles — the nested statement match keeps its void/empty branch, and only the outer [match ...] element is treated as a value. Added that as a regression test (select_value_arraylit_nested_stmt). compiler_errors_test + the array/match/if suites are unchanged.
…-value flag (#28000) Addresses two PR review comments: 1. Struct-field composition: an assignment-RHS branch ending in a struct literal whose field is a value match/if, e.g. `Holder{ value: match node { First { lower_first(node)! } ... } }`, kept the match-arm calls return-unused and could emit the empty C assignment. Add an `ast.StructInit` case to `mark_last_call_expr_return_as_used` (recurse through `init_fields`) and to `expr_contains_value_match_or_if` (so struct literals nested in call arguments are covered). The checker already supplies the field type, so no checker change is needed here. 2. Array-value flag scoping: `inside_array_init_value_elem` stayed set for the whole recursive `c.expr` of a `[match ...]` element, so a nested *statement-level* match/if inside an arm (valid with a void/empty branch) was wrongly reclassified as a value expression ("requires an expression as the last statement of every branch"). `match_expr`/`if_expr` now capture and immediately clear the flag, so it applies only to the outer element node. The v3 backend already handled both; regression tests are added there too. Cover `Holder{ value: match ... }` and an array element whose arm holds a nested statement match/if in both regression tests. compiler_errors_test plus the struct/array/match/if suites are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 353a90ae3e
ℹ️ 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 { | ||
| false | ||
| } |
There was a problem hiding this comment.
When an assignment-RHS branch ends with a map literal such as {'value': match value { First { lower_first(value)! } Second { lower_second(value)! } }}, this predicate falls through here because neither it nor mark_last_call_expr_return_as_used handles ast.MapInit. The match arms are parsed after inside_assign_rhs is cleared, so their propagated calls remain return-unused and C generation can reproduce the empty match-value assignment; traverse map keys/values containing value matches and add regression coverage.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 23236ba. Two parts, like the array-literal case:
- parser: added an
ast.MapInitcase tomark_last_call_expr_return_as_used(recurse through both keys and values) and toexpr_contains_value_match_or_if(so map literals nested in call arguments are covered). - checker: recognizing the map value surfaced the same void-context typing gap — nested in an if-branch the value match/if was checked with a void expected type and mistyped as
map[string]void. Reused the array value-element mechanism (renamedinside_array_init_value_elem->inside_container_value_elem), set around the first map value check inmap_initand honored bymatch_expr/if_expr(which already consume it, so nested statement match/if inside the arms are unaffected).
Verified: {'value': match ...}, {'value': (match ...)}, {'a': 100, 'value': match ...} (match not first), and a nested-statement-match arm all compile+run on both backends. v3 already handled map values (regression test added there too). Both regression tests now cover {'value': match ...}.
One scope note: a propagating match in a map key inside a void-context if-branch ({(match ...): v}) hits a separate, deeper expression evaluated but not used checker issue — the direct form x := {(match ...): v} works on both backends, and v3 handles the if-branch form too. The parser traverses keys as you asked; I scoped the checker change to values (the reported spelling) rather than ship a half-working key path. Happy to take the key case as a follow-up if you'd like.
…ation (#28000) Address PR review: an assignment-RHS branch ending in a map literal whose value is a value match/if, e.g. `{'value': match value { First { lower_first(value)! } ... }}`, was rejected and could recreate the invalid empty C. Two fixes: - checker: in a void context (nested in an if-branch) the map value match/if was checked with a void expected type and mistyped as a statement (`map[string]void`). As with array elements, an expected-type override mistypes the arms, so the existing value-element flag (renamed `inside_array_init_value_elem` -> `inside_container_value_elem`) is now also set around the first map value check in `map_init` and honored by `match_expr`/`if_expr` (which already consume it so nested statement match/if are unaffected). - parser: added an `ast.MapInit` case to `mark_last_call_expr_return_as_used` (recurse through keys and values) and to `expr_contains_value_match_or_if` (so map literals nested in call arguments are covered). The v3 backend already handled map values; a regression test is added there too. Cover `{'value': match ...}` in both regression tests. compiler_errors_test plus the map/array/match/if suites are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23236ba7ff
ℹ️ 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".
| p.mark_last_call_expr_return_as_used(mut expr.left) | ||
| p.mark_last_call_expr_return_as_used(mut expr.right) | ||
| } | ||
| ast.ComptimeCall, ast.ComptimeSelector, ast.PrefixExpr, ast.SelectorExpr { |
There was a problem hiding this comment.
In the current vlib/v parser/checker path, when an inferred assignment branch ends in a prefix expression such as -(match value { First { lower_first(value)! } Second { lower_second(value)! } }), this case inspects only the prefix's or_block and never traverses expr.right. The inner match is consequently checked in void context and its propagated calls remain return-unused, so valid value-producing code is rejected or reaches the same empty-value codegen path; recurse through the prefix operand, supply its value context in the checker, and add regression coverage.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9225506. Three parts, since (unlike the array/map/struct cases) v3 also needed a change here:
- parser:
mark_last_call_expr_return_as_usedhadPrefixExprgrouped with the or-block-only cases; split it out to recurse intoexpr.right, and addedPrefixExprtoexpr_contains_value_match_or_if(so prefixed values nested in call arguments are covered). - checker: the prefix operand match/if was checked with a void expected type and mistyped as
void.prefix_exprnow sets the value-required flag around the operand check. Since the flag now covers array elements, map values and prefix operands, I renamed itinside_container_value_elem->force_value_match_or_if; it's still consumed bymatch_expr/if_exprso nested statement match/if are unaffected. - vlib/v3:
transform_prefix_exprlowered the operand with plaintransform_expr; a valuematch/ifoperand now goes through the sharedtransform_value_operandhelper (renamed fromtransform_infix_operand).
Verified: -(match ...), ~(match ...), !(match ...) (bool arms), wrap(-(match ...)), and [-(match ...)] all compile+run identically on both backends. v3 self-hosts; compiler_errors_test (1617 snapshots) + the prefix/operator/match/array suites are unchanged. Both regression tests now cover -(match ...).
…ith propagation (#28000) Address PR review: an assignment-RHS branch ending in a prefix expression whose operand is a value match/if, e.g. `-(match value { First { lower_first(value)! } ... })`, was rejected ("value after `-` is of type `void`") and, on v3, reproduced the empty-value codegen. Three fixes: - vlib/v parser: `mark_last_call_expr_return_as_used` grouped `PrefixExpr` with the or-block-only cases and never visited `expr.right`. Split it out to recurse into the operand, and recognize `PrefixExpr` in `expr_contains_value_match_or_if` (so prefixed values nested in call arguments are covered). - vlib/v checker: the prefix operand match/if was checked with a void expected type and mistyped as `void`. `prefix_expr` now sets the value-required flag around the operand check. The flag (renamed `inside_container_value_elem` -> `force_value_match_or_if`, now covering array elements, map values and prefix operands) is consumed by `match_expr`/`if_expr`, so nested statement match/if are unaffected. - vlib/v3: `transform_prefix_expr` lowered the operand with plain `transform_expr`; a value `match`/`if` operand now goes through the shared `transform_value_operand` helper (renamed from `transform_infix_operand`). Cover `-(match ...)` in both regression tests. Verified v3 self-hosts and compiler_errors_test plus the prefix/operator/match/array suites are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9225506472
ℹ️ 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 { | ||
| false |
There was a problem hiding this comment.
When an assignment-RHS branch ends in an index expression such as values[match node { First { lower_first(node)! } Second { lower_second(node)! } }], ast.IndexExpr falls through here and is also absent from mark_last_call_expr_return_as_used. The index checker evaluates the index with the surrounding void expected type, and the propagated arm calls remain return-unused, so this valid value match is rejected or reaches the same empty-value codegen path; recurse through the indexed expression and index operand, provide value context for the index match, and add a regression test.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7020743. Three parts (v3 also needed a change, like the prefix case):
- parser:
ast.IndexExprfell through bothmark_last_call_expr_return_as_usedandexpr_contains_value_match_or_if. Handled in both, recursing through the indexed expression (left) and the index operand (plus its or-block). - checker: the array/string index match/if was evaluated with a void expected type and mistyped as
void.index_exprnow sets the value-required flag around the (non-map) index check — the map-index path already supplies the key type as expected. The flag is consumed bymatch_expr/if_expr, so nested statement match/if are unaffected. - vlib/v3:
transform_index_exprlowered operands with plaintransform_expr; they now go through the sharedtransform_value_operandhelper.
Verified: values[match ...] (array), m[match ...] (map), wrap(values[match ...]), and values[-(match ...) + 3] all compile+run identically on both backends. v3 self-hosts; compiler_errors_test (1617 snapshots) + the index/array/map/match suites are unchanged. Both regression tests now cover values[match ...].
…th propagation (#28000) Address PR review: an assignment-RHS branch ending in an index expression whose index is a value match/if, e.g. `values[match node { First { lower_first(node)! } ... }]`, was rejected ("non-integer index `void`") and, on v3, reproduced the empty-value codegen. Three fixes: - vlib/v parser: `ast.IndexExpr` fell through both `mark_last_call_expr_return_as_used` and `expr_contains_value_match_or_if`. Handle it in both, recursing through the indexed expression (`left`) and the index operand (and its or-block). - vlib/v checker: the array/string index match/if was evaluated with a void expected type and mistyped as `void`. `index_expr` now sets the value-required flag around the (non-map) index check; the map-index path already supplies the key type. The flag is consumed by `match_expr`/`if_expr`, so nested statement match/if are unaffected. - vlib/v3: `transform_index_expr` lowered operands with plain `transform_expr`; they now go through the shared `transform_value_operand` helper. Cover `values[match ...]` in both regression tests. Verified v3 self-hosts and compiler_errors_test plus the index/array/map/match suites are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7020743560
ℹ️ 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 !check_right_type_first && c.expected_type == ast.void_type | ||
| && operand_is_value_match_or_if(node.left) { |
There was a problem hiding this comment.
When an inferred branch tail uses a membership expression such as (match node { First { lower_first(node)! } Second { lower_second(node)! } }) in [1, 2], this path checks the right operand first and later uses its full type ([]int) as the expected type of the left-hand match. The match arms return int, so valid code is rejected as though each arm needed to return []int; not in and map membership have the same issue. Derive the array element/map key type for membership operators, or force the match to be a value without imposing the right operand's container type, and add a propagation regression test.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d61a5db. You're right that imposing the right operand's container type is wrong — I switched infix_expr to force the left match to be a value via the force_value_match_or_if flag (the same mechanism used for array/map/prefix/index operands), so its arms infer their own type and no container type is imposed. This replaces the earlier check-right-first-for-match path.
Confirmed the concrete-int case was passing before, but the flag is genuinely needed for context-dependent arms: I added an enum-shorthand regression ((match v { First { make_color(v)! } ... }) in [Color.red, Color.blue], arms returning .red/.green) which fails under the old container-type imposition and passes now. in, !in, and map membership all work. Regression test covers (match ...) in [1, 2]; compiler_errors_test + the infix/enum/membership suites are unchanged.
| ast.ComptimeCall, ast.ComptimeSelector, ast.SelectorExpr { | ||
| if expr.or_block.stmts.len > 0 { | ||
| mut or_block_last_stmt := expr.or_block.stmts.last() | ||
| p.mark_last_call_return_as_used(mut or_block_last_stmt) |
There was a problem hiding this comment.
When an inferred assignment branch ends with a selector such as (match node { First { lower_first(node)! } Second { lower_second(node)! } }).value, this case visits only the selector's or_block and never its receiver expression. The match-arm calls therefore remain return-unused, while selector_expr checks the receiver under the surrounding void context and treats the match as a statement, rejecting the otherwise valid value expression; the v3 selector transform likewise uses the ordinary statement-context path. Recurse through the selector receiver, propagate value context in both implementations, and cover this spelling with a regression test.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d61a5db. Recursed through the selector receiver in all three layers:
- parser: split
SelectorExprout of the or-block-only group inmark_last_call_expr_return_as_usedto recurse intoexpr.expr, and added it toexpr_contains_value_match_or_if. - checker:
selector_exprnow sets theforce_value_match_or_ifflag around the receiver check (c.expr(mut node.expr)), so the receiver match is checked as a value. - vlib/v3:
transform_selector_base_exprrouted a non-ident receiver through plaintransform_expr; it now uses the sharedtransform_value_operandhelper.
Verified: (match ...).value (arm tail is a struct value), (match ...).inner.n (selector chain, propagating arm tails), and (make_box(match ...)!).value all compile+run identically on both backends. (Note: the v3 issue only surfaced when the match arm tail is directly a propagating call — nested-in-arg propagation was already fine — so I made sure the regression exercises the direct spelling.) v3 self-hosts; regression test covers (match ...).value.
… selector receivers (#28000) Addresses two PR review comments: 1. Infix membership left operand: a value match/if on the left of a membership operator, e.g. `(match x { First { lower_first(x)! } ... }) in [1, 2]`, was handled by checking the right operand first and imposing its full type (`[]int`) as the match's expected type. Concrete `int` arms happened to work, but context-dependent arms (e.g. enum shorthand `.red`) were rejected as needing the container type. `infix_expr` now forces the left match to be a value via the `force_value_match_or_if` flag (arms infer their own type) instead of imposing the right operand's container type. Fixes `in`, `!in`, and map membership. 2. Selector receiver: a value match/if selector receiver, e.g. `(match x { ... }).field`, was left return-unused and typed `void`. - vlib/v parser: split `SelectorExpr` out of the or-block-only group in `mark_last_call_expr_return_as_used` to recurse into the receiver, and added it to `expr_contains_value_match_or_if`. - vlib/v checker: `selector_expr` sets the value-required flag around the receiver check. - vlib/v3: `transform_selector_base_expr` routes a non-ident receiver through the shared `transform_value_operand` helper. Cover `(match ...) in [1, 2]` and `(match ...).field` in both regression tests. Verified v3 self-hosts and compiler_errors_test plus the selector/infix/enum/ membership suites are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d61a5dba61
ℹ️ 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".
| // their own type), consumed by `match_expr`/`if_expr`. | ||
| mut restore_container_flag := false | ||
| if c.expected_type == ast.void_type && !c.force_value_match_or_if | ||
| && operand_is_value_match_or_if(val_) { |
There was a problem hiding this comment.
When an inferred map literal is the tail of an assignment-RHS branch and its first key is a match, such as {(match value { First { lower_first(value)! } Second { lower_second(value)! } }): 'x'}, the key is checked while expected_type is still void; the new forcing logic is applied only to val_. The key match is therefore classified as a statement and the map is inferred with a void key instead of accepting the valid integer-producing match. Apply equivalent value forcing while inferring the first key and add a propagation regression case.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in be7a393. Investigating this turned up that the first-key case actually fails earlier than the checker: {(match ...): 'x'} as an if-branch value was misparsed as a block, so (match ...) became a discarded statement (expression evaluated but not used) before type-checking. The stmt map-vs-block heuristic only checked peek_token(2) == colon, which a parenthesized key pushes past the ). I made it scan to the matching ) and treat { (…) : … } as a map when a : follows (this also fixes the non-match {(1 + 1): 'x'} spelling).
With parsing fixed, I applied the same value-forcing to the first key in map_init (matching the value path you noted), so the key match is checked as an expression and the map isn't inferred with a void key. {(match ...): v} now works on both backends (v3's parser already handled the paren key). Regression test covers it; compiler_errors_test + the parser/map suites are unchanged.
| ast.ArrayInit { | ||
| // last stmt on block is an array literal, e.g. `[match value { .. }]`; | ||
| // mark any element that is (or nests) a block-value match/if. | ||
| for mut element in expr.exprs { |
There was a problem hiding this comment.
When an assignment-RHS branch ends in a spread literal such as [...(match value { First { lower_first_array(value)! } Second { lower_second_array(value)! } })], the match is stored in ArrayInit.update_expr, but this case traverses only exprs. Its arm calls are consequently not marked return-used, and the checker also evaluates the spread operand in the surrounding void context, rejecting the otherwise valid array-valued match or recreating the empty propagated-value path. Include update_expr in the parser traversal, provide it value context in the container checker, and cover the spread spelling with a regression test.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in be7a393. ArrayInit.update_expr is now included in both parser helpers (mark_last_call_expr_return_as_used and expr_contains_value_match_or_if), and array_init forces the spread operand to be a value (same flag as the element path). [...(match ...)] (arms returning []int) compiles+runs correctly on the main compiler now.
One heads-up: the v3 backend has a separate, deeper pre-existing bug here — it mis-infers the spread array type as [][]int in a void if-branch (the []int spread operand is treated as a single element), so the value-context flag isn't enough there. The direct form x := [...(match ...)] works on v3; only the void if-branch spread mis-infers. Since this comment (and the referenced lines) are about the vlib/v parser + container checker, I scoped the regression to the main compiler for the spread and left the v3 type-inference fix as a follow-up — happy to take it separately if you'd like.
…f block values (#28000) Addresses two PR review comments: 1. Inferred map first key: `{(match value { First { lower_first(value)! } ... }): 'x'}` as an if-branch value failed at parse time — the `{ (` was misparsed as a block (the map-vs-block heuristic only checks `peek_token(2) == colon`, which a parenthesized key pushes past the `)`), so `(match ...)` became a discarded statement ("expression evaluated but not used"). The `stmt` `.lcbr` heuristic now scans to the matching `)` and treats it as a map when a `:` follows. With that, `map_init` forces the first key match/if to be a value (same flag as the value path), so it is not mistyped as a void key. 2. Array spread operand: `[...(match value { First { lower_first_array(value)! } ... })]` stores the match in `ArrayInit.update_expr`, which the parser traversal and the container checker ignored. `mark_last_call_expr_return_as_used` and `expr_contains_value_match_or_if` now include `update_expr`, and `array_init` forces the spread operand to be a value. Cover `{(match ...): v}` and `[...(match ...)]` in the vlib/v regression test (and the map-key spelling in the v3 test). compiler_errors_test plus the parser and map/array/block suites are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be7a3930dc
ℹ️ 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".
| ast.StructInit { | ||
| // e.g. `Holder{ value: match value { .. } }` as a call argument. | ||
| mut found := false | ||
| for field in expr.init_fields { |
There was a problem hiding this comment.
Fresh evidence beyond the array-spread case is Holder{ ...(match value { First { make_first(value)! } Second { make_second(value)! } }), other: 1 }: StructInit.update_expr is omitted from both new traversals, while checker/struct.v checks that operand under the surrounding void context of an inferred if branch. The match is therefore treated as a statement and rejected as a non-struct update, and its propagated arm calls are never marked return-used; traverse update_expr, force its value context, and add regression coverage.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bb7234. StructInit.update_expr is now included in both parser traversals (mark_last_call_expr_return_as_used and expr_contains_value_match_or_if). In practice the checker already gives the update operand the struct's value context, so only the parser marking was missing on the main compiler — Holder{ ...(match ...), other: 9 } now compiles+runs correctly. Regression test covers it.
(v3 has a separate pre-existing gap: it lowers a match struct-update operand incorrectly even in the direct form x := Holder{...(match ...)}, so I scoped the v3 regression to the interpolation spelling and left the v3 struct-update lowering as a follow-up.)
| ast.MapInit { | ||
| // e.g. `{'value': match value { .. }}` as a call argument. | ||
| mut found := false |
There was a problem hiding this comment.
Fresh evidence beyond the handled array spread is an inferred branch ending in {...(match value { First { map_first(value)! } Second { map_second(value)! } }), 'x': 1}. The MapInit cases inspect only keys and vals, omitting update_expr; checker/containers.v consequently checks the update match with a void expected type, rejects it as a non-map update, and leaves its propagating calls return-unused. Include update_expr in both traversals, provide value context in map_init, and cover this spelling.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bb7234. Two parts: { ...(match ...), 'k': v } as an if-branch value was first misparsed as a block ({ ... at statement position), so I taught the stmt .lcbr heuristic that a leading ... means a map literal (a block cannot start with ...). Then MapInit.update_expr is included in both parser traversals, and map_init forces the update operand to be a value (same flag as the value/key paths). Works on the main compiler now; regression covers it.
Heads-up: v3 has a separate parser block-vs-map ambiguity for {...} here too, so I scoped the v3 regression to the interpolation spelling for this batch.
| else { | ||
| false |
There was a problem hiding this comment.
When an inferred branch ends in a string interpolation such as '${match value { First { stringify_first(value)! } Second { stringify_second(value)! } }}', the checker correctly treats the interpolation operand as a string-valued match, but both new parser traversals fall through for ast.StringInterLiteral. Its arm calls therefore remain return-unused and C generation can emit the same empty match-value assignment this change is intended to prevent; recurse through StringInterLiteral.exprs and add a propagation regression.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bb7234. Both parser traversals now recurse through StringInterLiteral.exprs, so the interpolated match's arm calls are marked return-used and cgen no longer emits the empty _t = ;. The checker already types the interpolation operand as you noted, so no checker change was needed. On v3, transform_string_interp_part now routes the operand through the shared transform_value_operand helper.
Verified 'x=${match ...}' compiles+runs correctly on both backends; regression tests cover it in both. v3 self-hosts; compiler_errors_test + the string/interp suites are unchanged.
…ck values (#28000) Addresses three PR review comments: 1. Struct update operand: `Holder{ ...(match value { First { make_first(value)! } ... }), other: 1 }` — `StructInit.update_expr` was omitted from both parser traversals. Included in `mark_last_call_expr_return_as_used` and `expr_contains_value_match_or_if` (the checker already gives the operand the struct's value context). 2. Map update operand: `{ ...(match value { First { map_first(value)! } ... }), 'k': v }` — first misparsed: `{ ...` at statement position was treated as a block. The `stmt` `.lcbr` heuristic now recognizes a leading `...` (a block cannot start with it) as a map literal. `MapInit.update_expr` is included in both parser traversals, and `map_init` forces the update operand to be a value. 3. String interpolation operand: `'x=${match value { First { stringify_first(value)! } ... }}'` — `ast.StringInterLiteral` fell through both parser traversals; now recurses through `exprs`. (The checker already types the interpolation operand.) v3: the string-interpolation operand is now routed through the shared `transform_value_operand` helper. (v3's struct/map update lower a match operand incorrectly even in the direct form — a separate pre-existing v3 gap — so the v3 regression covers the interpolation spelling.) Cover `Holder{...(match)}`, `{...(match), k: v}`, and `'${match ...}'` in the vlib/v regression test (and interpolation in the v3 test). compiler_errors_test plus the parser and map/struct/string suites are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bb7234377
ℹ️ 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".
| p.mark_last_call_return_as_used(mut or_block_last_stmt) | ||
| } | ||
| } | ||
| else {} |
There was a problem hiding this comment.
When an assignment-RHS branch ends in dump(match value { First { lower_first(value)! } Second { lower_second(value)! } }), this visitor falls through for ast.DumpExpr, even though the checker treats the dump operand as a value and C generation directly emits that operand. The propagated match-arm calls therefore remain return-unused and can recreate the empty match-value assignment; recurse through DumpExpr.expr and add regression coverage for this spelling.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 58fba14. ast.DumpExpr is now handled in both parser traversals (mark_last_call_expr_return_as_used and expr_contains_value_match_or_if), recursing into DumpExpr.expr. The checker already treats the dumped operand as a value, so only the parser marking was missing. On v3, transform_dump_expr now routes the operand through the shared transform_value_operand helper.
Verified dump(match ...) compiles+runs on both backends. Regression covers it in the vlib/v test (left out of the v3 exact-output test since dump writes to stderr, which os.execute folds into the captured output).
| else { | ||
| false |
There was a problem hiding this comment.
When an inferred assignment branch ends in _likely_(match value { First { bool_first(value)! } Second { bool_second(value)! } }), ast.Likely reaches this default and is also absent from the marking visitor. The checker consequently evaluates the match under the branch's void context and rejects the valid boolean expression as though _likely_() received void, while its propagated calls remain return-unused; recurse through the wrapper, supply boolean value context, and add a regression test.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 58fba14. Two parts: ast.Likely is now handled in both parser traversals (recursing into Likely.expr), and the checker sets the value-required flag around the operand check so the match is treated as an expression instead of being typed void (expects a boolean expression, instead it got void). Its bool arms infer their own type — I used the shared force_value_match_or_if flag rather than imposing bool, consistent with the other operand fixes.
Verified _likely_(match ...) (and _unlikely_) compiles+runs on both backends; v3 already handled it once the parser marked the arms. Regression tests cover it in both. v3 self-hosts; compiler_errors_test + the likely/match suites are unchanged.
…#28000) Addresses two PR review comments: 1. `dump()` operand: `dump(match value { First { lower_first(value)! } ... })` — `ast.DumpExpr` fell through both parser traversals. Handle it in both, recursing into `DumpExpr.expr` (the checker already treats the dumped operand as a value). On v3, `transform_dump_expr` routes the operand through the shared `transform_value_operand` helper. 2. `_likely_()`/`_unlikely_()` operand: `_likely_(match value { First { bool_first(value)! } ... })` — `ast.Likely` fell through both parser traversals, and the checker evaluated the operand under the branch's void context ("expects a boolean expression, instead it got `void`"). Handle `Likely` in both parser traversals, and set the value-required flag around the operand check in `checker` so the match is checked as an expression (its bool arms infer their own type). Cover `dump(match ...)` and `_likely_(match ...)` in both regression tests (dump only in vlib/v, since its stderr output would perturb the v3 exact-output comparison). Verified v3 self-hosts and compiler_errors_test plus the dump/likely/match suites are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58fba1406f
ℹ️ 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".
| ast.ConcatExpr { | ||
| // last stmt on block is: a, b, c := ret1(), ret2(), ret3() | ||
| for mut val in expr.vals { | ||
| if mut val is ast.CallExpr { | ||
| val.is_return_used = true | ||
| } |
There was a problem hiding this comment.
Fresh evidence beyond the previously handled container and wrapper spellings is a multi-return branch such as a, b := if value := node { match value { First { lower_first(value)! } Second { lower_second(value)! } }, 9 } else { 0, 0 }. The ConcatExpr handler marks only values that are direct calls, so it never reaches the nested match; concat_expr then checks that match under the inferred branch's void context, rejects its result as void in a multi-return, and leaves its propagated arm calls return-unused. Recurse through every concatenated value and add regression coverage for this valid multi-return form.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4095e5f. The ConcatExpr parser handler now recurses into every value via mark_last_call_expr_return_as_used (a strict superset of the old direct-call-only marking), so a nested/wrapped match/if is reached. And concat_expr sets the value-required flag around each value check, so a value match/if is checked as an expression (its arms infer their own type) instead of being rejected as void.
Verified on the main compiler: match ..., 9, 9, match ..., and match ..., match ... all compile+run. Regression test covers it.
Heads-up: v3 has separate pre-existing gaps for this spelling — it mis-lowers a match in a multi-return even in the direct form (a, b := match ..., 9), and additionally misparses the multi-return match inside an if-branch — so I scoped the v3 regression out for this batch and left those as a follow-up. compiler_errors_test + the multi-return/match suites are unchanged.
…gation (#28000) Address PR review: a multi-return branch such as `a, b := if value := node { match value { First { lower_first(value)! } ... }, 9 } else { 0, 0 }` was rejected ("type `void` cannot be used in multi-return"). The `ConcatExpr` parser handler marked only values that were direct calls, never reaching a nested match, and `concat_expr` checked the match under the branch's void context. - vlib/v parser: `mark_last_call_expr_return_as_used`'s `ConcatExpr` case now recurses into every value via the helper (a strict superset of the previous direct-call marking), so nested/wrapped match/if/call values are marked. - vlib/v checker: `concat_expr` sets the value-required flag around each value check, so a value match/if is checked as an expression (its arms infer their own type) instead of being typed `void`. Cover `match ..., 9` in the vlib/v regression test. compiler_errors_test plus the multi-return/match/option suites are unchanged. (v3 has separate pre-existing gaps for this spelling — it mis-lowers a match in a multi-return even in the direct form, and misparses the multi-return match in an if-branch — so the v3 regression is left for a follow-up.)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4095e5f26b
ℹ️ 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".
| for mut arg in expr.args { | ||
| if p.expr_contains_value_match_or_if(arg.expr) { | ||
| p.mark_last_call_expr_return_as_used(mut arg.expr) | ||
| } |
There was a problem hiding this comment.
When an inferred branch ends in (match value { First { make_first(value)! } Second { make_second(value)! } }).get(), the match is stored in CallExpr.left, but this case traverses only args. The selector-field fix does not cover method syntax: checker/fn.v:1353-1365 checks the receiver with ordinary c.expr under the branch's void context, so the match is treated as a statement and its propagating arm calls remain return-unused. Recurse through call receivers containing value matches and add a regression for this form.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9c8e75b. Both CallExpr parser handlers now recurse through a method-call receiver (expr.is_method && ... expr.left) — mark_last_call_expr_return_as_used and expr_contains_value_match_or_if. In practice the receiver typechecks fine via method resolution (my repro reached cgen and emitted the empty _t = ;, rather than a checker void error), so only the parser marking was missing on the main compiler. (match ...).get() now compiles+runs; regression covers it.
Heads-up: v3's method-call transform mis-lowers a match receiver (separate pre-existing v3 gap), so I scoped the v3 regression out for this one.
| if p.expr_contains_value_match_or_if(expr.index) { | ||
| p.mark_last_call_expr_return_as_used(mut expr.index) |
There was a problem hiding this comment.
When an inferred branch ends in a slice such as values[(match value { First { lower_first(value)! } Second { lower_second(value)! } })..], expr.index is an ast.RangeExpr, so this predicate returns false and the marking visitor never reaches the lower bound. In addition, checker/checker.v:8545-8552 checks range bounds with ordinary c.expr, leaving the nested match in the surrounding void context; valid code is therefore rejected as a void index or retains the same return-unused propagation bug. Recurse through both RangeExpr bounds, provide their value context, and add regression coverage.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9c8e75b. Added an ast.RangeExpr case to both parser traversals (recursing into low/high), and index_expr now sets the value-required flag around each range-bound check, so a value match/if bound is checked as an expression instead of a void index.
Verified values[(match ...)..] compiles+runs on both backends (v3 already handled the range-bound lowering, so it needed no code change). Regression tests cover it in both. compiler_errors_test + the index/slice suites are unchanged.
…values (#28000) Addresses two PR review comments: 1. Method-call receiver: `(match value { First { make_first(value)! } ... }).get()` stores the match in `CallExpr.left`, which the `CallExpr` parser handlers traversed only for `args`. `mark_last_call_expr_return_as_used` and `expr_contains_value_match_or_if` now also recurse through a method-call receiver. (The receiver typechecks via method resolution, so no checker change was needed — only the parser marking was missing.) 2. Slice bound: `values[(match value { First { lower_first(value)! } ... })..]` stores the match in a `RangeExpr` bound, which both parser traversals fell through. Added a `RangeExpr` case to both (recursing into `low`/`high`), and `index_expr` now sets the value-required flag around each range-bound check so a value match/if is checked as an expression instead of a void index. Cover `(match ...).get()` and `values[(match ...)..]` in the vlib/v regression test (and the slice bound in the v3 test). compiler_errors_test plus the index/slice/method/call suites are unchanged. (v3's slice bound already works; v3's method-call receiver mis-lowers a match — a separate pre-existing v3 gap — so its regression is left for a follow-up.)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c8e75b12b
ℹ️ 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".
| // into the wrapped operand. | ||
| p.mark_last_call_expr_return_as_used(mut expr.expr) | ||
| } | ||
| else {} |
There was a problem hiding this comment.
When an inferred assignment branch ends in lock state { match value { First { lower_first(value)! } Second { lower_second(value)! } } }, the new visitor falls through for ast.LockExpr, so it never marks the match-arm calls as return-used. lock_expr also checks its tail under the surrounding void expected type, causing the nested match to remain a statement; the valid lock value is therefore rejected or reaches the same empty propagated-value codegen path. Recurse into the lock's final statement and preserve its value context, with a regression test for this form.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
| else { | ||
| false |
There was a problem hiding this comment.
When a block-value call uses a decomposed array argument such as sum(...[match value { First { lower_first(value)! } Second { lower_second(value)! } }]), the argument is an ast.ArrayDecompose, so this predicate returns false before reaching the contained array and match. The match-arm calls remain return-unused and C generation can emit the same empty match-value assignment; recurse through ArrayDecompose.expr in both parser helpers and add propagation coverage for a variadic call.
AGENTS.md reference: AGENTS.md:L652-L658
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c8e75b12b
ℹ️ 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".
| ast.ArrayInit { | ||
| // last stmt on block is an array literal, e.g. `[match value { .. }]` | ||
| // or a spread `[...(match value { .. })]`; mark any element or the | ||
| // spread operand that is (or nests) a block-value match/if. | ||
| for mut element in expr.exprs { |
There was a problem hiding this comment.
When an assignment-RHS branch ends in an initializer such as []int{len: 1, init: match value { First { lower_first(value)! } Second { lower_second(value)! } }}, the match is stored in ArrayInit.init_expr, but this case visits only exprs and update_expr. Although check_array_init_default_expr supplies the element type and treats the match as a value, its arm calls remain is_return_used == false, so propagation can recreate the invalid empty C temporary assignment; include init_expr in both parser traversals and add regression coverage.
AGENTS.md reference: AGENTS.md:L657-L658
Useful? React with 👍 / 👎.
|
Closing in favor of #28011, which keeps only the v3 backend fixes (the |
1 participant
Fix #28000
A
matchwhose arms use!/?error-propagation, when thematchis used as the value of anif-expression (e.g. anif value := opt { … } else { … }option-guard assigned to a variable), emitted invalid C:The if-expression's result temp was never assigned the propagated value.
Root cause
Parser.mark_last_call_return_as_used(vlib/v/parser/parser.v) is called for the last statement of an assignment-RHS block and recursively descends intoifbranches to flag the last call's return value as used (is_return_used = true). It had a case forIfExprbut no case forMatchExpr, so calls in match arms keptis_return_used == false.In cgen (
vlib/v/gen/c/fn.v), the branch that emits the unwrapped value of a propagated call (_t4 = (*(int*)_t5.data);) is guarded bynode.is_return_used || is_gen_or_and_assign_rhs. With neither set, cgen wrote only_t4 =followed by nothing.Fix
Add a
MatchExprcase symmetric to the existingIfExprone, marking the last statement of each match branch.Reproduction (from the issue)
Now compiles and runs correctly.
Tests
vlib/v/tests/match_as_if_expr_value_with_propagation_test.v— covers!and?propagation for a match inside an if-guard, as a direct return value, and assigned to a variable.vlib/v3/tests/match_as_if_expr_value_propagation_codegen_test.v— the v3 backend already handled this case correctly; a regression test is added to keep it covered.Existing
vlib/v/compiler_errors_test.v(1617 checker snapshots) and the related match/option/result/or-block test suites pass unchanged.