internal/refactor/inline: avoid unnecessary interface conversions

Avoid unnecessary interface conversions when inlining by detecting when
references are in an interface->interface assignment context.

Fixes golang/go#68554

Change-Id: I4d789d7bd1f29a16523d4feacfbe89a85f4bed0e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/629875
Auto-Submit: Robert Findley <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

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

@@ -0,0 +1,38 @@
+This test checks that inlining removes unnecessary interface conversions.
+
+-- main.go --
+package main
+
+import (
+	"fmt"
+	"io"
+)
+
+func f(d discard) {
+	g(d) //@codeaction("g", "refactor.inline.call", result=out)
+}
+
+func g(w io.Writer) { fmt.Println(w) }
+
+var d discard
+type discard struct{}
+func (discard) Write(p []byte) (int, error) { return len(p), nil }
+-- @out/main.go --
+package main
+
+import (
+	"fmt"
+	"io"
+)
+
+func f(d discard) {
+	fmt.Println(d) //@codeaction("g", "refactor.inline.call", result=out)
+}
+
+func g(w io.Writer) { fmt.Println(w) }
+
+var d discard
+
+type discard struct{}
+
+func (discard) Write(p []byte) (int, error) { return len(p), nil }

internal/refactor/inline/callee.go

@@ -387,11 +387,16 @@ type paramInfo struct {
 	IsResult   bool            // false for receiver or parameter, true for result variable
 	Assigned   bool            // parameter appears on left side of an assignment statement
 	Escapes    bool            // parameter has its address taken
-	Refs       []int           // FuncDecl-relative byte offset of parameter ref within body
+	Refs       []refInfo       // information about references to parameter within body
 	Shadow     map[string]bool // names shadowed at one of the above refs
 	FalconType string          // name of this parameter's type (if basic) in the falcon system
 }
 
+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
+}
+
 // analyzeParams computes information about parameters of function fn,
 // including a simple "address taken" escape analysis.
 //
@@ -458,10 +463,23 @@ func analyzeParams(logf func(string, ...any), fset *token.FileSet, info *types.I
 		if id, ok := n.(*ast.Ident); ok {
 			if v, ok := info.Uses[id].(*types.Var); ok {
 				if pinfo, ok := paramInfos[v]; ok {
-					// Record location of ref to parameter/result
-					// and any intervening (shadowing) names.
-					offset := int(n.Pos() - decl.Pos())
-					pinfo.Refs = append(pinfo.Refs, offset)
+					// Record ref information, and any intervening (shadowing) names.
+					//
+					// If the parameter v has an interface type, and the reference id
+					// appears in a context where assignability rules apply, there may be
+					// an implicit interface-to-interface widening. In that case it is
+					// not necessary to insert an explicit conversion from the argument
+					// to the parameter's type.
+					//
+					// Contrapositively, if param is not an interface type, then the
+					// assignment may lose type information, for example in the case that
+					// 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),
+					}
+					pinfo.Refs = append(pinfo.Refs, ref)
 					pinfo.Shadow = addShadows(pinfo.Shadow, info, pinfo.Name, stack)
 				}
 			}
@@ -481,6 +499,107 @@ func analyzeParams(logf func(string, ...any), fset *token.FileSet, info *types.I
 
 // -- callee helpers --
 
+// isAssignableOperand reports whether the given outer-to-inner stack has an
+// innermost expression operand for which assignability rules apply, meaning it
+// is used in a position where its type need only be assignable to the type
+// implied by its surrounding syntax.
+//
+// As this function is intended to be used for inlining analysis, it reports
+// false in one case where the operand technically need only be assignable: if
+// the type being assigned to is a call argument and contains type parameters,
+// 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
+		}
+	}
+	if parent == nil {
+		return false
+	}
+
+	// Types do not need to match for assignment to a variable.
+	if assign, ok := parent.(*ast.AssignStmt); ok && assign.Tok == token.ASSIGN {
+		for _, v := range assign.Rhs {
+			if v == expr {
+				return true
+			}
+		}
+	}
+
+	// Types do not need to match for an initializer with known type.
+	if spec, ok := parent.(*ast.ValueSpec); ok && spec.Type != nil {
+		for _, v := range spec.Values {
+			if v == expr {
+				return true
+			}
+		}
+	}
+
+	// Types do not need to match for composite literal keys, values, or
+	// fields.
+	if kv, ok := parent.(*ast.KeyValueExpr); ok {
+		if kv.Key == expr || kv.Value == expr {
+			return true
+		}
+	}
+	if lit, ok := parent.(*ast.CompositeLit); ok {
+		for _, v := range lit.Elts {
+			if v == expr {
+				return true
+			}
+		}
+	}
+
+	// Types do not need to match for values sent to a channel.
+	if send, ok := parent.(*ast.SendStmt); ok {
+		if send.Value == expr {
+			return true
+		}
+	}
+
+	// Types do not need to match for an argument to a call, unless the
+	// corresponding parameter has type parameters, as in that case the
+	// argument type may affect inference.
+	if call, ok := parent.(*ast.CallExpr); ok {
+		for i, arg := range call.Args {
+			if arg == expr {
+				if fn, _ := typeutil.Callee(info, call).(*types.Func); fn != nil { // Incidentally, this check rejects a conversion T(id).
+					sig := fn.Type().(*types.Signature)
+
+					// Find the relevant parameter type, accounting for variadics.
+					var paramType types.Type
+					if plen := sig.Params().Len(); sig.Variadic() && i >= plen-1 && !call.Ellipsis.IsValid() {
+						if s, ok := sig.Params().At(plen - 1).Type().(*types.Slice); ok {
+							paramType = s.Elem()
+						}
+					} else if i < plen {
+						paramType = sig.Params().At(i).Type()
+					}
+
+					if paramType != nil && !new(typeparams.Free).Has(paramType) {
+						return true
+					}
+				}
+				break
+			}
+		}
+	}
+
+	return false // In all other cases, preserve the parameter type.
+}
+
 // 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

