gopls/internal/analysis/modernize: appendclipped: unclip
The appendclipped pass must ascertain that the first argument
to append(x, y...) is clipped, so that we don't eliminate
possible intended side effects on x, but in some cases:
- append(x[:len(x):len(x)], y...)
- append(slices.Clip(x), y...)
we can further simplify the first argument to its unclipped
version (just x in both cases), so that the result is:
slices.Concat(x, y)
+ test
Fixes golang/go#71296
Change-Id: I89cc4350b5dbd57c88c35c0b4459b23347814441
Reviewed-on: https://go-review.googlesource.com/c/tools/+/647796
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go
index 861194e..0f7b58e 100644
--- a/gopls/internal/analysis/modernize/modernize.go
+++ b/gopls/internal/analysis/modernize/modernize.go
@@ -130,6 +130,7 @@
builtinAppend = types.Universe.Lookup("append")
builtinBool = types.Universe.Lookup("bool")
builtinFalse = types.Universe.Lookup("false")
+ builtinLen = types.Universe.Lookup("len")
builtinMake = types.Universe.Lookup("make")
builtinNil = types.Universe.Lookup("nil")
builtinTrue = types.Universe.Lookup("true")
diff --git a/gopls/internal/analysis/modernize/slices.go b/gopls/internal/analysis/modernize/slices.go
index aada97d..bdab9de 100644
--- a/gopls/internal/analysis/modernize/slices.go
+++ b/gopls/internal/analysis/modernize/slices.go
@@ -52,18 +52,21 @@
// Only appends whose base is a clipped slice can be simplified:
// We must conservatively assume an append to an unclipped slice
// such as append(y[:0], x...) is intended to have effects on y.
- clipped, empty := isClippedSlice(info, base)
- if !clipped {
+ clipped, empty := clippedSlice(info, base)
+ if clipped == nil {
return
}
// If the (clipped) base is empty, it may be safely ignored.
- // Otherwise treat it as just another arg (the first) to Concat.
+ // Otherwise treat it (or its unclipped subexpression, if possible)
+ // as just another arg (the first) to Concat.
if !empty {
- sliceArgs = append(sliceArgs, base)
+ sliceArgs = append(sliceArgs, clipped)
}
slices.Reverse(sliceArgs)
+ // TODO(adonovan): simplify sliceArgs[0] further: slices.Clone(s) -> s
+
// Concat of a single (non-trivial) slice degenerates to Clone.
if len(sliceArgs) == 1 {
s := sliceArgs[0]
@@ -111,11 +114,6 @@
}
// append(append(append(base, a...), b..., c...) -> slices.Concat(base, a, b, c)
- //
- // TODO(adonovan): simplify sliceArgs[0] further:
- // - slices.Clone(s) -> s
- // - s[:len(s):len(s)] -> s
- // - slices.Clip(s) -> s
_, prefix, importEdits := analysisinternal.AddImport(info, file, "slices", "slices", "Concat", call.Pos())
pass.Report(analysis.Diagnostic{
Pos: call.Pos(),
@@ -172,25 +170,36 @@
}
}
-// isClippedSlice reports whether e denotes a slice that is definitely
-// clipped, that is, its len(s)==cap(s).
+// clippedSlice returns res != nil if e denotes a slice that is
+// definitely clipped, that is, its len(s)==cap(s).
//
-// In addition, it reports whether the slice is definitely empty.
+// The value of res is either the same as e or is a subexpression of e
+// that denotes the same slice but without the clipping operation.
+//
+// In addition, it reports whether the slice is definitely empty,
//
// Examples of clipped slices:
//
// x[:0:0] (empty)
// []T(nil) (empty)
// Slice{} (empty)
-// x[:len(x):len(x)] (nonempty)
+// x[:len(x):len(x)] (nonempty) res=x
// x[:k:k] (nonempty)
-// slices.Clip(x) (nonempty)
-func isClippedSlice(info *types.Info, e ast.Expr) (clipped, empty bool) {
+// slices.Clip(x) (nonempty) res=x
+func clippedSlice(info *types.Info, e ast.Expr) (res ast.Expr, empty bool) {
switch e := e.(type) {
case *ast.SliceExpr:
- // x[:0:0], x[:len(x):len(x)], x[:k:k], x[:0]
- clipped = e.Slice3 && e.High != nil && e.Max != nil && equalSyntax(e.High, e.Max) // x[:k:k]
- empty = e.High != nil && isZeroLiteral(e.High) // x[:0:*]
+ // x[:0:0], x[:len(x):len(x)], x[:k:k]
+ if e.Slice3 && e.High != nil && e.Max != nil && equalSyntax(e.High, e.Max) { // x[:k:k]
+ res = e
+ empty = isZeroLiteral(e.High) // x[:0:0]
+ if call, ok := e.High.(*ast.CallExpr); ok &&
+ typeutil.Callee(info, call) == builtinLen &&
+ equalSyntax(call.Args[0], e.X) {
+ res = e.X // x[:len(x):len(x)] -> x
+ }
+ return
+ }
return
case *ast.CallExpr:
@@ -198,20 +207,20 @@
if info.Types[e.Fun].IsType() &&
is[*ast.Ident](e.Args[0]) &&
info.Uses[e.Args[0].(*ast.Ident)] == builtinNil {
- return true, true
+ return e, true
}
// slices.Clip(x)?
obj := typeutil.Callee(info, e)
if analysisinternal.IsFunctionNamed(obj, "slices", "Clip") {
- return true, false
+ return e.Args[0], false // slices.Clip(x) -> x
}
case *ast.CompositeLit:
// Slice{}?
if len(e.Elts) == 0 {
- return true, true
+ return e, true
}
}
- return false, false
+ return nil, false
}
diff --git a/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden b/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden
index 5d6761b..6352d52 100644
--- a/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden
+++ b/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden
@@ -20,7 +20,7 @@
print(slices.Concat(Bytes{1, 2, 3}, Bytes{4, 5, 6})) // want "Replace append with slices.Concat"
print(slices.Concat(s, other, other)) // want "Replace append with slices.Concat"
print(slices.Concat(os.Environ(), other, other)) // want "Replace append with slices.Concat"
- print(slices.Concat(other[:len(other):len(other)], s, other)) // want "Replace append with slices.Concat"
- print(slices.Concat(slices.Clip(other), s, other)) // want "Replace append with slices.Concat"
+ print(slices.Concat(other, s, other)) // want "Replace append with slices.Concat"
+ print(slices.Concat(other, s, other)) // want "Replace append with slices.Concat"
print(append(append(append(other[:0], s...), other...), other...)) // nope: intent may be to mutate other
}
