Skip to content

Commit f202b36

Browse files
madelinekalilmadelinekalil
committed
gopls/internal/analysis/modernize: string formatting
Adds a new modernizer that suggests replacing instances of []byte(fmt.Sprintf(...)) with fmt.Appendf(nil, ...) Accounts for the use of aliases of type byte and []byte Example: []byte(fmt.Sprintf("test %d", 1)) -> fmt.Appendf(nil, "test %d", 1) Updates golang/go#70815 Change-Id: I3aac83a028894a3b099b15b74886f8af4a763f9c Reviewed-on: https://go-review.googlesource.com/c/tools/+/638436 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
1 parent d6cc3cd commit f202b36

File tree

10 files changed

+182
-26
lines changed

10 files changed

+182
-26
lines changed

‎gopls/doc/analyzers.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -453,6 +453,8 @@ existing code by using more modern features of Go, such as:
453453
- replacing a loop around an m[k]=v map update by a call
454454
to one of the Collect, Copy, Clone, or Insert functions
455455
from the maps package, added in go1.21;
456+
- replacing []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),
457+
added in go1.19;
456458

457459
Default: on.
458460

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,6 @@
2121
// - replacing a loop around an m[k]=v map update by a call
2222
// to one of the Collect, Copy, Clone, or Insert functions
2323
// from the maps package, added in go1.21;
24+
// - replacing []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),
25+
// added in go1.19;
2426
package modernize

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

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
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/types"
10+
11+
"golang.org/x/tools/go/analysis"
12+
"golang.org/x/tools/go/analysis/passes/inspect"
13+
"golang.org/x/tools/go/ast/inspector"
14+
)
15+
16+
// The fmtappend function replaces []byte(fmt.Sprintf(...)) by
17+
// fmt.Appendf(nil, ...).
18+
func fmtappendf(pass *analysis.Pass) {
19+
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
20+
info := pass.TypesInfo
21+
for curFile := range filesUsing(inspect, info, "go1.19") {
22+
for curCallExpr := range curFile.Preorder((*ast.CallExpr)(nil)) {
23+
conv := curCallExpr.Node().(*ast.CallExpr)
24+
tv := info.Types[conv.Fun]
25+
if tv.IsType() && types.Identical(tv.Type, byteSliceType) {
26+
call, ok := conv.Args[0].(*ast.CallExpr)
27+
if ok {
28+
var appendText = ""
29+
var id *ast.Ident
30+
if id = isQualifiedIdent(info, call.Fun, "fmt", "Sprintf"); id != nil {
31+
appendText = "Appendf"
32+
} else if id = isQualifiedIdent(info, call.Fun, "fmt", "Sprint"); id != nil {
33+
appendText = "Append"
34+
} else if id = isQualifiedIdent(info, call.Fun, "fmt", "Sprintln"); id != nil {
35+
appendText = "Appendln"
36+
} else {
37+
continue
38+
}
39+
pass.Report(analysis.Diagnostic{
40+
Pos: conv.Pos(),
41+
End: conv.End(),
42+
Category: "fmtappendf",
43+
Message: "Replace []byte(fmt.Sprintf...) with fmt.Appendf",
44+
SuggestedFixes: []analysis.SuggestedFix{{
45+
Message: "Replace []byte(fmt.Sprintf...) with fmt.Appendf",
46+
TextEdits: []analysis.TextEdit{
47+
{
48+
// delete "[]byte("
49+
Pos: conv.Pos(),
50+
End: conv.Lparen + 1,
51+
},
52+
{
53+
// remove ")"
54+
Pos: conv.Rparen,
55+
End: conv.Rparen + 1,
56+
},
57+
{
58+
Pos: id.Pos(),
59+
End: id.End(),
60+
NewText: []byte(appendText), // replace Sprint with Append
61+
},
62+
{
63+
Pos: call.Lparen + 1,
64+
NewText: []byte("nil, "),
65+
},
66+
},
67+
}},
68+
})
69+
}
70+
}
71+
}
72+
}
73+
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ func run(pass *analysis.Pass) (any, error) {
6060
appendclipped(pass)
6161
bloop(pass)
6262
efaceany(pass)
63+
fmtappendf(pass)
6364
mapsloop(pass)
6465
minmax(pass)
6566
sortslice(pass)
@@ -147,4 +148,5 @@ var (
147148
builtinBool = types.Universe.Lookup("bool")
148149
builtinMake = types.Universe.Lookup("make")
149150
builtinNil = types.Universe.Lookup("nil")
151+
byteSliceType = types.NewSlice(types.Typ[types.Byte])
150152
)

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ func Test(t *testing.T) {
1616
"appendclipped",
1717
"bloop",
1818
"efaceany",
19+
"fmtappendf",
1920
"mapsloop",
2021
"minmax",
2122
"sortslice",

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

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -68,24 +68,25 @@ func appendclipped(pass *analysis.Pass) {
6868

6969
// Special case for common but redundant clone of os.Environ().
7070
// append(zerocap, os.Environ()...) -> os.Environ()
71-
if scall, ok := s.(*ast.CallExpr); ok &&
72-
isQualifiedIdent(info, scall.Fun, "os", "Environ") {
73-
74-
pass.Report(analysis.Diagnostic{
75-
Pos: call.Pos(),
76-
End: call.End(),
77-
Category: "slicesclone",
78-
Message: "Redundant clone of os.Environ()",
79-
SuggestedFixes: []analysis.SuggestedFix{{
80-
Message: "Eliminate redundant clone",
81-
TextEdits: []analysis.TextEdit{{
82-
Pos: call.Pos(),
83-
End: call.End(),
84-
NewText: formatNode(pass.Fset, s),
71+
if scall, ok := s.(*ast.CallExpr); ok {
72+
if id := isQualifiedIdent(info, scall.Fun, "os", "Environ"); id != nil {
73+
74+
pass.Report(analysis.Diagnostic{
75+
Pos: call.Pos(),
76+
End: call.End(),
77+
Category: "slicesclone",
78+
Message: "Redundant clone of os.Environ()",
79+
SuggestedFixes: []analysis.SuggestedFix{{
80+
Message: "Eliminate redundant clone",
81+
TextEdits: []analysis.TextEdit{{
82+
Pos: call.Pos(),
83+
End: call.End(),
84+
NewText: formatNode(pass.Fset, s),
85+
}},
8586
}},
86-
}},
87-
})
88-
return
87+
})
88+
return
89+
}
8990
}
9091

9192
// append(zerocap, s...) -> slices.Clone(s)
@@ -199,7 +200,7 @@ func isClippedSlice(info *types.Info, e ast.Expr) (clipped, empty bool) {
199200
}
200201

201202
// slices.Clip(x)?
202-
if isQualifiedIdent(info, e.Fun, "slices", "Clip") {
203+
if id := isQualifiedIdent(info, e.Fun, "slices", "Clip"); id != nil {
203204
return true, false
204205
}
205206

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,8 @@ func sortslice(pass *analysis.Pass) {
4343
check := func(file *ast.File, call *ast.CallExpr) {
4444
// call to sort.Slice{,Stable}?
4545
var stable string
46-
if isQualifiedIdent(info, call.Fun, "sort", "Slice") {
47-
} else if isQualifiedIdent(info, call.Fun, "sort", "SliceStable") {
46+
if isQualifiedIdent(info, call.Fun, "sort", "Slice") != nil {
47+
} else if isQualifiedIdent(info, call.Fun, "sort", "SliceStable") != nil {
4848
stable = "Stable"
4949
} else {
5050
return
@@ -112,17 +112,20 @@ func sortslice(pass *analysis.Pass) {
112112
}
113113
}
114114

115-
// isQualifiedIdent reports whether e is a reference to pkg.Name.
116-
func isQualifiedIdent(info *types.Info, e ast.Expr, pkgpath, name string) bool {
115+
// isQualifiedIdent reports whether e is a reference to pkg.Name. If so, it returns the identifier.
116+
func isQualifiedIdent(info *types.Info, e ast.Expr, pkgpath, name string) *ast.Ident {
117117
var id *ast.Ident
118118
switch e := e.(type) {
119119
case *ast.Ident:
120120
id = e // e.g. dot import
121121
case *ast.SelectorExpr:
122122
id = e.Sel
123123
default:
124-
return false
124+
return nil
125125
}
126126
obj, ok := info.Uses[id]
127-
return ok && isPackageLevel(obj, pkgpath, name)
127+
if ok && isPackageLevel(obj, pkgpath, name) {
128+
return id
129+
}
130+
return nil
128131
}

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
package fmtappendf
2+
3+
import (
4+
"fmt"
5+
)
6+
7+
func two() string {
8+
return "two"
9+
}
10+
11+
func bye() {
12+
bye := []byte(fmt.Sprintf("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf"
13+
print(bye)
14+
}
15+
16+
func funcsandvars() {
17+
one := "one"
18+
bye := []byte(fmt.Sprintf("bye %d %s %s", 1, two(), one)) // want "Replace .*Sprintf.* with fmt.Appendf"
19+
print(bye)
20+
}
21+
22+
func typealias() {
23+
type b = byte
24+
type bt = []byte
25+
bye := []b(fmt.Sprintf("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf"
26+
print(bye)
27+
bye = bt(fmt.Sprintf("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf"
28+
print(bye)
29+
}
30+
31+
func otherprints() {
32+
sprint := []byte(fmt.Sprint("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf"
33+
print(sprint)
34+
sprintln := []byte(fmt.Sprintln("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf"
35+
print(sprintln)
36+
}

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
package fmtappendf
2+
3+
import (
4+
"fmt"
5+
)
6+
7+
func two() string {
8+
return "two"
9+
}
10+
11+
func bye() {
12+
bye := fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf"
13+
print(bye)
14+
}
15+
16+
func funcsandvars() {
17+
one := "one"
18+
bye := fmt.Appendf(nil, "bye %d %s %s", 1, two(), one) // want "Replace .*Sprintf.* with fmt.Appendf"
19+
print(bye)
20+
}
21+
22+
func typealias() {
23+
type b = byte
24+
type bt = []byte
25+
bye := fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf"
26+
print(bye)
27+
bye = fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf"
28+
print(bye)
29+
}
30+
31+
func otherprints() {
32+
sprint := fmt.Append(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf"
33+
print(sprint)
34+
sprintln := fmt.Appendln(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf"
35+
print(sprintln)
36+
}

‎gopls/internal/doc/api.json

Lines changed: 2 additions & 2 deletions
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 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;",
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 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;",
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 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;",
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 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;",
11371137
"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize",
11381138
"Default": true
11391139
},

0 commit comments

Comments
 (0)