From 5f9967d63b2b964daae36c6f0fa3e1eecdd8eb06 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 6 Feb 2025 15:08:45 -0500 Subject: [PATCH] gopls/internal/analysis/modernize: strings.Split -> SplitSeq This CL defines a modernizer for calls to strings.Split and bytes.Split, that offers a fix to instead use go1.24's SplitSeq, which avoids allocating an array. The fix is offered only if Split is used as the operand of a range statement, either directly, or indirectly via a variable whose sole use is the range statement. + tests Change-Id: I7c6c128d21ccf7f8b3c7745538177d2d162f62de Reviewed-on: https://go-review.googlesource.com/c/tools/+/647438 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam Auto-Submit: Alan Donovan --- gopls/doc/analyzers.md | 2 + gopls/internal/analysis/modernize/doc.go | 2 + .../internal/analysis/modernize/modernize.go | 1 + .../analysis/modernize/modernize_test.go | 1 + gopls/internal/analysis/modernize/splitseq.go | 112 ++++++++++++++++++ .../testdata/src/splitseq/splitseq.go | 42 +++++++ .../testdata/src/splitseq/splitseq.go.golden | 42 +++++++ .../testdata/src/splitseq/splitseq_go123.go | 1 + gopls/internal/doc/api.json | 4 +- 9 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 gopls/internal/analysis/modernize/splitseq.go create mode 100644 gopls/internal/analysis/modernize/testdata/src/splitseq/splitseq.go create mode 100644 gopls/internal/analysis/modernize/testdata/src/splitseq/splitseq.go.golden create mode 100644 gopls/internal/analysis/modernize/testdata/src/splitseq/splitseq_go123.go diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md index 06ac853800f..68465f9809d 100644 --- a/gopls/doc/analyzers.md +++ b/gopls/doc/analyzers.md @@ -497,6 +497,8 @@ existing code by using more modern features of Go, such as: added in go1.21 - 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; Default: on. diff --git a/gopls/internal/analysis/modernize/doc.go b/gopls/internal/analysis/modernize/doc.go index 6a247feccf4..15aeab64d8d 100644 --- a/gopls/internal/analysis/modernize/doc.go +++ b/gopls/internal/analysis/modernize/doc.go @@ -30,4 +30,6 @@ // added in go1.21 // - 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; package modernize diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go index 96ab3131833..861194e6242 100644 --- a/gopls/internal/analysis/modernize/modernize.go +++ b/gopls/internal/analysis/modernize/modernize.go @@ -70,6 +70,7 @@ func run(pass *analysis.Pass) (any, error) { rangeint(pass) slicescontains(pass) slicesdelete(pass) + splitseq(pass) sortslice(pass) testingContext(pass) diff --git a/gopls/internal/analysis/modernize/modernize_test.go b/gopls/internal/analysis/modernize/modernize_test.go index 7e375c1c24c..6662914b28d 100644 --- a/gopls/internal/analysis/modernize/modernize_test.go +++ b/gopls/internal/analysis/modernize/modernize_test.go @@ -23,6 +23,7 @@ func Test(t *testing.T) { "rangeint", "slicescontains", "slicesdelete", + "splitseq", "sortslice", "testingcontext", ) diff --git a/gopls/internal/analysis/modernize/splitseq.go b/gopls/internal/analysis/modernize/splitseq.go new file mode 100644 index 00000000000..1f3da859e9b --- /dev/null +++ b/gopls/internal/analysis/modernize/splitseq.go @@ -0,0 +1,112 @@ +// Copyright 2025 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package modernize + +import ( + "go/ast" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/types/typeutil" + "golang.org/x/tools/internal/analysisinternal" + "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: +// +// for _, line := range strings.Split() {...} +// +// or indirectly, if the variable's sole use is the range statement: +// +// lines := strings.Split() +// for _, line := range lines {...} +// +// Variants: +// - bytes.SplitSeq +func splitseq(pass *analysis.Pass) { + if !analysisinternal.Imports(pass.Pkg, "strings") && + !analysisinternal.Imports(pass.Pkg, "bytes") { + return + } + info := pass.TypesInfo + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + for curFile := range filesUsing(inspect, info, "go1.24") { + for curRange := range curFile.Preorder((*ast.RangeStmt)(nil)) { + rng := curRange.Node().(*ast.RangeStmt) + + // Reject "for i, line := ..." since SplitSeq is not an iter.Seq2. + // (We require that i is blank.) + if id, ok := rng.Key.(*ast.Ident); ok && id.Name != "_" { + continue + } + + // Find the call operand of the range statement, + // whether direct or indirect. + call, ok := rng.X.(*ast.CallExpr) + if !ok { + if id, ok := rng.X.(*ast.Ident); ok { + if v, ok := info.Uses[id].(*types.Var); ok { + if ek, idx := curRange.Edge(); ek == edge.BlockStmt_List && idx > 0 { + curPrev, _ := curRange.PrevSibling() + if assign, ok := curPrev.Node().(*ast.AssignStmt); ok && + assign.Tok == token.DEFINE && + len(assign.Lhs) == 1 && + len(assign.Rhs) == 1 && + info.Defs[assign.Lhs[0].(*ast.Ident)] == v && + soleUse(info, v) == id { + // Have: + // lines := ... + // for _, line := range lines {...} + // and no other uses of lines. + call, _ = assign.Rhs[0].(*ast.CallExpr) + } + } + } + } + } + + if call != nil { + var edits []analysis.TextEdit + if rng.Key != nil { + // Delete (blank) RangeStmt.Key: + // for _, line := -> for line := + // for _, _ := -> for + // for _ := -> for + end := rng.Range + if rng.Value != nil { + end = rng.Value.Pos() + } + edits = append(edits, analysis.TextEdit{ + Pos: rng.Key.Pos(), + End: end, + }) + } + + if sel, ok := call.Fun.(*ast.SelectorExpr); ok && + (analysisinternal.IsFunctionNamed(typeutil.Callee(info, call), "strings", "Split") || + analysisinternal.IsFunctionNamed(typeutil.Callee(info, call), "bytes", "Split")) { + pass.Report(analysis.Diagnostic{ + Pos: sel.Pos(), + End: sel.End(), + Category: "splitseq", + Message: "Ranging over SplitSeq is more efficient", + SuggestedFixes: []analysis.SuggestedFix{{ + Message: "Replace Split with SplitSeq", + TextEdits: append(edits, analysis.TextEdit{ + // Split -> SplitSeq + Pos: sel.Sel.Pos(), + End: sel.Sel.End(), + NewText: []byte("SplitSeq")}), + }}, + }) + } + } + } + } +} diff --git a/gopls/internal/analysis/modernize/testdata/src/splitseq/splitseq.go b/gopls/internal/analysis/modernize/testdata/src/splitseq/splitseq.go new file mode 100644 index 00000000000..4f533ed22bc --- /dev/null +++ b/gopls/internal/analysis/modernize/testdata/src/splitseq/splitseq.go @@ -0,0 +1,42 @@ +//go:build go1.24 + +package splitseq + +import ( + "bytes" + "strings" +) + +func _() { + for _, line := range strings.Split("", "") { // want "Ranging over SplitSeq is more efficient" + println(line) + } + for i, line := range strings.Split("", "") { // nope: uses index var + println(i, line) + } + for i, _ := range strings.Split("", "") { // nope: uses index var + println(i) + } + for i := range strings.Split("", "") { // nope: uses index var + println(i) + } + for _ = range strings.Split("", "") { // want "Ranging over SplitSeq is more efficient" + } + for range strings.Split("", "") { // want "Ranging over SplitSeq is more efficient" + } + for range bytes.Split(nil, nil) { // want "Ranging over SplitSeq is more efficient" + } + { + lines := strings.Split("", "") // want "Ranging over SplitSeq is more efficient" + for _, line := range lines { + println(line) + } + } + { + lines := strings.Split("", "") // 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/splitseq/splitseq.go.golden b/gopls/internal/analysis/modernize/testdata/src/splitseq/splitseq.go.golden new file mode 100644 index 00000000000..d10e0e8e564 --- /dev/null +++ b/gopls/internal/analysis/modernize/testdata/src/splitseq/splitseq.go.golden @@ -0,0 +1,42 @@ +//go:build go1.24 + +package splitseq + +import ( + "bytes" + "strings" +) + +func _() { + for line := range strings.SplitSeq("", "") { // want "Ranging over SplitSeq is more efficient" + println(line) + } + for i, line := range strings.Split("", "") { // nope: uses index var + println(i, line) + } + for i, _ := range strings.Split("", "") { // nope: uses index var + println(i) + } + for i := range strings.Split("", "") { // nope: uses index var + println(i) + } + for range strings.SplitSeq("", "") { // want "Ranging over SplitSeq is more efficient" + } + for range strings.SplitSeq("", "") { // want "Ranging over SplitSeq is more efficient" + } + for range bytes.SplitSeq(nil, nil) { // want "Ranging over SplitSeq is more efficient" + } + { + lines := strings.SplitSeq("", "") // want "Ranging over SplitSeq is more efficient" + for line := range lines { + println(line) + } + } + { + lines := strings.Split("", "") // 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/splitseq/splitseq_go123.go b/gopls/internal/analysis/modernize/testdata/src/splitseq/splitseq_go123.go new file mode 100644 index 00000000000..c3e86bb2ed9 --- /dev/null +++ b/gopls/internal/analysis/modernize/testdata/src/splitseq/splitseq_go123.go @@ -0,0 +1 @@ +package splitseq diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json index b83dfe4bde0..8f101079a9c 100644 --- a/gopls/internal/doc/api.json +++ b/gopls/internal/doc/api.json @@ -510,7 +510,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;", + "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;", "Default": "true" }, { @@ -1189,7 +1189,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;", + "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;", "URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize", "Default": true },