diff --git a/cmd/linters/main.go b/cmd/linters/main.go index 030d381ff49..35838441ac2 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -51,6 +51,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/stringsindexcontains" "github.com/github/gh-aw/pkg/linters/timeafterleak" "github.com/github/gh-aw/pkg/linters/timesleepnocontext" "github.com/github/gh-aw/pkg/linters/tolowerequalfold" @@ -92,6 +93,7 @@ func main() { sprintfint.Analyzer, strconvparseignorederror.Analyzer, stringreplaceminusone.Analyzer, + stringsindexcontains.Analyzer, jsonmarshalignoredeerror.Analyzer, lenstringzero.Analyzer, lenstringsplit.Analyzer, diff --git a/docs/adr/43253-add-stringsindexcontains-linter.md b/docs/adr/43253-add-stringsindexcontains-linter.md new file mode 100644 index 00000000000..2996ea6303e --- /dev/null +++ b/docs/adr/43253-add-stringsindexcontains-linter.md @@ -0,0 +1,44 @@ +# ADR-43253: Add stringsindexcontains Custom Go Analysis Linter + +**Date**: 2026-07-04 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +The `gh-aw` codebase uses a custom Go static analysis framework (`pkg/linters/`) to enforce code style and correctness automatically. A scan of `pkg/` and `cmd/` revealed 42 occurrences of the pattern `strings.Index(s, substr) != -1` (and equivalent variants such as `>= 0`, `== -1`, `< 0`). These comparisons are semantically equivalent to `strings.Contains(s, substr)` or `!strings.Contains(s, substr)`, but are less readable and more error-prone to write correctly. The codebase already ships several analogous custom linters (e.g., `stringreplaceminusone`, `lenstringzero`) that follow the same `go/analysis` pass pattern. + +### Decision + +We will add a new `stringsindexcontains` custom `go/analysis` linter to `pkg/linters/stringsindexcontains/` that reports `strings.Index(s, substr)` comparisons against `-1` or `0` and suggests replacing them with `strings.Contains(s, substr)` or `!strings.Contains(s, substr)`. The linter will emit `SuggestedFix` text edits to enable automated batch repair. It will be registered in `cmd/linters/main.go` alongside existing analyzers. + +### Alternatives Considered + +#### Alternative 1: Rely on manual code review + +Code reviewers would be expected to flag `strings.Index` containment-check patterns during PR review. This approach requires no new tooling and imposes no maintenance burden. It was not chosen because it is inconsistent — reviewers can miss patterns, especially in large diffs — and it does not help with the 42 existing occurrences already in the codebase. + +#### Alternative 2: Enable an existing third-party linter + +Linters such as `gocritic` (via `sloppyReassign` or similar heuristics) or `staticcheck` cover some idiomatic Go patterns. Using a pre-built linter avoids writing and maintaining custom code. This was not chosen because no widely-adopted third-party linter precisely covers all six operator/literal combinations (`!= -1`, `>= 0`, `> -1`, `== -1`, `< 0`, `<= -1`) with yoda-order variants and automated fix suggestions, and integrating a new external dependency would require vetting and approval across the toolchain. + +### Consequences + +#### Positive +- Automatically detects all six semantic variants of the `strings.Index` containment-check anti-pattern, including yoda-order forms. +- Provides `SuggestedFix` text edits that allow the linter runner to apply fixes automatically, enabling bulk remediation of the 42 known occurrences. +- Follows the established pattern for custom linters in this codebase, making future maintenance and extension straightforward. + +#### Negative +- Adds a new package (`pkg/linters/stringsindexcontains/`) that must be maintained when upstream `go/analysis` APIs or internal utilities change. +- The linter will flag only the specific operator/literal combinations listed; any variant that was intentionally left as `strings.Index` for a non-containment reason (e.g., comparing against a non-`-1`/`0` threshold) is already excluded by design, but edge cases may surface over time. + +#### Neutral +- The linter is registered in `cmd/linters/main.go` alongside all other custom analyzers; it participates in the same execution pipeline with no special orchestration. +- Test fixtures in `pkg/linters/stringsindexcontains/testdata/` document both flagged and acceptable patterns, serving as living documentation of the linter's intent. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/linters b/linters index bb66bb9692d..91c6d678938 100755 Binary files a/linters and b/linters differ diff --git a/pkg/linters/README.md b/pkg/linters/README.md index cbe9bb8b26e..ad4516388a7 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -41,6 +41,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`. +- `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. - `tolowerequalfold` — reports case-insensitive string comparisons using `strings.ToLower`/`ToUpper` that should use `strings.EqualFold`. @@ -89,6 +90,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` | +| `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 | | `tolowerequalfold` | Custom `go/analysis` analyzer that flags case-insensitive comparisons via `strings.ToLower`/`ToUpper` that should use `strings.EqualFold` | @@ -197,6 +199,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/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 - `github.com/github/gh-aw/pkg/linters/tolowerequalfold` — to-lower-equal-fold analyzer subpackage diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 6f12c83cd90..14925b7939c 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -42,6 +42,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/stringsindexcontains" "github.com/github/gh-aw/pkg/linters/timeafterleak" "github.com/github/gh-aw/pkg/linters/timesleepnocontext" "github.com/github/gh-aw/pkg/linters/tolowerequalfold" @@ -62,7 +63,7 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 36 analyzers +// "Public API > Subpackages" table. The README documents 37 analyzers // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // @@ -73,7 +74,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, timeafterleak, timesleepnocontext, +// strconvparseignorederror, stringreplaceminusone, stringsindexcontains, timeafterleak, timesleepnocontext, // tolowerequalfold, uncheckedtypeassertion, wgdonenotdeferred func documentedAnalyzers() []docAnalyzer { return []docAnalyzer{ @@ -108,6 +109,7 @@ func documentedAnalyzers() []docAnalyzer { {"ssljson", ssljson.Analyzer}, {"strconvparseignorederror", strconvparseignorederror.Analyzer}, {"stringreplaceminusone", stringreplaceminusone.Analyzer}, + {"stringsindexcontains", stringsindexcontains.Analyzer}, {"timeafterleak", timeafterleak.Analyzer}, {"timesleepnocontext", timesleepnocontext.Analyzer}, {"tolowerequalfold", tolowerequalfold.Analyzer}, diff --git a/pkg/linters/stringsindexcontains/stringsindexcontains.go b/pkg/linters/stringsindexcontains/stringsindexcontains.go new file mode 100644 index 00000000000..7304246c6e4 --- /dev/null +++ b/pkg/linters/stringsindexcontains/stringsindexcontains.go @@ -0,0 +1,242 @@ +// Package stringsindexcontains implements a Go analysis linter that flags +// 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 the more +// readable strings.Contains(s, substr) or !strings.Contains(s, substr) instead. +package stringsindexcontains + +import ( + "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-index-contains analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "stringsindexcontains", + Doc: "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)", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/stringsindexcontains", + 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, "stringsindexcontains") + + 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 + } + + // Match patterns: + // strings.Index(s, sub) != -1 → strings.Contains(s, sub) + // strings.Index(s, sub) >= 0 → strings.Contains(s, sub) + // strings.Index(s, sub) == -1 → !strings.Contains(s, sub) + // strings.Index(s, sub) < 0 → !strings.Contains(s, sub) + // (and yoda variants: -1 != strings.Index(...), etc.) + + indexCall, negated, matched := matchIndexComparison(pass, expr) + if !matched { + return + } + + if len(indexCall.Args) != 2 { + return + } + + sText := astutil.NodeText(pass.Fset, indexCall.Args[0]) + subText := astutil.NodeText(pass.Fset, indexCall.Args[1]) + pkgText := indexPkgText(pass, indexCall) + if sText == "" || subText == "" || pkgText == "" { + return + } + + var msg string + if negated { + msg = "use !strings.Contains(" + sText + ", " + subText + ") instead of strings.Index comparison" + } else { + msg = "use strings.Contains(" + sText + ", " + subText + ") instead of strings.Index comparison" + } + + fix := buildContainsFix(pass, expr, pkgText, sText, subText, negated) + pass.Report(analysis.Diagnostic{ + Pos: expr.Pos(), + End: expr.End(), + Message: msg, + SuggestedFixes: fix, + }) + }) + + return nil, nil +} + +// matchIndexComparison reports whether expr is a strings.Index comparison with -1 or 0. +// It returns the strings.Index call, whether the result is negated (i.e., checks for absence), +// and whether the pattern matched. +// +// Matched patterns (contains → negated=false): +// - strings.Index(s, sub) != -1 +// - strings.Index(s, sub) >= 0 +// - -1 != strings.Index(s, sub) +// - 0 <= strings.Index(s, sub) +// +// Matched patterns (not-contains → negated=true): +// - strings.Index(s, sub) == -1 +// - strings.Index(s, sub) < 0 +// - -1 == strings.Index(s, sub) +// - 0 > strings.Index(s, sub) +func matchIndexComparison(pass *analysis.Pass, expr *ast.BinaryExpr) (call *ast.CallExpr, negated bool, matched bool) { + // Normalize so that the strings.Index call is on the left side. + left, right, flipped := normalizeOperands(pass, expr) + + indexCall, ok := asStringsIndexCall(pass, left) + if !ok { + return nil, false, false + } + + op := expr.Op + if flipped { + op = flipOp(op) + } + + litVal, ok := constIntValue(pass, right) + if !ok { + return nil, false, false + } + + // Check supported operator/literal combinations. + switch op { + case token.NEQ: + // strings.Index(...) != -1 → contains + if litVal == -1 { + return indexCall, false, true + } + case token.GEQ: + // strings.Index(...) >= 0 → contains + if litVal == 0 { + return indexCall, false, true + } + case token.GTR: + // strings.Index(...) > -1 → contains (less common but valid) + if litVal == -1 { + return indexCall, false, true + } + case token.EQL: + // strings.Index(...) == -1 → !contains + if litVal == -1 { + return indexCall, true, true + } + case token.LSS: + // strings.Index(...) < 0 → !contains + if litVal == 0 { + return indexCall, true, true + } + case token.LEQ: + // strings.Index(...) <= -1 → !contains (less common but valid) + if litVal == -1 { + return indexCall, true, true + } + } + + return nil, false, false +} + +// normalizeOperands returns (left, right) such that if the strings.Index 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 := asStringsIndexCall(pass, expr.X); ok { + return expr.X, expr.Y, false + } + return expr.Y, expr.X, true +} + +// asStringsIndexCall returns the *ast.CallExpr if expr is a call to strings.Index. +func asStringsIndexCall(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 != "Index" { + 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 +} + +// flipOp returns the comparison operator with left and right operands swapped. +func flipOp(op token.Token) token.Token { + switch op { + case token.LSS: + return token.GTR + case token.GTR: + return token.LSS + case token.LEQ: + return token.GEQ + case token.GEQ: + return token.LEQ + default: + return op + } +} + +// indexPkgText returns the package selector text (e.g., "strings") from a strings.Index call. +func indexPkgText(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.Index comparison with strings.Contains", + TextEdits: []analysis.TextEdit{{ + Pos: expr.Pos(), + End: expr.End(), + NewText: []byte(replacement), + }}, + }} +} diff --git a/pkg/linters/stringsindexcontains/stringsindexcontains_test.go b/pkg/linters/stringsindexcontains/stringsindexcontains_test.go new file mode 100644 index 00000000000..2b86a96d363 --- /dev/null +++ b/pkg/linters/stringsindexcontains/stringsindexcontains_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package stringsindexcontains_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/stringsindexcontains" +) + +func TestAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, stringsindexcontains.Analyzer, "stringsindexcontains") +} diff --git a/pkg/linters/stringsindexcontains/testdata/src/stringsindexcontains/stringsindexcontains.go b/pkg/linters/stringsindexcontains/testdata/src/stringsindexcontains/stringsindexcontains.go new file mode 100644 index 00000000000..155f28789fd --- /dev/null +++ b/pkg/linters/stringsindexcontains/testdata/src/stringsindexcontains/stringsindexcontains.go @@ -0,0 +1,66 @@ +package stringsindexcontains + +import "strings" + +func badContains(s, sub string) bool { + return strings.Index(s, sub) != -1 // want `use strings\.Contains\(s, sub\) instead of strings\.Index comparison` +} + +func badContainsGEQ(s, sub string) bool { + return strings.Index(s, sub) >= 0 // want `use strings\.Contains\(s, sub\) instead of strings\.Index comparison` +} + +func badContainsGTR(s, sub string) bool { + return strings.Index(s, sub) > -1 // want `use strings\.Contains\(s, sub\) instead of strings\.Index comparison` +} + +func badNotContains(s, sub string) bool { + return strings.Index(s, sub) == -1 // want `use !strings\.Contains\(s, sub\) instead of strings\.Index comparison` +} + +func badNotContainsLT(s, sub string) bool { + return strings.Index(s, sub) < 0 // want `use !strings\.Contains\(s, sub\) instead of strings\.Index comparison` +} + +func badNotContainsLEQ(s, sub string) bool { + return strings.Index(s, sub) <= -1 // want `use !strings\.Contains\(s, sub\) instead of strings\.Index comparison` +} + +func badYodaContains(s, sub string) bool { + return -1 != strings.Index(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Index comparison` +} + +func badYodaContainsLEQ(s, sub string) bool { + return 0 <= strings.Index(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Index comparison` +} + +func badYodaNotContains(s, sub string) bool { + return -1 == strings.Index(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Index comparison` +} + +func badYodaNotContainsGTR(s, sub string) bool { + return 0 > strings.Index(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Index comparison` +} + +func goodContains(s, sub string) bool { + return strings.Contains(s, sub) +} + +func goodNotContains(s, sub string) bool { + return !strings.Contains(s, sub) +} + +func goodIndexUsedForPosition(s, sub string) int { + // Using the index value itself (not just for containment check) is fine. + return strings.Index(s, sub) +} + +func goodIndexComparesNonMinusOne(s, sub string) bool { + // Comparing against a value other than -1/0 (as a containment sentinel) is fine. + return strings.Index(s, sub) > 3 +} + +func goodIndexEqualZero(s, sub string) bool { + // == 0 is a prefix check (not a containment check) and is intentionally not flagged. + return strings.Index(s, sub) == 0 +}