Skip to content

Commit 107c5b2

Browse files
adonovangopherbot
adonovan
authored and
gopherbot
committed
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 <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Alan Donovan <[email protected]>
1 parent 0b693ed commit 107c5b2

File tree

5 files changed

+69
-26
lines changed

5 files changed

+69
-26
lines changed

Diff for: ‎gopls/internal/analysis/modernize/maps.go

+13-1
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
}

Diff for: ‎gopls/internal/analysis/modernize/testdata/src/mapsloop/mapsloop.go

+20-10
Original file line numberDiff line numberDiff line change
@@ -27,30 +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"
3537
}
3638

3739
dst = map[int]string{}
3840
for key, value := range src {
39-
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
41+
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
4042
}
4143
println(dst)
4244
}
4345

44-
func useCloneParen(src map[int]string) {
46+
func useCopyParen(src map[int]string) {
47+
// Clone is tempting but wrong when src may be nil; see #71844.
48+
4549
// Replace (make)(...) by maps.Clone.
4650
dst := (make)(map[int]string, len(src))
4751
for key, value := range src {
48-
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
52+
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
4953
}
5054

5155
dst = (map[int]string{})
5256
for key, value := range src {
53-
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
57+
dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
5458
}
5559
println(dst)
5660
}
@@ -74,32 +78,38 @@ func useCopy_typesDiffer2(src map[int]string) {
7478
}
7579

7680
func useClone_typesDiffer3(src map[int]string) {
81+
// Clone is tempting but wrong when src may be nil; see #71844.
82+
7783
// Replace loop and make(...) as maps.Clone(src) returns map[int]string
7884
// which is assignable to M.
7985
var dst M
8086
dst = make(M, len(src))
8187
for key, value := range src {
82-
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"
8389
}
8490
println(dst)
8591
}
8692

8793
func useClone_typesDiffer4(src map[int]string) {
94+
// Clone is tempting but wrong when src may be nil; see #71844.
95+
8896
// Replace loop and make(...) as maps.Clone(src) returns map[int]string
8997
// which is assignable to M.
9098
var dst M
9199
dst = make(M, len(src))
92100
for key, value := range src {
93-
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"
94102
}
95103
println(dst)
96104
}
97105

98106
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+
99109
// Replace loop and make(...) by maps.Clone
100110
dst := make(Map, len(src))
101111
for key, value := range src {
102-
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"
103113
}
104114
println(dst)
105115
}

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

+27-10
Original file line numberDiff line numberDiff line change
@@ -23,19 +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.
2928

30-
dst = maps.Clone(src)
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)
3135
println(dst)
3236
}
3337

34-
func useCloneParen(src map[int]string) {
38+
func useCopyParen(src map[int]string) {
39+
// Clone is tempting but wrong when src may be nil; see #71844.
40+
3541
// Replace (make)(...) by maps.Clone.
36-
dst := maps.Clone(src)
42+
dst := (make)(map[int]string, len(src))
43+
maps.Copy(dst, src)
3744

38-
dst = maps.Clone(src)
45+
dst = (map[int]string{})
46+
maps.Copy(dst, src)
3947
println(dst)
4048
}
4149

@@ -54,24 +62,33 @@ func useCopy_typesDiffer2(src map[int]string) {
5462
}
5563

5664
func useClone_typesDiffer3(src map[int]string) {
65+
// Clone is tempting but wrong when src may be nil; see #71844.
66+
5767
// Replace loop and make(...) as maps.Clone(src) returns map[int]string
5868
// which is assignable to M.
5969
var dst M
60-
dst = maps.Clone(src)
70+
dst = make(M, len(src))
71+
maps.Copy(dst, src)
6172
println(dst)
6273
}
6374

6475
func useClone_typesDiffer4(src map[int]string) {
76+
// Clone is tempting but wrong when src may be nil; see #71844.
77+
6578
// Replace loop and make(...) as maps.Clone(src) returns map[int]string
6679
// which is assignable to M.
6780
var dst M
68-
dst = maps.Clone(src)
81+
dst = make(M, len(src))
82+
maps.Copy(dst, src)
6983
println(dst)
7084
}
7185

7286
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+
7389
// Replace loop and make(...) by maps.Clone
74-
dst := maps.Clone(src)
90+
dst := make(Map, len(src))
91+
maps.Copy(dst, src)
7592
println(dst)
7693
}
7794

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

+4-2
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
}

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

+5-3
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)