-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
parser: fix match-arm !/? propagation when a match is used as a block value (fix #28000)
#28002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
fab69be
0efc15b
439a513
1546c32
b210e50
77a271a
b3b5a13
e3c7894
f46183f
eb4b179
353a90a
23236ba
9225506
7020743
d61a5db
be7a393
6bb7234
58fba14
4095e5f
9c8e75b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -578,70 +578,84 @@ fn (mut p Parser) parse_block_no_scope(is_top_level bool) []ast.Stmt { | |
| } | ||
|
|
||
| fn (mut p Parser) mark_last_call_return_as_used(mut last_stmt ast.Stmt) { | ||
| match mut last_stmt { | ||
| ast.ExprStmt { | ||
| match mut last_stmt.expr { | ||
| ast.CallExpr { | ||
| // last stmt on block is CallExpr | ||
| last_stmt.expr.is_return_used = true | ||
| if last_stmt.expr.or_block.stmts.len > 0 { | ||
| mut or_block_last_stmt := last_stmt.expr.or_block.stmts.last() | ||
| p.mark_last_call_return_as_used(mut or_block_last_stmt) | ||
| } | ||
| if mut last_stmt is ast.ExprStmt { | ||
| p.mark_last_call_expr_return_as_used(mut last_stmt.expr) | ||
| } | ||
| } | ||
|
|
||
| fn (mut p Parser) mark_last_call_expr_return_as_used(mut expr ast.Expr) { | ||
| match mut expr { | ||
| ast.CallExpr { | ||
| // last stmt on block is CallExpr | ||
| expr.is_return_used = true | ||
|
Comment on lines
+722
to
+724
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an assignment-RHS block ends with a call such as AGENTS.md reference: AGENTS.md:L657-L658 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in b3b5a13. The 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 |
||
| 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) | ||
| } | ||
| } | ||
| 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 | ||
| } | ||
| ast.ConcatExpr { | ||
| // last stmt on block is: a, b, c := ret1(), ret2(), ret3() | ||
| for mut expr in last_stmt.expr.vals { | ||
| if mut expr is ast.CallExpr { | ||
| expr.is_return_used = true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ast.ParExpr { | ||
| // last stmt on block is parenthesized: ( match .. { a { foo() } } ) | ||
| p.mark_last_call_expr_return_as_used(mut expr.expr) | ||
|
Comment on lines
+834
to
+836
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an assignment-RHS branch uses a value such as AGENTS.md reference: AGENTS.md:L652-L658 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 439a513 — and this one needed fixing in both backends:
Both regression tests now cover |
||
| } | ||
| ast.IfExpr { | ||
| // last stmt on block is: if .. { foo() } else { bar() } | ||
| for mut branch in expr.branches { | ||
| if branch.stmts.len > 0 { | ||
| mut last_if_stmt := branch.stmts.last() | ||
| p.mark_last_call_return_as_used(mut last_if_stmt) | ||
| } | ||
| ast.IfExpr { | ||
| // last stmt on block is: if .. { foo() } else { bar() } | ||
| for mut branch in last_stmt.expr.branches { | ||
| if branch.stmts.len > 0 { | ||
| mut last_if_stmt := branch.stmts.last() | ||
| p.mark_last_call_return_as_used(mut last_if_stmt) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ast.MatchExpr { | ||
| // last stmt on block is: match .. { a { foo() } b { bar() } } | ||
| for mut branch in expr.branches { | ||
| if branch.stmts.len > 0 { | ||
| mut last_match_stmt := branch.stmts.last() | ||
| p.mark_last_call_return_as_used(mut last_match_stmt) | ||
| } | ||
| ast.InfixExpr { | ||
| if last_stmt.expr.or_block.stmts.len > 0 { | ||
| mut or_block_last_stmt := last_stmt.expr.or_block.stmts.last() | ||
| } | ||
| } | ||
| ast.InfixExpr { | ||
| 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) | ||
| } | ||
| // last stmt has infix expr with CallExpr: foo()? + 'a' | ||
| mut left_expr := expr.left | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an assignment-RHS block ends in an expression such as AGENTS.md reference: AGENTS.md:L657-L658 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 77a271a. Replaced the left-only Two extra things the composition surfaced:
Both regression tests now cover |
||
| for { | ||
| mut next_left_expr := ast.Expr(ast.EmptyExpr{}) | ||
| if mut left_expr is ast.InfixExpr { | ||
| if left_expr.or_block.stmts.len > 0 { | ||
| mut or_block_last_stmt := left_expr.or_block.stmts.last() | ||
| p.mark_last_call_return_as_used(mut or_block_last_stmt) | ||
| } | ||
| // last stmt has infix expr with CallExpr: foo()? + 'a' | ||
| mut left_expr := last_stmt.expr.left | ||
| for { | ||
| mut next_left_expr := ast.Expr(ast.EmptyExpr{}) | ||
| if mut left_expr is ast.InfixExpr { | ||
| if left_expr.or_block.stmts.len > 0 { | ||
| mut or_block_last_stmt := left_expr.or_block.stmts.last() | ||
| p.mark_last_call_return_as_used(mut or_block_last_stmt) | ||
| } | ||
| next_left_expr = left_expr.left | ||
| } else if mut left_expr is ast.CallExpr { | ||
| left_expr.is_return_used = true | ||
| if left_expr.or_block.stmts.len > 0 { | ||
| mut or_block_last_stmt := left_expr.or_block.stmts.last() | ||
| p.mark_last_call_return_as_used(mut or_block_last_stmt) | ||
| } | ||
| break | ||
| } else { | ||
| break | ||
| } | ||
| left_expr = next_left_expr | ||
| continue | ||
| } | ||
| } | ||
| ast.ComptimeCall, ast.ComptimeSelector, ast.PrefixExpr, ast.SelectorExpr { | ||
| if last_stmt.expr.or_block.stmts.len > 0 { | ||
| mut or_block_last_stmt := last_stmt.expr.or_block.stmts.last() | ||
| next_left_expr = left_expr.left | ||
| } else if mut left_expr is ast.CallExpr { | ||
| left_expr.is_return_used = true | ||
| if left_expr.or_block.stmts.len > 0 { | ||
| mut or_block_last_stmt := left_expr.or_block.stmts.last() | ||
| p.mark_last_call_return_as_used(mut or_block_last_stmt) | ||
| } | ||
| break | ||
| } else { | ||
| break | ||
| } | ||
| else {} | ||
| left_expr = next_left_expr | ||
| continue | ||
| } | ||
| } | ||
| ast.ComptimeCall, ast.ComptimeSelector, ast.PrefixExpr, ast.SelectorExpr { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In the current AGENTS.md reference: AGENTS.md:L657-L658 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 9225506. Three parts, since (unlike the array/map/struct cases) v3 also needed a change here:
Verified: |
||
| 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) | ||
| } | ||
| } | ||
| else {} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the block tail wraps the match in an AGENTS.md reference: AGENTS.md:L652-L658 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
vlib/v3
Both regression tests now cover There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an assignment-RHS branch ends in AGENTS.md reference: AGENTS.md:L657-L658 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 58fba14. Verified There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an inferred assignment branch ends in AGENTS.md reference: AGENTS.md:L652-L658 Useful? React with 👍 / 👎. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| // Regression test for https://github.com/vlang/v/issues/28000 | ||
| // A `match` whose arms use `!`/`?` propagation, used as the value of an | ||
| // `if`-expression (or directly), used to emit invalid C (`_t = ;`) because | ||
| // the calls in the match arms were not marked as having their return used. | ||
|
|
||
| struct First {} | ||
|
|
||
| struct Second {} | ||
|
|
||
| type Node = First | Second | ||
|
|
||
| fn lower_first(_ First) !int { | ||
| return 1 | ||
| } | ||
|
|
||
| fn lower_second(_ Second) !int { | ||
| return 2 | ||
| } | ||
|
|
||
| fn opt_first(_ First) ?int { | ||
| return 10 | ||
| } | ||
|
|
||
| fn opt_second(_ Second) ?int { | ||
| return 20 | ||
| } | ||
|
|
||
| // match inside an if-guard, assigned to a variable (the original repro) | ||
| fn select_value(node ?Node) !int { | ||
| result := if value := node { | ||
| match value { | ||
| First { lower_first(value)! } | ||
| Second { lower_second(value)! } | ||
| } | ||
| } else { | ||
| 0 | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| // parenthesized match value: `( match .. { .. } )` keeps an ast.ParExpr wrapper | ||
| fn select_value_paren(node ?Node) !int { | ||
| result := if value := node { | ||
| (match value { | ||
| First { lower_first(value)! } | ||
| Second { lower_second(value)! } | ||
| }) | ||
| } else { | ||
| 0 | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| // match with `?` option propagation | ||
| fn select_opt(node ?Node) ?int { | ||
| result := if value := node { | ||
| match value { | ||
| First { opt_first(value)? } | ||
| Second { opt_second(value)? } | ||
| } | ||
| } else { | ||
| 0 | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| // match used directly as the return value | ||
| fn direct_match(node Node) !int { | ||
| return match node { | ||
| First { lower_first(node)! } | ||
| Second { lower_second(node)! } | ||
| } | ||
| } | ||
|
|
||
| // match assigned directly to a variable | ||
| fn assign_match(node Node) !int { | ||
| x := match node { | ||
| First { lower_first(node)! } | ||
| Second { lower_second(node)! } | ||
| } | ||
| return x | ||
| } | ||
|
|
||
| fn test_match_as_if_expr_value_with_propagation() { | ||
| assert select_value(First{})! == 1 | ||
| assert select_value(Second{})! == 2 | ||
| assert select_value(none) or { -1 } == 0 | ||
| } | ||
|
|
||
| fn test_parenthesized_match_as_if_expr_value_with_propagation() { | ||
| assert select_value_paren(First{})! == 1 | ||
| assert select_value_paren(Second{})! == 2 | ||
| assert select_value_paren(none) or { -1 } == 0 | ||
| } | ||
|
|
||
| fn test_match_as_if_expr_value_with_option_propagation() { | ||
| assert select_opt(First{})? == 10 | ||
| assert select_opt(Second{})? == 20 | ||
| assert select_opt(none) or { -1 } == 0 | ||
| } | ||
|
|
||
| fn test_match_as_return_and_assign_value_with_propagation() { | ||
| assert direct_match(First{})! == 1 | ||
| assert direct_match(Second{})! == 2 | ||
| assert assign_match(First{})! == 1 | ||
| assert assign_match(Second{})! == 2 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| // Regression test for https://github.com/vlang/v/issues/28000 | ||
| // A `match` whose arms use `!`/`?` propagation, used as the value of an | ||
| // `if`-expression, must assign the unwrapped result to the if-expression's | ||
| // result temp (it used to emit an empty expression `_t = ;`). | ||
| import os | ||
|
|
||
| const vexe = @VEXE | ||
| const tests_dir = os.dir(@FILE) | ||
| const v3_dir = os.dir(tests_dir) | ||
| const vlib_dir = os.dir(v3_dir) | ||
| const v3_src = os.join_path(v3_dir, 'v3.v') | ||
|
|
||
| fn test_match_as_if_expr_value_with_propagation() { | ||
| v3_bin := os.join_path(os.temp_dir(), 'v3_match_as_if_expr_value_propagation_test') | ||
| build := | ||
| os.execute('${vexe} -gc none -path "${vlib_dir}|@vlib|@vmodules" -o ${v3_bin} ${v3_src}') | ||
| assert build.exit_code == 0, build.output | ||
|
|
||
| src := os.join_path(os.temp_dir(), 'v3_match_as_if_expr_value_propagation_input.v') | ||
| os.write_file(src, 'module main | ||
|
|
||
| struct First {} | ||
| struct Second {} | ||
|
|
||
| type Node = First | Second | ||
|
|
||
| fn lower_first(_ First) !int { | ||
| return 1 | ||
| } | ||
|
|
||
| fn lower_second(_ Second) !int { | ||
| return 2 | ||
| } | ||
|
|
||
| fn select_value(node ?Node) !int { | ||
| result := if value := node { | ||
| match value { | ||
| First { lower_first(value)! } | ||
| Second { lower_second(value)! } | ||
| } | ||
| } else { | ||
| 0 | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| fn select_value_paren(node ?Node) !int { | ||
| result := if value := node { | ||
| (match value { | ||
| First { lower_first(value)! } | ||
| Second { lower_second(value)! } | ||
| }) | ||
| } else { | ||
| 0 | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| fn direct_match(node Node) !int { | ||
| return match node { | ||
| First { lower_first(node)! } | ||
| Second { lower_second(node)! } | ||
| } | ||
| } | ||
|
|
||
| fn main() { | ||
| println(select_value(First{})!) | ||
| println(select_value(Second{})!) | ||
| println(select_value_paren(First{})!) | ||
| println(direct_match(Second{})!) | ||
| } | ||
| ') or { | ||
| panic(err) | ||
| } | ||
|
|
||
| bin := os.join_path(os.temp_dir(), 'v3_match_as_if_expr_value_propagation_out') | ||
| compile := os.execute('${v3_bin} ${src} -b c -o ${bin}') | ||
| assert compile.exit_code == 0, compile.output | ||
| assert !compile.output.contains('C compilation failed'), compile.output | ||
|
|
||
| run := os.execute(bin) | ||
| assert run.exit_code == 0, run.output | ||
| assert run.output.trim_space() == '1\n2\n1\n2' | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 noast.ArrayInitcase, so it never reaches the match or marks its arm calls as return-used. Because match-arm blocks are parsed afterinside_assign_rhsis 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in eb4b179. This one needed both a parser and a checker change:
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).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 ...].