diff --git a/cmd/linters/main.go b/cmd/linters/main.go index ddb71496355..a6eca1b9c31 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -58,6 +58,7 @@ import ( "github.com/github/gh-aw/pkg/linters/tolowerequalfold" "github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion" "github.com/github/gh-aw/pkg/linters/wgdonenotdeferred" + "github.com/github/gh-aw/pkg/linters/writebytestring" ) func main() { @@ -104,5 +105,6 @@ func main() { tolowerequalfold.Analyzer, uncheckedtypeassertion.Analyzer, wgdonenotdeferred.Analyzer, + writebytestring.Analyzer, ) } diff --git a/docs/adr/44101-writebytestring-linter-for-io-writestring.md b/docs/adr/44101-writebytestring-linter-for-io-writestring.md new file mode 100644 index 00000000000..662d61b4594 --- /dev/null +++ b/docs/adr/44101-writebytestring-linter-for-io-writestring.md @@ -0,0 +1,51 @@ +# ADR-44101: Enforce io.WriteString Over w.Write([]byte(s)) via a Custom Static Analysis Linter + +**Date**: 2026-07-07 +**Status**: Draft +**Deciders**: Unknown (generated by Linter Miner automation — review required) + +--- + +### Context + +Across the codebase, several call sites use `w.Write([]byte(s))` where `s` is a string value. This pattern incurs an unnecessary heap allocation: the runtime must copy the string's bytes into a new `[]byte` slice before passing it to the writer. Go's standard library provides `io.WriteString(w, s)`, which checks whether `w` also implements `io.StringWriter`; if it does, the underlying writer can accept the string directly, eliminating the allocation. The codebase already has 38 custom `go/analysis` linters catching similar Go anti-patterns (e.g., `appendbytestring` for `append(b, []byte(s)...)`), so a new linter fits the established pattern-enforcement approach. Six real instances of this anti-pattern were identified in production code under `pkg/` and `cmd/` (including `pkg/logger/logger.go`, `pkg/workflow/strings.go`, `pkg/parser/schedule_fuzzy_scatter.go`, and others). + +### Decision + +We will add a custom `go/analysis` linter named `writebytestring` to `pkg/linters/writebytestring/` that diagnoses any call of the form `.Write([]byte())` where `` has type `string` (or a named string type) and `` implements `io.Writer`. The linter reports the diagnostic and suggests replacing the call with `io.WriteString(, )`. It skips test files and lines annotated with `//nolint:writebytestring`. This keeps enforcement automatic, consistent with all other custom linters, and visible to CI without requiring manual code review vigilance. + +### Alternatives Considered + +#### Alternative 1: Rely on an external or upstream linter (e.g., staticcheck, golangci-lint SA1006) + +staticcheck's `S1030` rule covers a subset of this pattern, but the project uses its own custom linter binary (`cmd/linters`) rather than `staticcheck` or `golangci-lint`. Adopting an external tool solely for this check would introduce a new dependency and configuration surface, diverge from the established internal linter approach, and require integrating a separate analysis pipeline. Rejected in favour of consistency with the existing 38 custom analyzers. + +#### Alternative 2: Enforce via code review guidelines and documentation only + +Adding a style guide entry instructing developers to prefer `io.WriteString` would avoid tooling overhead but relies on human vigilance. The existing anti-pattern instances survived code review for an unknown period, showing that documentation-only approaches are insufficient for mechanical patterns. This also creates inconsistency given that structurally similar patterns (`appendbytestring`, `tolowerequalfold`) are already enforced by linters. Rejected. + +#### Alternative 3: Fix all existing instances without adding a linter + +One-time remediation of the six known call sites would eliminate current debt but not prevent recurrence. Without a linter, new instances would be silently introduced. Rejected as an incomplete solution. + +### Consequences + +#### Positive +- Eliminates an unnecessary `[]byte` heap allocation at call sites where the underlying writer implements `io.StringWriter` (e.g., `bytes.Buffer`, `os.File`), reducing GC pressure in hot paths such as hashing and logging. +- Prevents recurrence: all future `w.Write([]byte(s))` anti-patterns are caught at static analysis time in CI, with no ongoing manual review cost. +- Consistent with the existing linter infrastructure: the new analyzer integrates via the same `go/analysis` framework, `//nolint:writebytestring` escape hatch, and test-data pattern as all other custom analyzers. +- Raises the count of active analyzers from 38 to 39 and is documented in `pkg/linters/doc.go`, preserving discoverability. + +#### Negative +- Adds a maintenance burden: the linter must be updated if internal helper packages (`astutil`, `filecheck`, `nolint`) change their APIs or semantics. +- `io.WriteString` only avoids an allocation when the concrete writer also implements `io.StringWriter`; for writers that do not, the call is functionally equivalent to `Write([]byte(s))` with no performance benefit, making some diagnostics semantically neutral rather than strictly improvements. +- Developers unfamiliar with the linter collection must learn the `//nolint:writebytestring` suppression mechanism for intentional `[]byte` conversions. + +#### Neutral +- The linter was generated by the Linter Miner automated workflow; human review of the detection logic (particularly `isByteSliceConversion`, `implementsWriter`, and the `[]uint8` handling) is needed before this ADR moves to Accepted. +- The change increments the documented analyzer count in `pkg/linters/doc.go` from 38 to 39; teams maintaining integration lists of analyzer names should update their inventories. +- Test coverage uses the standard `analysistest` framework and includes bad/good/named-type/suppressed cases, but does not exercise the linter against the six real call sites identified in the codebase — a follow-up PR to fix those instances should be expected. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/linters/README.md b/pkg/linters/README.md index c2bcc4e2d11..617bb42a21e 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -48,6 +48,7 @@ This package currently provides custom Go analyzers in the following subpackages - `tolowerequalfold` — reports case-insensitive string comparisons using `strings.ToLower`/`ToUpper` that should use `strings.EqualFold`. - `uncheckedtypeassertion` — reports single-value type assertions where unchecked panics are possible. - `wgdonenotdeferred` — reports non-deferred `sync.WaitGroup.Done()` calls that can deadlock on panics or early returns. +- `writebytestring` — reports `w.Write([]byte(s))` calls where `s` is a string, which can be replaced with `io.WriteString` to avoid an unnecessary `[]byte` allocation. - `internal` — shared helper packages for analyzers (file checks and `nolint` handling). ## Public API @@ -98,6 +99,7 @@ This package currently provides custom Go analyzers in the following subpackages | `tolowerequalfold` | Custom `go/analysis` analyzer that flags case-insensitive comparisons via `strings.ToLower`/`ToUpper` that should use `strings.EqualFold` | | `uncheckedtypeassertion` | Custom `go/analysis` analyzer that flags unchecked single-value type assertions | | `wgdonenotdeferred` | Custom `go/analysis` analyzer that flags non-deferred `sync.WaitGroup.Done()` calls | +| `writebytestring` | Custom `go/analysis` analyzer that flags `w.Write([]byte(s))` calls where `s` is a string that can be replaced with `io.WriteString` | | `internal` | Shared helper subpackages used by analyzers (`internal/filecheck`, `internal/nolint`) | ### Namespace exports @@ -208,6 +210,7 @@ _ = timesleepnocontext.Analyzer - `github.com/github/gh-aw/pkg/linters/tolowerequalfold` — to-lower-equal-fold analyzer subpackage - `github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion` — unchecked-type-assertion analyzer subpackage - `github.com/github/gh-aw/pkg/linters/wgdonenotdeferred` — wg-done-not-deferred analyzer subpackage +- `github.com/github/gh-aw/pkg/linters/writebytestring` — write-byte-string analyzer subpackage **Transitive / Internal helpers**: - `github.com/github/gh-aw/pkg/linters/internal/filecheck` — shared file-path filtering helpers used by multiple analyzers diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index f161ce222ff..322d039a1c1 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,6 +1,6 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// The actual analyzers are implemented in subpackages. All 38 active analyzers: +// The actual analyzers are implemented in subpackages. All 39 active analyzers: // // - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...) // - contextcancelnotdeferred — flags context cancel functions called directly instead of deferred @@ -40,6 +40,7 @@ // - tolowerequalfold — flags case-insensitive comparisons via ToLower/ToUpper that should use EqualFold // - uncheckedtypeassertion — flags unchecked single-value type assertions // - wgdonenotdeferred — flags non-deferred sync.WaitGroup.Done() calls +// - writebytestring — flags w.Write([]byte(s)) calls where s is a string that can be replaced with io.WriteString(w, s) // // The package also exposes a compatibility alias (ErrorMessageAnalyzer) that // points to the errormessage subpackage analyzer. diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 2e42a1431b1..f011538ad72 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -49,6 +49,7 @@ import ( "github.com/github/gh-aw/pkg/linters/tolowerequalfold" "github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion" "github.com/github/gh-aw/pkg/linters/wgdonenotdeferred" + "github.com/github/gh-aw/pkg/linters/writebytestring" ) // TestSpec tests derive from pkg/linters/README.md. They enforce the documented @@ -64,7 +65,7 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 38 analyzers +// "Public API > Subpackages" table. The README documents 39 analyzers // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // @@ -76,7 +77,7 @@ type docAnalyzer struct { // manualmutexunlock, osexitinlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib, // regexpcompileinfunction, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, ssljson, // strconvparseignorederror, stringreplaceminusone, stringsindexcontains, timeafterleak, timesleepnocontext, -// tolowerequalfold, uncheckedtypeassertion, wgdonenotdeferred +// tolowerequalfold, uncheckedtypeassertion, wgdonenotdeferred, writebytestring func documentedAnalyzers() []docAnalyzer { return []docAnalyzer{ {"appendbytestring", appendbytestring.Analyzer}, @@ -117,6 +118,7 @@ func documentedAnalyzers() []docAnalyzer { {"tolowerequalfold", tolowerequalfold.Analyzer}, {"uncheckedtypeassertion", uncheckedtypeassertion.Analyzer}, {"wgdonenotdeferred", wgdonenotdeferred.Analyzer}, + {"writebytestring", writebytestring.Analyzer}, } } diff --git a/pkg/linters/writebytestring/testdata/src/writebytestring/writebytestring.go b/pkg/linters/writebytestring/testdata/src/writebytestring/writebytestring.go new file mode 100644 index 00000000000..fafdaa60ec6 --- /dev/null +++ b/pkg/linters/writebytestring/testdata/src/writebytestring/writebytestring.go @@ -0,0 +1,74 @@ +package writebytestring + +import ( + "bytes" + "io" + "os" +) + +type customWriter struct{} + +func (c *customWriter) Write(p []byte) (int, error) { return len(p), nil } + +func bad() { + var buf bytes.Buffer + s := "hello" + buf.Write([]byte(s)) // want `buf\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(&buf, s\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` + + buf.Write([]byte("world")) // want `buf\.Write\(\[\]byte\("world"\)\) can be replaced with io\.WriteString\(&buf, "world"\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` +} + +type myString string + +func badNamedString() { + var buf bytes.Buffer + s := myString("hello") + buf.Write([]byte(s)) // want `buf\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(&buf, s\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` +} + +func badFile() { + f, err := os.Create("/tmp/test.txt") + if err != nil { + return + } + defer f.Close() + msg := "hello" + f.Write([]byte(msg)) // want `f\.Write\(\[\]byte\(msg\)\) can be replaced with io\.WriteString\(f, msg\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` +} + +func badCustomWriter() { + w := &customWriter{} + s := "hello" + w.Write([]byte(s)) // want `w\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(w, s\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` +} + +func badInterfaceWriter(w io.Writer) { + s := "hello" + w.Write([]byte(s)) // want `w\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(w, s\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` +} + +func goodBytes() { + var buf bytes.Buffer + b := []byte("hello") + buf.Write(b) // already []byte — no conversion +} + +func goodWriteString() { + // Using io.WriteString is the idiomatic form. + var buf bytes.Buffer + s := "hello" + buf.WriteString(s) +} + +func goodNotString() { + var buf bytes.Buffer + n := 42 + buf.Write([]byte{byte(n)}) +} + +func suppressed() { + var buf bytes.Buffer + s := "hello" + //nolint:writebytestring + buf.Write([]byte(s)) +} diff --git a/pkg/linters/writebytestring/writebytestring.go b/pkg/linters/writebytestring/writebytestring.go new file mode 100644 index 00000000000..932d3c0b200 --- /dev/null +++ b/pkg/linters/writebytestring/writebytestring.go @@ -0,0 +1,194 @@ +// Package writebytestring implements a Go analysis linter that flags +// w.Write([]byte(s)) calls where s is a string, which can be replaced with +// io.WriteString(w, s) to avoid an unnecessary []byte allocation. +package writebytestring + +import ( + "fmt" + "go/ast" + "go/token" + "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" +) + +// writerIface is a synthetic *types.Interface matching io.Writer: +// +// Write(p []byte) (n int, err error) +// +// Built once at package init so it can be reused across analysis passes. +var writerIface = func() *types.Interface { + byteSlice := types.NewSlice(types.Typ[types.Byte]) + errType := types.Universe.Lookup("error").Type() + params := types.NewTuple(types.NewVar(token.NoPos, nil, "p", byteSlice)) + results := types.NewTuple( + types.NewVar(token.NoPos, nil, "n", types.Typ[types.Int]), + types.NewVar(token.NoPos, nil, "err", errType), + ) + sig := types.NewSignatureType(nil, nil, nil, params, results, false) + method := types.NewFunc(token.NoPos, nil, "Write", sig) + iface := types.NewInterfaceType([]*types.Func{method}, nil) + iface.Complete() + return iface +}() + +// Analyzer is the write-byte-string analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "writebytestring", + Doc: "reports w.Write([]byte(s)) calls where s is a string that can be replaced with io.WriteString(w, s)", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/writebytestring", + 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, "writebytestring") + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + } + + insp.Preorder(nodeFilter, func(n ast.Node) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } + + // Match .Write() + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Write" { + 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 single argument must be a []byte(s) conversion where s is a string. + conv, ok := call.Args[0].(*ast.CallExpr) + if !ok { + return + } + if !isByteSliceConversion(pass, conv) { + return + } + strArg := conv.Args[0] + if !isStringType(pass, strArg) { + return + } + + // The receiver must implement io.Writer. + if !implementsWriter(pass, sel.X) { + return + } + + sText := astutil.NodeText(pass.Fset, strArg) + wText := astutil.NodeText(pass.Fset, sel.X) + if sText == "" || wText == "" { + return + } + + // When the receiver is an addressable value whose Write method lives on + // the pointer type (e.g. var buf bytes.Buffer), io.WriteString requires + // the pointer form so that the interface conversion compiles. + writerArg := wText + if t := pass.TypesInfo.TypeOf(sel.X); t != nil && + !types.Implements(t, writerIface) && + types.Implements(types.NewPointer(t), writerIface) { + writerArg = "&" + wText + } + + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: fmt.Sprintf("%s.Write([]byte(%s)) can be replaced with io.WriteString(%s, %s) to potentially avoid a []byte allocation if the writer implements io.StringWriter", wText, sText, writerArg, sText), + SuggestedFixes: buildFix(call, writerArg, sText), + }) + }) + + return nil, nil +} + +// isByteSliceConversion reports whether conv is a []byte or []uint8 conversion expression. +func isByteSliceConversion(pass *analysis.Pass, conv *ast.CallExpr) bool { + funTypeInfo, ok := pass.TypesInfo.Types[conv.Fun] + if !ok || !funTypeInfo.IsType() { + return false + } + return isByteSlice(pass, conv) +} + +// isByteSlice reports whether expr has type []byte ([]uint8). +func isByteSlice(pass *analysis.Pass, expr ast.Expr) bool { + t := pass.TypesInfo.TypeOf(expr) + if t == nil { + return false + } + sl, ok := t.Underlying().(*types.Slice) + if !ok { + return false + } + elem, ok := sl.Elem().(*types.Basic) + return ok && elem.Kind() == types.Byte +} + +// isStringType reports whether expr has type string (or named string type). +func isStringType(pass *analysis.Pass, expr ast.Expr) bool { + t := pass.TypesInfo.TypeOf(expr) + if t == nil { + return false + } + basic, ok := t.Underlying().(*types.Basic) + return ok && basic.Kind() == types.String +} + +// implementsWriter reports whether expr's type implements io.Writer. +// It uses types.Implements against a synthetic io.Writer interface so the check +// is idiomatic and avoids manually re-implementing the signature comparison. +// Only T and *T are tried; **T is never constructed so pointer types are not +// double-wrapped. +func implementsWriter(pass *analysis.Pass, expr ast.Expr) bool { + t := pass.TypesInfo.TypeOf(expr) + if t == nil { + return false + } + if types.Implements(t, writerIface) { + return true + } + // Only add a pointer wrapper when t is not already a pointer, to avoid + // constructing a semantically meaningless **T type. + if _, alreadyPtr := t.Underlying().(*types.Pointer); alreadyPtr { + return false + } + return types.Implements(types.NewPointer(t), writerIface) +} + +// buildFix returns a SuggestedFix rewriting w.Write([]byte(s)) to io.WriteString(w, s). +// Note: if the file does not already import "io", tools such as gopls will add the import automatically. +func buildFix(call *ast.CallExpr, writerArg, sText string) []analysis.SuggestedFix { + replacement := fmt.Sprintf("io.WriteString(%s, %s)", writerArg, sText) + return []analysis.SuggestedFix{{ + Message: "Replace with " + replacement, + TextEdits: []analysis.TextEdit{{ + Pos: call.Pos(), + End: call.End(), + NewText: []byte(replacement), + }}, + }} +} diff --git a/pkg/linters/writebytestring/writebytestring_test.go b/pkg/linters/writebytestring/writebytestring_test.go new file mode 100644 index 00000000000..5e2f458c301 --- /dev/null +++ b/pkg/linters/writebytestring/writebytestring_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package writebytestring_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/writebytestring" +) + +func TestAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, writebytestring.Analyzer, "writebytestring") +}