Reframe bytescomparestring as style/intent lint and fix single-import autofix output#44488
bytescomparestring as style/intent lint and fix single-import autofix output#44488Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
bytescomparestring as style/intent lint and fix single-import autofix output
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44488 does not have the 'implementation' label and has only 54 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The changes affect the bytescomparestring linter implementation and its testdata fixtures (golden files and input fixtures), but no *_test.go or *.test.cjs files were touched. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
This pull request updates the bytescomparestring analyzer to frame string(a) == string(b) / != as a readability/intent issue (not an allocation claim), and improves the suggested-fix import insertion so it can rewrite import "x" into a grouped import when adding bytes.
Changes:
- Reword diagnostics and package documentation to remove allocation claims and emphasize clearer intent via
bytes.Equal. - Extend the suggested-fix import edit logic to handle single non-grouped imports by converting them to an import block when adding
bytes. - Update/add testdata fixtures and goldens to cover the new diagnostic text and the single-import autofix shape.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/doc.go | Updates the analyzer list entry to match the new “clearer intent” framing. |
| pkg/linters/bytescomparestring/bytescomparestring.go | Rewords diagnostics/docs and updates import autofix behavior for import "x" files. |
| pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go | Updates want expectations for the new diagnostic message text. |
| pkg/linters/bytescomparestring/testdata/src/bytescomparestring/bytescomparestring.go.golden | Updates golden output to match the new message text. |
| pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go | Updates want expectations for the new diagnostic message text. |
| pkg/linters/bytescomparestring/testdata/src/bytescomparestring/noimport.go.golden | Updates golden output to match the new message text. |
| pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go | Adds a new fixture covering import "fmt" (single non-grouped import) behavior. |
| pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go.golden | Adds the golden output for the new single-import fixture. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Low
| // If the file has a single non-grouped import declaration, convert it to a | ||
| // grouped import block while adding "bytes". | ||
| 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 | ||
| } | ||
|
|
||
| return analysis.TextEdit{ | ||
| Pos: genDecl.Pos(), | ||
| End: genDecl.End(), | ||
| NewText: []byte("import (\n\t" + specText + "\n\t\"" + bytesPkg + "\"\n)"), | ||
| }, true | ||
| } |
![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 /codebase-design and /tdd — requesting changes on two focused issues.
📋 Key Themes & Highlights
Issues Found
-
Import ordering (
bytescomparestring.go:167) — The new single-import rewrite hardcodes the existing spec first, then appends"bytes". Since"bytes" < "fmt"alphabetically, this produces non-canonical import order that goimports would reorder. The golden filesingleimport.go.goldenalready shows the non-sorted order, so the golden itself needs fixing too. -
Analyzer.Docnot updated (bytescomparestring.go:26) — The struct-levelDocfield still says"that should use bytes.Equal instead"(the old phrasing), while every other surface (package doc, diagnostic messages,pkg/linters/doc.go) was updated to the new intent rationale. Small inconsistency visible to programmatic consumers. -
singleimportfixture only tests==— The!=operator takes a separate code path; asingleImportNotEqualtest case would make the fixture complete.
Positive Highlights
- ✅ Correct technical rationale: the allocation claim was inaccurate under gc for non-escaping temporaries; the style/intent framing is more defensible.
- ✅ The single-import autofix gap was a real correctness issue (would have emitted a second standalone
importdeclaration); the fix is clean and well-placed. - ✅ New fixture + golden pair is a good pattern and matches the existing testdata conventions.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 52.9 AIC · ⌖ 8.15 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/linters/bytescomparestring/bytescomparestring.go:167
[/codebase-design] Import ordering: the new template appends "bytes" after the existing spec, producing "fmt"/"bytes" — but Go style (goimports) sorts alphabetically, so bytes should come before fmt.
<details>
<summary>💡 Suggestion</summary>
Insert "bytes" before the existing spec when it sorts earlier alphabetically. The current golden singleimport.go.golden already shows "fmt" before "bytes", which goimports would reorder. Fix the emission order and update the golde…
pkg/linters/bytescomparestring/bytescomparestring.go:26
[/codebase-design] The Analyzer.Doc field (line 26) still says "that should use bytes.Equal instead" — it was not updated to match the new style/intent rationale used in the package doc, diagnostic messages, and pkg/linters/doc.go. This creates a small but observable inconsistency when users inspect the analyzer programmatically.
<details>
<summary>💡 Suggested fix</summary>
Doc: "reports string(a) == string(b) and string(a) != string(b) comparisons where a and b are []byte va…
</details>
<details><summary>pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go:7</summary>
**[/tdd]** The `singleimport` fixture covers only the `==` (EQL) operator. The symmetrical `!=` (NEQ) path in the same single-import scenario is untested — both operators produce separate code paths that call `buildFix`/`addBytesImportEdit`, and a future refactor could break one without the other.
<details>
<summary>💡 Suggestion</summary>
Add a second function to `singleimport.go`:
```go
func singleImportNotEqual(a, b []byte) bool {
fmt.Println("compare")
return string(a) != string(…
</details>
There was a problem hiding this comment.
Review summary
The PR is well-scoped and the core changes (reframing the diagnostic as style/intent, fixing single-import autofix) are correct. Two issues need addressing before merge:
Blocking:
Analyzer.Docfield not updated —bytescomparestring.go:26still uses the old allocation-framing text while the package comment, diagnostic messages, anddoc.gohave all been updated. IDE integrations andgo vet -jsonwill surface this stale text.
Non-blocking:
- Non-alphabetical import order in the singleimport golden — the autofix places
"bytes"after"fmt". Sincebytessorts beforefmt, inserting it first would produce already-canonical output and avoid a follow-upgoimportschurn for users.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 54.4 AIC · ⌖ 5.93 AIC · ⊞ 4.8K
Comments that could not be inline-anchored
pkg/linters/bytescomparestring/bytescomparestring.go:26
The Analyzer.Doc field still says "that should use bytes.Equal instead" (the old allocation-framing wording), while the package comment, diagnostic messages, and doc.go have all been updated to the new "clearer intent" framing. This creates an inconsistency — tools that surface the Doc field (e.g. go vet -json, IDE integrations) will show the old text.
Suggested fix:
Doc: "reports string(a) == string(b) and string(a) != string(b) comparisons where a and b are []byte values …
</details>
<details><summary>pkg/linters/bytescomparestring/testdata/src/bytescomparestring/singleimport.go.golden:5</summary>
The autofix emits `"bytes"` after `"fmt"`, producing non-alphabetical import order. Users who run `goimports` (or have editor formatters) after applying the fix will see a follow-up diff reordering the block.
Consider inserting alphabetically — `"bytes"` sorts before `"fmt"`, so put it first:
```go
import (
\t"bytes"
\t"fmt"
)This makes the autofix output already-canonical and avoids the double-edit.
@copilot please address this.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 191.6 AIC · ⌖ 6.8 AIC · ⊞ 5.4K
Comment /review to run again
| return analysis.TextEdit{ | ||
| Pos: genDecl.Pos(), | ||
| End: genDecl.End(), | ||
| NewText: []byte("import (\n\t" + specText + "\n\t\"" + bytesPkg + "\"\n)"), |
There was a problem hiding this comment.
Import sort order violation: "bytes" is always appended after the existing import, producing non-canonical order that goimports will immediately reformat.
💡 Suggested fix
The current NewText construction:
NewText: []byte("import (\n\t" + specText + "\n\t\"" + bytesPkg + "\"\n)"),places the original import before "bytes" unconditionally. For import "fmt" this yields:
import (
"fmt"
"bytes"
)goimports canonically sorts alphabetically within the stdlib group, so "bytes" must come before "fmt". The golden file at testdata/.../singleimport.go.golden confirms the wrong order is now checked in.
Minimal fix — sort the two specifiers before constructing the block:
newSpec := "\"" + bytesPkg + "\""
first, second := specText, newSpec
if newSpec < specText {
first, second = newSpec, specText
}
NewText: []byte("import (\n\t" + first + "\n\t" + second + "\n)"),Note: the existing grouped-import path (line ~147) also appends without sorting — that is a separate pre-existing issue.
|
|
||
| // If the file has a single non-grouped import declaration, convert it to a | ||
| // grouped import block while adding "bytes". | ||
| for _, decl := range file.Decls { |
There was a problem hiding this comment.
Duplicate TextEdit conflict on multi-violation files: if a single-import file contains two or more flagged comparisons, both buildFix calls generate a TextEdit that replaces the identical range [genDecl.Pos(), genDecl.End()] with the same grouped-import block. The go/analysis framework may apply both edits or panic depending on the tooling, and no test covers this scenario.
💡 Explanation and fix
addBytesImportEdit is called once per flagged BinaryExpr. For a file like:
import "fmt"
func f(a, b []byte) bool { return string(a) == string(b) }
func g(a, b []byte) bool { return string(a) == string(b) }Two diagnostics are reported and each SuggestedFix includes an independent TextEdit covering [genDecl.Pos(), genDecl.End()]. When both fixes are applied together (e.g. via go fix -fix=bytescomparestring), the overlapping TextEdits for the import rewrite can corrupt the file or cause the framework to emit "conflicting text edits" errors.
The standard pattern to avoid this is to make the import-rewrite edit idempotent at the source span: once the edit has been generated for a file, skip it for subsequent diagnostics in the same file. This can be done by tracking file.Pos() in a local map per pass.Files iteration, or — even simpler — relying on the existing "already imported?" check that runs at the top of addBytesImportEdit (which already handles the case where bytes is already imported).
The real gap is that after the first suggested fix is applied the import is present, but during a single analysis pass both diagnostics are generated simultaneously before any fix is applied, so both carry the import edit. Add a test file with two violations in a single-import context to make this concrete.
|
@copilot please run the Unresolved review feedback to address first:
Run: https://github.com/github/gh-aw/actions/runs/29000324183
|
…t guard, and != coverage Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in the latest commit. Changes made:
|
|
@copilot please run the
|
All review feedback was addressed in the previous commit ( |
|
@copilot merge main and recompile |
…tring-fix-allocates-premise Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged main ( |
|
@copilot please run the Unresolved review feedback to address first:
Run: https://github.com/github/gh-aw/actions/runs/29016118662
|
All three unresolved threads are addressed in the current code (all are marked
Local validation is green. No further code changes are needed; CI needs a human re-trigger to reflect the current HEAD.
|
🤖 PR Triage — Run §29020837798
Score breakdown: Impact 22 + Urgency 15 + Quality 15 Rationale: Linter correctness fix — reframes inaccurate allocation claims and fixes single-import autofix. Small (67 +/ 17 −) with updated test goldens. No user-facing behavior change. Fits naturally with other lint-fixes batch.
|
|
@copilot please run the Unresolved review feedback:
|
All three threads are already addressed in the current code at
Local validation: |
|
|
…same file Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in
|
The new
bytescomparestringanalyzer was functionally correct but describedstring(a) == string(b)/!=as allocating, which is inaccurate under gc for non-escaping comparison temporaries. This change updates the linter rationale to readability/idiom and tightens its suggested-fix import behavior for single-import files.Diagnostic + docs now match actual compiler behavior
[]bytecomparison written the long way; usebytes.Equalfor clearer intent.”pkg/linters/doc.goto match.Suggested-fix import insertion handles single non-grouped imports
addBytesImportEditto detectimport "x"shape and rewrite it to a grouped import when addingbytes, instead of emitting a second top-level import declaration.Testdata updated for message + import-shape coverage
wantexpectations and goldens for the new diagnostic text.import "fmt"(single non-grouped import) case.