Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
fa25149
checker: fix or-unwrapping an option map field/var/index (fix #27867)
medvednikov Jul 19, 2026
756eced
checker: address review — only relax the map-copy guard for immutable…
medvednikov Jul 19, 2026
2a6f7dd
checker: address review — only exempt or-unwrap that clears the option
medvednikov Jul 19, 2026
8c4713c
checker: address review — keep map-copy guard for mutable option sources
medvednikov Jul 19, 2026
66afd39
tests: use the review's exact mutable-source form in the alias regres…
medvednikov Jul 19, 2026
b870fb7
checker: reject or-unwrap through immutable pointers to mutable data
medvednikov Jul 19, 2026
6448418
checker: never exempt shared/atomic or-unwrap destinations from map-c…
medvednikov Jul 19, 2026
e65b20e
checker: keep map-copy guard when or-block default returns a mutable …
medvednikov Jul 19, 2026
5e63eb7
checker: unalias types before the or-unwrap pointer check
medvednikov Jul 19, 2026
b599c51
checker: only exempt fresh/owned or-block map defaults, not immutable…
medvednikov Jul 19, 2026
8f35843
checker: filter semicolons and tighten or-block default classification
medvednikov Jul 19, 2026
6565262
checker: only exempt fresh/noreturn or-block map call defaults
medvednikov Jul 19, 2026
b82d49b
checker: restrict fresh or-block clone/move defaults to builtin map ops
medvednikov Jul 19, 2026
01d21af
checker: require direct map receiver for fresh or-block clone/move de…
medvednikov Jul 19, 2026
c31f53b
checker: strip parens before classifying or-block map defaults
medvednikov Jul 19, 2026
73ce15c
checker: revert unsound option-map or-unwrap exemption; require clone…
medvednikov Jul 20, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 99 additions & 1 deletion vlib/v/checker/assign.v
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,74 @@ fn assign_expr_is_auto_deref(expr ast.Expr) bool {
return expr.is_auto_deref_var()
}

// assign_or_unwrap_source_is_immutable conservatively reports whether the source
// of an `or {}` unwrap can never be mutated later (so a copy of its map cannot
// observe a later mutation of the original). Only the plainly-immutable roots are
// recognised; anything unknown returns false, keeping the map-copy guard.
// A pointer/reference anywhere in the chain is treated as mutable/unknown, since
// an immutable pointer can still alias storage that its owner mutates later
// (e.g. `mut c := ...; p := &c; x := p.f or { ... }; c.f?[k] = v`).
fn assign_or_unwrap_source_is_immutable(expr ast.Expr) bool {
return match expr {
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Treat pointer aliases as mutable unwrap sources

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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
ast.IndexExpr {
!expr.left_type.is_ptr() && assign_or_unwrap_source_is_immutable(expr.left)
}
ast.ParExpr {
assign_or_unwrap_source_is_immutable(expr.expr)
}
else {
false
}
}
}

// assign_expr_or_block returns the `or {}` block attached to `expr`, if any.
fn assign_expr_or_block(expr ast.Expr) ast.OrExpr {
return match expr {
ast.Ident { expr.or_expr }
ast.IndexExpr { expr.or_expr }
ast.SelectorExpr { expr.or_block }
else { ast.OrExpr{} }
}
}

// assign_or_block_default_is_safe conservatively reports whether the value an
// `or {}` block falls back to cannot alias mutable storage. `x := opt or { d }`
// makes `x` the block's value `d` when the option is empty, so if `d` is a
// mutable map lvalue (e.g. `or { fallback }`) then `x` aliases it. Only a fresh
// value (map literal / by-value call), an immutable lvalue, or a block that
// yields no value (diverges via `return`/`panic`/... or just propagates) is safe.
fn assign_or_block_default_is_safe(or_expr ast.OrExpr) bool {
if or_expr.kind != .block || or_expr.stmts.len == 0 {
return true
}
last := or_expr.stmts.last()
if last !is ast.ExprStmt {
return true

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Filter semicolons before trusting the or-block default

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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return match (last as ast.ExprStmt).expr {
ast.MapInit, ast.CallExpr {
true
}
ast.Ident, ast.SelectorExpr, ast.IndexExpr, ast.ParExpr {
assign_or_unwrap_source_is_immutable((last as ast.ExprStmt).expr)
}
else {
false
}
}
}

fn (c &Checker) auto_deref_source_type_is_pointer(expr ast.Expr) bool {
if expr !is ast.Ident || c.table.cur_fn == unsafe { nil } || !expr.is_auto_deref_var() {
return false
Expand Down Expand Up @@ -886,8 +954,38 @@ or use an explicit `unsafe{ a[..] }`, if you do not want a copy of the slice.',
} else {
right.is_lvalue()
}
// `x := opt_map or { ... }` unwraps an option/result into a new immutable
// variable, so `x` can never become a mutable alias of the underlying map
// and the shallow copy is safe. This mirrors how V already accepts the
// equivalent immutable `x := opt_array_field or { ... }`. Several things
// must hold for the copy to be safe, otherwise the guard is kept so
// aliasing still requires a `clone`/`move`:
// - the destination is a new immutable variable (a mutable `mut x := ...`
// or a reassignment `x = ...` could still mutate through `x`);
// - the `or` actually clears the option/result — for `map[K]?map[...]`,
// `v := m[k] or { none }` keeps the option-map type, so `v` is still an
// option handle aliasing the map in `m`;
// - the unwrapped source itself is immutable — otherwise a later mutation
// of the source (e.g. `mut opt := ...; x := opt or { ... }; opt?[k] = v`)
// would be observed through `x`.
// See vlang/v issue #27867.
// A `shared`/`atomic` destination is mutable under `lock`, so it must never
// be exempted even though it is a `:=` declaration. Its `is_mut` flag is
// already set by the parser, but check `share` explicitly so this safety
// bypass does not depend on that incidental detail.
left_is_lockable_dest := left is ast.Ident && left.info is ast.IdentVar
&& left.info.share in [.shared_t, .atomic_t]
mut right_is_immutable_or_unwrap := false
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) {
unwrapped_right := right.remove_par()
or_expr := assign_expr_or_block(unwrapped_right)
right_is_immutable_or_unwrap = or_expr.kind != .absent
&& assign_or_unwrap_source_is_immutable(unwrapped_right)
&& assign_or_block_default_is_safe(or_expr)
}
if left_sym.kind == .map && is_assign && right_sym.kind == .map && !c.inside_unsafe
&& !left.is_blank_ident() && right_is_lvalue
&& !left.is_blank_ident() && right_is_lvalue && !right_is_immutable_or_unwrap
&& (!right_type.is_ptr() || (right is ast.Ident && assign_expr_is_auto_deref(right))) {
// Do not allow `a = b`
c.error('cannot copy map: call `move` or `clone` method (or use a reference)',
Expand Down
14 changes: 14 additions & 0 deletions vlib/v/checker/tests/option_map_or_unwrap_mut_alias_err.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
vlib/v/checker/tests/option_map_or_unwrap_mut_alias_err.vv:14:17: error: cannot copy map: call `move` or `clone` method (or use a reference)
12 | 'x': 1
13 | }]
14 | mut a := xs[0] or { panic('missing') }
| ~~~~~~~~~~~~~~~~~~~~~~~
15 | a['x'] = 2
16 |
vlib/v/checker/tests/option_map_or_unwrap_mut_alias_err.vv:22:13: error: cannot copy map: call `move` or `clone` method (or use a reference)
20 | }
21 | }
22 | mut b := c.translations or { panic('missing') }
| ~~~~~~~~~~~~
23 | b['x'] = 2
24 |
29 changes: 29 additions & 0 deletions vlib/v/checker/tests/option_map_or_unwrap_mut_alias_err.vv
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Or-unwrapping an option map into a *mutable* destination would let the new
// variable alias the container/field storage, so it must still require an
// explicit `clone`/`move`. See vlang/v issue #27867 (the immutable
// `x := opt_map or { ... }` form is allowed, the mutable form is not).
struct Category {
mut:
translations ?map[string]int
}

