From 02033b269b1767d750e5a3602eed2c9ff59066da Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 18 Dec 2024 22:08:34 -0500 Subject: [PATCH] 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 LUCI-TryBot-Result: Go LUCI --- gopls/doc/analyzers.md | 2 + gopls/internal/analysis/modernize/doc.go | 2 + .../internal/analysis/modernize/modernize.go | 28 ++- .../analysis/modernize/modernize_test.go | 3 +- gopls/internal/analysis/modernize/slices.go | 233 ++++++++++++++++++ .../src/appendclipped/appendclipped.go | 26 ++ .../src/appendclipped/appendclipped.go.golden | 26 ++ gopls/internal/doc/api.json | 4 +- 8 files changed, 317 insertions(+), 7 deletions(-) create mode 100644 gopls/internal/analysis/modernize/slices.go create mode 100644 gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go create mode 100644 gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md index d6bb488d25d..b7ba913e0e9 100644 --- a/gopls/doc/analyzers.md +++ b/gopls/doc/analyzers.md @@ -448,6 +448,8 @@ existing code by using more modern features of Go, such as: - replacing sort.Slice(x, func(i, j int) bool) { return s[i] < s[j] } by a call to slices.Sort(s), added in go1.21; - replacing interface{} by the 'any' type added in go1.18; + - replacing append([]T(nil), s...) by slices.Clone(s) or + slices.Concat(s), added in go1.21; Default: on. diff --git a/gopls/internal/analysis/modernize/doc.go b/gopls/internal/analysis/modernize/doc.go index 6d6b11cb78d..e2bbb70c678 100644 --- a/gopls/internal/analysis/modernize/doc.go +++ b/gopls/internal/analysis/modernize/doc.go @@ -16,4 +16,6 @@ // - replacing sort.Slice(x, func(i, j int) bool) { return s[i] < s[j] } // by a call to slices.Sort(s), added in go1.21; // - replacing interface{} by the 'any' type added in go1.18; +// - replacing append([]T(nil), s...) by slices.Clone(s) or +// slices.Concat(s), added in go1.21; package modernize diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go index 81b53412b55..b131c5354e7 100644 --- a/gopls/internal/analysis/modernize/modernize.go +++ b/gopls/internal/analysis/modernize/modernize.go @@ -5,6 +5,7 @@ package modernize import ( + "bytes" _ "embed" "go/ast" "go/format" @@ -55,9 +56,16 @@ func run(pass *analysis.Pass) (any, error) { minmax(pass) sortslice(pass) efaceany(pass) + appendclipped(pass) - // TODO(adonovan): more modernizers here; see #70815. - // TODO(adonovan): opt: interleave these micro-passes within a single inspection. + // TODO(adonovan): + // - more modernizers here; see #70815. + // - opt: interleave these micro-passes within a single inspection. + // - solve the "duplicate import" problem (#68765) when a number of + // fixes in the same file are applied in parallel and all add + // the same import. The tests exhibit the problem. + // - should all diagnostics be of the form "x can be modernized by y" + // or is that a foolish consistency? return nil, nil } @@ -81,8 +89,20 @@ func equalSyntax(x, y ast.Expr) bool { } // formatNode formats n. -func formatNode(fset *token.FileSet, n ast.Node) string { - var buf strings.Builder +func formatNode(fset *token.FileSet, n ast.Node) []byte { + var buf bytes.Buffer format.Node(&buf, fset, n) // ignore errors + return buf.Bytes() +} + +// formatExprs formats a comma-separated list of expressions. +func formatExprs(fset *token.FileSet, exprs []ast.Expr) string { + var buf strings.Builder + for i, e := range exprs { + if i > 0 { + buf.WriteString(", ") + } + format.Node(&buf, fset, e) // ignore errors + } return buf.String() } diff --git a/gopls/internal/analysis/modernize/modernize_test.go b/gopls/internal/analysis/modernize/modernize_test.go index 696aaccb331..187f059930c 100644 --- a/gopls/internal/analysis/modernize/modernize_test.go +++ b/gopls/internal/analysis/modernize/modernize_test.go @@ -15,5 +15,6 @@ func Test(t *testing.T) { analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), modernize.Analyzer, "minmax", "sortslice", - "efaceany") + "efaceany", + "appendclipped") } diff --git a/gopls/internal/analysis/modernize/slices.go b/gopls/internal/analysis/modernize/slices.go new file mode 100644 index 00000000000..b26db3b101b --- /dev/null +++ b/gopls/internal/analysis/modernize/slices.go @@ -0,0 +1,233 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package modernize + +// This file defines modernizers that use the "slices" package. + +import ( + "fmt" + "go/ast" + "go/token" + "go/types" + "slices" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/internal/analysisinternal" + "golang.org/x/tools/internal/astutil/cursor" + "golang.org/x/tools/internal/versions" +) + +// The appendclipped pass offers to simplify a tower of append calls: +// +// append(append(append(base, a...), b..., c...) +// +// with a call to go1.21's slices.Concat(base, a, b, c), or simpler +// replacements such as slices.Clone(a) in degenerate cases. +// +// The base expression must denote a clipped slice (see [isClipped] +// for definition), otherwise the replacement might eliminate intended +// side effects to the base slice's array. +// +// 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() +// +// The fix does not always preserve nilness the of base slice when the +// addends (a, b, c) are all empty. +func appendclipped(pass *analysis.Pass) { + if pass.Pkg.Path() == "slices" { + return + } + + // sliceArgs is a non-empty (reversed) list of slices to be concatenated. + simplifyAppendEllipsis := func(call *ast.CallExpr, base ast.Expr, sliceArgs []ast.Expr) { + // 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(pass.TypesInfo, base) + if !clipped { + return + } + + // If the (clipped) base is empty, it may be safely ignored. + // Otherwise treat it as just another arg (the first) to Concat. + if !empty { + sliceArgs = append(sliceArgs, base) + } + slices.Reverse(sliceArgs) + + // Concat of a single (non-trivial) slice degenerates to Clone. + if len(sliceArgs) == 1 { + s := sliceArgs[0] + + // Special case for common but redundant clone of os.Environ(). + // append(zerocap, os.Environ()...) -> os.Environ() + if scall, ok := s.(*ast.CallExpr); ok && + isQualifiedIdent(pass.TypesInfo, scall.Fun, "os", "Environ") { + + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Category: "slicesclone", + Message: "Redundant clone of os.Environ()", + SuggestedFixes: []analysis.SuggestedFix{{ + Message: "Eliminate redundant clone", + TextEdits: []analysis.TextEdit{{ + Pos: call.Pos(), + End: call.End(), + NewText: formatNode(pass.Fset, s), + }}, + }}, + }) + return + } + + // append(zerocap, s...) -> slices.Clone(s) + file := enclosingFile(pass, call.Pos()) + slicesName, importEdits := analysisinternal.AddImport(pass.TypesInfo, file, call.Pos(), "slices", "slices") + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Category: "slicesclone", + Message: "Replace append with slices.Clone", + SuggestedFixes: []analysis.SuggestedFix{{ + Message: "Replace append with slices.Clone", + TextEdits: append(importEdits, []analysis.TextEdit{{ + Pos: call.Pos(), + End: call.End(), + NewText: []byte(fmt.Sprintf("%s.Clone(%s)", slicesName, formatNode(pass.Fset, s))), + }}...), + }}, + }) + return + } + + // 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 + file := enclosingFile(pass, call.Pos()) + slicesName, importEdits := analysisinternal.AddImport(pass.TypesInfo, file, call.Pos(), "slices", "slices") + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Category: "slicesclone", + Message: "Replace append with slices.Concat", + SuggestedFixes: []analysis.SuggestedFix{{ + Message: "Replace append with slices.Concat", + TextEdits: append(importEdits, []analysis.TextEdit{{ + Pos: call.Pos(), + End: call.End(), + NewText: []byte(fmt.Sprintf("%s.Concat(%s)", slicesName, formatExprs(pass.Fset, sliceArgs))), + }}...), + }}, + }) + } + + // Mark nested calls to append so that we don't emit diagnostics for them. + skip := make(map[*ast.CallExpr]bool) + + // Visit calls of form append(x, y...). + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + filter := []ast.Node{(*ast.File)(nil), (*ast.CallExpr)(nil)} + cursor.Root(inspect).Inspect(filter, func(cur cursor.Cursor, push bool) (descend bool) { + if push { + switch n := cur.Node().(type) { + case *ast.File: + if versions.Before(pass.TypesInfo.FileVersions[n], "go1.21") { + return false + } + + case *ast.CallExpr: + call := n + if skip[call] { + return true + } + + // Recursively unwrap ellipsis calls to append, so + // append(append(append(base, a...), b..., c...) + // yields (base, [c b a]). + base, slices := ast.Expr(call), []ast.Expr(nil) // base case: (call, nil) + again: + if call, ok := base.(*ast.CallExpr); ok { + if id, ok := call.Fun.(*ast.Ident); ok && + call.Ellipsis.IsValid() && + len(call.Args) == 2 && + pass.TypesInfo.Uses[id] == builtinAppend { + + // Have: append(base, s...) + base, slices = call.Args[0], append(slices, call.Args[1]) + skip[call] = true + goto again + } + } + + if len(slices) > 0 { + simplifyAppendEllipsis(call, base, slices) + } + } + } + return true + }) +} + +// isClippedSlice reports whether 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. +// +// Examples of clipped slices: +// +// x[:0:0] (empty) +// []T(nil) (empty) +// Slice{} (empty) +// x[:len(x):len(x)] (nonempty) +// x[:k:k] (nonempty) +// slices.Clip(x) (nonempty) +func isClippedSlice(info *types.Info, e ast.Expr) (clipped, empty bool) { + switch e := e.(type) { + case *ast.SliceExpr: + // x[:0:0], x[:len(x):len(x)], x[:k:k], x[:0] + isZeroLiteral := func(e ast.Expr) bool { + lit, ok := e.(*ast.BasicLit) + return ok && lit.Kind == token.INT && lit.Value == "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:*] + return + + case *ast.CallExpr: + // []T(nil)? + if info.Types[e.Fun].IsType() && + is[*ast.Ident](e.Args[0]) && + info.Uses[e.Args[0].(*ast.Ident)] == builtinNil { + return true, true + } + + // slices.Clip(x)? + if isQualifiedIdent(info, e.Fun, "slices", "Clip") { + return true, false + } + + case *ast.CompositeLit: + // Slice{}? + if len(e.Elts) == 0 { + return true, true + } + } + return false, false +} + +var ( + builtinAppend = types.Universe.Lookup("append") + builtinNil = types.Universe.Lookup("nil") +) diff --git a/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go b/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go new file mode 100644 index 00000000000..c4e98535a37 --- /dev/null +++ b/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go @@ -0,0 +1,26 @@ +package appendclipped + +import ( + "os" + "slices" +) + +type Bytes []byte + +func _(s, other []string) { + print(append([]string{}, s...)) // want "Replace append with slices.Clone" + print(append([]string(nil), s...)) // want "Replace append with slices.Clone" + print(append(Bytes(nil), Bytes{1, 2, 3}...)) // want "Replace append with slices.Clone" + print(append(other[:0:0], s...)) // want "Replace append with slices.Clone" + print(append(other[:0:0], os.Environ()...)) // want "Redundant clone of os.Environ()" + print(append(other[:0], s...)) // nope: intent may be to mutate other + + print(append(append(append([]string{}, s...), other...), other...)) // want "Replace append with slices.Concat" + print(append(append(append([]string(nil), s...), other...), other...)) // want "Replace append with slices.Concat" + print(append(append(Bytes(nil), Bytes{1, 2, 3}...), Bytes{4, 5, 6}...)) // want "Replace append with slices.Concat" + print(append(append(append(other[:0:0], s...), other...), other...)) // want "Replace append with slices.Concat" + print(append(append(append(other[:0:0], os.Environ()...), other...), other...)) // want "Replace append with slices.Concat" + print(append(append(other[:len(other):len(other)], s...), other...)) // want "Replace append with slices.Concat" + print(append(append(slices.Clip(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 +} diff --git a/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden b/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden new file mode 100644 index 00000000000..5d6761b5371 --- /dev/null +++ b/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden @@ -0,0 +1,26 @@ +package appendclipped + +import ( + "os" + "slices" +) + +type Bytes []byte + +func _(s, other []string) { + print(slices.Clone(s)) // want "Replace append with slices.Clone" + print(slices.Clone(s)) // want "Replace append with slices.Clone" + print(slices.Clone(Bytes{1, 2, 3})) // want "Replace append with slices.Clone" + print(slices.Clone(s)) // want "Replace append with slices.Clone" + print(os.Environ()) // want "Redundant clone of os.Environ()" + print(append(other[:0], s...)) // nope: intent may be to mutate other + + print(slices.Concat(s, other, other)) // want "Replace append with slices.Concat" + print(slices.Concat(s, other, other)) // want "Replace append with slices.Concat" + 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(append(append(append(other[:0], s...), other...), other...)) // nope: intent may be to mutate other +} diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json index f154d72f0d8..894d33b6371 100644 --- a/gopls/internal/doc/api.json +++ b/gopls/internal/doc/api.json @@ -467,7 +467,7 @@ }, { "Name": "\"modernize\"", - "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;", + "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;", "Default": "true" }, { @@ -1133,7 +1133,7 @@ }, { "Name": "modernize", - "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;", + "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;", "URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize", "Default": true },