From b38749b002352ee9f4983ad8c8c851aafe736920 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 18:00:19 +0000 Subject: [PATCH 1/3] Add buffer-reset-before-reuse linter 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> --- .../bufferresetbeforereuse.go | 232 ++++++++++++++++++ .../bufferresetbeforereuse_test.go | 17 ++ .../testdata/src/a/a.go | 116 +++++++++ pkg/linters/registry.go | 2 + 4 files changed, 367 insertions(+) create mode 100644 pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go create mode 100644 pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse_test.go create mode 100644 pkg/linters/bufferresetbeforereuse/testdata/src/a/a.go diff --git a/pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go b/pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go new file mode 100644 index 00000000000..284d55c6e25 --- /dev/null +++ b/pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go @@ -0,0 +1,232 @@ +// Package bufferresetbeforereuse implements a Go analysis linter that flags +// reuse of bytes.Buffer or strings.Builder without calling Reset() between writes, +// which can accumulate previous data. +package bufferresetbeforereuse + +import ( + "go/ast" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + + "github.com/github/gh-aw/pkg/linters/internal/analyzerutil" + "github.com/github/gh-aw/pkg/linters/internal/filecheck" + "github.com/github/gh-aw/pkg/linters/internal/nolint" + "github.com/github/gh-aw/pkg/logger" +) + +var pkgLog = logger.New("linters:bufferresetbeforereuse") + +// Analyzer is the buffer-reset-before-reuse analysis pass. +var Analyzer = analyzerutil.New("bufferresetbeforereuse", "reports reuse of bytes.Buffer or strings.Builder without calling Reset() between writes, which can accumulate previous data", run) + +func run(pass *analysis.Pass) (any, error) { + pkgLog.Printf("analyzing package %s", pass.Pkg.Path()) + + noLintIndex, generatedFiles, err := analyzerutil.Indexes(pass) + if err != nil { + return nil, err + } + + // Process function declarations and function literals + nodeFilter := []ast.Node{(*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)} + return analyzerutil.Preorder(pass, nodeFilter, func(n ast.Node) { + checkBufferReuse(pass, n, generatedFiles, noLintIndex) + }) +} + +// checkBufferReuse analyzes a function body for buffer/builder reuse without Reset. +func checkBufferReuse(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + var body *ast.BlockStmt + switch fn := n.(type) { + case *ast.FuncDecl: + body = fn.Body + case *ast.FuncLit: + body = fn.Body + default: + return + } + + if body == nil { + return + } + + pos := pass.Fset.PositionFor(body.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + + // Analyze the function body for buffer write patterns + analyzeBlockForBufferReuse(pass, body, generatedFiles, noLintIndex) +} + +// event represents a write, read, or reset operation on a buffer/builder +type event struct { + varName string + typ string // "write", "read", or "reset" + pos token.Pos + node ast.Node +} + +// analyzeBlockForBufferReuse examines a block statement for improper buffer reuse +func analyzeBlockForBufferReuse(pass *analysis.Pass, block *ast.BlockStmt, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + // Collect all events (writes, reads, resets) in order + var events []*event + collectEvents(pass, block, &events) + + // For each variable, check if writes follow reads without Reset + written := make(map[string]bool) // has this variable been written? + read := make(map[string]bool) // has this variable been read? + + for _, e := range events { + switch e.typ { + case "write": + if read[e.varName] { + // This is a write after the buffer has been read, without Reset + pkgLog.Printf("flagging %s reuse at line %d", e.varName, pass.Fset.Position(e.pos).Line) + + // Check nolint directive + filePos := pass.Fset.PositionFor(e.pos, false) + if filecheck.ShouldSkipFilename(filePos.Filename, generatedFiles) { + continue + } + if nolint.HasDirectiveForLinter(filePos, noLintIndex, "bufferresetbeforereuse") { + pkgLog.Printf("suppressed diagnostic for %s at line %d", e.varName, filePos.Line) + continue + } + + pass.ReportRangef( + e.node, + "%s is reused without calling Reset() between writes, which can accumulate previous data", + e.varName, + ) + } + written[e.varName] = true + + case "read": + if written[e.varName] { + read[e.varName] = true + } + + case "reset": + read[e.varName] = false + written[e.varName] = false + } + } +} + +// collectEvents collects all write, read, and reset operations on buffers in order +func collectEvents(pass *analysis.Pass, block *ast.BlockStmt, events *[]*event) { + ast.Inspect(block, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + + ident, ok := sel.X.(*ast.Ident) + if !ok { + return true + } + + // Check if this is a buffer/builder variable + if !isBufferOrBuilderVar(pass, ident) { + return true + } + + varName := ident.Name + methodName := sel.Sel.Name + + if methodName == "Reset" { + *events = append(*events, &event{ + varName: varName, + typ: "reset", + pos: call.Pos(), + node: call, + }) + pkgLog.Printf("found reset on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line) + } else if isWriteMethod(methodName) { + *events = append(*events, &event{ + varName: varName, + typ: "write", + pos: call.Pos(), + node: call, + }) + pkgLog.Printf("found write on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line) + } else if isReadMethod(methodName) { + *events = append(*events, &event{ + varName: varName, + typ: "read", + pos: call.Pos(), + node: call, + }) + pkgLog.Printf("found read on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line) + } + + return true + }) +} + +// isBufferOrBuilderVar checks if an identifier refers to a bytes.Buffer or strings.Builder variable +func isBufferOrBuilderVar(pass *analysis.Pass, ident *ast.Ident) bool { + obj := pass.TypesInfo.ObjectOf(ident) + if obj == nil { + return false + } + + t := obj.Type() + if t == nil { + return false + } + + // Handle pointer types + if ptr, ok := t.(*types.Pointer); ok { + t = ptr.Elem() + } + + // Check if it's bytes.Buffer or strings.Builder + return isNamedType(t, "bytes", "Buffer") || isNamedType(t, "strings", "Builder") +} + +// isNamedType checks if a type is named type from specified package and name +func isNamedType(t types.Type, pkgPath, name string) bool { + named, ok := t.(*types.Named) + if !ok { + return false + } + + obj := named.Obj() + if obj == nil { + return false + } + + pkg := obj.Pkg() + if pkg == nil { + return false + } + + return pkg.Path() == pkgPath && obj.Name() == name +} + +// isWriteMethod checks if a method is a write operation +func isWriteMethod(name string) bool { + switch name { + case "Write", "WriteString", "WriteRune", "WriteByte": + return true + } + return false +} + +// isReadMethod checks if a method is a read operation +func isReadMethod(name string) bool { + switch name { + case "String", "Bytes", "Len": + return true + } + return false +} diff --git a/pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse_test.go b/pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse_test.go new file mode 100644 index 00000000000..d1d6437f667 --- /dev/null +++ b/pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse_test.go @@ -0,0 +1,17 @@ +//go:build !integration + +// Package bufferresetbeforereuse_test provides tests for the bufferresetbeforereuse analyzer. +package bufferresetbeforereuse_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/bufferresetbeforereuse" +) + +func TestBufferResetBeforeReuse(t *testing.T) { + t.Parallel() + analysistest.Run(t, analysistest.TestData(), bufferresetbeforereuse.Analyzer, "a") +} diff --git a/pkg/linters/bufferresetbeforereuse/testdata/src/a/a.go b/pkg/linters/bufferresetbeforereuse/testdata/src/a/a.go new file mode 100644 index 00000000000..cffc574a223 --- /dev/null +++ b/pkg/linters/bufferresetbeforereuse/testdata/src/a/a.go @@ -0,0 +1,116 @@ +// Package a is the test fixture for the bufferresetbeforereuse analyzer. +package a + +import ( + "bytes" + "strings" +) + +// okSingleWrite is OK - buffer used once +func okSingleWrite() { + var buf bytes.Buffer + buf.WriteString("hello") + _ = buf.String() +} + +// okResetBetweenWrites is OK - Reset called between writes +func okResetBetweenWrites() { + var buf bytes.Buffer + buf.WriteString("first") + _ = buf.String() + + buf.Reset() + buf.WriteString("second") + _ = buf.String() +} + +// notOkReuseWithoutReset is NOT OK - reused without Reset +func notOkReuseWithoutReset() { + var buf bytes.Buffer + buf.WriteString("first") // First write (OK) + _ = buf.String() + buf.WriteString("second") // want "buf is reused without calling Reset" + _ = buf.String() +} + +// okDifferentBuffers is OK - different variables +func okDifferentBuffers() { + var buf1 bytes.Buffer + var buf2 bytes.Buffer + buf1.WriteString("first") + buf2.WriteString("second") + _ = buf1.String() + buf2.String() +} + +// okBuilderWithReset is OK - builder reset between writes +func okBuilderWithReset() { + var sb strings.Builder + sb.WriteString("first") + _ = sb.String() + + sb.Reset() + sb.WriteString("second") + _ = sb.String() +} + +// notOkBuilderReuseWithoutReset is NOT OK - builder reused without Reset +func notOkBuilderReuseWithoutReset() { + var sb strings.Builder + sb.WriteString("first") // First write (OK) + _ = sb.String() + sb.WriteString("second") // want "sb is reused without calling Reset" + _ = sb.String() +} + +// okMultipleWritesAfterReset is OK - multiple writes after reset each time +func okMultipleWritesAfterReset() { + var buf bytes.Buffer + buf.WriteByte('a') + buf.WriteRune('b') + _ = buf.String() + + buf.Reset() + buf.WriteByte('c') + buf.WriteRune('d') + _ = buf.String() +} + +// notOkMultipleWritesWithoutReset is NOT OK - multiple writes without reset +func notOkMultipleWritesWithoutReset() { + var buf bytes.Buffer + buf.WriteByte('a') // First write (OK) + _ = buf.String() + buf.WriteByte('b') // want "buf is reused without calling Reset" + buf.WriteRune('c') // want "buf is reused without calling Reset" + _ = buf.String() +} + +// okSuppressed is OK - //nolint directive suppresses the diagnostic +func okSuppressed() { + var buf bytes.Buffer + buf.WriteString("first") + _ = buf.String() + //nolint:bufferresetbeforereuse + buf.WriteString("second") + _ = buf.String() +} + +// okPointerBuffer is OK - testing with pointer receiver +func okPointerBuffer() { + buf := &bytes.Buffer{} + buf.WriteString("first") + _ = buf.String() + + buf.Reset() + buf.WriteString("second") + _ = buf.String() +} + +// notOkPointerBufferReuseWithoutReset is NOT OK - pointer buffer reused without Reset +func notOkPointerBufferReuseWithoutReset() { + buf := &bytes.Buffer{} + buf.WriteString("first") // First write (OK) + _ = buf.String() + buf.WriteString("second") // want "buf is reused without calling Reset" + _ = buf.String() +} diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index 4f876da190e..42c8a431b61 100644 --- a/pkg/linters/registry.go +++ b/pkg/linters/registry.go @@ -5,6 +5,7 @@ import ( "github.com/github/gh-aw/pkg/linters/appendbytestring" "github.com/github/gh-aw/pkg/linters/appendoneelement" + "github.com/github/gh-aw/pkg/linters/bufferresetbeforereuse" "github.com/github/gh-aw/pkg/linters/bytesbufferstring" "github.com/github/gh-aw/pkg/linters/bytescomparestring" "github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred" @@ -84,6 +85,7 @@ func All() []*analysis.Analyzer { var allAnalyzers = []*analysis.Analyzer{ appendbytestring.Analyzer, appendoneelement.Analyzer, + bufferresetbeforereuse.Analyzer, bytesbufferstring.Analyzer, bytescomparestring.Analyzer, contextcancelnotdeferred.Analyzer, From c568e82f7608d0dfcb555d7e148dc8e5801ae917 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 18:51:24 +0000 Subject: [PATCH 2/3] Add ADR for buffer reset before reuse linter --- ...26-add-buffer-reset-before-reuse-linter.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 docs/adr/60026-add-buffer-reset-before-reuse-linter.md diff --git a/docs/adr/60026-add-buffer-reset-before-reuse-linter.md b/docs/adr/60026-add-buffer-reset-before-reuse-linter.md new file mode 100644 index 00000000000..e2302c481bd --- /dev/null +++ b/docs/adr/60026-add-buffer-reset-before-reuse-linter.md @@ -0,0 +1,50 @@ +# ADR-60026: Add Buffer Reset Before Reuse Linter + +**Date**: 2026-09-10 +**Status**: Draft +**Deciders**: gh-aw maintainers + +--- + +### Context + +This pull request adds a new Go analysis linter under `pkg/linters/bufferresetbeforereuse/` and registers it in the shared linter registry. The implementation scans function bodies for `bytes.Buffer` and `strings.Builder` variables that are written to, read from, and then written to again without an intervening `Reset()`. The PR includes analyzer test coverage and test fixtures that show both accepted and rejected usage patterns, including support for pointer buffers and `nolint` suppression. The architectural question is whether this buffer-reuse bug pattern should be enforced as a first-class repository linter rather than left to code review or broader existing lint rules. + +### Decision + +We will add a dedicated `bufferresetbeforereuse` analyzer to the repository's linter suite and register it in the global analyzer registry. We decided to model this as a custom AST and type-based linter because the PR evidence shows a specific correctness bug pattern involving stateful reuse of `bytes.Buffer` and `strings.Builder` that is not covered by the existing standard lint set. This makes the rule reusable across the codebase, testable with fixture-based cases, and suppressible through the existing `nolint` mechanism when a caller intentionally accepts the pattern. + +### Alternatives Considered + +#### Alternative 1: Rely on manual code review for buffer and builder reuse + +The team could continue to detect improper `bytes.Buffer` and `strings.Builder` reuse during human review. This was considered because the misuse pattern is understandable to experienced Go reviewers and does not require new analyzer infrastructure. It was not chosen because the PR explicitly introduces automated detection, test fixtures, and registry integration, indicating that the bug is subtle enough to escape review and valuable enough to check systematically. + +#### Alternative 2: Depend only on existing general-purpose Go linters + +Another option would be to keep the current linter set unchanged and assume existing upstream analyzers are sufficient. This was considered because adding a custom analyzer increases maintenance cost and the repository already ships many lint rules. It was not chosen because the PR description and implementation are centered on a repository-specific gap: writes after reads on buffers/builders without `Reset()` are treated as a correctness hazard that standard lint tooling does not already flag. + +#### Alternative 3: Detect the pattern with simpler text or grep-based heuristics + +The project could try to catch buffer reuse with lightweight string matching on method calls rather than AST and type inspection. This was considered because it would be simpler to implement initially. It was not chosen because the analyzer needs to distinguish actual `bytes.Buffer` and `strings.Builder` variables, support both value and pointer forms, and respect existing `nolint` and generated-file behavior, which are better served by structured analysis. + +### Consequences + +#### Positive +- The repository gains automated detection of a concrete stale-data bug pattern involving `bytes.Buffer` and `strings.Builder` reuse. +- The rule is centralized in the linter registry, so the check can run consistently across the codebase rather than depending on reviewer memory. +- Fixture-based tests document expected behavior for valid resets, invalid reuse, pointer buffers, and suppression directives. + +#### Negative +- The project takes on long-term maintenance for another custom analyzer, including updates if supported write/read APIs or repository linter infrastructure evolve. +- A block-level event model may still miss some control-flow-sensitive cases or produce behavior that needs future refinement as real code patterns appear. +- Contributors now face another lint rule that may require code changes or explicit suppression in edge cases. + +#### Neutral +- The implementation follows the repository's existing analyzer utility, generated-file skipping, logging, and `nolint` integration patterns. +- The rule is added as a new package plus registry wiring, without changing the broader linter execution architecture. +- The PR expands testdata-based analyzer coverage alongside the production linter code. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From 42df05156ce7351731b3a3b923e15c1263ad9c9b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 10 Sep 2026 19:52:17 +0000 Subject: [PATCH 3/3] Fix buffer reset linter review issues Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/linters/README.md | 2 + .../bufferresetbeforereuse.go | 134 +++++++++++------- .../testdata/src/a/a.go | 48 ++++++- pkg/linters/doc.go | 3 +- pkg/linters/doc_sync_test.go | 1 + pkg/linters/spec_test.go | 6 +- 6 files changed, 131 insertions(+), 63 deletions(-) diff --git a/pkg/linters/README.md b/pkg/linters/README.md index d83d826c314..4bf10406cf5 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -8,6 +8,7 @@ This package currently provides custom Go analyzers in the following subpackages - `appendbytestring` — reports `append(b, []byte(s)...)` calls where `b` is `[]byte` and `s` is a string, which can be simplified to `append(b, s...)`. - `appendoneelement` — reports `append(s, []T{x}...)` calls where a single-element slice literal is spread and can be simplified to `append(s, x)`. +- `bufferresetbeforereuse` — reports `bytes.Buffer` or `strings.Builder` writes after a read without an intervening `Reset()`, which can accumulate stale content. - `bytescomparestring` — reports `string(a) == string(b)` and `string(a) != string(b)` comparisons where `a` and `b` are `[]byte` values; use `bytes.Equal(a, b)` for `==` and `!bytes.Equal(a, b)` for `!=`. - `bytesbufferstring` — reports `string(buf.Bytes())` calls where `buf` is a `bytes.Buffer` value receiver, suggesting `buf.String()` instead. - `contextcancelnotdeferred` — reports context cancel functions that are called directly instead of deferred. @@ -103,6 +104,7 @@ environment variable and gates findings on the recorded execution hit count for |------------|-------------| | `appendbytestring` | Custom `go/analysis` analyzer that flags `append(b, []byte(s)...)` calls where `s` is a string that can be simplified to `append(b, s...)` | | `appendoneelement` | Custom `go/analysis` analyzer that flags `append(s, []T{x}...)` calls where a single-element slice literal is spread and can be simplified to `append(s, x)` | +| `bufferresetbeforereuse` | Custom `go/analysis` analyzer that flags `bytes.Buffer` or `strings.Builder` writes after a read without an intervening `Reset()` | | `bytescomparestring` | Custom `go/analysis` analyzer that flags `string(a) == string(b)` / `!=` comparisons on `[]byte` values; use `bytes.Equal(a, b)` for `==` and `!bytes.Equal(a, b)` for `!=` | | `bytesbufferstring` | Custom `go/analysis` analyzer that flags `string(buf.Bytes())` calls where `buf` is a `bytes.Buffer` value and suggests `buf.String()` instead | | `contextcancelnotdeferred` | Custom `go/analysis` analyzer that flags context cancel functions called directly instead of deferred | diff --git a/pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go b/pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go index 284d55c6e25..5d8c909c801 100644 --- a/pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go +++ b/pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go @@ -64,6 +64,7 @@ func checkBufferReuse(pass *analysis.Pass, n ast.Node, generatedFiles filecheck. // event represents a write, read, or reset operation on a buffer/builder type event struct { varName string + obj types.Object typ string // "write", "read", or "reset" pos token.Pos node ast.Node @@ -71,18 +72,30 @@ type event struct { // analyzeBlockForBufferReuse examines a block statement for improper buffer reuse func analyzeBlockForBufferReuse(pass *analysis.Pass, block *ast.BlockStmt, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + ast.Inspect(block, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.FuncLit: + return false + case *ast.BlockStmt: + analyzeStraightLineBlock(pass, node, generatedFiles, noLintIndex) + } + return true + }) +} + +func analyzeStraightLineBlock(pass *analysis.Pass, block *ast.BlockStmt, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { // Collect all events (writes, reads, resets) in order var events []*event collectEvents(pass, block, &events) // For each variable, check if writes follow reads without Reset - written := make(map[string]bool) // has this variable been written? - read := make(map[string]bool) // has this variable been read? + written := make(map[types.Object]bool) // has this variable been written? + read := make(map[types.Object]bool) // has this variable been read? for _, e := range events { switch e.typ { case "write": - if read[e.varName] { + if read[e.obj] { // This is a write after the buffer has been read, without Reset pkgLog.Printf("flagging %s reuse at line %d", e.varName, pass.Fset.Position(e.pos).Line) @@ -102,16 +115,16 @@ func analyzeBlockForBufferReuse(pass *analysis.Pass, block *ast.BlockStmt, gener e.varName, ) } - written[e.varName] = true + written[e.obj] = true case "read": - if written[e.varName] { - read[e.varName] = true + if written[e.obj] { + read[e.obj] = true } case "reset": - read[e.varName] = false - written[e.varName] = false + read[e.obj] = false + written[e.obj] = false } } } @@ -119,69 +132,81 @@ func analyzeBlockForBufferReuse(pass *analysis.Pass, block *ast.BlockStmt, gener // collectEvents collects all write, read, and reset operations on buffers in order func collectEvents(pass *analysis.Pass, block *ast.BlockStmt, events *[]*event) { ast.Inspect(block, func(n ast.Node) bool { - call, ok := n.(*ast.CallExpr) - if !ok { - return true + if n != block { + switch n.(type) { + case *ast.BlockStmt, *ast.FuncLit, *ast.IfStmt, *ast.ForStmt, *ast.RangeStmt, *ast.SwitchStmt, *ast.TypeSwitchStmt, *ast.SelectStmt: + return false + } } - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok { - return true + if _, ok := n.(*ast.FuncLit); ok { + return false } - ident, ok := sel.X.(*ast.Ident) + call, ok := n.(*ast.CallExpr) if !ok { return true } - // Check if this is a buffer/builder variable - if !isBufferOrBuilderVar(pass, ident) { - return true - } - - varName := ident.Name - methodName := sel.Sel.Name - - if methodName == "Reset" { - *events = append(*events, &event{ - varName: varName, - typ: "reset", - pos: call.Pos(), - node: call, - }) - pkgLog.Printf("found reset on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line) - } else if isWriteMethod(methodName) { - *events = append(*events, &event{ - varName: varName, - typ: "write", - pos: call.Pos(), - node: call, - }) - pkgLog.Printf("found write on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line) - } else if isReadMethod(methodName) { - *events = append(*events, &event{ - varName: varName, - typ: "read", - pos: call.Pos(), - node: call, - }) - pkgLog.Printf("found read on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line) - } + appendCallEvent(pass, call, events) return true }) } -// isBufferOrBuilderVar checks if an identifier refers to a bytes.Buffer or strings.Builder variable -func isBufferOrBuilderVar(pass *analysis.Pass, ident *ast.Ident) bool { +func appendCallEvent(pass *analysis.Pass, call *ast.CallExpr, events *[]*event) { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return + } + + ident, ok := sel.X.(*ast.Ident) + if !ok { + return + } + + obj := bufferOrBuilderObject(pass, ident) + if obj == nil { + return + } + + appendMethodEvent(pass, call, ident.Name, obj, sel.Sel.Name, events) +} + +func appendMethodEvent(pass *analysis.Pass, call *ast.CallExpr, varName string, obj types.Object, methodName string, events *[]*event) { + switch { + case methodName == "Reset": + *events = append(*events, newEvent(varName, obj, "reset", call)) + pkgLog.Printf("found reset on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line) + case isWriteMethod(methodName): + *events = append(*events, newEvent(varName, obj, "write", call)) + pkgLog.Printf("found write on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line) + case isReadMethod(methodName): + *events = append(*events, newEvent(varName, obj, "read", call)) + pkgLog.Printf("found read on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line) + } +} + +func newEvent(varName string, obj types.Object, typ string, call *ast.CallExpr) *event { + return &event{ + varName: varName, + obj: obj, + typ: typ, + pos: call.Pos(), + node: call, + } +} + +// bufferOrBuilderObject returns the object for identifiers that refer to a bytes.Buffer or strings.Builder variable. +func bufferOrBuilderObject(pass *analysis.Pass, ident *ast.Ident) types.Object { obj := pass.TypesInfo.ObjectOf(ident) if obj == nil { - return false + return nil } t := obj.Type() if t == nil { - return false + return nil } // Handle pointer types @@ -190,7 +215,10 @@ func isBufferOrBuilderVar(pass *analysis.Pass, ident *ast.Ident) bool { } // Check if it's bytes.Buffer or strings.Builder - return isNamedType(t, "bytes", "Buffer") || isNamedType(t, "strings", "Builder") + if isNamedType(t, "bytes", "Buffer") || isNamedType(t, "strings", "Builder") { + return obj + } + return nil } // isNamedType checks if a type is named type from specified package and name diff --git a/pkg/linters/bufferresetbeforereuse/testdata/src/a/a.go b/pkg/linters/bufferresetbeforereuse/testdata/src/a/a.go index cffc574a223..cbcd23891d5 100644 --- a/pkg/linters/bufferresetbeforereuse/testdata/src/a/a.go +++ b/pkg/linters/bufferresetbeforereuse/testdata/src/a/a.go @@ -27,7 +27,7 @@ func okResetBetweenWrites() { // notOkReuseWithoutReset is NOT OK - reused without Reset func notOkReuseWithoutReset() { var buf bytes.Buffer - buf.WriteString("first") // First write (OK) + buf.WriteString("first") // First write (OK) _ = buf.String() buf.WriteString("second") // want "buf is reused without calling Reset" _ = buf.String() @@ -56,9 +56,9 @@ func okBuilderWithReset() { // notOkBuilderReuseWithoutReset is NOT OK - builder reused without Reset func notOkBuilderReuseWithoutReset() { var sb strings.Builder - sb.WriteString("first") // First write (OK) + sb.WriteString("first") // First write (OK) _ = sb.String() - sb.WriteString("second") // want "sb is reused without calling Reset" + sb.WriteString("second") // want "sb is reused without calling Reset" _ = sb.String() } @@ -78,10 +78,10 @@ func okMultipleWritesAfterReset() { // notOkMultipleWritesWithoutReset is NOT OK - multiple writes without reset func notOkMultipleWritesWithoutReset() { var buf bytes.Buffer - buf.WriteByte('a') // First write (OK) + buf.WriteByte('a') // First write (OK) _ = buf.String() - buf.WriteByte('b') // want "buf is reused without calling Reset" - buf.WriteRune('c') // want "buf is reused without calling Reset" + buf.WriteByte('b') // want "buf is reused without calling Reset" + buf.WriteRune('c') // want "buf is reused without calling Reset" _ = buf.String() } @@ -109,8 +109,42 @@ func okPointerBuffer() { // notOkPointerBufferReuseWithoutReset is NOT OK - pointer buffer reused without Reset func notOkPointerBufferReuseWithoutReset() { buf := &bytes.Buffer{} - buf.WriteString("first") // First write (OK) + buf.WriteString("first") // First write (OK) _ = buf.String() buf.WriteString("second") // want "buf is reused without calling Reset" _ = buf.String() } + +// okShadowedBuffer is OK - inner buf is a distinct object. +func okShadowedBuffer() { + var buf bytes.Buffer + buf.WriteString("outer") + _ = buf.String() + + { + var buf bytes.Buffer + buf.WriteString("inner") + _ = buf.String() + } +} + +// notOkFuncLiteralReuseWithoutReset is reported once for the function literal. +func notOkFuncLiteralReuseWithoutReset() { + func() { + var buf bytes.Buffer + buf.WriteString("first") + _ = buf.String() + buf.WriteString("second") // want "buf is reused without calling Reset" + }() +} + +// okMutuallyExclusiveBranches is OK - the read returns before the later write path. +func okMutuallyExclusiveBranches(cond bool) string { + var buf bytes.Buffer + buf.WriteString("init") + if cond { + return buf.String() + } + buf.WriteString("more") + return buf.String() +} diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 873a6a3ace2..8a379ca1616 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,9 +1,10 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// All 67 active analyzers: +// All 68 active analyzers: // // - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...) // - appendoneelement — flags append(s, []T{x}...) calls where a single-element slice literal is spread and can be simplified to append(s, x) +// - bufferresetbeforereuse — flags bytes.Buffer or strings.Builder reuse after a read without an intervening Reset // - bytesbufferstring — reports string(buf.Bytes()) calls where buf is a bytes.Buffer value and suggests buf.String() instead // - bytescomparestring — flags string(a) == string(b) and string(a) != string(b) comparisons where a and b are []byte values and recommends bytes.Equal for clearer intent // - contextcancelnotdeferred — flags context cancel functions called directly instead of deferred diff --git a/pkg/linters/doc_sync_test.go b/pkg/linters/doc_sync_test.go index 97fd4ebeeed..25ce85c2c59 100644 --- a/pkg/linters/doc_sync_test.go +++ b/pkg/linters/doc_sync_test.go @@ -23,6 +23,7 @@ var ( ) var notYetEnforced = map[string]string{ + "bufferresetbeforereuse": "new correctness analyzer needs an enforcement-readiness audit before native CI enables it", "errorfwrapv": "requires an enforcement audit after the recent false-positive fix (#51928)", "errormessage": "dedicated lint-error-messages CI job is intentionally advisory (continue-on-error per #54800)", "excessivefuncparams": "existing production violations need remediation before enforcement; nolint suppression already works", diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index debc3b1aaf1..1bdc7b604dc 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -13,6 +13,7 @@ import ( "github.com/github/gh-aw/pkg/linters" "github.com/github/gh-aw/pkg/linters/appendbytestring" "github.com/github/gh-aw/pkg/linters/appendoneelement" + "github.com/github/gh-aw/pkg/linters/bufferresetbeforereuse" "github.com/github/gh-aw/pkg/linters/bytesbufferstring" "github.com/github/gh-aw/pkg/linters/bytescomparestring" "github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred" @@ -93,13 +94,13 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 67 analyzers +// "Public API > Subpackages" table. The README documents 68 analyzers // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // // Spec (README "Public API > Subpackages"): // -// appendbytestring, appendoneelement, bytesbufferstring, bytescomparestring, contextcancelnotdeferred, ctxbackground, deferinloop, errorfwrapv, excessivefuncparams, errormessage, +// appendbytestring, appendoneelement, bufferresetbeforereuse, bytesbufferstring, bytescomparestring, contextcancelnotdeferred, ctxbackground, deferinloop, errorfwrapv, excessivefuncparams, errormessage, // errortypeassertion, errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, // generatedyamlheredoc, globwalkignorederror, goroutinemissingrecover, hardcodedfilepath, httpnoctx, httprespbodyclose, httpstatuscode, ioutildeprecated, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero, // logfatallibrary, manualmutexunlock, manualpathconcat, mapclearloop, mapdeletecheck, nilctxpassed, osexitinlibrary, osgetenvlibrary, ossetenvlibrary, packagelevelmutableslicemap, panic-in-library-code, rawloginlib, @@ -110,6 +111,7 @@ func documentedAnalyzers() []docAnalyzer { return []docAnalyzer{ {"appendbytestring", appendbytestring.Analyzer}, {"appendoneelement", appendoneelement.Analyzer}, + {"bufferresetbeforereuse", bufferresetbeforereuse.Analyzer}, {"bytesbufferstring", bytesbufferstring.Analyzer}, {"bytescomparestring", bytescomparestring.Analyzer}, {"contextcancelnotdeferred", contextcancelnotdeferred.Analyzer},