internal/analysisinternal: extract DeleteVar

This CL extracts the "delete variable" operation from the
unusedvariable analyzer to a new library function.
The implementation is completely rewritten to handle
the surprising number of edge cases more systematically,
and to make greater use of the library, notably:

- Cursor instead of PathEnclosingInterval;
- analysisinternal.DeleteStmt instead of its own
  deleteStmtFromBlock algorithm; and
- typesinternal.NoEffects (formerly modernize.noeffects)
  instead of its own weaker algorithm.

It also avoids cloning/mutating/formatting syntax trees,
which we have learned is not a robust way to compute edits.
The change also includes a comprehensive test suite.

The existing analysisinternal.DeleteStmt algorithm preserves
trailing line comments in places where the DeleteVar logic
wants to remove them. I think DeleteStmt needs to change,
but to avoid making yet more subtle behavior changes in
this CL, I've commented out 3 test cases until I can
discuss it with pjw.

Change-Id: Ie53383361d598eaa12e68f20b4a6188b6f218bdb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/709055
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>

Author: adonovan

go/analysis/passes/modernize/modernize.go

@@ -151,44 +151,6 @@ var (
 	omitemptyRegex = regexp.MustCompile(`(?:^json| json):"[^"]*(,omitempty)(?:"|,[^"]*")\s?`)
 )
 
-// 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.Fun].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
-}
-
 // lookup returns the symbol denoted by name at the position of the cursor.
 func lookup(info *types.Info, cur inspector.Cursor, name string) types.Object {
 	scope := analysisinternal.EnclosingScope(info, cur)

go/analysis/passes/modernize/reflect.go

@@ -49,7 +49,7 @@ func reflecttypefor(pass *analysis.Pass) (any, error) {
 		// Have: reflect.TypeOf(expr)
 
 		expr := call.Args[0]
-		if !noEffects(info, expr) {
+		if !typesinternal.NoEffects(info, expr) {
 			continue // don't eliminate operand: may have effects
 		}
 

go/analysis/passes/modernize/slicesdelete.go

@@ -16,6 +16,7 @@ import (
 	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/analysisinternal/generated"
 	"golang.org/x/tools/internal/astutil"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 // Warning: this analyzer is not safe to enable by default (not nil-preserving).
@@ -141,7 +142,8 @@ func slicesdelete(pass *analysis.Pass) (any, error) {
 					slice2, ok2 := call.Args[1].(*ast.SliceExpr)
 					if ok1 && slice1.Low == nil && !slice1.Slice3 &&
 						ok2 && slice2.High == nil && !slice2.Slice3 &&
-						astutil.EqualSyntax(slice1.X, slice2.X) && noEffects(info, slice1.X) &&
+						astutil.EqualSyntax(slice1.X, slice2.X) &&
+						typesinternal.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/unusedvariable/testdata/src/assign/a.go

@@ -55,8 +55,10 @@ func sideEffects(cBool chan bool, cInt chan int) {
 	f := map[int]int{       // want `declared (and|but) not used`
 		fInt(): <-cInt,
 	}
-	g := []int{<-cInt}       // want `declared (and|but) not used`
-	h := func(s string) {}   // want `declared (and|but) not used`
+	g := []int{<-cInt}     // want `declared (and|but) not used`
+	h := func(s string) {} // want `declared (and|but) not used`
+
+	// (ill-typed)
 	i := func(s string) {}() // want `declared (and|but) not used`
 }
 

gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go.golden

@@ -14,13 +14,17 @@ type A struct {
 }
 
 func singleAssignment() {
+	// want `declared (and|but) not used`
+
 	if 1 == 1 {
+		// want `declared (and|but) not used`
 	}
 
 	panic("I should survive")
 }
 
 func noOtherStmtsInBlock() {
+	// want `declared (and|but) not used`
 }
 
 func partOfMultiAssignment() {
@@ -29,32 +33,39 @@ func partOfMultiAssignment() {
 }
 
 func sideEffects(cBool chan bool, cInt chan int) {
-	<-c            // want `declared (and|but) not used`
-	fmt.Sprint("") // want `declared (and|but) not used`
-	A{             // want `declared (and|but) not used`
+	_ = <-c            // want `declared (and|but) not used`
+	_ = fmt.Sprint("") // want `declared (and|but) not used`
+	_ = A{             // want `declared (and|but) not used`
 		b: func() int {
 			return 1
 		}(),
 	}
-	A{<-cInt}          // want `declared (and|but) not used`
-	fInt() + <-cInt    // want `declared (and|but) not used`
-	fBool() && <-cBool // want `declared (and|but) not used`
-	map[int]int{       // want `declared (and|but) not used`
+	_ = A{<-cInt}          // want `declared (and|but) not used`
+	_ = fInt() + <-cInt    // want `declared (and|but) not used`
+	_ = fBool() && <-cBool // want `declared (and|but) not used`
+	_ = map[int]int{       // want `declared (and|but) not used`
 		fInt(): <-cInt,
 	}
-	[]int{<-cInt}       // want `declared (and|but) not used`
-	func(s string) {}() // want `declared (and|but) not used`
+	_ = []int{<-cInt} // want `declared (and|but) not used`
+	// want `declared (and|but) not used`
+
+	// (ill-typed)
+	_ = func(s string) {}() // want `declared (and|but) not used`
 }
 
 func commentAbove() {
 	// v is a variable
+	// want `declared (and|but) not used`
 }
 
 func commentBelow() {
+	// want `declared (and|but) not used`
 	// v is a variable
 }
 
 func commentSpaceBelow() {
+	// want `declared (and|but) not used`
+
 	// v is a variable
 }
 

gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go.golden

@@ -9,16 +9,19 @@ func a() {
 	panic(c)
 
 	if 1 == 1 {
+		// want `declared (and|but) not used`
 	}
 }
 
 func b() {
 	// b is a variable
+	// want `declared (and|but) not used`
 }
 
 func c() {
 	var (
 		d string
 	)
+
 	panic(d)
 }