internal/refactor/inline: more precise SelectorExpr effects

The treatment of MethodVal and FieldVal selections should
mostly be consistent, as both apply the same implicit
field selection operations to the explicit receiver.
(Only the final implicit &v or *ptr operations differ.)
However, Selection.Indirect has a bug that masked this
similarity.

This change handles SelectorExpr more carefully throughout.
The indirectSelection method helper provides a non-buggy
implementation of Selection.Indirect.

The net result is that, in the callee effects analysis,
selection operations that don't indirect a pointer are pure,
and those that do indirect a pointer act like a read,
allowing them to commute with other reads.

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

internal/refactor/inline/calleefx.go

@@ -116,10 +116,23 @@ func calleefx(info *types.Info, body *ast.BlockStmt, paramInfos map[*types.Var]*
 			visitExpr(n.X)
 
 		case *ast.SelectorExpr:
-			if sel, ok := info.Selections[n]; ok {
+			if seln, ok := info.Selections[n]; ok {
 				visitExpr(n.X)
-				if sel.Indirect() {
-					effect(rinf) // indirect read x.f of heap variable
+
+				// See types.SelectionKind for background.
+				switch seln.Kind() {
+				case types.MethodExpr:
+					// A method expression T.f acts like a
+					// reference to a func decl,
+					// so it doesn't read x until called.
+
+				case types.MethodVal, types.FieldVal:
+					// A field or method value selection x.f
+					// reads x if the selection indirects a pointer.
+
+					if indirectSelection(seln) {
+						effect(rinf)
+					}
 				}
 			} else {
 				// qualified identifier: treat like unqualified

internal/refactor/inline/escape.go

@@ -72,9 +72,11 @@ func escape(info *types.Info, root ast.Node, f func(v *types.Var, escapes bool))
 			if sel, ok := n.Fun.(*ast.SelectorExpr); ok {
 				if seln, ok := info.Selections[sel]; ok &&
 					seln.Kind() == types.MethodVal &&
-					!seln.Indirect() &&
-					is[*types.Pointer](seln.Obj().Type().(*types.Signature).Recv().Type()) {
-					lvalue(sel.X, true) // &x.f
+					isPointer(seln.Obj().Type().(*types.Signature).Recv().Type()) {
+					tArg, indirect := effectiveReceiver(seln)
+					if !indirect && !isPointer(tArg) {
+						lvalue(sel.X, true) // &x.f
+					}
 				}
 			}
 

internal/refactor/inline/inline.go

@@ -1093,7 +1093,7 @@ func arguments(caller *Caller, calleeDecl *ast.FuncDecl, assign1 func(*types.Var
 						debugFormatNode(caller.Fset, caller.Call.Fun),
 						fld.Name())
 				}
-				if is[*types.Pointer](arg.typ.Underlying()) {
+				if isPointer(arg.typ) {
 					arg.pure = false // implicit *ptr operation => impure
 				}
 				arg.expr = &ast.SelectorExpr{
@@ -1105,8 +1105,8 @@ func arguments(caller *Caller, calleeDecl *ast.FuncDecl, assign1 func(*types.Var
 			}
 
 			// Make * or & explicit.
-			argIsPtr := arg.typ != deref(arg.typ)
-			paramIsPtr := is[*types.Pointer](seln.Obj().Type().(*types.Signature).Recv().Type())
+			argIsPtr := isPointer(arg.typ)
+			paramIsPtr := isPointer(seln.Obj().Type().(*types.Signature).Recv().Type())
 			if !argIsPtr && paramIsPtr {
 				// &recv
 				arg.expr = &ast.UnaryExpr{Op: token.AND, X: arg.expr}
@@ -1910,27 +1910,23 @@ func pure(info *types.Info, assign1 func(*types.Var) bool, e ast.Expr) bool {
 			return true
 
 		case *ast.SelectorExpr:
-			if sel, ok := info.Selections[e]; ok {
-				switch sel.Kind() {
+			if seln, ok := info.Selections[e]; ok {
+
+				// See types.SelectionKind for background.
+				switch seln.Kind() {
 				case types.MethodExpr:
 					// A method expression T.f acts like a
 					// reference to a func decl, so it is pure.
 					return true
 
-				case types.MethodVal:
-					// A method value x.f acts like a
-					// closure around a T.f(x, ...) call,
-					// so it is as pure as x.
-					return pure(e.X)
-
-				case types.FieldVal:
-					// A field selection x.f is pure if
-					// x is pure and the selection does
+				case types.MethodVal, types.FieldVal:
+					// A field or method selection x.f is pure
+					// if x is pure and the selection does
 					// not indirect a pointer.
-					return !sel.Indirect() && pure(e.X)
+					return !indirectSelection(seln) && pure(e.X)
 
 				default:
-					panic(sel)
+					panic(seln)
 				}
 			} else {
 				// A qualified identifier is
@@ -1978,7 +1974,7 @@ func callsPureBuiltin(info *types.Info, call *ast.CallExpr) bool {
 // integer literals, unary negation, and selectors x.f where x is not
 // a pointer. But we would not wish to duplicate expressions that:
 // - have side effects (e.g. nearly all calls),
-// - are not referentially transparent (e.g. &T{}, ptr.field), or
+// - are not referentially transparent (e.g. &T{}, ptr.field, *ptr), or
 // - are long (e.g. "huge string literal").
 func duplicable(info *types.Info, e ast.Expr) bool {
 	switch e := e.(type) {
@@ -2009,10 +2005,10 @@ func duplicable(info *types.Info, e ast.Expr) bool {
 		return false
 
 	case *ast.SelectorExpr:
-		if sel, ok := info.Selections[e]; ok {
+		if seln, ok := info.Selections[e]; ok {
 			// A field or method selection x.f is referentially
 			// transparent if it does not indirect a pointer.
-			return !sel.Indirect()
+			return !indirectSelection(seln)
 		}
 		// A qualified identifier pkg.Name is referentially transparent.
 		return true

internal/refactor/inline/inline_test.go

@@ -721,6 +721,21 @@ func TestParameterBindingDecl(t *testing.T) {
 			`func _() { f(g(1), g(2), g(3)) }`,
 			`func _() { func(int, y any, z int) { defer g(0); println(int, y, z) }(g(1), g(2), g(3)) }`,
 		},
+		{
+			"An indirect method selection (*x).g acts as a read.",
+			`func f(x *T, y any) any { return x.g(y) }; type T struct{}; func (T) g(x any) any { return x }`,
+			`func _(x *T) { f(x, recover()) }`,
+			`func _(x *T) {
+	var y any = recover()
+	x.g(y)
+}`,
+		},
+		{
+			"A direct method selection x.g is pure.",
+			`func f(x *T, y any) any { return x.g(y) }; type T struct{}; func (*T) g(x any) any { return x }`,
+			`func _(x *T) { f(x, recover()) }`,
+			`func _(x *T) { x.g(recover()) }`,
+		},
 	})
 }
 

internal/refactor/inline/util.go

@@ -12,6 +12,8 @@ import (
 	"go/types"
 	"reflect"
 	"strings"
+
+	"golang.org/x/tools/internal/typeparams"
 )
 
 func is[T any](x any) bool {
@@ -117,3 +119,42 @@ func convert(T, x ast.Expr) *ast.CallExpr {
 		Args: []ast.Expr{x},
 	}
 }
+
+// isPointer reports whether t is a pointer type.
+func isPointer(t types.Type) bool { return t != deref(t) }
+
+// indirectSelection is like seln.Indirect() without bug #8353.
+func indirectSelection(seln *types.Selection) bool {
+	// Work around bug #8353 in Selection.Indirect when Kind=MethodVal.
+	if seln.Kind() == types.MethodVal {
+		tArg, indirect := effectiveReceiver(seln)
+		if indirect {
+			return true
+		}
+
+		tParam := seln.Obj().Type().(*types.Signature).Recv().Type()
+		return isPointer(tArg) && !isPointer(tParam) // implicit *
+	}
+
+	return seln.Indirect()
+}
+
+// effectiveReceiver returns the effective type of the method
+// receiver after all implicit field selections (but not implicit * or
+// & operations) have been applied.
+//
+// The boolean indicates whether any implicit field selection was indirect.
+func effectiveReceiver(seln *types.Selection) (types.Type, bool) {
+	assert(seln.Kind() == types.MethodVal, "not MethodVal")
+	t := seln.Recv()
+	indices := seln.Index()
+	indirect := false
+	for _, index := range indices[:len(indices)-1] {
+		if tElem := deref(t); tElem != t {
+			indirect = true
+			t = tElem
+		}
+		t = typeparams.CoreType(t).(*types.Struct).Field(index).Type()
+	}
+	return t, indirect
+}