[linter-miner] feat(linters): add sprintferrdot — flag redundant .Error() calls in fmt format functions#40371
Conversation
… format calls The sprintferrdot analyzer flags calls where err.Error() is passed as a variadic argument to fmt.Sprintf/Errorf/Printf/Fprintf when the corresponding format verb is %s or %v. Those verbs already invoke .Error() implicitly, so the explicit call is redundant and should be removed. Evidence from source scan: 6 instances found in pkg/ (frontmatter_error.go, model_alias_validation.go, actions_build_command.go, generate_action_metadata_command.go). New files: pkg/linters/sprintferrdot/sprintferrdot.go — analyzer pkg/linters/sprintferrdot/sprintferrdot_test.go — tests pkg/linters/sprintferrdot/testdata/src/… — fixtures cmd/linters/main.go — registration Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds a new custom Go analysis linter (sprintferrdot) to the gh-aw linter suite to detect redundant .Error() calls when passing errors to fmt formatting functions with %s / %v, and wires it into the cmd/linters multichecker.
Changes:
- Introduces the
sprintferrdotanalyzer implementation that inspectsfmt.*printf-style calls and reports redundant.Error()usage for%s/%v. - Adds analysistest-based unit tests and testdata fixtures for the new analyzer.
- Registers the new analyzer in
cmd/linters/main.goso it runs with the full custom linter set.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/sprintferrdot/sprintferrdot.go | New analyzer implementation for detecting redundant .Error() in fmt formatting calls. |
| pkg/linters/sprintferrdot/sprintferrdot_test.go | analysistest harness to validate expected diagnostics. |
| pkg/linters/sprintferrdot/testdata/src/sprintferrdot/sprintferrdot.go | Test fixtures (want-comments) covering flagged and allowed patterns. |
| cmd/linters/main.go | Registers sprintferrdot.Analyzer in the multichecker. |
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
| switch sel.Sel.Name { | ||
| case "Sprintf", "Errorf", "Printf": | ||
| return 0, 1, true | ||
| case "Fprintf", "Fscanf": | ||
| return 1, 2, true |
| // goodMultiVerb has a %d verb for the error position — no diagnostic expected. | ||
| func goodMultiVerb(n int, err error) string { | ||
| return fmt.Sprintf("code %d: %T", n, err.Error()) | ||
| } |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (287 new lines under 📄 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 for capturing the decision behind each new linter rather than leaving it implicit in the diff. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( Notes on
Verdict
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The TestSprintfErrDot function correctly uses analysistest.Run to verify the linter behavioral contract with 4 positive and 6 negative test cases.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /grill-with-docs — requesting changes for one correctness bug and a few test coverage gaps.
📋 Key Themes & Highlights
Blocking Issue
Fscanfis a scan function, not a format function (line 115 ofsprintferrdot.go):fmt.Fscanfreads formatted text FROM anio.Reader; its variadic args are output pointers. The linter will produce a false positive if it encountersfmt.Fscanf. Remove it from the case arm —Fprintfalone belongs there.
Test Coverage Gaps
- No fixture for multiple errors in one format call (
%s: %swith two.Error()args) — the loop inruncould have a silent off-by-one. - No fixture for a concrete pointer-receiver struct implementing
error— thetypes.Implements(types.NewPointer(t), errorIface)branch inisErrorDotCallis untested. fmt.Printfis registered but has no positive test case.
Minor Naming
goodNilErrdoesn't test nil errors; rename togoodPassErrDirectlyorgoodSentinelDirect.
Positive Highlights
- ✅
parseSimpleFormatVerbsconservatively returnsnilon%[n]and*width/precision — zero false positives for complex format strings. - ✅
isErrorDotCallchecks bothTand*Treceiver paths — solid defensive type-checking. - ✅ Good motivation: 6 real production hits cited in the PR body, with clear pairing to the existing
errorfwrapvlinter. - ✅ Test file skips generated/test files via
filecheck.IsTestFile— consistent with other linters in the codebase.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 68.2 AIC · ⌖ 7.66 AIC · ⊞ 6.9K
| switch sel.Sel.Name { | ||
| case "Sprintf", "Errorf", "Printf": | ||
| return 0, 1, true | ||
| case "Fprintf", "Fscanf": |
There was a problem hiding this comment.
[/diagnose] Fscanf is a scanning function — it reads formatted input FROM an io.Reader, it does not write formatted output. Its presence here alongside Fprintf is a semantic error that will produce false positives on code that uses fmt.Fscanf.
💡 What goes wrong & suggested fix
fmt.Fscanf(r, "%s", ptr) stores scanned input into ptr. If someone writes fmt.Fscanf(r, "%s", someErr.Error()) the analyzer will incorrectly report a "redundant .Error() call" diagnostic, even though the error relationship here is entirely different from the formatting context this linter is designed for.
Fix: remove Fscanf from this case arm:
case "Fprintf":
return 1, 2, trueAlso add a negative test fixture to confirm Fscanf is never flagged.
| // badFprintf calls fmt.Fprintf with err.Error() and a %s verb — should be flagged. | ||
| func badFprintf(w io.Writer, err error) { | ||
| fmt.Fprintf(w, "error: %s\n", err.Error()) // want `redundant \.Error\(\) call: pass the error value directly with %s` | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing edge case: no fixture tests multiple error arguments in a single format string. Without this, an off-by-one in the variadicArgs slice loop could silently miss second-and-later errors.
💡 Suggested fixture addition
// badSprintfMultiErr verifies both .Error() calls are flagged in one format call.
func badSprintfMultiErr(err1, err2 error) string {
return fmt.Sprintf("%s: %s", err1.Error(), err2.Error()) // want `redundant \.Error\(\) call` `redundant \.Error\(\) call`
}| "os" | ||
| ) | ||
|
|
||
| var sentinel = errors.New("sentinel") |
There was a problem hiding this comment.
[/tdd] All positive fixtures use the error interface directly; there's no test with a concrete pointer-receiver struct that implements error. This would exercise the types.Implements(types.NewPointer(t), errorIface) branch in isErrorDotCall that is otherwise untested.
💡 Suggested fixture addition
type myErr struct{ msg string }
func (e *myErr) Error() string { return e.msg }
func badConcreteErr() string {
err := &myErr{"boom"}
return fmt.Sprintf("failed: %s", err.Error()) // want `redundant \.Error\(\) call: pass the error value directly with %s`
}| func badErrorf(err error) error { | ||
| return fmt.Errorf("wrapped: %s", err.Error()) // want `redundant \.Error\(\) call: pass the error value directly with %s` | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] fmt.Printf is registered in fmtFormatCallInfo (alongside Sprintf and Errorf) but has no positive test case in the fixture. Add a badPrintf case for symmetry and to guard against accidental removal from the switch.
💡 Suggested fixture addition
// badPrintf calls fmt.Printf with err.Error() and a %s verb — should be flagged.
func badPrintf(err error) {
fmt.Printf("error: %s\n", err.Error()) // want `redundant \.Error\(\) call: pass the error value directly with %s`
}| return fmt.Sprintf("file: %s", name) | ||
| } | ||
|
|
||
| // goodNilErr explicitly avoids the pattern. |
There was a problem hiding this comment.
[/grill-with-docs] goodNilErr is a misleading name — this function doesn't test nil-error behavior; it tests passing a non-nil error directly (without .Error()) to confirm the linter stays quiet. Consider goodPassErrDirectly or goodSentinelDirect so the intent is immediately clear to future readers.
There was a problem hiding this comment.
REQUEST_CHANGES — two blocking issues before this can merge.
Blocking
1. No //nolint support (high): pass.Reportf fires unconditionally. //nolint:sprintferrdot directives are silently ignored, giving developers no escape hatch for intentional patterns. 19 of ~30 peer linters in this tree implement nolint.BuildLineIndex — this one must too.
2. Silent no-op when errorIface == nil (medium): if types.Universe.Lookup("error") fails at init, the analyzer runs without error and reports nothing — indistinguishable from a clean codebase. The peer errorfwrapv explicitly return nil, errors.New(...) guards this. Add the same.
Non-blocking
fmt.Printfis listed infmtFormatCallInfobut has zero test fixture coverage — easy to regress silently.fmt.Fscanfis included in the format-function dispatch (flagged in separate comment): it is a scanner, not a printer, and should be removed.goodMultiVerbfixture comment says%dverb is at the error position but the format string uses%Tthere (flagged in separate comment).
🔎 Code quality review by PR Code Quality Reviewer · 81.1 AIC · ⌖ 7.6 AIC · ⊞ 5.1K
| continue | ||
| } | ||
| if isErrorDotCall(pass, arg) { | ||
| pass.Reportf(arg.Pos(), |
There was a problem hiding this comment.
Findings fire unconditionally — //nolint:sprintferrdot will be silently ignored, making unsuppressible CI failures the only outcome for any intentional pattern.
💡 Suggested fix
About 19 of the ~30 peer linters in this tree use nolint.BuildLineIndex to honour suppression directives. Without it, a developer who deliberately calls .Error() for type-safe string extraction (or wants to temporarily suppress a false-positive) has no escape hatch — the linter hard-fails with no workaround.
Add the index at the top of run() and gate pass.Reportf behind it:
import "github.com/github/gh-aw/pkg/linters/internal/nolint"
func run(pass *analysis.Pass) (any, error) {
noLintLines := nolint.BuildLineIndex(pass, "sprintferrdot")
// ...
// in Preorder callback, just before pass.Reportf:
argPos := pass.Fset.PositionFor(arg.Pos(), false)
if !nolint.HasDirective(argPos, noLintLines) {
pass.Reportf(arg.Pos(),
"redundant .Error() call: pass the error value directly with %%%c", verbs[i])
}See pkg/linters/errorfwrapv/errorfwrapv.go for the exact pattern used by the closest peer linter.
| Run: run, | ||
| } | ||
|
|
||
| func run(pass *analysis.Pass) (any, error) { |
There was a problem hiding this comment.
run() does not guard against errorIface == nil: if types.Universe.Lookup("error") ever fails at init, the analyzer silently becomes a no-op — every invocation succeeds with zero findings, indistinguishable from a clean codebase.
💡 Suggested fix
The peer linter errorfwrapv explicitly fails fast at the top of run():
func run(pass *analysis.Pass) (any, error) {
if errorIface == nil {
return nil, errors.New("failed to resolve built-in error interface from types.Universe")
}
// ...Without this guard, a misconfigured or unusual toolchain build where types.Universe is empty will silently pass all files, making the linter undetectable as broken. Returning an error surfaces the failure immediately in CI output.
|
|
||
| // badFprintf calls fmt.Fprintf with err.Error() and a %s verb — should be flagged. | ||
| func badFprintf(w io.Writer, err error) { | ||
| fmt.Fprintf(w, "error: %s\n", err.Error()) // want `redundant \.Error\(\) call: pass the error value directly with %s` |
There was a problem hiding this comment.
fmt.Printf is registered in fmtFormatCallInfo but has no test fixture coverage: a regression that removes or mishandles Printf would go undetected.
💡 Suggested fix
Add a badPrintf case alongside the existing badFprintf:
// badPrintf calls fmt.Printf with err.Error() and a %s verb — should be flagged.
func badPrintf(err error) {
fmt.Printf("error: %s\n", err.Error()) // want `redundant \.Error\(\) call: pass the error value directly with %s`
}Each function listed in fmtFormatCallInfo should have at least one positive fixture to prove the detection path is live.
Summary
Adds
sprintferrdot, a newgo/analysislinter that flags redundant zero-argument.Error()calls on error-typed values passed tofmtformat functions (Sprintf,Errorf,Printf,Fprintf) under%sor%vverbs. Becausefmtalready invokes.Error()implicitly through the error/Stringerinterface, the explicit call is dead code that obscures error-chain intent. A source scan found 6 live production instances of this pattern not caught by any existing linter.ADR:
docs/adr/40371-flag-redundant-error-calls-in-fmt-linter.mdWhat changed and why
pkg/linters/sprintferrdot/sprintferrdot.go(new)Core analyzer implementation following the project's established
pkg/linters/pattern:*ast.CallExprnodes; identifies calls tofmt.{Sprintf,Errorf,Printf,Fprintf}viaastutil.IsPkgSelector; resolves the format-string argument positionally; maps variadic arguments against parsed verbs.parseSimpleFormatVerbs): hand-rolled percent-sequence parser that handles flags, width, and precision digits. Deliberately returnsnil(skips the call) for explicit argument indices (%[n]v) or*width/precision, preventing false positives on complex format strings.isErrorDotCall): confirms the receiver implements the built-inerrorinterface viago/types(types.Implements), so non-error.Error()methods are not flagged.filecheck.IsTestFileto skip_test.gofiles, matching sibling linters.pkg/linters/sprintferrdot/sprintferrdot_test.go(new)analysistest-driven test entry point. Runs the analyzer against the fixture package viaanalysistest.Run.pkg/linters/sprintferrdot/testdata/src/sprintferrdot/sprintferrdot.go(new)Fixture package with annotated
// wantdirectives covering:fmt.Sprintf+%s+.Error()badSprintfSfmt.Sprintf+%v+.Error()badSprintfVfmt.Errorf+%s+.Error()badErrorffmt.Fprintf+%s+.Error()badFprintf%s,%w)goodSprintfS,goodSprintfW.Error()outside a format callgoodStandaloneError%s/%vverb at error positiongoodMultiVerbError()method callgoodNonErrorDotcmd/linters/main.go(modified)Imports
pkg/linters/sprintferrdotand appendssprintferrdot.Analyzerto the analyzer slice passed tomain, making it active in the project-wide lint pass.docs/adr/40371-flag-redundant-error-calls-in-fmt-linter.md(new — draft)Draft ADR documenting:
errorfwrapv; rejects relying solely ongolangci-lint/staticcheck).Behavior
Before: No lint diagnostic. Both of these compile and run silently.
After:
sprintferrdotreports:Intended fix in each case:
Deliberate non-coverage (by design):
%[n]v/*width/precision format strings → skipped (complex positional mapping).Error()called outside afmtformat argument → not flagged (legitimate use)%wverb → not in scope (handled byerrorfwrapv)_test.gofiles → excluded viafilecheck.IsTestFileRelationship to existing linters
errorfwrapvfmt.Errorfwith%vwhere%wshould be usedsprintferrdot(this PR)fmt.{Sprintf,Errorf,Printf,Fprintf}with.Error()under%s/%vThe two analyzers are complementary and deliberately kept separate per the project's one-analyzer-per-smell convention.
Checklist
pkg/linters/conventions (astutil,filecheck,analysistest)cmd/linters/main.goanalysistestfixture covers all four target functions and representative clean casesdocs/adr/40371-...finalized (currently Draft — author action required)