internal/refactor/inline: improve check for last uses of free vars

This showed up when I used the inliner for removing parameters: we
should be more precise about detecting the last use of a local var.

A TODO is left to make this perfect, which will be done in a subsequent
CL.

For golang/go#63534

Change-Id: Id5c753f3e7ae51e07db1d29a59e82e51c6d5952c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/535335
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

internal/refactor/inline/inline.go

@@ -1258,11 +1258,11 @@ next:
 			// remove the last reference to a caller local var.
 			if caller.enclosingFunc != nil {
 				for free := range arg.freevars {
-					if v, ok := caller.lookup(free).(*types.Var); ok && within(v.Pos(), caller.enclosingFunc.Body) {
-						// TODO(adonovan): be more precise and check that v
-						// is indeed referenced only by call arguments.
-						// Better: proceed, but blank out its declaration as needed.
-						logf("keeping param %q: arg contains perhaps the last reference to possible caller local %v @ %v",
+					// TODO(rfindley): we can get this 100% right by looking for
+					// references among other arguments which have non-zero references
+					// within the callee.
+					if v, ok := caller.lookup(free).(*types.Var); ok && within(v.Pos(), caller.enclosingFunc.Body) && !isUsedOutsideCall(caller, v) {
+						logf("keeping param %q: arg contains perhaps the last reference to caller local %v @ %v",
 							param.info.Name, v, caller.Fset.PositionFor(v.Pos(), false))
 						continue next
 					}
@@ -1332,6 +1332,34 @@ next:
 	}
 }
 
+// isUsedOutsideCall reports whether v is used outside of caller.Call, within
+// the body of caller.enclosingFunc.
+func isUsedOutsideCall(caller *Caller, v *types.Var) bool {
+	used := false
+	ast.Inspect(caller.enclosingFunc.Body, func(n ast.Node) bool {
+		if n == caller.Call {
+			return false
+		}
+		switch n := n.(type) {
+		case *ast.Ident:
+			if use := caller.Info.Uses[n]; use == v {
+				used = true
+			}
+		case *ast.FuncType:
+			// All params are used.
+			for _, fld := range n.Params.List {
+				for _, n := range fld.Names {
+					if def := caller.Info.Defs[n]; def == v {
+						used = true
+					}
+				}
+			}
+		}
+		return !used // keep going until we find a use
+	})
+	return used
+}
+
 // checkFalconConstraints checks whether constant arguments
 // are safe to substitute (e.g. s[i] -> ""[0] is not safe.)
 //

internal/refactor/inline/inline_test.go

@@ -594,6 +594,36 @@ func TestSubstitution(t *testing.T) {
 			`func _() { var local int; f(local) }`,
 			`func _() { var local int; _ = local }`,
 		},
+		{
+			"Arguments that are used are detected",
+			`func f(int) {}`,
+			`func _() { var local int; _ = local; f(local) }`,
+			`func _() { var local int; _ = local }`,
+		},
+		{
+			"Arguments that are used are detected",
+			`func f(x, y int) { print(x) }`,
+			`func _() { var z int; f(z, z) }`,
+			`func _() {
+	var z int
+	var _ int = z
+	print(z)
+}`,
+		},
+		{
+			"Function parameters are always used",
+			`func f(int) {}`,
+			`func _() {
+	func(local int) {
+		f(local)
+	}(1)
+}`,
+			`func _() {
+	func(local int) {
+
+	}(1)
+}`,
+		},
 		{
 			"Regression test for detection of shadowing in nested functions.",
 			`func f(x int) { _ = func() { y := 1; print(y); print(x) } }`,