diff --git a/cmd/linters/main.go b/cmd/linters/main.go index 9c443f7f483..d544456ac3c 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -31,6 +31,7 @@ import ( "github.com/github/gh-aw/pkg/linters/httpnoctx" "github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror" "github.com/github/gh-aw/pkg/linters/largefunc" + "github.com/github/gh-aw/pkg/linters/lenstringsplit" "github.com/github/gh-aw/pkg/linters/lenstringzero" "github.com/github/gh-aw/pkg/linters/manualmutexunlock" "github.com/github/gh-aw/pkg/linters/osexitinlibrary" @@ -81,6 +82,7 @@ func main() { strconvparseignorederror.Analyzer, jsonmarshalignoredeerror.Analyzer, lenstringzero.Analyzer, + lenstringsplit.Analyzer, timeafterleak.Analyzer, timesleepnocontext.Analyzer, tolowerequalfold.Analyzer, diff --git a/docs/adr/41090-add-lenstringsplit-linter.md b/docs/adr/41090-add-lenstringsplit-linter.md new file mode 100644 index 00000000000..1b44cbfb974 --- /dev/null +++ b/docs/adr/41090-add-lenstringsplit-linter.md @@ -0,0 +1,44 @@ +# ADR-41090: Add `lenstringsplit` linter for `len(strings.Split(...))` expressions + +**Date**: 2026-06-23 +**Status**: Draft + +## Context + +The codebase maintains a custom `go/analysis` linter suite (`pkg/linters/*`, registered in `cmd/linters/main.go`) to enforce house style and catch performance and correctness anti-patterns that off-the-shelf golangci-lint rules miss. The expression `len(strings.Split(s, sep))` allocates a full `[]string` slice only to discard it after taking the count; `strings.Count(s, sep)+1` produces the identical value with zero intermediate allocation. A code-pattern scan found four occurrences of this pattern in non-test files (`pkg/workflow/codex_logs.go`, `pkg/workflow/xml_comments.go`, `pkg/workflow/pip.go`, `pkg/parser/schema_errors.go`), establishing it as a recurring rather than one-off issue. A new rule must fit the established analyzer framework and produce zero false positives. + +## Decision + +We will add a new `go/analysis` analyzer package `pkg/linters/lenstringsplit` and register it in `cmd/linters/main.go`. The analyzer visits every `*ast.CallExpr` via the shared `inspect` pass, matches the builtin `len` applied to a single argument that is itself a call, and resolves that inner call's selector type against the standard-library `strings` package (via `astutil.IsPkgSelector`) to confirm it is `strings.Split`. Test files are skipped via `filecheck.IsTestFile`. The diagnostic is purely advisory (`pass.ReportRangef`) and names the exact replacement. + +## Alternatives Considered + +### Alternative 1: Rely on an off-the-shelf linter + +No enabled golangci-lint rule flags `len(strings.Split(...))`. Adopting or re-enabling an external linter to cover this would pull in unrelated checks and is subject to upstream timelines outside our control. Rejected in favor of a focused local analyzer consistent with the existing `pkg/linters` suite (e.g. the sibling `lenstringzero` rule). + +### Alternative 2: Name-based matching on `Split()` without package resolution + +The analyzer could flag any selector call ending in `.Split()` without consulting type information. This is simpler but would misreport unrelated `Split()` methods on user types (e.g. `bytes.Split`, or a custom splitter). Rejected; resolving the selector against the `strings` package keeps false positives at zero, as exercised by the `strings.Fields` and pre-split-slice negative fixtures. + +## Consequences + +### Positive + +- Catches a recurring, measurable allocation waste (four confirmed sites) that no currently enabled linter detects, extending the suite alongside the sibling `lenstringzero` analyzer. +- Package-resolved matching yields zero false positives: the testdata fixtures confirm `strings.Fields`, plain `len(slice)`, and bare `strings.Split` (result actually used) are not flagged. +- Follows the established analyzer pattern, so it is consistent with the rest of the suite and covered by `analysistest`-based tests. + +### Negative + +- Adds another analyzer to maintain and run, marginally increasing lint time and the registration surface in `cmd/linters/main.go`. +- Only the literal `len(strings.Split(...))` shape is detected; an equivalent count taken through an intermediate variable (`parts := strings.Split(...); len(parts)`) is out of scope. + +### Neutral + +- The rule is purely diagnostic; it offers no automated fix/suggested edit, so the four existing call sites must be migrated manually if desired. +- `strings.SplitN` and `strings.SplitAfter` are not matched — only `Split` — keeping the rule narrowly scoped to the exact wasteful idiom. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/28048403873) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/linters/lenstringsplit/lenstringsplit.go b/pkg/linters/lenstringsplit/lenstringsplit.go new file mode 100644 index 00000000000..0ea93f0b47a --- /dev/null +++ b/pkg/linters/lenstringsplit/lenstringsplit.go @@ -0,0 +1,146 @@ +// Package lenstringsplit implements a Go analysis linter that flags +// len(strings.Split(s, sep)) expressions with a provably non-empty separator +// that allocate a []string just to count substrings. strings.Count(s, sep)+1 +// achieves the same result for non-empty separators without the intermediate +// allocation. +package lenstringsplit + +import ( + "fmt" + "go/ast" + "go/constant" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + + "github.com/github/gh-aw/pkg/linters/internal/astutil" + "github.com/github/gh-aw/pkg/linters/internal/filecheck" + "github.com/github/gh-aw/pkg/linters/internal/nolint" +) + +// Analyzer is the len-strings-split analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "lenstringsplit", + Doc: "reports len(strings.Split(s, sep)) expressions with a provably non-empty separator that allocate a []string just to count substrings; use strings.Count(s, sep)+1 instead", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/lenstringsplit", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err + } + noLintLinesByFile := nolint.BuildLineIndex(pass, "lenstringsplit") + + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} + + insp.Preorder(nodeFilter, func(n ast.Node) { + outer, ok := n.(*ast.CallExpr) + if !ok { + return + } + + if !isBuiltinLen(pass, outer) { + return + } + + if len(outer.Args) != 1 { + return + } + inner, ok := outer.Args[0].(*ast.CallExpr) + if !ok { + return + } + if !isStringsSplit(pass, inner) { + return + } + if !hasProvablyNonEmptySeparator(pass, inner) { + return + } + + pos := pass.Fset.PositionFor(outer.Pos(), false) + if filecheck.IsTestFile(pos.Filename) { + return + } + if nolint.HasDirective(pos, noLintLinesByFile) { + return + } + + pass.Report(analysis.Diagnostic{ + Pos: outer.Pos(), + End: outer.End(), + Message: "len(strings.Split(...)) allocates a []string just to count substrings; use strings.Count(...)+1 instead", + SuggestedFixes: buildCountFix(pass, outer, inner), + }) + }) + + return nil, nil +} + +// isBuiltinLen reports whether call is an invocation of the builtin len function. +func isBuiltinLen(pass *analysis.Pass, call *ast.CallExpr) bool { + ident, ok := call.Fun.(*ast.Ident) + if !ok || ident.Name != "len" { + return false + } + obj := pass.TypesInfo.Uses[ident] + return obj == nil || obj == types.Universe.Lookup("len") +} + +// isStringsSplit reports whether call is strings.Split from the standard +// library "strings" package. +func isStringsSplit(pass *analysis.Pass, call *ast.CallExpr) bool { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Split" { + return false + } + return astutil.IsPkgSelector(pass, sel, "strings") +} + +func hasProvablyNonEmptySeparator(pass *analysis.Pass, call *ast.CallExpr) bool { + if len(call.Args) != 2 { + return false + } + if lit, ok := call.Args[1].(*ast.BasicLit); ok && lit.Kind == token.STRING { + return lit.Value != `""` + } + tv, ok := pass.TypesInfo.Types[call.Args[1]] + if !ok || tv.Value == nil || tv.Value.Kind() != constant.String { + return false + } + return constant.StringVal(tv.Value) != "" +} + +func buildCountFix(pass *analysis.Pass, outer, inner *ast.CallExpr) []analysis.SuggestedFix { + if len(inner.Args) != 2 { + return nil + } + + sText := astutil.NodeText(pass.Fset, inner.Args[0]) + sepText := astutil.NodeText(pass.Fset, inner.Args[1]) + pkgText := splitPkgText(pass, inner) + if sText == "" || sepText == "" || pkgText == "" { + return nil + } + + return []analysis.SuggestedFix{{ + Message: "Replace with strings.Count(...)+1", + TextEdits: []analysis.TextEdit{{ + Pos: outer.Pos(), + End: outer.End(), + NewText: fmt.Appendf(nil, "%s.Count(%s, %s)+1", pkgText, sText, sepText), + }}, + }} +} + +func splitPkgText(pass *analysis.Pass, call *ast.CallExpr) string { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return "" + } + return astutil.NodeText(pass.Fset, sel.X) +} diff --git a/pkg/linters/lenstringsplit/lenstringsplit_test.go b/pkg/linters/lenstringsplit/lenstringsplit_test.go new file mode 100644 index 00000000000..4c654d5bb66 --- /dev/null +++ b/pkg/linters/lenstringsplit/lenstringsplit_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package lenstringsplit_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/lenstringsplit" +) + +func TestLenStringSplit(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, lenstringsplit.Analyzer, "lenstringsplit") +} diff --git a/pkg/linters/lenstringsplit/testdata/src/lenstringsplit/lenstringsplit.go b/pkg/linters/lenstringsplit/testdata/src/lenstringsplit/lenstringsplit.go new file mode 100644 index 00000000000..8587e6625d5 --- /dev/null +++ b/pkg/linters/lenstringsplit/testdata/src/lenstringsplit/lenstringsplit.go @@ -0,0 +1,58 @@ +package lenstringsplit + +import "strings" + +const comma = "," + +// flagged: len(strings.Split(...)) with a non-empty separator inside a return statement. +func countLines(content string) int { + return len(strings.Split(content, "\n")) // want `len\(strings\.Split\(\.\.\.` +} + +// flagged: len(strings.Split(...)) assigned to a variable. +func countFields(s string) int { + n := len(strings.Split(s, comma)) // want `len\(strings\.Split\(\.\.\.` + return n +} + +// not flagged: len() is not called on the split result. +func splitAndUse(s string) []string { + return strings.Split(s, "/") +} + +// not flagged: len applied to a pre-split slice, not a strings.Split call. +func countFromSlice(parts []string) int { + return len(parts) +} + +// not flagged: strings.Fields is a different function. +func countWords(s string) int { + return len(strings.Fields(s)) +} + +// not flagged: empty separators are not equivalent to strings.Count(...)+1. +func countRunes(s string) int { + return len(strings.Split(s, "")) +} + +func customLenNotFlagged(s string) int { + len := func(parts []string) int { + return 0 + } + return len(strings.Split(s, ",")) +} + +// not flagged: a nolint directive suppresses the diagnostic. +func suppressedCount(s string) int { + return len(strings.Split(s, ",")) //nolint:lenstringsplit +} + +// not flagged: strings.SplitN has different semantics. +func countSplitN(s string) int { + return len(strings.SplitN(s, ",", 3)) +} + +// not flagged: strings.SplitAfter is a distinct function. +func countSplitAfter(s string) int { + return len(strings.SplitAfter(s, ",")) +} diff --git a/pkg/linters/lenstringsplit/testdata/src/lenstringsplit/lenstringsplit.go.golden b/pkg/linters/lenstringsplit/testdata/src/lenstringsplit/lenstringsplit.go.golden new file mode 100644 index 00000000000..408a4057c29 --- /dev/null +++ b/pkg/linters/lenstringsplit/testdata/src/lenstringsplit/lenstringsplit.go.golden @@ -0,0 +1,58 @@ +package lenstringsplit + +import "strings" + +const comma = "," + +// flagged: len(strings.Split(...)) with a non-empty separator inside a return statement. +func countLines(content string) int { + return strings.Count(content, "\n")+1 // want `len\(strings\.Split\(\.\.\.` +} + +// flagged: len(strings.Split(...)) assigned to a variable. +func countFields(s string) int { + n := strings.Count(s, comma)+1 // want `len\(strings\.Split\(\.\.\.` + return n +} + +// not flagged: len() is not called on the split result. +func splitAndUse(s string) []string { + return strings.Split(s, "/") +} + +// not flagged: len applied to a pre-split slice, not a strings.Split call. +func countFromSlice(parts []string) int { + return len(parts) +} + +// not flagged: strings.Fields is a different function. +func countWords(s string) int { + return len(strings.Fields(s)) +} + +// not flagged: empty separators are not equivalent to strings.Count(...)+1. +func countRunes(s string) int { + return len(strings.Split(s, "")) +} + +func customLenNotFlagged(s string) int { + len := func(parts []string) int { + return 0 + } + return len(strings.Split(s, ",")) +} + +// not flagged: a nolint directive suppresses the diagnostic. +func suppressedCount(s string) int { + return len(strings.Split(s, ",")) //nolint:lenstringsplit +} + +// not flagged: strings.SplitN has different semantics. +func countSplitN(s string) int { + return len(strings.SplitN(s, ",", 3)) +} + +// not flagged: strings.SplitAfter is a distinct function. +func countSplitAfter(s string) int { + return len(strings.SplitAfter(s, ",")) +}