Skip to content

Commit 93cc684

Browse files
adonovanadonovan
committed
gopls/internal/analysis/modernize: sort.Slice -> slices.Sort
This CL adds a modernizer for calls to sort.Slice, where the less function is the natural order: sort.Slice(s, func(i, j int) bool { return s[i] < s[j] }) => slices.Sort(s) (Ditto SliceStable -> SortStable.) The foldingrange test was tweaked to avoid triggering this fix. Updates golang/go#70815 Change-Id: I755351c77aab8ddf7f21369c0f980e8d3334fc01 Reviewed-on: https://go-review.googlesource.com/c/tools/+/635681 Reviewed-by: Robert Findley <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Alan Donovan <[email protected]>
1 parent 3cad859 commit 93cc684

File tree

11 files changed

+235
-17
lines changed

11 files changed

+235
-17
lines changed

‎go/analysis/analysistest/analysistest.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@ func Run(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Res
352352
testenv.NeedsGoPackages(t)
353353
}
354354

355-
pkgs, err := loadPackages(a, dir, patterns...)
355+
pkgs, err := loadPackages(dir, patterns...)
356356
if err != nil {
357357
t.Errorf("loading %s: %v", patterns, err)
358358
return nil
@@ -433,7 +433,7 @@ type Result struct {
433433
// dependencies) from dir, which is the root of a GOPATH-style project tree.
434434
// loadPackages returns an error if any package had an error, or the pattern
435435
// matched no packages.
436-
func loadPackages(a *analysis.Analyzer, dir string, patterns ...string) ([]*packages.Package, error) {
436+
func loadPackages(dir string, patterns ...string) ([]*packages.Package, error) {
437437
env := []string{"GOPATH=" + dir, "GO111MODULE=off", "GOWORK=off"} // GOPATH mode
438438

439439
// Undocumented module mode. Will be replaced by something better.

‎gopls/doc/analyzers.md

+2
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,8 @@ existing code by using more modern features of Go, such as:
445445

446446
- replacing if/else conditional assignments by a call to the
447447
built-in min or max functions.
448+
- replacing sort.Slice(x, func(i, j int) bool) { return s[i] < s[j] }
449+
by slices.Sort(s).
448450

449451
Default: on.
450452

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

+2
Original file line numberDiff line numberDiff line change
@@ -13,4 +13,6 @@
1313
//
1414
// - replacing if/else conditional assignments by a call to the
1515
// built-in min or max functions.
16+
// - replacing sort.Slice(x, func(i, j int) bool) { return s[i] < s[j] }
17+
// by slices.Sort(s).
1618
package modernize

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

+16
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"go/ast"
1010
"go/format"
1111
"go/token"
12+
"go/types"
1213
"strings"
1314

1415
"golang.org/x/tools/go/analysis"
@@ -30,12 +31,27 @@ var Analyzer = &analysis.Analyzer{
3031

3132
func run(pass *analysis.Pass) (any, error) {
3233
minmax(pass)
34+
sortslice(pass)
35+
3336
// TODO(adonovan): more modernizers here; see #70815.
37+
// Consider interleaving passes with the same inspection
38+
// criteria (e.g. CallExpr).
39+
3440
return nil, nil
3541
}
3642

3743
// -- helpers --
3844

45+
// TODO(adonovan): factor with analysisutil.Imports.
46+
func _imports(pkg *types.Package, path string) bool {
47+
for _, imp := range pkg.Imports() {
48+
if imp.Path() == path {
49+
return true
50+
}
51+
}
52+
return false
53+
}
54+
3955
// equalSyntax reports whether x and y are syntactically equal (ignoring comments).
4056
func equalSyntax(x, y ast.Expr) bool {
4157
sameName := func(x, y *ast.Ident) bool { return x.Name == y.Name }

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -13,5 +13,6 @@ import (
1313

1414
func Test(t *testing.T) {
1515
analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), modernize.Analyzer,
16-
"minmax")
16+
"minmax",
17+
"sortslice")
1718
}

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

+137
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
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+
"fmt"
9+
"go/ast"
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+
"golang.org/x/tools/gopls/internal/util/astutil"
17+
"golang.org/x/tools/internal/analysisinternal"
18+
)
19+
20+
// The sortslice pass replaces sort.Slice(slice, less) with
21+
// slices.Sort(slice) when slice is a []T and less is a FuncLit
22+
// equivalent to cmp.Ordered[T].
23+
//
24+
// sort.Slice(s, func(i, j int) bool { return s[i] < s[j] })
25+
// => slices.Sort(s)
26+
//
27+
// It also supports the SliceStable variant.
28+
//
29+
// TODO(adonovan): support
30+
//
31+
// - sort.Slice(s, func(i, j int) bool { return s[i] ... s[j] })
32+
// -> slices.SortFunc(s, func(x, y int) bool { return x ... y })
33+
// iff all uses of i, j can be replaced by s[i], s[j].
34+
//
35+
// - sort.Sort(x) where x has a named slice type whose Less method is the natural order.
36+
// -> sort.Slice(x)
37+
func sortslice(pass *analysis.Pass) {
38+
if !_imports(pass.Pkg, "sort") {
39+
return
40+
}
41+
42+
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
43+
info := pass.TypesInfo
44+
for call := range inspector.All[*ast.CallExpr](inspect) {
45+
// call to sort.Slice{,Stable}?
46+
var stable string
47+
if isQualifiedIdent(info, call.Fun, "sort", "Slice") {
48+
} else if isQualifiedIdent(info, call.Fun, "sort", "SliceStable") {
49+
stable = "Stable"
50+
} else {
51+
continue
52+
}
53+
54+
if lit, ok := call.Args[1].(*ast.FuncLit); ok && len(lit.Body.List) == 1 {
55+
sig := info.Types[lit.Type].Type.(*types.Signature)
56+
57+
// Have: sort.Slice(s, func(i, j int) bool { return ... })
58+
s := call.Args[0]
59+
i := sig.Params().At(0)
60+
j := sig.Params().At(1)
61+
62+
ret := lit.Body.List[0].(*ast.ReturnStmt)
63+
if compare, ok := ret.Results[0].(*ast.BinaryExpr); ok && compare.Op == token.LSS {
64+
// isIndex reports whether e is s[v].
65+
isIndex := func(e ast.Expr, v *types.Var) bool {
66+
index, ok := e.(*ast.IndexExpr)
67+
return ok &&
68+
equalSyntax(index.X, s) &&
69+
is[*ast.Ident](index.Index) &&
70+
info.Uses[index.Index.(*ast.Ident)] == v
71+
}
72+
if isIndex(compare.X, i) && isIndex(compare.Y, j) {
73+
// Have: sort.Slice(s, func(i, j int) bool { return s[i] < s[j] })
74+
75+
file := enclosingFile(pass, call.Pos())
76+
slicesName, importEdits := analysisinternal.AddImport(pass.TypesInfo, file, call.Pos(), "slices", "slices")
77+
78+
pass.Report(analysis.Diagnostic{
79+
// Highlight "sort.Slice".
80+
Pos: call.Fun.Pos(),
81+
End: call.Fun.End(),
82+
Category: "sortslice",
83+
Message: fmt.Sprintf("sort.Slice%[1]s can be modernized using slices.Sort%[1]s", stable),
84+
SuggestedFixes: []analysis.SuggestedFix{{
85+
Message: fmt.Sprintf("Replace sort.Slice%[1]s call by slices.Sort%[1]s", stable),
86+
TextEdits: append(importEdits, []analysis.TextEdit{
87+
{
88+
// Replace sort.Slice with slices.Sort.
89+
Pos: call.Fun.Pos(),
90+
End: call.Fun.End(),
91+
NewText: []byte(slicesName + ".Sort" + stable),
92+
},
93+
{
94+
// Eliminate FuncLit.
95+
Pos: call.Args[0].End(),
96+
End: call.Rparen,
97+
},
98+
}...),
99+
}},
100+
})
101+
}
102+
}
103+
}
104+
}
105+
}
106+
107+
// isQualifiedIdent reports whether e is a reference to pkg.Name.
108+
func isQualifiedIdent(info *types.Info, e ast.Expr, pkgpath, name string) bool {
109+
var id *ast.Ident
110+
switch e := e.(type) {
111+
case *ast.Ident:
112+
id = e // e.g. dot import
113+
case *ast.SelectorExpr:
114+
id = e.Sel
115+
default:
116+
return false
117+
}
118+
obj, ok := info.Uses[id]
119+
return ok && isPackageLevel(obj) && obj.Pkg().Path() == pkgpath && id.Name == name
120+
}
121+
122+
// isPackageLevel reports whether obj is a package-level func/var/const/type.
123+
func isPackageLevel(obj types.Object) bool {
124+
pkg := obj.Pkg()
125+
return pkg != nil && obj.Parent() == pkg.Scope()
126+
}
127+
128+
// enclosingFile returns the file enclosing pos.
129+
// (It walks over the list of files, so is not terribly efficient.)
130+
func enclosingFile(pass *analysis.Pass, pos token.Pos) *ast.File {
131+
for _, file := range pass.Files {
132+
if astutil.NodeContains(file, pos) {
133+
return file
134+
}
135+
}
136+
return nil
137+
}

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

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
package sortslice
2+
3+
import "sort"
4+
5+
type myint int
6+
7+
func _(s []myint) {
8+
sort.Slice(s, func(i, j int) bool { return s[i] < s[j] }) // want "sort.Slice can be modernized using slices.Sort"
9+
10+
sort.SliceStable(s, func(i, j int) bool { return s[i] < s[j] }) // want "sort.SliceStable can be modernized using slices.SortStable"
11+
}
12+
13+
func _(x *struct{ s []int }) {
14+
sort.Slice(x.s, func(first, second int) bool { return x.s[first] < x.s[second] }) // want "sort.Slice can be modernized using slices.Sort"
15+
}
16+
17+
func _(s []int) {
18+
sort.Slice(s, func(i, j int) bool { return s[i] > s[j] }) // nope: wrong comparison operator
19+
}
20+
21+
func _(s []int) {
22+
sort.Slice(s, func(i, j int) bool { return s[j] < s[i] }) // nope: wrong index var
23+
}
24+
25+
func _(s2 []struct{ x int }) {
26+
sort.Slice(s2, func(i, j int) bool { return s2[i].x < s2[j].x }) // nope: not a simple index operation
27+
}

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

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package sortslice
2+
3+
import "slices"
4+
5+
import "slices"
6+
7+
import "slices"
8+
9+
import "sort"
10+
11+
type myint int
12+
13+
func _(s []myint) {
14+
slices.Sort(s) // want "sort.Slice can be modernized using slices.Sort"
15+
16+
slices.SortStable(s) // want "sort.SliceStable can be modernized using slices.SortStable"
17+
}
18+
19+
func _(x *struct{ s []int }) {
20+
slices.Sort(x.s) // want "sort.Slice can be modernized using slices.Sort"
21+
}
22+
23+
func _(s []int) {
24+
sort.Slice(s, func(i, j int) bool { return s[i] > s[j] }) // nope: wrong comparison operator
25+
}
26+
27+
func _(s []int) {
28+
sort.Slice(s, func(i, j int) bool { return s[j] < s[i] }) // nope: wrong index var
29+
}
30+
31+
func _(s2 []struct{ x int }) {
32+
sort.Slice(s2, func(i, j int) bool { return s2[i].x < s2[j].x }) // nope: not a simple index operation
33+
}

‎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.",
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.\n - replacing sort.Slice(x, func(i, j int) bool) { return s[i] \u003c s[j] }\n by slices.Sort(s).",
471471
"Default": "true"
472472
},
473473
{
@@ -1138,7 +1138,7 @@
11381138
},
11391139
{
11401140
"Name": "modernize",
1141-
"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.",
1141+
"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.\n - replacing sort.Slice(x, func(i, j int) bool) { return s[i] \u003c s[j] }\n by slices.Sort(s).",
11421142
"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize",
11431143
"Default": true
11441144
},

‎gopls/internal/test/marker/testdata/foldingrange/a.txt

+6-6
Original file line numberDiff line numberDiff line change
@@ -85,15 +85,15 @@ func _() {
8585
slice := []int{1, 2, 3}
8686
sort.Slice(slice, func(i, j int) bool {
8787
a, b := slice[i], slice[j]
88-
return a < b
88+
return a > b
8989
})
9090

91-
sort.Slice(slice, func(i, j int) bool { return slice[i] < slice[j] })
91+
sort.Slice(slice, func(i, j int) bool { return slice[i] > slice[j] })
9292

9393
sort.Slice(
9494
slice,
9595
func(i, j int) bool {
96-
return slice[i] < slice[j]
96+
return slice[i] > slice[j]
9797
},
9898
)
9999

@@ -216,15 +216,15 @@ func _() {<35 kind="">
216216
slice := []int{<36 kind="">1, 2, 3</36>}
217217
sort.Slice(<37 kind="">slice, func(<38 kind="">i, j int</38>) bool {<39 kind="">
218218
a, b := slice[i], slice[j]
219-
return a < b
219+
return a > b
220220
</39>}</37>)
221221

222-
sort.Slice(<40 kind="">slice, func(<41 kind="">i, j int</41>) bool {<42 kind=""> return slice[i] < slice[j] </42>}</40>)
222+
sort.Slice(<40 kind="">slice, func(<41 kind="">i, j int</41>) bool {<42 kind=""> return slice[i] > slice[j] </42>}</40>)
223223

224224
sort.Slice(<43 kind="">
225225
slice,
226226
func(<44 kind="">i, j int</44>) bool {<45 kind="">
227-
return slice[i] < slice[j]
227+
return slice[i] > slice[j]
228228
</45>},
229229
</43>)
230230

‎gopls/internal/test/marker/testdata/foldingrange/a_lineonly.txt

+6-6
Original file line numberDiff line numberDiff line change
@@ -92,15 +92,15 @@ func _() {
9292
slice := []int{1, 2, 3}
9393
sort.Slice(slice, func(i, j int) bool {
9494
a, b := slice[i], slice[j]
95-
return a < b
95+
return a > b
9696
})
9797

98-
sort.Slice(slice, func(i, j int) bool { return slice[i] < slice[j] })
98+
sort.Slice(slice, func(i, j int) bool { return slice[i] > slice[j] })
9999

100100
sort.Slice(
101101
slice,
102102
func(i, j int) bool {
103-
return slice[i] < slice[j]
103+
return slice[i] > slice[j]
104104
},
105105
)
106106

@@ -221,15 +221,15 @@ func _() {<23 kind="">
221221
slice := []int{1, 2, 3}
222222
sort.Slice(slice, func(i, j int) bool {<24 kind="">
223223
a, b := slice[i], slice[j]
224-
return a < b</24>
224+
return a > b</24>
225225
})
226226

227-
sort.Slice(slice, func(i, j int) bool { return slice[i] < slice[j] })
227+
sort.Slice(slice, func(i, j int) bool { return slice[i] > slice[j] })
228228

229229
sort.Slice(<25 kind="">
230230
slice,
231231
func(i, j int) bool {<26 kind="">
232-
return slice[i] < slice[j]</26>
232+
return slice[i] > slice[j]</26>
233233
},</25>
234234
)
235235

0 commit comments

Comments
 (0)