checker: keep map-copy guard for option-map or-unwraps (#27867)#27870
checker: keep map-copy guard for option-map or-unwraps (#27867)#27870medvednikov wants to merge 16 commits into
Conversation
An `or {}` block unwraps an option/result into a fresh value, so the
unwrapped expression must not be treated as an lvalue that aliases the
original map. Previously `x := c.some_map or { ... }` (and the variable
and index-expression forms) wrongly triggered the `cannot copy map: call
`move` or `clone` method` error, even though the equivalent `get() or {}`
call and option array field cases already compiled.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
medvednikov
left a comment
medvednikov
left a comment
There was a problem hiding this comment.
I found two blocking correctness gaps in the current implementation:
- Exempting every checked
IndexExprpermits shallow map copies from container storage, bypassing the aliasing protection this check implements. - The top-level AST match misses parenthesized versions of the same
or-unwrap expression, so the original diagnostic still occurs there.
CI/check results were intentionally not assessed.
| // call and option array field cases. | ||
| right_has_or_block := match right { | ||
| ast.Ident { right.or_expr.kind != .absent } | ||
| ast.IndexExpr { right.or_expr.kind != .absent } |
There was a problem hiding this comment.
[P1] Keep the map-copy guard for checked index expressions. An or on an index handles lookup/bounds failure; it does not materialize a fresh map. Both the array and map C lowering paths copy the successful element from *tmp_opt_ptr into the option payload without cloning. Consequently this change allows, for example, mut xs := [{'x': 1}]; mut y := xs[0] or { panic('missing') }; y['x'] = 2, where y aliases xs[0] and bypasses the exact protection below. Please retain the guard for IndexExpr, or change lowering to clone/move the map and add a mutation-based regression test.
| // unwrapped expression is not an lvalue that aliases the original map | ||
| // (see vlang/v issue #27867). This mirrors the already allowed `get() or {}` | ||
| // call and option array field cases. | ||
| right_has_or_block := match right { |
There was a problem hiding this comment.
[P2] Inspect through parentheses before classifying the RHS. This match only sees the outer node. (c.translations or { panic('expected') }) is parsed as a ParExpr wrapping the SelectorExpr; ParExpr.is_lvalue() delegates to the inner expression, so this still emits cannot copy map. Please normalize/remove ParExpr wrappers (or make this a recursive helper) and add a parenthesized regression case.
|
@codex review |
![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: fa2514901c
ℹ️ 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".
| // call and option array field cases. | ||
| right_has_or_block := match right { | ||
| ast.Ident { right.or_expr.kind != .absent } | ||
| ast.IndexExpr { right.or_expr.kind != .absent } |
There was a problem hiding this comment.
or
For map[K]?map[...] lookups where the or block returns none or another ?map, the index expression keeps the option-map type rather than unwrapping it. This line marks every indexed or as non-lvalue, so v := m[k] or { none } no longer gets the clone/move diagnostic and copies the optional map handle; mutating v? can alias the map stored in m. Gate this case on the expression actually clearing option/result, or keep the lvalue check when the indexed or preserves an option value.
Useful? React with 👍 / 👎.
… or-unwrap Follow-up to the review on #27870. The previous fix exempted every checked `or {}` unwrap of a map from the copy guard, which let a *mutable* destination silently alias container/field storage, e.g. `mut y := xs[0] or { panic('') }; y['x'] = 2` mutated `xs[0]`. Mirror V's array guard, which is mutability-aware: relax the map guard only for an immutable declaration (`x := opt_map or { ... }`), where `x` can never become a mutable alias. Mutable destinations (`mut x := m[k] or { ... }`, `x = m[k] or { ... }`) keep requiring `clone`/`move`. Parenthesized unwraps are handled via `remove_par()`, so `(c.m) or { ... }` compiles too. Adds a mutation-based checker error test for the mutable index/selector forms, and extends the runtime test with the parenthesized case.
|
Thanks — both points were valid. Fixed in 756eced. The root cause of gap 1 is that the map-copy guard, unlike the array guard, is not mutability-aware. Confirmed empirically that the aliasing footgun only exists for a mutable destination — and it applies equally to the mut xs := [{'x': 1}]
mut y := xs[0] or { panic('') }
y['x'] = 2 // mutated xs[0]So instead of exempting every or-unwrap, I now mirror V's array guard and relax the map guard only for an immutable declaration (
Gap 2 (parenthesized form) is handled with Added a mutation-based checker error test ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 756eced9a8
ℹ️ 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".
| // the guard, so aliasing container/field storage still requires a | ||
| // `clone`/`move`. See vlang/v issue #27867. | ||
| mut right_is_immutable_or_unwrap := false | ||
| if node.op == .decl_assign && left is ast.Ident && !left.is_mut { |
There was a problem hiding this comment.
This exception only checks that the new local is immutable, but the existing map-copy check also rejects immutable aliases because the original map can still be mutated later. For example, mut opt := ?map[string]int{}; opt = {'x': 1}; x := opt or { panic('missing') }; opt?['x'] = 2 is now allowed, and x aliases the same map storage and observes the mutation. Limit this bypass to sources that cannot be mutated later, or require clone/move for mutable option-map variables/fields/indexes.
Useful? React with 👍 / 👎.
Follow-up to the review on #27870. For `map[K]?map[...]`, an index `or` block that returns `none` (or another `?map`) does not unwrap the option: the result keeps the option-map type, so `v := m[k] or { none }` copies an option handle that still aliases the map stored in `m`. Gate the immutable or-unwrap exemption on the result type actually clearing option/result (`!right_type.has_flag(.option) && !right_type.has_flag(.result)`). The option-clearing form `m[k] or { panic(...) }` stays exempt (issue #27867); the option-preserving form is kept under the map-copy guard. Adds a checker error test for the option-preserving index `or`.
|
Good catch — fixed in 2a6f7dd. Confirmed the bug: for The exemption is now gated on the if node.op == .decl_assign && left is ast.Ident && !left.is_mut
&& !right_type.has_flag(.option) && !right_type.has_flag(.result) {
Since |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a6f7dd221
ℹ️ 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 node.op == .decl_assign && left is ast.Ident && !left.is_mut | ||
| && !right_type.has_flag(.option) && !right_type.has_flag(.result) { |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new exemption still keys only off the destination being immutable, while the added alias regression test covers only mut x := .... For mut opt := ?map[string]int({'x': 1}); x := opt or { panic('missing') }; opt?['x'] = 2, this bypass suppresses the existing map-copy diagnostic even though x shares the same map storage and observes the later mutation, which is the aliasing the m2 := m1 guard is meant to prevent. Keep requiring clone/move unless the unwrapped option source itself cannot be mutated later.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This was resolved in 8c4713c (the review is anchored to the earlier 2a6f7dd revision). The exemption no longer keys only off the destination — it now also requires the unwrapped source to be immutable, via a conservative helper that walks to the root:
fn assign_or_unwrap_source_is_immutable(expr ast.Expr) bool {
return match expr {
ast.Ident { !expr.is_mut() }
ast.SelectorExpr { assign_or_unwrap_source_is_immutable(expr.expr) }
ast.IndexExpr { assign_or_unwrap_source_is_immutable(expr.left) }
ast.ParExpr { assign_or_unwrap_source_is_immutable(expr.expr) }
else { false }
}
}Your exact example now errors on HEAD:
$ v run x.v
x.v:7:7: error: cannot copy map: call `move` or `clone` method (or use a reference)
7 | x := opt or { panic('missing') }
| ~~~
I've also extended option_map_or_unwrap_mut_source_err.vv to use your exact mut opt := ?map[string]int({'x': 1}) form and to cover mutable variable, field, and index sources (previously the only alias test, ..._mut_alias_err.vv, covered just the mut x := ... destination). Verified that mutable param and mutable-receiver-field sources are guarded too. Full compiler_errors_test.v and all 216 vlib/v/tests/options/ tests pass.
Follow-up to the review on #27870. The immutable or-unwrap exemption only checked that the destination local was immutable, but if the *source* option map is mutable it can be mutated later and the immutable copy observes it, e.g. `mut opt := ...; x := opt or { panic('') }; opt?['x'] = 2` left `x` aliasing `opt`'s storage. Gate the exemption on the unwrapped source itself being immutable via a new conservative `assign_or_unwrap_source_is_immutable` helper (walks Ident / SelectorExpr / IndexExpr / ParExpr roots; anything unknown keeps the guard). Immutable-source unwraps (`c := Category{}; x := c.f or { ... }`) stay exempt per issue #27867; mutable variables/fields/indexes keep requiring clone/move. Adds a checker error test covering the mutable variable, field and index sources.
|
Fixed in 8c4713c. Confirmed: The exemption now also requires the unwrapped source to be immutable, via a conservative recursive helper: fn assign_or_unwrap_source_is_immutable(expr ast.Expr) bool {
return match expr {
ast.Ident { !expr.is_mut() }
ast.SelectorExpr { assign_or_unwrap_source_is_immutable(expr.expr) }
ast.IndexExpr { assign_or_unwrap_source_is_immutable(expr.left) }
ast.ParExpr { assign_or_unwrap_source_is_immutable(expr.expr) }
else { false }
}
}It walks to the root and only exempts plainly-immutable sources; anything unknown keeps the guard. So:
Added |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c4713c4d2
ℹ️ 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".
| ast.Ident { !expr.is_mut() } | ||
| ast.SelectorExpr { assign_or_unwrap_source_is_immutable(expr.expr) } | ||
| ast.IndexExpr { assign_or_unwrap_source_is_immutable(expr.left) } |
There was a problem hiding this comment.
When the immutable root is a pointer/reference to mutable storage, this returns true even though the underlying option map can still be mutated later through the original mutable owner. For example, with mut c := Category{translations: {'x': 1}}; p := &c; x := p.translations or { panic('missing') }; c.translations?['x'] = 2, the new exemption suppresses the map-copy guard because p is immutable, but x aliases the same map and observes the later mutation. Treat pointer roots (or selector receivers whose type is a pointer) as mutable/unknown so the existing clone/move diagnostic is preserved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — real hole, fixed in b870fb7. Confirmed your exact example was suppressing the guard on the previous revision (checker allowed it), while master rejects it.
assign_or_unwrap_source_is_immutable now treats a pointer/reference anywhere in the chain as mutable/unknown:
ast.Ident {
if expr.obj is ast.Var {
!expr.is_mut() && !expr.obj.typ.is_ptr()
} else {
!expr.is_mut()
}
}
ast.SelectorExpr {
!expr.expr_type.is_ptr() && assign_or_unwrap_source_is_immutable(expr.expr)
}
ast.IndexExpr {
!expr.left_type.is_ptr() && assign_or_unwrap_source_is_immutable(expr.left)
}So the guard is preserved for:
p := &c; x := p.translations or { ... }(pointer variable root)fn f(p &Category) { x := p.translations or { ... } }(immutable reference parameter — receiverexpr_typeis a pointer)arr := [&c]; x := arr[0].translations or { ... }(pointer array-element receiver — caught by theexpr_typecheck even though the array root itself is immutable)
Added option_map_or_unwrap_ptr_source_err.vv covering all three. Full compiler_errors_test.v and all 216 vlib/v/tests/options/ tests pass.
…sion test
The map-copy guard for mutable option sources was already implemented in the
previous commit; this only strengthens the regression coverage. Use the exact
`mut opt := ?map[string]int({'x': 1})` form from the review thread and keep the
mutable variable/field/index source cases, so the guarded aliasing is explicit.
No checker changes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66afd396ed
ℹ️ 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".
| // recognised; anything unknown returns false, keeping the map-copy guard. | ||
| fn assign_or_unwrap_source_is_immutable(expr ast.Expr) bool { | ||
| return match expr { | ||
| ast.Ident { !expr.is_mut() } |
There was a problem hiding this comment.
This treats any non-mut identifier as storage that cannot be mutated later, but an immutable ?map binding can still wrap a map that is mutable through another alias, e.g. mut base := {'x': 1}; opt := ?map[string]int(base); x := opt or { panic('missing') }; base['x'] = 2 now bypasses the map-copy guard and x observes the later mutation. The same applies to ?map parameters passed from mutable callers, so suppressing the guard needs an ownership/freshness check rather than just !expr.is_mut().
Useful? React with 👍 / 👎.
Follow-up to the review on #27870. assign_or_unwrap_source_is_immutable treated an immutable pointer root as immutable, but an immutable pointer can still alias storage its owner mutates later, e.g. `mut c := Category{...}; p := &c; x := p.translations or { panic('') }; c.translations?['x'] = 2` left `x` observing the mutation. Treat a pointer/reference anywhere in the chain as mutable/unknown: reject when the root variable's type is a pointer, and when a selector receiver (`expr_type`) or index container (`left_type`) is a pointer, keeping the `clone`/`move` diagnostic. Adds a checker error test covering the pointer variable root, immutable reference parameter, and pointer array-element receiver.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b870fb739a
ℹ️ 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".
| // would be observed through `x`. | ||
| // See vlang/v issue #27867. | ||
| mut right_is_immutable_or_unwrap := false | ||
| if node.op == .decl_assign && left is ast.Ident && !left.is_mut |
There was a problem hiding this comment.
When the declaration is shared b := c.translations or { ... }, left.is_mut is still false, so this path marks the RHS as safe and suppresses the map-copy guard even though b is mutable under lock b { b['x'] = 2 }. That creates the same shallow alias with the map still stored in c that the existing guard rejects for shared b := a; exclude shared declarations (for example via left.info.share == .shared_t) from this immutable-destination exemption.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6448418, with a nuance worth flagging: on this codebase the shared case was already rejected. The parser sets is_mut for shared/atomic declarations (parser.v:1553: is_mut := p.tok.kind == .key_mut || is_shared || is_atomic), so !left.is_mut already excluded shared destinations. I instrumented it to be sure — for shared b := c.translations or { ... } the exemption evaluated to false and the guard fired, matching master.
That said, relying on is_mut incidentally covering shared is fragile for a safety bypass, so I made it explicit as you suggested:
left_is_lockable_dest := left is ast.Ident && left.info is ast.IdentVar
&& left.info.share in [.shared_t, .atomic_t]
...
if node.op == .decl_assign && left is ast.Ident && !left.is_mut && !left_is_lockable_dest
&& !right_type.has_flag(.option) && !right_type.has_flag(.result) {Now the exemption is independent of the parser's is_mut-includes-shared detail (and also covers atomic). Added option_map_or_unwrap_shared_dest_err.vv locking in that shared b := c.translations or { ... } keeps the cannot copy map diagnostic. Full compiler_errors_test.v and all 216 vlib/v/tests/options/ tests pass.
…opy guard Follow-up to the review on #27870. A `shared b := c.translations or { ... }` destination is mutable under `lock b { b['x'] = 2 }`, so the immutable-destination exemption must not apply. The parser already sets `is_mut` for shared/atomic declarations (so `!left.is_mut` currently excludes them), but check `left.info.share` explicitly so this safety bypass no longer depends on that incidental detail. Adds a checker error test for the shared destination.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6448418396
ℹ️ 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".
| right_is_immutable_or_unwrap = has_or_block | ||
| && assign_or_unwrap_source_is_immutable(unwrapped_right) |
There was a problem hiding this comment.
When the option source is immutable but the or block returns another map lvalue, this exemption suppresses the existing copy diagnostic even though the value can come from that mutable fallback. For example, mut fallback := {'x': 1}; opt := maybe_none(); x := opt or { fallback }; fallback['x'] = 2 now bypasses the guard because only opt is checked for immutability, but x aliases fallback and observes the later mutation; inspect the block's returned expression or keep the guard when the default path can return a map lvalue.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e65b20e. Confirmed x := opt or { fallback } (mutable fallback) aliased and observed the later mutation; the exemption now also inspects the or block's fallback value and only holds when the default cannot alias mutable storage — a fresh value (map literal / by-value call), an immutable lvalue, or a block that yields no value (diverges/propagates). or { fallback } now keeps the cannot copy map diagnostic; or { panic(...) }, or { {...} }, and or { immutable_map } still compile. Added option_map_or_unwrap_mut_default_err.vv.
Bigger picture — I want to be straight with you rather than keep patching: this is the 8th aliasing edge on this exemption, and the underlying one from an earlier round is still open and not fixable with a local heuristic:
mut base := {'x': 1}
opt := ?map[string]int(base) // opt is an immutable binding, but wraps base's map
x := opt or { panic('') } // x aliases base
base['x'] = 2 // observed through xopt is immutable, so assign_or_unwrap_source_is_immutable (correctly) can't see that its value aliases base — that needs data-flow/ownership info the checker doesn't have. Since maps share storage and V has no ownership tracking, no !is_mut()-style check can be made sound — there will always be another edge.
There are two sound resolutions, and I think it's worth picking one instead of continuing:
- Revert the exemption —
x := opt or {}errors likem2 := m1; users write(opt or {}).clone()(already compiles today). Most consistent with V's explicit-map-copy rule; closes Cannot or-unwrap ?map[string]T field #27867 as "use.clone()". - Auto-clone the unwrapped map (like
mut a := arr[..]) — bare form compiles and runs as the issue asks, and it's sound becausexis fresh. Deletes this whole heuristic and every past/future aliasing edge at once.
Happy to implement either — which do you prefer?
…lvalue Follow-up to the review on #27870. `x := opt or { fallback }` makes `x` the block's value when the option is empty, so a mutable map lvalue default (e.g. `or { fallback }`) leaves `x` aliasing that storage. The exemption checked only the option source's immutability, not the default path. Also require the `or` block default to be non-aliasing: a fresh value (map literal / by-value call), an immutable lvalue, or a block that yields no value (diverges or propagates). Otherwise keep the clone/move diagnostic. Adds a checker error test for the mutable-lvalue or-block default.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e65b20ebe4
ℹ️ 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".
| ast.SelectorExpr { | ||
| !expr.expr_type.is_ptr() && assign_or_unwrap_source_is_immutable(expr.expr) |
There was a problem hiding this comment.
Fresh evidence is that the final helper still uses raw Type.is_ptr() checks rather than unaliasing pointer aliases. For a supported selector receiver like type CategoryRef = &Category; p := CategoryRef(&c); x := p.translations or { panic('missing') }, the receiver type can be the alias, so this branch classifies the source as immutable and suppresses the map-copy guard even though c.translations?['x'] = 2 can still mutate the same map storage through the original owner. Please treat alias-to-pointer receivers (and the corresponding Ident root) as mutable/unknown before applying this exemption.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5e63eb7. Confirmed the alias-to-pointer receiver bypassed the guard on HEAD while master rejects it. The helper now fully unaliases types before every pointer check (Ident root, selector receiver, index container) via c.table.fully_unaliased_type(...).is_ptr(), so type CategoryRef = &Category is treated as a pointer:
ast.SelectorExpr {
!c.table.fully_unaliased_type(expr.expr_type).is_ptr()
&& c.assign_or_unwrap_source_is_immutable(expr.expr)
}(The helpers became Checker methods to reach the table.) Added option_map_or_unwrap_alias_ptr_source_err.vv. Full compiler_errors_test.v and all 216 vlib/v/tests/options/ tests pass.
This is round 9 of tightening this exemption, and it doesn't change the core issue I raised on the previous thread: the opt := ?map(mutable_base); x := opt or { ... } alias is still open and can't be fixed by any local is_mut/is_ptr heuristic — it needs ownership/data-flow info the checker doesn't have. I'll keep addressing concrete edges, but the only ways to actually close this out are revert the exemption (users write (opt or {}).clone()) or auto-clone the unwrapped map (sound, fixes #27867, deletes the whole heuristic). Happy to do either as soon as you pick one.
Follow-up to the review on #27870. The immutable-source helper used raw Type.is_ptr(), so a `type Ref = &Category` receiver was classified as immutable and the map-copy guard was suppressed, even though the underlying `&Category` can mutate the same map through the original owner. Fully unalias types before the pointer check (Ident root, selector receiver, index container). The helpers are now Checker methods so they can reach the table. Adds a checker error test for the alias-to-pointer receiver.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e63eb7f10
ℹ️ 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".
| ast.Ident, ast.SelectorExpr, ast.IndexExpr, ast.ParExpr { | ||
| c.assign_or_unwrap_source_is_immutable((last as ast.ExprStmt).expr) |
There was a problem hiding this comment.
When the or block's fallback is an immutable map parameter or another immutable alias of caller-owned mutable storage, this treats it as safe solely because the fallback lvalue is not mut. For example, fn pick(fallback map[string]int) map[string]int { opt := maybe_none(); x := opt or { fallback }; return x } now bypasses the map-copy diagnostic, but a caller can pass a mutable map and mutate it after pick, so the returned x aliases and observes that mutation. This is the same lvalue map copy that x := fallback would reject, so the exemption should only apply to fresh/owned fallback values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b599c51. Confirmed x := opt or { fallback } with an immutable map parameter aliased the caller's map and observed the later mutation. The or-block default check now only accepts genuinely fresh/owned values (map literal or by-value call) or blocks that yield no value; any lvalue default — even an immutable one — keeps the cannot copy map diagnostic:
return match (last as ast.ExprStmt).expr {
ast.MapInit, ast.CallExpr { true }
else { false }
}Added option_map_or_unwrap_immut_default_alias_err.vv (immutable param + immutable local defaults). Full compiler_errors_test.v and all 216 vlib/v/tests/options/ tests pass.
Worth naming explicitly: this is the same unsoundness as the still-open source case — an immutable binding is not proof of freshness, because a map lvalue can alias caller-owned mutable storage. You've now confirmed it for the default path; it remains true for the source path (opt := ?map(mutable_base); x := opt or { ... }), which no local is_mut/is_ptr check can catch. That's round 10. The exemption cannot be made sound this way; the only closures are revert (users write (opt or {}).clone()) or auto-clone (sound, fixes #27867, deletes the heuristic). Say the word and I'll land either.
… lvalues Follow-up to the review on #27870. The or-block default check accepted an immutable map lvalue, but an immutable map parameter/field can alias caller-owned storage the caller mutates later, so `x := opt or { fallback }` returns a value that aliases and observes that mutation — the same copy `x := fallback` rejects. Restrict assign_or_block_default_is_safe to genuinely fresh/owned values (map literal or by-value call) or blocks that yield no value; any lvalue default now keeps the map-copy guard. Adds a checker error test for immutable parameter and immutable local defaults.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b599c5156a
ℹ️ 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".
| last := or_expr.stmts.last() | ||
| if last !is ast.ExprStmt { | ||
| return true |
There was a problem hiding this comment.
When an or block ends with a trailing semicolon, the parser leaves a SemicolonStmt at the end and check_or_expr already filters those before deciding which expression supplies the block value. This helper instead reads the raw last statement and returns safe for any non-ExprStmt, so x := opt or { fallback; } bypasses the map-copy guard even though the none path still returns the map lvalue fallback and can observe later mutations. Filter SemicolonStmt here before classifying the fallback value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8f35843. The helper now filters SemicolonStmt (matching check_or_expr) before classifying the block value, and I also tightened the fallthrough: previously any non-ExprStmt last statement was treated as safe, which was too permissive. Now only a genuinely diverging terminator (return/break/continue; panic/exit are calls already classified as fresh) is safe — any other or unclassifiable last statement keeps the guard.
valid_stmts := or_expr.stmts.filter(it !is ast.SemicolonStmt)
...
match last {
ast.ExprStmt { match last.expr { ast.MapInit, ast.CallExpr { true } else { false } } }
ast.Return, ast.BranchStmt { true }
else { false }
}One clarification from testing: x := opt or { fallback; } is rejected on the normal compile path with expression evaluated but not used (identically on master and this branch), so the trailing-semicolon block never compiles and there's no reachable aliasing footgun — the map-copy difference was only observable under -check (which keeps collecting errors past the first). I therefore didn't add a harness regression test: the error-test harness uses the normal path where both versions produce identical output, so it can't isolate this change. The fix is still correct defensively and now matches master under -check. Full compiler_errors_test.v (1598 tests) and all 216 vlib/v/tests/options/ tests pass.
Same standing note as the prior threads: the source-side opt := ?map(mutable_base); x := opt or { ... } alias remains open and unfixable by any local heuristic, so revert or auto-clone are still the only ways to actually close this out. Ready to land either.
Follow-up to the review on #27870. assign_or_block_default_is_safe read the raw last statement of the or-block and returned safe for any non-ExprStmt, so a trailing `;` (`or { fallback; }`) made it skip the map-copy guard. Filter SemicolonStmt first (matching check_or_expr), and only treat a genuinely diverging terminator (`return`/`break`/`continue`; `panic`/`exit` are calls handled as fresh values) as safe — any other/unclassifiable last statement now keeps the guard. No separate test: `or { d; }` is rejected with 'expression evaluated but not used' on the normal compile path (identically on master), so the map-copy difference is only observable under -check, which the error-test harness does not use.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f35843884
ℹ️ 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".
| return match last { | ||
| ast.ExprStmt { | ||
| match last.expr { | ||
| ast.MapInit, ast.CallExpr { true } |
There was a problem hiding this comment.
When the or block's default value is a call, this unconditionally marks it safe, but a call can simply return a caller-owned map alias. For example, fn id(m map[string]int) map[string]int { return m } with x := opt or { id(fallback) } is now accepted when opt is an immutable source; if fallback is mutated later, x observes the same storage, bypassing the lvalue-default guard added for or { fallback }. Keep the map-copy guard for arbitrary map-returning calls unless the callee is known to produce fresh storage, such as a literal/clone/move/noreturn path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6565262. Confirmed x := opt or { id(fallback) } aliased fallback and observed the later mutation. The or-block default check no longer trusts arbitrary calls:
fn assign_call_default_produces_fresh_map(expr ast.Expr) bool {
if expr is ast.CallExpr {
return expr.is_noreturn || expr.kind in [.clone, .clone_to_depth, .move]
|| (expr.is_method && expr.name in ['clone', 'move'])
}
return false
}So only clone/move results (fresh storage) or @[noreturn] calls (panic/exit, no value) are exempt; or { id(fallback) } now keeps the cannot copy map diagnostic, while or { panic(...) }, or { fallback.clone() }, or { exit(1) }, and or { {...} } still compile. Added option_map_or_unwrap_call_default_alias_err.vv. Full compiler_errors_test.v and all 216 vlib/v/tests/options/ tests pass.
Round 12. As with the source side, this is the interprocedural-freshness problem: without data-flow the checker can't tell a fresh-returning call from an aliasing one, so I've restricted to the syntactically-known-fresh set you listed. The core opt := ?map(mutable_base); x := opt or { ... } source alias is still open and unfixable locally. revert or auto-clone remain the only ways to actually close this out — ready to land either.
Follow-up to the review on #27870. The or-block default check treated any CallExpr as fresh, but an arbitrary map-returning call can return a caller-owned alias (`fn id(m map) map { return m }`), so `x := opt or { id(fallback) }` aliased `fallback` and observed later mutations. Only exempt calls known to produce fresh storage — `clone`/`move` results — or `@[noreturn]` calls (`panic`/`exit`) that yield no value. Any other map-returning call now keeps the map-copy guard. Adds a checker error test for the aliasing call default.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6565262cf8
ℹ️ 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".
| return expr.is_noreturn || expr.kind in [.clone, .clone_to_depth, .move] | ||
| || (expr.is_method && expr.name in ['clone', 'move']) |
There was a problem hiding this comment.
Because CallKind is assigned from the callee name, this also treats user-defined functions or methods named clone/move as fresh. For example, a Holder.clone() map[string]int method that just returns a stored map lets x := opt or { holder.clone() } bypass the guard; if that stored map is reachable through a mutable alias, later mutations are observed through x. Please only exempt the builtin map clone/move operations, or otherwise prove the call returns fresh storage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b82d49b. Confirmed a user-defined Holder.clone() map[string]int returning a stored map let x := opt or { holder.clone() } bypass the guard. The clone/move exemption now requires the call's receiver to be a map, so only the builtin map operations qualify:
return expr.is_method && expr.name in ['clone', 'move']
&& c.table.final_sym(expr.receiver_type).kind == .map(final_sym so a type MyMap = map[...] receiver still counts.) Now or { holder.clone() } keeps the cannot copy map diagnostic, while the builtin or { fallback.clone() } / or { fallback.move() }, or { panic(...) }, and or { {...} } still compile. Added option_map_or_unwrap_user_clone_default_err.vv. Full compiler_errors_test.v and all 216 vlib/v/tests/options/ tests pass.
Round 13. The source-side opt := ?map(mutable_base); x := opt or { ... } alias is still open and unfixable by any local heuristic — revert or auto-clone remain the only ways to actually close this out. Ready to land either.
Follow-up to the review on #27870. CallKind and the method name alone do not distinguish the builtin map clone/move from a user-defined `clone`/`move` that returns a stored (aliasable) map, so `x := opt or { holder.clone() }` bypassed the guard. Require the clone/move call's receiver to be a map (final_sym.kind == .map) before treating it as fresh; noreturn calls still qualify. Adds a checker error test for a user-defined clone method default.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b82d49bd9f
ℹ️ 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".
| return expr.is_method && expr.name in ['clone', 'move'] | ||
| && c.table.final_sym(expr.receiver_type).kind == .map |
There was a problem hiding this comment.
This still treats any .clone()/.move() call whose receiver ultimately unaliases to map as a fresh builtin map operation. Fresh evidence in this revision is the final_sym(expr.receiver_type).kind == .map check: checker/fn.v resolves alias receiver methods before map builtins when left_sym.kind == .alias && left_sym.has_method(method_name), so type M = map[string]int; fn (m M) clone() map[string]int { return map[string]int(m) } can be used in x := opt or { holder.m.clone() }; the guard is suppressed, but x aliases holder.m and observes later mutations. Require that the call actually resolved to the builtin map method, not just that the receiver unaliases to a map.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 01d21af. Confirmed type MapAlias = map[string]int with a user-defined clone() let x := opt or { holder.m.clone() } alias holder.m (master rejects it; HEAD suppressed the guard). The check now uses the receiver type directly, not unaliased:
return expr.is_method && expr.name in ['clone', 'move']
&& c.table.sym(expr.receiver_type).kind == .mapA map carries no user methods, so a direct .map receiver is unambiguously the builtin; an alias receiver is .alias and no longer qualifies (matching fn.v, which resolves the alias's own method first). or { holder.m.clone() } now keeps the cannot copy map diagnostic, while builtin or { fallback.clone() } / or { fallback.move() }, or { panic(...) }, and or { {...} } still compile. Added option_map_or_unwrap_alias_user_clone_default_err.vv. Full compiler_errors_test.v and all 216 vlib/v/tests/options/ tests pass.
Round 14. Same standing note: the source-side opt := ?map(mutable_base); x := opt or { ... } alias is still open and unfixable by any local heuristic — revert or auto-clone remain the only ways to actually close this out. Ready to land either.
…faults Follow-up to the review on #27870. Using final_sym to check the clone/move receiver still trusted an alias whose base is a map, but a map type alias can define its own clone/move (`type M = map[...]; fn (m M) clone() ...`), which fn.v resolves before the map builtin — so `x := opt or { holder.m.clone() }` aliased holder.m. Check the receiver type directly (`sym().kind == .map`): a map carries no user methods, so a direct map receiver is the builtin, while an alias receiver is `.alias` and no longer qualifies. Adds a checker error test for an alias with a user-defined clone default.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01d21af409
ℹ️ 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".
| match last.expr { | ||
| ast.MapInit { true } | ||
| ast.CallExpr { c.assign_call_default_produces_fresh_map(last.expr) } | ||
| else { false } |
There was a problem hiding this comment.
When the or block's value is parenthesized, this match sees an ast.ParExpr and falls into the unsafe case, so valid fresh defaults like x := opt or { (map[string]int{}) } or x := opt or { (fallback.clone()) } still hit the map-copy diagnostic even though the unparenthesized forms are accepted. Unwrap last.expr.remove_par() before this classification so harmless parentheses do not change checker behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c31f53b. remove_par() the default value before classifying it:
ast.ExprStmt {
default_expr := last.expr.remove_par()
match default_expr {
ast.MapInit { true }
ast.CallExpr { c.assign_call_default_produces_fresh_map(default_expr) }
else { false }
}
}Now or { (map[string]int{}) } and or { (fallback.clone()) } compile like their unparenthesized forms, while parenthesized unsafe defaults stay guarded — or { (fallback) } and or { (holder.clone()) } still emit cannot copy map (remove_par exposes the lvalue/user-call, which then fails the freshness check). Extended the runtime regression test with the parenthesized fresh cases. Full compiler_errors_test.v and all 216 vlib/v/tests/options/ tests pass.
(This one was a false-negative rather than an aliasing hole — thanks.) The standing source-side opt := ?map(mutable_base) alias is still open and unfixable locally; revert or auto-clone remain the only ways to fully close this out.
Follow-up to the review on #27870. A parenthesized fresh default (`or { (map[string]int{}) }`, `or { (fallback.clone()) }`) matched ast.ParExpr and fell into the unsafe branch, so harmless parentheses wrongly triggered the map-copy diagnostic while the unparenthesized forms compiled. remove_par() the default value before classifying it. Extends the runtime regression test with parenthesized fresh defaults.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c31f53bca4
ℹ️ 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".
| return match expr { | ||
| ast.Ident { | ||
| if expr.obj is ast.Var { | ||
| !expr.is_mut() && !c.table.fully_unaliased_type(expr.obj.typ).is_ptr() |
There was a problem hiding this comment.
Fresh evidence in this revision is that this helper still returns true for any non-mut, non-pointer ast.Var, treating immutability as ownership. An immutable option map can still be a shallow wrapper around mutable storage, e.g. mut base := {'x': 1}; opt := ?map[string]int(base); x := opt or { panic('missing') }; base['x'] = 2; the new bypass suppresses the map-copy error at the unwrap and x observes the later mutation. Keep requiring clone/move unless the unwrapped source is proven fresh/owned, not merely immutable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved by reverting the exemption in 73ce15c. You're right, and this is the terminal version of the whole thread: assign_or_unwrap_source_is_immutable treated immutability as ownership, and those aren't the same — opt := ?map(base) is an immutable binding over base's mutable storage. There's no local is_mut/is_ptr/receiver/provenance check that can prove freshness (it needs whole-program data-flow V doesn't have), and applied rigorously the "prove fresh" requirement rejects essentially all lvalue sources — i.e. it collapses to keeping the guard.
So I've restored master's behavior: an option-map or-unwrap is a map copy and keeps the cannot copy map diagnostic, exactly like m2 := m1. checker/assign.v is now identical to master (0-line diff); all the exemption helpers and their probe tests are removed, and I left a single regression test documenting that the guard applies to the option-map or-unwrap forms.
For #27867 the unwrap is written with an explicit copy — x := (c.f or { ... }).clone() — which compiles today.
…/move
The exemption added earlier tried to let `x := opt_map or { ... }` compile when
the option source looked immutable. As the review on #27870 showed across many
rounds, this cannot be made sound: V has no ownership/data-flow, so an immutable
binding is not proof the unwrapped map is unaliased — e.g.
`mut base := {'x':1}; opt := ?map[string]int(base); x := opt or { panic('') }`
leaves `x` aliasing `base`. No local is_mut/is_ptr/receiver check can prove
freshness, so restore master's behavior: an option-map or-unwrap is a map copy
and keeps the `cannot copy map` guard, exactly like `m2 := m1`.
For #27867, unwrap with an explicit copy: `x := (c.f or { ... }).clone()`.
assign.v is now identical to master; adds a single regression test documenting
that the guard applies to option-map or-unwrap forms.
medvednikov
changed the title
💡 Codex Review Stop expecting the #27867 repro to fail
The linked issue (#27867, #27867) expects ℹ️ 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". |
1 similar comment
💡 Codex Review Stop expecting the #27867 repro to fail
The linked issue (#27867, #27867) expects ℹ️ 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". |
1 participant
Background
#27867 reports that
x := c.translations or { panic('...') }fails withcannot copy map: call move or clone method (or use a reference)whentranslationsis a?map[...].This PR originally tried to exempt such or-unwraps from the map-copy guard when the option source looked immutable. Over many review rounds it became clear that the exemption cannot be made sound: V has no ownership/data-flow analysis, so an immutable binding is not proof that the unwrapped map is unaliased. For example:
No local
is_mut/is_ptr/ receiver / provenance check can prove freshness, and "prove fresh" applied rigorously rejects essentially all lvalue sources — i.e. it collapses back to keeping the guard.What this PR does now
Restores master's behavior: an option-map or-unwrap is a map copy and keeps the
cannot copy mapdiagnostic, just likem2 := m1.vlib/v/checker/assign.vis identical to master (0-line diff). Adds one regression test documenting that the guard applies to the option-map or-unwrap forms (field / variable / index).#27867 resolution
The map is unwrapped with an explicit copy, which compiles today:
If instead V should make the bare form compile, the only sound route is to auto-clone the unwrapped map (like
mut a := arr[..]already does) — a deliberate language-design choice (implicit allocation) that would be a separate change.