[linter-miner] feat(linters): add stringscutprefix linter#48210
[linter-miner] feat(linters): add stringscutprefix linter#48210github-actions[bot] wants to merge 7 commits into
Conversation
Detects if-blocks that check strings.HasPrefix(s, p) then call strings.TrimPrefix(s, p) with the same arguments inside the body, and suggests using strings.CutPrefix (available since Go 1.20) instead. strings.CutPrefix is cleaner: it returns (after, found) in a single call, eliminating the redundant HasPrefix check and the risk of the prefix and variable getting out of sync between the two calls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds a Go analyzer detecting strings.HasPrefix followed by matching strings.TrimPrefix calls.
Changes:
- Implements
stringscutprefix. - Adds fixture-based analyzer tests.
- Registers the analyzer globally.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/stringscutprefix/stringscutprefix.go |
Implements detection and expression matching. |
pkg/linters/stringscutprefix/stringscutprefix_test.go |
Runs analyzer fixtures. |
pkg/linters/stringscutprefix/testdata/src/stringscutprefix/stringscutprefix.go |
Provides positive and negative cases. |
pkg/linters/registry.go |
Registers the analyzer. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/linters/stringscutprefix/stringscutprefix.go:141
- Comparing literal source text misses semantically equal prefixes such as
"foo"versus a raw-stringfoo, or"\x66oo"versus"foo", despite the promised value-based matching. Compare the type-checker's constant values instead, and add a fixture with alternate literal spellings.
return ok && av.Kind == bv.Kind && av.Value == bv.Value
pkg/linters/stringscutprefix/stringscutprefix.go:129
- Parentheses are not normalized, so equivalent arguments such as
HasPrefix((s), "foo")andTrimPrefix(s, "foo")are missed. UnwrapParenExpron either input before comparing and add a fixture for this form.
func sameExpr(pass *analysis.Pass, a, b ast.Expr) bool {
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
| stringsconcatloop.Analyzer, | ||
| stringsindexcontains.Analyzer, | ||
| stringsindexhasprefix.Analyzer, | ||
| stringscutprefix.Analyzer, |
There was a problem hiding this comment.
Already addressed in d63b6b4: /home/runner/work/gh-aw/gh-aw/pkg/linters/doc.go, /home/runner/work/gh-aw/gh-aw/pkg/linters/README.md, and /home/runner/work/gh-aw/gh-aw/pkg/linters/spec_test.go now include stringscutprefix, and TestDocSurfacesMatchRegistryAndSpecList passes locally.
| // to strings.TrimPrefix with arguments that textually match s and prefix. | ||
| func bodyContainsTrimPrefix(pass *analysis.Pass, body *ast.BlockStmt, s, prefix ast.Expr) bool { | ||
| found := false | ||
| ast.Inspect(body, func(n ast.Node) bool { |
There was a problem hiding this comment.
Already addressed in d63b6b4: /home/runner/work/gh-aw/gh-aw/pkg/linters/stringscutprefix/stringscutprefix.go now restricts matches to the direct statement shape, skips nested flow/closures, and stops after reassignment, and /home/runner/work/gh-aw/gh-aw/pkg/linters/stringscutprefix/testdata/src/stringscutprefix/stringscutprefix.go covers the negative cases. make test-unit passes locally for the linter packages.
🧪 Test Quality Sentinel Report❌ Test Quality Score: 40/100 — Poor
📊 Metrics (1 test)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Test Quality Sentinel: 40/100. Hard violation: missing required //go:build !integration tag on line 1 of stringscutprefix_test.go. This is a coding guideline violation that must be resolved per repository standards. Review flagged tests in the comment above.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (215 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.
Review: stringscutprefix linter
Overall the implementation is correct and well-structured. Two non-blocking issues worth addressing:
-
Closure traversal (false positives) - bodyContainsTrimPrefix uses ast.Inspect which descends into FuncLit nodes. A strings.TrimPrefix call inside an inline closure in the if body will trigger the diagnostic even if the closure is never invoked. See inline comment at line 95.
-
Dead fallback in sameExpr - The a.Pos() == b.Pos() fallback will never return true for two distinct subexpressions (they cannot share a position), so it misleads rather than helps. See inline comment at line 154.
Neither blocks merging the linter behaves correctly for all common cases but fixing them improves correctness and code clarity.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 32 AIC · ⌖ 4.73 AIC · ⊞ 5K
There was a problem hiding this comment.
REQUEST_CHANGES — Three correctness issues must be fixed before this linter ships.
### Blocking issues
-
False positives on nested calls (line 113): uses which recurses into all nested blocks. If is inside a nested /, the diagnostic fires but the suggested fix (swap to ) is structurally incorrect. Only top-level occurrences in qualify.
-
** position fallback always returns false** (line 182): The fallback
a.Pos() == b.Pos()for unhandled expression types (calls, unary, binary) compares positions that are always different between the args and the args — silently producing false negatives for any complex argument expression. Replace with an explicitreturn falseand a comment. -
** selector compared by name string, not type identity** (line 173):
av.Sel.Name == bv.Sel.Namecan match unrelated fields with the same name on different types. UseTypesInfo.ObjectOf(av.Sel) == TypesInfo.ObjectOf(bv.Sel), consistent with the*ast.Identbranch.
### Non-blocking
- in is never consumed via ; if needs it, that dependency belongs to 's analyzer, not here.
- Test data has no case for nested (should be a non-flagged case) or complex argument expressions.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 43.2 AIC · ⌖ 5.02 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/linters/stringscutprefix/stringscutprefix.go:113
False positives when TrimPrefix appears in nested statements: bodyContainsTrimPrefix walks the entire subtree recursively via ast.Inspect, so it flags the outer if even when TrimPrefix is inside a nested if/for. The suggested replacement (strings.CutPrefix) only works as a direct swap when TrimPrefix is at the top level of the block.
<details>
<summary>💡 Explanation and fix</summary>
This pattern would be incorrectly flagged:
if strings.HasPrefix(s, "foo") {
…
</details>
<details><summary>pkg/linters/stringscutprefix/stringscutprefix.go:182</summary>
**`sameExpr` position-equality fallback is a dead no-op that silently produces false negatives**: For expression types not handled by the switch (e.g., `*ast.CallExpr`, `*ast.UnaryExpr`, `*ast.BinaryExpr`), the fallback compares `a.Pos() == b.Pos()`. The `s` and `prefix` arguments come from the `HasPrefix` call; the matching arguments inside the body come from a `TrimPrefix` call at a different source location — they will never share a `Pos()`. This always returns `false`, silently missing patt…
</details>
<details><summary>pkg/linters/stringscutprefix/stringscutprefix.go:173</summary>
**`SelectorExpr` comparison uses string name equality for the selector field, not type identity**: `av.Sel.Name == bv.Sel.Name` can produce false positives when two different types have a field with the same name and both receivers resolve to the same object (e.g., promoted fields from embedded structs).
<details>
<summary>💡 Suggested fix</summary>
Compare using `TypesInfo.ObjectOf` for the selector, consistent with the `*ast.Ident` case:
```go
case *ast.SelectorExpr:
bv, ok := b.(*ast.…
</details>
<details><summary>pkg/linters/stringscutprefix/stringscutprefix.go:50</summary>
**`inspect.Analyzer` is listed as a dependency but never used**: `pass.ResultOf[inspect.Analyzer]` is never called in `run`. The dependency declaration forces the analysis framework to run the inspect pass for this package unnecessarily.
<details>
<summary>💡 Fix</summary>
Remove `inspect.Analyzer` from `Requires` — `astutil.Root` presumably handles its own dependency. If `astutil.Root` internally uses the inspect pass result, that dependency should be declared by `astutil`'s underlying analy…
</details>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on two correctness issues.
📋 Key Themes & Highlights
Issues
- False positive via closure (line 89):
bodyContainsTrimPrefixwalks into nested*ast.FuncLitnodes. ATrimPrefixcall inside a closure is not equivalent to a direct call in theifbody; stop traversal atFuncLitboundaries (consistent withmanualmutexunlockand other linters here). - Dead-code fallback in
sameExpr(line 156): thea.Pos() == b.Pos()check is alwaysfalsefor arguments coming from two distinct call expressions. Replace it with a plainreturn falseso intent is clear.
Positive Highlights
- ✅ Solid use of type-aware matching (
pass.TypesInfo.ObjectOf) to avoid string-comparison false positives on locally-renamedstringsimports - ✅ Correct infrastructure wiring: nolint directives, generated-file skipping,
inspect.Analyzerdependency - ✅ Good fixture coverage for field selectors and mismatched arguments
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 45.7 AIC · ⌖ 4.95 AIC · ⊞ 6.7K
Comment /matt to run again
|
@copilot please run the Unresolved review threads (newest first):
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved code feedback in d63b6b4. The analyzer now avoids closure/nested-branch/reassignment false positives, the negative fixtures and required build tag are in place, and the doc/spec surfaces are synced. Local validation passed ( |
|
@copilot please run the
|
|
@copilot please run the
|
Branch is up to date on I reran the local checks on the current head:
I also checked the recent workflow state: the latest branch-attached run is the current Copilot run, and the referenced PR Sous Chef run ( No additional |
|
@copilot please run the Unresolved review threads:
|
Branch is up to date on Of the seven threads listed here, five are already resolved in GitHub. The two still-open Copilot reviewer threads are already satisfied by the current branch, and I replied on those threads with the exact locations of the fixes in Local validation on the current head:
Recent CI state I could inspect in this pass:
No additional |
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed one more in-scope follow-up in Local checks on this head:
I also inspected the last-known CI state with GitHub Actions tools: the recent |
|
@copilot please run the
|
No branch update was needed: the current head I rechecked the current branch state and the
I also inspected the recent branch workflow runs with the Actions MCP tools: the For the |
|
@copilot please run the
|
I rechecked the current branch and the blocking I reran the relevant local checks on this head:
No additional linter code changes were needed in this pass. CI on the PR head is still unverified/stale until a maintainer re-triggers it. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in |
github-actions
Bot
added
the
closed:ci-failure
Summary
Adds a new custom linter
stringscutprefixthat detects a common Go anti-pattern: usingstrings.HasPrefix(s, p)as anifcondition and then callingstrings.TrimPrefix(s, p)with the same arguments inside the body.What it catches
strings.CutPrefix(available since Go 1.20) is cleaner:(after, found)— no risk of the prefix literal or variable getting out of sync betweenHasPrefixandTrimPrefixEvidence
This pattern appears repeatedly in Go codebases. The
strings.CutPrefix/strings.CutSuffixfunctions were introduced precisely to replace this idiom, but many projects have not migrated older code. A linter enforces the migration automatically.Implementation
pkg/linters/stringscutprefix/stringscutprefix*ast.IfStmtwhose condition isstrings.HasPrefix(s, p)and whose body containsstrings.TrimPrefix(s, p)with matching arguments (identifiers by object identity, literals by value, selector expressions recursively)pkg/linters/registry.goanalysistestRun: https://github.com/github/gh-aw/actions/runs/30218624846
Run: https://github.com/github/gh-aw/actions/runs/30221620246
Run: https://github.com/github/gh-aw/actions/runs/30223671533
Run: https://github.com/github/gh-aw/actions/runs/30224763533
Run: https://github.com/github/gh-aw/actions/runs/30228781602
Run: https://github.com/github/gh-aw/actions/runs/30230557448
Run: https://github.com/github/gh-aw/actions/runs/30233154470
Run: https://github.com/github/gh-aw/actions/runs/30236419434