Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/linters/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ This package currently provides custom Go analyzers in the following subpackages
- `stringsconcatloop` — reports `string +=` concatenation inside `for`/`range` loop bodies, which allocates a new string copy on every iteration (O(n²) memory); use `strings.Builder` instead.
- `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.
- `stringsindexhasprefix` — reports `strings.Index(s, sub)` comparisons with `0` (`== 0`, `!= 0`) and their yoda-order variants that should use `strings.HasPrefix(s, sub)` or `!strings.HasPrefix(s, sub)` instead.
- `stringsjoinone` — reports `strings.Join([]string{s}, sep)` calls with a single-element slice literal where the separator is never used and the call is equivalent to just `s`.
- `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.
Expand Down Expand Up @@ -121,6 +122,7 @@ This package currently provides custom Go analyzers in the following subpackages
| `stringsconcatloop` | Custom `go/analysis` analyzer that flags `string +=` concatenation inside `for`/`range` loops that should use `strings.Builder` |
| `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` |
| `stringsindexhasprefix` | Custom `go/analysis` analyzer that flags `strings.Index(s, sub)` comparisons with `0` (`== 0`, `!= 0`) that should use `strings.HasPrefix` or `!strings.HasPrefix` |
| `stringsjoinone` | Custom `go/analysis` analyzer that flags `strings.Join([]string{s}, sep)` calls with a single-element slice literal where the separator is unused and the call is equivalent to just `s` |
| `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 |
Expand Down Expand Up @@ -250,6 +252,7 @@ _ = trimleftright.Analyzer
- `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/stringsindexhasprefix` — strings-index-has-prefix analyzer subpackage
- `github.com/github/gh-aw/pkg/linters/stringsjoinone` — strings-join-one 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
Expand Down
3 changes: 2 additions & 1 deletion pkg/linters/doc.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Package linters is a namespace for gh-aw's custom Go analysis linters.
//
// All 58 active analyzers:
// All 59 active analyzers:
//
// - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...)
// - appendoneelement — flags append(s, []T{x}...) calls where a single-element slice literal is spread and can be simplified to append(s, x)
Expand Down Expand Up @@ -51,6 +51,7 @@
// - stringsconcatloop — flags string += concatenation inside for/range loops that should use strings.Builder
// - 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)
// - stringsindexcontains — flags strings.Index(s, substr) comparisons that should use strings.Contains
// - stringsindexhasprefix — reports strings.Index(s, sub) comparisons with 0 (== 0 and != 0) and their yoda-order variants that should use strings.HasPrefix(s, sub) or !strings.HasPrefix(s, sub)
// - stringsjoinone — flags strings.Join([]string{s}, sep) calls with a single-element slice literal where the separator is unused and the call is equivalent to just s
// - timeafterleak — flags time.After in select cases inside loops that leak timer channels
// - timesleepnocontext — flags time.Sleep calls in context-aware functions that should propagate cancellation
Expand Down
12 changes: 12 additions & 0 deletions pkg/linters/internal/astutil/astutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,18 @@ func ConstIntValue(pass *analysis.Pass, expr ast.Expr) (int64, bool) {
return v, exact
}

// UnwrapParenExpr unwraps any layers of redundant parentheses around expr,
// returning the innermost non-parenthesized expression.
func UnwrapParenExpr(expr ast.Expr) ast.Expr {
for {
p, ok := expr.(*ast.ParenExpr)
if !ok {
return expr
}
expr = p.X
}
}

// AsStringsMethodCall returns the *ast.CallExpr if expr is a call to the
// named method on the "strings" package (e.g. "Index" or "Count").
func AsStringsMethodCall(pass *analysis.Pass, expr ast.Expr, methodName string) (*ast.CallExpr, bool) {
Expand Down
2 changes: 2 additions & 0 deletions pkg/linters/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import (
"github.com/github/gh-aw/pkg/linters/stringsconcatloop"
"github.com/github/gh-aw/pkg/linters/stringscountcontains"
"github.com/github/gh-aw/pkg/linters/stringsindexcontains"
"github.com/github/gh-aw/pkg/linters/stringsindexhasprefix"
"github.com/github/gh-aw/pkg/linters/stringsjoinone"
"github.com/github/gh-aw/pkg/linters/timeafterleak"
"github.com/github/gh-aw/pkg/linters/timenowsub"
Expand Down Expand Up @@ -115,6 +116,7 @@ func All() []*analysis.Analyzer {
stringreplaceminusone.Analyzer,
stringsconcatloop.Analyzer,
stringsindexcontains.Analyzer,
stringsindexhasprefix.Analyzer,
stringsjoinone.Analyzer,
stringscountcontains.Analyzer,
jsonmarshalignoredeerror.Analyzer,
Expand Down
6 changes: 4 additions & 2 deletions pkg/linters/spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import (
"github.com/github/gh-aw/pkg/linters/stringsconcatloop"
"github.com/github/gh-aw/pkg/linters/stringscountcontains"
"github.com/github/gh-aw/pkg/linters/stringsindexcontains"
"github.com/github/gh-aw/pkg/linters/stringsindexhasprefix"
"github.com/github/gh-aw/pkg/linters/stringsjoinone"
"github.com/github/gh-aw/pkg/linters/timeafterleak"
"github.com/github/gh-aw/pkg/linters/timenowsub"
Expand All @@ -84,7 +85,7 @@ type docAnalyzer struct {
}

// documentedAnalyzers returns the analyzer subpackages documented in the README
// "Public API > Subpackages" table. The README documents 58 analyzers
// "Public API > Subpackages" table. The README documents 59 analyzers
// subpackages (the non-analyzer `internal` helper subpackage is excluded because
// it exposes no Analyzer).
//
Expand All @@ -95,7 +96,7 @@ type docAnalyzer struct {
// hardcodedfilepath, httpnoctx, httprespbodyclose, httpstatuscode, ioutildeprecated, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero,
// logfatallibrary, manualmutexunlock, mapclearloop, mapdeletecheck, nilctxpassed, osexitinlibrary, osgetenvlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib,
// regexpcompileinfunction, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, sprintfbool, sprintfint, ssljson,
// strconvparseignorederror, stringbytesroundtrip, stringreplaceminusone, stringsconcatloop, stringscountcontains, stringsindexcontains, stringsjoinone, timeafterleak, timesleepnocontext, timenowsub,
// strconvparseignorederror, stringbytesroundtrip, stringreplaceminusone, stringsconcatloop, stringscountcontains, stringsindexcontains, stringsindexhasprefix, stringsjoinone, timeafterleak, timesleepnocontext, timenowsub,
// tolowerequalfold, trimleftright, uncheckedtypeassertion, wgdonenotdeferred, writebytestring
func documentedAnalyzers() []docAnalyzer {
return []docAnalyzer{
Expand Down Expand Up @@ -148,6 +149,7 @@ func documentedAnalyzers() []docAnalyzer {
{"stringsconcatloop", stringsconcatloop.Analyzer},
{"stringscountcontains", stringscountcontains.Analyzer},
{"stringsindexcontains", stringsindexcontains.Analyzer},
{"stringsindexhasprefix", stringsindexhasprefix.Analyzer},
{"stringsjoinone", stringsjoinone.Analyzer},
{"timeafterleak", timeafterleak.Analyzer},
{"timesleepnocontext", timesleepnocontext.Analyzer},
Expand Down
143 changes: 143 additions & 0 deletions pkg/linters/stringsindexhasprefix/stringsindexhasprefix.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
// Package stringsindexhasprefix implements a Go analysis linter that flags
// strings.Index(s, sub) comparisons with 0 (== 0 and != 0) and their yoda-order
// variants that should use the more readable strings.HasPrefix(s, sub) or
// !strings.HasPrefix(s, sub) instead.
package stringsindexhasprefix

import (
"fmt"
"go/ast"
"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-hasprefix analysis pass.
var Analyzer = &analysis.Analyzer{
Name: "stringsindexhasprefix",
Doc: "reports strings.Index(s, sub) comparisons with 0 (== 0 and != 0) and their yoda-order variants that should use strings.HasPrefix(s, sub) or !strings.HasPrefix(s, sub)",
URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/stringsindexhasprefix",
Requires: []*analysis.Analyzer{inspect.Analyzer, nolint.Analyzer, filecheck.Analyzer},
Run: run,
}

func run(pass *analysis.Pass) (any, error) {
insp, err := astutil.Inspector(pass)
if err != nil {
return nil, err
}
noLintIndex, err := nolint.Index(pass)
if err != nil {
return nil, err
}
generatedFiles, err := filecheck.Index(pass)
if err != nil {
return nil, err
}

nodeFilter := []ast.Node{(*ast.BinaryExpr)(nil)}
insp.Preorder(nodeFilter, func(n ast.Node) {
analyzeIndexHasPrefix(pass, n, generatedFiles, noLintIndex)
})
return nil, nil
}

// analyzeIndexHasPrefix checks whether a binary expression is a strings.Index
// comparison with 0 that should use strings.HasPrefix.
func analyzeIndexHasPrefix(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) {
expr, ok := n.(*ast.BinaryExpr)
if !ok {
return
}
pos := pass.Fset.PositionFor(expr.Pos(), false)
if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) {
return
}
if nolint.HasDirectiveForLinter(pos, noLintIndex, "stringsindexhasprefix") {
return
}
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 := astutil.CallQualifierText(pass.Fset, indexCall)
if sText == "" || subText == "" || pkgText == "" {
return
}

var replacement, msg string
if negated {
replacement = "!" + pkgText + ".HasPrefix(" + sText + ", " + subText + ")"
msg = fmt.Sprintf("use !strings.HasPrefix(%s, %s) instead of strings.Index comparison", sText, subText)
} else {
replacement = pkgText + ".HasPrefix(" + sText + ", " + subText + ")"
msg = fmt.Sprintf("use strings.HasPrefix(%s, %s) instead of strings.Index comparison", sText, subText)
}

pass.Report(analysis.Diagnostic{
Pos: expr.Pos(),
End: expr.End(),
Message: msg,
SuggestedFixes: []analysis.SuggestedFix{{
Message: "Replace strings.Index comparison with strings.HasPrefix",
TextEdits: []analysis.TextEdit{{
Pos: expr.Pos(),
End: expr.End(),
NewText: []byte(replacement),
}},
}},
})
}

// matchIndexComparison reports whether expr is a strings.Index comparison with 0.
// It returns the strings.Index call, whether the result is negated (i.e., !HasPrefix),
// and whether the pattern matched.
func matchIndexComparison(pass *analysis.Pass, expr *ast.BinaryExpr) (call *ast.CallExpr, negated bool, matched bool) {
left, right, flipped := normalizeOperands(pass, expr)

indexCall, ok := astutil.AsStringsMethodCall(pass, left, "Index")
if !ok {
return nil, false, false
}

op := expr.Op
if flipped {
op = astutil.FlipComparisonOp(op)
}

litVal, ok := astutil.ConstIntValue(pass, right)
if !ok || litVal != 0 {
return nil, false, false
}

switch op {
case token.EQL:
return indexCall, false, true
case token.NEQ:
return indexCall, true, true
default:
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.
// Both operands are unwrapped of any redundant parentheses before the check.
func normalizeOperands(pass *analysis.Pass, expr *ast.BinaryExpr) (left, right ast.Expr, flipped bool) {
x := astutil.UnwrapParenExpr(expr.X)
y := astutil.UnwrapParenExpr(expr.Y)
if _, ok := astutil.AsStringsMethodCall(pass, x, "Index"); ok {
return x, y, false
}
return y, x, true
}
16 changes: 16 additions & 0 deletions pkg/linters/stringsindexhasprefix/stringsindexhasprefix_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//go:build !integration

package stringsindexhasprefix_test

import (
"testing"

"golang.org/x/tools/go/analysis/analysistest"

"github.com/github/gh-aw/pkg/linters/stringsindexhasprefix"
)

func TestAnalyzer(t *testing.T) {
testdata := analysistest.TestData()
analysistest.RunWithSuggestedFixes(t, testdata, stringsindexhasprefix.Analyzer, "stringsindexhasprefix")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package stringsindexhasprefix

import "strings"

func badHasPrefix(s, sub string) bool {
return strings.Index(s, sub) == 0 // want `use strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badNotHasPrefix(s, sub string) bool {
return strings.Index(s, sub) != 0 // want `use !strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badYodaHasPrefix(s, sub string) bool {
return 0 == strings.Index(s, sub) // want `use strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badYodaNotHasPrefix(s, sub string) bool {
return 0 != strings.Index(s, sub) // want `use !strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badParenHasPrefix(s, sub string) bool {
return (strings.Index(s, sub)) == 0 // want `use strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badParenNotHasPrefix(s, sub string) bool {
return (strings.Index(s, sub)) != 0 // want `use !strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badParenYodaHasPrefix(s, sub string) bool {
return 0 == (strings.Index(s, sub)) // want `use strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badParenYodaNotHasPrefix(s, sub string) bool {
return 0 != (strings.Index(s, sub)) // want `use !strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func goodHasPrefix(s, sub string) bool {
return strings.HasPrefix(s, sub)
}

