[linter-miner] linter: add sprintferrorsnew — flag errors.New(fmt.Sprintf(...))#40490
Conversation
Adds a new custom go/analysis linter that detects the inline anti-pattern
of wrapping fmt.Sprintf inside errors.New:
errors.New(fmt.Sprintf("invalid engine: %s", id))
This should always be replaced with:
fmt.Errorf("invalid engine: %s", id)
Using fmt.Errorf is shorter, avoids a redundant heap allocation for the
intermediate string, and supports %w for error wrapping when needed.
Evidence (found by linter-miner run #44):
- Code-pattern scan identified 4 instances across pkg/workflow:
engine.go, engine_definition.go, engine_validation.go,
runtime_validation.go
- Not covered by any commonly-enabled golangci-lint rule
Implementation follows the largefunc conventions:
- pkg/linters/sprintferrorsnew/sprintferrorsnew.go (Analyzer)
- pkg/linters/sprintferrorsnew/sprintferrorsnew_test.go
- pkg/linters/sprintferrorsnew/testdata/src/sprintferrorsnew/ (fixtures)
- cmd/linters/main.go updated to register the analyzer
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. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a new go/analysis linter (sprintferrorsnew) to detect the inline anti-pattern errors.New(fmt.Sprintf(...)) and registers it in the cmd/linters multichecker so it can be run with the existing linter driver.
Changes:
- Introduces
pkg/linters/sprintferrorsnewanalyzer to reporterrors.New(fmt.Sprintf(...))and recommendfmt.Errorf. - Adds
analysistestcoverage + test fixtures for flagged and allowed cases. - Registers the new analyzer in
cmd/linters/main.go.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/sprintferrorsnew/sprintferrorsnew.go |
New analyzer implementation that detects errors.New(fmt.Sprintf(...)). |
pkg/linters/sprintferrorsnew/sprintferrorsnew_test.go |
analysistest harness for the analyzer. |
pkg/linters/sprintferrorsnew/testdata/src/sprintferrorsnew/sprintferrorsnew.go |
Fixture cases for diagnostics and non-diagnostics. |
cmd/linters/main.go |
Registers sprintferrorsnew.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 the sole argument is a direct fmt.Sprintf(...) call. | ||
| argCall, ok := call.Args[0].(*ast.CallExpr) | ||
| if !ok { | ||
| return | ||
| } |
| ssljson.Analyzer, | ||
| seenmapbool.Analyzer, | ||
| sortslice.Analyzer, | ||
| sprintferrdot.Analyzer, | ||
| sprintferrorsnew.Analyzer, | ||
| strconvparseignorederror.Analyzer, |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (123 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. Adding a new linter is a small but real design decision (scope, precision trade-offs, where it lives) — capturing it helps future contributors understand the suite's conventions. 📋 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 /tdd and /grill-with-docs — commenting with improvement suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: the test-file exemption (
filecheck.IsTestFile) has no fixture locking it in; one of the two "bad" cases tests an identical AST path rather than an independent edge case. - Missing auto-fix: the transformation is 100% mechanical and an ideal candidate for
analysis.SuggestedFixes— without it developers must manually refactor every flagged site. - Diagnostic message: the PR body highlights
%wsupport as a motivating reason to preferfmt.Errorf, but the emitted message omits this hint.
Positive Highlights
- ✅ Clean implementation using existing
astutil/filecheckhelpers — consistent with sibling linters. - ✅ Intentionally narrow scope (inline form only) is well-documented and a sound design decision.
- ✅
goodVariablefixture explicitly documents the purposeful exclusion of the two-step pattern. - ✅ PR body includes solid gap analysis confirming no existing rule covers this pattern.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 66.5 AIC · ⌖ 10 AIC · ⊞ 6.9K
| return | ||
| } | ||
|
|
||
| if filecheck.IsTestFile(pass.Fset.Position(call.Pos()).Filename) { |
There was a problem hiding this comment.
[/tdd] The filecheck.IsTestFile guard is a behavioral contract — but there is no fixture confirming that *_test.go files are actually exempt. A regression in IsTestFile would silently change the linter's scope.
💡 Suggested fixture addition
Add testdata/src/sprintferrorsnew/sprintferrorsnew_test.go containing the bad pattern without a // want annotation:
package sprintferrorsnew_test
import (
"errors"
"fmt"
"testing"
)
func TestExample(t *testing.T) {
// No diagnostic expected — test files are exempt.
_ = errors.New(fmt.Sprintf("test value: %s", "x"))
}analysistest treats the absence of a // want annotation on a flagged line as a test failure, so this fixture would lock in the exemption behaviour automatically.
| return | ||
| } | ||
|
|
||
| pass.Reportf(call.Pos(), "use fmt.Errorf instead of errors.New(fmt.Sprintf(...))") |
There was a problem hiding this comment.
[/tdd] This transformation is fully mechanical — errors.New(fmt.Sprintf(args...)) → fmt.Errorf(args...) — making it an ideal candidate for a SuggestedFix. Without one, developers must manually refactor every flagged site.
💡 Adding a SuggestedFix + richer message
Replace pass.Reportf(...) with pass.Report(analysis.Diagnostic{...}) and supply a SuggestedFixes slice. The fix needs to:
- Replace the entire
errors.New(fmt.Sprintf(...))expression withfmt.Errorf(...)reusing the same arguments. - Remove the
"errors"import if it becomes unused (theanalysisutil.RemoveImporthelper covers this).
Also consider enriching the message to surface the %w benefit — the PR body highlights this as a motivating reason but the emitted diagnostic omits it:
pass.Reportf(call.Pos(), "use fmt.Errorf instead of errors.New(fmt.Sprintf(...)); fmt.Errorf also supports %%w for error wrapping")Sibling linter sprintferrdot is a good reference for how auto-fix is wired in this package.
| } | ||
|
|
||
| // badWithMultipleArgs demonstrates the flagged pattern with multiple format args. | ||
| func badWithMultipleArgs() error { |
There was a problem hiding this comment.
[/tdd] badWithMultipleArgs exercises the same AST path as bad — the analyzer never inspects how many arguments are passed to fmt.Sprintf, so varying the argument count adds no coverage.
💡 More distinctive edge cases
Consider replacing this with a case that actually exercises a different branch, such as a dynamic format string:
// badDynamicFormat: format string is a variable, not a literal — should still be flagged.
func badDynamicFormat(format string) error {
return errors.New(fmt.Sprintf(format)) // want `use fmt\.Errorf instead of errors\.New\(fmt\.Sprintf\(\.\.\.\)\)`
}This makes explicit that the linter fires on any fmt.Sprintf call as the sole argument, regardless of the inner argument types, and gives the fixture clearer specification value.
There was a problem hiding this comment.
Non-blocking observations
The implementation is correct for the stated scope — errors.New(fmt.Sprintf(...)) is detected accurately using type-aware import-path matching, and the detection logic has no false-positive or panic risk.
Observations (non-blocking)
1. pass.Fset.Position() vs PositionFor(false) for test-file exclusion — Using the adjusted form (which follows //line directives) is inconsistent with sprintferrdot and the majority of linters in this codebase. In generated code that uses //line directives, this can produce false negatives or unexpected behavior. One-line fix: use PositionFor(call.Pos(), false) to match the established pattern.
2. Missing test case for zero-variadic-arg fmt.Sprintf — errors.New(fmt.Sprintf("static string")) is correctly flagged, but the diagnostic message "use fmt.Errorf instead" is slightly misleading for this shape; the better fix is errors.New("static string"). The test fixture does not cover this case, leaving diagnostic quality for this edge case unverified.
🔎 Code quality review by PR Code Quality Reviewer · 83 AIC · ⌖ 7.75 AIC · ⊞ 5.1K
| return | ||
| } | ||
|
|
||
| if filecheck.IsTestFile(pass.Fset.Position(call.Pos()).Filename) { |
There was a problem hiding this comment.
Adjusted filename used for test-file exclusion: pass.Fset.Position() follows //line directives and could misidentify whether a file is a test file.
💡 Details
The majority of linters in this codebase — including sprintferrdot, the nearest sibling — use pass.Fset.PositionFor(call.Pos(), false) (unadjusted) when feeding the filename to filecheck.IsTestFile. pass.Fset.Position() is equivalent to PositionFor(pos, true) and follows //line directives.
With adjusted positions:
- A non-test file containing a
//line ..._test.go:Ndirective would make the linter silently skip allerrors.New(fmt.Sprintf(...))calls in that file (false negatives). - A test file with a
//line ...non_test.go:Ndirective would be analyzed when it should be excluded (false positives in generated code).
Generated code frequently uses //line directives to point back to source templates, so this is a realistic edge case.
Suggested fix:
- if filecheck.IsTestFile(pass.Fset.Position(call.Pos()).Filename) {
+ if filecheck.IsTestFile(pass.Fset.PositionFor(call.Pos(), false).Filename) {|
|
||
| // bad demonstrates the flagged pattern: errors.New wrapping a fmt.Sprintf call. | ||
| func bad() error { | ||
| return errors.New(fmt.Sprintf("invalid engine: %s", "claude")) // want `use fmt\.Errorf instead of errors\.New\(fmt\.Sprintf\(\.\.\.\)\)` |
There was a problem hiding this comment.
Missing test case: errors.New(fmt.Sprintf("static")) — no variadic args: the linter will flag this but the suggested message is slightly misleading for this shape.
💡 Details
The test fixture covers two bad cases, both with at least one format argument. There is no case for:
return errors.New(fmt.Sprintf("something went wrong")) // no variadic argsThe linter will correctly flag this (it matches errors.New(<fmt.Sprintf call>)), but the diagnostic "use fmt.Errorf instead of errors.New(fmt.Sprintf(...))" is subtly misleading: when fmt.Sprintf has no format arguments, the best fix is errors.New("something went wrong") — eliminating fmt.Sprintf entirely — not fmt.Errorf("something went wrong").
Two options to address this:
- Add a test fixture case to explicitly document the current behavior and detect any future regression.
- Detect the zero-variadic-arg case in the analyzer and emit a different message (e.g., "use errors.New("...") directly — fmt.Sprintf with no format arguments is a no-op").
Option 1 is lower cost; option 2 makes the fix more actionable for users.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( Notes: Testdata coverage:
Test inflation: 17 test lines vs 73 production lines (ratio 0.23:1 — well within 2:1 threshold). Verdict
|
Summary
Adds a new
sprintferrorsnewGo static-analysis linter that flags the anti-patternerrors.New(fmt.Sprintf(...))and recommends usingfmt.Errorfinstead. The linter is fully wired into the analyzer registry, covered by ananalysistest-based unit test, and documented with a draft ADR.What changed
New:
sprintferrorsnewanalyzerpkg/linters/sprintferrorsnew/sprintferrorsnew.go(added, high impact)Implements a
go/analysispass that detects call expressions of the formerrors.New(fmt.Sprintf(...))and emits a diagnostic recommendingfmt.Errorfinstead. Non-flagged patterns (bare string literals, pre-formatted variables,
existing
fmt.Errorfcalls) are explicitly left untouched.Registration
cmd/linters/main.go(modified, medium impact)Imports the new package and appends the
sprintferrorsnewanalyzer to themain analyzer slice, making it active in all linter runs.
Tests & fixtures
pkg/linters/sprintferrorsnew/sprintferrorsnew_test.go(added, low impact)Thin
analysistestdriver that runs the analyzer against the fixture packageand asserts expected diagnostics.
pkg/linters/sprintferrorsnew/testdata/src/sprintferrorsnew/sprintferrorsnew.go(added, low impact)Annotated fixture covering:
errors.New(fmt.Sprintf(...))fmt.Errorf, variable-held format string, plain string literalDocumentation
docs/adr/40490-add-sprintferrorsnew-linter.md(added, low impact)Draft ADR capturing the rationale for the new linter, alternatives considered,
and expected consequences.
Impact
No breaking changes. Existing code that triggers the new diagnostic will receive
a lint error but no automatic fixes are applied.
Checklist
go/analysiscmd/linters/main.goanalysistestunit test addedADR-40490)