internal/refactor/inline: don't insert unnecessary parens

This change causes parens to be inserted only if needed
around the new node in the replacement. Parens are needed
if the child is a unary or binary operation and either
(a) the parent is a unary or binary of higher precedence or
(b) the child is the operand of a postfix operator.

Also, tests.

Updates golang/go#63259

Change-Id: I12ee95ad79b4921755d9bc87952f6404cb166e2b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/532098
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>

gopls/internal/regtest/marker/testdata/codeaction/inline.txt

@@ -17,7 +17,7 @@ func add(x, y int) int { return x + y }
 package a
 
 func _() {
-	println((1 + 2)) //@codeaction("refactor.inline", "add", ")", inline)
+	println(1 + 2) //@codeaction("refactor.inline", "add", ")", inline)
 }
 
 func add(x, y int) int { return x + y }

internal/refactor/inline/inline.go

@@ -73,6 +73,31 @@ func Inline(logf func(string, ...any), caller *Caller, callee *Callee) ([]byte,
 	assert(res.old != nil, "old is nil")
 	assert(res.new != nil, "new is nil")
 
+	// A single return operand inlined to a unary
+	// expression context may need parens. Otherwise:
+	//    func two() int { return 1+1 }
+	//    print(-two())  =>  print(-1+1) // oops!
+	//
+	// Usually it is not necessary to insert ParenExprs
+	// as the formatter is smart enough to insert them as
+	// needed by the context. But the res.{old,new}
+	// substitution is done by formatting res.new in isolation
+	// and then splicing its text over res.old, so the
+	// formatter doesn't see the parent node and cannot do
+	// the right thing. (One solution would be to always
+	// format the enclosing node of old, but that requires
+	// non-lossy comment handling, #20744.)
+	//
+	// So, we must analyze the call's context
+	// to see whether ambiguity is possible.
+	// For example, if the context is x[y:z], then
+	// the x subtree is subject to precedence ambiguity
+	// (replacing x by p+q would give p+q[y:z] which is wrong)
+	// but the y and z subtrees are safe.
+	if needsParens(caller.path, res.old, res.new) {
+		res.new = &ast.ParenExpr{X: res.new.(ast.Expr)}
+	}
+
 	// Don't call replaceNode(caller.File, res.old, res.new)
 	// as it mutates the caller's syntax tree.
 	// Instead, splice the file, replacing the extent of the "old"
@@ -727,29 +752,8 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 			if callee.NumResults == 1 {
 				logf("strategy: reduce expr-context call to { return expr }")
 
-				// A single return operand inlined to a unary
-				// expression context may need parens. Otherwise:
-				//    func two() int { return 1+1 }
-				//    print(-two())  =>  print(-1+1) // oops!
-				//
-				// Usually it is not necessary to insert ParenExprs
-				// as the formatter is smart enough to insert them as
-				// needed by the context. But the res.{old,new}
-				// substitution is done by formatting res.new in isolation
-				// and then splicing its text over res.old, so the
-				// formatter doesn't see the parent node and cannot do
-				// the right thing. (One solution would be to always
-				// format the enclosing node of old, but that requires
-				// non-lossy comment handling, #20744.)
-				//
-				// TODO(adonovan): do better by analyzing 'context'
-				// to see whether ambiguity is possible.
-				// For example, if the context is x[y:z], then
-				// the x subtree is subject to precedence ambiguity
-				// (replacing x by p+q would give p+q[y:z] which is wrong)
-				// but the y and z subtrees are safe.
 				res.old = caller.Call
-				res.new = &ast.ParenExpr{X: results[0]}
+				res.new = results[0]
 			} else {
 				logf("strategy: reduce spread-context call to { return expr }")
 
@@ -2279,3 +2283,76 @@ func consistentOffsets(caller *Caller) bool {
 	}
 	return is[*ast.CallExpr](expr)
 }
+
+// needsParens reports whether parens are required to avoid ambiguity
+// around the new node replacing the specified old node (which is some
+// ancestor of the CallExpr identified by its PathEnclosingInterval).
+func needsParens(callPath []ast.Node, old, new ast.Node) bool {
+	// Find enclosing old node and its parent.
+	// TODO(adonovan): Use index[ast.Node]() in go1.20.
+	i := -1
+	for i = range callPath {
+		if callPath[i] == old {
+			break
+		}
+	}
+	if i == -1 {
+		panic("not found")
+	}
+
+	// There is no precedence ambiguity when replacing
+	// (e.g.) a statement enclosing the call.
+	if !is[ast.Expr](old) {
+		return false
+	}
+
+	// An expression beneath a non-expression
+	// has no precedence ambiguity.
+	parent, ok := callPath[i+1].(ast.Expr)
+	if !ok {
+		return false
+	}
+
+	precedence := func(n ast.Node) int {
+		switch n := n.(type) {
+		case *ast.UnaryExpr, *ast.StarExpr:
+			return token.UnaryPrec
+		case *ast.BinaryExpr:
+			return n.Op.Precedence()
+		}
+		return -1
+	}
+
+	// Parens are not required if the new node
+	// is not unary or binary.
+	newprec := precedence(new)
+	if newprec < 0 {
+		return false
+	}
+
+	// Parens are required if parent and child are both
+	// unary or binary and the parent has higher precedence.
+	if precedence(parent) > newprec {
+		return true
+	}
+
+	// Was the old node the operand of a postfix operator?
+	//  f().sel
+	//  f()[i:j]
+	//  f()[i]
+	//  f().(T)
+	//  f()(x)
+	switch parent := parent.(type) {
+	case *ast.SelectorExpr:
+		return parent.X == old
+	case *ast.IndexExpr:
+		return parent.X == old
+	case *ast.SliceExpr:
+		return parent.X == old
+	case *ast.TypeAssertExpr:
+		return parent.X == old
+	case *ast.CallExpr:
+		return parent.Fun == old
+	}
+	return false
+}

internal/refactor/inline/inline_test.go

@@ -359,7 +359,7 @@ func TestBasics(t *testing.T) {
 			"Basic",
 			`func f(x int) int { return x }`,
 			`var _ = f(0)`,
-			`var _ = (0)`,
+			`var _ = 0`,
 		},
 		{
 			"Empty body, no arg effects.",
@@ -387,6 +387,51 @@ func TestBasics(t *testing.T) {
 	})
 }
 
