-
-
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 4 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,20 @@ 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. | ||
| 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) } | ||
|
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 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 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. 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.
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:
Added |
||
| ast.ParExpr { assign_or_unwrap_source_is_immutable(expr.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 | ||
|
|
@@ -886,8 +900,37 @@ 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. | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the declaration is 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. Addressed in 6448418, with a nuance worth flagging: on this codebase the shared case was already rejected. The parser sets That said, relying on 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 |
||
| && !right_type.has_flag(.option) && !right_type.has_flag(.result) { | ||
|
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.
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 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. 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: I've also extended |
||
| unwrapped_right := right.remove_par() | ||
| has_or_block := match unwrapped_right { | ||
| ast.Ident { unwrapped_right.or_expr.kind != .absent } | ||
| ast.IndexExpr { unwrapped_right.or_expr.kind != .absent } | ||
| ast.SelectorExpr { unwrapped_right.or_block.kind != .absent } | ||
| else { false } | ||
| } | ||
|
|
||
| right_is_immutable_or_unwrap = has_or_block | ||
| && assign_or_unwrap_source_is_immutable(unwrapped_right) | ||
|
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 option source is immutable but 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 e65b20e. Confirmed 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 x
There are two sound resolutions, and I think it's worth picking one instead of continuing:
Happy to implement either — which do you prefer? |
||
| } | ||
| 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,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,21 @@ | ||
| vlib/v/checker/tests/option_map_or_unwrap_mut_source_err.vv:17:7: error: cannot copy map: call `move` or `clone` method (or use a reference) | ||
| 15 | 'x': 1 | ||
| 16 | } | ||
| 17 | a := opt or { panic('missing') } | ||
| | ~~~ | ||
| 18 | | ||
| 19 | mut c := Category{ | ||
| vlib/v/checker/tests/option_map_or_unwrap_mut_source_err.vv:24:9: error: cannot copy map: call `move` or `clone` method (or use a reference) | ||
| 22 | } | ||
| 23 | } | ||
| 24 | b := c.translations or { panic('missing') } | ||
| | ~~~~~~~~~~~~ | ||
| 25 | | ||
| 26 | inner := { | ||
| vlib/v/checker/tests/option_map_or_unwrap_mut_source_err.vv:32:14: error: cannot copy map: call `move` or `clone` method (or use a reference) | ||
| 30 | 'a': ?map[string]int(inner) | ||
| 31 | } | ||
| 32 | d := m['a'] or { panic('missing') } | ||
| | ~~~~~~~~~~~~~~~~~~~~~~~ | ||
| 33 | | ||
| 34 | println(a) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| // 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() { | ||
| mut opt := ?map[string]int(none) | ||
| opt = { | ||
| '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,70 @@ | ||
| // Regression test for https://github.com/vlang/v/issues/27867 | ||
| // or-unwrapping an option map (struct field, variable, index, parenthesized) | ||
| // into an immutable variable used to fail with | ||
| // `cannot copy map: call move or clone method (or use a reference)`. | ||
| // The mutable form (`mut x := m or { ... }`) still errors on purpose, see | ||
| // vlib/v/checker/tests/option_map_or_unwrap_mut_alias_err.vv. | ||
| struct Translation { | ||
| title string | ||
| } | ||
|
|
||
| struct Category { | ||
| title string | ||
| translations ?map[string]Translation | ||
| } | ||
|
|
||
| fn test_option_map_field_or_unwrap() { | ||
| c := Category{ | ||
| translations: { | ||
| 'en': Translation{ | ||
| title: 'Hello' | ||
| } | ||
| } | ||
| } | ||
| translations := c.translations or { panic('expected') } | ||
| assert translations['en'].title == 'Hello' | ||
| } | ||
|
|
||
| fn test_option_map_field_or_unwrap_none() { | ||
| c := Category{} | ||
| translations := c.translations or { | ||
| map[string]Translation{} | ||
| } | ||
|
|
||
| assert translations.len == 0 | ||
| } | ||
|
|
||
| fn get_option_map() ?map[string]int { | ||
| return { | ||
| 'a': 1 | ||
| } | ||
| } | ||
|
|
||
| fn test_option_map_var_or_unwrap() { | ||
| x := get_option_map() | ||
| y := x or { panic('expected') } | ||
| assert y['a'] == 1 | ||
| } | ||
|
|
||
| fn test_option_map_index_or_unwrap() { | ||
| inner_map := { | ||
| 'b': 2 | ||
| } | ||
| m := { | ||
| 'a': inner_map | ||
| } | ||
| inner := m['a'] or { panic('expected') } | ||
| assert inner['b'] == 2 | ||
| } | ||
|
|
||
| fn test_option_map_field_paren_or_unwrap() { | ||
| c := Category{ | ||
| translations: { | ||
| 'en': Translation{ | ||
| title: 'Hello' | ||
| } | ||
| } | ||
| } | ||
| translations := (c.translations or { panic('expected') }) | ||
| assert translations['en'].title == 'Hello' | ||
| } |
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.
This treats any non-
mutidentifier as storage that cannot be mutated later, but an immutable?mapbinding 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'] = 2now bypasses the map-copy guard andxobserves the later mutation. The same applies to?mapparameters passed from mutable callers, so suppressing the guard needs an ownership/freshness check rather than just!expr.is_mut().Useful? React with 👍 / 👎.