cgen: unwrap option smartcast for interface casts#27352
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: 8200199bac
ℹ️ 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".
| && resolved_var.smartcasts.len == 0 && resolved_var.ct_type_var != .smartcast | ||
| && !g.inside_opt_or_res && !g.is_assign_lhs && !g.inside_selector_lhs | ||
| && !g.right_is_opt && (!g.left_is_opt || g.inside_cast) { | ||
| g.unwrap_option_type(runtime_option_type, name, is_auto_heap) |
There was a problem hiding this comment.
When this new path is hit for an option variable captured by an anonymous function, name is still only the bare C variable name, while inherited variables need the closure_ctx-> prefix that the normal ident path adds later. For example, a closure that captures x ?T, checks if x == none { return ... }, and then calls use(x) will emit an unwrap like x.data instead of closure_ctx->x.data, so the generated C refers to a non-existent local rather than the captured field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 544b0d077e
ℹ️ 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".
| resolved_arg_type := g.unwrap_generic(g.recheck_concrete_type(resolved_arg_typ)) | ||
| skip_inherited_option_storage_type := arg.expr.obj is ast.Var | ||
| && arg.expr.obj.is_inherited && !expected_type.has_option_or_result() | ||
| && arg_typ != 0 && !arg_typ.has_option_or_result() | ||
| && resolved_arg_type.has_option_or_result() | ||
| if !skip_inherited_option_storage_type { | ||
| arg_typ = resolved_arg_type |
There was a problem hiding this comment.
When this path runs inside a generic closure, the skip only protects the first resolved_expr_type assignment; the later generic-context resolution below still runs for inherited idents and overwrites arg_typ back to the captured option storage type. For a captured x ?T after if x == none { return }, calling a generic/value helper with x then reaches the final arg_typ.has_flag(.option) branch and expr_with_opt suppresses the ident unwrapping, so the generated call receives the option wrapper instead of the T payload. Apply the same skip to the later generic resolution as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f46130841
ℹ️ 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 has_resolved_var && runtime_option_type != ast.no_type && !node_info_is_option | ||
| && resolved_var.smartcasts.len == 0 && resolved_var.ct_type_var != .smartcast | ||
| && !g.inside_opt_or_res && !g.is_assign_lhs && !g.inside_selector_lhs | ||
| && !g.right_is_opt && (!g.left_is_opt || g.inside_cast) { |
There was a problem hiding this comment.
When a guarded option is used in an implicit interface conversion (fn take(i I) then take(x) after if x == none { return }, or appending x to []I), the call/array paths go through expr_with_cast rather than cast_expr, so call_cfn_for_casting_expr sets left_is_opt while g.inside_cast remains false. This guard then skips the new unwrap and emits the interface conversion around the _option_T storage even though the checked type is the payload T, causing bad C or an interface containing the option wrapper; the added explicit I(x) test passes only because explicit casts set inside_cast.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d44410a1c
ℹ️ 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 has_resolved_var && runtime_option_type != ast.no_type && !node_info_is_option | ||
| && resolved_var.smartcasts.len == 0 && resolved_var.ct_type_var != .smartcast | ||
| && !g.inside_opt_or_res && !g.is_assign_lhs && !g.inside_selector_lhs | ||
| && !g.right_is_opt && (!g.left_is_opt || g.inside_cast || g.inside_interface_cast) { |
There was a problem hiding this comment.
When the option is narrowed in a normal if x != none { ... } branch, the scope variable still has smartcasts.len > 0, so this new unwrap path is skipped and the earlier node_info_is_option branch can return the option wrapper unchanged while call_cfn_for_casting_expr has set left_is_opt. In that context, Issue27340Value(x) or take_interface(x) inside the positive branch still casts _option_T storage rather than the T payload; the new tests only cover the fallthrough form if x == none { return } where this condition can be reached.
Useful? React with 👍 / 👎.
1 participant
Fixes #27340.
Summary
noneguard and is used as the payload value.datanoneguardTests
./v -g -keepc -o ./vnew cmd/v./vnew fmt -w vlib/v/gen/c/cgen.v vlib/v/tests/casts/cast_option_to_interface_test.v vlib/v/tests/options/option_none_guard_return_unwrap_test.vVFLAGS='-cc clang' ./vnew test vlib/v/tests/options/option_sumtype_test.v vlib/v/tests/options/option_sumtype_unwrap_test.v vlib/v/tests/options/option_sumtype_variant_test.v vlib/v/tests/options/option_unwrap_ifexpr_test.vVFLAGS='-cc clang' ./vnew test vlib/v/tests/casts/cast_option_to_interface_test.v vlib/v/tests/options/option_none_guard_return_unwrap_test.v vlib/v/tests/alias_cast_to_fixed_array_test.v vlib/v/tests/fns/fn_fixed_array_ret_test.v vlib/v/tests/comptime/comptime_match_assign_test.v./vnew -silent vlib/v/compiler_errors_test.v./vnew -silent vlib/v/gen/c/coutput_test.vVTEST_HIDE_OK=1 VFLAGS='-cc clang' ./vnew -silent test vlib/v/