[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 <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
(cherry picked from commit 107c5b2)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/650757
Reviewed-by: Alan Donovan <[email protected]>
Auto-Submit: Robert Findley <[email protected]>

Author: adonovan

gopls/internal/analysis/modernize/maps.go

@@ -32,7 +32,7 @@ import (
 //
 //	maps.Copy(m, x)		(x is map)
 //	maps.Insert(m, x)       (x is iter.Seq2)
-//	m = maps.Clone(x)       (x is map, m is a new map)
+//	m = maps.Clone(x)       (x is a non-nil map, m is a new map)
 //	m = maps.Collect(x)     (x is iter.Seq2, m is a new map)
 //
 // A map is newly created if the preceding statement has one of these
@@ -77,6 +77,8 @@ func mapsloop(pass *analysis.Pass) {
 		// Is the preceding statement of the form
 		//    m = make(M) or M{}
 		// and can we replace its RHS with slices.{Clone,Collect}?
+		//
+		// Beware: if x may be nil, we cannot use Clone as it preserves nilness.
 		var mrhs ast.Expr // make(M) or M{}, or nil
 		if curPrev, ok := curRange.PrevSibling(); ok {
 			if assign, ok := curPrev.Node().(*ast.AssignStmt); ok &&
@@ -122,6 +124,16 @@ func mapsloop(pass *analysis.Pass) {
 							mrhs = rhs
 						}
 					}
+
+					// Temporarily disable the transformation to the
+					// (nil-preserving) maps.Clone until we can prove
+					// that x is non-nil. This is rarely possible,
+					// and may require control flow analysis
+					// (e.g. a dominating "if len(x)" check).
+					// See #71844.
+					if xmap {
+						mrhs = nil
+					}
 				}
 			}
 		}

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

@@ -27,11 +27,34 @@ func useCopyGeneric[K comparable, V any, M ~map[K]V](dst, src M) {
 	}
 }
 
-func useClone(src map[int]string) {
-	// Replace make(...) by maps.Clone.
+func useCopyNotClone(src map[int]string) {
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
+	// Replace make(...) by maps.Copy.
 	dst := make(map[int]string, len(src))
 	for key, value := range src {
-		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
+		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
+	}
+
+	dst = map[int]string{}
+	for key, value := range src {
+		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
+	}
+	println(dst)
+}
+
+func useCopyParen(src map[int]string) {
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
+	// Replace (make)(...) by maps.Clone.
+	dst := (make)(map[int]string, len(src))
+	for key, value := range src {
+		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
+	}
+
+	dst = (map[int]string{})
+	for key, value := range src {
+		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
 	}
 	println(dst)
 }
@@ -55,32 +78,38 @@ func useCopy_typesDiffer2(src map[int]string) {
 }
 
 func useClone_typesDiffer3(src map[int]string) {
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
 	// Replace loop and make(...) as maps.Clone(src) returns map[int]string
 	// which is assignable to M.
 	var dst M
 	dst = make(M, len(src))
 	for key, value := range src {
-		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
+		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
 	}
 	println(dst)
 }
 
 func useClone_typesDiffer4(src map[int]string) {
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
 	// Replace loop and make(...) as maps.Clone(src) returns map[int]string
 	// which is assignable to M.
 	var dst M
 	dst = make(M, len(src))
 	for key, value := range src {
-		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
+		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
 	}
 	println(dst)
 }
 
 func useClone_generic[Map ~map[K]V, K comparable, V any](src Map) {
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
 	// Replace loop and make(...) by maps.Clone
 	dst := make(Map, len(src))
 	for key, value := range src {
-		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
+		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
 	}
 	println(dst)
 }

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

@@ -23,9 +23,27 @@ func useCopyGeneric[K comparable, V any, M ~map[K]V](dst, src M) {
 	maps.Copy(dst, src)
 }
 
-func useClone(src map[int]string) {
-	// Replace make(...) by maps.Clone.
-	dst := maps.Clone(src)
+func useCopyNotClone(src map[int]string) {
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
+	// Replace make(...) by maps.Copy.
+	dst := make(map[int]string, len(src))
+	maps.Copy(dst, src)
+
+	dst = map[int]string{}
+	maps.Copy(dst, src)
+	println(dst)
+}
+
+func useCopyParen(src map[int]string) {
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
+	// Replace (make)(...) by maps.Clone.
+	dst := (make)(map[int]string, len(src))
+	maps.Copy(dst, src)
+
+	dst = (map[int]string{})
+	maps.Copy(dst, src)
 	println(dst)
 }
 
@@ -44,24 +62,33 @@ func useCopy_typesDiffer2(src map[int]string) {
 }
 
 func useClone_typesDiffer3(src map[int]string) {
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
 	// Replace loop and make(...) as maps.Clone(src) returns map[int]string
 	// which is assignable to M.
 	var dst M
-	dst = maps.Clone(src)
+	dst = make(M, len(src))
+	maps.Copy(dst, src)
 	println(dst)
 }
 
 func useClone_typesDiffer4(src map[int]string) {
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
 	// Replace loop and make(...) as maps.Clone(src) returns map[int]string
 	// which is assignable to M.
 	var dst M
-	dst = maps.Clone(src)
+	dst = make(M, len(src))
+	maps.Copy(dst, src)
 	println(dst)
 }
 
 func useClone_generic[Map ~map[K]V, K comparable, V any](src Map) {
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
 	// Replace loop and make(...) by maps.Clone
-	dst := maps.Clone(src)
+	dst := make(Map, len(src))
+	maps.Copy(dst, src)
 	println(dst)
 }
 

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

@@ -14,10 +14,12 @@ func useCopyDot(dst, src map[int]string) {
 }
 
 func useCloneDot(src map[int]string) {
-	// Replace make(...) by maps.Clone.
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
+	// Replace make(...) by maps.Copy.
 	dst := make(map[int]string, len(src))
 	for key, value := range src {
-		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Clone"
+		dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy"
 	}
 	println(dst)
 }

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

@@ -12,8 +12,10 @@ func useCopyDot(dst, src map[int]string) {
 }
 
 func useCloneDot(src map[int]string) {
-	// Replace make(...) by maps.Clone.
-	dst := Clone(src)
+	// Clone is tempting but wrong when src may be nil; see #71844.
+
+	// Replace make(...) by maps.Copy.
+	dst := make(map[int]string, len(src))
+	Copy(dst, src)
 	println(dst)
 }
-