Skip to content

Commit 1bc9983

Browse files
pelikhanpelikhan
authored
Merge branch 'main' into copilot/feature-content-redaction
2 parents 2509c72 + 43f17c7 commit 1bc9983

8 files changed

Lines changed: 93 additions & 23 deletions

File tree

‎pkg/linters/bytescomparestring/bytescomparestring.go‎

Lines changed: 53 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Package bytescomparestring implements a Go analysis linter that flags
22
// string(a) == string(b) and string(a) != string(b) comparisons where both
3-
// a and b are []byte values, which should use bytes.Equal(a, b) instead to
4-
// avoid unnecessary allocations.
3+
// a and b are []byte values, which should use bytes.Equal(a, b) instead for
4+
// clearer intent.
55
package bytescomparestring
66

77
import (
@@ -23,7 +23,7 @@ const bytesPkg = "bytes"
2323
// Analyzer is the bytes-compare-string analysis pass.
2424
var Analyzer = &analysis.Analyzer{
2525
Name: "bytescomparestring",
26-
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",
26+
Doc: "flags string(a) == string(b) and string(a) != string(b) as []byte comparisons written the long way; use bytes.Equal for clearer intent",
2727
URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/bytescomparestring",
2828
Requires: []*analysis.Analyzer{inspect.Analyzer},
2929
Run: run,
@@ -36,6 +36,11 @@ func run(pass *analysis.Pass) (any, error) {
3636
}
3737
noLintLinesByFile := nolint.BuildLineIndex(pass, "bytescomparestring")
3838

39+
// seenImportFiles tracks files that have already received a bytes import
40+
// TextEdit in this pass, preventing duplicate overlapping edits when a
41+
// single file contains multiple flagged comparisons.
42+
seenImportFiles := make(map[token.Pos]bool)
43+
3944
nodeFilter := []ast.Node{
4045
(*ast.BinaryExpr)(nil),
4146
}
@@ -75,20 +80,19 @@ func run(pass *analysis.Pass) (any, error) {
7580
return
7681
}
7782

78-
op := bin.Op.String()
7983
if bin.Op == token.EQL {
8084
pass.Report(analysis.Diagnostic{
8185
Pos: bin.Pos(),
8286
End: bin.End(),
83-
Message: fmt.Sprintf("string(%s) == string(%s) allocates; use bytes.Equal(%s, %s) instead", lText, rText, lText, rText),
84-
SuggestedFixes: buildFix(pass, bin, fmt.Sprintf("bytes.Equal(%s, %s)", lText, rText)),
87+
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),
88+
SuggestedFixes: buildFix(pass, bin, fmt.Sprintf("bytes.Equal(%s, %s)", lText, rText), seenImportFiles),
8589
})
8690
} else {
8791
pass.Report(analysis.Diagnostic{
8892
Pos: bin.Pos(),
8993
End: bin.End(),
90-
Message: fmt.Sprintf("string(%s) %s string(%s) allocates; use !bytes.Equal(%s, %s) instead", lText, op, rText, lText, rText),
91-
SuggestedFixes: buildFix(pass, bin, fmt.Sprintf("!bytes.Equal(%s, %s)", lText, rText)),
94+
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),
95+
SuggestedFixes: buildFix(pass, bin, fmt.Sprintf("!bytes.Equal(%s, %s)", lText, rText), seenImportFiles),
9296
})
9397
}
9498
})
@@ -98,13 +102,15 @@ func run(pass *analysis.Pass) (any, error) {
98102

99103
// buildFix returns the SuggestedFix for rewriting bin to replacement, adding a
100104
// "bytes" import TextEdit when the file containing bin does not yet import it.
101-
func buildFix(pass *analysis.Pass, bin *ast.BinaryExpr, replacement string) []analysis.SuggestedFix {
105+
// seenImportFiles tracks files that have already received an import edit in
106+
// this pass so that multi-violation files do not get duplicate overlapping edits.
107+
func buildFix(pass *analysis.Pass, bin *ast.BinaryExpr, replacement string, seenImportFiles map[token.Pos]bool) []analysis.SuggestedFix {
102108
edits := []analysis.TextEdit{{
103109
Pos: bin.Pos(),
104110
End: bin.End(),
105111
NewText: []byte(replacement),
106112
}}
107-
if importEdit, ok := addBytesImportEdit(pass, bin.Pos()); ok {
113+
if importEdit, ok := addBytesImportEdit(pass, bin.Pos(), seenImportFiles); ok {
108114
edits = append(edits, importEdit)
109115
}
110116
return []analysis.SuggestedFix{{
@@ -114,9 +120,11 @@ func buildFix(pass *analysis.Pass, bin *ast.BinaryExpr, replacement string) []an
114120
}
115121

116122
// addBytesImportEdit returns a TextEdit that inserts an import for "bytes" into
117-
// the file containing pos, unless "bytes" is already imported in that file.
123+
// the file containing pos, unless "bytes" is already imported in that file or
124+
// an import edit for this file has already been emitted in this pass
125+
// (tracked via seenImportFiles to prevent duplicate overlapping edits).
118126
// Returns (TextEdit{}, false) when no edit is needed.
119-
func addBytesImportEdit(pass *analysis.Pass, pos token.Pos) (analysis.TextEdit, bool) {
127+
func addBytesImportEdit(pass *analysis.Pass, pos token.Pos, seenImportFiles map[token.Pos]bool) (analysis.TextEdit, bool) {
120128
var file *ast.File
121129
for _, f := range pass.Files {
122130
if f.Pos() <= pos && pos <= f.End() {
@@ -128,28 +136,61 @@ func addBytesImportEdit(pass *analysis.Pass, pos token.Pos) (analysis.TextEdit,
128136
return analysis.TextEdit{}, false
129137
}
130138

139+
// Skip if an import edit for this file was already emitted in this pass.
140+
if seenImportFiles[file.Pos()] {
141+
return analysis.TextEdit{}, false
142+
}
143+
131144
// Check if "bytes" is already imported in this file.
132145
for _, imp := range file.Imports {
133146
if imp.Path.Value == `"`+bytesPkg+`"` {
134147
return analysis.TextEdit{}, false
135148
}
136149
}
137150

151+
// Compute the edit to add and mark the file so subsequent violations in the
152+
// same pass do not emit a duplicate overlapping TextEdit.
153+
138154
// Find an existing grouped import declaration to add into.
139155
for _, decl := range file.Decls {
140156
genDecl, ok := decl.(*ast.GenDecl)
141157
if !ok || genDecl.Tok != token.IMPORT || !genDecl.Lparen.IsValid() {
142158
continue
143159
}
144160
// Insert "bytes" before the closing paren of the import block.
161+
seenImportFiles[file.Pos()] = true
145162
return analysis.TextEdit{
146163
Pos: genDecl.Rparen,
147164
End: genDecl.Rparen,
148165
NewText: []byte("\t\"" + bytesPkg + "\"\n"),
149166
}, true
150167
}
151168

169+
// If the file has exactly one import (non-grouped), convert it to a grouped
170+
// import block while adding "bytes" before it (alphabetical order).
171+
if len(file.Imports) == 1 {
172+
for _, decl := range file.Decls {
173+
genDecl, ok := decl.(*ast.GenDecl)
174+
if !ok || genDecl.Tok != token.IMPORT || genDecl.Lparen.IsValid() || len(genDecl.Specs) != 1 {
175+
continue
176+
}
177+
178+
specText := astutil.NodeText(pass.Fset, genDecl.Specs[0])
179+
if specText == "" {
180+
continue
181+
}
182+
183+
seenImportFiles[file.Pos()] = true
184+
return analysis.TextEdit{
185+
Pos: genDecl.Pos(),
186+
End: genDecl.End(),
187+
NewText: []byte("import (\n\t\"" + bytesPkg + "\"\n\t" + specText + "\n)"),
188+
}, true
189+
}
190+
}
191+
152192
// No grouped import block; insert a standalone import after the package name.
193+
seenImportFiles[file.Pos()] = true
153194
return analysis.TextEdit{
154195
Pos: file.Name.End(),
155196
End: file.Name.End(),

‎pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,17 @@ package bytescomparestring
33
import "bytes"
44

55
func badEqual(a, b []byte) bool {
6-
return string(a) == string(b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead`
6+
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`
77
}
88

99
func badNotEqual(a, b []byte) bool {
10-
return string(a) != string(b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead`
10+
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`
1111
}
1212

1313
type myBytes []byte
1414

1515
func badNamedType(a, b myBytes) bool {
16-
return string(a) == string(b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead`
16+
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`
1717
}
1818

1919
func goodBytesEqual(a, b []byte) bool {

‎pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go.golden‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,17 @@ package bytescomparestring
33
import "bytes"
44

55
func badEqual(a, b []byte) bool {
6-
return bytes.Equal(a, b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead`
6+
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`
77
}
88

99
func badNotEqual(a, b []byte) bool {
10-
return !bytes.Equal(a, b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead`
10+
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`
1111
}
1212

1313
type myBytes []byte
1414

1515
func badNamedType(a, b myBytes) bool {
16-
return bytes.Equal(a, b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead`
16+
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`
1717
}
1818

1919
func goodBytesEqual(a, b []byte) bool {

‎pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
package bytescomparestring
22

33
func noImportEqual(a, b []byte) bool {
4-
return string(a) == string(b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead`
4+
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`
55
}
66

77
func noImportNotEqual(a, b []byte) bool {
8-
return string(a) != string(b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead`
8+
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`
99
}

‎pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go.golden‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@ package bytescomparestring
33
import "bytes"
44

55
func noImportEqual(a, b []byte) bool {
6-
return bytes.Equal(a, b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead`
6+
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`
77
}
88

99
func noImportNotEqual(a, b []byte) bool {
10-
return !bytes.Equal(a, b) // want `string\(a\) != string\(b\) allocates; use !bytes\.Equal\(a, b\) instead`
10+
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`
1111
}

‎pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
package bytescomparestring
2+
3+
import "fmt"
4+
5+
func singleImportEqual(a, b []byte) bool {
6+
fmt.Println("compare")
7+
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`
8+
}
9+
10+
func singleImportNotEqual(a, b []byte) bool {
11+
fmt.Println("compare")
12+
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`
13+
}

‎pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go.golden‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
package bytescomparestring
2+
3+
import (
4+
"bytes"
5+
"fmt"
6+
)
7+
8+
func singleImportEqual(a, b []byte) bool {
9+
fmt.Println("compare")
10+
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`
11+
}
12+
13+
func singleImportNotEqual(a, b []byte) bool {
14+
fmt.Println("compare")
15+
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`
16+
}

‎pkg/linters/doc.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
// All 44 active analyzers:
44
//
55
// - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...)
6-
// - 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
6+
// - 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
77
// - contextcancelnotdeferred — flags context cancel functions called directly instead of deferred
88
// - ctxbackground — flags context.Background() inside functions that already receive a context
99
// - deferinloop — flags defer statements placed directly inside for or range loop bodies

0 commit comments

Comments
 (0)