internal/refactor/inline: avoid unnecessary desugaring in selectors

When substituting into the receiver of a selector expression, we do not
need to desugar the receiver argument, because the selector will still
be valid. Add call site analysis to detect this case and undo the
desugaring.

Fixes golang/go#69442

Change-Id: I66c5e37b7b12e003f4e4f2e5c41eff909507097f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/630855
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

gopls/internal/test/marker/testdata/codeaction/removeparam_method.txt

@@ -128,7 +128,7 @@ func (Missing) M(a, c, r0 int) (r1 int) { //@codeaction("b", "refactor.rewrite.r
 
 func _() {
 	m := &Missing{}
-	_ = (*m).M(1, 3, 4)
+	_ = m.M(1, 3, 4)
 }
 -- @missingrecv/missingrecvuse/p.go --
 package missingrecvuse

internal/refactor/inline/callee.go

@@ -395,6 +395,11 @@ type paramInfo struct {
 type refInfo struct {
 	Offset   int  // FuncDecl-relative byte offset of parameter ref within body
 	NeedType bool // type of substituted arg must be identical to type of param
+	// IsSelectionOperand indicates whether the parameter reference is the
+	// operand of a selection (param.f). If so, and param's argument is itself
+	// a receiver parameter (a common case), we don't need to desugar (&v or *ptr)
+	// the selection: if param.Method is a valid selection, then so is param.fieldOrMethod.
+	IsSelectionOperand bool
 }
 
 // analyzeParams computes information about parameters of function fn,
@@ -476,8 +481,9 @@ func analyzeParams(logf func(string, ...any), fset *token.FileSet, info *types.I
 					// the substituted expression is an untyped constant or unnamed type.
 					ifaceParam := isNonTypeParamInterface(v.Type())
 					ref := refInfo{
-						Offset:   int(n.Pos() - decl.Pos()),
-						NeedType: !ifaceParam || !isAssignableOperand(info, stack),
+						Offset:             int(n.Pos() - decl.Pos()),
+						NeedType:           !ifaceParam || !isAssignableOperand(info, stack),
+						IsSelectionOperand: isSelectionOperand(stack),
 					}
 					pinfo.Refs = append(pinfo.Refs, ref)
 					pinfo.Shadow = addShadows(pinfo.Shadow, info, pinfo.Name, stack)
@@ -510,21 +516,7 @@ func analyzeParams(logf func(string, ...any), fset *token.FileSet, info *types.I
 // then passing a different (yet assignable) type may affect type inference,
 // and so isAssignableOperand reports false.
 func isAssignableOperand(info *types.Info, stack []ast.Node) bool {
-	var (
-		parent  ast.Node                         // the parent node surrounding expr
-		expr, _ = stack[len(stack)-1].(ast.Expr) // the relevant expr, after stripping parentheses
-	)
-	if expr == nil {
-		return false
-	}
-	for i := len(stack) - 2; i >= 0; i-- {
-		if pexpr, ok := stack[i].(*ast.ParenExpr); ok {
-			expr = pexpr
-		} else {
-			parent = stack[i]
-			break
-		}
-	}
+	parent, expr := exprContext(stack)
 	if parent == nil {
 		return false
 	}
@@ -600,6 +592,40 @@ func isAssignableOperand(info *types.Info, stack []ast.Node) bool {
 	return false // In all other cases, preserve the parameter type.
 }
 
+// exprContext returns the innermost parent->child expression nodes for the
+// given outer-to-inner stack, after stripping parentheses.
+//
+// If no such context exists, returns (nil, nil).
+func exprContext(stack []ast.Node) (parent ast.Node, expr ast.Expr) {
+	expr, _ = stack[len(stack)-1].(ast.Expr)
+	if expr == nil {
+		return nil, nil
+	}
+	for i := len(stack) - 2; i >= 0; i-- {
+		if pexpr, ok := stack[i].(*ast.ParenExpr); ok {
+			expr = pexpr
+		} else {
+			parent = stack[i]
+			break
+		}
+	}
+	if parent == nil {
+		return nil, nil
+	}
+	return parent, expr
+}
+
+// isSelectionOperand reports whether the innermost node of stack is operand
+// (x) of a selection x.f.
+func isSelectionOperand(stack []ast.Node) bool {
+	parent, expr := exprContext(stack)
+	if parent == nil {
+		return false
+	}
+	sel, ok := parent.(*ast.SelectorExpr)
+	return ok && sel.X == expr
+}
+
 // addShadows returns the shadows set augmented by the set of names
 // locally shadowed at the location of the reference in the callee
 // (identified by the stack). The name of the reference itself is

internal/refactor/inline/inline.go

@@ -224,7 +224,7 @@ func (st *state) inline() (*Result, error) {
 		f, err = parser.ParseFile(fset, "callee.go", content, mode)
 		if err != nil {
 			// Something has gone very wrong.
-			logf("failed to reparse <<%s>>", string(content)) // debugging
+			logf("failed to reparse <<%s>>: %v", string(content), err) // debugging
 			return err
 		}
 		return nil
@@ -1395,6 +1395,7 @@ type argument struct {
 	freevars      map[string]bool // free names of expr
 	substitutable bool            // is candidate for substitution
 	variadic      bool            // is explicit []T{...} for eliminated variadic
+	desugaredRecv bool            // is *recv or &recv, where operator was elided
 }
 
 // arguments returns the effective arguments of the call.
@@ -1488,12 +1489,14 @@ func (st *state) arguments(caller *Caller, calleeDecl *ast.FuncDecl, assign1 fun
 				// &recv
 				arg.expr = &ast.UnaryExpr{Op: token.AND, X: arg.expr}
 				arg.typ = types.NewPointer(arg.typ)
+				arg.desugaredRecv = true
 			} else if argIsPtr && !paramIsPtr {
 				// *recv
 				arg.expr = &ast.StarExpr{X: arg.expr}
 				arg.typ = typeparams.Deref(arg.typ)
 				arg.duplicable = false
 				arg.pure = false
+				arg.desugaredRecv = true
 			}
 		}
 	}
@@ -1691,6 +1694,23 @@ next:
 			logf("replacing parameter %q by argument %q",
 				param.info.Name, debugFormatNode(caller.Fset, arg.expr))
 			for _, ref := range param.info.Refs {
+				// Apply any transformations necessary for this reference.
+				argExpr := arg.expr
+
+				// If the reference itself is being selected, and we applied desugaring
+				// (an explicit &x or *x), we can undo that desugaring here as it is
+				// not necessary for a selector. We don't need to check addressability
+				// here because if we desugared, the receiver must have been
+				// addressable.
+				if ref.IsSelectionOperand && arg.desugaredRecv {
+					switch e := argExpr.(type) {
+					case *ast.UnaryExpr:
+						argExpr = e.X
+					case *ast.StarExpr:
+						argExpr = e.X
+					}
+				}
+
 				// If the reference requires exact type agreement (as reported by
 				// param.info.NeedType), wrap the argument in an explicit conversion
 				// if substitution might materially change its type. (We already did
@@ -1699,7 +1719,7 @@ next:
 				// This is only needed for substituted arguments. All other arguments
 				// are given explicit types in either a binding decl or when using the
 				// literalization strategy.
-
+				//
 				// If the types are identical, we can eliminate
 				// redundant type conversions such as this:
 				//
@@ -1716,7 +1736,6 @@ next:
 				// its (possibly "untyped") inherent type, so
 				// the conversion from untyped 1 to int32 is non-trivial even
 				// though both arg and param have identical types (int32).
-				argExpr := arg.expr
 				if ref.NeedType &&
 					!types.Identical(arg.typ, param.obj.Type()) &&
 					!trivialConversion(arg.constant, arg.typ, param.obj.Type()) {

internal/refactor/inline/inline_test.go

@@ -1487,10 +1487,22 @@ func TestSubstitutionPreservesParameterType(t *testing.T) {
 			`func _() { T(1).g() }`,
 		},
 		{
-			"Implicit dereference is made explicit", // TODO(rfindley): fix unnecessary literalization.
-			`type T struct{ field int }; func (x T) f(y T) bool { return x == y && x.field == x.g() }; func (x T) g() int { return 0 }`,
-			`func _() { var t *T; var y T; _ = t.f(y) }`,
-			`func _() { var t *T; var y T; _ = func() bool { var x T = *t; return x == y && x.field == x.g() }() }`,
+			"Implicit reference is made explicit outside of selector",
+			`type T int; func (x *T) f() bool { return x == x.id() }; func (x *T) id() *T { return x }`,
+			`func _() { var t T; _ = t.f() }`,
+			`func _() { var t T; _ = &t == t.id() }`,
+		},
+		{
+			"Implicit parenthesized reference is not made explicit in selector",
+			`type T int; func (x *T) f() bool { return x == (x).id() }; func (x *T) id() *T { return x }`,
+			`func _() { var t T; _ = t.f() }`,
+			`func _() { var t T; _ = &t == (t).id() }`,
+		},
+		{
+			"Implicit dereference is made explicit outside of selector", // TODO(rfindley): avoid unnecessary literalization here
+			`type T int; func (x T) f() bool { return x == x.id() }; func (x T) id() T { return x }`,
+			`func _() { var t *T; _ = t.f() }`,
+			`func _() { var t *T; _ = func() bool { var x T = *t; return x == x.id() }() }`,
 		},
 		{
 			"Check for shadowing error on type used in the conversion.",

internal/refactor/inline/testdata/issue69442.txtar

@@ -0,0 +1,34 @@
+This test checks that we don't introduce unnecessary (&v) or (*ptr) operations
+when calling a method on an addressable receiver.
+
+-- go.mod --
+module testdata
+
+go 1.20
+
+-- main.go --
+package foo
+type T int
+
+func (*T) F() {}
+
+func (t *T) G() { t.F() }
+
+func main() {
+	var t T
+	t.G() //@ inline(re"G", inline)
+}
+
+-- inline --
+package foo
+
+type T int
+
+func (*T) F() {}
+
+func (t *T) G() { t.F() }
+
+func main() {
+	var t T
+	t.F() //@ inline(re"G", inline)
+}