internal/refactor/inline: sound treatment of named results

Previously the condition for eliminating named result vars
was that they do not escape, but that's not strong enough:
it should be that they are never referenced. If they are
referenced, they can be accommodated in a binding decl,
as done in this change.

Also, tests for the interaction of named results with
each strategy.

Change-Id: I2a4bdeec17c98efd488f9939aebe6ab1d72a0814
Reviewed-on: https://go-review.googlesource.com/c/tools/+/530438
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

Author: adonovan

internal/refactor/inline/callee.go

@@ -42,6 +42,7 @@ type gobCallee struct {
 	Params           []*paramInfo // information about parameters (incl. receiver)
 	Results          []*paramInfo // information about result variables
 	HasDefer         bool         // uses defer
+	HasBareReturn    bool         // uses bare return in non-void function
 	TotalReturns     int          // number of return statements
 	TrivialReturns   int          // number of return statements with trivial result conversions
 	Labels           []string     // names of all control labels
@@ -246,6 +247,7 @@ func AnalyzeCallee(logf func(string, ...any), fset *token.FileSet, pkg *types.Pa
 	// (but not any nested functions).
 	var (
 		hasDefer       = false
+		hasBareReturn  = false
 		totalReturns   = 0
 		trivialReturns = 0
 		labels         []string
@@ -281,6 +283,8 @@ func AnalyzeCallee(logf func(string, ...any), fset *token.FileSet, pkg *types.Pa
 						break
 					}
 				}
+			} else if sig.Results().Len() > 0 {
+				hasBareReturn = true
 			}
 			if trivial {
 				trivialReturns++
@@ -329,6 +333,7 @@ func AnalyzeCallee(logf func(string, ...any), fset *token.FileSet, pkg *types.Pa
 		Params:           params,
 		Results:          results,
 		HasDefer:         hasDefer,
+		HasBareReturn:    hasBareReturn,
 		TotalReturns:     totalReturns,
 		TrivialReturns:   trivialReturns,
 		Labels:           labels,

internal/refactor/inline/inline.go

@@ -508,7 +508,7 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 	updateCalleeParams(calleeDecl, params)
 
 	// Create a var (param = arg; ...) decl for use by some strategies.
-	bindingDeclStmt := createBindingDecl(logf, caller, args, calleeDecl)
+	bindingDeclStmt := createBindingDecl(logf, caller, args, calleeDecl, callee.Results)
 
 	var remainingArgs []ast.Expr
 	for _, arg := range args {
@@ -577,14 +577,12 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 		return res, nil
 	}
 
-	// Attempt to reduce parameterless calls
-	// whose result variables do not escape.
-	allParamsSubstituted := forall(params, func(i int, p *parameter) bool {
-		return p == nil
-	})
-	noResultEscapes := !exists(callee.Results, func(i int, r *paramInfo) bool {
-		return r.Escapes
-	})
+	// If all parameters have been substituted and no result
+	// variable is referenced, we don't need a binding decl.
+	// This may enable better reduction strategies.
+	allResultsUnreferenced := forall(callee.Results, func(i int, r *paramInfo) bool { return len(r.Refs) == 0 })
+	needBindingDecl := !allResultsUnreferenced ||
+		exists(params, func(i int, p *parameter) bool { return p != nil })
 
 	// Special case: call to { return exprs }.
 	//
@@ -595,9 +593,8 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 	//
 	// If:
 	// - the body is just "return expr" with trivial implicit conversions,
-	// - all parameters can be eliminated
-	//   (by substitution, or a binding decl),
-	// - no result var escapes,
+	// - all parameters and result vars can be eliminated
+	//   or replaced by a binding decl,
 	// then the call expression can be replaced by the
 	// callee's body expression, suitably substituted.
 	if len(calleeDecl.Body.List) == 1 &&
@@ -610,14 +607,14 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 
 		// statement context
 		if stmt, ok := context.(*ast.ExprStmt); ok &&
-			(allParamsSubstituted && noResultEscapes || bindingDeclStmt != nil) {
+			(!needBindingDecl || bindingDeclStmt != nil) {
 			logf("strategy: reduce stmt-context call to { return exprs }")
 			clearPositions(calleeDecl.Body)
 
 			if callee.ValidForCallStmt {
 				logf("callee body is valid as statement")
 				// Inv: len(results) == 1
-				if allParamsSubstituted && noResultEscapes {
+				if !needBindingDecl {
 					// Reduces to: expr
 					res.old = caller.Call
 					res.new = results[0]
@@ -643,7 +640,7 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 					Rhs: results,
 				}
 				res.old = stmt
-				if allParamsSubstituted && noResultEscapes {
+				if !needBindingDecl {
 					// Reduces to: _, _ = exprs
 					res.new = discard
 				} else {
@@ -660,7 +657,7 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 		}
 
 		// expression context
-		if allParamsSubstituted && noResultEscapes {
+		if !needBindingDecl {
 			clearPositions(calleeDecl.Body)
 
 			if callee.NumResults == 1 {
@@ -725,12 +722,12 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 	//         { var (bindings); body }
 	//         { body }
 	// so long as:
-	// - all parameters can be eliminated
-	//   (by substitution, or a binding decl),
+	// - all parameters can be eliminated or replaced by a binding decl,
 	// - call is a tail-call;
 	// - all returns in body have trivial result conversions;
 	// - there is no label conflict;
-	// - no result variable is referenced by name.
+	// - no result variable is referenced by name,
+	//   or implicitly by a bare return.
 	//
 	// The body may use defer, arbitrary control flow, and
 	// multiple returns.
@@ -744,16 +741,14 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 	if ret, ok := callContext(caller.path).(*ast.ReturnStmt); ok &&
 		len(ret.Results) == 1 &&
 		callee.TrivialReturns == callee.TotalReturns &&
-		(allParamsSubstituted && noResultEscapes || bindingDeclStmt != nil) &&
+		!callee.HasBareReturn &&
+		(!needBindingDecl || bindingDeclStmt != nil) &&
 		!hasLabelConflict(caller.path, callee.Labels) &&
-		forall(callee.Results, func(i int, p *paramInfo) bool {
-			// all result vars are unreferenced
-			return len(p.Refs) == 0
-		}) {
+		allResultsUnreferenced {
 		logf("strategy: reduce tail-call")
 		body := calleeDecl.Body
 		clearPositions(body)
-		if !(allParamsSubstituted && noResultEscapes) {
+		if needBindingDecl {
 			body.List = prepend(bindingDeclStmt, body.List...)
 		}
 		res.old = ret
@@ -774,20 +769,20 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 	// - callee is a void function (no returns)
 	// - callee does not use defer
 	// - there is no label conflict between caller and callee
-	// - all parameters can be eliminated
-	//   (by substitution, or a binding decl),
+	// - all parameters and result vars can be eliminated
+	//   or replaced by a binding decl,
 	//
 	// If there is only a single statement, the braces are omitted.
 	if stmt := callStmt(caller.path); stmt != nil &&
-		(allParamsSubstituted && noResultEscapes || bindingDeclStmt != nil) &&
+		(!needBindingDecl || bindingDeclStmt != nil) &&
 		!callee.HasDefer &&
 		!hasLabelConflict(caller.path, callee.Labels) &&
 		callee.TotalReturns == 0 {
 		logf("strategy: reduce stmt-context call to { stmts }")
 		body := calleeDecl.Body
 		var repl ast.Stmt = body
 		clearPositions(repl)
-		if !(allParamsSubstituted && noResultEscapes) {
+		if needBindingDecl {
 			body.List = prepend(bindingDeclStmt, body.List...)
 		}
 		if len(body.List) == 1 {
@@ -1175,11 +1170,13 @@ func updateCalleeParams(calleeDecl *ast.FuncDecl, params []*parameter) {
 }
 
 // createBindingDecl constructs a "binding decl" that implements
-// parameter assignment.
+// parameter assignment and declares any named result variables
+// referenced by the callee.
 //
-// If we succeed, the declaration may be used by reduction
-// strategies to relax the requirement that all parameters
-// have been substituted.
+// It may not always be possible to create the decl (e.g. due to
+// shadowing), in which case it returns nil; but if it succeeds, the
+// declaration may be used by reduction strategies to relax the
+// requirement that all parameters have been substituted.
 //
 // For example, a call:
 //
@@ -1210,7 +1207,7 @@ func updateCalleeParams(calleeDecl *ast.FuncDecl, params []*parameter) {
 //
 // Strategies may impose additional checks on return
 // conversions, labels, defer, etc.
-func createBindingDecl(logf func(string, ...any), caller *Caller, args []*argument, calleeDecl *ast.FuncDecl) ast.Stmt {
+func createBindingDecl(logf func(string, ...any), caller *Caller, args []*argument, calleeDecl *ast.FuncDecl, results []*paramInfo) ast.Stmt {
 	// Spread calls are tricky as they may not align with the
 	// parameters' field groupings nor types.
 	// For example, given
@@ -1228,27 +1225,15 @@ func createBindingDecl(logf func(string, ...any), caller *Caller, args []*argume
 		return nil
 	}
 
-	// Compute remaining argument expressions.
-	var values []ast.Expr
-	for _, arg := range args {
-		if arg != nil {
-			values = append(values, arg.expr)
-		}
-	}
-
 	var (
 		specs    []ast.Spec
 		shadowed = make(map[string]bool) // names defined by previous specs
 	)
-	for _, field := range calleeDecl.Type.Params.List {
-		// Each field (param group) becomes a ValueSpec.
-		spec := &ast.ValueSpec{
-			Names:  field.Names,
-			Type:   field.Type,
-			Values: values[:len(field.Names)],
-		}
-		values = values[len(field.Names):]
-
+	// shadow reports whether any name referenced by spec is
+	// shadowed by a name declared by a previous spec (since,
+	// unlike parameters, each spec of a var decl is within the
+	// scope of the previous specs).
+	shadow := func(spec *ast.ValueSpec) bool {
 		// Compute union of free names of type and values
 		// and detect shadowing. Values is the arguments
 		// (caller syntax), so we can use type info.
@@ -1260,22 +1245,78 @@ func createBindingDecl(logf func(string, ...any), caller *Caller, args []*argume
 				free[name] = true
 			}
 		}
-		freeishNames(free, field.Type)
+		freeishNames(free, spec.Type)
 		for name := range free {
 			if shadowed[name] {
 				logf("binding decl would shadow free name %q", name)
-				return nil
+				return true
 			}
 		}
 		for _, id := range spec.Names {
 			if id.Name != "_" {
 				shadowed[id.Name] = true
 			}
 		}
+		return false
+	}
 
+	// parameters
+	//
+	// Bind parameters that were not eliminated through
+	// substitution. (Non-nil arguments correspond to the
+	// remaining parameters in calleeDecl.)
+	var values []ast.Expr
+	for _, arg := range args {
+		if arg != nil {
+			values = append(values, arg.expr)
+		}
+	}
+	for _, field := range calleeDecl.Type.Params.List {
+		// Each field (param group) becomes a ValueSpec.
+		spec := &ast.ValueSpec{
+			Names:  field.Names,
+			Type:   field.Type,
+			Values: values[:len(field.Names)],
+		}
+		values = values[len(field.Names):]
+		if shadow(spec) {
+			return nil
+		}
 		specs = append(specs, spec)
 	}
 	assert(len(values) == 0, "args/params mismatch")
+
+	// results
+	//
+	// Add specs to declare any named result
+	// variables that are referenced by the body.
+	if calleeDecl.Type.Results != nil {
+		resultIdx := 0
+		for _, field := range calleeDecl.Type.Results.List {
+			if field.Names == nil {
+				resultIdx++
+				continue // unnamed field
+			}
+			var names []*ast.Ident
+			for _, id := range field.Names {
+				if len(results[resultIdx].Refs) > 0 {
+					names = append(names, id)
+				}
+				resultIdx++
+			}
+			if len(names) > 0 {
+				spec := &ast.ValueSpec{
+					Names: names,
+					Type:  field.Type,
+				}
+				if shadow(spec) {
+					return nil
+				}
+				specs = append(specs, spec)
+			}
+		}
+	}
+
 	decl := &ast.DeclStmt{
 		Decl: &ast.GenDecl{
 			Tok:   token.VAR,

internal/refactor/inline/inline_test.go

@@ -526,6 +526,82 @@ func TestTable(t *testing.T) {
 	}
 }`,
 		},
+		// Treatment of named result vars across strategies:
+		{
+			"Stmt-context call to {return g()} that mentions named result.",
+			`func f() (x int) { return g(x) }; func g(int) int`,
+			`func _() { f() }`,
+			`func _() {
+	{
+		var x int
+		g(x)
+	}
+}`,
+		},
+		{
+			"Ditto, with binding decl (due to repeated y refs).",
+			`func f(y string) (x string) { return x+y+y }`,
+			`func _() { f("") }`,
+			`func _() {
+	{
+		var (
+			y string = ""
+			x string
+		)
+		_ = x + y + y
+	}
+}`,
+		},
+		{
+			"Stmt-context call to {return binary} that mentions named result.",
+			`func f() (x int) { return x+x }`,
+			`func _() { f() }`,
+			`func _() {
+	{
+		var x int
+		_ = x + x
+	}
+}`,
+		},
+		{
+			"Ditto, with binding decl again.",
+			`func f(y string) (x int) { return x+x+len(y+y) }`,
+			`func _() { f("") }`,
+			`func _() {
+	{
+		var (
+			y string = ""
+			x int
+		)
+		_ = x + x + len(y+y)
+	}
+}`,
+		},
+		{
+			"Tail call to {return expr} that mentions named result.",
+			`func f() (x int) { return x }`,
+			`func _() int { return f() }`,
+			`func _() int { return func() (x int) { return x }() }`,
+		},
+		{
+			"Tail call to {return} that implicitly reads named result.",
+			`func f() (x int) { return }`,
+			`func _() int { return f() }`,
+			`func _() int { return func() (x int) { return }() }`,
+		},
+		{
+			"Spread-context call to {return expr} that mentions named result.",
+			`func f() (x, y int) { return x, y }`,
+			`func _() { var _, _ = f() }`,
+			`func _() { var _, _ = func() (x, y int) { return x, y }() }`,
+		},
+		{
+			"Shadowing in binding decl for named results => literalization.",
+			`func f(y string) (x y) { return x+x+len(y+y) }; type y = int`,
+			`func _() { f("") }`,
+			`func _() { func(y string) (x y) { return x + x + len(y+y) }("") }`,
+		},
+
 		// TODO(adonovan): improve coverage of the cross
 		// product of each strategy with the checklist of
 		// concerns enumerated in the package doc comment.