From 07219402b2fc707689574d91ee3cfd2c9a544a87 Mon Sep 17 00:00:00 2001 From: xieyuschen Date: Tue, 4 Mar 2025 19:05:13 +0800 Subject: [PATCH] gopls/internal/analysis/modernize: strings.Fields -> FieldsSeq 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 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI Reviewed-by: Junyang Shao --- gopls/doc/analyzers.md | 2 +- gopls/internal/analysis/modernize/doc.go | 2 +- .../internal/analysis/modernize/modernize.go | 2 +- .../analysis/modernize/modernize_test.go | 1 + .../modernize/{splitseq.go => stringsseq.go} | 31 +++++++++----- .../testdata/src/fieldsseq/fieldsseq.go | 42 +++++++++++++++++++ .../src/fieldsseq/fieldsseq.go.golden | 42 +++++++++++++++++++ .../testdata/src/fieldsseq/fieldsseq_go123.go | 1 + gopls/internal/doc/api.json | 4 +- 9 files changed, 111 insertions(+), 16 deletions(-) rename gopls/internal/analysis/modernize/{splitseq.go => stringsseq.go} (77%) create mode 100644 gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq.go create mode 100644 gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq.go.golden create mode 100644 gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq_go123.go diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md index dde95591718..aa95e024089 100644 --- a/gopls/doc/analyzers.md +++ b/gopls/doc/analyzers.md @@ -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: diff --git a/gopls/internal/analysis/modernize/doc.go b/gopls/internal/analysis/modernize/doc.go index 3759fdb10c5..b12abab7063 100644 --- a/gopls/internal/analysis/modernize/doc.go +++ b/gopls/internal/analysis/modernize/doc.go @@ -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: diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go index 354836d6b40..96e8b325df4 100644 --- a/gopls/internal/analysis/modernize/modernize.go +++ b/gopls/internal/analysis/modernize/modernize.go @@ -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) diff --git a/gopls/internal/analysis/modernize/modernize_test.go b/gopls/internal/analysis/modernize/modernize_test.go index 6662914b28d..7bdc8014389 100644 --- a/gopls/internal/analysis/modernize/modernize_test.go +++ b/gopls/internal/analysis/modernize/modernize_test.go @@ -24,6 +24,7 @@ func Test(t *testing.T) { "slicescontains", "slicesdelete", "splitseq", + "fieldsseq", "sortslice", "testingcontext", ) diff --git a/gopls/internal/analysis/modernize/splitseq.go b/gopls/internal/analysis/modernize/stringsseq.go similarity index 77% rename from gopls/internal/analysis/modernize/splitseq.go rename to gopls/internal/analysis/modernize/stringsseq.go index 1f3da859e9b..ca9d918912e 100644 --- a/gopls/internal/analysis/modernize/splitseq.go +++ b/gopls/internal/analysis/modernize/stringsseq.go @@ -5,6 +5,7 @@ package modernize import ( + "fmt" "go/ast" "go/token" "go/types" @@ -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() {...} // @@ -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 @@ -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)}), }}, }) } diff --git a/gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq.go b/gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq.go new file mode 100644 index 00000000000..b86df1a8a94 --- /dev/null +++ b/gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq.go @@ -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) + } +} diff --git a/gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq.go.golden b/gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq.go.golden new file mode 100644 index 00000000000..9fa1bfd1b62 --- /dev/null +++ b/gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq.go.golden @@ -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) + } +} diff --git a/gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq_go123.go b/gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq_go123.go new file mode 100644 index 00000000000..c2bd314db75 --- /dev/null +++ b/gopls/internal/analysis/modernize/testdata/src/fieldsseq/fieldsseq_go123.go @@ -0,0 +1 @@ +package fieldsseq diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json index 5775d0d4361..4001e3605bb 100644 --- a/gopls/internal/doc/api.json +++ b/gopls/internal/doc/api.json @@ -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" }, { @@ -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 },