@@ -1604,40 +1604,6 @@ next:
 	for i, param := range params {
 		if arg := args[i]; arg.substitutable {
 
-			// Wrap the argument in an explicit conversion if
-			// substitution might materially change its type.
-			// (We already did the necessary shadowing check
-			// on the parameter type syntax.)
-			//
-			// 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:
-			//
-			// Callee:
-			//    func f(i int32) { print(i) }
-			// Caller:
-			//    func g() { f(int32(1)) }
-			// Inlined as:
-			//    func g() { print(int32(int32(1)))
-			//
-			// Recall that non-trivial does not imply non-identical
-			// for constant conversions; however, at this point state.arguments
-			// has already re-typechecked the constant and set arg.type to
-			// 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).
-			if len(param.info.Refs) > 0 &&
-				!types.Identical(arg.typ, param.obj.Type()) &&
-				!trivialConversion(arg.constant, arg.typ, param.obj.Type()) {
-				arg.expr = convert(param.fieldType, arg.expr)
-				logf("param %q: adding explicit %s -> %s conversion around argument",
-					param.info.Name, arg.typ, param.obj.Type())
-			}
-
 			// It is safe to substitute param and replace it with arg.
 			// The formatter introduces parens as needed for precedence.
 			//
@@ -1646,14 +1612,72 @@ next:
 			logf("replacing parameter %q by argument %q",
 				param.info.Name, debugFormatNode(caller.Fset, arg.expr))
 			for _, ref := range param.info.Refs {
-				replace(ref, internalastutil.CloneNode(arg.expr).(ast.Expr), arg.variadic)
+				// 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
+				// the necessary shadowing check on the parameter type syntax.)
+				//
+				// 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:
+				//
+				// Callee:
+				//    func f(i int32) { print(i) }
+				// Caller:
+				//    func g() { f(int32(1)) }
+				// Inlined as:
+				//    func g() { print(int32(int32(1)))
+				//
+				// Recall that non-trivial does not imply non-identical
+				// for constant conversions; however, at this point state.arguments
+				// has already re-typechecked the constant and set arg.type to
+				// 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()) {
+
+					// If arg.expr is already an interface call, strip it.
+					if call, ok := argExpr.(*ast.CallExpr); ok && len(call.Args) == 1 {
+						if typ, ok := isConversion(caller.Info, call); ok && isNonTypeParamInterface(typ) {
+							argExpr = call.Args[0]
+						}
+					}
+
+					argExpr = convert(param.fieldType, argExpr)
+					logf("param %q (offset %d): adding explicit %s -> %s conversion around argument",
+						param.info.Name, ref.Offset, arg.typ, param.obj.Type())
+				}
+				replace(ref.Offset, internalastutil.CloneNode(argExpr).(ast.Expr), arg.variadic)
 			}
 			params[i] = nil // substituted
 			args[i] = nil   // substituted
 		}
 	}
 }
 
