[linter-miner] linter: add bytescomparestring — flag string(a)==string(b) where a,b are []byte#44389
Conversation
…are []byte Reports string(a) == string(b) and string(a) != string(b) comparisons where both operands are []byte values. These conversions allocate; the idiomatic replacement is bytes.Equal(a, b) or !bytes.Equal(a, b). Evidence found in the codebase (3 locations): - pkg/parser/import_bfs.go:566: string(aJSON) == string(bJSON) - pkg/parser/tools_merger.go:279: string(aJSON) == string(bJSON) - pkg/cli/packages.go:146: string(existingContent) == string(sourceContent) Changes: - pkg/linters/bytescomparestring/: new analyzer with tests and golden file - cmd/linters/main.go: register bytescomparestring.Analyzer - pkg/linters/doc.go: update linter count (39→40) and add entry Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom go/analysis linter (bytescomparestring) to flag string(a)==string(b) / != comparisons where both sides are []byte conversions, and suggests bytes.Equal(a, b) / !bytes.Equal(a, b) to avoid allocations.
Changes:
- Introduces
pkg/linters/bytescomparestringanalyzer with SuggestedFix rewriting the full binary expression. - Adds analysistest coverage + fixtures/golden outputs for the new analyzer.
- Registers the analyzer in
cmd/lintersand updates the linter package docs.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/bytescomparestring/bytescomparestring.go | Implements analyzer that detects string([]byte) ==/!= string([]byte) and suggests bytes.Equal fixes. |
| pkg/linters/bytescomparestring/bytescomparestring_test.go | Adds analysistest harness for SuggestedFix validation. |
| pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go | Test fixture with “bad” and “good” patterns. |
| pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go.golden | Expected post-fix output for the fixture. |
| cmd/linters/main.go | Registers the new analyzer in the multichecker runner. |
| pkg/linters/doc.go | Updates linter documentation list/count and adds bytescomparestring entry. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Low
| multichecker.Main( | ||
| appendbytestring.Analyzer, | ||
| bytescomparestring.Analyzer, | ||
| contextcancelnotdeferred.Analyzer, |
| // All 40 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 |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Documents the decision to add a custom go/analysis linter that flags string(a)==string(b) comparisons on []byte values and suggests bytes.Equal. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (248 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two correctness gaps and two test-coverage gaps.
📋 Key Themes & Highlights
Blocking Issues
- Missing import text-edit in SuggestedFix: The automated fix emits
bytes.Equal(...)but never injects the"bytes"import. Files that don't already importbyteswill fail to compile after applying the fix withgopls/go fix. - Silent early-return on empty NodeText: If
astutil.NodeTextreturns an empty string the diagnostic is silently dropped, but there's no test that exercises complex sub-expressions (e.g.string(buf.Bytes())).
Minor Issues
- Duplicated
analysis.Diagnosticconstruction: The==and!=branches build structurally identical diagnostics; extracting a helper would reduce maintenance drift. - Named-type
!=test case missing: The fixture only testsmyByteswith==; a symmetric!=case would confirm operator dispatch is correct for named byte-slice types.
Positive Highlights
- ✅ Solid type-checking logic — correctly uses
types.TypesInfoand.Underlying()to handle named types without false positives - ✅ Clean zero-false-positive promise, well-described in the PR body
- ✅ Good negative-case coverage in the fixture (
goodStringLiteral,goodStringVars,goodMixedOneSideString) - ✅ Consistent with the existing
appendbytestringlinter's structure and patterns
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 71 AIC · ⌖ 8.22 AIC · ⊞ 6.6K
Comment /matt to run again
| TextEdits: []analysis.TextEdit{{ | ||
| Pos: bin.Pos(), | ||
| End: bin.End(), | ||
| NewText: []byte(fmt.Sprintf("bytes.Equal(%s, %s)", lText, rText)), |
There was a problem hiding this comment.
[/tdd] The SuggestedFix emits bytes.Equal(...) but does not add a "bytes" import text-edit. For callers without an existing bytes import, applying the fix via gopls/go fix produces non-compilable code.\n\n
💡 Suggested approach
\n\nConsider adding a secondTextEdit to insert the import, or at minimum document that goimports must run after applying the fix. The sibling appendbytestring linter avoids this issue because its fix never introduces a new package symbol.\n\n|
|
||
| func badNamedType(a, b myBytes) bool { | ||
| return string(a) == string(b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead` | ||
| } |
There was a problem hiding this comment.
[/tdd] The test covers named types (myBytes) for the == case but not for !=. A badNamedTypeNotEqual case would close that gap and confirm the operator-dispatch logic is symmetric for non-[]byte underlying types.\n\n@copilot please address this.
| func goodMixedOneSideString(a []byte, b string) bool { | ||
| // One side is a string variable, not string([]byte); not flagged. | ||
| return string(a) == b | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing negative test: string(a) == string(b) where one argument is []uint8 and the other is a named []byte alias. The isByteSlice helper accepts both because it checks types.Byte, but having a fixture case makes that guarantee explicit and prevents future regressions.\n\n@copilot please address this.
| return | ||
| } | ||
|
|
||
| op := bin.Op.String() |
There was a problem hiding this comment.
[/codebase-design] The if/else on bin.Op (lines 77–105) duplicates the analysis.Diagnostic structure; both branches differ only in message template and replacement text prefix. Extracting a small helper reduces the divergence surface.
💡 Suggested refactor
Extract a helper that accepts the op-specific strings and returns analysis.Diagnostic, then call it from both branches. This mirrors how appendbytestring keeps its reporting in a single buildFix function.
@copilot please address this.
| if !ok { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] Missing edge case: what happens when either lText or rText is empty (line 69 guard returns early) — the diagnostic is silently suppressed. Add a test fixture with a complex expression like string(f()) or string(buf.Bytes()) to verify the linter fires even when astutil.NodeText might produce multi-token text, and confirm the early-return is never reachable for valid Go ASTs.\n\n@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — one high-severity correctness bug must be fixed before this can be used safely.
Blocking issue: SuggestedFix generates uncompilable code
The linter detection logic is correct and the diagnostic messages are accurate. However, the SuggestedFix emitted for both == and != cases replaces the expression with bytes.Equal(...) / !bytes.Equal(...) but never adds the "bytes" import. Every file that applies the auto-fix via gopls or go fix and does not already import "bytes" will fail to compile.
This is not hypothetical: all three real-world violations cited in the PR description (import_bfs.go, tools_merger.go, packages.go) were verified to have no "bytes" import — so applying the suggested fix to each of them produces broken code today.
The test suite does not catch this because the single test fixture already imports "bytes" (for goodBytesEqual), so RunWithSuggestedFixes never exercises the import-absent path.
What needs to change
- Add import-insertion
TextEditto bothSuggestedFixinstances (EQL and NEQ branches). Usegolang.org/x/tools/go/ast/astutil.AddImportor compute the edit by locating the file's import block. - Add a
noimporttest fixture (a package with the bad pattern but no pre-existing"bytes"import) and a corresponding.goldenfile that includes the import. This will fail until fix #1 is in place and prevents regression.
🔎 Code quality review by PR Code Quality Reviewer · 104.1 AIC · ⌖ 5.47 AIC · ⊞ 5.4K
Comment /review to run again
|
|
||
| // 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) { |
There was a problem hiding this comment.
SuggestedFix emits bytes.Equal(...) without adding the "bytes" import, producing uncompilable code on every real violation in this repo.
💡 Details and fix
The TextEdit replaces the binary expression with bytes.Equal(a, b) or !bytes.Equal(a, b), but never inserts an import "bytes" declaration. Applying the fix to any file that does not already import "bytes" results in a compile error (undefined: bytes).
All three live callsites identified in the PR description have been verified to lack the "bytes" import:
pkg/parser/import_bfs.go— importsencoding/json,errors,fmt,maps,path,path/filepath,strings— no"bytes"pkg/parser/tools_merger.go— importsencoding/json,fmt,maps,strings— no"bytes"pkg/cli/packages.go— importsbufio,fmt,os,path/filepath,regexp,strings— no"bytes"
So every suggested fix the linter would propose in this codebase is broken out of the box.
Fix: The analysis.SuggestedFix can include multiple TextEdit entries. Add a second edit that inserts "bytes" into the file's import block. Use golang.org/x/tools/go/ast/astutil.AddImport to locate the right position and generate the edit, or locate the end of the existing import (...) block and inject \t\"bytes\"\n there.
Note: the appendbytestring peer linter avoids this entirely because its fix removes a conversion rather than adding a new package reference.
| @@ -0,0 +1,37 @@ | |||
| package bytescomparestring | |||
|
|
|||
| import "bytes" | |||
There was a problem hiding this comment.
Test fixture masks the missing-import bug: it already imports "bytes", so RunWithSuggestedFixes never exercises the broken path.
💡 Details and suggested test
The testdata file imports "bytes" on line 3 solely because goodBytesEqual uses bytes.Equal. This means analysistest.RunWithSuggestedFixes applies the generated TextEdits to a file that already has the import, and the golden-file comparison passes even though the linter never emits an import-adding edit.
There is no test for the real-world scenario where the file with the bad pattern has no pre-existing "bytes" import — which is true of all three live violations in this repo.
Suggested addition: add a second fixture package (e.g. testdata/src/noimport/noimport.go) that has the bad pattern but no "bytes" import:
package noimport
func bad(a, b []byte) bool {
return string(a) == string(b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead`
}And a corresponding .golden file that adds the "bytes" import:
package noimport
import "bytes"
func bad(a, b []byte) bool {
return bytes.Equal(a, b) // want `string\(a\) == string\(b\) allocates; use bytes\.Equal\(a, b\) instead`
}This test will fail until the import-insertion TextEdit is added to the SuggestedFix, enforcing the fix is correct end-to-end.
There was a problem hiding this comment.
Review: bytescomparestring linter
The linter logic is correct and well-structured — the type-based detection is precise, and the tests cover the key true/false-positive cases. One blocking issue needs to be addressed before merge.
🔴 Blocking: SuggestedFix omits the "bytes" import
All three real-world instances called out in the PR description (import_bfs.go, tools_merger.go, packages.go) do not import "bytes". Applying the SuggestedFix via go fix ./... will rewrite those expressions to bytes.Equal(...) without adding the import, producing build errors.
The test fixture passes only because it already contains import "bytes", so this gap is invisible in CI.
Fix options:
- Add a
TextEditfor the import statement alongside the expression rewrite (usegolang.org/x/tools/go/ast/astutil.AddImportto locate the correct insertion point), or - Add a test fixture that does not pre-import
"bytes"and confirmanalysistest.RunWithSuggestedFixesstill produces a compiling result.
🟡 Non-blocking: op variable asymmetry
op is computed but only used in the else (NEQ) branch; the EQL branch hardcodes " == ". This is correct but inconsistent — see inline comment.
What looks good
- Type-based detection via
types.TypesInfois robust — no string-matching, no false positives onstring(a) == "literal"orstring(a) == stringVar. - Named-type coverage (
type myBytes []byte) via.Underlying()is correct. nolintandfilecheck.IsTestFileguards are wired in properly.doc.goupdated with correct count and description.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 95.8 AIC · ⌖ 5.42 AIC · ⊞ 4.8K
| 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 |
There was a problem hiding this comment.
Blocking: The SuggestedFix rewrites the expression to bytes.Equal(...) but does not add a "bytes" import if the file doesn't already have one. All 3 real-world instances flagged in the PR description (import_bfs.go, tools_merger.go, packages.go) do not import "bytes", so applying this fix via go fix would produce non-compiling code.
The test fixture passes only because it pre-imports "bytes", masking this gap.
Either:
- Add a second
TextEditthat inserts the"bytes"import (usegolang.org/x/tools/go/ast/astutil.AddImportto get the correct position), or - Add a test case where
bytesis not pre-imported to confirmanalysistest.RunWithSuggestedFixesworks end-to-end.
Without this, go fix ./... will break compilation for the very instances this linter flags.
@copilot please address this.
| Pos: bin.Pos(), | ||
| End: bin.End(), | ||
| NewText: []byte(fmt.Sprintf("!bytes.Equal(%s, %s)", lText, rText)), | ||
| }}, |
There was a problem hiding this comment.
Non-blocking: op is only used in the else branch (line 121 — the != diagnostic message). In the if bin.Op == token.EQL branch the string " == " is hardcoded. Consider either:
- Remove
opand hardcode" != "in the else branch too (more consistent), or - Use
opin both branches to ensure the message always reflects the actual operator.
As written the code is correct, but the asymmetry is easy to misread.
@copilot please address this.
|
Run: https://github.com/github/gh-aw/actions/runs/28969326363
|
…iolations, update docs - Fix 3 live instances of string([]byte)==string([]byte) in pkg/parser/import_bfs.go, pkg/parser/tools_merger.go, and pkg/cli/packages.go (migrate to bytes.Equal) - Update SuggestedFix to include a TextEdit adding 'bytes' import when not present - Add noimport.go + noimport.go.golden test fixtures for the no-import code path - Fix pkg/linters/doc.go: update count 40→44, add httprespbodyclose, httpstatuscode, osgetenvlibrary, sprintfint to the analyzer list - Update ADR status Draft→Accepted, fix linter count references Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in commit
|
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new custom
go/analysislinterbytescomparestringthat flagsstring(a) == string(b)andstring(a) != string(b)comparisons where bothaandbare[]bytevalues. These comparisons create unnecessary heap allocations; the idiomatic and allocation-free replacement isbytes.Equal(a, b)/!bytes.Equal(a, b).Evidence
Code-pattern scan found 3 live instances in the repo:
pkg/parser/import_bfs.goreturn string(aJSON) == string(bJSON)pkg/parser/tools_merger.goreturn string(aJSON) == string(bJSON)pkg/cli/packages.goif string(existingContent) == string(sourceContent)What the linter catches
The linter only fires when both sides of
==/!=arestring(x)type-conversions whose argument has underlying type[]byte. False-positive rate is zero: it will not flagstring(a) == "literal",string(a) == stringVar, or regular string comparisons.Suggested fix
The analyzer emits a
SuggestedFixthat rewrites the entire binary expression in-place, sogopls/go fixcan apply it automatically.Changes
pkg/linters/bytescomparestring/bytescomparestring.go— analyzer implementationpkg/linters/bytescomparestring/bytescomparestring_test.go— analysis testpkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go— fixture filepkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go.golden— expected fixescmd/linters/main.go— registersbytescomparestring.Analyzerpkg/linters/doc.go— updates linter count (39→40) and adds entryTesting