Skip to content

Commit c437d40

Browse files
committed
gopls/internal/analysis/modernize: for...{m[k]=v} -> maps.Collect et al
This CL adds a "mapsloop" modernizer pass that replaces a loop whose body consists only of a map update m[k]=v by a call to one of the four functions in Go 1.23's maps package: maps.Copy(m, x) (x is map) maps.Insert(m, x) (x is iter.Seq2) m = maps.Clone(x) (x is map, m is a new map) m = maps.Collect(x) (x is iter.Seq2, m is a new map) Updates golang/go#70815 Change-Id: Ia64066c29394cc3233b44202caea97736cf912d4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/638875 LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Alan Donovan <[email protected]> Reviewed-by: Robert Findley <[email protected]>
1 parent a039694 commit c437d40

File tree

9 files changed

+500
-15
lines changed

9 files changed

+500
-15
lines changed

gopls/doc/analyzers.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -443,13 +443,16 @@ Package documentation: [lostcancel](https://pkg.go.dev/golang.org/x/tools/go/ana
443443
This analyzer reports opportunities for simplifying and clarifying
444444
existing code by using more modern features of Go, such as:
445445

446-
- replacing if/else conditional assignments by a call to the
446+
- replacing an if/else conditional assignment by a call to the
447447
built-in min or max functions added in go1.21;
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;
451451
- replacing append([]T(nil), s...) by slices.Clone(s) or
452452
slices.Concat(s), added in go1.21;
453+
- replacing a loop around an m[k]=v map update by a call
454+
to one of the Collect, Copy, Clone, or Insert functions
455+
from the maps package, added in go1.21;
453456

454457
Default: on.
455458

gopls/internal/analysis/modernize/doc.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,14 @@
1111
// This analyzer reports opportunities for simplifying and clarifying
1212
// existing code by using more modern features of Go, such as:
1313
//
14-
// - replacing if/else conditional assignments by a call to the
14+
// - replacing an if/else conditional assignment by a call to the
1515
// built-in min or max functions added in go1.21;
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;
1919
// - replacing append([]T(nil), s...) by slices.Clone(s) or
2020
// slices.Concat(s), added in go1.21;
21+
// - replacing a loop around an m[k]=v map update by a call
22+
// to one of the Collect, Copy, Clone, or Insert functions
23+
// from the maps package, added in go1.21;
2124
package modernize
Lines changed: 222 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,222 @@
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 "maps" package.
8+
9+
import (
10+
"fmt"
11+
"go/ast"
12+
"go/token"
13+
"go/types"
14+
15+
"golang.org/x/tools/go/analysis"
16+
"golang.org/x/tools/go/analysis/passes/inspect"
17+
"golang.org/x/tools/go/ast/inspector"
18+
"golang.org/x/tools/internal/analysisinternal"
19+
"golang.org/x/tools/internal/astutil/cursor"
20+
"golang.org/x/tools/internal/typeparams"
21+
"golang.org/x/tools/internal/versions"
22+
)
23+
24+
// The mapsloop pass offers to simplify a loop of map insertions:
25+
//
26+
// for k, v := range x {
27+
// m[k] = v
28+
// }
29+
//
30+
// by a call to go1.23's maps package. There are four variants, the
31+
// product of two axes: whether the source x is a map or an iter.Seq2,
32+
// and whether the destination m is a newly created map:
33+
//
34+
// maps.Copy(m, x) (x is map)
35+
// maps.Insert(m, x) (x is iter.Seq2)
36+
// m = maps.Clone(x) (x is map, m is a new map)
37+
// m = maps.Collect(x) (x is iter.Seq2, m is a new map)
38+
//
39+
// A map is newly created if the preceding statement has one of these
40+
// forms, where M is a map type:
41+
//
42+
// m = make(M)
43+
// m = M{}
44+
func mapsloop(pass *analysis.Pass) {
45+
if pass.Pkg.Path() == "maps " {
46+
return
47+
}
48+
49+
info := pass.TypesInfo
50+
51+
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
52+
for cur := range cursor.Root(inspect).Preorder((*ast.RangeStmt)(nil)) {
53+
n := cur.Node().(*ast.RangeStmt)
54+
if n.Tok == token.DEFINE && n.Key != nil && n.Value != nil && len(n.Body.List) == 1 {
55+
56+
// Have: for k, v := range x { S }
57+
if assign, ok := n.Body.List[0].(*ast.AssignStmt); ok && len(assign.Lhs) == 1 {
58+
if index, ok := assign.Lhs[0].(*ast.IndexExpr); ok &&
59+
equalSyntax(n.Key, index.Index) &&
60+
equalSyntax(n.Value, assign.Rhs[0]) {
61+
62+
// Have: for k, v := range x { m[k] = v }
63+
var (
64+
x = n.X
65+
m = index.X
66+
)
67+
68+
if !is[*types.Map](typeparams.CoreType(info.TypeOf(m))) {
69+
continue // m is not a map
70+
}
71+
72+
// Check file version.
73+
file := enclosingFile(pass, n.Pos())
74+
if versions.Before(info.FileVersions[file], "go1.23") {
75+
continue
76+
}
77+
78+
// Is x a map or iter.Seq2?
79+
tx := types.Unalias(info.TypeOf(x))
80+
var xmap bool
81+
switch typeparams.CoreType(tx).(type) {
82+
case *types.Map:
83+
xmap = true
84+
85+
case *types.Signature:
86+
k, v, ok := assignableToIterSeq2(tx)
87+
if !ok {
88+
continue // a named isomer of Seq2
89+
}
90+
xmap = false
91+
92+
// Record in tx the unnamed map[K]V type
93+
// derived from the yield function.
94+
// This is the type of maps.Collect(x).
95+
tx = types.NewMap(k, v)
96+
97+
default:
98+
continue // e.g. slice, channel (or no core type!)
99+
}
100+
101+
// Is the preceding statement of the form
102+
// m = make(M) or M{}
103+
// and can we replace its RHS with slices.{Clone,Collect}?
104+
var mrhs ast.Expr // make(M) or M{}, or nil
105+
if curPrev, ok := cur.PrevSibling(); ok {
106+
if assign, ok := curPrev.Node().(*ast.AssignStmt); ok &&
107+
len(assign.Lhs) == 1 &&
108+
len(assign.Rhs) == 1 &&
109+
equalSyntax(assign.Lhs[0], m) {
110+
111+
// Have: m = rhs; for k, v := range x { m[k] = v }
112+
var newMap bool
113+
rhs := assign.Rhs[0]
114+
switch rhs := rhs.(type) {
115+
case *ast.CallExpr:
116+
if id, ok := rhs.Fun.(*ast.Ident); ok &&
117+
info.Uses[id] == builtinMake {
118+
// Have: m = make(...)
119+
newMap = true
120+
}
121+
case *ast.CompositeLit:
122+
if len(rhs.Elts) == 0 {
123+
// Have m = M{}
124+
newMap = true
125+
}
126+
}
127+
128+
// Take care not to change type of m's RHS expression.
129+
if newMap {
130+
trhs := info.TypeOf(rhs)
131+
132+
// Inv: tx is the type of maps.F(x)
133+
// - maps.Clone(x) has the same type as x.
134+
// - maps.Collect(x) returns an unnamed map type.
135+
136+
if assign.Tok == token.DEFINE {
137+
// DEFINE (:=): we must not
138+
// change the type of RHS.
139+
if types.Identical(tx, trhs) {
140+
mrhs = rhs
141+
}
142+
} else {
143+
// ASSIGN (=): the types of LHS
144+
// and RHS may differ in namedness.
145+
if types.AssignableTo(tx, trhs) {
146+
mrhs = rhs
147+
}
148+
}
149+
}
150+
}
151+
}
152+
153+
// Choose function, report diagnostic, and suggest fix.
154+
mapsName, importEdits := analysisinternal.AddImport(info, file, n.Pos(), "maps", "maps")
155+
var (
156+
funcName string
157+
newText []byte
158+
start, end token.Pos
159+
)
160+
if mrhs != nil {
161+
// Replace RHS of preceding m=... assignment (and loop) with expression.
162+
start, end = mrhs.Pos(), n.End()
163+
funcName = cond(xmap, "Clone", "Collect")
164+
newText = fmt.Appendf(nil, "%s.%s(%s)",
165+
mapsName,
166+
funcName,
167+
formatNode(pass.Fset, x))
168+
} else {
169+
// Replace loop with call statement.
170+
start, end = n.Pos(), n.End()
171+
funcName = cond(xmap, "Copy", "Insert")
172+
newText = fmt.Appendf(nil, "%s.%s(%s, %s)",
173+
mapsName,
174+
funcName,
175+
formatNode(pass.Fset, m),
176+
formatNode(pass.Fset, x))
177+
}
178+
pass.Report(analysis.Diagnostic{
179+
Pos: assign.Lhs[0].Pos(),
180+
End: assign.Lhs[0].End(),
181+
Category: "mapsloop",
182+
Message: "Replace m[k]=v loop with maps." + funcName,
183+
SuggestedFixes: []analysis.SuggestedFix{{
184+
Message: "Replace m[k]=v loop with maps." + funcName,
185+
TextEdits: append(importEdits, []analysis.TextEdit{{
186+
Pos: start,
187+
End: end,
188+
NewText: newText,
189+
}}...),
190+
}},
191+
})
192+
}
193+
}
194+
}
195+
}
196+
}
197+
198+
// assignableToIterSeq2 reports whether t is assignable to
199+
// iter.Seq[K, V] and returns K and V if so.
200+
func assignableToIterSeq2(t types.Type) (k, v types.Type, ok bool) {
201+
// The only named type assignable to iter.Seq2 is iter.Seq2.
202+
if named, isNamed := t.(*types.Named); isNamed {
203+
if !isPackageLevel(named.Obj(), "iter", "Seq2") {
204+
return
205+
}
206+
t = t.Underlying()
207+
}
208+
209+
if t, ok := t.(*types.Signature); ok {
210+
// func(yield func(K, V) bool)?
211+
if t.Params().Len() == 1 && t.Results().Len() == 0 {
212+
if yield, ok := t.Params().At(0).Type().(*types.Signature); ok { // sic, no Underlying/CoreType
213+
if yield.Params().Len() == 2 &&
214+
yield.Results().Len() == 1 &&
215+
types.Identical(yield.Results().At(0).Type(), builtinBool.Type()) {
216+
return yield.Params().At(0).Type(), yield.Params().At(1).Type(), true
217+
}
218+
}
219+
}
220+
}
221+
return
222+
}

gopls/internal/analysis/modernize/modernize.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,12 @@ func run(pass *analysis.Pass) (any, error) {
5353
}
5454
}
5555

