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: 580a00aa8b36ec44fd63e9fb44140c4b62cf7219 GitHub-Pull-Request: golang/tools#494 Reviewed-on: https://go-review.googlesource.com/c/tools/+/588175 Reviewed-by: Robert Findley <rfindley@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/internal/refactor/inline/inline.go b/internal/refactor/inline/inline.go index 6f20d2c..4cf14e8 100644 --- a/internal/refactor/inline/inline.go +++ b/internal/refactor/inline/inline.go
@@ -1437,10 +1437,29 @@ // 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 @@ 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 {
diff --git a/internal/refactor/inline/inline_test.go b/internal/refactor/inline/inline_test.go index 96d322b..a64c806 100644 --- a/internal/refactor/inline/inline_test.go +++ b/internal/refactor/inline/inline_test.go
@@ -414,52 +414,171 @@ } 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 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
diff --git a/internal/refactor/inline/util.go b/internal/refactor/inline/util.go index 3dc46ad..0ed33ba 100644 --- a/internal/refactor/inline/util.go +++ b/internal/refactor/inline/util.go
@@ -68,14 +68,19 @@ // // 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