[linter-miner] feat(linters): add fmterrorfnoverbs linter#36146
Conversation
Reports fmt.Errorf calls with no format verbs, where errors.New is the idiomatic and cheaper alternative. 14 production occurrences found across wasm stubs and other files. 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. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 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 (fmterrorfnoverbs) to the gh-aw linter suite to flag fmt.Errorf calls whose constant format string has no formatting directives, where errors.New(...) is the cheaper/idiomatic alternative.
Changes:
- Implement
pkg/linters/fmterrorfnoverbsanalyzer and register it incmd/linters. - Add
analysistest-based unit test and testdata fixtures for expected diagnostics.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go | New analyzer implementation that inspects fmt.Errorf calls and reports when format string has no % directives. |
| pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs_test.go | Runs the analyzer via analysistest. |
| pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go | Fixtures for “want” diagnostics and non-diagnostic cases. |
| cmd/linters/main.go | Registers the new analyzer in the multichecker entrypoint. |
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: 4
| import ( | ||
| "go/ast" | ||
| "go/token" | ||
| "go/types" | ||
| "strings" | ||
|
|
||
| "golang.org/x/tools/go/analysis" | ||
| "golang.org/x/tools/go/analysis/passes/inspect" | ||
| "golang.org/x/tools/go/ast/inspector" | ||
| ) |
| insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) | ||
| if !ok { | ||
| return nil, nil | ||
| } |
| // Unquote the string value | ||
| val := lit.Value | ||
| if len(val) >= 2 { | ||
| val = val[1 : len(val)-1] | ||
| } | ||
|
|
||
| if !strings.Contains(val, "%") { | ||
| pass.ReportRangef(call, "fmt.Errorf called with no format verbs; use errors.New(%s) instead", lit.Value) | ||
| } |
| func goodWithWrap(err error) error { | ||
| return fmt.Errorf("wrapper: %w", err) | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 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.
|
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 (134 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 custom linter (vs. adopting an upstream one, or folding the rule into an existing analyzer) is exactly the kind of decision future contributors will want the rationale for. 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 /tdd — requesting changes on one correctness bug and two missing test cases.
📋 Key Themes & Highlights
Key Issues
%%false negative:strings.Contains(val, "%")matches%%-only strings (e.g.fmt.Errorf("100%% done")), silently skipping them — but they have no actual format verbs and should be flagged.- Manual string unquoting:
val[1:len(val)-1]works in practice but is fragile;constant.StringVal(pass.TypesInfo.Types[lit].Value)is the idiomatic, type-system-backed alternative. - Missing test fixtures: No coverage for
%%-only strings or raw (backtick) string literals.
Positive Highlights
- ✅ Type-system-based package resolution in
isFmtErrorfcorrectly handles import aliases — no string-matching on import paths. - ✅ Build tag and
analysistestwiring follow established codebase patterns. - ✅ Good conservative design: no false positives on variables or non-literal first args.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.4M
| if len(val) >= 2 { | ||
| val = val[1 : len(val)-1] | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] False negative for %% escape sequences: strings.Contains(val, "%") returns true for strings like "100%% done", which contain no actual format verbs — so the call is never flagged, even though errors.New would be more appropriate.
💡 Suggested fix
Strip %% before checking for %, so only real format verbs are detected:
if !strings.Contains(strings.ReplaceAll(val, "%%", ""), "%") {
pass.ReportRangef(call, "fmt.Errorf called with no format verbs; use errors.New(%s) instead", lit.Value)
}Add a test fixture for this case:
func badPercentEscape() error {
return fmt.Errorf("100%% done") // want `fmt\.Errorf called with no format verbs`
}|
|
||
| // Unquote the string value | ||
| val := lit.Value | ||
| if len(val) >= 2 { |
There was a problem hiding this comment.
[/tdd] Manual string unquoting (val[1:len(val)-1]) is fragile. It happens to work for both double-quoted and backtick-quoted literals since both delimiters are single characters, but it silently mishandles any edge case go/constant would ordinarily handle. Prefer strconv.Unquote for interpreted string literals and skip the manual slice for raw literals.
💡 Safer approach
import (
"go/constant"
"go/token"
)
// Replace the manual unquoting block with:
val := constant.StringVal(pass.TypesInfo.Types[lit].Value)constant.StringVal already handles interpreted string escapes, raw literals, and multi-line strings uniformly — no manual slicing needed.
|
|
||
| func goodWithWrap(err error) error { | ||
| return fmt.Errorf("wrapper: %w", err) | ||
| } |
There was a problem hiding this comment.
[/tdd] Test fixtures are missing two important edge cases: %% escape sequences (false negative as noted in the implementation) and raw string literals (backtick-quoted). Adding these as fixtures would both document expected behaviour and guard against regressions.
💡 Suggested additions
// Should be flagged — %% is a literal percent, not a verb
func badPercentEscape() error {
return fmt.Errorf("100%% done") // want `fmt\.Errorf called with no format verbs`
}
// Should be flagged — raw string with no verb
func badRawLit() error {
return fmt.Errorf(`no verbs here`) // want `fmt\.Errorf called with no format verbs`
}
// Should NOT be flagged — raw string with a verb
func goodRawLitWithVerb(name string) error {
return fmt.Errorf(`failed to process %s`, name)
}Note: badPercentEscape will only pass once the implementation is fixed to strip %% before the % check.
There was a problem hiding this comment.
REQUEST_CHANGES — one logic bug must be fixed before merge.
### Findings
Blocking (1)
%%false negative:strings.Contains(val, "%")treats%%(the fmt literal-percent escape) as a verb, sofmt.Errorf("100%% complete")is silently skipped. This is a correctness bug in the core detection — the linter's raison d'être.
Non-blocking (1)
- Test fixtures only cover double-quoted strings; raw string literals and the
%%case are untested. Fix the%%logic first and add the missing fixtures together.
What's solid
- Type-safe
isFmtErrorfviaTypesInfo.ObjectOfcorrectly rules out other packages that happen to exportErrorf. - Conservative bail-out on non-literal first arguments avoids false positives.
- Build-tag discipline and
analysistestharness are correct.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.6M
| // Unquote the string value | ||
| val := lit.Value | ||
| if len(val) >= 2 { | ||
| val = val[1 : len(val)-1] |
There was a problem hiding this comment.
False negative: %%-only strings are silently skipped. strings.Contains(val, "%") returns true for "100%%" so fmt.Errorf("100%% complete") is never flagged — but %% is the fmt escape for a literal %, not a format verb.
💡 Suggested fix
Replace the broad % check with a scan that skips %% pairs:
// hasFormatVerb reports whether the raw unquoted literal content contains
// at least one fmt format verb — a % not immediately followed by another %.
func hasFormatVerb(s string) bool {
for i := 0; i < len(s); i++ {
if s[i] == '%' {
if i+1 < len(s) && s[i+1] == '%' {
i++ // skip escaped %%
continue
}
return true
}
}
return false
}Then:
if !hasFormatVerb(val) {
pass.ReportRangef(call, "fmt.Errorf called with no format verbs; use errors.New(%s) instead", lit.Value)
}Add a fixture:
func badWithDoublePercent() error {
return fmt.Errorf("100%% complete") // want `fmt\.Errorf called with no format verbs`
}|
|
||
| func goodWithWrap(err error) error { | ||
| return fmt.Errorf("wrapper: %w", err) | ||
| } |
There was a problem hiding this comment.
Missing test cases leave two detectable paths untested. The fixtures only cover double-quoted strings. Raw string literals (backtick) and the %% false-negative case are not exercised, so regressions in those paths would go unnoticed.
💡 Suggested additions
// raw string literal — should be flagged
func badRaw() error {
return fmt.Errorf(`something went wrong`) // want `fmt\.Errorf called with no format verbs`
}
// %% is not a format verb — should be flagged (once logic is fixed)
func badDoublePercent() error {
return fmt.Errorf("100%% complete") // want `fmt\.Errorf called with no format verbs`
}
// raw string with a real verb — should NOT be flagged
func goodRawWithVerb(name string) error {
return fmt.Errorf(`failed to process %s`, name)
}
// non-literal first arg — should NOT be flagged (conservative)
func goodDynamic(msg string) error {
return fmt.Errorf(msg) // not flagged: format string is not a constant literal
}
|
@copilot apply copilot-review skill |
|
``
|
|
`` Please summarize the remaining blockers and post a short completion plan.
|
|
``
|
|
`` Please summarize the remaining blockers and post a short completion plan.
|
|
|
|
Fix the % false negative and add raw-string/% test coverage.
|
Add
fmterrorfnoverbslinterIntroduces a new static-analysis pass that flags
fmt.Errorfcalls whose format string contains no%verbs, recommendingerrors.Newas the correct alternative.Changes
New linter:
pkg/linters/fmterrorfnoverbsfmterrorfnoverbs.go— Core analyzer implementation. Detectsfmt.Errorf(...)calls where the format string literal has no format verbs and reports a diagnostic suggestingerrors.Newinstead.fmterrorfnoverbs_test.go—analysistest-based test suite wiring the analyzer to its fixture package.testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go— Fixture source file with representative passing and failing examples used by the test suite.Registration
cmd/linters/main.go— Registeredfmterrorfnoverbsin the central analyzer list so it runs as part of the standard linter suite.Documentation
docs/adr/36146-add-fmterrorfnoverbs-linter.md— Draft ADR-36146 recording the motivation, considered alternatives, and decision to introduce this linter as a dedicated analysis pass.Why
fmt.Errorfwith no format verbs allocates a format-engine call needlessly and obscures intent.errors.Newis the idiomatic, allocation-cheaper choice for static error strings. Automating detection prevents the pattern from silently accumulating in the codebase.Checklist
analysistestfixtures