+// isConversion reports whether the given call is a type conversion, returning
+// (operand, true) if so.
+//
+// If the call is not a conversion, it returns (nil, false).
+func isConversion(info *types.Info, call *ast.CallExpr) (types.Type, bool) {
+	if tv, ok := info.Types[call.Fun]; ok && tv.IsType() {
+		return tv.Type, true
+	}
+	return nil, false
+}
+
+// isNonTypeParamInterface reports whether t is a non-type parameter interface
+// type.
+func isNonTypeParamInterface(t types.Type) bool {
+	return !typeparams.IsTypeParam(t) && types.IsInterface(t)
+}
+
 // isUsedOutsideCall reports whether v is used outside of caller.Call, within
 // the body of caller.enclosingFunc.
 func isUsedOutsideCall(caller *Caller, v *types.Var) bool {

internal/refactor/inline/inline_test.go

@@ -1509,6 +1509,88 @@ func TestRedundantConversions(t *testing.T) {
 			`func _() { f(int32(1))  }`,
 			`func _() { print(int32(1)) }`,
 		},
+		{
+			"No type conversion for argument to interface parameter",
+			`type T int; func f(x any) { g(x) }; func g(any) {}`,
+			`func _() { f(T(1)) }`,
+			`func _() { g(T(1)) }`,
+		},
+		{
+			"No type conversion for parenthesized argument to interface parameter",
+			`type T int; func f(x any) { g((x)) }; func g(any) {}`,
+			`func _() { f(T(1)) }`,
+			`func _() { g((T(1))) }`,
+		},
+		{
+			"Type conversion for argument to type parameter",
+			`type T int; func f(x any) { g(x) }; func g[P any](P) {}`,
+			`func _() { f(T(1)) }`,
+			`func _() { g(any(T(1))) }`,
+		},
+		{
+			"Strip redundant interface conversions",
+			`type T interface{ M() }; func f(x any) { g(x) }; func g[P any](P) {}`,
+			`func _() { f(T(nil)) }`,
+			`func _() { g(any(nil)) }`,
+		},
+		{
+			"No type conversion for argument to variadic interface parameter",
+			`type T int; func f(x ...any) { g(x...) }; func g(...any) {}`,
+			`func _() { f(T(1)) }`,
+			`func _() { g(T(1)) }`,
+		},
+		{
+			"Type conversion for variadic argument",
+			`type T int; func f(x ...any) { g(x...) }; func g(...any) {}`,
+			`func _() { f([]any{T(1)}...) }`,
+			`func _() { g([]any{T(1)}...) }`,
+		},
+		{
+			"No type conversion for assignment to an explicit interface type",
+			`type T int; func f(x any) { var y any; y = x; _ = y }`,
+			`func _() { f(T(1)) }`,
+			`func _() {
+	var y any
+	y = T(1)
+	_ = y
+}`,
+		},
+		{
+			"No type conversion for initializer of an explicit interface type",
+			`type T int; func f(x any) { var y any = x; _ = y }`,
+			`func _() { f(T(1)) }`,
+			`func _() {
+	var y any = T(1)
+	_ = y
+}`,
+		},
+		{
+			"No type conversion for use as a composite literal key",
+			`type T int; func f(x any) { _ = map[any]any{x: 1} }`,
+			`func _() { f(T(1)) }`,
+			`func _() { _ = map[any]any{T(1): 1} }`,
+		},
+		{
+			"No type conversion for use as a composite literal value",
+			`type T int; func f(x any) { _ = []any{x} }`,
+			`func _() { f(T(1)) }`,
+			`func _() { _ = []any{T(1)} }`,
+		},
+		{
+			"No type conversion for use as a composite literal field",
+			`type T int; func f(x any) { _ = struct{ F any }{F: x} }`,
+			`func _() { f(T(1)) }`,
+			`func _() { _ = struct{ F any }{F: T(1)} }`,
+		},
+		{
+			"No type conversion for use in a send statement",
+			`type T int; func f(x any) { var c chan any; c <- x }`,
+			`func _() { f(T(1)) }`,
+			`func _() {
+	var c chan any
+	c <- T(1)
+}`,
+		},
 	})
 }