Skip to content

Commit 1261a24

Browse files
madelinekalilmadelinekalil
committed
gopls/internal/analysis/modernize: slicesdelete
Adds a new modernizer that suggests replacing instances of append(s[:i], s[i+k:]...) with slices.Delete(s, i, i+k) Handles other variations like append(s[:i-1], s[i:]...) and append(s[:i+2], s[i+3:]...) Updates golang/go#70815 Change-Id: I71981d6e8d6973bca17153b95f2eb9f1f229522d Reviewed-on: https://go-review.googlesource.com/c/tools/+/641357 Reviewed-by: Alan Donovan <[email protected]> Reviewed-by: Robert Findley <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent cab6608 commit 1261a24

File tree

8 files changed

+207
-4
lines changed

8 files changed

+207
-4
lines changed

‎gopls/doc/analyzers.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,9 @@ existing code by using more modern features of Go, such as:
482482
added in go1.19;
483483
- replacing uses of context.WithCancel in tests with t.Context, added in
484484
go1.24;
485-
- replacing omitempty by omitzero on structs, added in go 1.24
485+
- replacing omitempty by omitzero on structs, added in go 1.24;
486+
- replacing append(s[:i], s[i+1]...) by slices.Delete(s, i, i+1),
487+
added in go1.21
486488

487489
Default: on.
488490

‎gopls/internal/analysis/modernize/doc.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,5 +25,7 @@
2525
// added in go1.19;
2626
// - replacing uses of context.WithCancel in tests with t.Context, added in
2727
// go1.24;
28-
// - replacing omitempty by omitzero on structs, added in go 1.24
28+
// - replacing omitempty by omitzero on structs, added in go 1.24;
29+
// - replacing append(s[:i], s[i+1]...) by slices.Delete(s, i, i+1),
30+
// added in go1.21
2931
package modernize