fn main() {
mut xs := [{
'x': 1
}]
mut a := xs[0] or { panic('missing') }
a['x'] = 2

mut c := Category{
translations: {
'x': 1
}
}
mut b := c.translations or { panic('missing') }
b['x'] = 2

println(xs)
println(c)
println(a)
println(b)
}
7 changes: 7 additions & 0 deletions vlib/v/checker/tests/option_map_or_unwrap_mut_default_err.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
vlib/v/checker/tests/option_map_or_unwrap_mut_default_err.vv:15:7: error: cannot copy map: call `move` or `clone` method (or use a reference)
13 | }
14 | opt := maybe_none()
15 | x := opt or { fallback }
| ~~~
16 | fallback['x'] = 2
17 | println(x)
18 changes: 18 additions & 0 deletions vlib/v/checker/tests/option_map_or_unwrap_mut_default_err.vv
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// `x := opt or { fallback }` makes `x` the `or` block's value when the option is
// empty, so if that default is a mutable map lvalue, `x` aliases it and observes
// later mutations (`fallback['x'] = 2`). The immutable-destination exemption from
// issue #27867 must not apply when the default path can return a mutable map
// lvalue, even if the option source itself is immutable.
fn maybe_none() ?map[string]int {
return none
}

fn main() {
mut fallback := {
'x': 1
}
opt := maybe_none()
x := opt or { fallback }
fallback['x'] = 2
println(x)
}
21 changes: 21 additions & 0 deletions vlib/v/checker/tests/option_map_or_unwrap_mut_source_err.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
vlib/v/checker/tests/option_map_or_unwrap_mut_source_err.vv:18:7: error: cannot copy map: call `move` or `clone` method (or use a reference)
16 | 'x': 1
17 | })
18 | a := opt or { panic('missing') }
| ~~~
19 |
20 | mut c := Category{
vlib/v/checker/tests/option_map_or_unwrap_mut_source_err.vv:25:9: error: cannot copy map: call `move` or `clone` method (or use a reference)
23 | }
24 | }
25 | b := c.translations or { panic('missing') }
| ~~~~~~~~~~~~
26 |
27 | inner := {
vlib/v/checker/tests/option_map_or_unwrap_mut_source_err.vv:33:14: error: cannot copy map: call `move` or `clone` method (or use a reference)
31 | 'a': ?map[string]int(inner)
32 | }
33 | d := m['a'] or { panic('missing') }
| ~~~~~~~~~~~~~~~~~~~~~~~
34 |
35 | println(a)
38 changes: 38 additions & 0 deletions vlib/v/checker/tests/option_map_or_unwrap_mut_source_err.vv
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Or-unwrapping an option map into an immutable local is only safe when the
// *source* can never be mutated later. If the source (variable, field, or index)
// is mutable, the immutable local still aliases the same map storage and would
// observe a later mutation, so the map-copy guard must stay. See vlang/v issue
// #27867 (only immutable-source unwraps like `c := Category{}; x := c.f or {...}`
// are exempted).
struct Category {
mut:
translations ?map[string]int
}

