Skip to content

Commit

Permalink
gopls/internal/analysis/modernize: strings.Fields -> FieldsSeq
Browse files Browse the repository at this point in the history
This CL enhances the existing modernizer to support calls to strings.Fields and
bytes.Fields, that offers a fix to instead use go1.24's FieldsSeq,
which avoids allocating an array.

Fixes golang/go#72033

Change-Id: I2059f66f38a639c5a264b650137ced7b4f84550e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/654535
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Junyang Shao <[email protected]>
  • Loading branch information
xieyuschen authored and gopherbot committed Mar 5, 2025
1 parent 8d38122 commit 0721940
Show file tree
Hide file tree
Showing 9 changed files with 111 additions and 16 deletions.
2 changes: 1 addition & 1 deletion gopls/doc/analyzers.md
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ existing code by using more modern features of Go, such as:
- replacing a 3-clause for i := 0; i < n; i++ {} loop by
for i := range n {}, added in go1.22;
- replacing Split in "for range strings.Split(...)" by go1.24's
more efficient SplitSeq;
more efficient SplitSeq, or Fields with FieldSeq;

To apply all modernization fixes en masse, you can use the
following command:
Expand Down
2 changes: 1 addition & 1 deletion gopls/internal/analysis/modernize/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
// - replacing a 3-clause for i := 0; i < n; i++ {} loop by
// for i := range n {}, added in go1.22;
// - replacing Split in "for range strings.Split(...)" by go1.24's
// more efficient SplitSeq;
// more efficient SplitSeq, or Fields with FieldSeq;
//
// To apply all modernization fixes en masse, you can use the
// following command:
Expand Down
2 changes: 1 addition & 1 deletion gopls/internal/analysis/modernize/modernize.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func run(pass *analysis.Pass) (any, error) {
rangeint(pass)
slicescontains(pass)
slicesdelete(pass)
splitseq(pass)
stringsseq(pass)
sortslice(pass)
testingContext(pass)

Expand Down
1 change: 1 addition & 0 deletions gopls/internal/analysis/modernize/modernize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ func Test(t *testing.T) {
"slicescontains",
"slicesdelete",
"splitseq",
"fieldsseq",
"sortslice",
"testingcontext",
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package modernize

import (
"fmt"
"go/ast"
"go/token"
"go/types"
Expand All @@ -17,8 +18,9 @@ import (
"golang.org/x/tools/internal/astutil/edge"
)

// splitseq offers a fix to replace a call to strings.Split with
// SplitSeq when it is the operand of a range loop, either directly:
// stringsseq offers a fix to replace a call to strings.Split with
// SplitSeq or strings.Fields with FieldsSeq
// when it is the operand of a range loop, either directly:
//
// for _, line := range strings.Split() {...}
//
Expand All @@ -29,7 +31,8 @@ import (
//
// Variants:
// - bytes.SplitSeq
func splitseq(pass *analysis.Pass) {
// - bytes.FieldsSeq
func stringsseq(pass *analysis.Pass) {
if !analysisinternal.Imports(pass.Pkg, "strings") &&
!analysisinternal.Imports(pass.Pkg, "bytes") {
return
Expand Down Expand Up @@ -88,21 +91,27 @@ func splitseq(pass *analysis.Pass) {
})
}

if sel, ok := call.Fun.(*ast.SelectorExpr); ok &&
(analysisinternal.IsFunctionNamed(typeutil.Callee(info, call), "strings", "Split") ||
analysisinternal.IsFunctionNamed(typeutil.Callee(info, call), "bytes", "Split")) {
sel, ok := call.Fun.(*ast.SelectorExpr)
if !ok {
continue
}

obj := typeutil.Callee(info, call)
if analysisinternal.IsFunctionNamed(obj, "strings", "Split", "Fields") ||
analysisinternal.IsFunctionNamed(obj, "bytes", "Split", "Fields") {
oldFnName := obj.Name()
seqFnName := fmt.Sprintf("%sSeq", oldFnName)
pass.Report(analysis.Diagnostic{
Pos: sel.Pos(),
End: sel.End(),
Category: "splitseq",
Message: "Ranging over SplitSeq is more efficient",
Category: "stringsseq",
Message: fmt.Sprintf("Ranging over %s is more efficient", seqFnName),
SuggestedFixes: []analysis.SuggestedFix{{
Message: "Replace Split with SplitSeq",
Message: fmt.Sprintf("Replace %s with %s", oldFnName, seqFnName),
TextEdits: append(edits, analysis.TextEdit{
// Split -> SplitSeq
Pos: sel.Sel.Pos(),
End: sel.Sel.End(),
NewText: []byte("SplitSeq")}),
NewText: []byte(seqFnName)}),
}},
})
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
//go:build go1.24

package fieldsseq

import (
"bytes"
"strings"
)

func _() {
for _, line := range strings.Fields("") { // want "Ranging over FieldsSeq is more efficient"
println(line)
}
for i, line := range strings.Fields("") { // nope: uses index var
println(i, line)
}
for i, _ := range strings.Fields("") { // nope: uses index var
println(i)
}
for i := range strings.Fields("") { // nope: uses index var
println(i)
}
for _ = range strings.Fields("") { // want "Ranging over FieldsSeq is more efficient"
}
for range strings.Fields("") { // want "Ranging over FieldsSeq is more efficient"
}
for range bytes.Fields(nil) { // want "Ranging over FieldsSeq is more efficient"
}
{
lines := strings.Fields("") // want "Ranging over FieldsSeq is more efficient"
for _, line := range lines {
println(line)
}
}
{
lines := strings.Fields("") // nope: lines is used not just by range
for _, line := range lines {
println(line)
}
println(lines)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
//go:build go1.24

package fieldsseq

import (
"bytes"
"strings"
)

func _() {
for line := range strings.FieldsSeq("") { // want "Ranging over FieldsSeq is more efficient"
println(line)
}
for i, line := range strings.Fields( "") { // nope: uses index var
println(i, line)
}
for i, _ := range strings.Fields( "") { // nope: uses index var
println(i)
}
for i := range strings.Fields( "") { // nope: uses index var
println(i)
}
for range strings.FieldsSeq("") { // want "Ranging over FieldsSeq is more efficient"
}
for range strings.FieldsSeq("") { // want "Ranging over FieldsSeq is more efficient"
}
for range bytes.FieldsSeq(nil) { // want "Ranging over FieldsSeq is more efficient"
}
{
lines := strings.FieldsSeq("") // want "Ranging over FieldsSeq is more efficient"
for line := range lines {
println(line)
}
}
{
lines := strings.Fields( "") // nope: lines is used not just by range
for _, line := range lines {
println(line)
}
println(lines)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package fieldsseq
4 changes: 2 additions & 2 deletions gopls/internal/doc/api.json
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,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 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;\n - replacing uses of context.WithCancel in tests with t.Context, added in\n go1.24;\n - replacing omitempty by omitzero on structs, added in go1.24;\n - replacing append(s[:i], s[i+1]...) by slices.Delete(s, i, i+1),\n added in go1.21\n - replacing a 3-clause for i := 0; i \u003c n; i++ {} loop by\n for i := range n {}, added in go1.22;\n - replacing Split in \"for range strings.Split(...)\" by go1.24's\n more efficient SplitSeq;\n\nTo apply all modernization fixes en masse, you can use the\nfollowing command:\n\n\t$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -test ./...\n\nIf the tool warns of conflicting fixes, you may need to run it more\nthan once until it has applied all fixes cleanly. This command is\nnot an officially supported interface and may change in the future.",
"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;\n - replacing uses of context.WithCancel in tests with t.Context, added in\n go1.24;\n - replacing omitempty by omitzero on structs, added in go1.24;\n - replacing append(s[:i], s[i+1]...) by slices.Delete(s, i, i+1),\n added in go1.21\n - replacing a 3-clause for i := 0; i \u003c n; i++ {} loop by\n for i := range n {}, added in go1.22;\n - replacing Split in \"for range strings.Split(...)\" by go1.24's\n more efficient SplitSeq, or Fields with FieldSeq;\n\nTo apply all modernization fixes en masse, you can use the\nfollowing command:\n\n\t$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -test ./...\n\nIf the tool warns of conflicting fixes, you may need to run it more\nthan once until it has applied all fixes cleanly. This command is\nnot an officially supported interface and may change in the future.",
"Default": "true"
},
{
Expand Down Expand Up @@ -1228,7 +1228,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 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;\n - replacing uses of context.WithCancel in tests with t.Context, added in\n go1.24;\n - replacing omitempty by omitzero on structs, added in go1.24;\n - replacing append(s[:i], s[i+1]...) by slices.Delete(s, i, i+1),\n added in go1.21\n - replacing a 3-clause for i := 0; i \u003c n; i++ {} loop by\n for i := range n {}, added in go1.22;\n - replacing Split in \"for range strings.Split(...)\" by go1.24's\n more efficient SplitSeq;\n\nTo apply all modernization fixes en masse, you can use the\nfollowing command:\n\n\t$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -test ./...\n\nIf the tool warns of conflicting fixes, you may need to run it more\nthan once until it has applied all fixes cleanly. This command is\nnot an officially supported interface and may change in the future.",
"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;\n - replacing uses of context.WithCancel in tests with t.Context, added in\n go1.24;\n - replacing omitempty by omitzero on structs, added in go1.24;\n - replacing append(s[:i], s[i+1]...) by slices.Delete(s, i, i+1),\n added in go1.21\n - replacing a 3-clause for i := 0; i \u003c n; i++ {} loop by\n for i := range n {}, added in go1.22;\n - replacing Split in \"for range strings.Split(...)\" by go1.24's\n more efficient SplitSeq, or Fields with FieldSeq;\n\nTo apply all modernization fixes en masse, you can use the\nfollowing command:\n\n\t$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -test ./...\n\nIf the tool warns of conflicting fixes, you may need to run it more\nthan once until it has applied all fixes cleanly. This command is\nnot an officially supported interface and may change in the future.",
"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize",
"Default": true
},
Expand Down

0 comments on commit 0721940

Please sign in to comment.