Repository navigation
checker: fix cgen 'pointer expected' for loop-local var passed to variadic interface arg - #27327
Conversation
…iadic interface arg When a variable declared inside a nested scope (e.g. a for loop body) was passed to a variadic interface parameter, mark_as_referenced only toggled is_auto_heap on the use-site Ident's obj copy, because c.fn_scope.find_var only locates function-level variables. The variable's declaration was then emitted as a plain value while reads of it were dereferenced, producing inconsistent pointer indirection and a 'pointer expected' C error. Resolve the canonical declaration variable via the use-site scope chain so the auto-heap promotion is recorded on the variable cgen sees at its declaration. Fixes #27326
![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: 33f0ce48a4
ℹ️ 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".
| obj = node.scope.find_var(node.obj.name) or { | ||
| if c.fn_scope != unsafe { nil } { | ||
| c.fn_scope.find_var(node.obj.name) or { obj } |
There was a problem hiding this comment.
When the argument identifier is used inside an if x is T/match smartcast branch, node.scope.find_var(...) resolves the branch's synthetic smartcast Var rather than the real declaration. Passing that smartcasted value to a variadic interface parameter then sets is_auto_heap only on the synthetic scope var while the original declaration remains a plain value; cgen also resolves the smartcast scope var and emits auto-heap/pointer access (for example x := Sum(Cat{}); if x is Cat { collect(x) } with collect(...Value)), regressing function-scope cases that the old c.fn_scope.find_var path promoted or left consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 56c0eda. mark_as_referenced now detects when node.scope.find_var resolved a synthetic smartcast var (non-empty smartcasts) and falls back to the function-scope declaration, so the is_auto_heap promotion lands on the variable cgen emits as the declaration rather than the smartcast copy. Added a if x is T/match regression test to interface_variadic_test.v; the interfaces, sumtypes and generics suites still pass.
…renced Addresses the Codex review on #27327: resolving the auto-heap target via node.scope.find_var could return a synthetic smartcast var (from an `if x is T`/`match` branch), which has no declaration of its own. Skip those and fall back to the function scope, so the is_auto_heap promotion is recorded on the variable cgen emits as the declaration. Adds a smartcast regression test to interface_variadic_test.v.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56c0eda8fb
ℹ️ 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 = if c.fn_scope != unsafe { nil } { | ||
| c.fn_scope.find_var(node.obj.name) or { obj } |
There was a problem hiding this comment.
When the variadic interface argument is both loop-local and smartcasted, e.g. for { x := ValueSum(Cat{}); if x is Cat { collect(x) } }, node.scope.find_var(...) first returns the branch's synthetic smartcast var, then this fallback only searches c.fn_scope, which cannot contain the loop-local declaration. The or { obj } path therefore promotes the synthetic var instead of the real nested declaration, leaving declaration/read pointer indirection desynchronized and the original pointer expected C failure still reachable. Fresh evidence versus the prior comment is that the current fallback handles function-scope smartcasts but still bypasses nested parent scopes after detecting smartcasts.len != 0.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 91b43b8. Replaced the c.fn_scope fallback with a new ast.Scope.find_var_decl, which walks the parent scope chain and skips synthetic smartcast vars (non-empty smartcasts). mark_as_referenced now resolves the real declaration regardless of which scope it lives in, so the loop-local + smartcast case (for { x := ValueSum(Cat{}); if x is Cat { collect(x) } }) promotes the nested declaration rather than the smartcast copy. Added a loop-local + smartcast regression test; interfaces/sumtypes (144) and generics (298) suites pass.
Addresses the Codex P2 follow-up on #27327: the previous fallback only searched c.fn_scope after detecting a smartcast var, so a variable that is both loop-local and smartcast (e.g. `for { x := ValueSum(Cat{}); if x is Cat { collect(x) } }`) had its declaration missed and the synthetic smartcast copy promoted instead. Add ast.Scope.find_var_decl, which walks the parent scope chain and skips synthetic smartcast vars (non-empty smartcasts list), so mark_as_referenced always resolves the real declaration regardless of which scope it lives in. Adds a loop-local + smartcast regression test.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91b43b89c8
ℹ️ 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 pobj.smartcasts.len == 0 { | ||
| return &pobj |
There was a problem hiding this comment.
This treats every Var with smartcasts.len > 0 as a synthetic branch copy, but real declaration scope objects can also get smartcasts applied in place, for example the option/none fallthrough path updates the current scope via scope.update_smartcasts(...). When such a variable is declared in a nested scope (e.g. inside a loop), then passed to a variadic interface argument after if x == none { continue }, this lookup skips the actual nested declaration and falls back to the function scope or an outer shadow, so is_auto_heap is not set on the declaration cgen emits and the original pointer-indirection mismatch can still occur.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 355185b. find_var_decl no longer keys off smartcasts.len alone, which conflated two cases. It now skips only true synthetic copies from if x is T/match, which are separate scope objects carrying a non-zero orig_type; a real declaration smartcast in place via update_smartcasts (e.g. an option var unwrapped by if x == none { continue }) keeps orig_type == 0 and is returned rather than skipped.
Verified with the exact example: the none-guard'd loop-local var now resolves to its real nested declaration (the lookup no longer falls back to the function scope / an outer shadow). interfaces+sumtypes (144) and generics (298) suites still pass.
Note: passing such an option-unwrapped value to a variadic interface arg additionally hits a separate, pre-existing cgen bug (the unwrapped-option .data read isn't deref'd for an auto-heap'd var) that reproduces identically on master before this PR — unrelated to the var-resolution change here, so I'll flag it as a follow-up rather than widen this PR.
…nwraps Addresses the Codex P2 follow-up on #27327: find_var_decl skipped every var with a non-empty smartcasts list, but scope.update_smartcasts mutates a real declaration in place (e.g. an option var unwrapped by `if x == none { continue }`), which would then be wrongly skipped in favor of the function scope or an outer shadow. Distinguish the two: synthetic smartcast copies from `if x is T`/`match` are separate scope objects carrying a non-zero orig_type, whereas in-place unwraps keep orig_type == 0. Skip only the former.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 355185be6a
ℹ️ 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".
| // branches: those are copies with no declaration of their own, so | ||
| // promoting them would leave the real declaration emitted as a | ||
| // plain value and desync the pointer indirection. | ||
| obj = node.scope.find_var_decl(node.obj.name) or { |
There was a problem hiding this comment.
When a smartcasted option value is passed to a variadic interface parameter, e.g. a loop-local x := maybe_cat(); if x != none { collect(x) }, this lookup skips the branch smartcast var and promotes only the real declaration. Cgen still formats the unwrapped option through the use-site Ident/inner scope smartcast (node.is_auto_heap() controls x.data vs x->data), so the declaration can become an auto-heap pointer while the branch expression is emitted with value-style option access. Fresh evidence beyond the earlier smartcast comments is that the current orig_type guard still skips ordinary option smartcast branch copies, while cgen's option unwrap path does not resolve that auto-heap flag back to the declaration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I verified this is a pre-existing cgen bug, not introduced by this PR. Passing an unwrapped option value to a variadic interface arg fails C compilation identically on master/the PR base (f5e464e) before any change here — at function scope, in loops, and for both the if x != none { ... } and if x == none { continue } forms (the unwrapped-option .data is read value-style on an auto-heap'd pointer). My var-resolution change only shifts the error text; no var-resolution choice (orig-PR node.scope.find_var, function scope, or this find_var_decl) makes these compile, because the gap is in cgen's option-unwrap deref path, not in which declaration is promoted.
The orig_type guard here is correct for what this PR targets (issue 27326: a non-option loop-local var → variadic interface), and it doesn't regress the option cases (they were already broken). Resolving the auto-heap flag back through the option-unwrap path is a separate, pre-existing cgen fix that I'll track as its own issue rather than widening this PR. 👎 on folding it in here.
There was a problem hiding this comment.
Update: I've now actually fixed this, in 72eabcd. Root cause was as you described — the declaration was auto-heap promoted but the use-site smartcast/unwrap copy that cgen resolves (resolved_ident_is_auto_heap) wasn't, so the unwrapped option's .data was read value-style on a pointer. The fix propagates is_auto_heap to the use-site Ident obj + its scope var, so cgen's existing auto-heap option-unwrap path emits ->data.
Now compiling+running correctly (all failed on master before): if x != none { collect(x) } at function scope and in loops, the if-guard if x := maybe() { collect(x) }, and match. interfaces/sumtypes/generics/options suites pass (654 tests), plus a new regression test.
One related case is left as a separate follow-up: the none-guard fallthrough (if x == none { continue } / return) flowing the unwrapped value into interface boxing. That's a broader, pre-existing bug — it fails identically on master even for a non-variadic single interface arg (the unwrapped value is materialized as its option struct), so it's a different root cause (the in-place unwrap's type representation) better fixed on its own.
…rface args deref correctly When a value passed to a variadic interface parameter is auto-heap promoted, mark_as_referenced promoted only the canonical declaration. But cgen resolves the use-site variable (a branch-local smartcast/option-unwrap copy for `if x is T` / `if x != none`) when emitting the read, via resolved_ident_is_auto_heap. With the flag missing there, an unwrapped option's `.data` was read value-style (`x.data`) on a pointer declaration (`_option_T *x = HEAP(...)`), producing 'pointer expected' / 'struct or union expected' C errors. Propagate is_auto_heap to the use-site Ident obj and its scope var as well, so cgen's existing auto-heap option-unwrap path (-> data) triggers. This was a pre-existing failure on master for option values unwrapped via if-guard / `if x != none` and passed to a variadic interface arg. interfaces, sumtypes, generics and options suites pass.
Fixes #27326
Problem
Passing a local variable that is declared inside a nested scope (such as a
forloop body) to a variadic interface parameter produced apointer expectedC compilation error:Root cause
When an argument is passed to a variadic interface parameter, the checker calls
mark_as_referenced, which promotes the variable to auto-heap (is_auto_heap = true) so the boxed interface can safely outlive the current scope.To locate the canonical declaration variable,
mark_as_referencedusedc.fn_scope.find_var(...). That only finds variables declared at the function scope and misses variables declared in nested scopes (loops,ifblocks). For those, it fell back to togglingis_auto_heapon the use-siteIdent'sobjcopy only.The result was inconsistent indirection in the generated C:
obj.is_auto_heap == trueand emitted a dereference (*code),string code = ...) instead of a heap pointer,so the generated code dereferenced a non-pointer:
(The same code compiled fine when the variable was declared at function scope, where it was correctly emitted as
string *code = HEAP(...).)Fix
Resolve the canonical declaration variable via the use-site scope chain (
node.scope.find_var), which walks parent scopes and therefore finds variables declared in nested scopes, falling back toc.fn_scopeand finally to the node's ownobj. The auto-heap promotion is now recorded on the variable that cgen sees when it emits the declaration, so the declaration and all reads use consistent pointer indirection.Testing
vlib/v/tests/interfaces/interface_variadic_test.v.firebirdstatement_test.v) now compiles past C code generation.vlib/v/tests/interfaces/,vlib/v/tests/generics/, andvlib/v/tests/sumtypes/all pass.