fn main() {
// mutable variable source (the exact case from the review): `a` aliases the
// map in `opt`, which can still be mutated later via `opt?['x'] = ...`.
mut opt := ?map[string]int({
'x': 1
})
a := opt or { panic('missing') }

mut c := Category{
translations: {
'x': 1
}
}
b := c.translations or { panic('missing') }

inner := {
'x': 1
}
mut m := {
'a': ?map[string]int(inner)
}
d := m['a'] or { panic('missing') }

println(a)
println(b)
println(d)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
vlib/v/checker/tests/option_map_or_unwrap_preserves_option_err.vv:14:14: error: cannot copy map: call `move` or `clone` method (or use a reference)
12 | 'a': ?map[string]int(inner)
13 | }
14 | v := m['a'] or { none }
| ~~~~~~~~~~~
15 | println(v)
16 | }
16 changes: 16 additions & 0 deletions vlib/v/checker/tests/option_map_or_unwrap_preserves_option_err.vv
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// For `map[K]?map[...]`, an `or` block that returns `none` (or another `?map`)
// does NOT clear the option: `v := m[k] or { none }` keeps the option-map type,
// so `v` is still an option handle that aliases the map stored in `m`. The
// map-copy guard must stay in that case, even though the destination is an
// immutable declaration. See vlang/v issue #27867 (only the option-clearing
// form `m[k] or { panic(...) }` is exempted).
fn main() {
inner := {
'x': 1
}
m := {
'a': ?map[string]int(inner)
}
v := m['a'] or { none }
println(v)
}
21 changes: 21 additions & 0 deletions vlib/v/checker/tests/option_map_or_unwrap_ptr_source_err.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
vlib/v/checker/tests/option_map_or_unwrap_ptr_source_err.vv:13:9: error: cannot copy map: call `move` or `clone` method (or use a reference)
11 | // immutable reference parameter: `x` aliases the caller's map, which the
12 | // caller can still mutate.
13 | x := p.translations or { panic('missing') }
| ~~~~~~~~~~~~
14 | return x
15 | }
vlib/v/checker/tests/option_map_or_unwrap_ptr_source_err.vv:26:9: error: cannot copy map: call `move` or `clone` method (or use a reference)
24 | // aliases `c.translations`, which is mutated afterwards through `c`.
25 | p := &c
26 | a := p.translations or { panic('missing') }
| ~~~~~~~~~~~~
27 | c.translations?['x'] = 2
28 |
vlib/v/checker/tests/option_map_or_unwrap_ptr_source_err.vv:31:14: error: cannot copy map: call `move` or `clone` method (or use a reference)
29 | // pointer as an array element receiver.
30 | arr := [&c]
31 | b := arr[0].translations or { panic('missing') }
| ~~~~~~~~~~~~
32 |
33 | println(a)
36 changes: 36 additions & 0 deletions vlib/v/checker/tests/option_map_or_unwrap_ptr_source_err.vv
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// An immutable pointer/reference can still alias storage that its owner mutates
// later, so or-unwrapping an option map through a pointer root (or a selector
// receiver whose type is a pointer) must keep the map-copy guard, even though the
// pointer binding itself is immutable. See vlang/v issue #27867.
struct Category {
mut:
translations ?map[string]int
}

