[linter-miner] linter: add appendbytestring — flag redundant []byte(s) conversion in append calls#43423
Conversation
Reports append(b, []byte(s)...) where s is a string value and b is []byte. The []byte(s) conversion is unnecessary because the Go spec permits appending a string directly to a []byte slice with an ellipsis: append(b, s...) // no allocation The []byte(s) form allocates a temporary byte slice on every call, which is wasteful and less readable. The linter: - detects the exact pattern using type-aware AST analysis - skips test files and nolint-annotated lines - emits a suggested fix rewriting []byte(s) to s Evidence: the pattern appears in pkg/workflow/action_cache.go and matches the style of existing linters (largefunc, sprintfint). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
Great addition from the Linter Miner workflow! 🎉 The All checklist items pass:
This PR looks ready for review and merge. 🚀
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds a new appendbytestring Go analysis linter to the gh-aw multichecker, intended to flag and auto-fix redundant []byte(string) conversions inside append calls (e.g. append(b, []byte(s)...) → append(b, s...)) to avoid unnecessary allocations.
Changes:
- Introduces the
pkg/linters/appendbytestringanalyzer that detectsappend(b, []byte(s)...)whenbis[]byteandsis a string. - Registers the new analyzer in
cmd/linters/main.go. - Adds analysistest coverage and fixtures for flagged, unflagged, and
//nolint:appendbytestring-suppressed cases.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/appendbytestring/appendbytestring.go | Implements the analyzer and its suggested fix for redundant []byte(string) conversions in append. |
| pkg/linters/appendbytestring/appendbytestring_test.go | Adds unit test harness for the new analyzer (currently not validating suggested fixes output). |
| pkg/linters/appendbytestring/testdata/src/appendbytestring/appendbytestring.go | Provides test fixtures for diagnostics and suppression behavior. |
| cmd/linters/main.go | Registers appendbytestring.Analyzer in the linter multichecker. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Low
| // Replace the entire second argument []byte(s) with just s. | ||
| // The ellipsis token follows the closing paren of the outer append call, | ||
| // so we only need to rewrite conv.Pos()..conv.End() to sText. | ||
| return []analysis.SuggestedFix{{ | ||
| Message: "Replace []byte(s) with s in append", | ||
| TextEdits: []analysis.TextEdit{ | ||
| { | ||
| Pos: conv.Pos(), | ||
| End: token.Pos(int(call.Rparen)), // rewrite up to (but not including) the closing paren of append | ||
| NewText: []byte(sText), | ||
| }, |
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, appendbytestring.Analyzer, "appendbytestring") | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Testdata scenarios covered (via
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.
|
ADR documents the decision to add a custom Go static analysis linter that flags the redundant append(b, []byte(s)...) conversion pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (214 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.
Review: appendbytestring linter
The detection logic is solid and the overall structure matches the codebase conventions. There are two blocking issues to fix before merging.
🔴 Blocking
-
buildFixproduces broken code (line 148 ofappendbytestring.go)
End: token.Pos(int(call.Rparen))spans[]byte(s)...(the...is betweenconv.End()andcall.Rparen). Replacing that entire range with the bare string expression drops the ellipsis, turningappend(b, s...)intoappend(b, s)— a compile error or silent semantics change. Fix: useconv.End()as theEndof the text edit. -
Suggested fix is never tested (
appendbytestring_test.goline 15)
analysistest.Rundoes not exercise fix rewrites. All other linters in this repo that emit fixes useanalysistest.RunWithSuggestedFixeswith a.goldenfile. Without that, the bug above passes CI silently.
Non-blocking observations
isByteSliceConversionchecks for the literal identifierbytebut notuint8, which is the same underlying type. In practice[]uint8(s)is virtually never written, so this is an acceptable trade-off, but worth noting.- The
filecheck.IsTestFileguard is appropriately placed before the type-check lookup. - Nolint suppression is correctly wired.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 55.4 AIC · ⌖ 6.15 AIC · ⊞ 4.9K
| TextEdits: []analysis.TextEdit{ | ||
| { | ||
| Pos: conv.Pos(), | ||
| End: token.Pos(int(call.Rparen)), // rewrite up to (but not including) the closing paren of append |
There was a problem hiding this comment.
Bug: incorrect End position in suggested fix — drops the ellipsis.
The text edit sets:
End: token.Pos(int(call.Rparen)), // rewrite up to (but not including) the closing parencall.Rparen is the position of ) in append(b, []byte(s)...). The source span conv.Pos()..call.Rparen covers []byte(s)... — the trailing ... is included in that range. Replacing the whole span with sText (e.g. s) therefore produces append(b, s), which drops the ellipsis and silently changes semantics (string treated as a single byte value, not expanded as its bytes).
The fix should only cover the conversion expression itself:
TextEdits: []analysis.TextEdit{{
Pos: conv.Pos(),
End: conv.End(), // only replace []byte(s); keep the trailing ...
NewText: []byte(sText),
}},@copilot please address this.
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, appendbytestring.Analyzer, "appendbytestring") |
There was a problem hiding this comment.
The test uses analysistest.Run but the linter provides a SuggestedFix. Other linters in this repo that offer fixes (e.g. ctxbackground, lenstringsplit, tolowerequalfold) use analysistest.RunWithSuggestedFixes together with a .golden testdata file to verify that the rewrite produces correct output.
Without this, the fix logic is completely untested — the ellipsis-drop bug on line 148 would go undetected, for example.
Please switch to RunWithSuggestedFixes and add a testdata/src/appendbytestring/appendbytestring.go.golden file showing the expected post-fix source.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes for a correctness bug in the suggested fix and missing golden-file test coverage.
📋 Key Themes & Highlights
Key Themes
- Correctness bug (line 148 of
appendbytestring.go):buildFixusescall.Rparenas the end of itsTextEdit, which inadvertently includes the...ellipsis. The resulting rewrite isappend(b, s)— a compile error. The end should beconv.End(). - Suggested-fix test gap: The test uses
analysistest.Runrather thananalysistest.RunWithSuggestedFixes; the fix output is never exercised. Switching + adding a.goldenfile (the pattern every other linter in this repo uses) would have caught the bug above automatically. - Missing edge-case fixture: Named string types (
type MyString string) — handled byisStringTypeviaUnderlying()— have no corresponding fixture, so that behaviour is untested.
Positive Highlights
- ✅ Solid pattern-matching logic using the type system (
isByteSlice,isStringType) — correctly uses.Underlying()to handle named types - ✅ Consistent use of internal helpers (
astutil,filecheck,nolint) in line with other linters - ✅ Good fixture coverage for all the "good" cases (idiomatic form, already-bytes, multi-arg, suppression)
- ✅ Evidence-based: PR cites a real usage in the codebase
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 71.3 AIC · ⌖ 11.2 AIC · ⊞ 6.6K
Comment /matt to run again
| TextEdits: []analysis.TextEdit{ | ||
| { | ||
| Pos: conv.Pos(), | ||
| End: token.Pos(int(call.Rparen)), // rewrite up to (but not including) the closing paren of append |
There was a problem hiding this comment.
[/tdd] Bug in buildFix: the TextEdit range [conv.Pos(), call.Rparen) includes the ... ellipsis token, so the rewritten text becomes append(b, s) (missing ...), which is a compile error.
💡 Fix: use `conv.End()` instead of `call.Rparen`
The ... in append(b, []byte(s)...) lives between conv.End() and call.Rparen. The edit must only replace the []byte(s) span:
// Wrong — eats the ellipsis:
End: token.Pos(int(call.Rparen)),
// Correct — replaces only []byte(s), leaving ... intact:
End: conv.End(),With conv.End(), the rewrite produces append(b, s...) as intended.
@copilot please address this.
| ) | ||
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() |
There was a problem hiding this comment.
[/tdd] The test uses analysistest.Run instead of analysistest.RunWithSuggestedFixes — the suggested fix produced by buildFix is never validated, so fix correctness regressions would go undetected.
💡 Switch to RunWithSuggestedFixes + add a golden file
All other linters in this repo that provide a SuggestedFix (e.g. lenstringsplit, tolowerequalfold) use RunWithSuggestedFixes paired with a .golden file:
// appendbytestring_test.go
analysistest.RunWithSuggestedFixes(t, testdata, appendbytestring.Analyzer, "appendbytestring")Then add testdata/src/appendbytestring/appendbytestring.go.golden containing the expected post-fix source. This would also immediately surface the call.Rparen vs conv.End() bug in buildFix.
@copilot please address this.
| _ = b | ||
| } | ||
|
|
||
| func goodMultipleArgs() { |
There was a problem hiding this comment.
[/tdd] Missing test fixture: named type that wraps string (e.g. type MyString string). The isStringType helper uses Underlying(), so it will flag conversions of named string types — but that case isn't documented in the fixtures, leaving the edge case untested.
💡 Suggested fixture to add
type MyString string
func badNamedString() {
b := []byte("hello")
s := MyString(" world")
b = append(b, []byte(s)...) // want `...can be simplified...`
_ = b
}This confirms the linter works correctly for named string types and documents the intended behaviour.
@copilot please address this.
There was a problem hiding this comment.
❌ Request Changes — 1 critical bug + 1 high false-negative + 2 medium gaps
The linter concept is correct and the detection logic is mostly solid. However the suggested fix in buildFix produces non-compiling Go (missing ...), which must be fixed before merge. The []uint8 false negative, the hardcoded s in the diagnostic message, and the missing spec_test.go registration are the other blocking issues.
Summary of findings
| Severity | Location | Issue |
|---|---|---|
| Critical | appendbytestring.go:148 |
buildFix uses call.Rparen as End — erases ..., generates invalid Go |
| High | appendbytestring.go:109 |
isByteSliceConversion checks ident.Name == "byte" — misses []uint8(s) |
| Medium | appendbytestring.go:86 |
Diagnostic always says []byte(s) even for string literals |
| Medium | appendbytestring_test.go |
Not registered in pkg/linters/spec_test.go documentedAnalyzers() |
| Medium | testdata/.../appendbytestring.go |
No fixture case for []uint8(s) false negative |
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 208.9 AIC · ⌖ 9.09 AIC · ⊞ 5.4K
Comment /review to run again
| TextEdits: []analysis.TextEdit{ | ||
| { | ||
| Pos: conv.Pos(), | ||
| End: token.Pos(int(call.Rparen)), // rewrite up to (but not including) the closing paren of append |
There was a problem hiding this comment.
buildFix generates invalid Go — the ... is erased by the suggested fix. The rewrite lands as append(b, s) (missing the ellipsis) which will not compile.
💡 Details and fix
The TextEdit spans from conv.Pos() (the [ of []byte(s)) to call.Rparen (the closing ) of the outer append call). The ... token lives in the gap between conv.End() and call.Rparen, so it gets consumed by the rewrite.
Before:
End: token.Pos(int(call.Rparen)), // erases "..." along with "[]byte(s)"After:
End: conv.End(), // replaces only "[]byte(s)", leaves "..." intactWith conv.End() the edit replaces exactly []byte(s) with s, yielding the correct append(b, s...).
The comment directly above this line even says "we only need to rewrite conv.Pos()..conv.End() to sText" — which is correct — but the code uses call.Rparen instead of conv.End().
| } | ||
|
|
||
| // isByteSliceConversion reports whether conv is a []byte(...) type conversion expression. | ||
| func isByteSliceConversion(conv *ast.CallExpr) bool { |
There was a problem hiding this comment.
isByteSliceConversion checks the source identifier name "byte" and silently misses []uint8(s)... — an identical anti-pattern. byte and uint8 are the same predeclared type in Go; the linter flags one but not the other.
💡 Details and suggested approach
append(b, []uint8(s)...) is exactly the same unnecessary allocation as append(b, []byte(s)...), but isByteSliceConversion returns false because the ast.Ident name is "uint8", not "byte".
A robust fix is to drop the AST-level name check and use pass.TypesInfo to confirm the conversion produces []byte:
// isByteSliceConversion reports whether conv is a []byte (or []uint8) type conversion.
func isByteSliceConversion(pass *analysis.Pass, conv *ast.CallExpr) bool {
// A type conversion — not a function call — has a type expression as Fun.
_, isArrayOrSliceType := conv.Fun.(*ast.ArrayType)
_, isIdentType := conv.Fun.(*ast.Ident) // e.g. "byte" alias
if !isArrayOrSliceType && !isIdentType {
return false
}
return isByteSlice(pass, conv) // reuse the type-system check
}This also handles type Bytes = []byte aliases (currently a false negative for the same reason — the Fun would be an *ast.Ident, not an *ast.ArrayType).
Note: the signature change requires updating all call sites to pass pass.
| pass.Report(analysis.Diagnostic{ | ||
| Pos: call.Pos(), | ||
| End: call.End(), | ||
| Message: "append(b, []byte(s)...) can be simplified to append(b, s...); the []byte conversion is unnecessary", |
There was a problem hiding this comment.
Diagnostic message hardcodes s as the variable name, producing a confusing message for string literals. When the code is append(b, []byte(" world")...) the user sees "...[]byte(s)..." — s appears nowhere in their code.
💡 Suggested fix
The argument text is already computed as sText in buildFix. Thread it into the run function (or compute it inline) and interpolate the actual source text:
sText := astutil.NodeText(pass.Fset, strArg)
msg := fmt.Sprintf(
"append(b, []byte(%s)...) can be simplified to append(b, %s...); the []byte conversion is unnecessary",
sText, sText,
)The fixture badLiteral // want regex will also need updating once this is fixed.
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, appendbytestring.Analyzer, "appendbytestring") | ||
| } |
There was a problem hiding this comment.
appendbytestring is missing from pkg/linters/spec_test.go's documentedAnalyzers(), so TestSpec_PublicAPI_SubpackageAnalyzers does not validate its contract.
💡 Details
Every other linter registered in cmd/linters/main.go is also listed in pkg/linters/spec_test.go. That test validates that each analyzer has a non-nil Analyzer var, a non-empty Name, Doc, URL, and Run fields. This PR adds appendbytestring to the multichecker but skips the spec registration.
Add to spec_test.go:
- Import
"github.com/github/gh-aw/pkg/linters/appendbytestring" - Add
{"appendbytestring", appendbytestring.Analyzer}todocumentedAnalyzers() - Update the comment that says "The README documents 37 analyzers subpackages" to 38
| //nolint:appendbytestring | ||
| b = append(b, []byte(s)...) | ||
| _ = b | ||
| } |
There was a problem hiding this comment.
No test case for []uint8(s)... — the known false negative is unexercised and undocumented.
💡 Suggested addition
Add a fixture case to document the current behaviour:
func goodUint8Alias() {
b := []byte("hello")
s := " world"
// TODO: []uint8 is byte-identical but not currently flagged (false negative).
b = append(b, []uint8(s)...)
_ = b
}Until isByteSliceConversion is fixed to use the type system, this case should be documented as an explicit known limitation so a future fix has a regression baseline.
|
/souschef |
|
✅ PR Sous Chef finished PR sous-chef nudges. processed=5; skipped_checks_running=1; skipped_last_comment_from_sous_chef=1; skipped_cooldown=0; nudged=3; branch_update_attempts=0; formatter_pushes=0; merge_main_scheduled=0 |
|
|
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed. I ran the I also reran local checks after the changes:
Addressed follow-up items include fixing the suggested-fix edit range to preserve |
Summary
Adds a new
appendbytestringcustom Go static analysis linter that flags redundant[]byte(s)conversions insideappendcalls and provides an auto-fix.Problem
append(b, []byte(s)...)wherebis[]byteandsis astringallocates an unnecessary intermediate byte slice. Go natively supportsappend(b, s...)— appending a string directly to a byte slice — without the allocation. This pattern was identified inpkg/workflow/action_cache.goand potentially elsewhere.Changes
pkg/linters/appendbytestring/appendbytestring.gogolang.org/x/tools/go/analysispass: detectsappend(b, []byte(s)...)wherebis[]byteandsisstring; reports diagnostic with suggested fix toappend(b, s...)pkg/linters/appendbytestring/appendbytestring_test.goanalysistest.RunWithSuggestedFixes-based unit testpkg/linters/appendbytestring/testdata/src/appendbytestring/appendbytestring.go[]uint8, idiomatic forms,//nolintsuppressionpkg/linters/appendbytestring/testdata/src/appendbytestring/appendbytestring.go.goldencmd/linters/main.goappendbytestring.Analyzerin the multichecker entry pointpkg/linters/spec_test.goappendbytestringto the alphabetical specdocs/adr/43423-appendbytestring-linter-for-redundant-byte-conversion.mdAnalyzer behaviour
append(b, []byte(s)...)where the first arg is[]byteand the inner conversion argument is astring(including named string types and[]uint8aliases).//nolint:appendbytestring.[]byte(s)tosin place, preserving the...spread operator on the outerappend.Alternatives considered
gocritic/staticcheck): rejected — large dependency for a single narrow check; existing in-house infrastructure covers the need.Commits
0de1474bflinter: add appendbytestring linterae6a02418docs(adr): add draft ADR-43423 for appendbytestring linterb710ffb0cfix appendbytestring suggested fixes and coverageTesting
analysistest.RunWithSuggestedFixesvalidates both diagnostics and auto-fix output against the golden file. Coverage includes: variable string, string literal, named string type,[]uint8conversion, already-idiomatic form, multi-arg append, and//nolint-suppressed case.