gopls/internal/analysis/modernize: fix slicedelete triggers on slice identifiers with side effects

Add a check that the expression defining the slice has no side effects to trigger slicedelete. This is a necessary condition to ensure that the change does not change the program behavior.

Fixes golang/go#72955

Change-Id: Ic326baa37e0b621fa7ba204bbfeb61c3e7daea47
GitHub-Last-Rev: 54e9082
GitHub-Pull-Request: #567
Reviewed-on: https://go-review.googlesource.com/c/tools/+/659295
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>

Author: ashurbekovz

gopls/internal/analysis/modernize/modernize.go

@@ -187,3 +187,41 @@ func enabledCategory(filter, category string) bool {
 	}
 	return exclude
 }
+
+// noEffects reports whether the expression has no side effects, i.e., it
+// does not modify the memory state. This function is conservative: it may
+// return false even when the expression has no effect.
+func noEffects(info *types.Info, expr ast.Expr) bool {
+	noEffects := true
+	ast.Inspect(expr, func(n ast.Node) bool {
+		switch v := n.(type) {
+		case nil, *ast.Ident, *ast.BasicLit, *ast.BinaryExpr, *ast.ParenExpr,
+			*ast.SelectorExpr, *ast.IndexExpr, *ast.SliceExpr, *ast.TypeAssertExpr,
+			*ast.StarExpr, *ast.CompositeLit, *ast.ArrayType, *ast.StructType,
+			*ast.MapType, *ast.InterfaceType, *ast.KeyValueExpr:
+			// No effect
+		case *ast.UnaryExpr:
+			// Channel send <-ch has effects
+			if v.Op == token.ARROW {
+				noEffects = false
+			}
+		case *ast.CallExpr:
+			// Type conversion has no effects
+			if !info.Types[v].IsType() {
+				// TODO(adonovan): Add a case for built-in functions without side
+				// effects (by using callsPureBuiltin from tools/internal/refactor/inline)
+
+				noEffects = false
+			}
+		case *ast.FuncLit:
+			// A FuncLit has no effects, but do not descend into it.
+			return false
+		default:
+			// All other expressions have effects
+			noEffects = false
+		}
+
+		return noEffects
+	})
+	return noEffects
+}

gopls/internal/analysis/modernize/slices.go

