[linter-miner] linter: add lenstringsplit analyzer (#41090)

Author: github-actions[bot]

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,

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.*

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)
+}

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")
+}

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, ","))
+}

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, ","))
+}