feat(linters): add stringscountcontains linter#44820
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… comment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR TriageCategory: feature | Risk: low | Priority: low | Score: 30/100 (impact:15, urgency:5, quality:10) | Action: batch_review | Batch: pr-batch:draft-review Adds stringscountcontains linter flagging inefficient strings.Count comparisons. Additive/low-risk. Still a draft; no CI yet. Grouped with other draft PRs for batch review.
|
There was a problem hiding this comment.
Pull request overview
Adds a custom Go analyzer that replaces containment-only strings.Count comparisons with strings.Contains.
Changes:
- Implements diagnostics, suggested fixes, exclusions, and comparison variants.
- Adds analyzer fixtures and documentation.
- Registers the analyzer in the multichecker and documentation tests.
Show a summary per file
| File | Description |
|---|---|
cmd/linters/main.go |
Registers the analyzer. |
pkg/linters/README.md |
Documents the new linter. |
pkg/linters/spec_test.go |
Adds documentation validation. |
pkg/linters/stringscountcontains/stringscountcontains.go |
Implements the analyzer. |
pkg/linters/stringscountcontains/stringscountcontains_test.go |
Runs analyzer tests. |
pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go |
Provides analyzer fixtures. |
pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go.golden |
Defines expected fixes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Medium
| strconvparseignorederror.Analyzer, | ||
| stringreplaceminusone.Analyzer, | ||
| stringsindexcontains.Analyzer, | ||
| stringscountcontains.Analyzer, |
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.RunWithSuggestedFixes(t, testdata, stringscountcontains.Analyzer, "stringscountcontains") |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: stringscountcontains linter
The implementation is correct and follows all established linter conventions in this repo.
Logic verified:
- All 6 canonical patterns (
> 0,>= 1,!= 0,== 0,< 1,<= 0) handled correctly - Yoda-order variants handled via
normalizeOperands+FlipComparisonOp— traced manually, correct nolintdirective support, test-file skip, andSuggestedFixtext edits all present- Registration in
cmd/linters/main.goandspec_test.gois correct; doc count updated 39→40
Minor gap (non-blocking): The testdata covers yoda variants for 5 of the 6 operators but omits the yoda-LEQ case (0 >= strings.Count(s, sub)). The logic handles it correctly, but a test case would complete coverage parity.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 28.1 AIC · ⌖ 5.43 AIC · ⊞ 4.8K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (1 test)
Coverage DetailTest Name: Pattern: Standard go/analysis analyzer test using Scenarios Covered:
Design Contract: The test verifies the Analyzer public API exposes correct diagnostic messages and suggested fixes for all comparison patterns that should be replaced with Verdict✅ APPROVED. 0% implementation tests (threshold: 30%). No violations. Test follows standard go/analysis patterns with comprehensive testdata coverage (13 scenarios). The single TestAnalyzer function is exemplary: it delegates to analysistest.RunWithSuggestedFixes, which validates both diagnostics and fixes against testdata. This avoids unit test inflation (test:prod ratio 0.07:1) while maintaining high behavioral coverage. Pattern is consistent with 40+ other linters in the repository. 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.
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (355 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — 2 small test-coverage gaps found; no blocking issues in the implementation.
📋 Key Themes & Highlights
Issues
- Missing yoda-LEQ test case:
0 >= strings.Count(s, sub)is the 6th yoda variant documented in the PR description but absent from testdata. - No nolint smoke-test: The
//nolint:stringscountcontainssuppression path is wired but never exercised in testdata.
Positive Highlights
- ✅ All 6 comparison operators × 2 orientations (normal + yoda) handled correctly in the implementation
- ✅
SuggestedFixwithTextEditenables automated batch repair — consistent withstringsindexcontains - ✅
normalizeOperands/matchCountComparisondecomposition is clean and readable - ✅
filecheck.IsTestFile+nolint.HasDirectiveguards are in place - ✅ Documentation, README entry, and
spec_test.goregistration are all correct
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 49 AIC · ⌖ 4.62 AIC · ⊞ 6.6K
Comment /matt to run again
| return 0 == strings.Count(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison` | ||
| } | ||
|
|
||
| func badYodaLSS(s, sub string) bool { |
There was a problem hiding this comment.
[/tdd] The yoda-LEQ case (0 >= strings.Count(s, sub)) is missing from testdata — 5 of the 6 yoda variants are covered but this one is absent.
💡 Suggested addition
After line 45 (badYodaLSS), add:
func badYodaLEQ(s, sub string) bool {
return 0 >= strings.Count(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison`
}And its golden counterpart:
func badYodaLEQ(s, sub string) bool {
return !strings.Contains(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison`
}The peer linter stringsindexcontains has a gap of its own (4 yoda cases vs 6), so it is fine to note this is already a step up — but since all 6 operators are documented in the PR description, full testdata coverage would prevent a silent regression if the LEQ case were ever broken.
@copilot please address this.
| return | ||
| } | ||
|
|
||
| countCall, negated, matched := matchCountComparison(pass, expr) |
There was a problem hiding this comment.
[/tdd] The //nolint:stringscountcontains directive is documented as supported but there is no testdata case exercising it — a future refactor could silently break nolint suppression.
💡 Suggested testdata addition
Add a case in testdata/src/stringscountcontains/stringscountcontains.go:
func nolintCase(s, sub string) bool {
return strings.Count(s, sub) > 0 (nolint/redacted):stringscountcontains
}This function should produce no diagnostic, confirming the nolint path is exercised.
@copilot please address this.
There was a problem hiding this comment.
Three gaps need fixing before merge: a missing testdata case, a broken normalizeOperands contract, and an untested suppression path.
Findings summary
Medium — missing badYodaLEQ test case
0 >= strings.Count(s, sub) is the only yoda variant not covered by testdata, leaving the token.LEQ flip path unverified. All other yoda variants have tests; this inconsistency is an explicit gap.
Medium — normalizeOperands contract is broken
When neither side of the binary expression is a strings.Count call, the function returns (expr.Y, expr.X, true) — flipped=true despite Count being absent. The caller's early-return saves us today, but the invariant is wrong and will silently mislead future readers or callers.
Medium — //nolint:stringscountcontains suppression has no test
run() calls nolint.HasDirective but no testdata case asserts it suppresses a diagnostic. A regression here would be invisible.
Low — diagnostic message/fix disagree for aliased imports
pkgText drives the fix (correct), but the message hardcodes strings.Contains. Minor but visibly inconsistent in tooling output.
🔎 Code quality review by PR Code Quality Reviewer · 64.3 AIC · ⌖ 4.83 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go:47
Missing test for badYodaLEQ — the only documented yoda variant without coverage.
<details>
<summary>💡 Details and suggested fix</summary>
The PR comment in normalizeOperands lists 0 >= strings.Count(s, sub) as a pattern (via 1 > strings.Count above it), but neither the testdata nor the golden file includes it. When flipped, GEQ → LEQ with litVal=0 should hit the token.LEQ branch and return negated=true. Without a test case, a regression here would be silent.
Add to the…
pkg/linters/stringscountcontains/stringscountcontains.go:255
normalizeOperands returns flipped=true even when neither operand is a strings.Count call — the contract is silently wrong.
<details>
<summary>💡 Details and suggested fix</summary>
When expr.X is not a Count call, the function unconditionally returns (expr.Y, expr.X, true) — including when expr.Y is also not a Count call. The caller does check asStringsCountCall on the returned left and returns early if it fails, so no wrong diagnostic is emitted today. But flipped=true …
pkg/linters/stringscountcontains/stringscountcontains_test.go:15
No test for //nolint:stringscountcontains suppression — a documented feature with zero coverage.
<details>
<summary>💡 Details and suggested fix</summary>
run() calls nolint.HasDirective to suppress diagnostics, but no testdata case exercises this path. If nolint.BuildLineIndex or HasDirective silently breaks for this linter name, it goes undetected.
Add a testdata function with a //nolint:stringscountcontains annotation that should produce no diagnostic:
func suppress…
</details>
<details><summary>pkg/linters/stringscountcontains/stringscountcontains.go:163</summary>
**Diagnostic message hardcodes `strings.Contains` but the fix uses the actual import alias — they diverge for aliased imports.**
<details>
<summary>💡 Details and suggested fix</summary>
`countPkgText` returns the local identifier (e.g., `str` for `import str "strings"`). The `SuggestedFix` correctly uses `pkgText`, producing `str.Contains(...)`. But the diagnostic message is hardcoded:
```go
msg = fmt.Sprintf("use strings.Contains(%s, %s) instead ...", sText, subText)For an aliased im…
Adds a new
stringscountcontainscustom Go analysis linter that flagsstrings.Count(s, sub)comparisons with0or1used purely as containment checks.strings.Countwalks the entire string whilestrings.Containsshort-circuits on first match — making the latter both clearer in intent and more efficient.Flagged patterns → suggested fix
Changes
pkg/linters/stringscountcontains/— new analyzer package; emitsSuggestedFixtext edits for automated batch repair; respects//nolint:stringscountcontainsdirectives; skips_test.gofilesstringscountcontains_test.go— usesanalysistest.RunWithSuggestedFixeswith a.go.goldenfile, consistent with other fix-emitting linterscmd/linters/main.go— registered in the multichecker binarypkg/linters/README.md— documented in overview list, subpackages table, and import paths sectionpkg/linters/spec_test.go— added todocumentedAnalyzers()(39 → 40 documented analyzers)