+func TestPrecedenceParens(t *testing.T) {
+	// Ensure that parens are inserted when (and only when) necessary
+	// around the replacement for the call expression. (This is a special
+	// case in the way the inliner uses a combination of AST formatting
+	// for the call and text splicing for the rest of the file.)
+	runTests(t, []testcase{
+		{
+			"Multiplication in addition context (no parens).",
+			`func f(x, y int) int { return x * y }`,
+			`func _() { _ = 1 + f(2, 3) }`,
+			`func _() { _ = 1 + 2*3 }`,
+		},
+		{
+			"Addition in multiplication context (parens).",
+			`func f(x, y int) int { return x + y }`,
+			`func _() { _ = 1 * f(2, 3) }`,
+			`func _() { _ = 1 * (2 + 3) }`,
+		},
+		{
+			"Addition in negation context (parens).",
+			`func f(x, y int) int { return x + y }`,
+			`func _() { _ = -f(1, 2) }`,
+			`func _() { _ = -(1 + 2) }`,
+		},
+		{
+			"Addition in call context (no parens).",
+			`func f(x, y int) int { return x + y }`,
+			`func _() { println(f(1, 2)) }`,
+			`func _() { println(1 + 2) }`,
+		},
+		{
+			"Addition in slice operand context (parens).",
+			`func f(x, y string) string { return x + y }`,
+			`func _() { _ = f("x",  "y")[1:2] }`,
+			`func _() { _ = ("x" + "y")[1:2] }`,
+		},
+		{
+			"String literal in slice operand context (no parens).",
+			`func f(x string) string { return x }`,
+			`func _() { _ = f("xy")[1:2] }`,
+			`func _() { _ = "xy"[1:2] }`,
+		},
+	})
+}
+
 func TestSubstitution(t *testing.T) {
 	runTests(t, []testcase{
 		{
@@ -421,7 +466,7 @@ func TestTailCallStrategy(t *testing.T) {
 			"Tail call.",
 			`func f() int { return 1 }`,
 			`func _() int { return f() }`,
-			`func _() int { return (1) }`,
+			`func _() int { return 1 }`,
 		},
 		{
 			"Void tail call.",

internal/refactor/inline/testdata/basic-err.txtar

@@ -19,6 +19,6 @@ package a
 
 import "io"
 
-var _ = (io.EOF.Error()) //@ inline(re"getError", getError)
+var _ = io.EOF.Error() //@ inline(re"getError", getError)
 
 func getError(err error) string { return err.Error() }

internal/refactor/inline/testdata/basic-reduce.txtar

@@ -14,7 +14,7 @@ func zero() int { return 0 }
 -- zero --
 package a
 
-var _ = (0) //@ inline(re"zero", zero)
+var _ = 0 //@ inline(re"zero", zero)
 
 func zero() int { return 0 }
 
@@ -46,5 +46,5 @@ var _ = add(len(""), 2) //@ inline(re"add", add2)
 -- add2 --
 package a
 
-var _ = (len("") + 2) //@ inline(re"add", add2)
+var _ = len("") + 2 //@ inline(re"add", add2)
 

internal/refactor/inline/testdata/comments.txtar

@@ -51,7 +51,7 @@ func g() int { return 1 /*hello*/ + /*there*/ 1 }
 package a
 
 func _() {
-	println((1 + 1)) //@ inline(re"g", g)
+	println(1 + 1) //@ inline(re"g", g)
 }
 
 func g() int { return 1 /*hello*/ + /*there*/ 1 }

internal/refactor/inline/testdata/param-subst.txtar

@@ -14,6 +14,6 @@ func add(x, y int) int { return x + 2*y }
 -- add --
 package a
 
-var _ = (2 + 2*(1+1)) //@ inline(re"add", add)
+var _ = 2 + 2*(1+1) //@ inline(re"add", add)
 
 func add(x, y int) int { return x + 2*y }