Skip to content

bytescomparestring (new 44th linter): the "allocates" premise is inaccurate under gc — string(a) == string(b) does NOT allocate [Content truncated due to length] #44484

Description

@github-actions

Summary

Registry delta this run: 43 → 44 analyzers (grep -c Analyzer cmd/linters/main.go = 44; pkg/linters/doc.go:3 = "All 44 active analyzers"). The new 44th linter is bytescomparestring (cmd/linters/main.go:68), which flags string(a) == string(b) / string(a) != string(b) for []byte operands and suggests bytes.Equal(a, b) / !bytes.Equal(a, b).

Good news first — this linter is well-built and avoids every trap prior new linters fell into:

So there is no compile bug and no FP/FN here. The problem is the linter's stated rationale.

Finding 1 (primary): the "allocates" claim is factually wrong under the gc compiler

Both diagnostic messages and the package doc assert an allocation cost:

  • bytescomparestring.go:83"string(%s) == string(%s) allocates; use bytes.Equal(...) instead"
  • bytescomparestring.go:90"... allocates; use !bytes.Equal(...) instead"
  • bytescomparestring.go:1-3 (package doc) — "...to avoid unnecessary allocations."
  • pkg/linters/doc.go:6 — mirrors the same framing.

Under the gc compiler this is not true. A string([]byte) conversion whose result does not escape and is used only in a comparison is lowered to runtime.slicebytetostringtmp, which returns a string header aliasing the original backing array with no copy and no allocation. Comparison is exactly one of the documented non-escaping cases (runtime/string.go documents if string(b) == "..." / map-key / comparison usage as the safe non-allocating forms). In string(a) == string(b) both temporaries are consumed by the == and never escape, so gc allocates for neither side.

Net: string(a) == string(b) (both []byte) and bytes.Equal(a, b) are allocation-equivalent under gc. The rewrite's genuine value is idiomatic clarity and explicit intent (and robustness if the expression is later refactored so a conversion escapes) — not allocation avoidance.

Recommendation: reword the two messages and the package doc + doc.go line to drop the allocation claim and lead with clarity/idiom, e.g. "string(a) == string(b) is a []byte comparison written the long way; use bytes.Equal(a, b) for clearer intent". Sibling lenstringsplit.go:75 legitimately says "allocates a []string" because strings.Split genuinely allocates a slice — that message is fine; this one is the outlier.

Finding 2 (secondary, coupled to #1): enforce-readiness — but as a style lint

Production violation sites: zero. Broad scan grep -rEn 'string\([^)]*\) *(==|!=) *string\(' pkg/ cmd/ --include='*.go' (excluding tests/testdata and the linter's own doc strings) returns no hits. Combined with the RWSF test + nolint/test-skip parity, the analyzer is structurally CI-ready and is currently absent from cgo.yml LINTER_FLAGS (.github/workflows/cgo.yml:1208, 17 linters enforced), consistent with the rest of the un-enforced simplification family (appendbytestring, writebytestring, stringsindexcontains).

Recommendation: it can be added to the enforced set, but frame the enforcement as a readability/idiom gate, not a performance gate (see Finding 1). Do not justify CI enforcement on allocation grounds.

Finding 3 (minor): import-insertion misses the single non-grouped-import shape

addBytesImportEdit (bytescomparestring.go:139-150) only reuses an import block when genDecl.Lparen.IsValid() (grouped import ( ... )). A file whose only import is a single non-parenthesized import "fmt" skips that branch and falls through to the standalone insert (:152-157), producing a second top-level import "bytes" decl:

package foo

import "bytes"

import "fmt"

This is valid Go (compiles; gofmt/goimports would merge it), so it is low-severity, but it is untested — the two fixtures cover only "grouped import already present" and "no imports at all" (testdata/.../noimport.go). Consider folding the single-import case into the grouped branch (or add a fixture) so the fix emits one grouped block.

Validation checklist

  • Confirm via go build -gcflags=-m / escape dump (or a testing.AllocsPerRun micro-benchmark) that string(a) == string(b) for []byte reports 0 allocs on the target toolchain, then reword the two messages + package doc + doc.go:6.
  • Decide enforcement framing; if adding to cgo.yml LINTER_FLAGS, label it a style/idiom gate.
  • Add a fixture for a single non-grouped import "x" file (bytes not yet imported) and, if desired, make the fix reuse it as a grouped block.

Effort: S (messages/doc are one-line edits; import-edge is optional and localized to addBytesImportEdit).


Sergo R61 · strategy: registry-delta new-linter audit (44th=bytescomparestring) + novel message-premise validation. sg60a1=#44187 verified landed+closed; all prior sergo issues closed.

Generated by 🤖 Sergo - Serena Go Expert · 345.9 AIC · ⌖ 14.1 AIC · ⊞ 5.8K ·

  • expires on Jul 15, 2026, 9:15 PM UTC-08:00

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

cookieIssue Monster Loves Cookies!sergo

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions