internal/refactor/inline: allow duplicable type conversions

The current implementation does not consider type conversions to be
duplicable operations; this change resolves it by handling *ast.CallExpr.
All type conversions except []byte(string) and []rune(string) are now
considered to be duplicable.

The change also removes redundant type conversions when typed constants
are passed to inlined functions.

Fixes golang/go#67589

Change-Id: I524dbff1ae09466f56459e0bf3b6425be38d157c
GitHub-Last-Rev: 580a00a
GitHub-Pull-Request: #494
Reviewed-on: https://go-review.googlesource.com/c/tools/+/588175
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: vladvalkov

internal/refactor/inline/inline.go

@@ -1437,10 +1437,29 @@ next:
 			// other arguments are given explicit types in either
 			// a binding decl or when using the literalization
 			// strategy.
-			if len(param.info.Refs) > 0 && !trivialConversion(args[i].constant, args[i].typ, params[i].obj.Type()) {
-				arg.expr = convert(params[i].fieldType, arg.expr)
+
+			// 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, args[i].typ, params[i].obj.Type())
+					param.info.Name, arg.typ, param.obj.Type())
 			}
 
 			// It is safe to substitute param and replace it with arg.
@@ -2206,16 +2225,35 @@ func duplicable(info *types.Info, e ast.Expr) bool {
 		return false
 
 	case *ast.CallExpr:
-		// Don't treat a conversion T(x) as duplicable even
-		// if x is duplicable because it could duplicate
-		// allocations.
+		// Treat type conversions as duplicable if they do not observably allocate.
+		// The only cases of observable allocations are
+		// the `[]byte(string)` and `[]rune(string)` conversions.
 		//
-		// TODO(adonovan): there are cases to tease apart here:
-		// duplicating string([]byte) conversions increases
+		// Duplicating string([]byte) conversions increases
 		// allocation but doesn't change behavior, but the
 		// reverse, []byte(string), allocates a distinct array,
-		// which is observable
-		return false
+		// which is observable.
+
+		if !info.Types[e.Fun].IsType() { // check whether e.Fun is a type conversion
+			return false
+		}
+
+		fun := info.TypeOf(e.Fun)
+		arg := info.TypeOf(e.Args[0])
+
+		switch fun := fun.Underlying().(type) {
+		case *types.Slice:
+			// Do not mark []byte(string) and []rune(string) as duplicable.
+			elem, ok := fun.Elem().Underlying().(*types.Basic)
+			if ok && (elem.Kind() == types.Rune || elem.Kind() == types.Byte) {
+				from, ok := arg.Underlying().(*types.Basic)
+				isString := ok && from.Info()&types.IsString != 0
+				return !isString
+			}
+		case *types.TypeParam:
+			return false // be conservative
+		}
+		return true
 
 	case *ast.SelectorExpr:
 		if seln, ok := info.Selections[e]; ok {

internal/refactor/inline/inline_test.go

@@ -414,52 +414,171 @@ func TestBasics(t *testing.T) {
 }
 
 func TestDuplicable(t *testing.T) {
-	runTests(t, []testcase{
-		{
-			"Empty strings are duplicable.",
-			`func f(s string) { print(s, s) }`,
-			`func _() { f("")  }`,
-			`func _() { print("", "") }`,
-		},
-		{
-			"Non-empty string literals are not duplicable.",
-			`func f(s string) { print(s, s) }`,
-			`func _() { f("hi")  }`,
-			`func _() {
+	t.Run("basic", func(t *testing.T) {
+		runTests(t, []testcase{
+			{
+				"Empty strings are duplicable.",
+				`func f(s string) { print(s, s) }`,
+				`func _() { f("")  }`,
+				`func _() { print("", "") }`,
+			},
+			{
+				"Non-empty string literals are not duplicable.",
+				`func f(s string) { print(s, s) }`,
+				`func _() { f("hi")  }`,
+				`func _() {
 	var s string = "hi"
 	print(s, s)
 }`,
-		},
-		{
-			"Empty array literals are duplicable.",
-			`func f(a [2]int) { print(a, a) }`,
-			`func _() { f([2]int{})  }`,
-			`func _() { print([2]int{}, [2]int{}) }`,
-		},
-		{
-			"Non-empty array literals are not duplicable.",
-			`func f(a [2]int) { print(a, a) }`,
-			`func _() { f([2]int{1, 2})  }`,
-			`func _() {
+			},
+			{
+				"Empty array literals are duplicable.",
+				`func f(a [2]int) { print(a, a) }`,
+				`func _() { f([2]int{})  }`,
+				`func _() { print([2]int{}, [2]int{}) }`,
+			},
+			{
+				"Non-empty array literals are not duplicable.",
+				`func f(a [2]int) { print(a, a) }`,
+				`func _() { f([2]int{1, 2})  }`,
+				`func _() {
 	var a [2]int = [2]int{1, 2}
 	print(a, a)
 }`,
-		},
-		{
-			"Empty struct literals are duplicable.",
-			`func f(s S) { print(s, s) }; type S struct { x int }`,
-			`func _() { f(S{})  }`,
-			`func _() { print(S{}, S{}) }`,
-		},
-		{
-			"Non-empty struct literals are not duplicable.",
-			`func f(s S) { print(s, s) }; type S struct { x int }`,
-			`func _() { f(S{x: 1})  }`,
-			`func _() {
+			},
+			{
+				"Empty struct literals are duplicable.",
+				`func f(s S) { print(s, s) }; type S struct { x int }`,
+				`func _() { f(S{})  }`,
+				`func _() { print(S{}, S{}) }`,
+			},
+			{
+				"Non-empty struct literals are not duplicable.",
+				`func f(s S) { print(s, s) }; type S struct { x int }`,
+				`func _() { f(S{x: 1})  }`,
+				`func _() {
 	var s S = S{x: 1}
 	print(s, s)
 }`,
-		},
+			},
+		})
+	})
+
+	t.Run("conversions", func(t *testing.T) {
+		runTests(t, []testcase{
+			{
+				"Conversions to integer are duplicable.",
+				`func f(i int) { print(i, i) }`,
+				`func _() { var i int8 = 1; f(int(i))  }`,
+				`func _() { var i int8 = 1; print(int(i), int(i)) }`,
+			},
+			{
+				"Implicit conversions from underlying types are duplicable.",
+				`func f(i I) { print(i, i) }; type I int`,
+				`func _() { f(1)  }`,
+				`func _() { print(I(1), I(1)) }`,
+			},
+			{
+				"Conversions to array are duplicable.",
+				`func f(a [2]int) { print(a, a) }; type A [2]int`,
+				`func _() { var a A; f([2]int(a)) }`,
+				`func _() { var a A; print([2]int(a), [2]int(a)) }`,
+			},
+			{
+				"Conversions from array are duplicable.",
+				`func f(a A) { print(a, a) }; type A [2]int`,
+				`func _() { var a [2]int; f(A(a)) }`,
+				`func _() { var a [2]int; print(A(a), A(a)) }`,
+			},
+			{
+				"Conversions from byte slice to string are duplicable.",
+				`func f(s string) { print(s, s) }`,
+				`func _() { var b []byte; f(string(b)) }`,
+				`func _() { var b []byte; print(string(b), string(b)) }`,
+			},
+			{
+				"Conversions from string to byte slice are not duplicable.",
+				`func f(b []byte) { print(b, b) }`,
+				`func _() { var s string; f([]byte(s)) }`,
+				`func _() {
+	var s string
+	var b []byte = []byte(s)
+	print(b, b)
+}`,
+			},
+			{
+				"Conversions from string to uint8 slice are not duplicable.",
+				`func f(b []uint8) { print(b, b) }`,
+				`func _() { var s string; f([]uint8(s)) }`,
+				`func _() {
+	var s string
+	var b []uint8 = []uint8(s)
+	print(b, b)
+}`,
+			},
+			{
+				"Conversions from string to rune slice are not duplicable.",
+				`func f(r []rune) { print(r, r) }`,
+				`func _() { var s string; f([]rune(s)) }`,
+				`func _() {
+	var s string
+	var r []rune = []rune(s)
+	print(r, r)
+}`,
+			},
+			{
+				"Conversions from string to named type with underlying byte slice are not duplicable.",
+				`func f(b B) { print(b, b) }; type B []byte`,
+				`func _() { var s string; f(B(s)) }`,
+				`func _() {
+	var s string
+	var b B = B(s)
+	print(b, b)
+}`,
+			},
+			{
+				"Conversions from string to named type of string are duplicable.",
+				`func f(s S) { print(s, s) }; type S string`,
+				`func _() { var s string; f(S(s)) }`,
+				`func _() { var s string; print(S(s), S(s)) }`,
+			},
+			{
+				"Built-in function calls are not duplicable.",
+				`func f(i int) { print(i, i) }`,
+				`func _() { f(len(""))  }`,
+				`func _() {
+	var i int = len("")
+	print(i, i)
+}`,
+			},
+			{
+				"Built-in function calls are not duplicable.",
+				`func f(c complex128) { print(c, c) }`,
+				`func _() { f(complex(1.0, 2.0)) }`,
+				`func _() {
+	var c complex128 = complex(1.0, 2.0)
+	print(c, c)
+}`,
+			},
+			{
+				"Non built-in function calls are not duplicable.",
+				`func f(i int) { print(i, i) }
+//go:noinline
+func f1(i int) int { return i + 1 }`,
+				`func _() { f(f1(1))  }`,
+				`func _() {
+	var i int = f1(1)
+	print(i, i)
+}`,
+			},
+			{
+				"Conversions between function types are duplicable.",
+				`func f(f F) { print(f, f) }; type F func(); func f1() {}`,
+				`func _() { f(F(f1))  }`,
+				`func _() { print(F(f1), F(f1)) }`,
+			},
+		})
+
 	})
 }
 
@@ -1358,6 +1477,23 @@ func TestSubstitutionPreservesParameterType(t *testing.T) {
 	})
 }
 
+func TestRedundantConversions(t *testing.T) {
+	runTests(t, []testcase{
+		{
+			"Type conversion must be added if the constant is untyped.",
+			`func f(i int32) { print(i) }`,
+			`func _() { f(1)  }`,
+			`func _() { print(int32(1)) }`,
+		},
+		{
+			"Type conversion must not be added if the constant is typed.",
+			`func f(i int32) { print(i) }`,
+			`func _() { f(int32(1))  }`,
+			`func _() { print(int32(1)) }`,
+		},
+	})
+}
+
 func runTests(t *testing.T, tests []testcase) {
 	for _, test := range tests {
 		test := test

internal/refactor/inline/util.go

@@ -68,14 +68,19 @@ func within(pos token.Pos, n ast.Node) bool {
 //
 // trivialConversion under-approximates trivial conversions, as unfortunately
 // go/types does not record the type of an expression *before* it is implicitly
-// converted, and therefore it cannot distinguish typed constant constant
+// converted, and therefore it cannot distinguish typed constant
 // expressions from untyped constant expressions. For example, in the
 // expression `c + 2`, where c is a uint32 constant, trivialConversion does not
-// detect that the default type of this express is actually uint32, not untyped
+// detect that the default type of this expression is actually uint32, not untyped
 // int.
 //
 // We could, of course, do better here by reverse engineering some of go/types'
-// constant handling. That may or may not be worthwhile..
+// constant handling. That may or may not be worthwhile.
+//
+// Example: in func f() int32 { return 0 },
+// the type recorded for 0 is int32, not untyped int;
+// although it is Identical to the result var,
+// the conversion is non-trivial.
 func trivialConversion(fromValue constant.Value, from, to types.Type) bool {
 	if fromValue != nil {
 		var defaultType types.Type