-
-
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 9 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 |
|---|---|---|
|
|
@@ -113,20 +113,29 @@ fn (mut c Checker) infix_expr(mut node ast.InfixExpr) ast.Type { | |
| } | ||
| // In bool contexts like `assert` and `return`, short enum literals on the left | ||
| // need the right operand type first, so `.a == x` resolves `.a` correctly. | ||
| mut check_right_type_first_for_left_short_enum := false | ||
| mut check_right_type_first := false | ||
| if node.op in [.eq, .ne] && node.left is ast.EnumVal { | ||
| left_enum := node.left as ast.EnumVal | ||
| if left_enum.enum_name.len == 0 { | ||
| if node.right is ast.EnumVal { | ||
| right_enum := node.right as ast.EnumVal | ||
| check_right_type_first_for_left_short_enum = right_enum.enum_name.len > 0 | ||
| check_right_type_first = right_enum.enum_name.len > 0 | ||
| } else { | ||
| check_right_type_first_for_left_short_enum = true | ||
| check_right_type_first = true | ||
| } | ||
| } | ||
| } | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an inferred branch tail uses a membership 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 d61a5db. You're right that imposing the right operand's container type is wrong — I switched Confirmed the concrete- |
||
| // A `match`/`if` value operand on the left, e.g. `(match x { ... }) + 1`, | ||
| // would otherwise be checked with the (void) surrounding expected type and | ||
| // mistyped as a statement (e.g. when nested inside an if-branch). Resolve | ||
| // the right operand's type first and use it as the expected type so the | ||
| // left operand is checked as a value expression. | ||
| check_right_type_first = true | ||
| } | ||
| mut right_type := ast.void_type | ||
| if check_right_type_first_for_left_short_enum { | ||
| if check_right_type_first { | ||
| right_type = c.expr(mut node.right) | ||
| if right_type == ast.no_type { | ||
| node.right_type = right_type | ||
|
|
@@ -229,7 +238,7 @@ fn (mut c Checker) infix_expr(mut node ast.InfixExpr) ast.Type { | |
| } | ||
| } | ||
| } | ||
| if !check_right_type_first_for_left_short_enum { | ||
| if !check_right_type_first { | ||
| right_type = c.expr(mut node.right) | ||
| if right_type == ast.no_type { | ||
| node.right_type = right_type | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -578,70 +578,131 @@ 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) | ||
| } | ||
| } | ||
|
|
||
| // expr_contains_value_match_or_if reports whether an expression is (or, through | ||
| // transparent `(...)`/`unsafe { }`/cast/`as`-cast wrappers and nested call | ||
| // arguments, contains) a `match`/`if` value expression. Used to decide whether a | ||
| // call argument holds a block-value match/if whose arm calls must be marked as | ||
| // return-used, including nested compositions like `wrap(wrap(match value { .. }))`. | ||
| fn (p &Parser) expr_contains_value_match_or_if(expr ast.Expr) bool { | ||
| return match expr { | ||
| ast.MatchExpr, ast.IfExpr { | ||
| true | ||
| } | ||
| ast.ParExpr { | ||
| p.expr_contains_value_match_or_if(expr.expr) | ||
| } | ||
| ast.UnsafeExpr { | ||
| p.expr_contains_value_match_or_if(expr.expr) | ||
| } | ||
| ast.CastExpr { | ||
| p.expr_contains_value_match_or_if(expr.expr) | ||
| } | ||
| ast.AsCast { | ||
| p.expr_contains_value_match_or_if(expr.expr) | ||
| } | ||
| ast.CallExpr { | ||
| mut found := false | ||
| for arg in expr.args { | ||
| if p.expr_contains_value_match_or_if(arg.expr) { | ||
| found = true | ||
| break | ||
| } | ||
| 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 | ||
| } | ||
| } | ||
| } | ||
| found | ||
| } | ||
| ast.InfixExpr { | ||
| // e.g. `1 + (match value { .. })` as a call argument. | ||
|
|
||
| p.expr_contains_value_match_or_if(expr.left) | ||
| || p.expr_contains_value_match_or_if(expr.right) | ||
| } | ||
| else { | ||
| false | ||
|
Comment on lines
+714
to
+715
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 an index 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 7020743. Three parts (v3 also needed a change, like the prefix case):
Verified:
Comment on lines
+714
to
+715
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 branch ends in a string interpolation 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 6bb7234. Both parser traversals now recurse through Verified
Comment on lines
+714
to
+715
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: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. Two parts: Verified
Comment on lines
+714
to
+715
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 a block-value call uses a decomposed array argument such as AGENTS.md reference: AGENTS.md:L652-L658 Useful? React with 👍 / 👎. |
||
| } | ||
|
Comment on lines
+714
to
+716
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 a block-value call argument contains the match inside an infix expression, 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 f46183f. Added an Verified compositions: Both regression tests now cover
Comment on lines
+714
to
+716
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 with a map literal 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 23236ba. Two parts, like the array-literal case:
Verified: One scope note: a propagating match in a map key inside a void-context if-branch ( |
||
| } | ||
| } | ||
|
|
||
| fn (mut p Parser) mark_last_call_expr_return_as_used(mut expr ast.Expr) { | ||
|
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.
Fresh evidence beyond the already-fixed call/infix spellings is an assignment-RHS branch ending 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 eb4b179. This one needed both a parser and a checker change:
Verified: Both regression tests now cover |
||
| 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) | ||
| } | ||
| // an argument may itself be (or nest) a block-value match/if, e.g. | ||
| // `wrap(match value { First { foo()! } })` or | ||
| // `wrap(wrap(match value { .. }))`; mark its arm calls too. | ||
| 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) | ||
| } | ||
|
Comment on lines
+732
to
+735
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 branch ends in 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 9c8e75b. Both 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. |
||
| 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.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.InfixExpr { | ||
| 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) | ||
| } | ||
| // 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.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.UnsafeExpr { | ||
| // last stmt on block is unsafe-wrapped: unsafe { match .. { a { foo() } } } | ||
| p.mark_last_call_expr_return_as_used(mut expr.expr) | ||
| } | ||
| ast.CastExpr { | ||
| // last stmt on block is cast-wrapped: i64(match .. { a { foo() } }) | ||
| p.mark_last_call_expr_return_as_used(mut expr.expr) | ||
| } | ||
| ast.AsCast { | ||
| // last stmt on block is as-cast-wrapped: (match .. { a { foo() } }) as T | ||
| p.mark_last_call_expr_return_as_used(mut expr.expr) | ||
| } | ||
| 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.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() | ||
| p.mark_last_call_return_as_used(mut or_block_last_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) | ||
| } | ||
| else {} | ||
| } | ||
| } | ||
| 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 value operands, e.g. | ||
| // `foo()? + 'a'` or `1 + (match value { First { bar()! } })`. | ||
| // Recurse into both sides so nested/wrapped match/if/call values on | ||
| // either operand are marked as return-used. | ||
| 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. 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 👍 / 👎. |
||
|
|
||
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.
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 peelsParExpr.cast_exprtherefore leavesexpected_typeas void;unsafe_exprforwards 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in b210e50. You're right — the predicates only peeled
(...). Recursing throughunsafe { }in both, plus one extra fix the composition surfaced:cast_operand_is_value_match_or_ifnow also recurses throughast.UnsafeExpr, soi64(unsafe { match ... })(in any paren composition) is checked as a value.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.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, andcompiler_errors_test(1617 snapshots) + the cast/sumtype/as suites are unchanged.