56-
minmax(pass)
57-
sortslice(pass)
58-
efaceany(pass)
5956
appendclipped(pass)
6057
bloop(pass)
58+
efaceany(pass)
59+
mapsloop(pass)
60+
minmax(pass)
61+
sortslice(pass)
6162

6263
// TODO(adonovan):
6364
// - more modernizers here; see #70815.
@@ -122,3 +123,10 @@ func isPackageLevel(obj types.Object, pkgpath, name string) bool {
122123
obj.Pkg().Path() == pkgpath &&
123124
obj.Name() == name
124125
}
126+
127+
var (
128+
builtinAppend = types.Universe.Lookup("append")
129+
builtinBool = types.Universe.Lookup("bool")
130+
builtinMake = types.Universe.Lookup("make")
131+
builtinNil = types.Universe.Lookup("nil")
132+
)

gopls/internal/analysis/modernize/modernize_test.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,11 @@ import (
1313

1414
func Test(t *testing.T) {
1515
analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), modernize.Analyzer,
16+
"appendclipped",
17+
"bloop",
18+
"efaceany",
19+
"mapsloop",
1620
"minmax",
1721
"sortslice",
18-
"efaceany",
19-
"appendclipped",
20-
"bloop")
22+
)
2123
}

gopls/internal/analysis/modernize/slices.go

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -221,8 +221,3 @@ func isClippedSlice(info *types.Info, e ast.Expr) (clipped, empty bool) {
221221
}
222222
return false, false
223223
}
224-
225-
var (
226-
builtinAppend = types.Universe.Lookup("append")
227-
builtinNil = types.Universe.Lookup("nil")
228-
)

0 commit comments

Comments
 (0)