From 1058109b662941a394d08bbfbe728020129e5b78 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Sun, 1 Oct 2023 16:02:31 -0400 Subject: [PATCH] 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 Reviewed-by: Robert Findley Auto-Submit: Alan Donovan --- .../marker/testdata/codeaction/inline.txt | 2 +- internal/refactor/inline/inline.go | 121 ++++++++++++++---- internal/refactor/inline/inline_test.go | 49 ++++++- .../refactor/inline/testdata/basic-err.txtar | 2 +- .../inline/testdata/basic-reduce.txtar | 4 +- .../refactor/inline/testdata/comments.txtar | 2 +- .../inline/testdata/param-subst.txtar | 2 +- 7 files changed, 152 insertions(+), 30 deletions(-) diff --git a/gopls/internal/regtest/marker/testdata/codeaction/inline.txt b/gopls/internal/regtest/marker/testdata/codeaction/inline.txt index 134065f26b9..15d3cabfcc8 100644 --- a/gopls/internal/regtest/marker/testdata/codeaction/inline.txt +++ b/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 } diff --git a/internal/refactor/inline/inline.go b/internal/refactor/inline/inline.go index 0c9c1b87992..e5fe3336122 100644 --- a/internal/refactor/inline/inline.go +++ b/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 +} diff --git a/internal/refactor/inline/inline_test.go b/internal/refactor/inline/inline_test.go index eca5a914b8d..f0459464b26 100644 --- a/internal/refactor/inline/inline_test.go +++ b/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.", diff --git a/internal/refactor/inline/testdata/basic-err.txtar b/internal/refactor/inline/testdata/basic-err.txtar index c289e9bb544..4868b2cbfb1 100644 --- a/internal/refactor/inline/testdata/basic-err.txtar +++ b/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() } diff --git a/internal/refactor/inline/testdata/basic-reduce.txtar b/internal/refactor/inline/testdata/basic-reduce.txtar index 46e44b77625..10aca5284ef 100644 --- a/internal/refactor/inline/testdata/basic-reduce.txtar +++ b/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) diff --git a/internal/refactor/inline/testdata/comments.txtar b/internal/refactor/inline/testdata/comments.txtar index 189e9a20e8d..76f64926b13 100644 --- a/internal/refactor/inline/testdata/comments.txtar +++ b/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 } diff --git a/internal/refactor/inline/testdata/param-subst.txtar b/internal/refactor/inline/testdata/param-subst.txtar index 135313b865a..b6e462d7e71 100644 --- a/internal/refactor/inline/testdata/param-subst.txtar +++ b/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 } \ No newline at end of file