diff --git a/.github/workflows/smoke-copilot-auto.lock.yml b/.github/workflows/smoke-copilot-auto.lock.yml index 9cac699d23d..5c316f6e8b8 100644 --- a/.github/workflows/smoke-copilot-auto.lock.yml +++ b/.github/workflows/smoke-copilot-auto.lock.yml @@ -149,6 +149,7 @@ jobs: GH_AW_INFO_FIREWALL_TYPE: "squid" GH_AW_INFO_FRONTMATTER_EMOJI: "🌸" GH_AW_COMPILED_STRICT: "true" + GH_AW_INFO_MODEL_COSTS: '{"providers":{"github-copilot":{"models":{"auto":{"cost":{"input":"0","output":"0"}}}}}}' GH_AW_INFO_FEATURES: '{"gh-aw-detection":false}' uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: diff --git a/pkg/linters/README.md b/pkg/linters/README.md index 8ec9dd722d0..10c37d2f93b 100644 --- a/pkg/linters/README.md +++ b/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 diff --git a/pkg/linters/bytescomparezero/bytescomparezero.go b/pkg/linters/bytescomparezero/bytescomparezero.go new file mode 100644 index 00000000000..f131b21fd48 --- /dev/null +++ b/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 +} diff --git a/pkg/linters/bytescomparezero/bytescomparezero_test.go b/pkg/linters/bytescomparezero/bytescomparezero_test.go new file mode 100644 index 00000000000..5a43dfdfbe6 --- /dev/null +++ b/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") +} diff --git a/pkg/linters/bytescomparezero/testdata/src/bytescomparezero/aliased_import.go b/pkg/linters/bytescomparezero/testdata/src/bytescomparezero/aliased_import.go new file mode 100644 index 00000000000..2711a1a4a5d --- /dev/null +++ b/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` +} diff --git a/pkg/linters/bytescomparezero/testdata/src/bytescomparezero/aliased_import.go.golden b/pkg/linters/bytescomparezero/testdata/src/bytescomparezero/aliased_import.go.golden new file mode 100644 index 00000000000..8247859d86f --- /dev/null +++ b/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` +} diff --git a/pkg/linters/bytescomparezero/testdata/src/bytescomparezero/bytescomparezero.go b/pkg/linters/bytescomparezero/testdata/src/bytescomparezero/bytescomparezero.go new file mode 100644 index 00000000000..9f4701e420a --- /dev/null +++ b/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 +} diff --git a/pkg/linters/bytescomparezero/testdata/src/bytescomparezero/bytescomparezero.go.golden b/pkg/linters/bytescomparezero/testdata/src/bytescomparezero/bytescomparezero.go.golden new file mode 100644 index 00000000000..bfdf1a40cff --- /dev/null +++ b/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 +} diff --git a/pkg/linters/bytescomparezero/testdata/src/bytescomparezero/generated.go b/pkg/linters/bytescomparezero/testdata/src/bytescomparezero/generated.go new file mode 100644 index 00000000000..4d918a825d3 --- /dev/null +++ b/pkg/linters/bytescomparezero/testdata/src/bytescomparezero/generated.go @@ -0,0 +1,9 @@ +// Code generated by tests; DO NOT EDIT. + +package bytescomparezero + +import "bytes" + +func generatedShouldSkip(a, b []byte) bool { + return bytes.Compare(a, b) == 0 +} diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index a99645363fb..121cfd48f31 100644 --- a/pkg/linters/doc.go +++ b/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 diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index b81f1bd1ebf..6ecd529c401 100644 --- a/pkg/linters/registry.go +++ b/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, diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 1a0c2fb898a..41711599350 100644 --- a/pkg/linters/spec_test.go +++ b/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},