Skip to content

Commit 260fd3c

Browse files
adonovangopherbot
adonovan
authored and
gopherbot
committed
[gopls-release-branch.0.18] gopls/internal/analysis/modernize: disable unsound maps.Clone fix
The fix is sound only if the operand is provably non-nil. My word, these simple cases are subtle. Are we wrong to want to expand access to the framework? Fixes golang/go#71844 Change-Id: I5cf414cae41bf9b6445b7a4a7175dbb9c292e4b8 Reviewed-on: https://go-review.googlesource.com/c/tools/+/650756 Reviewed-by: Jonathan Amsterdam <jba@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Alan Donovan <adonovan@google.com> (cherry picked from commit 107c5b2) Reviewed-on: https://go-review.googlesource.com/c/tools/+/650757 Reviewed-by: Alan Donovan <adonovan@google.com> Auto-Submit: Robert Findley <rfindley@google.com>
1 parent 78316fa commit 260fd3c

File tree

5 files changed

+90
-18
lines changed

5 files changed

+90
-18
lines changed

‎gopls/internal/analysis/modernize/maps.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ import (
3232
//
3333
// maps.Copy(m, x) (x is map)
3434
// maps.Insert(m, x) (x is iter.Seq2)
35-
// m = maps.Clone(x) (x is map, m is a new map)
35+
// m = maps.Clone(x) (x is a non-nil map, m is a new map)
3636
// m = maps.Collect(x) (x is iter.Seq2, m is a new map)
3737
//
3838
// A map is newly created if the preceding statement has one of these
@@ -77,6 +77,8 @@ func mapsloop(pass *analysis.Pass) {
7777
// Is the preceding statement of the form
7878
// m = make(M) or M{}
7979
// and can we replace its RHS with slices.{Clone,Collect}?
80+
//
81+
// Beware: if x may be nil, we cannot use Clone as it preserves nilness.
8082
var mrhs ast.Expr // make(M) or M{}, or nil
8183
if curPrev, ok := curRange.PrevSibling(); ok {
8284
if assign, ok := curPrev.Node().(*ast.AssignStmt); ok &&
@@ -122,6 +124,16 @@ func mapsloop(pass *analysis.Pass) {
122124
mrhs = rhs
123125
}
124126
}
127+
128+
// Temporarily disable the transformation to the
129+
// (nil-preserving) maps.Clone until we can prove
130+
// that x is non-nil. This is rarely possible,
131+
// and may require control flow analysis
132+
// (e.g. a dominating "if len(x)" check).
133+
// See #71844.
134+
if xmap {
135+
mrhs = nil
136+
}
125137
}
126138
}
127139
}

‎gopls/internal/analysis/modernize/testdata/src/mapsloop/mapsloop.go‎

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,34 @@ func useCopyGeneric[K comparable, V any, M ~map[K]V](dst, src M) {
2727
}
2828
}
2929