@@ -210,6 +210,9 @@ func appendclipped(pass *analysis.Pass) {
 //	x[:len(x):len(x)]		(nonempty)  res=x
 //	x[:k:k]	 	         	(nonempty)
 //	slices.Clip(x)			(nonempty)  res=x
+//
+// TODO(adonovan): Add a check that the expression x has no side effects in
+// case x[:len(x):len(x)] -> x. Now the program behavior may change.
 func clippedSlice(info *types.Info, e ast.Expr) (res ast.Expr, empty bool) {
 	switch e := e.(type) {
 	case *ast.SliceExpr:

gopls/internal/analysis/modernize/slicescontains.go

@@ -46,6 +46,9 @@ import (
 // It may change cardinality of effects of the "needle" expression.
 // (Mostly this appears to be a desirable optimization, avoiding
 // redundantly repeated evaluation.)
+//
+// TODO(adonovan): Add a check that needle/predicate expression from
+// if-statement has no effects. Now the program behavior may change.
 func slicescontains(pass *analysis.Pass) {
 	// Skip the analyzer in packages where its
 	// fixes would create an import cycle.

gopls/internal/analysis/modernize/slicesdelete.go

@@ -94,7 +94,7 @@ func slicesdelete(pass *analysis.Pass) {
 					slice2, ok2 := call.Args[1].(*ast.SliceExpr)
 					if ok1 && slice1.Low == nil && !slice1.Slice3 &&
 						ok2 && slice2.High == nil && !slice2.Slice3 &&
-						equalSyntax(slice1.X, slice2.X) &&
+						equalSyntax(slice1.X, slice2.X) && noEffects(info, slice1.X) &&
 						increasingSliceIndices(info, slice1.High, slice2.Low) {
 						// Have append(s[:a], s[b:]...) where we can verify a < b.
 						report(file, call, slice1, slice2)

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

@@ -2,6 +2,10 @@ package slicesdelete
 
 var g struct{ f []int }
 
+func h() []int { return []int{} }
+
+var ch chan []int
+
 func slicesdelete(test, other []byte, i int) {
 	const k = 1
 	_ = append(test[:i], test[i+1:]...) // want "Replace append with slices.Delete"
@@ -26,6 +30,10 @@ func slicesdelete(test, other []byte, i int) {
 
 	_ = append(g.f[:i], g.f[i+k:]...) // want "Replace append with slices.Delete"
 
+	_ = append(h()[:i], h()[i+1:]...) // potentially has side effects
+
+	_ = append((<-ch)[:i], (<-ch)[i+1:]...) // has side effects
+
 	_ = append(test[:3], test[i+1:]...) // cannot verify a < b
 
 	_ = append(test[:i-4], test[i-1:]...) // want "Replace append with slices.Delete"

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

@@ -4,35 +4,43 @@ import "slices"
 
 var g struct{ f []int }
 
+func h() []int { return []int{} }
+
+var ch chan []int
+
 func slicesdelete(test, other []byte, i int) {
-    const k = 1
-    _ = slices.Delete(test, i, i+1) // want "Replace append with slices.Delete"
+	const k = 1
+	_ = slices.Delete(test, i, i+1) // want "Replace append with slices.Delete"
+
+	_ = slices.Delete(test, i+1, i+2) // want "Replace append with slices.Delete"
+
+	_ = append(test[:i+1], test[i+1:]...) // not deleting any slice elements
 
-    _ = slices.Delete(test, i+1, i+2) // want "Replace append with slices.Delete"
+	_ = append(test[:i], test[i-1:]...) // not deleting any slice elements
 
-    _ = append(test[:i+1], test[i+1:]...) // not deleting any slice elements
+	_ = slices.Delete(test, i-1, i) // want "Replace append with slices.Delete"
 
-    _ = append(test[:i], test[i-1:]...) // not deleting any slice elements
+	_ = slices.Delete(test, i-2, i+1) // want "Replace append with slices.Delete"
 
-    _ = slices.Delete(test, i-1, i) // want "Replace append with slices.Delete"
+	_ = append(test[:i-2], other[i+1:]...) // different slices "test" and "other"
 
-    _ = slices.Delete(test, i-2, i+1) // want "Replace append with slices.Delete"
+	_ = append(test[:i-2], other[i+1+k:]...) // cannot verify a < b
 
-    _ = append(test[:i-2], other[i+1:]...) // different slices "test" and "other"
+	_ = append(test[:i-2], test[11:]...) // cannot verify a < b
 
-    _ = append(test[:i-2], other[i+1+k:]...) // cannot verify a < b
+	_ = slices.Delete(test, 1, 3) // want "Replace append with slices.Delete"
 
-    _ = append(test[:i-2], test[11:]...) // cannot verify a < b
+	_ = slices.Delete(g.f, i, i+k) // want "Replace append with slices.Delete"
 
-    _ = slices.Delete(test, 1, 3) // want "Replace append with slices.Delete"
+	_ = append(h()[:i], h()[i+1:]...) // potentially has side effects
 
-    _ = slices.Delete(g.f, i, i+k) // want "Replace append with slices.Delete"
+	_ = append((<-ch)[:i], (<-ch)[i+1:]...) // has side effects
 
-    _ = append(test[:3], test[i+1:]...) // cannot verify a < b
+	_ = append(test[:3], test[i+1:]...) // cannot verify a < b
 
-    _ = slices.Delete(test, i-4, i-1) // want "Replace append with slices.Delete"
+	_ = slices.Delete(test, i-4, i-1) // want "Replace append with slices.Delete"
 
-    _ = slices.Delete(test, 1+2, 3+4) // want "Replace append with slices.Delete"
+	_ = slices.Delete(test, 1+2, 3+4) // want "Replace append with slices.Delete"
 
-    _ = append(test[:1+2], test[i-1:]...) // cannot verify a < b
-}
+	_ = append(test[:1+2], test[i-1:]...) // cannot verify a < b
+}