‎gopls/internal/analysis/modernize/modernize.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ func run(pass *analysis.Pass) (any, error) {
6868
minmax(pass)
6969
omitzero(pass)
7070
slicescontains(pass)
71+
slicesdelete(pass)
7172
sortslice(pass)
7273
testingContext(pass)
7374

‎gopls/internal/analysis/modernize/modernize_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ func Test(t *testing.T) {
2121
"minmax",
2222
"omitzero",
2323
"slicescontains",
24+
"slicesdelete",
2425
"sortslice",
2526
"testingcontext",
2627
)

‎gopls/internal/analysis/modernize/slicesdelete.go‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
// Copyright 2024 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
package modernize
6+
7+
import (
8+
"go/ast"
9+
"go/constant"
10+
"go/token"
11+
"go/types"
12+
13+
"golang.org/x/tools/go/analysis"
14+
"golang.org/x/tools/go/analysis/passes/inspect"
15+
"golang.org/x/tools/go/ast/inspector"
16+
)
17+
18+
// The slicesdelete pass attempts to replace instances of append(s[:i], s[i+k:]...)
19+
// with slices.Delete(s, i, i+k) where k is some positive constant.
20+
// Other variations that will also have suggested replacements include:
21+
// append(s[:i-1], s[i:]...) and append(s[:i+k1], s[i+k2:]) where k2 > k1.
22+
func slicesdelete(pass *analysis.Pass) {
23+
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
24+
info := pass.TypesInfo
25+
report := func(call *ast.CallExpr, slice1, slice2 *ast.SliceExpr) {
26+
pass.Report(analysis.Diagnostic{
27+
Pos: call.Pos(),
28+
End: call.End(),
29+
Category: "slicesdelete",
30+
Message: "Replace append with slices.Delete",
31+
SuggestedFixes: []analysis.SuggestedFix{{
32+
Message: "Replace append with slices.Delete",
33+
TextEdits: []analysis.TextEdit{
34+
// Change name of called function.
35+
{
36+
Pos: call.Fun.Pos(),
37+
End: call.Fun.End(),
38+
NewText: []byte("slices.Delete"),
39+
},
40+
// Delete ellipsis.
41+
{
42+
Pos: call.Ellipsis,
43+
End: call.Ellipsis + token.Pos(len("...")), // delete ellipsis
44+
},
45+
// Remove second slice variable name.
46+
{
47+
Pos: slice2.X.Pos(),
48+
End: slice2.X.End(),
49+
},
50+
// Insert after first slice variable name.
51+
{
52+
Pos: slice1.X.End(),
53+
NewText: []byte(", "),
54+
},
55+
// Remove brackets and colons.
56+
{
57+
Pos: slice1.Lbrack,
58+
End: slice1.High.Pos(),
59+
},
60+
{
61+
Pos: slice1.Rbrack,
62+
End: slice1.Rbrack + 1,
63+
},
64+
{
65+
Pos: slice2.Lbrack,
66+
End: slice2.Lbrack + 1,
67+
},
68+
{
69+
Pos: slice2.Low.End(),
70+
End: slice2.Rbrack + 1,
71+
},
72+
},
73+
}},
74+
})
75+
}
76+
for curFile := range filesUsing(inspect, info, "go1.21") {
77+
for curCall := range curFile.Preorder((*ast.CallExpr)(nil)) {
78+
call := curCall.Node().(*ast.CallExpr)
79+
if id, ok := call.Fun.(*ast.Ident); ok && len(call.Args) == 2 {
80+
// Verify we have append with two slices and ... operator,
81+
// the first slice has no low index and second slice has no
82+
// high index, and not a three-index slice.
83+
if call.Ellipsis.IsValid() && info.Uses[id] == builtinAppend {
84+
slice1, ok1 := call.Args[0].(*ast.SliceExpr)
85+
slice2, ok2 := call.Args[1].(*ast.SliceExpr)
86+
if ok1 && slice1.Low == nil && !slice1.Slice3 &&
87+
ok2 && slice2.High == nil && !slice2.Slice3 &&
88+
equalSyntax(slice1.X, slice2.X) &&
89+
increasingSliceIndices(info, slice1.High, slice2.Low) {
90+
// Have append(s[:a], s[b:]...) where we can verify a < b.
91+
report(call, slice1, slice2)
92+
}
93+
}
94+
}
95+
}
96+
}
97+
}
98+
99+
// Given two slice indices a and b, returns true if we can verify that a < b.
100+
// It recognizes certain forms such as i+k1 < i+k2 where k1 < k2.
101+
func increasingSliceIndices(info *types.Info, a, b ast.Expr) bool {
102+
103+
// Given an expression of the form i±k, returns (i, k)
104+
// where k is a signed constant. Otherwise it returns (e, 0).
105+
split := func(e ast.Expr) (ast.Expr, constant.Value) {
106+
if binary, ok := e.(*ast.BinaryExpr); ok && (binary.Op == token.SUB || binary.Op == token.ADD) {
107+
// Negate constants if operation is subtract instead of add
108+
if k := info.Types[binary.Y].Value; k != nil {
109+
return binary.X, constant.UnaryOp(binary.Op, k, 0) // i ± k
110+
}
111+
}
112+
return e, constant.MakeInt64(0)
113+
}
114+
115+
// Handle case where either a or b is a constant
116+
ak := info.Types[a].Value
117+
bk := info.Types[b].Value
118+
if ak != nil || bk != nil {
119+
return ak != nil && bk != nil && constant.Compare(ak, token.LSS, bk)
120+
}
121+
122+
ai, ak := split(a)
123+
bi, bk := split(b)
124+
return equalSyntax(ai, bi) && constant.Compare(ak, token.LSS, bk)
125+
}

‎gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
package slicesdelete
2+
3+
var g struct{ f []int }
4+
5+
func slicesdelete(test, other []byte, i int) {
6+
const k = 1
7+
_ = append(test[:i], test[i+1:]...) // want "Replace append with slices.Delete"
8+
9+
_ = append(test[:i+1], test[i+2:]...) // want "Replace append with slices.Delete"
10+
11+
_ = append(test[:i+1], test[i+1:]...) // not deleting any slice elements
12+
13+
_ = append(test[:i], test[i-1:]...) // not deleting any slice elements
14+
15+
_ = append(test[:i-1], test[i:]...) // want "Replace append with slices.Delete"
16+
17+
_ = append(test[:i-2], test[i+1:]...) // want "Replace append with slices.Delete"
18+
19+
_ = append(test[:i-2], other[i+1:]...) // different slices "test" and "other"
20+
21+
_ = append(test[:i-2], other[i+1+k:]...) // cannot verify a < b
22+
23+
_ = append(test[:i-2], test[11:]...) // cannot verify a < b
24+
25+
_ = append(test[:1], test[3:]...) // want "Replace append with slices.Delete"
26+
27+
_ = append(g.f[:i], g.f[i+k:]...) // want "Replace append with slices.Delete"
28+
29+
_ = append(test[:3], test[i+1:]...) // cannot verify a < b
30+
31+
_ = append(test[:i-4], test[i-1:]...) // want "Replace append with slices.Delete"
32+
33+
_ = append(test[:1+2], test[3+4:]...) // want "Replace append with slices.Delete"
34+
35+
_ = append(test[:1+2], test[i-1:]...) // cannot verify a < b
36+
}

‎gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
package slicesdelete
2+
3+
var g struct{ f []int }
4+
5+
func slicesdelete(test, other []byte, i int) {
6+
const k = 1
7+
_ = slices.Delete(test, i, i+1) // want "Replace append with slices.Delete"
8+
9+
_ = slices.Delete(test, i+1, i+2) // want "Replace append with slices.Delete"
10+
11+
_ = append(test[:i+1], test[i+1:]...) // not deleting any slice elements
12+
13+
_ = append(test[:i], test[i-1:]...) // not deleting any slice elements
14+
15+
_ = slices.Delete(test, i-1, i) // want "Replace append with slices.Delete"
16+
17+
_ = slices.Delete(test, i-2, i+1) // want "Replace append with slices.Delete"
18+
19+
_ = append(test[:i-2], other[i+1:]...) // different slices "test" and "other"
20+
21+
_ = append(test[:i-2], other[i+1+k:]...) // cannot verify a < b
22+
23+
_ = append(test[:i-2], test[11:]...) // cannot verify a < b
24+
25+
_ = slices.Delete(test, 1, 3) // want "Replace append with slices.Delete"
26+
27+
_ = slices.Delete(g.f, i, i+k) // want "Replace append with slices.Delete"
28+
29+
_ = append(test[:3], test[i+1:]...) // cannot verify a < b
30+
31+
_ = slices.Delete(test, i-4, i-1) // want "Replace append with slices.Delete"
32+
33+
_ = slices.Delete(test, 1+2, 3+4) // want "Replace append with slices.Delete"
34+
35+
_ = append(test[:1+2], test[i-1:]...) // cannot verify a < b
36+
}

‎gopls/internal/doc/api.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -472,7 +472,7 @@
472472
},
473473
{
474474
"Name": "\"modernize\"",
475-
"Doc": "simplify code by using modern constructs\n\nThis analyzer reports opportunities for simplifying and clarifying\nexisting code by using more modern features of Go, such as:\n\n - replacing an if/else conditional assignment by a call to the\n built-in min or max functions added in go1.21;\n - replacing sort.Slice(x, func(i, j int) bool) { return s[i] \u003c s[j] }\n by a call to slices.Sort(s), added in go1.21;\n - replacing interface{} by the 'any' type added in go1.18;\n - replacing append([]T(nil), s...) by slices.Clone(s) or\n slices.Concat(s), added in go1.21;\n - replacing a loop around an m[k]=v map update by a call\n to one of the Collect, Copy, Clone, or Insert functions\n from the maps package, added in go1.21;\n - replacing []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),\n added in go1.19;\n - replacing uses of context.WithCancel in tests with t.Context, added in\n go1.24;\n - replacing omitempty by omitzero on structs, added in go 1.24",
475+
"Doc": "simplify code by using modern constructs\n\nThis analyzer reports opportunities for simplifying and clarifying\nexisting code by using more modern features of Go, such as:\n\n - replacing an if/else conditional assignment by a call to the\n built-in min or max functions added in go1.21;\n - replacing sort.Slice(x, func(i, j int) bool) { return s[i] \u003c s[j] }\n by a call to slices.Sort(s), added in go1.21;\n - replacing interface{} by the 'any' type added in go1.18;\n - replacing append([]T(nil), s...) by slices.Clone(s) or\n slices.Concat(s), added in go1.21;\n - replacing a loop around an m[k]=v map update by a call\n to one of the Collect, Copy, Clone, or Insert functions\n from the maps package, added in go1.21;\n - replacing []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),\n added in go1.19;\n - replacing uses of context.WithCancel in tests with t.Context, added in\n go1.24;\n - replacing omitempty by omitzero on structs, added in go 1.24;\n - replacing append(s[:i], s[i+1]...) by slices.Delete(s, i, i+1),\n added in go1.21",
476476
"Default": "true"
477477
},
478478
{
@@ -1129,7 +1129,7 @@
11291129
},
11301130
{
11311131
"Name": "modernize",
1132-
"Doc": "simplify code by using modern constructs\n\nThis analyzer reports opportunities for simplifying and clarifying\nexisting code by using more modern features of Go, such as:\n\n - replacing an if/else conditional assignment by a call to the\n built-in min or max functions added in go1.21;\n - replacing sort.Slice(x, func(i, j int) bool) { return s[i] \u003c s[j] }\n by a call to slices.Sort(s), added in go1.21;\n - replacing interface{} by the 'any' type added in go1.18;\n - replacing append([]T(nil), s...) by slices.Clone(s) or\n slices.Concat(s), added in go1.21;\n - replacing a loop around an m[k]=v map update by a call\n to one of the Collect, Copy, Clone, or Insert functions\n from the maps package, added in go1.21;\n - replacing []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),\n added in go1.19;\n - replacing uses of context.WithCancel in tests with t.Context, added in\n go1.24;\n - replacing omitempty by omitzero on structs, added in go 1.24",
1132+
"Doc": "simplify code by using modern constructs\n\nThis analyzer reports opportunities for simplifying and clarifying\nexisting code by using more modern features of Go, such as:\n\n - replacing an if/else conditional assignment by a call to the\n built-in min or max functions added in go1.21;\n - replacing sort.Slice(x, func(i, j int) bool) { return s[i] \u003c s[j] }\n by a call to slices.Sort(s), added in go1.21;\n - replacing interface{} by the 'any' type added in go1.18;\n - replacing append([]T(nil), s...) by slices.Clone(s) or\n slices.Concat(s), added in go1.21;\n - replacing a loop around an m[k]=v map update by a call\n to one of the Collect, Copy, Clone, or Insert functions\n from the maps package, added in go1.21;\n - replacing []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),\n added in go1.19;\n - replacing uses of context.WithCancel in tests with t.Context, added in\n go1.24;\n - replacing omitempty by omitzero on structs, added in go 1.24;\n - replacing append(s[:i], s[i+1]...) by slices.Delete(s, i, i+1),\n added in go1.21",
11331133
"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize",
11341134
"Default": true
11351135
},

0 commit comments

Comments
 (0)