[linter-miner] Add buffer-reset-before-reuse linter - #60026
Conversation
Implements a new Go analysis linter that detects when bytes.Buffer or strings.Builder instances are reused without calling Reset() between write operations, which can lead to accumulation of previous data. The linter: - Tracks write operations (Write, WriteString, WriteRune, WriteByte) - Tracks read operations (String, Bytes, Len) that consume content - Flags subsequent writes after read without intervening Reset() - Respects //nolint:bufferresetbeforereuse directives - Includes comprehensive test fixtures covering all scenarios This linter was identified through analysis of recurring Go code patterns and addresses a practical bug pattern that is not covered by standard golangci-lint rules. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
🟡 Changes recommended
The analyzer mishandles control flow and variable scope, duplicates closure diagnostics, and leaves required registry integration incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a custom analyzer intended to detect buffer reuse without Reset().
Changes:
- Implements buffer/builder event analysis and suppression support.
- Adds analysistest fixtures.
- Registers the analyzer globally.
File summaries
| File | Description |
|---|---|
pkg/linters/registry.go |
Registers the analyzer. |
pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go |
Implements detection logic. |
pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse_test.go |
Runs analyzer fixtures. |
pkg/linters/bufferresetbeforereuse/testdata/src/a/a.go |
Defines test scenarios. |
Review details
Suppressed comments (2)
pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go:76
- The collected AST events are treated as one execution path, which produces incorrect diagnostics. For example, a conditional
Reset()always clears the state here even on the path where the condition is false, mutually exclusive branches are combined, and a loop containing one write/read pair is never checked for reuse on its next iteration. Use CFG-based dataflow so state is propagated and merged per executable path.
// Collect all events (writes, reads, resets) in order
var events []*event
collectEvents(pass, block, &events)
pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go:125
runseparately analyzes everyFuncLit, but this walk also descends into literals while analyzing the enclosing function. Consequently, a violation inside a closure is reported twice, and even a never-invoked closure can change the enclosing function's buffer state. Stop this walk at nested function literals; their own preorder callback will analyze them.
ast.Inspect(block, func(n ast.Node) bool {
call, ok := n.(*ast.CallExpr)
if !ok {
return true
}
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| var allAnalyzers = []*analysis.Analyzer{ | ||
| appendbytestring.Analyzer, | ||
| appendoneelement.Analyzer, | ||
| bufferresetbeforereuse.Analyzer, |
There was a problem hiding this comment.
Fixed in 42df051: added bufferresetbeforereuse to pkg/linters/doc.go, pkg/linters/README.md, documentedAnalyzers(), and notYetEnforced with an enforcement-readiness reason.
| type event struct { | ||
| varName string | ||
| typ string // "write", "read", or "reset" | ||
| pos token.Pos | ||
| node ast.Node |
There was a problem hiding this comment.
Fixed in 42df051: events now carry the receiver types.Object, and the analyzer keys read/write state by object rather than identifier text.
ADR RequiredThis PR adds an architectural change without an ADR in the PR description or existing ADR set, so I generated a draft ADR and committed it to the branch:
Evidence used:
Next action:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the new buffer-reset-before-reuse linter. The idea is solid, but I found two confirmed correctness bugs in the analyzer logic itself (verified by building the analyzer binary and running it against small repro cases via go vet -vettool):
- Duplicate diagnostics for nested function literals — the flat
ast.Inspectwalk incollectEventsrecurses into nestedFuncLitbodies, which are then also visited (and re-analyzed) separately by thenodeFilterpass inrun, producing the same diagnostic twice for one call site. - False positives across mutually-exclusive control-flow branches — the analyzer treats all writes/reads in a function as one flat sequential timeline with no awareness of
if/switch/early-return branching, so it flags perfectly safe code where a read only happens on one path and a write only happens on another (they never actually execute back-to-back).
See inline comments for concrete repro snippets and suggested fixes. Given #2 in particular, this linter is likely to produce noisy false positives on real code with branching, so I'd like these addressed (or the check scoped to straight-line blocks only) before merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 46 AIC · ⌖ 13.9 AIC · ⊞ 8.4K
|
|
||
| // Process function declarations and function literals | ||
| nodeFilter := []ast.Node{(*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)} | ||
| return analyzerutil.Preorder(pass, nodeFilter, func(n ast.Node) { |
There was a problem hiding this comment.
Bug: duplicate diagnostics for nested function literals.
ast.Inspect(block, ...) in collectEvents recurses into every descendant node, including the bodies of any nested FuncLit inside the block. Meanwhile, the nodeFilter := []ast.Node{(*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)} in run separately visits that same nested FuncLit and calls analyzeBlockForBufferReuse on its body again. The result is the same violation reported twice.
Reproduced with:
func outerWithClosure() {
fn := func() {
var buf bytes.Buffer
buf.WriteString("a")
_ = buf.String()
buf.WriteString("b")
}
fn()
}This produces two identical pkgx/nestedclosure.go:14:3: buf is reused without calling Reset()... diagnostics for the single call site.
Fix: skip descending into nested FuncLit bodies when walking block in collectEvents (e.g. stop ast.Inspect from recursing into *ast.FuncLit and instead let the nodeFilter pass handle it separately), or de-duplicate by only running checkBufferReuse on top-level FuncDecls and handling literals internally.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 42df051: nested function literals are no longer traversed from the enclosing block, so the separate FuncLit analyzer pass reports the violation only once.
| written := make(map[string]bool) // has this variable been written? | ||
| read := make(map[string]bool) // has this variable been read? | ||
|
|
||
| for _, e := range events { |
There was a problem hiding this comment.
False positive: control flow (branches) is ignored.
analyzeBlockForBufferReuse walks all buffer/builder events in one flat, source-order sequence via ast.Inspect, without any awareness of if/else, switch, or early returns. This causes false positives when a read and a subsequent write are actually on mutually exclusive control-flow paths and never happen in the same execution.
Reproduced with:
func mutexBranches(cond bool) string {
var buf bytes.Buffer
buf.WriteString("init")
if cond {
return buf.String()
}
buf.WriteString("more")
return buf.String()
}Here the second WriteString only executes when the if cond branch was not taken, so buf.String() was never actually called before it — yet the linter flags it as reused-without-reset.
Given the flat event-list approach, this analyzer will likely produce noisy false positives on any function with branching before/after a read. Consider a CFG-based approach (e.g. golang.org/x/tools/go/cfg) or scoping the check to straight-line blocks only (bailing out on nested if/switch/for containing writes or reads) to avoid flagging safe mutually-exclusive branches.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 42df051: state is now scoped to straight-line blocks, so reads from mutually exclusive branch paths are not propagated into later writes.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes. I reproduced two correctness bugs in the new analyzer by building small fixture programs and running them through analysistest.
📋 Key Themes & Highlights
Key Themes
- Duplicate diagnostics: the
FuncDecl/FuncLitnode filter causes nested closures to be analyzed both as part of their enclosing function's walk and again as their own top-level node, producing the same diagnostic twice for a single reuse. - False positive from name-based tracking: state is keyed by
varName string(the identifier text) rather than the resolvedtypes.Object, so a shadowed variable with the same name in a nested scope is treated as a "reuse" of the outer one even though it's a distinct, unread variable. - Test coverage gap: neither of these edge cases (nested closures, shadowing) has fixture coverage in
testdata/src/a/a.go, despite the analyzer relying on precise scope/identity handling for correctness.
Positive Highlights
- ✅ Clean, well-organized event-collection design (write/read/reset) that is easy to follow
- ✅ Respects
nolintdirectives and skips generated files consistently with other linters - ✅ Solid baseline test coverage for the straightforward single-scope reuse cases
Note: skill selection did not require the pr-triage fallback heuristic — this is clearly a new-linter/bugfix-adjacent change, so /diagnosing-bugs and /tdd were applied directly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 51.3 AIC · ⌖ 15.2 AIC · ⊞ 10.4K
Comment /matt to run again
|
|
||
| // Process function declarations and function literals | ||
| nodeFilter := []ast.Node{(*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)} | ||
| return analyzerutil.Preorder(pass, nodeFilter, func(n ast.Node) { |
There was a problem hiding this comment.
[/diagnosing-bugs] Double-reporting bug: since the node filter matches both *ast.FuncDecl and *ast.FuncLit, a closure nested inside a function body gets analyzed twice — once as part of the enclosing function's block (via ast.Inspect walking into nested literals) and once again as its own top-level node. I verified this produces duplicate diagnostics for the same reuse.
💡 Repro & fix
Repro (outer function containing a closure that reuses a buffer):
func outer() {
var buf bytes.Buffer
func() {
buf.WriteString("a")
_ = buf.String()
buf.WriteString("b")
}()
}Running the analyzer against this fixture reports the same diagnostic twice at the same position.
Fix: either (a) stop ast.Inspect in collectEvents from descending into nested *ast.FuncLit bodies (they'll be visited separately by the Preorder pass), or (b) only run checkBufferReuse on top-level FuncDecl/FuncLit nodes that aren't themselves nested inside another already-visited function body. Add a regression test fixture covering nested closures.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 42df051: collectEvents skips nested function literals and a regression fixture covers closure reuse without duplicate diagnostics.
| collectEvents(pass, block, &events) | ||
|
|
||
| // For each variable, check if writes follow reads without Reset | ||
| written := make(map[string]bool) // has this variable been written? |
There was a problem hiding this comment.
[/diagnosing-bugs] False positive: state is tracked by varName string (identifier text) rather than by the resolved types.Object/scope, so two distinct (e.g. shadowed) variables that share a name are treated as the same buffer. I confirmed this produces a false diagnostic.
💡 Repro
func shadow() {
var buf bytes.Buffer
buf.WriteString("outer")
_ = buf.String()
{
var buf bytes.Buffer // shadowed, different variable
buf.WriteString("inner")
}
}This reports "buf is reused without calling Reset()" even though the inner buf is a completely separate, freshly-declared variable that was never read.
Fix: key written/read/events by pass.TypesInfo.ObjectOf(ident) (or types.Object) instead of the bare identifier name, so shadowed variables in different scopes aren't conflated. Add a test fixture for shadowing.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 42df051: the analyzer tracks receivers by types.Object, and a shadowed-buffer fixture verifies distinct declarations are not conflated.
| func TestBufferResetBeforeReuse(t *testing.T) { | ||
| t.Parallel() | ||
| analysistest.Run(t, analysistest.TestData(), bufferresetbeforereuse.Analyzer, "a") | ||
| } |
There was a problem hiding this comment.
[/tdd] The test fixture (testdata/src/a/a.go) is thorough for the straightforward reuse cases but has no coverage for the two edge cases above: nested closures/FuncLit reuse and variable shadowing. Since analyzer correctness hinges on precise scope/identity tracking, these are exactly the boundary conditions /tdd calls out as needing explicit specification via tests — please add fixture cases (and want comments) for both before merging, ideally written first to pin the expected (non-)diagnostic behavior.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 42df051: added regression fixtures for nested function literal reuse, shadowed buffers, and mutually exclusive branch paths.
|
@copilot The PR still has failed checks:
Please fix the failing tests, refresh the branch if needed, and then run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in |
|
🎉 This pull request is included in a new release. Release: |
Summary
This PR adds a new Go analysis linter called
buffer-reset-before-reusethat detects improper reuse ofbytes.Bufferandstrings.Builderinstances.Problem
When a
bytes.Bufferorstrings.Builderis reused without callingReset()between write operations, it can accumulate previous data, leading to subtle bugs where output contains stale content.Solution
The new linter flags cases where:
Evidence
This linter was identified through systematic analysis of:
pkg/andcmd/directoriesThe pattern represents a practical bug that is not covered by standard golangci-lint rules.
Implementation Details
pkg/linters/bufferresetbeforereuse/*bytes.Bufferand*strings.Builder(nolint/redacted):bufferresetbeforereusecommentspkg/linters/registry.goin alphabetical orderValidation
Run: https://github.com/github/gh-aw/actions/runs/34520309155
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.