Repository navigation
checker: fix return if fn multi-return - #27347
Conversation
![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: 574b9591e0
ℹ️ 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 c.inside_return && !c.expected_type.has_option_or_result() | ||
| && c.expected_or_type == ast.void_type && expected_sym.info is ast.MultiReturn |
There was a problem hiding this comment.
When the return if is nested in an or {} block, return_stmt() only resets c.expected_type to the enclosing function return type; c.expected_or_type remains the or-block default type set while checking the or block. This guard therefore skips the new multi-return normalization for cases like _ := maybe() or { return if ok { 1, noop } else { 2, noop } } in a function returning (int, fn () int), so the bare function symbol still registers a raw tuple type and the cgen failure this patch is meant to fix remains.
Useful? React with 👍 / 👎.
574b959 to
d88dfa0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d88dfa03ad
ℹ️ 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 c.inside_return && !c.expected_type.has_option_or_result() | ||
| && expected_sym.info is ast.MultiReturn && expected_sym.info.types.len == mr_types.len { |
There was a problem hiding this comment.
For functions declared like fn f() !(int, fn () int) or ?(int, fn () int), a successful return if cond { 1, noop } else { ... } still has c.expected_type flagged as Result/Option, so this guard skips the new normalization even though expected_multi_return_type has already cleared the flag. The branch then falls through to registering the raw tuple type such as (int_literal, main__noop), leaving the same invalid function-symbol multi-return typedef that this change is meant to avoid. Please normalize against the cleared payload type for these return paths too.
Useful? React with 👍 / 👎.
d88dfa0 to
0b49020
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b49020b74
ℹ️ 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 c.inside_return && expected_sym.info is ast.MultiReturn | ||
| && expected_sym.info.types.len == mr_types.len { | ||
| mut use_expected_type := true | ||
| for i, expected_typ in expected_sym.info.types { | ||
| if !c.check_types(mr_types[i], expected_typ) { |
There was a problem hiding this comment.
When this compatibility check runs before the existing void-element rejection below, a return-position if/match comma expression can hide a void value whenever the expected slot is a permissive type like voidptr or any. For example, fn f(x bool) (voidptr, int) { return if x { print('a'), 1 } else { voidptr(0), 2 } } now passes because check_types(void, voidptr) succeeds, so node.return_type is set to the expected multi-return type and the later type void cannot be used in multi-return check is skipped; cgen then has to emit print() as .arg0, which is invalid.
Useful? React with 👍 / 👎.
0b49020 to
54ab762
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54ab7628bc
ℹ️ 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".
| && expected_sym.info.types.len == mr_types.len { | ||
| mut use_expected_type := true | ||
| for i, expected_typ in expected_sym.info.types { | ||
| if !c.check_types(mr_types[i], expected_typ) { |
There was a problem hiding this comment.
When a return if branch supplies a numeric variable for a pointer slot, e.g. fn f(x bool, n u64) (voidptr, int) { return if x { n, 1 } else { voidptr(0), 2 } }, this shortcut treats the branch as the expected (voidptr, int) because check_types() accepts pointer/number pairs. That prevents return_stmt from seeing the actual u64 element and running its stricter pointer-return checks, so code that direct return n, 1 would reject is accepted for return if.
Useful? React with 👍 / 👎.
54ab762 to
7ede2c6
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ede2c6fc7
ℹ️ 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 !c.can_use_expected_multi_return_value_type(mr_types[i], expected_typ, | ||
| node.vals[i]) { |
There was a problem hiding this comment.
When a return-position comma expression matches the expected multi-return arity, this branch now replaces the raw element types with the expected tuple type based on check_types. Since check_types treats ?T/!T as compatible with T when their base type idx matches, fn maybe() ?int { ... }; fn f() (int, int) { return maybe(), 1 } can be accepted here and return_stmt later only sees (int, int), so its existing got_type.has_flag(.option/.result) diagnostics are bypassed. Keep option/result flags from the raw values, or reject them explicitly before returning the expected multi-return type.
Useful? React with 👍 / 👎.
7ede2c6 to
ee49475
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Fixes a cgen failure for
return ifmulti-return expressions where one returned value is a bare function symbol.The checker now uses the expected plain multi-return type for return-position comma expressions when the raw values are assignable to that type, while preserving raw element types whenever return checking needs stricter diagnostics. This prevents registering an unused raw tuple type such as
(int_literal, main__noop_cmd), which later caused cgen to emit an invalid C field type.Regression coverage includes returning
(int, fn () int)throughreturn if, the same shape nested inside anor { return if ... }block, andreturn iffrom!(int, fn () int)/?(int, fn () int)functions. Checker error fixtures also cover preserving existingvoidelement, pointer/number, and option/result element rejection before expected tuple reuse runs forifandmatchreturn expressions.Validation
./v -g -keepc -o ./vnew cmd/v./vnew -silent test vlib/v/tests/fns/fn_multiple_returns_test.v./vnew run, printing1and0./vnew -silent test vlib/v/checker/./vnew -silent vlib/v/compiler_errors_test.vgit diff --check -- vlib/v/checker/checker.v vlib/v/checker/if.v vlib/v/checker/match.v vlib/v/tests/fns/fn_multiple_returns_test.v vlib/v/checker/tests/multi_return_use_void_type_err.vv vlib/v/checker/tests/multi_return_use_void_type_err.out vlib/v/checker/tests/multi_return_pointer_number_return_expr_err.vv vlib/v/checker/tests/multi_return_pointer_number_return_expr_err.out vlib/v/checker/tests/multi_return_option_result_return_expr_err.vv vlib/v/checker/tests/multi_return_option_result_return_expr_err.outNote: a broader
./vnew -silent test vlib/v/attempt did not complete cleanly due unrelated worktree/test issues, including abuilder_teststdout mismatch from atcc compilation failed, falling back to ccwarning.