linters: add bytescomparezero analyzer for bytes.Compare(... ) == 0 equality checks

pkg/linters/README.md

@@ -9,6 +9,7 @@ This package currently provides custom Go analyzers in the following subpackages
 - `appendbytestring` — reports `append(b, []byte(s)...)` calls where `b` is `[]byte` and `s` is a string, which can be simplified to `append(b, s...)`.
 - `appendoneelement` — reports `append(s, []T{x}...)` calls where a single-element slice literal is spread and can be simplified to `append(s, x)`.
 - `bytescomparestring` — reports `string(a) == string(b)` and `string(a) != string(b)` comparisons where `a` and `b` are `[]byte` values; use `bytes.Equal(a, b)` for `==` and `!bytes.Equal(a, b)` for `!=`.
+- `bytescomparezero` — reports `bytes.Compare(a, b) == 0` / `!= 0` comparisons (including `0 == bytes.Compare(a, b)` forms) where `bytes.Equal(a, b)` / `!bytes.Equal(a, b)` is clearer.
 - `bytesbufferstring` — reports `string(buf.Bytes())` calls where `buf` is a `bytes.Buffer` value receiver, suggesting `buf.String()` instead.
 - `contextcancelnotdeferred` — reports context cancel functions that are called directly instead of deferred.
 - `ctxbackground` — reports `context.Background()` calls inside functions that already receive a `context.Context` parameter.
@@ -76,6 +77,7 @@ This package currently provides custom Go analyzers in the following subpackages
 | `appendbytestring` | Custom `go/analysis` analyzer that flags `append(b, []byte(s)...)` calls where `s` is a string that can be simplified to `append(b, s...)` |
 | `appendoneelement` | Custom `go/analysis` analyzer that flags `append(s, []T{x}...)` calls where a single-element slice literal is spread and can be simplified to `append(s, x)` |
 | `bytescomparestring` | Custom `go/analysis` analyzer that flags `string(a) == string(b)` / `!=` comparisons on `[]byte` values; use `bytes.Equal(a, b)` for `==` and `!bytes.Equal(a, b)` for `!=` |
+| `bytescomparezero` | Custom `go/analysis` analyzer that flags `bytes.Compare(a, b)` equality checks against zero (`== 0`, `!= 0`, and yoda forms) that should use `bytes.Equal(a, b)` / `!bytes.Equal(a, b)` |
 | `bytesbufferstring` | Custom `go/analysis` analyzer that flags `string(buf.Bytes())` calls where `buf` is a `bytes.Buffer` value and suggests `buf.String()` instead |
 | `contextcancelnotdeferred` | Custom `go/analysis` analyzer that flags context cancel functions called directly instead of deferred |
 | `ctxbackground` | Custom `go/analysis` analyzer that flags `context.Background()` calls inside functions that already receive a context parameter |
@@ -208,6 +210,7 @@ _ = trimleftright.Analyzer
 - `github.com/github/gh-aw/pkg/linters/appendbytestring` — append-byte-string analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/appendoneelement` — append-one-element analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/bytescomparestring` — bytes-compare-string analyzer subpackage
