Reframe bytescomparestring as style/intent lint and fix single-import autofix output

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,28 +136,61 @@ 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+`"` {
 			return analysis.TextEdit{}, false
 		}
 	}
 
+	// 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)
 		if !ok || genDecl.Tok != token.IMPORT || !genDecl.Lparen.IsValid() {
 			continue
 		}
 		// Insert "bytes" before the closing paren of the import block.
+		seenImportFiles[file.Pos()] = true
 		return analysis.TextEdit{
 			Pos:     genDecl.Rparen,
 			End:     genDecl.Rparen,
 			NewText: []byte("\t\"" + bytesPkg + "\"\n"),
 		}, 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(),

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 {

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 {

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`
 }

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`
 }

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`
+}

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`
+}

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