fn through_ref_param(p &Category) map[string]int {
// immutable reference parameter: `x` aliases the caller's map, which the
// caller can still mutate.
x := p.translations or { panic('missing') }
return x
}

fn main() {
mut c := Category{
translations: {
'x': 1
}
}
// immutable pointer to mutable data (the exact case from the review): `a`
// aliases `c.translations`, which is mutated afterwards through `c`.
p := &c
a := p.translations or { panic('missing') }
c.translations?['x'] = 2

// pointer as an array element receiver.
arr := [&c]
b := arr[0].translations or { panic('missing') }

println(a)
println(b)
println(through_ref_param(&c))
}
7 changes: 7 additions & 0 deletions vlib/v/checker/tests/option_map_or_unwrap_shared_dest_err.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
vlib/v/checker/tests/option_map_or_unwrap_shared_dest_err.vv:16:16: error: cannot copy map: call `move` or `clone` method (or use a reference)
14 | }
15 | }
16 | shared b := c.translations or { panic('missing') }
| ~~~~~~~~~~~~
17 | lock b {
18 | b['x'] = 2
23 changes: 23 additions & 0 deletions vlib/v/checker/tests/option_map_or_unwrap_shared_dest_err.vv
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// A `shared` (or `atomic`) destination is mutable under `lock`, so or-unwrapping
// an option map into it must keep the map-copy guard: the shallow copy would
// alias the map still stored in `c`, and `lock b { b['x'] = 2 }` could mutate it.
// The immutable-destination exemption from issue #27867 must not apply here.
struct Category {
mut:
translations ?map[string]int
}

fn main() {
c := Category{
translations: {
'x': 1
}
}
shared b := c.translations or { panic('missing') }
lock b {
b['x'] = 2
}
println(rlock b {
b.clone()
})
}
Loading
Loading