diff --git a/pkg/linters/bytescomparestring/bytescomparestring.go b/pkg/linters/bytescomparestring/bytescomparestring.go index 99ad1ac8cb9..452223afe39 100644 --- a/pkg/linters/bytescomparestring/bytescomparestring.go +++ b/pkg/linters/bytescomparestring/bytescomparestring.go @@ -1,7 +1,7 @@ // 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. +// a and b are []byte values, which should use bytes.Equal(a, b) instead for +// clearer intent. package bytescomparestring import ( @@ -23,7 +23,7 @@ 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", + Doc: "flags string(a) == string(b) and string(a) != string(b) as []byte comparisons written the long way; use bytes.Equal for clearer intent", URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/bytescomparestring", Requires: []*analysis.Analyzer{inspect.Analyzer}, Run: run, @@ -36,6 +36,11 @@ func run(pass *analysis.Pass) (any, error) { } noLintLinesByFile := nolint.BuildLineIndex(pass, "bytescomparestring") + // seenImportFiles tracks files that have already received a bytes import + // TextEdit in this pass, preventing duplicate overlapping edits when a + // single file contains multiple flagged comparisons. + seenImportFiles := make(map[token.Pos]bool) + nodeFilter := []ast.Node{ (*ast.BinaryExpr)(nil), } @@ -75,20 +80,19 @@ func run(pass *analysis.Pass) (any, error) { 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)), + Message: fmt.Sprintf("string(%s) == string(%s) is a []byte comparison written the long way; use bytes.Equal(%s, %s) for clearer intent", lText, rText, lText, rText), + SuggestedFixes: buildFix(pass, bin, fmt.Sprintf("bytes.Equal(%s, %s)", lText, rText), seenImportFiles), }) } 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)), + Message: fmt.Sprintf("string(%s) != string(%s) is a []byte comparison written the long way; use !bytes.Equal(%s, %s) for clearer intent", lText, rText, lText, rText), + SuggestedFixes: buildFix(pass, bin, fmt.Sprintf("!bytes.Equal(%s, %s)", lText, rText), seenImportFiles), }) } }) @@ -98,13 +102,15 @@ func run(pass *analysis.Pass) (any, error) { // 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 { +// seenImportFiles tracks files that have already received an import edit in +// this pass so that multi-violation files do not get duplicate overlapping edits. +func buildFix(pass *analysis.Pass, bin *ast.BinaryExpr, replacement string, seenImportFiles map[token.Pos]bool) []analysis.SuggestedFix { edits := []analysis.TextEdit{{ Pos: bin.Pos(), End: bin.End(), NewText: []byte(replacement), }} - if importEdit, ok := addBytesImportEdit(pass, bin.Pos()); ok { + if importEdit, ok := addBytesImportEdit(pass, bin.Pos(), seenImportFiles); ok { edits = append(edits, importEdit) } return []analysis.SuggestedFix{{ @@ -114,9 +120,11 @@ func buildFix(pass *analysis.Pass, bin *ast.BinaryExpr, replacement string) []an } // addBytesImportEdit returns a TextEdit that inserts an import for "bytes" into -// the file containing pos, unless "bytes" is already imported in that file. +// the file containing pos, unless "bytes" is already imported in that file or +// an import edit for this file has already been emitted in this pass +// (tracked via seenImportFiles to prevent duplicate overlapping edits). // Returns (TextEdit{}, false) when no edit is needed. -func addBytesImportEdit(pass *analysis.Pass, pos token.Pos) (analysis.TextEdit, bool) { +func addBytesImportEdit(pass *analysis.Pass, pos token.Pos, seenImportFiles map[token.Pos]bool) (analysis.TextEdit, bool) { var file *ast.File for _, f := range pass.Files { if f.Pos() <= pos && pos <= f.End() { @@ -128,6 +136,11 @@ func addBytesImportEdit(pass *analysis.Pass, pos token.Pos) (analysis.TextEdit, return analysis.TextEdit{}, false } + // Skip if an import edit for this file was already emitted in this pass. + if seenImportFiles[file.Pos()] { + return analysis.TextEdit{}, false + } + // Check if "bytes" is already imported in this file. for _, imp := range file.Imports { if imp.Path.Value == `"`+bytesPkg+`"` { @@ -135,6 +148,9 @@ func addBytesImportEdit(pass *analysis.Pass, pos token.Pos) (analysis.TextEdit, } } + // Compute the edit to add and mark the file so subsequent violations in the + // same pass do not emit a duplicate overlapping TextEdit. + // Find an existing grouped import declaration to add into. for _, decl := range file.Decls { genDecl, ok := decl.(*ast.GenDecl) @@ -142,6 +158,7 @@ func addBytesImportEdit(pass *analysis.Pass, pos token.Pos) (analysis.TextEdit, continue } // Insert "bytes" before the closing paren of the import block. + seenImportFiles[file.Pos()] = true return analysis.TextEdit{ Pos: genDecl.Rparen, End: genDecl.Rparen, @@ -149,7 +166,31 @@ func addBytesImportEdit(pass *analysis.Pass, pos token.Pos) (analysis.TextEdit, }, true } + // If the file has exactly one import (non-grouped), convert it to a grouped + // import block while adding "bytes" before it (alphabetical order). + if len(file.Imports) == 1 { + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok || genDecl.Tok != token.IMPORT || genDecl.Lparen.IsValid() || len(genDecl.Specs) != 1 { + continue + } + + specText := astutil.NodeText(pass.Fset, genDecl.Specs[0]) + if specText == "" { + continue + } + + seenImportFiles[file.Pos()] = true + return analysis.TextEdit{ + Pos: genDecl.Pos(), + End: genDecl.End(), + NewText: []byte("import (\n\t\"" + bytesPkg + "\"\n\t" + specText + "\n)"), + }, true + } + } + // No grouped import block; insert a standalone import after the package name. + seenImportFiles[file.Pos()] = true return analysis.TextEdit{ Pos: file.Name.End(), End: file.Name.End(), diff --git a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go index 6c4b87dbc8c..4ec2fcf2a63 100644 --- a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go +++ b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go @@ -3,17 +3,17 @@ 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` + return string(a) == string(b) // want `string\(a\) == string\(b\) is a \[\]byte comparison written the long way; use bytes\.Equal\(a, b\) for clearer intent` } func badNotEqual(a, b []byte) bool { - return string(a) != string(b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead` + return string(a) != string(b) // want `string\(a\) != string\(b\) is a \[\]byte comparison written the long way; use !bytes\.Equal\(a, b\) for clearer intent` } 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` + return string(a) == string(b) // want `string\(a\) == string\(b\) is a \[\]byte comparison written the long way; use bytes\.Equal\(a, b\) for clearer intent` } func goodBytesEqual(a, b []byte) bool { diff --git a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go.golden b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go.golden index 398552177cb..0d34c21d82d 100644 --- a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go.golden +++ b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go.golden @@ -3,17 +3,17 @@ 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` + return bytes.Equal(a, b) // want `string\(a\) == string\(b\) is a \[\]byte comparison written the long way; use bytes\.Equal\(a, b\) for clearer intent` } func badNotEqual(a, b []byte) bool { - return !bytes.Equal(a, b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead` + return !bytes.Equal(a, b) // want `string\(a\) != string\(b\) is a \[\]byte comparison written the long way; use !bytes\.Equal\(a, b\) for clearer intent` } 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` + return bytes.Equal(a, b) // want `string\(a\) == string\(b\) is a \[\]byte comparison written the long way; use bytes\.Equal\(a, b\) for clearer intent` } func goodBytesEqual(a, b []byte) bool { diff --git a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go index 9a7a119f8fd..0ce81214cc0 100644 --- a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go +++ b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go @@ -1,9 +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` + return string(a) == string(b) // want `string\(a\) == string\(b\) is a \[\]byte comparison written the long way; use bytes\.Equal\(a, b\) for clearer intent` } func noImportNotEqual(a, b []byte) bool { - return string(a) != string(b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead` + return string(a) != string(b) // want `string\(a\) != string\(b\) is a \[\]byte comparison written the long way; use !bytes\.Equal\(a, b\) for clearer intent` } diff --git a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go.golden b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go.golden index e42adc1baf1..6da48c240f4 100644 --- a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go.golden +++ b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go.golden @@ -3,9 +3,9 @@ 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` + return bytes.Equal(a, b) // want `string\(a\) == string\(b\) is a \[\]byte comparison written the long way; use bytes\.Equal\(a, b\) for clearer intent` } func noImportNotEqual(a, b []byte) bool { - return !bytes.Equal(a, b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead` + return !bytes.Equal(a, b) // want `string\(a\) != string\(b\) is a \[\]byte comparison written the long way; use !bytes\.Equal\(a, b\) for clearer intent` } diff --git a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go new file mode 100644 index 00000000000..7ebb0d3be2c --- /dev/null +++ b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go @@ -0,0 +1,13 @@ +package bytescomparestring + +import "fmt" + +func singleImportEqual(a, b []byte) bool { + fmt.Println("compare") + return string(a) == string(b) // want `string\(a\) == string\(b\) is a \[\]byte comparison written the long way; use bytes\.Equal\(a, b\) for clearer intent` +} + +func singleImportNotEqual(a, b []byte) bool { + fmt.Println("compare") + return string(a) != string(b) // want `string\(a\) != string\(b\) is a \[\]byte comparison written the long way; use !bytes\.Equal\(a, b\) for clearer intent` +} diff --git a/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go.golden b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go.golden new file mode 100644 index 00000000000..33c405e55cd --- /dev/null +++ b/pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go.golden @@ -0,0 +1,16 @@ +package bytescomparestring + +import ( + "bytes" + "fmt" +) + +func singleImportEqual(a, b []byte) bool { + fmt.Println("compare") + return bytes.Equal(a, b) // want `string\(a\) == string\(b\) is a \[\]byte comparison written the long way; use bytes\.Equal\(a, b\) for clearer intent` +} + +func singleImportNotEqual(a, b []byte) bool { + fmt.Println("compare") + return !bytes.Equal(a, b) // want `string\(a\) != string\(b\) is a \[\]byte comparison written the long way; use !bytes\.Equal\(a, b\) for clearer intent` +} diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index e4752598579..8c8fb89cecf 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -3,7 +3,7 @@ // 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 +// - 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 // - 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