diff --git a/cmd/linters/main.go b/cmd/linters/main.go index a6eca1b9c31..ac65719b0b2 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -17,6 +17,7 @@ import ( "golang.org/x/tools/go/analysis/multichecker" "github.com/github/gh-aw/pkg/linters/appendbytestring" + "github.com/github/gh-aw/pkg/linters/bytescomparestring" "github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred" "github.com/github/gh-aw/pkg/linters/ctxbackground" "github.com/github/gh-aw/pkg/linters/deferinloop" @@ -64,6 +65,7 @@ import ( func main() { multichecker.Main( appendbytestring.Analyzer, + bytescomparestring.Analyzer, contextcancelnotdeferred.Analyzer, ctxbackground.Analyzer, deferinloop.Analyzer, diff --git a/docs/adr/44389-bytescomparestring-linter-for-byte-slice-equality.md b/docs/adr/44389-bytescomparestring-linter-for-byte-slice-equality.md new file mode 100644 index 00000000000..eecdb3f1483 --- /dev/null +++ b/docs/adr/44389-bytescomparestring-linter-for-byte-slice-equality.md @@ -0,0 +1,44 @@ +# ADR-44389: Add `bytescomparestring` Linter to Flag Allocating `[]byte` Equality Checks + +**Date**: 2026-07-08 +**Status**: Accepted +**Deciders**: Unknown (automated PR by Linter Miner) + +--- + +### Context + +Comparing two `[]byte` values via `string(a) == string(b)` or `string(a) != string(b)` compiles successfully but causes two unnecessary heap allocations per comparison — one for each `string()` conversion. The allocation-free idiomatic alternative is `bytes.Equal(a, b)` and `!bytes.Equal(a, b)`. A code-pattern scan found 3 live instances of this pattern in the repo (`pkg/parser/import_bfs.go:566`, `pkg/parser/tools_merger.go:279`, `pkg/cli/packages.go:146`). The existing custom linter suite (`pkg/linters/`) is the established mechanism for enforcing repo-wide Go idioms at compile time with automatic suggested fixes. + +### Decision + +We will add a new `go/analysis` linter named `bytescomparestring` that detects `string(a) == string(b)` and `string(a) != string(b)` expressions where both `a` and `b` have underlying type `[]byte`, reports a diagnostic, and emits a `SuggestedFix` that rewrites the expression to `bytes.Equal(a, b)` or `!bytes.Equal(a, b)`. The linter is registered in `cmd/linters/main.go` and documented in `pkg/linters/doc.go`. False-positive rate is zero: the linter only fires when both sides are `string(x)` type-conversions with a `[]byte` argument. + +### Alternatives Considered + +#### Alternative 1: Rely on an External Tool (staticcheck / golangci-lint) + +Staticcheck's `SA4010` and similar rules cover some allocation patterns, but none specifically target the `string([]byte) == string([]byte)` form with auto-fix support. Adopting an external tool for this single pattern would add a build-tool dependency and complicate CI configuration, while the project already maintains a bespoke linter framework with consistent nolint directives, file-skip logic, and suggested-fix plumbing. Not chosen because the marginal benefit of a third-party tool does not justify adding a dependency. + +#### Alternative 2: Use `slices.Equal` or `reflect.DeepEqual` + +`slices.Equal` (Go 1.21+) and `reflect.DeepEqual` are functionally correct alternatives to `bytes.Equal` for `[]byte` comparisons. However, `reflect.DeepEqual` is significantly slower due to reflection overhead, and `slices.Equal` is a generic function that is less recognizable in idiomatic byte-handling code than `bytes.Equal`. The Go standard library convention for byte-slice equality is `bytes.Equal`; the linter's suggested fix should produce idiomatic output. + +### Consequences + +#### Positive +- Eliminates two unnecessary heap allocations per `[]byte` equality comparison at the 3 known sites and any future occurrences. +- Provides an auto-applicable `SuggestedFix` compatible with `gopls` and `go fix`, reducing friction to fix violations. +- Zero false-positive rate: the linter's type-aware check (`pass.TypesInfo`) ensures it only fires when both sides are `string()` conversions of `[]byte` arguments. +- Consistent enforcement: future `string(a) == string(b)` regressions are caught at lint time rather than discovered in code review. + +#### Negative +- Adds a new linter to maintain: API-breaking changes in `golang.org/x/tools/go/analysis` would require updating this analyzer alongside the 43 others. + +#### Neutral +- The linter skips test files (`filecheck.IsTestFile`) consistent with the project's convention; `string(a) == string(b)` patterns in test code are not flagged. +- The linter count in `pkg/linters/doc.go` increases from 43 to 44; this counter must be updated whenever analyzers are added or removed. + +--- + +*ADR finalized: implementation complete, all known violations fixed, and suggested fixes include the `bytes` import when not already present.* diff --git a/linters b/linters index 91c6d678938..04a7c5188f1 100755 Binary files a/linters and b/linters differ diff --git a/pkg/cli/packages.go b/pkg/cli/packages.go index 46273f52fdc..33a41063090 100644 --- a/pkg/cli/packages.go +++ b/pkg/cli/packages.go @@ -2,6 +2,7 @@ package cli import ( "bufio" + "bytes" "fmt" "os" "path/filepath" @@ -143,7 +144,7 @@ func copyIncludeDependenciesFromPackageWithForce(dependencies []IncludeDependenc if existingContent, err := os.ReadFile(targetPath); err == nil { fileExists = true // File exists, compare contents - if string(existingContent) == string(sourceContent) { + if bytes.Equal(existingContent, sourceContent) { // Contents are the same, skip if verbose { fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Include file %s already exists with same content, skipping", dep.TargetPath))) diff --git a/pkg/linters/bytescomparestring/bytescomparestring.go b/pkg/linters/bytescomparestring/bytescomparestring.go new file mode 100644 index 00000000000..99ad1ac8cb9 --- /dev/null +++ b/pkg/linters/bytescomparestring/bytescomparestring.go @@ -0,0 +1,205 @@ +// Package bytescomparestring implements a Go analysis linter that flags +// string(a) == string(b) and string(a) != string(b) comparisons where both +// a and b are []byte values, which should use bytes.Equal(a, b) instead to +// avoid unnecessary allocations. +package bytescomparestring + +import ( + "fmt" + "go/ast" + "go/token" + "go/types" + + "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" +) + +const bytesPkg = "bytes" + +// Analyzer is the bytes-compare-string analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "bytescomparestring", + Doc: "reports string(a) == string(b) and string(a) != string(b) comparisons where a and b are []byte values that should use bytes.Equal instead", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/bytescomparestring", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err + } + noLintLinesByFile := nolint.BuildLineIndex(pass, "bytescomparestring") + + nodeFilter := []ast.Node{ + (*ast.BinaryExpr)(nil), + } + + insp.Preorder(nodeFilter, func(n ast.Node) { + bin, ok := n.(*ast.BinaryExpr) + if !ok { + return + } + + // Only flag == and != operators. + if bin.Op != token.EQL && bin.Op != token.NEQ { + return + } + + pos := pass.Fset.PositionFor(bin.Pos(), false) + if filecheck.IsTestFile(pos.Filename) { + return + } + if nolint.HasDirective(pos, noLintLinesByFile) { + return + } + + // Both sides must be string(x) conversions where x is []byte. + lhsArg, ok := extractByteSliceStringConv(pass, bin.X) + if !ok { + return + } + rhsArg, ok := extractByteSliceStringConv(pass, bin.Y) + if !ok { + return + } + + lText := astutil.NodeText(pass.Fset, lhsArg) + rText := astutil.NodeText(pass.Fset, rhsArg) + if lText == "" || rText == "" { + return + } + + op := bin.Op.String() + if bin.Op == token.EQL { + pass.Report(analysis.Diagnostic{ + Pos: bin.Pos(), + End: bin.End(), + Message: fmt.Sprintf("string(%s) == string(%s) allocates; use bytes.Equal(%s, %s) instead", lText, rText, lText, rText), + SuggestedFixes: buildFix(pass, bin, fmt.Sprintf("bytes.Equal(%s, %s)", lText, rText)), + }) + } else { + pass.Report(analysis.Diagnostic{ + Pos: bin.Pos(), + End: bin.End(), + Message: fmt.Sprintf("string(%s) %s string(%s) allocates; use !bytes.Equal(%s, %s) instead", lText, op, rText, lText, rText), + SuggestedFixes: buildFix(pass, bin, fmt.Sprintf("!bytes.Equal(%s, %s)", lText, rText)), + }) + } + }) + + return nil, nil +} + +// buildFix returns the SuggestedFix for rewriting bin to replacement, adding a +// "bytes" import TextEdit when the file containing bin does not yet import it. +func buildFix(pass *analysis.Pass, bin *ast.BinaryExpr, replacement string) []analysis.SuggestedFix { + edits := []analysis.TextEdit{{ + Pos: bin.Pos(), + End: bin.End(), + NewText: []byte(replacement), + }} + if importEdit, ok := addBytesImportEdit(pass, bin.Pos()); ok { + edits = append(edits, importEdit) + } + return []analysis.SuggestedFix{{ + Message: "Replace with " + replacement, + TextEdits: edits, + }} +} + +// addBytesImportEdit returns a TextEdit that inserts an import for "bytes" into +// the file containing pos, unless "bytes" is already imported in that file. +// Returns (TextEdit{}, false) when no edit is needed. +func addBytesImportEdit(pass *analysis.Pass, pos token.Pos) (analysis.TextEdit, bool) { + var file *ast.File + for _, f := range pass.Files { + if f.Pos() <= pos && pos <= f.End() { + file = f + break + } + } + if file == nil { + return analysis.TextEdit{}, false + } + + // Check if "bytes" is already imported in this file. + for _, imp := range file.Imports { + if imp.Path.Value == `"`+bytesPkg+`"` { + return analysis.TextEdit{}, false + } + } + + // Find an existing grouped import declaration to add into. + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok || genDecl.Tok != token.IMPORT || !genDecl.Lparen.IsValid() { + continue + } + // Insert "bytes" before the closing paren of the import block. + return analysis.TextEdit{ + Pos: genDecl.Rparen, + End: genDecl.Rparen, + NewText: []byte("\t\"" + bytesPkg + "\"\n"), + }, true + } + + // No grouped import block; insert a standalone import after the package name. + return analysis.TextEdit{ + Pos: file.Name.End(), + End: file.Name.End(), + NewText: []byte("\n\nimport \"" + bytesPkg + "\""), + }, true +} + +// extractByteSliceStringConv checks whether expr is a string(x) conversion +// where x has underlying type []byte. If so, it returns x and true. +func extractByteSliceStringConv(pass *analysis.Pass, expr ast.Expr) (ast.Expr, bool) { + call, ok := expr.(*ast.CallExpr) + if !ok || len(call.Args) != 1 { + return nil, false + } + + // Must be a type conversion, not a function call. + funInfo, ok := pass.TypesInfo.Types[call.Fun] + if !ok || !funInfo.IsType() { + return nil, false + } + + // The outer conversion must produce a string. + resultInfo, ok := pass.TypesInfo.Types[call] + if !ok { + return nil, false + } + basic, ok := resultInfo.Type.Underlying().(*types.Basic) + if !ok || basic.Kind() != types.String { + return nil, false + } + + // The argument must be []byte (or []uint8). + arg := call.Args[0] + if !isByteSlice(pass, arg) { + return nil, false + } + + return arg, true +} + +// isByteSlice reports whether expr has underlying type []byte ([]uint8). +func isByteSlice(pass *analysis.Pass, expr ast.Expr) bool { + t := pass.TypesInfo.TypeOf(expr) + if t == nil { + return false + } + sl, ok := t.Underlying().(*types.Slice) + if !ok { + return false + } + elem, ok := sl.Elem().(*types.Basic) + return ok && elem.Kind() == types.Byte +} diff --git a/pkg/linters/bytescomparestring/bytescomparestring_test.go b/pkg/linters/bytescomparestring/bytescomparestring_test.go new file mode 100644 index 00000000000..381127b73b5 --- /dev/null +++ b/pkg/linters/bytescomparestring/bytescomparestring_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package bytescomparestring_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/bytescomparestring" +) + +func TestAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, bytescomparestring.Analyzer, "bytescomparestring") +} diff --git a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go new file mode 100644 index 00000000000..6c4b87dbc8c --- /dev/null +++ b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go @@ -0,0 +1,37 @@ +package bytescomparestring + +import "bytes" + +func badEqual(a, b []byte) bool { + return string(a) == string(b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead` +} + +func badNotEqual(a, b []byte) bool { + return string(a) != string(b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead` +} + +type myBytes []byte + +func badNamedType(a, b myBytes) bool { + return string(a) == string(b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead` +} + +func goodBytesEqual(a, b []byte) bool { + // Correct usage — no diagnostic expected. + return bytes.Equal(a, b) +} + +func goodStringLiteral(a []byte) bool { + // Only one side is string([]byte); not flagged. + return string(a) == "hello" +} + +func goodStringVars(a, b string) bool { + // Neither side is a []byte conversion; not flagged. + return a == b +} + +func goodMixedOneSideString(a []byte, b string) bool { + // One side is a string variable, not string([]byte); not flagged. + return string(a) == b +} diff --git a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go.golden b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go.golden new file mode 100644 index 00000000000..398552177cb --- /dev/null +++ b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go.golden @@ -0,0 +1,37 @@ +package bytescomparestring + +import "bytes" + +func badEqual(a, b []byte) bool { + return bytes.Equal(a, b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead` +} + +func badNotEqual(a, b []byte) bool { + return !bytes.Equal(a, b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead` +} + +type myBytes []byte + +func badNamedType(a, b myBytes) bool { + return bytes.Equal(a, b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead` +} + +func goodBytesEqual(a, b []byte) bool { + // Correct usage — no diagnostic expected. + return bytes.Equal(a, b) +} + +func goodStringLiteral(a []byte) bool { + // Only one side is string([]byte); not flagged. + return string(a) == "hello" +} + +func goodStringVars(a, b string) bool { + // Neither side is a []byte conversion; not flagged. + return a == b +} + +func goodMixedOneSideString(a []byte, b string) bool { + // One side is a string variable, not string([]byte); not flagged. + return string(a) == b +} diff --git a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go new file mode 100644 index 00000000000..9a7a119f8fd --- /dev/null +++ b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go @@ -0,0 +1,9 @@ +package bytescomparestring + +func noImportEqual(a, b []byte) bool { + return string(a) == string(b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead` +} + +func noImportNotEqual(a, b []byte) bool { + return string(a) != string(b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead` +} diff --git a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go.golden b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go.golden new file mode 100644 index 00000000000..e42adc1baf1 --- /dev/null +++ b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go.golden @@ -0,0 +1,11 @@ +package bytescomparestring + +import "bytes" + +func noImportEqual(a, b []byte) bool { + return bytes.Equal(a, b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead` +} + +func noImportNotEqual(a, b []byte) bool { + return !bytes.Equal(a, b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead` +} diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 322d039a1c1..e4752598579 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,8 +1,9 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// The actual analyzers are implemented in subpackages. All 39 active analyzers: +// All 44 active analyzers: // // - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...) +// - bytescomparestring — flags string(a) == string(b) and string(a) != string(b) comparisons where a and b are []byte values that should use bytes.Equal instead // - 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 @@ -17,12 +18,15 @@ // - fprintlnsprintf — flags fmt.Fprintln(..., fmt.Sprintf(...)) patterns // - hardcodedfilepath — flags hard-coded file path string literals that match known path constants or should be extracted as named constants // - httpnoctx — flags HTTP calls that do not accept a context.Context +// - httprespbodyclose — flags HTTP response bodies that are not closed +// - httpstatuscode — flags HTTP status code anti-patterns // - jsonmarshalignoredeerror — flags json.Marshal/Unmarshal calls where the error is discarded with _ // - largefunc — flags function bodies that exceed a configurable line-count threshold // - lenstringsplit — flags len(strings.Split(s, sep)) with a non-empty separator that should use strings.Count(s, sep)+1 // - lenstringzero — flags len(s) == 0 / len(s) != 0 on string values that should use s == "" / s != "" // - manualmutexunlock — flags non-deferred mutex Unlock() calls // - osexitinlibrary — flags os.Exit calls in library packages +// - osgetenvlibrary — flags os.Getenv calls in library packages // - ossetenvlibrary — flags os.Setenv calls in library packages // - panic-in-library-code — flags panic() calls in library packages // - rawloginlib — flags direct usage of the standard log package in library packages @@ -31,6 +35,7 @@ // - sortslice — flags sort.Slice / sort.SliceStable calls that should use slices.SortFunc / slices.SortStableFunc // - sprintferrdot — flags redundant .Error() calls on error values passed to fmt format functions // - sprintferrorsnew — flags errors.New(fmt.Sprintf(...)) calls that should use fmt.Errorf instead +// - sprintfint — flags fmt.Sprintf calls that format integers that should use strconv.Itoa // - ssljson — validates ssl.json skill artifacts in .github/skills/ against the SSL spec // - strconvparseignorederror — flags strconv parsing calls where the error is discarded with _ // - stringreplaceminusone — flags strings.Replace calls with n=-1 that should use strings.ReplaceAll diff --git a/pkg/parser/import_bfs.go b/pkg/parser/import_bfs.go index 9800551f911..bb3bc899804 100644 --- a/pkg/parser/import_bfs.go +++ b/pkg/parser/import_bfs.go @@ -5,6 +5,7 @@ package parser import ( + "bytes" "encoding/json" "errors" "fmt" @@ -563,7 +564,7 @@ func importInputsEqual(a, b map[string]any) bool { if err != nil { return false } - return string(aJSON) == string(bJSON) + return bytes.Equal(aJSON, bJSON) } // formatImportInputs serializes an import input map to a compact JSON string for diff --git a/pkg/parser/tools_merger.go b/pkg/parser/tools_merger.go index c2f884b6830..28cd00ac10c 100644 --- a/pkg/parser/tools_merger.go +++ b/pkg/parser/tools_merger.go @@ -1,6 +1,7 @@ package parser import ( + "bytes" "encoding/json" "fmt" "maps" @@ -276,5 +277,5 @@ func areEqual(a, b any) bool { return false } - return string(aJSON) == string(bJSON) + return bytes.Equal(aJSON, bJSON) }