+- `github.com/github/gh-aw/pkg/linters/bytescomparezero` — bytes-compare-zero analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/bytesbufferstring` — bytes-buffer-string analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred` — context-cancel-not-deferred analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/ctxbackground` — context-background analyzer subpackage

pkg/linters/bytescomparezero/bytescomparezero.go

@@ -0,0 +1,150 @@
+// Package bytescomparezero implements a Go analysis linter that flags
+// bytes.Compare(a, b) == 0 and bytes.Compare(a, b) != 0 comparisons (including
+// yoda-order variants) that should use bytes.Equal(a, b) or !bytes.Equal(a, b).
+package bytescomparezero
+
+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 bytes-compare-zero analysis pass.
+var Analyzer = &analysis.Analyzer{
+	Name:     "bytescomparezero",
+	Doc:      "reports bytes.Compare(a, b) == 0 / != 0 (and yoda variants) that should use bytes.Equal(a, b) or !bytes.Equal(a, b)",
+	URL:      "https://github.com/github/gh-aw/tree/main/pkg/linters/bytescomparezero",
+	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) {
+		analyzeCompareZero(pass, n, generatedFiles, noLintIndex)
+	})
+	return nil, nil
+}
+
+func analyzeCompareZero(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, "bytescomparezero") {
+		return
+	}
+
+	compareCall, negated, matched := matchCompareZero(pass, expr)
+	if !matched {
+		return
+	}
+	if len(compareCall.Args) != 2 {
+		return
+	}
+
+	aText := astutil.NodeText(pass.Fset, compareCall.Args[0])
+	bText := astutil.NodeText(pass.Fset, compareCall.Args[1])
+	pkgText := astutil.CallQualifierText(pass.Fset, compareCall)
+	if aText == "" || bText == "" || pkgText == "" {
+		return
+	}
+
+	var replacement, msg string
+	if negated {
+		replacement = "!" + pkgText + ".Equal(" + aText + ", " + bText + ")"
+		msg = fmt.Sprintf("use !bytes.Equal(%s, %s) instead of bytes.Compare comparison with 0", aText, bText)
+	} else {
+		replacement = pkgText + ".Equal(" + aText + ", " + bText + ")"
+		msg = fmt.Sprintf("use bytes.Equal(%s, %s) instead of bytes.Compare comparison with 0", aText, bText)
+	}
+
+	pass.Report(analysis.Diagnostic{
+		Pos:     expr.Pos(),
+		End:     expr.End(),
+		Message: msg,
+		SuggestedFixes: []analysis.SuggestedFix{{
+			Message: "Replace bytes.Compare equality check with bytes.Equal",
+			TextEdits: []analysis.TextEdit{{
+				Pos:     expr.Pos(),
+				End:     expr.End(),
+				NewText: []byte(replacement),
+			}},
+		}},
+	})
+}
+
+func matchCompareZero(pass *analysis.Pass, expr *ast.BinaryExpr) (call *ast.CallExpr, negated bool, matched bool) {
+	left, right, flipped := normalizeOperands(pass, expr)
+	compareCall, ok := asBytesCompareCall(pass, left)
+	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 compareCall, false, true
+	case token.NEQ:
+		return compareCall, true, true
+	default:
+		return nil, false, false
+	}
+}
+
+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 := asBytesCompareCall(pass, x); ok {
+		return x, y, false
+	}
+	return y, x, true
+}
+
+func asBytesCompareCall(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 != "Compare" {
+		return nil, false
+	}
+	if !astutil.IsPkgSelector(pass, sel, "bytes") {
+		return nil, false
+	}
+	return call, true
+}

pkg/linters/bytescomparezero/bytescomparezero_test.go

@@ -0,0 +1,16 @@
+//go:build !integration
+
+package bytescomparezero_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+
+	"github.com/github/gh-aw/pkg/linters/bytescomparezero"
+)
+
+func TestAnalyzer(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.RunWithSuggestedFixes(t, testdata, bytescomparezero.Analyzer, "bytescomparezero")
+}

pkg/linters/bytescomparezero/testdata/src/bytescomparezero/aliased_import.go

@@ -0,0 +1,11 @@
+package bytescomparezero
+
+import bx "bytes"
+
+func badAliasedEqual(a, b []byte) bool {
+	return bx.Compare(a, b) == 0 // want `use bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badAliasedNotEqual(a, b []byte) bool {
+	return bx.Compare(a, b) != 0 // want `use !bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}

pkg/linters/bytescomparezero/testdata/src/bytescomparezero/aliased_import.go.golden

@@ -0,0 +1,11 @@
+package bytescomparezero
+
+import bx "bytes"
+
+func badAliasedEqual(a, b []byte) bool {
+	return bx.Equal(a, b) // want `use bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badAliasedNotEqual(a, b []byte) bool {
+	return !bx.Equal(a, b) // want `use !bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}

pkg/linters/bytescomparezero/testdata/src/bytescomparezero/bytescomparezero.go

@@ -0,0 +1,47 @@
+package bytescomparezero
+
+import "bytes"
+
+func badEqual(a, b []byte) bool {
+	return bytes.Compare(a, b) == 0 // want `use bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badNotEqual(a, b []byte) bool {
+	return bytes.Compare(a, b) != 0 // want `use !bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badYodaEqual(a, b []byte) bool {
+	return 0 == bytes.Compare(a, b) // want `use bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badYodaNotEqual(a, b []byte) bool {
+	return 0 != bytes.Compare(a, b) // want `use !bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badParenYodaEqual(a, b []byte) bool {
+	return 0 == (bytes.Compare(a, b)) // want `use bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badParenYodaNotEqual(a, b []byte) bool {
+	return 0 != (bytes.Compare(a, b)) // want `use !bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func goodAlreadyEqual(a, b []byte) bool {
+	return bytes.Equal(a, b)
+}
+
+func goodLessThan(a, b []byte) bool {
+	return bytes.Compare(a, b) < 0
+}
+
+func goodGreaterThan(a, b []byte) bool {
+	return bytes.Compare(a, b) > 0
+}
+
+func goodEqualsOne(a, b []byte) bool {
+	return bytes.Compare(a, b) == 1
+}
+
+func goodNolint(a, b []byte) bool {
+	return bytes.Compare(a, b) == 0 //nolint:bytescomparezero
+}

pkg/linters/bytescomparezero/testdata/src/bytescomparezero/bytescomparezero.go.golden

@@ -0,0 +1,47 @@
+package bytescomparezero
+
+import "bytes"
+
+func badEqual(a, b []byte) bool {
+	return bytes.Equal(a, b) // want `use bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badNotEqual(a, b []byte) bool {
+	return !bytes.Equal(a, b) // want `use !bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badYodaEqual(a, b []byte) bool {
+	return bytes.Equal(a, b) // want `use bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badYodaNotEqual(a, b []byte) bool {
+	return !bytes.Equal(a, b) // want `use !bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badParenYodaEqual(a, b []byte) bool {
+	return bytes.Equal(a, b) // want `use bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func badParenYodaNotEqual(a, b []byte) bool {
+	return !bytes.Equal(a, b) // want `use !bytes\.Equal\(a, b\) instead of bytes\.Compare comparison with 0`
+}
+
+func goodAlreadyEqual(a, b []byte) bool {
+	return bytes.Equal(a, b)
+}
+
+func goodLessThan(a, b []byte) bool {
+	return bytes.Compare(a, b) < 0
+}
+
+func goodGreaterThan(a, b []byte) bool {
+	return bytes.Compare(a, b) > 0
+}
+
+func goodEqualsOne(a, b []byte) bool {
+	return bytes.Compare(a, b) == 1
+}
+
+func goodNolint(a, b []byte) bool {
+	return bytes.Compare(a, b) == 0 //nolint:bytescomparezero
+}

pkg/linters/doc.go

@@ -1,11 +1,12 @@
 // Package linters is a namespace for gh-aw's custom Go analysis linters.
 //
-// All 59 active analyzers:
+// All 60 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)
 //   - bytesbufferstring — reports string(buf.Bytes()) calls where buf is a bytes.Buffer value and suggests buf.String() instead
 //   - bytescomparestring — flags string(a) == string(b) and string(a) != string(b) comparisons where a and b are []byte values and recommends bytes.Equal for clearer intent
+//   - bytescomparezero — flags bytes.Compare(a, b) == 0 / != 0 comparisons (including yoda variants) and recommends bytes.Equal(a, b) or !bytes.Equal(a, b)
 //   - contextcancelnotdeferred — flags context cancel functions called directly instead of deferred
 //   - ctxbackground — flags context.Background() inside functions that already receive a context
 //   - deferinloop — flags defer statements placed directly inside for or range loop bodies

pkg/linters/registry.go

@@ -7,6 +7,7 @@ import (
 	"github.com/github/gh-aw/pkg/linters/appendoneelement"
 	"github.com/github/gh-aw/pkg/linters/bytesbufferstring"
 	"github.com/github/gh-aw/pkg/linters/bytescomparestring"
+	"github.com/github/gh-aw/pkg/linters/bytescomparezero"
 	"github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred"
 	"github.com/github/gh-aw/pkg/linters/ctxbackground"
 	"github.com/github/gh-aw/pkg/linters/deferinloop"
@@ -75,6 +76,7 @@ func All() []*analysis.Analyzer {
 		appendoneelement.Analyzer,
 		bytesbufferstring.Analyzer,
 		bytescomparestring.Analyzer,
+		bytescomparezero.Analyzer,
 		contextcancelnotdeferred.Analyzer,
 		ctxbackground.Analyzer,
 		deferinloop.Analyzer,

pkg/linters/spec_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/github/gh-aw/pkg/linters/appendoneelement"
 	"github.com/github/gh-aw/pkg/linters/bytesbufferstring"
 	"github.com/github/gh-aw/pkg/linters/bytescomparestring"
+	"github.com/github/gh-aw/pkg/linters/bytescomparezero"
 	"github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred"
 	"github.com/github/gh-aw/pkg/linters/ctxbackground"
 	"github.com/github/gh-aw/pkg/linters/deferinloop"
@@ -91,7 +92,7 @@ type docAnalyzer struct {
 //
 // Spec (README "Public API > Subpackages"):
 //
-//	appendbytestring, appendoneelement, bytesbufferstring, bytescomparestring, contextcancelnotdeferred, ctxbackground, deferinloop, errorfwrapv, excessivefuncparams, errormessage,
+//	appendbytestring, appendoneelement, bytesbufferstring, bytescomparestring, bytescomparezero, contextcancelnotdeferred, ctxbackground, deferinloop, errorfwrapv, excessivefuncparams, errormessage,
 //	errortypeassertion, errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf,
 //	hardcodedfilepath, httpnoctx, httprespbodyclose, httpstatuscode, ioutildeprecated, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero,
 //	logfatallibrary, manualmutexunlock, mapclearloop, mapdeletecheck, nilctxpassed, osexitinlibrary, osgetenvlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib,
@@ -104,6 +105,7 @@ func documentedAnalyzers() []docAnalyzer {
 		{"appendoneelement", appendoneelement.Analyzer},
 		{"bytesbufferstring", bytesbufferstring.Analyzer},
 		{"bytescomparestring", bytescomparestring.Analyzer},
+		{"bytescomparezero", bytescomparezero.Analyzer},
 		{"contextcancelnotdeferred", contextcancelnotdeferred.Analyzer},
 		{"ctxbackground", ctxbackground.Analyzer},
 		{"deferinloop", deferinloop.Analyzer},