internal/typesinternal: merge HasSideEffects -> NoEffects

Apparently we wrote this function the same way twice.

Also:
- add a couple of missing cases (FuncType, ChanType).
- implement the CallsPureBuiltin optimization, moved
  from internal/refactor/inline.

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

Author: adonovan

go/analysis/passes/assign/assign.go

@@ -21,6 +21,7 @@ import (
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/internal/astutil"
 	"golang.org/x/tools/internal/refactor"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 //go:embed doc.go
@@ -62,8 +63,8 @@ func run(pass *analysis.Pass) (any, error) {
 			isSelfAssign := false
 			var le string
 
-			if !analysisutil.HasSideEffects(info, lhs) &&
-				!analysisutil.HasSideEffects(info, rhs) &&
+			if typesinternal.NoEffects(info, lhs) &&
+				typesinternal.NoEffects(info, rhs) &&
 				!isMapIndex(info, lhs) &&
 				reflect.TypeOf(lhs) == reflect.TypeOf(rhs) { // short-circuit the heavy-weight gofmt check
 

go/analysis/passes/bools/bools.go

@@ -13,9 +13,9 @@ import (
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/analysis/passes/internal/analysisutil"
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/internal/astutil"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 const Doc = "check for common mistakes involving boolean operators"
@@ -84,7 +84,7 @@ func (op boolOp) commutativeSets(info *types.Info, e *ast.BinaryExpr, seen map[*
 	i := 0
 	var sets [][]ast.Expr
 	for j := 0; j <= len(exprs); j++ {
-		if j == len(exprs) || analysisutil.HasSideEffects(info, exprs[j]) {
+		if j == len(exprs) || !typesinternal.NoEffects(info, exprs[j]) {
 			if i < j {
 				sets = append(sets, exprs[i:j])
 			}

go/analysis/passes/internal/analysisutil/util.go

@@ -7,48 +7,13 @@
 package analysisutil
 
 import (
-	"go/ast"
 	"go/token"
-	"go/types"
 	"os"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/internal/analysisinternal"
 )
 
-// HasSideEffects reports whether evaluation of e has side effects.
-func HasSideEffects(info *types.Info, e ast.Expr) bool {
-	safe := true
-	ast.Inspect(e, func(node ast.Node) bool {
-		switch n := node.(type) {
-		case *ast.CallExpr:
-			typVal := info.Types[n.Fun]
-			switch {
-			case typVal.IsType():
-				// Type conversion, which is safe.
-			case typVal.IsBuiltin():
-				// Builtin func, conservatively assumed to not
-				// be safe for now.
-				safe = false
-				return false
-			default:
-				// A non-builtin func or method call.
-				// Conservatively assume that all of them have
-				// side effects for now.
-				safe = false
-				return false
-			}
-		case *ast.UnaryExpr:
-			if n.Op == token.ARROW {
-				safe = false
-				return false
-			}
-		}
-		return true
-	})
-	return !safe
-}
-
 // ReadFile reads a file and adds it to the FileSet
 // so that we can report errors against it using lineStart.
 func ReadFile(pass *analysis.Pass, filename string) ([]byte, *token.File, error) {

go/analysis/passes/modernize/testdata/src/reflecttypefor/reflecttypefor.go

@@ -15,7 +15,7 @@ var (
 	_ = reflect.TypeOf((*error)(nil))     // want "reflect.TypeOf call can be simplified using TypeFor"
 	_ = reflect.TypeOf(io.Reader(nil))    // nope (likely a mistake)
 	_ = reflect.TypeOf((*io.Reader)(nil)) // want "reflect.TypeOf call can be simplified using TypeFor"
-	_ = reflect.TypeOf(*new(time.Time))   // nope (false negative of noEffects)
+	_ = reflect.TypeOf(*new(time.Time))   // want "reflect.TypeOf call can be simplified using TypeFor"
 	_ = reflect.TypeOf(time.Time{})       // want "reflect.TypeOf call can be simplified using TypeFor"
 	_ = reflect.TypeOf(time.Duration(0))  // want "reflect.TypeOf call can be simplified using TypeFor"
 )

go/analysis/passes/modernize/testdata/src/reflecttypefor/reflecttypefor.go.golden

@@ -15,7 +15,7 @@ var (
 	_ = reflect.TypeFor[*error]()        // want "reflect.TypeOf call can be simplified using TypeFor"
 	_ = reflect.TypeOf(io.Reader(nil))   // nope (likely a mistake)
 	_ = reflect.TypeFor[*io.Reader]()    // want "reflect.TypeOf call can be simplified using TypeFor"
-	_ = reflect.TypeOf(*new(time.Time))  // nope (false negative of noEffects)
+	_ = reflect.TypeFor[time.Time]()     // want "reflect.TypeOf call can be simplified using TypeFor"
 	_ = reflect.TypeFor[time.Time]()     // want "reflect.TypeOf call can be simplified using TypeFor"
 	_ = reflect.TypeFor[time.Duration]() // want "reflect.TypeOf call can be simplified using TypeFor"
 )

internal/refactor/inline/calleefx.go

@@ -10,6 +10,8 @@ import (
 	"go/ast"
 	"go/token"
 	"go/types"
+
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 const (
@@ -180,7 +182,7 @@ func calleefx(info *types.Info, body *ast.BlockStmt, paramInfos map[*types.Var]*
 				// The pure built-ins have no effects beyond
 				// those of their operands (not even memory reads).
 				// All other calls have unknown effects.
-				if !callsPureBuiltin(info, n) {
+				if !typesinternal.CallsPureBuiltin(info, n) {
 					unknown() // arbitrary effects
 				}
 			}

internal/refactor/inline/inline.go

@@ -2539,7 +2539,7 @@ func (st *state) effects(info *types.Info, expr ast.Expr) bool {
 		case *ast.CallExpr:
 			if info.Types[n.Fun].IsType() {
 				// A conversion T(x) has only the effect of its operand.
-			} else if !callsPureBuiltin(info, n) {
+			} else if !typesinternal.CallsPureBuiltin(info, n) {
 				// A handful of built-ins have no effect
 				// beyond those of their arguments.
 				// All other calls (including append, copy, recover)
@@ -2635,7 +2635,7 @@ func pure(info *types.Info, assign1 func(*types.Var) bool, e ast.Expr) bool {
 			}
 
 			// Calls to some built-ins are as pure as their arguments.
-			if callsPureBuiltin(info, e) {
+			if typesinternal.CallsPureBuiltin(info, e) {
 				for _, arg := range e.Args {
 					if !pure(arg) {
 						return false
@@ -2712,24 +2712,6 @@ func pure(info *types.Info, assign1 func(*types.Var) bool, e ast.Expr) bool {
 	return pure(e)
 }
 
-// callsPureBuiltin reports whether call is a call of a built-in
-// function that is a pure computation over its operands (analogous to
-// a + operator). Because it does not depend on program state, it may
-// be evaluated at any point--though not necessarily at multiple
-// points (consider new, make).
-func callsPureBuiltin(info *types.Info, call *ast.CallExpr) bool {
-	if id, ok := ast.Unparen(call.Fun).(*ast.Ident); ok {
-		if b, ok := info.ObjectOf(id).(*types.Builtin); ok {
-			switch b.Name() {
-			case "len", "cap", "complex", "imag", "real", "make", "new", "max", "min":
-				return true
-			}
-			// Not: append clear close copy delete panic print println recover
-		}
-	}
-	return false
-}
-
 // duplicable reports whether it is appropriate for the expression to
 // be freely duplicated.
 //

internal/typesinternal/fx.go

@@ -19,25 +19,46 @@ func NoEffects(info *types.Info, expr ast.Expr) 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
+			*ast.StarExpr, *ast.CompositeLit,
+			// non-expressions that may appear within expressions
+			*ast.KeyValueExpr,
+			*ast.FieldList,
+			*ast.Field,
+			*ast.Ellipsis,
+			*ast.IndexListExpr:
+			// No effect.
+
+		case *ast.ArrayType,
+			*ast.StructType,
+			*ast.ChanType,
+			*ast.FuncType,
+			*ast.MapType,
+			*ast.InterfaceType:
+			// Type syntax: no effects, recursively.
+			// Prune descent.
+			return false
+
 		case *ast.UnaryExpr:
-			// Channel send <-ch has effects
+			// Channel send <-ch has effects.
 			if v.Op == token.ARROW {
 				noEffects = false
 			}
+
 		case *ast.CallExpr:
-			// Type conversion has no effects
+			// 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
+				if CallsPureBuiltin(info, v) {
+					// A call such as len(e) has no effects of its
+					// own, though the subexpression e might.
+				} else {
+					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
@@ -47,3 +68,21 @@ func NoEffects(info *types.Info, expr ast.Expr) bool {
 	})
 	return noEffects
 }
+
+// CallsPureBuiltin reports whether call is a call of a built-in
+// function that is a pure computation over its operands (analogous to
+// a + operator). Because it does not depend on program state, it may
+// be evaluated at any point--though not necessarily at multiple
+// points (consider new, make).
+func CallsPureBuiltin(info *types.Info, call *ast.CallExpr) bool {
+	if id, ok := ast.Unparen(call.Fun).(*ast.Ident); ok {
+		if b, ok := info.ObjectOf(id).(*types.Builtin); ok {
+			switch b.Name() {
+			case "len", "cap", "complex", "imag", "real", "make", "new", "max", "min":
+				return true
+			}
+			// Not: append clear close copy delete panic print println recover
+		}
+	}
+	return false
+}

go/analysis/passes/internal/analysisutil/util_test.go renamed to internal/typesinternal/fx_test.go

@@ -1,8 +1,8 @@
-// Copyright 2021 The Go Authors. All rights reserved.
+// Copyright 2025 The Go Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package analysisutil_test
+package typesinternal_test
 
 import (
 	"go/ast"
@@ -11,10 +11,10 @@ import (
 	"go/types"
 	"testing"
 
-	"golang.org/x/tools/go/analysis/passes/internal/analysisutil"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
-func TestHasSideEffects(t *testing.T) {
+func TestNoEffects(t *testing.T) {
 	src := `package p
 
 type T int
@@ -45,8 +45,8 @@ func _() {
 		if !ok {
 			return true
 		}
-		if got := analysisutil.HasSideEffects(info, call); got != false {
-			t.Errorf("HasSideEffects(%s) = true, want false", types.ExprString(call))
+		if !typesinternal.NoEffects(info, call) {
+			t.Errorf("NoEffects(%s) = false", types.ExprString(call))
 		}
 		return true
 	})