-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
checker: keep map-copy guard for option-map or-unwraps (#27867) #27870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 9 commits
fa25149
756eced
2a6f7dd
8c4713c
66afd39
b870fb7
6448418
e65b20e
5e63eb7
b599c51
8f35843
6565262
b82d49b
01d21af
c31f53b
73ce15c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,77 @@ 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`). Types are | ||
| // fully unaliased first, so a `type Ref = &T` receiver is treated as a pointer. | ||
| fn (c &Checker) assign_or_unwrap_source_is_immutable(expr ast.Expr) bool { | ||
| return match expr { | ||
| ast.Ident { | ||
| if expr.obj is ast.Var { | ||
| !expr.is_mut() && !c.table.fully_unaliased_type(expr.obj.typ).is_ptr() | ||
| } else { | ||
| !expr.is_mut() | ||
| } | ||
| } | ||
| ast.SelectorExpr { | ||
| !c.table.fully_unaliased_type(expr.expr_type).is_ptr() | ||
| && c.assign_or_unwrap_source_is_immutable(expr.expr) | ||
| } | ||
| ast.IndexExpr { | ||
| !c.table.fully_unaliased_type(expr.left_type).is_ptr() | ||
| && c.assign_or_unwrap_source_is_immutable(expr.left) | ||
| } | ||
| ast.ParExpr { | ||
| c.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 (c &Checker) 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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 8f35843. The helper now filters 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: Same standing note as the prior threads: the source-side |
||
| } | ||
| return match (last as ast.ExprStmt).expr { | ||
| ast.MapInit, ast.CallExpr { | ||
| true | ||
| } | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in b599c51. Confirmed return match (last as ast.ExprStmt).expr {
ast.MapInit, ast.CallExpr { true }
else { false }
}Added 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 ( |
||
| } | ||
| 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 | ||
|
|
@@ -886,8 +957,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 | ||
| && c.assign_or_unwrap_source_is_immutable(unwrapped_right) | ||
| && c.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)', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| vlib/v/checker/tests/option_map_or_unwrap_alias_ptr_source_err.vv:20:9: error: cannot copy map: call `move` or `clone` method (or use a reference) | ||
| 18 | } | ||
| 19 | p := CategoryRef(&c) | ||
| 20 | x := p.translations or { panic('missing') } | ||
| | ~~~~~~~~~~~~ | ||
| 21 | c.translations?['x'] = 2 | ||
| 22 | println(x) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| // A `type Ref = &Category` receiver is a pointer once unaliased, so or-unwrapping | ||
| // an option map through it can still alias storage mutated through the original | ||
| // owner. The immutable-source exemption from issue #27867 must unalias types | ||
| // before its pointer check, so an alias-to-pointer receiver keeps the map-copy | ||
| // guard just like a plain `&Category` receiver. | ||
| struct Category { | ||
| mut: | ||
| translations ?map[string]int | ||
| } | ||
|
|
||
| type CategoryRef = &Category | ||
|
|
||
| fn main() { | ||
| mut c := Category{ | ||
| translations: { | ||
| 'x': 1 | ||
| } | ||
| } | ||
| p := CategoryRef(&c) | ||
| x := p.translations or { panic('missing') } | ||
| c.translations?['x'] = 2 | ||
| println(x) | ||
| } |
| 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 | |
| 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) | ||
| } |
| 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) |
| 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) | ||
| } |
| 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) |
| 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 | } |
| 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) | ||
| } |
| 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) |
| 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)) | ||
| } |
| 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 |
| 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() | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 andxobserves the later mutation. Keep requiringclone/moveunless the unwrapped source is proven fresh/owned, not merely immutable.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_immutabletreated immutability as ownership, and those aren't the same —opt := ?map(base)is an immutable binding overbase's mutable storage. There's no localis_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 mapdiagnostic, exactly likem2 := m1.checker/assign.vis 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.