diff --git a/cmd/linters/main.go b/cmd/linters/main.go index ac65719b0b2..2c3571f0d4c 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -53,6 +53,7 @@ import ( "github.com/github/gh-aw/pkg/linters/ssljson" "github.com/github/gh-aw/pkg/linters/strconvparseignorederror" "github.com/github/gh-aw/pkg/linters/stringreplaceminusone" + "github.com/github/gh-aw/pkg/linters/stringscountcontains" "github.com/github/gh-aw/pkg/linters/stringsindexcontains" "github.com/github/gh-aw/pkg/linters/timeafterleak" "github.com/github/gh-aw/pkg/linters/timesleepnocontext" @@ -99,6 +100,7 @@ func main() { strconvparseignorederror.Analyzer, stringreplaceminusone.Analyzer, stringsindexcontains.Analyzer, + stringscountcontains.Analyzer, jsonmarshalignoredeerror.Analyzer, lenstringzero.Analyzer, lenstringsplit.Analyzer, diff --git a/docs/adr/44820-add-stringscountcontains-linter.md b/docs/adr/44820-add-stringscountcontains-linter.md new file mode 100644 index 00000000000..c40be42ddb1 --- /dev/null +++ b/docs/adr/44820-add-stringscountcontains-linter.md @@ -0,0 +1,48 @@ +# ADR-44820: Add stringscountcontains Linter + +**Date**: 2026-07-11 +**Status**: Draft +**Deciders**: pelikhan (PR author) + +--- + +### Context + +The `pkg/linters/` package contains a suite of custom Go static-analysis analyzers that enforce project-specific code quality rules. A common Go antipattern is using `strings.Count(s, sub)` compared against `0` or `1` as a containment check (e.g., `strings.Count(s, sub) > 0`). `strings.Count` traverses the entire string to count all occurrences, whereas `strings.Contains` short-circuits on the first match. The antipattern is therefore both less readable — it expresses counting, not containment — and potentially less efficient. No existing linter in the suite flags this pattern. + +### Decision + +We will add a new `stringscountcontains` analyzer to `pkg/linters/` that flags `strings.Count(s, sub)` comparisons with `0` or `1` (in canonical and yoda order) and emits a `SuggestedFix` rewriting them to `strings.Contains(s, sub)` or `!strings.Contains(s, sub)`. The analyzer is registered in the multichecker binary and documented in the README. It follows the same structural conventions as the existing `stringsindexcontains` linter (AST traversal, `nolint` directive support, `_test.go` file exclusion, `analysistest`-based tests with a `.go.golden` file). + +### Alternatives Considered + +#### Alternative 1: Rely on external community linters (e.g., `gocritic`) + +`gocritic` includes checks that overlap with this pattern. However, integrating a large external linter as a dependency would add transitive dependency weight, reduce control over diagnostic messages and fix behaviour, and introduce versioning friction with the rest of the custom linter suite. The project has deliberately invested in a custom linter framework to keep full control over rule semantics, fix generation, and `nolint` integration. + +#### Alternative 2: Manual code review enforcement only + +Documenting the preferred pattern in style guides and relying on reviewers to catch it during PR review is zero-cost to implement. However, it does not scale: reviewers are inconsistent, the pattern is easy to miss, and it provides no automated-fix path. The existing suite demonstrates a project preference for automated, machine-checkable rules over guidance-only policies. + +#### Alternative 3: No action (accept the pattern as harmless) + +The performance difference between `strings.Count` and `strings.Contains` for containment checks is typically negligible. One could argue the antipattern does not warrant enforcement. However, the readability argument is independent of performance: code that uses `strings.Count` to test containment misleads readers about intent, and the project's existing `stringsindexcontains` linter already enforces the analogous `strings.Index` → `strings.Contains` rewrite, establishing a precedent that this family of patterns should be machine-checked. + +### Consequences + +#### Positive +- All `strings.Count`-as-containment antipatterns in non-test code will be flagged automatically, improving readability and intent clarity. +- The `SuggestedFix` allows batch automated repair (e.g., via `gopls` or `go fix`), reducing developer friction when adopting the rule on an existing codebase. +- Extends the existing linter suite consistently: the `stringsindexcontains` precedent is maintained. + +#### Negative +- Existing code that uses the flagged pattern must be fixed or suppressed with `//nolint:stringscountcontains`, which is a one-time migration cost. +- Adds a new package to maintain; future changes to `strings.Count` semantics or Go AST APIs require updating this analyzer. + +#### Neutral +- The linter skips `_test.go` files, so test code using the pattern for clarity is unaffected. +- The 40-analyzer count in `spec_test.go` and README must be kept in sync when adding future linters — this PR establishes that the count is a documented invariant. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/linters/README.md b/pkg/linters/README.md index b2e2febd634..6d1cace5842 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -43,6 +43,7 @@ This package currently provides custom Go analyzers in the following subpackages - `ssljson` — validates `ssl.json` skill artifacts found in `.github/skills/` against the SSL spec (enum membership, graph integrity, transition targets, entry pointer validity). - `strconvparseignorederror` — reports `strconv` parsing calls (`Atoi`, `ParseInt`, etc.) where the error return is discarded with `_`. - `stringreplaceminusone` — reports `strings.Replace` calls whose `n` argument is `-1`, which should use the more readable `strings.ReplaceAll`. +- `stringscountcontains` — reports `strings.Count(s, sub)` comparisons with `0` or `1` (e.g. `> 0`, `>= 1`, `== 0`, `!= 0`, `< 1`, `<= 0`) and their yoda-order variants that should use `strings.Contains(s, sub)` or `!strings.Contains(s, sub)` instead. - `stringsindexcontains` — reports `strings.Index(s, substr)` comparisons with `-1` or `0` (e.g. `!= -1`, `>= 0`, `> -1`, `== -1`, `< 0`, `<= -1`) and their yoda-order variants that should use `strings.Contains(s, substr)` or `!strings.Contains(s, substr)` instead. - `timeafterleak` — reports `time.After` calls used as the channel-receive expression in a `select` case inside a `for` or `range` loop that leak a timer channel on each iteration when another case fires first. - `timesleepnocontext` — reports `time.Sleep` calls inside functions that already receive a `context.Context`, where a context-aware `select` should be used instead. @@ -95,6 +96,7 @@ This package currently provides custom Go analyzers in the following subpackages | `ssljson` | Custom `go/analysis` analyzer that validates SSL JSON skill artifacts in `.github/skills/` | | `strconvparseignorederror` | Custom `go/analysis` analyzer that flags `strconv` parsing calls where the error return is discarded with `_` | | `stringreplaceminusone` | Custom `go/analysis` analyzer that flags `strings.Replace` calls with `n=-1` that should use `strings.ReplaceAll` | +| `stringscountcontains` | Custom `go/analysis` analyzer that flags `strings.Count(s, sub)` comparisons with `0` or `1` that should use `strings.Contains` or `!strings.Contains` | | `stringsindexcontains` | Custom `go/analysis` analyzer that flags `strings.Index(s, substr)` comparisons with `-1` or `0` that should use `strings.Contains` or `!strings.Contains` | | `timeafterleak` | Custom `go/analysis` analyzer that flags `time.After` in `select` cases inside loops that leak a timer channel on each iteration when another case fires first | | `timesleepnocontext` | Custom `go/analysis` analyzer that flags `time.Sleep` calls in context-aware functions | @@ -207,6 +209,7 @@ _ = timesleepnocontext.Analyzer - `github.com/github/gh-aw/pkg/linters/ssljson` — ssl-json analyzer subpackage - `github.com/github/gh-aw/pkg/linters/strconvparseignorederror` — strconv-parse-ignored-error analyzer subpackage - `github.com/github/gh-aw/pkg/linters/stringreplaceminusone` — string-replace-minus-one analyzer subpackage +- `github.com/github/gh-aw/pkg/linters/stringscountcontains` — strings-count-contains analyzer subpackage - `github.com/github/gh-aw/pkg/linters/stringsindexcontains` — strings-index-contains analyzer subpackage - `github.com/github/gh-aw/pkg/linters/timeafterleak` — time-after-leak analyzer subpackage - `github.com/github/gh-aw/pkg/linters/timesleepnocontext` — time-sleep-no-context analyzer subpackage diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index f011538ad72..1d2d4e3f208 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -43,6 +43,7 @@ import ( "github.com/github/gh-aw/pkg/linters/ssljson" "github.com/github/gh-aw/pkg/linters/strconvparseignorederror" "github.com/github/gh-aw/pkg/linters/stringreplaceminusone" + "github.com/github/gh-aw/pkg/linters/stringscountcontains" "github.com/github/gh-aw/pkg/linters/stringsindexcontains" "github.com/github/gh-aw/pkg/linters/timeafterleak" "github.com/github/gh-aw/pkg/linters/timesleepnocontext" @@ -65,7 +66,7 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 39 analyzers +// "Public API > Subpackages" table. The README documents 40 analyzers // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // @@ -76,7 +77,7 @@ type docAnalyzer struct { // hardcodedfilepath, httpnoctx, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero, // manualmutexunlock, osexitinlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib, // regexpcompileinfunction, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, ssljson, -// strconvparseignorederror, stringreplaceminusone, stringsindexcontains, timeafterleak, timesleepnocontext, +// strconvparseignorederror, stringreplaceminusone, stringscountcontains, stringsindexcontains, timeafterleak, timesleepnocontext, // tolowerequalfold, uncheckedtypeassertion, wgdonenotdeferred, writebytestring func documentedAnalyzers() []docAnalyzer { return []docAnalyzer{ @@ -112,6 +113,7 @@ func documentedAnalyzers() []docAnalyzer { {"ssljson", ssljson.Analyzer}, {"strconvparseignorederror", strconvparseignorederror.Analyzer}, {"stringreplaceminusone", stringreplaceminusone.Analyzer}, + {"stringscountcontains", stringscountcontains.Analyzer}, {"stringsindexcontains", stringsindexcontains.Analyzer}, {"timeafterleak", timeafterleak.Analyzer}, {"timesleepnocontext", timesleepnocontext.Analyzer}, diff --git a/pkg/linters/stringscountcontains/stringscountcontains.go b/pkg/linters/stringscountcontains/stringscountcontains.go new file mode 100644 index 00000000000..8e5e5c3be64 --- /dev/null +++ b/pkg/linters/stringscountcontains/stringscountcontains.go @@ -0,0 +1,220 @@ +// Package stringscountcontains implements a Go analysis linter that flags +// strings.Count(s, sub) comparisons with 0 or 1 (e.g. > 0, >= 1, == 0, +// != 0, < 1, <= 0) and their yoda-order variants that should use the more +// readable strings.Contains(s, sub) or !strings.Contains(s, sub) instead. +package stringscountcontains + +import ( + "fmt" + "go/ast" + "go/constant" + "go/token" + + "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 strings-count-contains analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "stringscountcontains", + Doc: "reports strings.Count(s, sub) comparisons with 0 or 1 (e.g. > 0, >= 1, == 0, != 0, < 1, <= 0) and their yoda-order variants that should use strings.Contains(s, sub) or !strings.Contains(s, sub)", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/stringscountcontains", + 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, "stringscountcontains") + + nodeFilter := []ast.Node{(*ast.BinaryExpr)(nil)} + + insp.Preorder(nodeFilter, func(n ast.Node) { + expr, ok := n.(*ast.BinaryExpr) + if !ok { + return + } + + pos := pass.Fset.PositionFor(expr.Pos(), false) + if filecheck.IsTestFile(pos.Filename) { + return + } + if nolint.HasDirective(pos, noLintLinesByFile) { + return + } + + countCall, negated, matched := matchCountComparison(pass, expr) + if !matched { + return + } + + if len(countCall.Args) != 2 { + return + } + + sText := astutil.NodeText(pass.Fset, countCall.Args[0]) + subText := astutil.NodeText(pass.Fset, countCall.Args[1]) + pkgText := countPkgText(pass, countCall) + if sText == "" || subText == "" || pkgText == "" { + return + } + + var msg string + if negated { + msg = fmt.Sprintf("use !strings.Contains(%s, %s) instead of strings.Count comparison", sText, subText) + } else { + msg = fmt.Sprintf("use strings.Contains(%s, %s) instead of strings.Count comparison", sText, subText) + } + + pass.Report(analysis.Diagnostic{ + Pos: expr.Pos(), + End: expr.End(), + Message: msg, + SuggestedFixes: buildContainsFix(pass, expr, pkgText, sText, subText, negated), + }) + }) + + return nil, nil +} + +// matchCountComparison reports whether expr is a strings.Count comparison with +// 0 or 1 that can be replaced with strings.Contains. +// +// Matched patterns (contains → negated=false): +// - strings.Count(s, sub) > 0 +// - strings.Count(s, sub) >= 1 +// - strings.Count(s, sub) != 0 +// - 0 < strings.Count(s, sub) +// - 1 <= strings.Count(s, sub) +// - 0 != strings.Count(s, sub) +// +// Matched patterns (not-contains → negated=true): +// - strings.Count(s, sub) == 0 +// - strings.Count(s, sub) < 1 +// - strings.Count(s, sub) <= 0 +// - 0 == strings.Count(s, sub) +// - 1 > strings.Count(s, sub) +func matchCountComparison(pass *analysis.Pass, expr *ast.BinaryExpr) (call *ast.CallExpr, negated bool, matched bool) { + // Normalize so the strings.Count call is on the left side. + left, right, flipped := normalizeOperands(pass, expr) + + countCall, ok := asStringsCountCall(pass, left) + if !ok { + return nil, false, false + } + + op := expr.Op + if flipped { + op = astutil.FlipComparisonOp(op) + } + + litVal, ok := constIntValue(pass, right) + if !ok { + return nil, false, false + } + + switch op { + case token.GTR: + // strings.Count(...) > 0 → contains + if litVal == 0 { + return countCall, false, true + } + case token.GEQ: + // strings.Count(...) >= 1 → contains + if litVal == 1 { + return countCall, false, true + } + case token.NEQ: + // strings.Count(...) != 0 → contains + if litVal == 0 { + return countCall, false, true + } + case token.EQL: + // strings.Count(...) == 0 → !contains + if litVal == 0 { + return countCall, true, true + } + case token.LSS: + // strings.Count(...) < 1 → !contains + if litVal == 1 { + return countCall, true, true + } + case token.LEQ: + // strings.Count(...) <= 0 → !contains + if litVal == 0 { + return countCall, true, true + } + } + + return nil, false, false +} + +// normalizeOperands returns (left, right) such that if the strings.Count call +// is on the right side, the operands are swapped and flipped=true. +func normalizeOperands(pass *analysis.Pass, expr *ast.BinaryExpr) (left, right ast.Expr, flipped bool) { + if _, ok := asStringsCountCall(pass, expr.X); ok { + return expr.X, expr.Y, false + } + return expr.Y, expr.X, true +} + +// asStringsCountCall returns the *ast.CallExpr if expr is a call to strings.Count. +func asStringsCountCall(pass *analysis.Pass, expr ast.Expr) (*ast.CallExpr, bool) { + call, ok := expr.(*ast.CallExpr) + if !ok { + return nil, false + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Count" { + return nil, false + } + if !astutil.IsPkgSelector(pass, sel, "strings") { + return nil, false + } + return call, true +} + +// constIntValue returns the integer constant value of expr, if it is a constant integer. +func constIntValue(pass *analysis.Pass, expr ast.Expr) (int64, bool) { + tv, ok := pass.TypesInfo.Types[expr] + if !ok || tv.Value == nil || tv.Value.Kind() != constant.Int { + return 0, false + } + v, exact := constant.Int64Val(tv.Value) + return v, exact +} + +// countPkgText returns the package selector text (e.g., "strings") from a strings.Count call. +func countPkgText(pass *analysis.Pass, call *ast.CallExpr) string { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return "" + } + return astutil.NodeText(pass.Fset, sel.X) +} + +// buildContainsFix builds the suggested fix rewriting the comparison to strings.Contains. +func buildContainsFix(pass *analysis.Pass, expr *ast.BinaryExpr, pkgText, sText, subText string, negated bool) []analysis.SuggestedFix { + var replacement string + if negated { + replacement = "!" + pkgText + ".Contains(" + sText + ", " + subText + ")" + } else { + replacement = pkgText + ".Contains(" + sText + ", " + subText + ")" + } + + return []analysis.SuggestedFix{{ + Message: "Replace strings.Count comparison with strings.Contains", + TextEdits: []analysis.TextEdit{{ + Pos: expr.Pos(), + End: expr.End(), + NewText: []byte(replacement), + }}, + }} +} diff --git a/pkg/linters/stringscountcontains/stringscountcontains_test.go b/pkg/linters/stringscountcontains/stringscountcontains_test.go new file mode 100644 index 00000000000..0c1647e6573 --- /dev/null +++ b/pkg/linters/stringscountcontains/stringscountcontains_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package stringscountcontains_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/stringscountcontains" +) + +func TestAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, stringscountcontains.Analyzer, "stringscountcontains") +} diff --git a/pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go b/pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go new file mode 100644 index 00000000000..a694d4d3063 --- /dev/null +++ b/pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go @@ -0,0 +1,56 @@ +package stringscountcontains + +import "strings" + +func badCountGTR(s, sub string) bool { + return strings.Count(s, sub) > 0 // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badCountGEQ(s, sub string) bool { + return strings.Count(s, sub) >= 1 // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badCountNEQ(s, sub string) bool { + return strings.Count(s, sub) != 0 // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badCountEQL(s, sub string) bool { + return strings.Count(s, sub) == 0 // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badCountLSS(s, sub string) bool { + return strings.Count(s, sub) < 1 // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badCountLEQ(s, sub string) bool { + return strings.Count(s, sub) <= 0 // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badYodaGTR(s, sub string) bool { + return 0 < strings.Count(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badYodaGEQ(s, sub string) bool { + return 1 <= strings.Count(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badYodaNEQ(s, sub string) bool { + return 0 != strings.Count(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badYodaEQL(s, sub string) bool { + return 0 == strings.Count(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badYodaLSS(s, sub string) bool { + return 1 > strings.Count(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func goodCountActualCount(s, sub string) bool { + // Comparing against a value other than 0/1 (as a containment sentinel) is fine. + return strings.Count(s, sub) > 2 +} + +func goodContainsAlready(s, sub string) bool { + return strings.Contains(s, sub) +} diff --git a/pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go.golden b/pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go.golden new file mode 100644 index 00000000000..222926fb4fc --- /dev/null +++ b/pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go.golden @@ -0,0 +1,56 @@ +package stringscountcontains + +import "strings" + +func badCountGTR(s, sub string) bool { + return strings.Contains(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badCountGEQ(s, sub string) bool { + return strings.Contains(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badCountNEQ(s, sub string) bool { + return strings.Contains(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badCountEQL(s, sub string) bool { + return !strings.Contains(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badCountLSS(s, sub string) bool { + return !strings.Contains(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badCountLEQ(s, sub string) bool { + return !strings.Contains(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badYodaGTR(s, sub string) bool { + return strings.Contains(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badYodaGEQ(s, sub string) bool { + return strings.Contains(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badYodaNEQ(s, sub string) bool { + return strings.Contains(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badYodaEQL(s, sub string) bool { + return !strings.Contains(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func badYodaLSS(s, sub string) bool { + return !strings.Contains(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison` +} + +func goodCountActualCount(s, sub string) bool { + // Comparing against a value other than 0/1 (as a containment sentinel) is fine. + return strings.Count(s, sub) > 2 +} + +func goodContainsAlready(s, sub string) bool { + return strings.Contains(s, sub) +}