30-
func useClone(src map[int]string) {
31-
// Replace make(...) by maps.Clone.
30+
func useCopyNotClone(src map[int]string) {
31+
// Clone is tempting but wrong when src may be nil; see #71844.
32+
33+
// Replace make(...) by maps.Copy.
3234
dst := make(map[int]string, len(src))
3335
for key, value := range src {
34-
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
36+
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
37+
}
38+
39+
dst = map[int]string{}
40+
for key, value := range src {
41+
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
42+
}
43+
println(dst)
44+
}
45+
46+
func useCopyParen(src map[int]string) {
47+
// Clone is tempting but wrong when src may be nil; see #71844.
48+
49+
// Replace (make)(...) by maps.Clone.
50+
dst := (make)(map[int]string, len(src))
51+
for key, value := range src {
52+
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
53+
}
54+
55+
dst = (map[int]string{})
56+
for key, value := range src {
57+
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
3558
}
3659
println(dst)
3760
}
@@ -55,32 +78,38 @@ func useCopy_typesDiffer2(src map[int]string) {
5578
}
5679

5780
func useClone_typesDiffer3(src map[int]string) {
81+
// Clone is tempting but wrong when src may be nil; see #71844.
82+
5883
// Replace loop and make(...) as maps.Clone(src) returns map[int]string
5984
// which is assignable to M.
6085
var dst M
6186
dst = make(M, len(src))
6287
for key, value := range src {
63-
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
88+
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
6489
}
6590
println(dst)
6691
}
6792

6893
func useClone_typesDiffer4(src map[int]string) {
94+
// Clone is tempting but wrong when src may be nil; see #71844.
95+
6996
// Replace loop and make(...) as maps.Clone(src) returns map[int]string
7097
// which is assignable to M.
7198
var dst M
7299
dst = make(M, len(src))
73100
for key, value := range src {
74-
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
101+
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
75102
}
76103
println(dst)
77104
}
78105

79106
func useClone_generic[Map ~map[K]V, K comparable, V any](src Map) {
107+
// Clone is tempting but wrong when src may be nil; see #71844.
108+
80109
// Replace loop and make(...) by maps.Clone
81110
dst := make(Map, len(src))
82111
for key, value := range src {
83-
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
112+
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
84113
}
85114
println(dst)
86115
}

‎gopls/internal/analysis/modernize/testdata/src/mapsloop/mapsloop.go.golden‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,27 @@ func useCopyGeneric[K comparable, V any, M ~map[K]V](dst, src M) {
2323
maps.Copy(dst, src)
2424
}
2525

26-
func useClone(src map[int]string) {
27-
// Replace make(...) by maps.Clone.
28-
dst := maps.Clone(src)
26+
func useCopyNotClone(src map[int]string) {
27+
// Clone is tempting but wrong when src may be nil; see #71844.
28+
29+
// Replace make(...) by maps.Copy.
30+
dst := make(map[int]string, len(src))
31+
maps.Copy(dst, src)
32+
33+
dst = map[int]string{}
34+
maps.Copy(dst, src)
35+
println(dst)
36+
}
37+
38+
func useCopyParen(src map[int]string) {
39+
// Clone is tempting but wrong when src may be nil; see #71844.
40+
41+
// Replace (make)(...) by maps.Clone.
42+
dst := (make)(map[int]string, len(src))
43+
maps.Copy(dst, src)
44+
45+
dst = (map[int]string{})
46+
maps.Copy(dst, src)
2947
println(dst)
3048
}
3149

@@ -44,24 +62,33 @@ func useCopy_typesDiffer2(src map[int]string) {
4462
}
4563

4664
func useClone_typesDiffer3(src map[int]string) {
65+
// Clone is tempting but wrong when src may be nil; see #71844.
66+
4767
// Replace loop and make(...) as maps.Clone(src) returns map[int]string
4868
// which is assignable to M.
4969
var dst M
50-
dst = maps.Clone(src)
70+
dst = make(M, len(src))
71+
maps.Copy(dst, src)
5172
println(dst)
5273
}
5374

5475
func useClone_typesDiffer4(src map[int]string) {
76+
// Clone is tempting but wrong when src may be nil; see #71844.
77+
5578
// Replace loop and make(...) as maps.Clone(src) returns map[int]string
5679
// which is assignable to M.
5780
var dst M
58-
dst = maps.Clone(src)
81+
dst = make(M, len(src))
82+
maps.Copy(dst, src)
5983
println(dst)
6084
}
6185

6286
func useClone_generic[Map ~map[K]V, K comparable, V any](src Map) {
87+
// Clone is tempting but wrong when src may be nil; see #71844.
88+
6389
// Replace loop and make(...) by maps.Clone
64-
dst := maps.Clone(src)
90+
dst := make(Map, len(src))
91+
maps.Copy(dst, src)
6592
println(dst)
6693
}
6794

‎gopls/internal/analysis/modernize/testdata/src/mapsloop/mapsloop_dot.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,12 @@ func useCopyDot(dst, src map[int]string) {
1414
}
1515

1616
func useCloneDot(src map[int]string) {
17-
// Replace make(...) by maps.Clone.
17+
// Clone is tempting but wrong when src may be nil; see #71844.
18+
19+
// Replace make(...) by maps.Copy.
1820
dst := make(map[int]string, len(src))
1921
for key, value := range src {
20-
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
22+
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
2123
}
2224
println(dst)
2325
}

‎gopls/internal/analysis/modernize/testdata/src/mapsloop/mapsloop_dot.go.golden‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,10 @@ func useCopyDot(dst, src map[int]string) {
1212
}
1313

1414
func useCloneDot(src map[int]string) {
15-
// Replace make(...) by maps.Clone.
16-
dst := Clone(src)
15+
// Clone is tempting but wrong when src may be nil; see #71844.
16+
17+
// Replace make(...) by maps.Copy.
18+
dst := make(map[int]string, len(src))
19+
Copy(dst, src)
1720
println(dst)
1821
}
19-

0 commit comments

Comments
 (0)