func goodNotHasPrefix(s, sub string) bool {
return !strings.HasPrefix(s, sub)
}

func goodContainsStyleCheck(s, sub string) bool {
return strings.Index(s, sub) >= 0
}

func goodNotContainsStyleCheck(s, sub string) bool {
return strings.Index(s, sub) == -1
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package stringsindexhasprefix

import "strings"

func badHasPrefix(s, sub string) bool {
return strings.HasPrefix(s, sub) // want `use strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badNotHasPrefix(s, sub string) bool {
return !strings.HasPrefix(s, sub) // want `use !strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badYodaHasPrefix(s, sub string) bool {
return strings.HasPrefix(s, sub) // want `use strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badYodaNotHasPrefix(s, sub string) bool {
return !strings.HasPrefix(s, sub) // want `use !strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badParenHasPrefix(s, sub string) bool {
return strings.HasPrefix(s, sub) // want `use strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badParenNotHasPrefix(s, sub string) bool {
return !strings.HasPrefix(s, sub) // want `use !strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badParenYodaHasPrefix(s, sub string) bool {
return strings.HasPrefix(s, sub) // want `use strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func badParenYodaNotHasPrefix(s, sub string) bool {
return !strings.HasPrefix(s, sub) // want `use !strings\.HasPrefix\(s, sub\) instead of strings\.Index comparison`
}

func goodHasPrefix(s, sub string) bool {
return strings.HasPrefix(s, sub)
}

func goodNotHasPrefix(s, sub string) bool {
return !strings.HasPrefix(s, sub)
}

func goodContainsStyleCheck(s, sub string) bool {
return strings.Index(s, sub) >= 0
}

func goodNotContainsStyleCheck(s, sub string) bool {
return strings.Index(s, sub) == -1
}
Loading