-
-
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 11 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,87 @@ 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 any map | ||
| // lvalue (e.g. `or { fallback }`) then `x` aliases it. Even an immutable lvalue | ||
| // is unsafe: an immutable map parameter/field can alias caller-owned storage the | ||
| // caller mutates later (the same copy `x := d` would reject). Only a fresh/owned | ||
| // value (map literal / by-value call), or a block that yields no value because | ||
| // it diverges (`return`/`break`/`continue`; `panic`/`exit` are calls handled | ||
| // above), is safe. Anything else — an lvalue value, or an unclassifiable last | ||
| // statement — keeps the guard. | ||
| fn (c &Checker) assign_or_block_default_is_safe(or_expr ast.OrExpr) bool { | ||
| if or_expr.kind != .block { | ||
| return true | ||
| } | ||
| // A trailing `;` leaves a `SemicolonStmt`; filter those out so the block's | ||
| // real value statement is classified, matching `check_or_expr`. | ||
| valid_stmts := or_expr.stmts.filter(it !is ast.SemicolonStmt) | ||
| if valid_stmts.len == 0 { | ||
| return true | ||
| } | ||
| last := valid_stmts.last() | ||
| return match last { | ||
| ast.ExprStmt { | ||
| match last.expr { | ||
| ast.MapInit, ast.CallExpr { 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 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 6565262. Confirmed 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 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 |
||
| else { false } | ||
| } | ||
| } | ||
| ast.Return, ast.BranchStmt { | ||
| true | ||
| } | ||
| 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 +967,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_immut_default_alias_err.vv:13:7: error: cannot copy map: call `move` or `clone` method (or use a reference) | ||
| 11 | opt := maybe_none() | ||
| 12 | // immutable parameter fallback: `x` can alias the caller's map. | ||
| 13 | x := opt or { fallback } | ||
| | ~~~ | ||
| 14 | return x | ||
| 15 | } | ||
| vlib/v/checker/tests/option_map_or_unwrap_immut_default_alias_err.vv:23:7: error: cannot copy map: call `move` or `clone` method (or use a reference) | ||
| 21 | opt := maybe_none() | ||
| 22 | // immutable local lvalue fallback is rejected too (cannot prove freshness). | ||
| 23 | y := opt or { local } | ||
| | ~~~ | ||
| 24 | | ||
| 25 | println(pick({ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| // An `or {}` block whose fallback is an immutable map lvalue is still unsafe: | ||
| // an immutable map parameter (or any immutable alias) can point at caller-owned | ||
| // storage the caller mutates later, so `x := opt or { fallback }` would alias it | ||
| // — the same copy `x := fallback` rejects. Only fresh/owned fallbacks (map | ||
| // literal / by-value call) are exempted. See vlang/v issue #27867. | ||
| fn maybe_none() ?map[string]int { | ||
| return none | ||
| } | ||
|
|
||
| fn pick(fallback map[string]int) map[string]int { | ||
| opt := maybe_none() | ||
| // immutable parameter fallback: `x` can alias the caller's map. | ||
| x := opt or { fallback } | ||
| return x | ||
| } | ||
|
|
||
| fn main() { | ||
| local := { | ||
| 'y': 9 | ||
| } | ||
| opt := maybe_none() | ||
| // immutable local lvalue fallback is rejected too (cannot prove freshness). | ||
| y := opt or { local } | ||
|
|
||
| println(pick({ | ||
| 'x': 1 | ||
| })) | ||
| println(y) | ||
| } |
| 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) |
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.