v3: fix for-in-by-ref copies and @[heap] struct allocation - #28087
v3: fix for-in-by-ref copies and @[heap] struct allocation#28087quaesitor-scientiam wants to merge 1 commit into
Conversation
`for i in &arr` bound the loop variable by value instead of by
reference: the parser only sets the for-in node's `.amp` op for
`for mut i in arr`, and the transformer stripped the container's own
leading `&` off its type without keeping track of it, so
lower_indexed_for_in never knew the container had been referenced.
Track it explicitly from the container's AST shape.
`@[heap]`-tagged structs were never actually heap-allocated for plain
`t := Test{}` declarations, only for explicit `&Test{}` - the checker
already relied on the attribute as an unconditional heap promise
for borrow-safety checks, but nothing in transform/codegen enforced
it. Hook the existing escape-analysis heap-promotion path
(heap_escaping_source_decl) so `@[heap]` structs get the same
treatment unconditionally, including a real deep copy (not a pointer
alias) on `y := t`, matching classic V. This also fixes stringify_expr
and string_interp_needs_value_read, which assumed a `&T`-typed local
was always a genuine nilable pointer and produced invalid C for the
new heap-promoted case.
Fixes vlang#28084
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The `v3` CI check is failing, but it's unrelated to this change: the failure is in `vlib/v3/tests/aoc_compat_codegen_test.v`, which doesn't touch for-in iteration or `@[heap]` structs at all. It's a closure-argument-count bug for captured-variable closures passed to `.any()`/`.all()`/`.count()`: ``` Confirmed pre-existing by running that exact test with `-old-compiler` (routes entirely through the classic `vlib/v/` compiler — a code path this PR doesn't touch at all, since every change here is under `vlib/v3/`): it fails identically there too, so it can't be caused by this diff. No existing tracking issue for it that I could find — happy to file one separately if useful. |
|
Since this is v3, @medvednikov should decide on merging. Local AI check said: No concrete correctness issues found |
|
This would complicate the language. Just use for with an index. |
Can't compile my code in V3 then |
|
my bad, it's already in the language
|
|
Why #28087 itself should not be merged as-is The PR combines three concerns: Reference iteration over fixed arrays. Although the diff is only 45 additions across four compiler files, those are distinct semantic changes. There are also no committed regression-test files in the patch; the extensive test work is described in the PR body but is not preserved as tests that future compiler changes would run. I would submit two separate changes:
Only fix: for item in &fixed_array { The compiler’s typed representation should carry an explicit element-binding mode—conceptually value, ref, or mut_ref—rather than discovering reference semantics late in lowering by inspecting whether the original container AST happened to be a prefix-& node. That would keep parentheses, aliases, pointer variables, fixed arrays, and dynamic arrays consistent. Tests should cover at least: A local fixed array. Heap promotion changes storage representation, assignment semantics, escaping references, copying, method calls, and string conversion. That deserves a dedicated PR and dedicated tests rather than being coupled to a for lowering correction. |
|
Although it would still be cleaner to use for with an index. :-\ |
I will split it |
Summary
Fixes two related bugs in the
v3compiler pipeline (the default on macOS forv run/v build) reported in #28084:for i in &arrcopied elements by value instead of by reference. The for-in node's.ampop is only set forfor mut i in arr, unrelated to a&on the container expression. The transformer stripped the container's own leading&off its type without tracking that fact anywhere downstream, solower_indexed_for_inalways bound the element by value regardless of the explicit&. Now tracked directly from the container's AST shape (a genuine.prefix .ampcontainer expression), independent ofnode.op.@[heap]-tagged structs were never actually heap-allocated for plaint := Test{}declarations — only&Test{}worked. The type checker already treats@[heap]as an unconditional heap-allocation promise for borrow-safety checks (seechecker.v/checker_tail.v), but nothing in transform/codegen enforced it, so the promise was unsound. This hooks the existing escape-analysis heap-promotion machinery (heap_escaping_source_decl, already used for locals whose address is later found to escape) so@[heap]structs get the same treatment unconditionally at declaration, including a real deep copy (not a pointer alias) ony := t, matching classic V's value-copy-with-heap-storage semantics exactly.Fixing the second bug surfaced a real latent codegen bug:
stringify_expr/string_interp_needs_value_readassumed any local whose tracked type is&Tis a genuine nilable pointer, and produced invalid C (a pointer/value type mismatch) for the new heap-promoted case —println(t)for a@[heap]struct. Fixed alongside.Test plan
-old-compiler) backend for parity across: methods (value + pointer receiver), by-value function args,mutparams, struct auto-str/println, return-value capture, nested scopes, per-iteration loop locals, self-referential structs, arrays/maps of heap structs — all byte-identical to classic V.vlib/v3/tests/suite (206 files) before and after: the 30 failures seen post-patch are pre-existing (28 fail identically on the unpatched baseline) or environmental flakiness under 10-way parallel test execution (2 tests flip pass/fail across repeated runs on both the baseline and patched binaries) — no reproducible new regressions../vnew fmt -diffclean on all touched files.Fixes #28084
🤖 Generated with Claude Code