-
-
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 1 commit
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 |
|---|---|---|
|
|
@@ -881,7 +881,17 @@ or use an explicit `unsafe{ a[..] }`, if you do not want a copy of the slice.', | |
| } | ||
| } | ||
| } | ||
| right_is_lvalue := if right is ast.ComptimeSelector { | ||
| // An `or {}` block unwraps an option/result into a fresh value, so the | ||
| // unwrapped expression is not an lvalue that aliases the original map | ||
| // (see vlang/v issue #27867). This mirrors the already allowed `get() or {}` | ||
| // call and option array field cases. | ||
| right_has_or_block := match right { | ||
| ast.Ident { right.or_expr.kind != .absent } | ||
| ast.IndexExpr { right.or_expr.kind != .absent } | ||
|
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. [P1] Keep the map-copy guard for checked index expressions. An 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.
For Useful? React with 👍 / 👎. |
||
| ast.SelectorExpr { right.or_block.kind != .absent } | ||
| else { false } | ||
| } | ||
| right_is_lvalue := !right_has_or_block && if right is ast.ComptimeSelector { | ||
| right.left.is_lvalue() | ||
| } else { | ||
| right.is_lvalue() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| // Regression test for https://github.com/vlang/v/issues/27867 | ||
| // or-unwrapping an option map (struct field, variable, index) used to fail | ||
| // with `cannot copy map: call move or clone method (or use a reference)`. | ||
| 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 | ||
| } |
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.
[P2] Inspect through parentheses before classifying the RHS. This match only sees the outer node.
(c.translations or { panic('expected') })is parsed as aParExprwrapping theSelectorExpr;ParExpr.is_lvalue()delegates to the inner expression, so this still emitscannot copy map. Please normalize/removeParExprwrappers (or make this a recursive helper) and add a parenthesized regression case.