[linter-miner] linter: add stringsindexcontains — flag strings.Index comparisons that should use strings.Contains#43253
Conversation
Add a new custom go/analysis linter that flags strings.Index(s, sub) comparisons with -1 or 0 that should use the more readable strings.Contains(s, sub) (or !strings.Contains for the negated form). Pattern caught: 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) == -1 → !strings.Contains(s, sub) strings.Index(s, sub) < 0 → !strings.Contains(s, sub) strings.Index(s, sub) <= -1 → !strings.Contains(s, sub) (plus yoda-order variants) Evidence: 42 occurrences of this pattern were found in the pkg/ and cmd/ directories of this repository during the automated code scan, confirming it is a recurring pattern worth linting. The linter provides SuggestedFix text edits for automated repair. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Great work from the Linter Miner workflow! 🎉 The
This looks ready for maintainer review! 🚀 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom go/analysis analyzer to improve readability by replacing common strings.Index(...) containment checks with strings.Contains(...) (or its negation), and wires it into the cmd/linters multichecker so it runs with the rest of gh-aw’s linters.
Changes:
- Implemented
pkg/linters/stringsindexcontainsanalyzer withSuggestedFixrewrites for supported comparison patterns. - Added analysistest-based unit test and testdata fixtures for the new analyzer.
- Registered the analyzer in
cmd/linters/main.go.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/stringsindexcontains/stringsindexcontains.go |
New analyzer implementation to detect strings.Index comparisons that can be simplified to strings.Contains. |
pkg/linters/stringsindexcontains/stringsindexcontains_test.go |
Test harness for running the analyzer via analysistest. |
pkg/linters/stringsindexcontains/testdata/src/stringsindexcontains/stringsindexcontains.go |
Test fixtures that assert reported diagnostics (and indirectly validate suggested fixes). |
cmd/linters/main.go |
Registers the new analyzer in the multichecker binary. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Low
| // Analyzer is the strings-index-contains analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "stringsindexcontains", | ||
| Doc: "reports strings.Index(s, substr) comparisons with -1 that should use strings.Contains(s, substr)", | ||
| URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/stringsindexcontains", | ||
| Requires: []*analysis.Analyzer{inspect.Analyzer}, | ||
| Run: run, | ||
| } |
| func goodIndexComparesNonMinusOne(s, sub string) bool { | ||
| // Comparing against a value other than -1/0 is fine. | ||
| return strings.Index(s, sub) > 3 | ||
| } |
| sprintfint.Analyzer, | ||
| strconvparseignorederror.Analyzer, | ||
| stringreplaceminusone.Analyzer, | ||
| stringsindexcontains.Analyzer, | ||
| jsonmarshalignoredeerror.Analyzer, |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
🔍 Test Analysis
Uses Build tag Testdata coverage (10 cases):
The four missing cases correspond to operator branches in Verdict
References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (302 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on test coverage gaps and documentation accuracy.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: The
> -1(GTR) and<= -1(LEQ) operator branches have no fixture tests; named-constant patterns are also untested. These code paths could regress silently. - Documentation inaccuracy:
Analyzer.Docand the inline comment block both undercount the number of supported patterns, making the spec harder to reason about. - Minor interface noise:
buildContainsFixcarries an unusedpassparameter.
Positive Highlights
- ✅ Solid pattern decomposition:
normalizeOperands+flipOpcleanly handles yoda-order comparisons - ✅
constIntValuecorrectly uses the type-checker rather than syntactic literal matching — handles aliased constants robustly - ✅ Follows established linter conventions: nolint directives, test-file skipping,
SuggestedFixemission - ✅ Well-documented godoc on
matchIndexComparisonlisting all matched patterns clearly
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 80.7 AIC · ⌖ 5.63 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, stringsindexcontains.Analyzer, "stringsindexcontains") |
There was a problem hiding this comment.
[/tdd] The > -1 (GTR) and <= -1 (LEQ) patterns implemented in matchIndexComparison have no corresponding testdata fixtures — these code paths are untested and could regress silently.
💡 Suggested fixture additions
Add to testdata/src/stringsindexcontains/stringsindexcontains.go:
func badGTRMinusOne(s, sub string) bool {
return strings.Index(s, sub) > -1 // want `use strings\.Contains\(s, sub\) instead of strings\.Index comparison`
}
func badLEQMinusOne(s, sub string) bool {
return strings.Index(s, sub) <= -1 // want `use !strings\.Contains\(s, sub\) instead of strings\.Index comparison`
}Similarly, the yoda-order variants for 0 <= strings.Index(...) and 0 > strings.Index(...) are documented in the function godoc but have no fixture coverage.
@copilot please address this.
| // Analyzer is the strings-index-contains analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "stringsindexcontains", | ||
| Doc: "reports strings.Index(s, substr) comparisons with -1 that should use strings.Contains(s, substr)", |
There was a problem hiding this comment.
[/grill-with-docs] The Doc field says "comparisons with -1" but the linter also catches >= 0 and < 0 comparisons (and > -1 / <= -1). The description is misleading and will confuse users checking go vet -json output.
💡 Suggested fix
Doc: "reports strings.Index(s, substr) comparisons with -1 or 0 that should use strings.Contains(s, substr)",@copilot please address this.
| } | ||
|
|
||
| // 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 { |
There was a problem hiding this comment.
[/codebase-design] buildContainsFix accepts pass *analysis.Pass but never uses it. The unused parameter adds noise to the interface and suggests the function signature was copied from a similar helper that did use pass.
💡 Suggested fix
Remove the pass parameter since all needed information is already provided via the explicit text arguments:
func buildContainsFix(expr *ast.BinaryExpr, pkgText, sText, subText string, negated bool) []analysis.SuggestedFix {Update the call site at line 81 accordingly.
@copilot please address this.
| // 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.) |
There was a problem hiding this comment.
[/grill-with-docs] The inline comment block only lists 4 of the 6 supported patterns — it omits strings.Index(s, sub) > -1 and strings.Index(s, sub) <= -1. A reader relying on this comment as spec would miss two valid patterns.
💡 Suggested fix
// 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) == -1 → !strings.Contains(s, sub)
// strings.Index(s, sub) < 0 → !strings.Contains(s, sub)
// strings.Index(s, sub) <= -1 → !strings.Contains(s, sub)
// (and yoda variants: -1 != strings.Index(...), etc.)@copilot please address this.
| func goodIndexComparesNonMinusOne(s, sub string) bool { | ||
| // Comparing against a value other than -1/0 is fine. | ||
| return strings.Index(s, sub) > 3 | ||
| } |
There was a problem hiding this comment.
[/tdd] The test fixtures cover "good" cases where strings.Index is used to retrieve a position or compared against a non-sentinel value — but there is no test for a named constant like const notFound = -1; strings.Index(s, sub) == notFound. Other linters in this codebase (e.g. stringreplaceminusone) explicitly test named-constant variants.
💡 Suggested fixture addition
const notFound = -1
func badNamedConst(s, sub string) bool {
\treturn strings.Index(s, sub) == notFound // want `use !strings\.Contains\(s, sub\) instead of strings\.Index comparison`
}This confirms that constIntValue correctly resolves named constants (not just literals), matching the behavior documented via constant.Int64Val.
@copilot please address this.
There was a problem hiding this comment.
Review: stringsindexcontains linter
The implementation is clean, well-structured, and follows the repo's linter conventions. The logic for matching operator/literal combinations (including yoda variants) is correct. Two non-blocking suggestions and one blocking gap:
Blocking
- Missing test fixtures (comment on test file, line 14): The operator table in the implementation's doc comment covers 6 operators × 2 yoda variants = 12 logical patterns, but the fixture only tests 6 of them. The
> -1,<= -1, yoda-GEQ, and yoda-LSS branches have no test coverage. This should be filled in before merge.
Non-blocking
- Unused
passparameter inbuildContainsFix(line 225): The parameter is never referenced in the body; removing it reduces noise. normalizeOperands"no match" semantic (line 166): When neither operand is an Index call the function returns(Y, X, flipped=true), which is harmless because the caller immediately re-checks, but the invariant is surprising. A brief doc note would help future readers.
Patterns with no test fixture coverage
| Pattern | Operator | Expected replacement |
|---|---|---|
strings.Index(s, sub) > -1 |
GTR / -1 | strings.Contains(s, sub) |
strings.Index(s, sub) <= -1 |
LEQ / -1 | !strings.Contains(s, sub) |
0 <= strings.Index(s, sub) |
yoda-GEQ | strings.Contains(s, sub) |
0 > strings.Index(s, sub) |
yoda-LSS | !strings.Contains(s, sub) |
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 83.5 AIC · ⌖ 6.45 AIC · ⊞ 4.9K
| } | ||
|
|
||
| // 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 { |
There was a problem hiding this comment.
The pass *analysis.Pass parameter is declared but never used in this function — all the needed data (the replacement text and AST position) is already passed in as pre-computed strings and the expr node. Consider removing it to keep the signature minimal:
func buildContainsFix(expr *ast.BinaryExpr, pkgText, sText, subText string, negated bool) []analysis.SuggestedFix {And update the call site at line ~82 accordingly.
@copilot please address this.
|
|
||
| // 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) { |
There was a problem hiding this comment.
The pass *analysis.Pass parameter is passed through here but is only used by the inner asStringsIndexCall call. This is fine structurally, but it's worth noting that normalizeOperands has a subtle issue: when neither operand is a strings.Index call, it returns (expr.Y, expr.X, true) — putting the right side as the left result with flipped=true. The caller in matchIndexComparison then immediately calls asStringsIndexCall(pass, left) on that value, which will correctly return false. But the semantic is slightly confusing: "flipped" should mean "we found Index on the right", yet here it can also mean "we found nothing". Consider renaming the return value or adding a doc comment clarifying the "no match" case to prevent future misuse.
@copilot please address this.
| ) | ||
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() |
There was a problem hiding this comment.
The test coverage is minimal — only 6 bad cases and 4 good cases. Missing coverage for the patterns listed in the PR description and in the doc comment of matchIndexComparison:
strings.Index(s, sub) > -1(GTR / -1 → contains)strings.Index(s, sub) <= -1(LEQ / -1 → not-contains)- Yoda variants for GEQ (
0 <= strings.Index(...)) and LSS (0 > strings.Index(...))
Please add fixtures for these cases to testdata/src/stringsindexcontains/stringsindexcontains.go so the full operator coverage documented in the implementation is actually tested.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — Two issues must be addressed before merging; one structural nit is non-blocking.
🔍 Summary of findings
Blocking
-
Suggested fixes are unvalidated (
stringsindexcontains_test.go:15) — The test usesanalysistest.Runinstead ofanalysistest.RunWithSuggestedFixes. TheSuggestedFixtext edits inbuildContainsFix— the linter's main automated-repair value — are never applied or checked. Every other fix-emitting linter in this repo (ctxbackground,lenstringsplit,lenstringzero,fprintlnsprintf,execcommandwithoutcontext) usesRunWithSuggestedFixesplus.goldenfiles. Fix: swap the call and add a golden file. -
Two supported patterns have zero test coverage (
testdata/.../stringsindexcontains.go:45) —strings.Index(s, sub) > -1(GTR case) andstrings.Index(s, sub) <= -1(LEQ case) are handled inmatchIndexComparisonand listed in the PR description table, but no// wantannotation exercises them. These branches can silently regress.
Non-blocking
normalizeOperandsforces a redundantasStringsIndexCalltraversal (stringsindexcontains.go:166) — Minor design issue: the call expr is discarded and immediately re-looked-up. Refactoring to return*ast.CallExprwould be cleaner, but this does not affect correctness or performance in any meaningful way at linter scale.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 225.2 AIC · ⌖ 9.29 AIC · ⊞ 5.4K
Comment /review to run again
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, stringsindexcontains.Analyzer, "stringsindexcontains") |
There was a problem hiding this comment.
Suggested fixes are emitted but never validated — this is the primary value of the linter and it is completely untested.
The test calls analysistest.Run but the linter emits SuggestedFixes. analysistest.Run only checks that diagnostics fire on the expected lines; it never applies or validates the fix text edits. Every other linter in this repo that emits fixes uses RunWithSuggestedFixes with matching .golden files (e.g., ctxbackground, lenstringsplit, lenstringzero, fprintlnsprintf, execcommandwithoutcontext).
Given the stated purpose — automated repair of 42 occurrences — silently shipping unvalidated fix text is a serious quality gap.
💡 How to fix
- Change
analysistest.Run→analysistest.RunWithSuggestedFixesin the test. - Create a
.goldenfile undertestdata/src/stringsindexcontains/stringsindexcontains.go.goldencontaining the expected post-fix source.
// stringsindexcontains_test.go
func TestAnalyzer(t *testing.T) {
testdata := analysistest.TestData()
analysistest.RunWithSuggestedFixes(t, testdata, stringsindexcontains.Analyzer, "stringsindexcontains")
}Without this change, buildContainsFix could produce incorrect replacements (wrong parenthesization, wrong package alias) and all tests would still pass.
| func goodIndexComparesNonMinusOne(s, sub string) bool { | ||
| // Comparing against a value other than -1/0 is fine. | ||
| return strings.Index(s, sub) > 3 | ||
| } |
There was a problem hiding this comment.
> -1 and <= -1 patterns are in matchIndexComparison but have zero test coverage — they can silently regress.
matchIndexComparison handles token.GTR / litVal==-1 (→ strings.Index(s,sub) > -1) and token.LEQ / litVal==-1 (→ strings.Index(s,sub) <= -1), but no testdata fixture or // want annotation exercises these two cases. They also appear in the PR description table as supported patterns. If either branch is broken today — or broken by a future refactor — the tests will not catch it.
💡 Suggested additions to testdata
func badContainsGTR(s, sub string) bool {
return strings.Index(s, sub) > -1 // 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`
}And the corresponding yoda variants if the PR claims to support them.
|
|
||
| // 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) { |
There was a problem hiding this comment.
normalizeOperands discards the *ast.CallExpr and forces asStringsIndexCall to re-traverse the same node immediately after.
normalizeOperands calls asStringsIndexCall to decide which side is the index call, returns expr.X/expr.Y (raw ast.Expr), and then matchIndexComparison immediately calls asStringsIndexCall again on the returned left. The same type assertion and selector walk runs twice on every binary expression that has a strings.Index call on either side.
💡 Suggested fix
Return the *ast.CallExpr from normalizeOperands to avoid redundant work:
func normalizeOperands(pass *analysis.Pass, expr *ast.BinaryExpr) (indexCall *ast.CallExpr, right ast.Expr, flipped bool, ok bool) {
if call, found := asStringsIndexCall(pass, expr.X); found {
return call, expr.Y, false, true
}
if call, found := asStringsIndexCall(pass, expr.Y); found {
return call, expr.X, true, true
}
return nil, nil, false, false
}This is a minor structural issue, not blocking correctness, but the current design exists only because the return type was chosen poorly.
|
@copilot please run the
|
…EADME, spec - Update Analyzer.Doc and package doc comment to describe all six supported operator/literal patterns (> -1, <= -1, yoda variants) not just comparisons with -1 - Add missing testdata fixtures for > -1, <= -1, 0 <= (yoda), 0 > (yoda) patterns to prevent regressions - Fix misleading comment about index==0 comparisons: clarify it is a prefix check and is intentionally not flagged - Add stringsindexcontains to pkg/linters/README.md (description list, subpackages table, and dependencies section) - Add stringsindexcontains to pkg/linters/spec_test.go (import, documentedAnalyzers slice, and count comment updated 36→37) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all review feedback in commit
Local validation: all |
Summary
Adds a new
stringsindexcontainscustomgo/analysislinter topkg/linters/that flagsstrings.Index(s, substr)comparisons used as containment checks and rewrites them tostrings.Containsor!strings.Contains. The linter was motivated by 42 existing occurrences of the anti-pattern in the codebase and follows the establishedgo/analysispass pattern already used bystringreplaceminusone,lenstringzero, and similar analyzers. It emitsSuggestedFixtext edits to enable automated bulk remediation.Changes
Implementation
pkg/linters/stringsindexcontains/stringsindexcontains.go(new, 242 lines) — Fullgo/analysisanalyzer:Analyzerwith name, doc, URL, andinspect.Analyzerdependency.run()walks*ast.BinaryExprnodes; skips test files and//nolint:stringsindexcontainsdirectives.matchIndexComparison()normalizes yoda-order operands, identifiesstrings.Indexcalls viaIsPkgSelector, reads constant integer values frompass.TypesInfo, and matches all six operator/literal combos:!= -1,>= 0,> -1(contains) and== -1,< 0,<= -1(not-contains).buildContainsFix()emits aSuggestedFixrewriting the entire binary expression to[!]strings.Contains(s, substr).cmd/linters/main.go— Added import and registeredstringsindexcontains.Analyzerin the multi-analyzer runner.Tests
pkg/linters/stringsindexcontains/stringsindexcontains_test.go(new) — Standardanalysistest.Runharness.pkg/linters/stringsindexcontains/testdata/src/stringsindexcontains/stringsindexcontains.go(new, 66 lines) — Test fixture with 10// wantcases (all 6 operators + 4 yoda-order variants) and 5 intentional non-flagged cases: directstrings.Contains, index used for position (no comparison), comparison against non-sentinel (> 3), and== 0(prefix check).pkg/linters/spec_test.go— AddedstringsindexcontainstodocumentedAnalyzers(); updated documented analyzer count from 36 to 37.Docs / Infra
pkg/linters/README.md— Addedstringsindexcontainsto the description list, the subpackages table, and the package list; updated count from 36 to 37.docs/adr/43253-add-stringsindexcontains-linter.md(new) — Draft ADR documenting context, decision, alternatives considered (manual review, third-party linters), and consequences.linters(binary) — Recompiled binary updated.Design notes
normalizeOperandsswaps operands and setsflipped=true;flipOpinverts the comparison token so the switch table stays canonical (index on left, literal on right).== 0intentionally excluded:strings.Index(s, sub) == 0is a prefix check, not a containment check, so it is not flagged.astutil.IsPkgSelector(type-info-based) rather than string matching to avoid false positives from shadowedstringsidentifiers.filecheck.IsTestFilegates the visitor, keeping the linter focused on production code.Testing
The
analysistestfixture covers all six canonical operator/literal combinations and four yoda-order variants as positive cases, plus five negative cases verifying that legitimatestrings.Indexuses (position lookup, non-sentinel comparisons, prefix check with== 0) are not flagged.