gopls/internal/golang: refactor.inline.variable: add parens

Depending on the context into which we are inlining a variable's
initializer expression, parens may be needed to preserve operator
precedence.

We already have logic for this in internal/refactor/inline, so
it has been promoted to astutil.MaybeParenthesize.

+ test. (In fact one existing test covered the buggy behavior!)

Fixes golang/go#77381

Change-Id: I0d3d4445d444d8e048e1fdf8ff5ff16a3a2755ee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/740640
Reviewed-by: Madeline Kalil <mkalil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>

Author: adonovan

gopls/internal/golang/inline.go

@@ -221,10 +221,11 @@ func inlineVariableOne(pkg *cache.Package, pgf *parsego.File, start, end token.P
 	if !ok {
 		return nil, nil, fmt.Errorf("cannot inline variable here")
 	}
-	use := curUse.Node().(*ast.Ident)
 
 	// Check that free symbols of rhs are unshadowed at curUse.
 	var (
+		use   = curUse.Node().(*ast.Ident)
+		rhs   = curRHS.Node().(ast.Expr)
 		pos   = use.Pos()
 		scope = info.Scopes[pgf.File].Innermost(pos)
 	)
@@ -240,7 +241,7 @@ func inlineVariableOne(pkg *cache.Package, pgf *parsego.File, start, end token.P
 		if v, ok := obj1.(*types.Var); ok && v.IsField() {
 			continue // a field reference T{F: 0} is non-lexical
 		}
-		if astutil.NodeContainsPos(curRHS.Node(), obj1.Pos()) {
+		if astutil.NodeContainsPos(rhs, obj1.Pos()) {
 			continue // not free (id is defined within RHS)
 		}
 		_, obj2 := scope.LookupParent(id.Name, pos)
@@ -252,13 +253,16 @@ func inlineVariableOne(pkg *cache.Package, pgf *parsego.File, start, end token.P
 
 	// TODO(adonovan): also reject variables that are updated by assignments?
 
+	// Add parens to 'new' as needed by the 'use' context.
+	rhs = astutil.MaybeParenthesize(curUse.Parent().Node(), use, rhs)
+
 	return pkg.FileSet(), &analysis.SuggestedFix{
 		Message: fmt.Sprintf("Replace variable %q by its initializer expression", use.Name),
 		TextEdits: []analysis.TextEdit{
 			{
 				Pos:     use.Pos(),
 				End:     use.End(),
-				NewText: []byte(FormatNode(pkg.FileSet(), curRHS.Node())),
+				NewText: []byte(FormatNode(pkg.FileSet(), rhs)),
 			},
 		},
 	}, nil

gopls/internal/test/marker/testdata/codeaction/inline-lhs-var-method.txt

@@ -71,7 +71,7 @@ func _() {
     v.PointerMethod() //@codeaction("v", "refactor.inline.variable", err="0 CodeActions of kind refactor.inline.variable")
     (v).PointerMethod() //@codeaction("v", "refactor.inline.variable", err="0 CodeActions of kind refactor.inline.variable")
     var vptr *V = &v
-    &v.PointerMethod() //@codeaction("vptr", "refactor.inline.variable", result=inlintVptr)
+    (&v).PointerMethod() //@codeaction("vptr", "refactor.inline.variable", result=inlintVptr)
     (vptr).PointerMethod() //@codeaction("vptr", "refactor.inline.variable", result=inlintVptrpar)
     fmt.Println(v, vptr)
 }

gopls/internal/test/marker/testdata/codeaction/inline-var-parens.txt

@@ -0,0 +1,31 @@
+Test that refactor.inline.variable adds parens as needed to preserve
+operator precedence (go.dev/issue/77381).
+
+-- go.mod --
+module example.com/a
+go 1.18
+
+-- a/a.go --
+package a
+
+func _() {
+	x := 3 + 4
+	print(x) //@codeaction("x", "refactor.inline.variable", result=one)
+	print(2 * x) //@codeaction("x", "refactor.inline.variable", result=two)
+}
+-- @one/a/a.go --
+package a
+
+func _() {
+	x := 3 + 4
+	print(3 + 4) //@codeaction("x", "refactor.inline.variable", result=one)
+	print(2 * x) //@codeaction("x", "refactor.inline.variable", result=two)
+}
+-- @two/a/a.go --
+package a
+
+func _() {
+	x := 3 + 4
+	print(x) //@codeaction("x", "refactor.inline.variable", result=one)
+	print(2 * (3 + 4)) //@codeaction("x", "refactor.inline.variable", result=two)
+}

internal/astutil/util.go

@@ -228,3 +228,68 @@ func Select(curFile inspector.Cursor, start, end token.Pos) (_enclosing, _start,
 }
 
 var noCursor inspector.Cursor
+
+// MaybeParenthesize returns new, possibly wrapped in parens if needed
+// to preserve operator precedence when it replaces old, whose parent
+// is parentNode.
+//
+// (This would be more naturally written in terms of Cursor, but one of
+// the callers--the inliner--does not have cursors handy.)
+func MaybeParenthesize(parentNode ast.Node, old, new ast.Expr) ast.Expr {
+	if needsParens(parentNode, old, new) {
+		new = &ast.ParenExpr{X: new}
+	}
+	return new
+}
+
+func needsParens(parentNode ast.Node, old, new ast.Expr) bool {
+	// An expression beneath a non-expression
+	// has no precedence ambiguity.
+	parent, ok := parentNode.(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.go

@@ -132,8 +132,9 @@ func (st *state) inline() (*Result, error) {
 	// 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)}
+	if new, ok := res.new.(ast.Expr); ok {
+		parent := caller.path[slices.Index(caller.path, res.old)+1]
+		res.new = internalastutil.MaybeParenthesize(parent, res.old.(ast.Expr), new)
 	}
 
 	// Some reduction strategies return a new block holding the
@@ -3137,73 +3138,6 @@ func last[T any](slice []T) T {
 	return *new(T)
 }
 
-// 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.
-	i := slices.Index(callPath, old)
-	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
-}
-
 // declares returns the set of lexical names declared by a
 // sequence of statements from the same block, excluding sub-blocks.
 // (Lexical names do not include control labels.)