[linter-miner] feat(linters): add bytesbufferstring linter — flag string(buf.Bytes()) in favour of buf.String()

cmd/linters/main.go

@@ -17,6 +17,7 @@ import (
 	"golang.org/x/tools/go/analysis/multichecker"
 
 	"github.com/github/gh-aw/pkg/linters/appendbytestring"
+	"github.com/github/gh-aw/pkg/linters/bytesbufferstring"
 	"github.com/github/gh-aw/pkg/linters/bytescomparestring"
 	"github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred"
 	"github.com/github/gh-aw/pkg/linters/ctxbackground"
@@ -68,6 +69,7 @@ import (
 func main() {
 	multichecker.Main(
 		appendbytestring.Analyzer,
+		bytesbufferstring.Analyzer,
 		bytescomparestring.Analyzer,
 		contextcancelnotdeferred.Analyzer,
 		ctxbackground.Analyzer,

docs/adr/45301-bytesbufferstring-linter-flag-string-of-buf-bytes.md

@@ -0,0 +1,50 @@
+# ADR-45301: Add `bytesbufferstring` Linter — Flag `string(buf.Bytes())` in Favour of `buf.String()`
+
+**Date**: 2026-07-13
+**Status**: Draft
+**Deciders**: Unknown (automated PR by linter-miner)
+
+---
+
+### Context
+
+The `gh-aw` repository maintains a custom suite of Go static-analysis linters under `pkg/linters/`. Each linter detects a specific sub-optimal code pattern and provides an auto-applicable suggested fix. The pattern `string(buf.Bytes())` — where `buf` is `*bytes.Buffer` — creates an unnecessary intermediate `[]byte` allocation before the final `string` conversion. `bytes.Buffer.String()` already returns the buffer's contents as a string directly, making the intermediate conversion redundant and wasteful. The linter-miner agent identified this pattern by scanning non-test Go source files in `pkg/` and `cmd/`.
+
+### Decision
+
+We will add a new Go analysis pass, `bytesbufferstring`, to the custom linter suite. The analyzer flags any `string(x.Bytes())` call where `x` is statically typed as `*bytes.Buffer` or `bytes.Buffer`, and offers a single suggested fix that rewrites the expression to `x.String()`. The analyzer skips test files and respects `//nolint:bytesbufferstring` suppressions. It is registered in `cmd/linters/main.go` alongside all other custom analyzers.
+
+### Alternatives Considered
+
+#### Alternative 1: Rely on Human Code Review
+
+Leave detection of this pattern entirely to human reviewers during pull request review. No tooling change needed, and no new linter to maintain.
+
+This was rejected because human review is inconsistent and does not scale: the same sub-optimal pattern can be introduced repeatedly across a large codebase. An automated linter provides reliable, zero-effort enforcement.
+
+#### Alternative 2: Use an Existing Third-Party Linter (`staticcheck` / `golangci-lint`)
+
+Configure an external linter tool that already covers this pattern, rather than writing a custom one.
+
+This was not viable in this context: the existing tooling infrastructure is built around custom `golang.org/x/tools/go/analysis` passes in the `pkg/linters/` package. Introducing a new external tool would require changes to the CI pipeline, build configuration, and version-pinning strategy, which exceeds the scope of adding a single focused rule. No existing linter in the project's current set covers this pattern.
+
+### Consequences
+
+#### Positive
+- Eliminates unnecessary intermediate `[]byte` allocations wherever the pattern is found, producing modest runtime improvements.
+- The suggested fix is auto-applicable (zero manual effort for the author once the linter runs).
+- Zero false-positive risk: the match is purely structural — any `string(x.Bytes())` where `x` is `*bytes.Buffer` is unambiguously replaceable.
+- Follows the established linter conventions in the repository (`largefunc` layout, `nolint` suppression, test fixture with golden file).
+
+#### Negative
+- Adds one more analyzer to the custom linter suite, increasing maintenance surface.
+- Each additional linter marginally increases the time for a full lint pass.
+- Any existing occurrences of `string(buf.Bytes())` in the codebase that are not suppressed will become lint failures, requiring authors to update code or add `//nolint:bytesbufferstring`.
+
+#### Neutral
+- The pattern is detected at analysis time only; no runtime behaviour or API contracts change.
+- Suppression via `//nolint:bytesbufferstring` is available for cases where the conversion is intentional (e.g., operating on a copy, not the original buffer).
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/linters/bytesbufferstring/bytesbufferstring.go

@@ -0,0 +1,124 @@
+// Package bytesbufferstring implements a Go analysis linter that flags
+// string(buf.Bytes()) calls where buf is a bytes.Buffer value receiver,
+// suggesting buf.String() instead.
+package bytesbufferstring
+
+import (
+	"fmt"
+	"go/ast"
+	"go/types"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+
+	"github.com/github/gh-aw/pkg/linters/internal/astutil"
+	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
+	"github.com/github/gh-aw/pkg/linters/internal/nolint"
+)
+
+// Analyzer is the bytes-buffer-string analysis pass.
+var Analyzer = &analysis.Analyzer{
+	Name:     "bytesbufferstring",
+	Doc:      "reports string(buf.Bytes()) calls where buf is a bytes.Buffer value and suggests buf.String() instead",
+	URL:      "https://github.com/github/gh-aw/tree/main/pkg/linters/bytesbufferstring",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
+	}
+	noLintLinesByFile := nolint.BuildLineIndex(pass, "bytesbufferstring")
+
+	nodeFilter := []ast.Node{
+		(*ast.CallExpr)(nil),
+	}
+
+	insp.Preorder(nodeFilter, func(n ast.Node) {
+		call, ok := n.(*ast.CallExpr)
+		if !ok {
+			return
+		}
+
+		// Match string(...) type conversion.
+		typeInfo, ok := pass.TypesInfo.Types[call.Fun]
+		if !ok || !typeInfo.IsType() {
+			return
+		}
+		basic, ok := typeInfo.Type.(*types.Basic)
+		if !ok || basic.Kind() != types.String {
+			return
+		}
+
+		if len(call.Args) != 1 {
+			return
+		}
+
+		pos := pass.Fset.PositionFor(call.Pos(), false)
+		if filecheck.IsTestFile(pos.Filename) {
+			return
+		}
+		if nolint.HasDirective(pos, noLintLinesByFile) {
+			return
+		}
+
+		// The argument must be buf.Bytes() where buf is a bytes.Buffer value.
+		inner, ok := call.Args[0].(*ast.CallExpr)
+		if !ok {
+			return
+		}
+		sel, ok := inner.Fun.(*ast.SelectorExpr)
+		if !ok || sel.Sel.Name != "Bytes" {
+			return
+		}
+		if len(inner.Args) != 0 {
+			return
+		}
+		receiverType := pass.TypesInfo.TypeOf(sel.X)
+		if receiverType == nil {
+			return
+		}
+		// Only flag value receivers (bytes.Buffer), not pointer receivers (*bytes.Buffer).
+		// The rewrite string(buf.Bytes()) → buf.String() is not semantics-preserving when
+		// buf is a nil *bytes.Buffer: string(buf.Bytes()) panics, while buf.String() returns
+		// "<nil>". Restricting to value receivers avoids this semantic difference entirely.
+		if !isBytesBufferValue(receiverType) {
+			return
+		}
+
+		receiverText := astutil.NodeText(pass.Fset, sel.X)
+		if receiverText == "" {
+			return
+		}
+
+		pass.Report(analysis.Diagnostic{
+			Pos:     call.Pos(),
+			End:     call.End(),
+			Message: fmt.Sprintf("string(%s.Bytes()) can be simplified to %s.String()", receiverText, receiverText),
+			SuggestedFixes: []analysis.SuggestedFix{{
+				Message: fmt.Sprintf("Replace string(%s.Bytes()) with %s.String()", receiverText, receiverText),
+				TextEdits: []analysis.TextEdit{{
+					Pos:     call.Pos(),
+					End:     call.End(),
+					NewText: []byte(receiverText + ".String()"),
+				}},
+			}},
+		})
+	})
+
+	return nil, nil
+}
+
+// isBytesBufferValue reports whether t is exactly bytes.Buffer (value receiver, not pointer).
+// We intentionally exclude *bytes.Buffer: the rewrite string(buf.Bytes()) → buf.String() is not
+// semantics-preserving when buf is nil — the former panics while the latter returns "<nil>".
+func isBytesBufferValue(t types.Type) bool {
+	named, ok := t.(*types.Named)
+	if !ok {
+		return false
+	}
+	obj := named.Obj()
+	return obj.Pkg() != nil && obj.Pkg().Path() == "bytes" && obj.Name() == "Buffer"
+}

pkg/linters/bytesbufferstring/bytesbufferstring_test.go

@@ -0,0 +1,16 @@
+//go:build !integration
+
+package bytesbufferstring_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+
+	"github.com/github/gh-aw/pkg/linters/bytesbufferstring"
+)
+
+func TestAnalyzer(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.RunWithSuggestedFixes(t, testdata, bytesbufferstring.Analyzer, "bytesbufferstring")
+}

pkg/linters/bytesbufferstring/testdata/src/bytesbufferstring/bytesbufferstring.go

@@ -0,0 +1,59 @@
+package bytesbufferstring
+
+import (
+	"bytes"
+)
+
+func BadStringOfBytesCall() string {
+	var buf bytes.Buffer
+	buf.WriteString("hello")
+	return string(buf.Bytes()) // want `string\(buf\.Bytes\(\)\) can be simplified to buf\.String\(\)`
+}
+
+func BadStringOfBytesCallPtr() string {
+	buf := &bytes.Buffer{}
+	buf.WriteString("world")
+	// pointer receiver: not flagged — rewrite would change nil-pointer semantics
+	return string(buf.Bytes())
+}
+
+func NilPointerBuf() string {
+	var buf *bytes.Buffer
+	// nil *bytes.Buffer: string(buf.Bytes()) panics; buf.String() returns "<nil>" — not flagged
+	return string(buf.Bytes())
+}
+
+// wrappedBuffer embeds bytes.Buffer but is a distinct named type.
+type wrappedBuffer struct {
+	bytes.Buffer
+}
+
+func GoodStringConversionWrappedType() string {
+	var buf wrappedBuffer
+	// wrappedBuffer is not bytes.Buffer — receiver type check excludes it; no diagnostic
+	return string(buf.Bytes())
+}
+
+func getBufferPtr() *bytes.Buffer { return &bytes.Buffer{} }
+
+func GoodStringOfBytesCallIndirect() string {
+	// getBufferPtr() returns *bytes.Buffer (pointer receiver) — not flagged
+	return string(getBufferPtr().Bytes())
+}
+
+func SuppressedStringOfBytes() string {
+	var buf bytes.Buffer
+	buf.WriteString("hello")
+	return string(buf.Bytes()) //nolint:bytesbufferstring
+}
+
+func GoodStringCall() string {
+	var buf bytes.Buffer
+	buf.WriteString("hello")
+	return buf.String() // correct pattern — no diagnostic expected
+}
+
+func GoodStringConversionOther() string {
+	b := []byte("hello")
+	return string(b) // not a buf.Bytes() call — no diagnostic
+}

pkg/linters/bytesbufferstring/testdata/src/bytesbufferstring/bytesbufferstring.go.golden

@@ -0,0 +1,59 @@
+package bytesbufferstring
+
+import (
+	"bytes"
+)
+
+func BadStringOfBytesCall() string {
+	var buf bytes.Buffer
+	buf.WriteString("hello")
+	return buf.String() // want `string\(buf\.Bytes\(\)\) can be simplified to buf\.String\(\)`
+}
+
+func BadStringOfBytesCallPtr() string {
+	buf := &bytes.Buffer{}
+	buf.WriteString("world")
+	// pointer receiver: not flagged — rewrite would change nil-pointer semantics
+	return string(buf.Bytes())
+}
+
+func NilPointerBuf() string {
+	var buf *bytes.Buffer
+	// nil *bytes.Buffer: string(buf.Bytes()) panics; buf.String() returns "<nil>" — not flagged
+	return string(buf.Bytes())
+}
+
+// wrappedBuffer embeds bytes.Buffer but is a distinct named type.
+type wrappedBuffer struct {
+	bytes.Buffer
+}
+
+func GoodStringConversionWrappedType() string {
+	var buf wrappedBuffer
+	// wrappedBuffer is not bytes.Buffer — receiver type check excludes it; no diagnostic
+	return string(buf.Bytes())
+}
+
+func getBufferPtr() *bytes.Buffer { return &bytes.Buffer{} }
+
+func GoodStringOfBytesCallIndirect() string {
+	// getBufferPtr() returns *bytes.Buffer (pointer receiver) — not flagged
+	return string(getBufferPtr().Bytes())
+}
+
+func SuppressedStringOfBytes() string {
+	var buf bytes.Buffer
+	buf.WriteString("hello")
+	return string(buf.Bytes()) //nolint:bytesbufferstring
+}
+
+func GoodStringCall() string {
+	var buf bytes.Buffer
+	buf.WriteString("hello")
+	return buf.String() // correct pattern — no diagnostic expected
+}
+
+func GoodStringConversionOther() string {
+	b := []byte("hello")
+	return string(b) // not a buf.Bytes() call — no diagnostic
+}