-
-
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.
Open
Changes from 2 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
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) | ||
| } |
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,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' | ||
| } |
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.
This exception only checks that the new local is immutable, but the existing map-copy check also rejects immutable aliases because the original map can still be mutated later. For example,
mut opt := ?map[string]int{}; opt = {'x': 1}; x := opt or { panic('missing') }; opt?['x'] = 2is now allowed, andxaliases the same map storage and observes the mutation. Limit this bypass to sources that cannot be mutated later, or requireclone/movefor mutable option-map variables/fields/indexes.Useful? React with 👍 / 👎.