Skip to content

Commit 02033b2

Browse files
committed
gopls/internal/analysis/modernize: simplify append(base, s...)
This CL adds the first modernizer for the "slices" package. It offers to replace an append (or tower of nested appends) to a clipped slice by a call to slices.Concat or slices.Clone. Examples: append(append(append(x[:0:0], a...), b...), c...) -> slices.Concat(a, b, c) append(append(slices.Clip(a), b...) -> slices.Concat(a, b) append([]T{}, a...) -> slices.Clone(a) append([]string(nil), os.Environ()...) -> os.Environ() It takes care not to offer fixes that would eliminate potentially important side effects such as to y's array in this example: append(y[:0], x...) -> slices.Clone(x) (unsound) However, it does not attempt to preserve the nilness of base in append(base, empty...) when the second argument is empty. Updates golang/go#70815 Change-Id: I5ea13bbf8bfd0b78a9fb71aef4c14848b860cc3a Reviewed-on: https://go-review.googlesource.com/c/tools/+/637735 Reviewed-by: Robert Findley <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent aa94d89 commit 02033b2

File tree

8 files changed

+317
-7
lines changed

8 files changed

+317
-7
lines changed

gopls/doc/analyzers.md

+2
Original file line numberDiff line numberDiff line change
@@ -448,6 +448,8 @@ existing code by using more modern features of Go, such as:
448448
- replacing sort.Slice(x, func(i, j int) bool) { return s[i] < s[j] }
449449
by a call to slices.Sort(s), added in go1.21;
450450
- replacing interface{} by the 'any' type added in go1.18;
451+
- replacing append([]T(nil), s...) by slices.Clone(s) or
452+
slices.Concat(s), added in go1.21;
451453

452454
Default: on.
453455

gopls/internal/analysis/modernize/doc.go

+2
Original file line numberDiff line numberDiff line change
@@ -16,4 +16,6 @@
1616
// - replacing sort.Slice(x, func(i, j int) bool) { return s[i] < s[j] }
1717
// by a call to slices.Sort(s), added in go1.21;
1818
// - replacing interface{} by the 'any' type added in go1.18;
19+
// - replacing append([]T(nil), s...) by slices.Clone(s) or
20+
// slices.Concat(s), added in go1.21;
1921
package modernize

gopls/internal/analysis/modernize/modernize.go

+24-4
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
package modernize
66

77
import (
8+
"bytes"
89
_ "embed"
910
"go/ast"
1011
"go/format"
@@ -55,9 +56,16 @@ func run(pass *analysis.Pass) (any, error) {
5556
minmax(pass)
5657
sortslice(pass)
5758
efaceany(pass)
59+
appendclipped(pass)
5860

59-
// TODO(adonovan): more modernizers here; see #70815.
60-
// TODO(adonovan): opt: interleave these micro-passes within a single inspection.
61+
// TODO(adonovan):
62+
// - more modernizers here; see #70815.
63+
// - opt: interleave these micro-passes within a single inspection.
64+
// - solve the "duplicate import" problem (#68765) when a number of
65+
// fixes in the same file are applied in parallel and all add
66+
// the same import. The tests exhibit the problem.
67+
// - should all diagnostics be of the form "x can be modernized by y"
68+
// or is that a foolish consistency?
6169

6270
return nil, nil
6371
}
@@ -81,8 +89,20 @@ func equalSyntax(x, y ast.Expr) bool {
8189
}
8290

8391
// formatNode formats n.
84-
func formatNode(fset *token.FileSet, n ast.Node) string {
85-
var buf strings.Builder
92+
func formatNode(fset *token.FileSet, n ast.Node) []byte {
93+
var buf bytes.Buffer
8694
format.Node(&buf, fset, n) // ignore errors
95+
return buf.Bytes()
96+
}
97+
98+
// formatExprs formats a comma-separated list of expressions.
99+
func formatExprs(fset *token.FileSet, exprs []ast.Expr) string {
100+
var buf strings.Builder
101+
for i, e := range exprs {
102+
if i > 0 {
103+
buf.WriteString(", ")
104+
}
105+
format.Node(&buf, fset, e) // ignore errors
106+
}
87107
return buf.String()
88108
}

gopls/internal/analysis/modernize/modernize_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -15,5 +15,6 @@ func Test(t *testing.T) {
1515
analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), modernize.Analyzer,
1616
"minmax",
1717
"sortslice",
18-
"efaceany")
18+
"efaceany",
19+
"appendclipped")
1920
}
+233
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,233 @@
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+
// This file defines modernizers that use the "slices" package.
8+
9+
import (
10+
"fmt"
11+
"go/ast"
12+
"go/token"
13+
"go/types"
14+
"slices"
15+
16+
"golang.org/x/tools/go/analysis"
17+
"golang.org/x/tools/go/analysis/passes/inspect"
18+
"golang.org/x/tools/go/ast/inspector"
19+
"golang.org/x/tools/internal/analysisinternal"
20+
"golang.org/x/tools/internal/astutil/cursor"
21+
"golang.org/x/tools/internal/versions"
22+
)
23+
24+
// The appendclipped pass offers to simplify a tower of append calls:
25+
//
26+
// append(append(append(base, a...), b..., c...)
27+
//
28+
// with a call to go1.21's slices.Concat(base, a, b, c), or simpler
29+
// replacements such as slices.Clone(a) in degenerate cases.
30+
//
31+
// The base expression must denote a clipped slice (see [isClipped]
32+
// for definition), otherwise the replacement might eliminate intended
33+
// side effects to the base slice's array.
34+
//
35+
// Examples:
36+
//
37+
// append(append(append(x[:0:0], a...), b...), c...) -> slices.Concat(a, b, c)
38+
// append(append(slices.Clip(a), b...) -> slices.Concat(a, b)
39+
// append([]T{}, a...) -> slices.Clone(a)
40+
// append([]string(nil), os.Environ()...) -> os.Environ()
41+
//
42+
// The fix does not always preserve nilness the of base slice when the
43+
// addends (a, b, c) are all empty.
44+
func appendclipped(pass *analysis.Pass) {
45+
if pass.Pkg.Path() == "slices" {
46+
return
47+
}
48+
49+
// sliceArgs is a non-empty (reversed) list of slices to be concatenated.
50+
simplifyAppendEllipsis := func(call *ast.CallExpr, base ast.Expr, sliceArgs []ast.Expr) {
51+
// Only appends whose base is a clipped slice can be simplified:
52+
// We must conservatively assume an append to an unclipped slice
53+
// such as append(y[:0], x...) is intended to have effects on y.
54+
clipped, empty := isClippedSlice(pass.TypesInfo, base)
55+
if !clipped {
56+
return
57+
}
58+
59+
// If the (clipped) base is empty, it may be safely ignored.
60+
// Otherwise treat it as just another arg (the first) to Concat.
61+
if !empty {
62+
sliceArgs = append(sliceArgs, base)
63+
}
64+
slices.Reverse(sliceArgs)
65+
66+
// Concat of a single (non-trivial) slice degenerates to Clone.
67+
if len(sliceArgs) == 1 {
68+
s := sliceArgs[0]
69+
70+
// Special case for common but redundant clone of os.Environ().
71+
// append(zerocap, os.Environ()...) -> os.Environ()
72+
if scall, ok := s.(*ast.CallExpr); ok &&
73+
isQualifiedIdent(pass.TypesInfo, scall.Fun, "os", "Environ") {
74+
75+
pass.Report(analysis.Diagnostic{
76+
Pos: call.Pos(),
77+
End: call.End(),
78+
Category: "slicesclone",
79+
Message: "Redundant clone of os.Environ()",
80+
SuggestedFixes: []analysis.SuggestedFix{{
81+
Message: "Eliminate redundant clone",
82+
TextEdits: []analysis.TextEdit{{
83+
Pos: call.Pos(),
84+
End: call.End(),
85+
NewText: formatNode(pass.Fset, s),
86+
}},
87+
}},
88+
})
89+
return
90+
}
91+
92+
// append(zerocap, s...) -> slices.Clone(s)
93+
file := enclosingFile(pass, call.Pos())
94+
slicesName, importEdits := analysisinternal.AddImport(pass.TypesInfo, file, call.Pos(), "slices", "slices")
95+
pass.Report(analysis.Diagnostic{
96+
Pos: call.Pos(),
97+
End: call.End(),
98+
Category: "slicesclone",
99+
Message: "Replace append with slices.Clone",
100+
SuggestedFixes: []analysis.SuggestedFix{{
101+
Message: "Replace append with slices.Clone",
102+
TextEdits: append(importEdits, []analysis.TextEdit{{
103+
Pos: call.Pos(),
104+
End: call.End(),
105+
NewText: []byte(fmt.Sprintf("%s.Clone(%s)", slicesName, formatNode(pass.Fset, s))),
106+
}}...),
107+
}},
108+
})
109+
return
110+
}
111+
112+
// append(append(append(base, a...), b..., c...) -> slices.Concat(base, a, b, c)
113+
//
114+
// TODO(adonovan): simplify sliceArgs[0] further:
115+
// - slices.Clone(s) -> s
116+
// - s[:len(s):len(s)] -> s
117+
// - slices.Clip(s) -> s
118+
file := enclosingFile(pass, call.Pos())
119+
slicesName, importEdits := analysisinternal.AddImport(pass.TypesInfo, file, call.Pos(), "slices", "slices")
120+
pass.Report(analysis.Diagnostic{
121+
Pos: call.Pos(),
122+
End: call.End(),
123+
Category: "slicesclone",
124+
Message: "Replace append with slices.Concat",
125+
SuggestedFixes: []analysis.SuggestedFix{{
126+
Message: "Replace append with slices.Concat",
127+
TextEdits: append(importEdits, []analysis.TextEdit{{
128+
Pos: call.Pos(),
129+
End: call.End(),
130+
NewText: []byte(fmt.Sprintf("%s.Concat(%s)", slicesName, formatExprs(pass.Fset, sliceArgs))),
131+
}}...),
132+
}},
133+
})
134+
}
135+
136+
// Mark nested calls to append so that we don't emit diagnostics for them.
137+
skip := make(map[*ast.CallExpr]bool)
138+
139+
// Visit calls of form append(x, y...).
140+
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
141+
filter := []ast.Node{(*ast.File)(nil), (*ast.CallExpr)(nil)}
142+
cursor.Root(inspect).Inspect(filter, func(cur cursor.Cursor, push bool) (descend bool) {
143+
if push {
144+
switch n := cur.Node().(type) {
145+
case *ast.File:
146+
if versions.Before(pass.TypesInfo.FileVersions[n], "go1.21") {
147+
return false
148+
}
149+
150+
case *ast.CallExpr:
151+
call := n
152+
if skip[call] {
153+
return true
154+
}
155+
156+
// Recursively unwrap ellipsis calls to append, so
157+
// append(append(append(base, a...), b..., c...)
158+
// yields (base, [c b a]).
159+
base, slices := ast.Expr(call), []ast.Expr(nil) // base case: (call, nil)
160+
again:
161+
if call, ok := base.(*ast.CallExpr); ok {
162+
if id, ok := call.Fun.(*ast.Ident); ok &&
163+
call.Ellipsis.IsValid() &&
164+
len(call.Args) == 2 &&
165+
pass.TypesInfo.Uses[id] == builtinAppend {
166+
167+
// Have: append(base, s...)
168+
base, slices = call.Args[0], append(slices, call.Args[1])
169+
skip[call] = true
170+
goto again
171+
}
172+
}
173+
174+
if len(slices) > 0 {
175+
simplifyAppendEllipsis(call, base, slices)
176+
}
177+
}
178+
}
179+
return true
180+
})
181+
}
182+
183+
// isClippedSlice reports whether e denotes a slice that is definitely
184+
// clipped, that is, its len(s)==cap(s).
185+
//
186+
// In addition, it reports whether the slice is definitely empty.
187+
//
188+
// Examples of clipped slices:
189+
//
190+
// x[:0:0] (empty)
191+
// []T(nil) (empty)
192+
// Slice{} (empty)
193+
// x[:len(x):len(x)] (nonempty)
194+
// x[:k:k] (nonempty)
195+
// slices.Clip(x) (nonempty)
196+
func isClippedSlice(info *types.Info, e ast.Expr) (clipped, empty bool) {
197+
switch e := e.(type) {
198+
case *ast.SliceExpr:
199+
// x[:0:0], x[:len(x):len(x)], x[:k:k], x[:0]
200+
isZeroLiteral := func(e ast.Expr) bool {
201+
lit, ok := e.(*ast.BasicLit)
202+
return ok && lit.Kind == token.INT && lit.Value == "0"
203+
}
204+
clipped = e.Slice3 && e.High != nil && e.Max != nil && equalSyntax(e.High, e.Max) // x[:k:k]
205+
empty = e.High != nil && isZeroLiteral(e.High) // x[:0:*]
206+
return
207+
208+
case *ast.CallExpr:
209+
// []T(nil)?
210+
if info.Types[e.Fun].IsType() &&
211+
is[*ast.Ident](e.Args[0]) &&
212+
info.Uses[e.Args[0].(*ast.Ident)] == builtinNil {
213+
return true, true
214+
}
215+
216+
// slices.Clip(x)?
217+
if isQualifiedIdent(info, e.Fun, "slices", "Clip") {
218+
return true, false
219+
}
220+
221+
case *ast.CompositeLit:
222+
// Slice{}?
223+
if len(e.Elts) == 0 {
224+
return true, true
225+
}
226+
}
227+
return false, false
228+
}
229+
230+
var (
231+
builtinAppend = types.Universe.Lookup("append")
232+
builtinNil = types.Universe.Lookup("nil")
233+
)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package appendclipped
2+
3+
import (
4+
"os"
5+
"slices"
6+
)
7+
8+
type Bytes []byte
9+
10+
func _(s, other []string) {
11+
print(append([]string{}, s...)) // want "Replace append with slices.Clone"
12+
print(append([]string(nil), s...)) // want "Replace append with slices.Clone"
13+
print(append(Bytes(nil), Bytes{1, 2, 3}...)) // want "Replace append with slices.Clone"
14+
print(append(other[:0:0], s...)) // want "Replace append with slices.Clone"
15+
print(append(other[:0:0], os.Environ()...)) // want "Redundant clone of os.Environ()"
16+
print(append(other[:0], s...)) // nope: intent may be to mutate other
17+
18+
print(append(append(append([]string{}, s...), other...), other...)) // want "Replace append with slices.Concat"
19+
print(append(append(append([]string(nil), s...), other...), other...)) // want "Replace append with slices.Concat"
20+
print(append(append(Bytes(nil), Bytes{1, 2, 3}...), Bytes{4, 5, 6}...)) // want "Replace append with slices.Concat"
21+
print(append(append(append(other[:0:0], s...), other...), other...)) // want "Replace append with slices.Concat"
22+
print(append(append(append(other[:0:0], os.Environ()...), other...), other...)) // want "Replace append with slices.Concat"
23+
print(append(append(other[:len(other):len(other)], s...), other...)) // want "Replace append with slices.Concat"
24+
print(append(append(slices.Clip(other), s...), other...)) // want "Replace append with slices.Concat"
25+
print(append(append(append(other[:0], s...), other...), other...)) // nope: intent may be to mutate other
26+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package appendclipped
2+
3+
import (
4+
"os"
5+
"slices"
6+
)
7+
8+
type Bytes []byte
9+
10+
func _(s, other []string) {
11+
print(slices.Clone(s)) // want "Replace append with slices.Clone"
12+
print(slices.Clone(s)) // want "Replace append with slices.Clone"
13+
print(slices.Clone(Bytes{1, 2, 3})) // want "Replace append with slices.Clone"
14+
print(slices.Clone(s)) // want "Replace append with slices.Clone"
15+
print(os.Environ()) // want "Redundant clone of os.Environ()"
16+
print(append(other[:0], s...)) // nope: intent may be to mutate other
17+
18+
print(slices.Concat(s, other, other)) // want "Replace append with slices.Concat"
19+
print(slices.Concat(s, other, other)) // want "Replace append with slices.Concat"
20+
print(slices.Concat(Bytes{1, 2, 3}, Bytes{4, 5, 6})) // want "Replace append with slices.Concat"
21+
print(slices.Concat(s, other, other)) // want "Replace append with slices.Concat"
22+
print(slices.Concat(os.Environ(), other, other)) // want "Replace append with slices.Concat"
23+
print(slices.Concat(other[:len(other):len(other)], s, other)) // want "Replace append with slices.Concat"
24+
print(slices.Concat(slices.Clip(other), s, other)) // want "Replace append with slices.Concat"
25+
print(append(append(append(other[:0], s...), other...), other...)) // nope: intent may be to mutate other
26+
}

gopls/internal/doc/api.json

+2-2
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,7 @@
467467
},
468468
{
469469
"Name": "\"modernize\"",
470-
"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 if/else conditional assignments 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;",
470+
"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 if/else conditional assignments 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;",
471471
"Default": "true"
472472
},
473473
{
@@ -1133,7 +1133,7 @@
11331133
},
11341134
{
11351135
"Name": "modernize",
1136-
"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 if/else conditional assignments 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;",
1136+
"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 if/else conditional assignments 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;",
11371137
"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize",
11381138
"Default": true
11391139
},

0 commit comments

Comments
 (0)