Problem
stringbytesroundtrip.isExactString is a no-op alias of isStringType — it currently produces the right answer only by accident, because of which type snapshot (.Underlying() vs. raw) each call site happens to pass in. This is the exact same distinction whose absence caused a shipped, non-compiling autofix in the sibling linter writebytestring (fixed as sg60a1).
Location
pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go:138-145
func isStringType(t types.Type) bool {
basic, ok := t.(*types.Basic)
return ok && basic.Kind() == types.String
}
func isExactString(t types.Type) bool {
return isStringType(t)
}isExactString is meant to distinguish the predeclared string type from named string types (type MyString string), the same distinction writebytestring.isExactString implements as:
func isExactString(t types.Type) bool {
_, ok := t.(*types.Basic)
return ok
}In stringbytesroundtrip, isExactString is defined identically to isStringType. It "works" today only because every call to isExactString (line 113) passes the raw type from pass.TypesInfo.TypeOf(...), while every call to isStringType for the gating check (lines 107, 128) passes an already-.Underlying()-resolved type. *types.Basic type-assertion succeeds on a raw predeclared type and fails on *types.Named, so the exactness check happens to hold — but nothing in the function signature, its name, or its body enforces that discipline.
Impact
This is a latent correctness risk, not a live bug today (this analyzer only emits diagnostics, no SuggestedFix, for the branch that calls isExactString, so there's no autofix-doesn't-compile exposure yet). But:
- A future contributor adding an autofix to the "genuinely redundant" branch (the doc comment at the top of the file already anticipates this: "For named string types, the inner conversion can still be removed, but an outer conversion may be necessary") would reasonably assume
isExactString does real exactness checking, and could reuse it with an .Underlying()-resolved type by mistake — silently reintroducing the same class of bug as sg60a1.
- Any refactor of
isStringType (e.g. to also accept untyped constants, or to add an .Underlying() call for consistency with its own name) would silently break isExactString without any test signal, since they're the same function.
Recommendation
Implement isExactString independently of isStringType, e.g.:
func isExactString(t types.Type) bool {
_, isNamed := t.(*types.Named)
return isStringType(t.Underlying()) && !isNamed
}(or reuse writebytestring's *types.Basic assertion directly), and add a regression test case with a named string type (e.g. type namedString string) feeding both round-trip branches, so a future autofix on this branch can't reintroduce the sg60a1 bug class silently.
Validation checklist
Effort
Small — isolated to two small functions plus a testdata fixture addition.
Generated by 🤖 Sergo - Serena Go Expert · agent · 275.9 AIC · ⌖ 9.07 AIC · ⊞ 6.8K · ◷
Problem
stringbytesroundtrip.isExactStringis a no-op alias ofisStringType— it currently produces the right answer only by accident, because of which type snapshot (.Underlying()vs. raw) each call site happens to pass in. This is the exact same distinction whose absence caused a shipped, non-compiling autofix in the sibling linterwritebytestring(fixed assg60a1).Location
pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go:138-145isExactStringis meant to distinguish the predeclaredstringtype from named string types (type MyString string), the same distinctionwritebytestring.isExactStringimplements as:In
stringbytesroundtrip,isExactStringis defined identically toisStringType. It "works" today only because every call toisExactString(line 113) passes the raw type frompass.TypesInfo.TypeOf(...), while every call toisStringTypefor the gating check (lines 107, 128) passes an already-.Underlying()-resolved type.*types.Basictype-assertion succeeds on a raw predeclared type and fails on*types.Named, so the exactness check happens to hold — but nothing in the function signature, its name, or its body enforces that discipline.Impact
This is a latent correctness risk, not a live bug today (this analyzer only emits diagnostics, no
SuggestedFix, for the branch that callsisExactString, so there's no autofix-doesn't-compile exposure yet). But:isExactStringdoes real exactness checking, and could reuse it with an.Underlying()-resolved type by mistake — silently reintroducing the same class of bug assg60a1.isStringType(e.g. to also accept untyped constants, or to add an.Underlying()call for consistency with its own name) would silently breakisExactStringwithout any test signal, since they're the same function.Recommendation
Implement
isExactStringindependently ofisStringType, e.g.:(or reuse
writebytestring's*types.Basicassertion directly), and add a regression test case with a named string type (e.g.type namedString string) feeding both round-trip branches, so a future autofix on this branch can't reintroduce thesg60a1bug class silently.Validation checklist
isExactStringno longer delegates toisStringType; it has its own logic distinguishing*types.Basicfrom*types.NamedanalysistestisExactStringdocuments why it must differ fromisStringType(raw type vs. underlying type expectations)Effort
Small — isolated to two small functions plus a testdata fixture addition.