[linter-miner] feat(linters): add fprintlnsprintf linter#34498
Conversation
Reports fmt.Fprintln(w, fmt.Sprintf(...)) calls that should be rewritten as fmt.Fprintf(w, ...) to avoid unnecessary allocations. Evidence found in: - pkg/cli/trial_support.go (multiple occurrences) - pkg/cli/update_merge.go (multiple occurrences) - pkg/cli/compile_watch.go (multiple occurrences) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR adds a new custom Go analyzer (fprintlnsprintf) to the gh-aw linter suite to detect redundant fmt.Fprintln(w, fmt.Sprintf(...)) patterns and recommend fmt.Fprintf(w, ...) to avoid intermediate string allocations.
Changes:
- Implement
pkg/linters/fprintlnsprintfanalyzer to reportfmt.Fprintln(..., fmt.Sprintf(...))call patterns. - Add
analysistest-based unit tests andtestdatafixtures for the analyzer. - Register the new analyzer in the
cmd/lintersmultichecker entrypoint.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/fprintlnsprintf/fprintlnsprintf.go |
New analyzer implementation that matches fmt.Fprintln whose last arg is fmt.Sprintf. |
pkg/linters/fprintlnsprintf/fprintlnsprintf_test.go |
Adds analysistest runner for the analyzer. |
pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf.go |
Test fixtures with one flagged and two non-flagged examples. |
cmd/linters/main.go |
Registers the new analyzer in the linter driver. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| // Check if this is fmt.Fprintln with at least 2 args. | ||
| if !isFmtFunc(call, "Fprintln") { | ||
| return | ||
| } | ||
| if len(call.Args) < 2 { | ||
| return | ||
| } | ||
|
|
||
| // Skip test files. | ||
| pos := pass.Fset.Position(call.Pos()) | ||
| if filecheck.IsTestFile(pos.Filename) { | ||
| return | ||
| } | ||
|
|
||
| // Check if the last argument is fmt.Sprintf(...). | ||
| lastArg, ok := call.Args[len(call.Args)-1].(*ast.CallExpr) | ||
| if !ok { | ||
| return | ||
| } | ||
| if !isFmtFunc(lastArg, "Sprintf") { |
| // Check if this is fmt.Fprintln with at least 2 args. | ||
| if !isFmtFunc(call, "Fprintln") { | ||
| return | ||
| } | ||
| if len(call.Args) < 2 { | ||
| return | ||
| } | ||
|
|
||
| // Skip test files. | ||
| pos := pass.Fset.Position(call.Pos()) | ||
| if filecheck.IsTestFile(pos.Filename) { | ||
| return | ||
| } | ||
|
|
||
| // Check if the last argument is fmt.Sprintf(...). | ||
| lastArg, ok := call.Args[len(call.Args)-1].(*ast.CallExpr) | ||
| if !ok { | ||
| return | ||
| } | ||
| if !isFmtFunc(lastArg, "Sprintf") { | ||
| return | ||
| } | ||
|
|
||
| pass.Reportf(call.Pos(), "use fmt.Fprintf instead of fmt.Fprintln(w, fmt.Sprintf(...))") | ||
| }) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good idea for a linter — eliminating the Fprintln+Sprintf double-allocation is a clear win. Two issues need fixing before this is ready.
🔎 Review themes
1. False positive for multi-arg Fprintln (blocking)
fmt.Fprintln(w, "prefix:", fmt.Sprintf(...)) will be flagged even though the transform to fmt.Fprintf is not mechanical (the semantics of multi-arg Fprintln differ). Restrict the match to len(call.Args) == 2. The testdata should also gain a multi-arg not-flagged fixture.
2. Missing nolint support (consistency)
Other linters in the codebase use nolint.BuildLineIndex so callers can suppress with //nolint:fprintlnsprintf. This linter doesn't, making it impossible to suppress on intentional or legacy call-sites.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1M
| if !isFmtFunc(call, "Fprintln") { | ||
| return | ||
| } | ||
| if len(call.Args) < 2 { |
There was a problem hiding this comment.
False positive when Fprintln has more than two args: the guard len(call.Args) < 2 allows cases like fmt.Fprintln(w, "prefix:", fmt.Sprintf(...)) to be flagged, but there is no mechanical single-step rewrite to fmt.Fprintf — the multi-arg form's concatenation semantics differ. The linter should only fire when there is exactly one variadic argument (len(call.Args) == 2).
💡 Suggested fix
Change the guard from:
if len(call.Args) < 2 {
return
}to:
// Only flag the single-arg form: fmt.Fprintln(w, fmt.Sprintf(...)).
// Multi-arg forms cannot be mechanically rewritten to fmt.Fprintf.
if len(call.Args) != 2 {
return
}Also add a fixture to the testdata to confirm the multi-arg form is not flagged:
func notFlaggedMultiArg(name string) {
fmt.Fprintln(os.Stderr, "prefix:", fmt.Sprintf("val=%s", name))
}| return | ||
| } | ||
|
|
||
| pass.Reportf(call.Pos(), "use fmt.Fprintf instead of fmt.Fprintln(w, fmt.Sprintf(...))") |
There was a problem hiding this comment.
nolint suppression is not supported: other linters in the codebase (e.g. errstringmatch) use nolint.BuildLineIndex to respect //nolint:fprintlnsprintf comments. Without this, callers cannot suppress the diagnostic on intentional or legacy call-sites.
💡 Suggested addition
import "github.com/github/gh-aw/pkg/linters/internal/nolint"
func run(pass *analysis.Pass) (any, error) {
insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
noLintLines := nolint.BuildLineIndex(pass, "fprintlnsprintf")
...
// inside the Preorder callback, before Reportf:
if noLintLines[pos.Line] {
return
}
pass.Reportf(...)
|
🧪 Test Quality Sentinel completed test quality analysis. |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR introduces a new static-analysis linter package ( 📄 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 Matter
ADRs 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 References: §26368672971
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.5M
|
|
||
| // Check if this is fmt.Fprintln with at least 2 args. | ||
| if !isFmtFunc(call, "Fprintln") { | ||
| return |
There was a problem hiding this comment.
[/tdd] False positive risk: len(call.Args) < 2 allows fmt.Fprintln(w, x, fmt.Sprintf(...)) to be flagged, but the suggested fmt.Fprintf rewrite does not trivially apply when there are multiple non-writer arguments.
💡 Fix: tighten the guard to exactly 2 arguments
fmt.Fprintln(w, fmt.Sprintf(...)) is the only form where the rewrite to fmt.Fprintf(w, "format\n", args...) is unambiguous. When there are additional positional args (e.g. fmt.Fprintln(w, prefix, fmt.Sprintf(...))), the replacement is not straightforward and flagging it misleads the developer.
Change the guard to:
if len(call.Args) != 2 {
return
}And add a test fixture covering the multi-arg case to confirm it is not flagged:
func notFlagged(name string) {
fmt.Fprintln(os.Stderr, "prefix", fmt.Sprintf("hello %s", name)) // must NOT be flagged
}| if !ok { | ||
| return false | ||
| } | ||
| ident, ok := sel.X.(*ast.Ident) |
There was a problem hiding this comment.
[/tdd] isFmtFunc uses identifier name matching (ident.Name == "fmt") rather than type-checker resolution, so aliased imports silently bypass the linter.
💡 Known limitation — document or fix
Code like:
import f "fmt"
f.Fprintln(os.Stderr, f.Sprintf("hello %s", name)) // NOT flaggedwill be silently missed. This is a known trade-off in name-based linters. Two options:
- Document it — add a comment in
isFmtFuncnoting that aliased imports are out of scope by design (acceptable for an internal codebase wheregoimportsenforces the canonical name). - Fix it — use
pass.TypesInfo.Uses[sel.Sel]to resolve the package path instead:
func isFmtFuncTyped(pass *analysis.Pass, call *ast.CallExpr, name string) bool {
sel, ok := call.Fun.(*ast.SelectorExpr)
if !ok { return false }
obj, ok := pass.TypesInfo.Uses[sel.Sel]
if !ok { return false }
fn, ok := obj.(*types.Func)
if !ok { return false }
return fn.Pkg().Path() == "fmt" && fn.Name() == name
}Option 1 is perfectly fine for this codebase — just make it explicit.
| return | ||
| } | ||
| if !isFmtFunc(lastArg, "Sprintf") { | ||
| return |
There was a problem hiding this comment.
[/grill-with-docs] The diagnostic message does not include the suggested replacement, making it harder for developers to act on the finding.
💡 Include the suggested rewrite in the message
Other linters in this repo (e.g. fileclosenotdeferred) phrase their messages as actionable directives. Consider:
pass.Reportf(call.Pos(),
"replace fmt.Fprintln(w, fmt.Sprintf(format, args...)) with fmt.Fprintf(w, format+\"\\n\", args...)")Or, if you want to show a concrete snippet extracted from the AST, you can use pass.Fset and format.Node to render the Sprintf call, then embed it in the message. At minimum, including "\\n" in the format string hint prevents the common mistake of forgetting the newline when migrating.
| ) | ||
|
|
||
| func flagged(name string) { | ||
| fmt.Fprintln(os.Stderr, fmt.Sprintf("hello %s", name)) // want "use fmt.Fprintf" |
There was a problem hiding this comment.
[/tdd] Test fixture is sparse — only one flagged case and two unflagged cases. Adding edge-case fixtures now prevents future regressions when the implementation evolves.
💡 Suggested additional fixture cases
// Should NOT be flagged: multiple positional args — rewrite is ambiguous
func multiArgs(name string) {
fmt.Fprintln(os.Stderr, "prefix:", fmt.Sprintf("hello %s", name))
}
// Should NOT be flagged: Sprintf is not the last arg
func sprintfNotLast(name string) {
fmt.Fprintln(os.Stderr, fmt.Sprintf("hello %s", name), "suffix")
}
// Should be flagged: Sprintf with no format args
func noFormatArgs() {
fmt.Fprintln(os.Stderr, fmt.Sprintf("static")) // want "use fmt.Fprintf"
}The first two cases are especially important for validating the len(call.Args) != 2 guard suggested in the implementation review.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
No mocks, build tag present ( 📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
|
@copilot review all comments and reviews and resolve once you are done |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I reviewed the feedback, tightened the analyzer to only match the exact 2-arg |
Summary
Adds a new
fprintlnsprintfstatic-analysis linter that detects the redundantfmt.Fprintln(w, fmt.Sprintf(...))pattern and recommends replacing it with themore efficient
fmt.Fprintf(w, format+"\n", args...).The linter was motivated by a linter-miner run (#17) that surfaced 11+ instances
of this pattern across
pkg/cli/trial_support.go(5),pkg/cli/update_merge.go(3), and
pkg/cli/compile_watch.go(3). The pattern is wasteful becausefmt.Sprintfallocates an intermediate string thatfmt.Fprintlnthen wrapsagain for the newline — two heap allocations where one suffices.
What Changed
New linter —
pkg/linters/fprintlnsprintf/fprintlnsprintf.gofprintlnsprintf_test.goanalysistest-based unit test (skipped on integration builds)testdata/src/fprintlnsprintf/fprintlnsprintf.goDetection logic (syntactic, not type-based):
fmt.Fprintln.fmt.Sprintf(...)call.Fprintlncalls (3+ args) are intentionally not flagged —they cannot be trivially collapsed.
fmtimports are not matched (syntactic identifier check only).Diagnostic message:
Linter registration —
cmd/linters/main.goThe
fprintlnsprintfanalyzer is imported and wired intomultichecker.Main,making it available to all linter runs immediately.
Architecture Decision Record —
docs/adr/34498-add-fprintlnsprintf-linter.mdA draft ADR documents:
Commit Breakdown
768ff4cddfeat(linters): add fprintlnsprintf linter0eaab4626docs(adr): add draft ADR-34498 for fprintlnsprintf linter213cf5666fix(linters): avoid multi-arg false positives in fprintlnsprintfThe fix commit tightened the initial implementation to only flag
fmt.Fprintln(w, fmt.Sprintf(...))with exactly two arguments, preventingfalse positives on calls like
fmt.Fprintln(w, fmt.Sprintf(...), extraArg).Test Coverage
The
analysistestharness validates:fmt.Fprintln(w, fmt.Sprintf("fmt: %v", x))fmt.Fprintln(w, "plain string")fmt.Fprintln(w, fmt.Sprintf(...), extraArg)fmt.Fprintf(w, "fmt: %v\n", x)Impact Assessment
import f "fmt"patterns are not caught (documented in ADR)