internal/refactor/inline: reify implicit return conversions

This change fixes a bug that caused func() error { return nil }()
to be reduced to nil in some cases, because the expr-context
reduction strategy was abusing safeReturn, which is only
appropriate for tailcalls. (That function has been renamed
for clarity.)

The safeReturn condition in the expr-context strategy has
been pushed down to the leaves, allowing it to be relaxed
in some cases (e.g. simple tail calls) by materializing
conversions.

Also, tests.

There is more work to do to cleanly factor the reification
of implicit return conversions across strategies, and to
do it only when actually necessary.

Change-Id: I775554a0a3d1348f8dbd9930904edd819f7c3839
Reviewed-on: https://go-review.googlesource.com/c/tools/+/535796
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

internal/refactor/inline/inline.go

@@ -760,6 +760,16 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 	needBindingDecl := !allResultsUnreferenced ||
 		exists(params, func(i int, p *parameter) bool { return p != nil })
 
+	// The two strategies below overlap for a tail call of {return exprs}:
+	// The expr-context reduction is nice because it keeps the
+	// caller's return stmt and merely switches its operand,
+	// without introducing a new block, but it doesn't work with
+	// implicit return conversions.
+	//
+	// TODO(adonovan): unify these cases more cleanly, allowing return-
+	// operand replacement and implicit conversions, by adding
+	// conversions around each return operand (if not a spread return).
+
 	// Special case: call to { return exprs }.
 	//
 	// Reduces to:
@@ -776,8 +786,7 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 	// callee's body expression, suitably substituted.
 	if len(calleeDecl.Body.List) == 1 &&
 		is[*ast.ReturnStmt](calleeDecl.Body.List[0]) &&
-		len(calleeDecl.Body.List[0].(*ast.ReturnStmt).Results) > 0 && // not a bare return
-		safeReturn(caller, calleeSymbol, callee) {
+		len(calleeDecl.Body.List[0].(*ast.ReturnStmt).Results) > 0 { // not a bare return
 		results := calleeDecl.Body.List[0].(*ast.ReturnStmt).Results
 
 		context := callContext(caller.path)
@@ -839,11 +848,24 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 
 			if callee.NumResults == 1 {
 				logf("strategy: reduce expr-context call to { return expr }")
+				// (includes some simple tail-calls)
+
+				// Make implicit return conversion explicit.
+				if callee.TrivialReturns < callee.TotalReturns {
+					results[0] = convert(calleeDecl.Type.Results.List[0].Type, results[0])
+				}
 
 				res.old = caller.Call
 				res.new = results[0]
-			} else {
+				return res, nil
+
+			} else if callee.TrivialReturns == callee.TotalReturns {
 				logf("strategy: reduce spread-context call to { return expr }")
+				// There is no general way to reify conversions in a spread
+				// return, hence the requirement above.
+				//
+				// TODO(adonovan): allow this reduction when no
+				// conversion is required by the context.
 
 				// The call returns multiple results but is
 				// not a standalone call statement. It must
@@ -880,8 +902,8 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 				default:
 					return nil, fmt.Errorf("internal error: unexpected context %T for spread call", context)
 				}
+				return res, nil
 			}
-			return res, nil
 		}
 	}
 
@@ -911,7 +933,7 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 	// or implicit) return.
 	if ret, ok := callContext(caller.path).(*ast.ReturnStmt); ok &&
 		len(ret.Results) == 1 &&
-		safeReturn(caller, calleeSymbol, callee) &&
+		tailCallSafeReturn(caller, calleeSymbol, callee) &&
 		!callee.HasBareReturn &&
 		(!needBindingDecl || bindingDeclStmt != nil) &&
 		!hasLabelConflict(caller.path, callee.Labels) &&
@@ -2624,9 +2646,9 @@ func declares(stmts []ast.Stmt) map[string]bool {
 	return names
 }
 
-// safeReturn reports whether the callee's return statements may be safely
+// tailCallSafeReturn reports whether the callee's return statements may be safely
 // used to return from the function enclosing the caller (which must exist).
-func safeReturn(caller *Caller, calleeSymbol *types.Func, callee *gobCallee) bool {
+func tailCallSafeReturn(caller *Caller, calleeSymbol *types.Func, callee *gobCallee) bool {
 	// It is safe if all callee returns involve only trivial conversions.
 	if callee.TrivialReturns == callee.TotalReturns {
 		return true

internal/refactor/inline/inline_test.go

@@ -397,6 +397,16 @@ func TestBasics(t *testing.T) {
 	var _ = 1
 	var _ = 2
 	var _ = 3
+}`,
+		},
+		{
+			// (a regression test for a missing conversion)
+			"Implicit return conversions are inserted in expr-context reduction.",
+			`func f(x int) error { return nil }`,
+			`func _() { if err := f(0); err != nil {} }`,
+			`func _() {
+	if err := error(nil); err != nil {
+	}
 }`,
 		},
 	})
@@ -673,7 +683,7 @@ func TestTailCallStrategy(t *testing.T) {
 			"Tail call with non-trivial return conversion (caller.sig != callee.sig).",
 			`func f() error { return E{} }; type E struct{error}`,
 			`func _() any { return f() }`,
-			`func _() any { return func() error { return E{} }() }`,
+			`func _() any { return error(E{}) }`,
 		},
 	})
 }
@@ -714,6 +724,12 @@ func TestSpreadCalls(t *testing.T) {
 			`func _() (int, error) { return f() }`,
 			`func _() (int, error) { return 0, nil }`,
 		},
+		{
+			"Implicit return conversions defeat reduction of spread returns, for now.",
+			`func f(x int) (_, _ error) { return nil, nil }`,
+			`func _() { _, _ = f(0) }`,
+			`func _() { _, _ = func() (_, _ error) { return nil, nil }() }`,
+		},
 	})
 }