[linter-miner] linters: add writebytestring — flag w.Write([]byte(s)) calls that should use io.WriteString#44101
Conversation
Flags w.Write([]byte(s)) calls where s is a string. These can be replaced with io.WriteString(w, s) which avoids an unnecessary []byte heap allocation — the io.WriteString function checks whether the writer implements io.StringWriter and short-circuits the copy. - New package: pkg/linters/writebytestring/ - Analyzer registered in cmd/linters/main.go - pkg/linters/doc.go count updated (38 → 39) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom go/analysis analyzer (writebytestring) to flag w.Write([]byte(s)) (where s is a string) and recommend using io.WriteString to enable the io.StringWriter fast path.
Changes:
- Added
pkg/linters/writebytestringanalyzer implementation plus analysistest coverage and fixtures. - Registered the analyzer in the
cmd/lintersdriver. - Updated
pkg/linterspackage docs to include the new analyzer and increment the analyzer count.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/writebytestring/writebytestring.go | New analyzer that detects Write([]byte(string)) and reports a diagnostic. |
| pkg/linters/writebytestring/writebytestring_test.go | Adds analysistest harness for the new analyzer. |
| pkg/linters/writebytestring/testdata/src/writebytestring/writebytestring.go | Adds fixtures for bad/good/suppressed cases. |
| cmd/linters/main.go | Registers the new analyzer with the multichecker driver. |
| pkg/linters/doc.go | Updates the documented analyzer count and list entry. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 6
- Review effort level: Low
| sText := astutil.NodeText(pass.Fset, strArg) | ||
| wText := astutil.NodeText(pass.Fset, sel.X) | ||
| if sText == "" || wText == "" { | ||
| return | ||
| } | ||
|
|
||
| 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 avoid a []byte allocation", wText, sText, wText, sText), | ||
| }) |
There was a problem hiding this comment.
Both issues fixed: (1) value-type receivers now emit io.WriteString(&buf, s) (using types.Implements to detect when *T is needed); (2) the message now reads "…to avoid a []byte allocation when the writer implements io.StringWriter".
| buf.Write([]byte(s)) // want `buf\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(buf, s\) to avoid a \[\]byte allocation` | ||
|
|
||
| buf.Write([]byte("world")) // want `buf\.Write\(\[\]byte\("world"\)\) can be replaced with io\.WriteString\(buf, "world"\) to avoid a \[\]byte allocation` |
There was a problem hiding this comment.
Fixed. The fixtures now expect io.WriteString(&buf, ...) for all bytes.Buffer value cases, and the // want regexes reflect the updated message wording.
| 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 avoid a \[\]byte allocation` |
There was a problem hiding this comment.
Fixed. The badNamedString fixture now expects io.WriteString(&buf, s) and the // want regex uses the updated wording.
| } | ||
| defer f.Close() | ||
| msg := "hello" | ||
| f.Write([]byte(msg)) // want `f\.Write\(\[\]byte\(msg\)\) can be replaced with io\.WriteString\(f, msg\) to avoid a \[\]byte allocation` |
There was a problem hiding this comment.
The message was updated to "…to avoid a []byte allocation when the writer implements io.StringWriter" and all // want regexes in the fixture have been updated to match.
| func badCustomWriter() { | ||
| w := &customWriter{} | ||
| s := "hello" | ||
| w.Write([]byte(s)) // want `w\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(w, s\) to avoid a \[\]byte allocation` |
There was a problem hiding this comment.
The message and all // want regexes in the fixture have been updated. The badFile and badCustomWriter cases now match the new wording.
| timesleepnocontext.Analyzer, | ||
| tolowerequalfold.Analyzer, | ||
| uncheckedtypeassertion.Analyzer, | ||
| wgdonenotdeferred.Analyzer, | ||
| writebytestring.Analyzer, |
There was a problem hiding this comment.
writebytestring has been added to both pkg/linters/README.md (overview list, Subpackages table, Dependencies) and pkg/linters/spec_test.go documentedAnalyzers() (38 → 39). All 39 linter package tests pass.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (1 test)
✅ VerdictPASS. 0% implementation tests (threshold: 30%). Strengths:
Design Invariants Enforced:
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.
|
Adds a draft Architecture Decision Record documenting the decision to enforce io.WriteString over w.Write([]byte(s)) via a custom go/analysis linter, consistent with the project's existing 38 custom analyzers.
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (263 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 MatterADRs 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.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on three issues: a missing auto-fix, a double-pointer logic gap, and an untested io.Writer interface fixture.
📋 Key Themes & Highlights
Issues Found
- No
SuggestedFixes— Unlike the siblingappendbytestringlinter, the diagnostic offers no automated text edit. Theio.WriteString(w, s)rewrite is deterministic and should be auto-fixable. implementsWriterdouble-pointer —types.NewPointer(t)is called unconditionally; whentis already*Tthis silently creates**T. The logic still works by coincidence (thehasWriteMethod(t)check fires first), but the guard should be explicit.- Missing
io.Writerinterface test fixture — Concrete receiver types are covered, but a function parameter typed asio.Writeris not. This is the dominant real-world pattern and the path throughimplementsWriterfor an interface type is untested.
Positive Highlights
- ✅ Excellent pattern matching: AST guard correctly checks
funTypeInfo.IsType()before treating a call as a conversion. - ✅ Consistent internal helpers (
astutil,filecheck,nolint) — good cohesion with the existing linter family. - ✅ Named string types (
myString) correctly handled viaUnderlying()unwrapping. - ✅
nolint:writebytestringsuppression works and is tested.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 101.8 AIC · ⌖ 7.93 AIC · ⊞ 6.6K
Comment /matt to run again
| Pos: call.Pos(), | ||
| End: call.End(), | ||
| Message: fmt.Sprintf("%s.Write([]byte(%s)) can be replaced with io.WriteString(%s, %s) to avoid a []byte allocation", wText, sText, wText, sText), | ||
| }) |
There was a problem hiding this comment.
[/codebase-design] Missing SuggestedFixes — the sibling appendbytestring linter offers an automated text edit, but this diagnostic does not. The rewrite is mechanical and well-defined, so users lose the ability to auto-fix with gopls or go fix.
💡 Suggested implementation
Add a buildFix helper similar to appendbytestring/appendbytestring.go (lines 133–151):
func buildFix(pass *analysis.Pass, call *ast.CallExpr, wText, sText string) []analysis.SuggestedFix {
return []analysis.SuggestedFix{{
Message: "Replace w.Write([]byte(s)) with io.WriteString(w, s)",
TextEdits: []analysis.TextEdit{{
Pos: call.Pos(),
End: call.End(),
NewText: []byte(fmt.Sprintf("io.WriteString(%s, %s)", wText, sText)),
}},
}}
}Then wire it in the Report call:
pass.Report(analysis.Diagnostic{
Pos: call.Pos(),
End: call.End(),
Message: ...,
SuggestedFixes: buildFix(pass, call, wText, sText),
})Note: the fix also needs an "io" import added if the file doesn't already import it. You can include a second TextEdit for the import block.
@copilot please address this.
There was a problem hiding this comment.
Added in the latest commit. buildFix follows the appendbytestring template: replaces the entire w.Write([]byte(s)) call expression with io.WriteString(w, s) (using &w when needed for value receivers). Tools like gopls handle the "io" import automatically.
| if t == nil { | ||
| return false | ||
| } | ||
| return hasWriteMethod(t) || hasWriteMethod(types.NewPointer(t)) |
There was a problem hiding this comment.
[/codebase-design] Double-pointer edge case in implementsWriter: when t is already a pointer type (e.g. *os.File), calling types.NewPointer(t) produces **os.File, which never has a Write method. The first check hasWriteMethod(t) will return true for *os.File, so the bug doesn't surface in practice — but it signals a subtle logic gap.
💡 Suggested fix
Only add the pointer check when t is not already a pointer:
func implementsWriter(pass *analysis.Pass, expr ast.Expr) bool {
t := pass.TypesInfo.TypeOf(expr)
if t == nil {
return false
}
if hasWriteMethod(t) {
return true
}
// Only try *T if t is not already a pointer.
if _, isPtr := t.Underlying().(*types.Pointer); isPtr {
return false
}
return hasWriteMethod(types.NewPointer(t))
}This matches the pattern used elsewhere in the codebase and avoids creating semantically meaningless **T types.
@copilot please address this.
There was a problem hiding this comment.
Fixed. implementsWriter now uses types.Implements against a synthetic writerIface and explicitly guards against double-pointer: if t is already a pointer type, we skip the types.NewPointer(t) check entirely.
| w.Write([]byte(s)) // want `w\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(w, s\) to avoid a \[\]byte allocation` | ||
| } | ||
|
|
||
| func goodBytes() { |
There was a problem hiding this comment.
[/tdd] Missing test fixture for an io.Writer interface variable. The current fixtures only cover concrete types (bytes.Buffer, *os.File, *customWriter). A receiver typed as io.Writer is arguably the most common real-world pattern but is not exercised.
💡 Suggested addition
func badInterfaceWriter(w io.Writer) {
s := "hello"
w.Write([]byte(s)) // want `w\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(w, s\)`
}This ensures the linter fires when the static type is an interface rather than a concrete type, which tests the implementsWriter path through the interface method set.
@copilot please address this.
There was a problem hiding this comment.
Added badInterfaceWriter(w io.Writer) to the testdata fixture. The "io" import was also added to the testdata file.
There was a problem hiding this comment.
REQUEST_CHANGES — one correctness bug must be fixed before merge; one consistency gap should be addressed.
### Findings summary
🔴 High: Misleading (non-compilable) diagnostic for value-type receivers
implementsWriter returns true for any type whose pointer has a Write method — including bytes.Buffer value. The diagnostic then tells the user to write io.WriteString(buf, s), which does not compile because a bytes.Buffer value does not satisfy io.Writer (only *bytes.Buffer does). The existing bad() test uses exactly this case and would cause users who follow the fix to get a compile error. Fix: gate the flag on types.Implements(t, ioWriterIface) directly, or make the suggestion context-sensitive (&buf vs buf).
🟡 Medium: No SuggestedFixes emitted, inconsistent with sibling linters
All comparable linters in this package (appendbytestring, lenstringsplit, tolowerequalfold) emit analysis.SuggestedFix with TextEdits. This linter only emits a message, breaking gopls/golangci-lint --fix workflows and the established authoring contract.
🟡 Medium: Missing io.Writer-typed interface receiver test case
All test fixtures use concrete types. The func f(w io.Writer) { w.Write([]byte(s)) } pattern — the most common case in production code — is untested.
🔎 Code quality review by PR Code Quality Reviewer · 175.4 AIC · ⌖ 5.41 AIC · ⊞ 5.4K
Comment /review to run again
| if t == nil { | ||
| return false | ||
| } | ||
| return hasWriteMethod(t) || hasWriteMethod(types.NewPointer(t)) |
There was a problem hiding this comment.
implementsWriter generates misleading (non-compiling) diagnostic messages for value-type receivers: when expr is a value type (e.g. var buf bytes.Buffer), hasWriteMethod(t) is false but hasWriteMethod(types.NewPointer(t)) is true, so the linter fires and emits io.WriteString(buf, s) — but buf (value type) does not satisfy io.Writer and that replacement will not compile.
💡 Details and fix
var buf bytes.Buffer
buf.Write([]byte(s)) // flagged ✓ — linter fires
io.WriteString(buf, s) // ← suggested replacement, does NOT compile:
// cannot use buf (type bytes.Buffer) as type io.Writer:
// bytes.Buffer does not implement io.Writer (Write method has pointer receiver)The fix: use types.Implements against an explicit io.Writer interface object rather than falling back to checking *t. Only flag the call when t directly satisfies io.Writer (value method set includes Write). This avoids false suggestions for addressable-but-non-interface-satisfying value types.
As a fallback fix for the message: when only *t has Write, either skip the warning or emit the correct form (io.WriteString(&buf, s) or buf.WriteString(s) if the method exists).
The existing bad() test uses var buf bytes.Buffer (value type) and asserts the diagnostic fires — but following the suggested fix yields a compile error.
There was a problem hiding this comment.
Fixed. The writerArg logic now uses types.Implements(t, writerIface) to detect value receivers. When only *T implements io.Writer, the suggestion emits io.WriteString(&buf, s) which compiles correctly.
| return | ||
| } | ||
|
|
||
| pass.Report(analysis.Diagnostic{ |
There was a problem hiding this comment.
No SuggestedFixes provided, inconsistent with every comparable linter in this repo: appendbytestring, tolowerequalfold, lenstringsplit, and others all emit analysis.SuggestedFix with TextEdits so tools like gopls and golangci-lint --fix can auto-apply them. This linter only emits a message, requiring manual edits.
💡 Details and suggested approach
appendbytestring's buildFix is the direct template (same diff shape): rewrite the entire w.Write([]byte(s)) call expression to io.WriteString(w, s). The challenge is ensuring the io import is present, which analysis.SuggestedFix does not handle automatically — a common workaround is to document in the fix message that the import must be added, or use analysisutil.AddImport from golang.org/x/tools/go/analysis/passes/internal/analysisutil.
For types that already have WriteString(string)(int,error) (e.g. *bytes.Buffer, *strings.Builder), the preferred fix is wText.WriteString(sText) which avoids the import change entirely and is strictly more efficient.
Without fixes, this linter is less useful than its siblings and breaks the established authoring contract for this package.
There was a problem hiding this comment.
Added SuggestedFixes via buildFix in the latest commit. The fix rewrites w.Write([]byte(s)) → io.WriteString(w, s) (or io.WriteString(&w, s) for value receivers) as a single TextEdit.
| type customWriter struct{} | ||
|
|
||
| func (c *customWriter) Write(p []byte) (int, error) { return len(p), nil } | ||
|
|
There was a problem hiding this comment.
Test suite missing a io.Writer interface-typed receiver case: all four bad*() tests use concrete types (bytes.Buffer, *os.File, *customWriter). A very common real-world pattern — func f(w io.Writer) { w.Write([]byte(s)) } — is not covered. If the linter misbehaves for interface-typed receivers (e.g. returns wrong wText) the tests will not catch it.
💡 Suggested addition to testdata
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 avoid a \[\]byte allocation`
}This also requires adding "io" to the test file's imports.
There was a problem hiding this comment.
Added badInterfaceWriter(w io.Writer) fixture. The "io" import was also added to the testdata file.
There was a problem hiding this comment.
Review: writebytestring linter
Solid implementation, well-structured and consistent with existing linters in this repo. The linter correctly identifies w.Write([]byte(s)) patterns across all io.Writer implementations, handles named string types, skips test files, and respects //nolint:writebytestring.
Three findings:
-
Dead code (minor):
len(conv.Args) != 1guard at line 87 can never be true afterisByteSliceConversionreturnstrue— a Go type conversion always has exactly one argument. Safe to remove. -
Maintainability (suggestion):
implementsWriter/hasWriteMethodhand-rolls a method-set check instead of usingtypes.Implementsagainstio.Writer. Functionally correct, but more verbose than necessary and diverges from thenolint.ImplementsErrorpattern already in the codebase. -
Missing
SuggestedFix(suggestion): The peer linterappendbytestringincludes aTextEditfix so tools can auto-apply the rewrite. Adding one here (w.Write([]byte(s))→io.WriteString(w, s)) would make this linter more ergonomic and consistent.
None of these are blocking. The linter logic, tests, and registration are all correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 111.8 AIC · ⌖ 6.32 AIC · ⊞ 4.8K
| wText := astutil.NodeText(pass.Fset, sel.X) | ||
| if sText == "" || wText == "" { | ||
| return | ||
| } |
There was a problem hiding this comment.
The check len(conv.Args) != 1 (line 87) is dead code after isByteSliceConversion returns true. A type-conversion expression such as []byte(s) is always exactly one argument — []byte(a, b) doesn't compile — so this guard can never be true at this point. Consider removing it to avoid misleading readers.
@copilot please address this.
There was a problem hiding this comment.
Removed in the latest commit. The len(conv.Args) != 1 check was dead code after isByteSliceConversion — a []byte(s) type-conversion expression always has exactly one argument.
| if t == nil { | ||
| return false | ||
| } | ||
| return hasWriteMethod(t) || hasWriteMethod(types.NewPointer(t)) |
There was a problem hiding this comment.
The implementsWriter function checks for the Write method manually by building a method set and validating the signature (params/results). This is equivalent to checking io.Writer compliance, but it re-implements that check instead of using types.Implements against the standard io.Writer interface — the same approach used by nolint.ImplementsError.
Using types.Implements would be more idiomatic, shorter, and automatically handles pointer/non-pointer variants. Consider:
func implementsWriter(pass *analysis.Pass, expr ast.Expr) bool {
t := pass.TypesInfo.TypeOf(expr)
if t == nil {
return false
}
writerIface := ... // look up io.Writer via pass.Pkg or types.Universe equivalent
return types.Implements(t, writerIface) || types.Implements(types.NewPointer(t), writerIface)
}(The io.Writer interface can be retrieved at analysis time via pass.Pkg.Imports() or a helper similar to how ImplementsError looks up error from types.Universe.)
This is a maintainability suggestion, not a correctness bug — the manual method is functionally correct.
@copilot please address this.
There was a problem hiding this comment.
Refactored in the latest commit. implementsWriter now uses types.Implements against a package-level synthetic writerIface (built once via an init-time IIFE), following the same pattern as nolint.ImplementsError. The manual hasWriteMethod function has been removed.
| Message: fmt.Sprintf("%s.Write([]byte(%s)) can be replaced with io.WriteString(%s, %s) to avoid a []byte allocation", wText, sText, wText, sText), | ||
| }) | ||
| }) | ||
|
|
There was a problem hiding this comment.
The writebytestring linter reports a diagnostic but provides no SuggestedFix, unlike the closely related appendbytestring linter which includes an automated TextEdit fix.
Since the rewrite from w.Write([]byte(s)) to io.WriteString(w, s) is mechanical and safe, adding a suggested fix would be a nice improvement — tools like gopls and golangci-lint can then apply it automatically. This also makes the linter consistent with appendbytestring.
Not blocking, but worth adding.
@copilot please address this.
There was a problem hiding this comment.
Added SuggestedFixes in the latest commit — buildFix produces a TextEdit replacing the whole call with io.WriteString(w, s).
|
@copilot please run the
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Great work on the
|
…test, wording Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ng, variable rename Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All unresolved threads addressed across two commits:
CI must be re-triggered by a maintainer before merge. |
|
@copilot please run the
|
All prior review feedback has been addressed. Here's the current status:
Actions taken this run: Verified local validation green; no new code changes were needed — all review feedback was already addressed in the existing 7 commits on this branch. Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, |
|
\n@copilot please run the skill, make this PR merge-ready, address all unresolved review feedback, and rerun checks once the branch is up to date.
|
🤖 PR Triage
Rationale: New Run §28909358158
|
|
@copilot please run the
|
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new writebytestring custom go/analysis linter (analyzer #39) that flags w.Write([]byte(s)) calls where s is a string type, and suggests replacing them with io.WriteString(w, s) to potentially avoid an unnecessary heap allocation when the underlying writer implements io.StringWriter.
Changes
New: pkg/linters/writebytestring/ package
Registration
Documentation
Detection logic
All four conditions must hold:
Caveats
References