-
-
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
Open
medvednikov
wants to merge
16
commits into
master
Choose a base branch
from
fix-27867-or-unwrap-option-map
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+56
−0
Open
Changes from 15 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 756eced
checker: address review — only relax the map-copy guard for immutable…
medvednikov 2a6f7dd
checker: address review — only exempt or-unwrap that clears the option
medvednikov 8c4713c
checker: address review — keep map-copy guard for mutable option sources
medvednikov 66afd39
tests: use the review's exact mutable-source form in the alias regres…
medvednikov b870fb7
checker: reject or-unwrap through immutable pointers to mutable data
medvednikov 6448418
checker: never exempt shared/atomic or-unwrap destinations from map-c…
medvednikov e65b20e
checker: keep map-copy guard when or-block default returns a mutable …
medvednikov 5e63eb7
checker: unalias types before the or-unwrap pointer check
medvednikov b599c51
checker: only exempt fresh/owned or-block map defaults, not immutable…
medvednikov 8f35843
checker: filter semicolons and tighten or-block default classification
medvednikov 6565262
checker: only exempt fresh/noreturn or-block map call defaults
medvednikov b82d49b
checker: restrict fresh or-block clone/move defaults to builtin map ops
medvednikov 01d21af
checker: require direct map receiver for fresh or-block clone/move de…
medvednikov c31f53b
checker: strip parens before classifying or-block map defaults
medvednikov 73ce15c
checker: revert unsound option-map or-unwrap exemption; require clone…
medvednikov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 changes: 7 additions & 0 deletions
7
vlib/v/checker/tests/option_map_or_unwrap_alias_ptr_source_err.out
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) |
23 changes: 23 additions & 0 deletions
23
vlib/v/checker/tests/option_map_or_unwrap_alias_ptr_source_err.vv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) | ||
| } |
7 changes: 7 additions & 0 deletions
7
vlib/v/checker/tests/option_map_or_unwrap_alias_user_clone_default_err.out
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| vlib/v/checker/tests/option_map_or_unwrap_alias_user_clone_default_err.vv:28:7: error: cannot copy map: call `move` or `clone` method (or use a reference) | ||
| 26 | } | ||
| 27 | opt := maybe_none() | ||
| 28 | x := opt or { holder.m.clone() } | ||
| | ~~~ | ||
| 29 | holder.m['x'] = 2 | ||
| 30 | println(x) |
31 changes: 31 additions & 0 deletions
31
vlib/v/checker/tests/option_map_or_unwrap_alias_user_clone_default_err.vv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| // Only the *builtin* map clone/move produce fresh storage. A map type alias can | ||
| // define its own `clone`/`move`, which `fn.v` resolves before the map builtin, so | ||
| // `holder.m.clone()` here runs the user method that returns an alias of `holder.m`. | ||
| // The exemption must require a direct map receiver (not one that merely unaliases | ||
| // to a map), so this default keeps the map-copy guard. See vlang/v issue #27867. | ||
| type MapAlias = map[string]int | ||
|
|
||
| fn (m MapAlias) clone() map[string]int { | ||
| return map[string]int(m) | ||
| } | ||
|
|
||
| fn maybe_none() ?map[string]int { | ||
| return none | ||
| } | ||
|
|
||
| struct Holder { | ||
| mut: | ||
| m MapAlias | ||
| } | ||
|
|
||
| fn main() { | ||
| mut holder := Holder{ | ||
| m: MapAlias({ | ||
| 'x': 1 | ||
| }) | ||
| } | ||
| opt := maybe_none() | ||
| x := opt or { holder.m.clone() } | ||
| holder.m['x'] = 2 | ||
| println(x) | ||
| } |
7 changes: 7 additions & 0 deletions
7
vlib/v/checker/tests/option_map_or_unwrap_call_default_alias_err.out
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| vlib/v/checker/tests/option_map_or_unwrap_call_default_alias_err.vv:19:7: error: cannot copy map: call `move` or `clone` method (or use a reference) | ||
| 17 | } | ||
| 18 | opt := maybe_none() | ||
| 19 | x := opt or { id(fallback) } | ||
| | ~~~ | ||
| 20 | fallback['x'] = 2 | ||
| 21 | println(x) |
22 changes: 22 additions & 0 deletions
22
vlib/v/checker/tests/option_map_or_unwrap_call_default_alias_err.vv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // An `or {}` default that is an arbitrary map-returning call can return a | ||
| // caller-owned alias (`id` just returns its argument), so `x := opt or { id(fb) }` | ||
| // would alias `fb` and observe later mutations — the same as `or { fb }`. Only | ||
| // calls known to produce fresh storage (`clone`/`move`) or `@[noreturn]` calls | ||
| // are exempted. See vlang/v issue #27867. | ||
| fn maybe_none() ?map[string]int { | ||
| return none | ||
| } | ||
|
|
||
| fn id(m map[string]int) map[string]int { | ||
| return m | ||
| } | ||
|
|
||
| fn main() { | ||
| mut fallback := { | ||
| 'x': 1 | ||
| } | ||
| opt := maybe_none() | ||
| x := opt or { id(fallback) } | ||
| fallback['x'] = 2 | ||
| println(x) | ||
| } |
14 changes: 14 additions & 0 deletions
14
vlib/v/checker/tests/option_map_or_unwrap_immut_default_alias_err.out
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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({ |
29 changes: 29 additions & 0 deletions
29
vlib/v/checker/tests/option_map_or_unwrap_immut_default_alias_err.vv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) | ||
| } |
14 changes: 14 additions & 0 deletions
14
vlib/v/checker/tests/option_map_or_unwrap_mut_alias_err.out
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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
29
vlib/v/checker/tests/option_map_or_unwrap_mut_alias_err.vv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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
7
vlib/v/checker/tests/option_map_or_unwrap_mut_default_err.out
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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
18
vlib/v/checker/tests/option_map_or_unwrap_mut_default_err.vv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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
21
vlib/v/checker/tests/option_map_or_unwrap_mut_source_err.out
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.