-
Notifications
You must be signed in to change notification settings - Fork 429
[linter-miner] linter: add errorfwrapv — flag fmt.Errorf using %v to wrap errors instead of %w #39263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
[linter-miner] linter: add errorfwrapv — flag fmt.Errorf using %v to wrap errors instead of %w #39263
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
e746b8a
linter: add errorfwrapv — flag fmt.Errorf %v wrapping errors instead …
github-actions[bot] 3b21d26
docs(adr): add draft ADR-39263 for errorfwrapv linter
github-actions[bot] ce189fd
fix errorfwrapv parsing and docs
Copilot bdda094
fix: suppress intentional errorfwrapv violation
Copilot 7db30f8
fix: avoid unchecked error interface assertion
Copilot 1eafd1a
refactor: avoid errorfwrapv variable shadowing
Copilot 551cfee
fix: return static error for nil error interface
Copilot 3b971b9
chore: clarify errorfwrapv init error
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
42 changes: 42 additions & 0 deletions
42
docs/adr/39263-custom-linter-for-errorf-percent-v-error-wrapping.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| # ADR-39263: Ship a Custom `errorfwrapv` Linter for `fmt.Errorf` Error Wrapping with `%v` | ||
|
|
||
| **Date**: 2026-06-14 | ||
| **Status**: Draft | ||
|
|
||
| ## Context | ||
|
|
||
| `fmt.Errorf("...: %v", err)` formats an `error` argument with `%v`, which renders the error's text but discards the wrapped-error reference. Only the `%w` verb records the cause in the chain that `errors.Is` and `errors.As` traverse, so a `%v` wrap silently breaks sentinel- and type-based error inspection by downstream callers — a correctness bug that is easy to overlook in review. No commonly enabled linter flags this distinction: `go vet`'s `printf` pass validates verb/argument type compatibility but treats `%v` on an error as valid. A scan of this repository (run #38) found a real occurrence in `pkg/workflow/compiler_orchestrator_frontmatter.go:50` (suppressed with a `nolint`), so the project needs an automated guard that fits its existing in-repo linter suite (`pkg/linters/*` registered in `cmd/linters/main.go`). | ||
|
|
||
| ## Decision | ||
|
|
||
| We will add a bespoke `go/analysis` analyzer, `errorfwrapv`, to the project's custom linter suite rather than relying on an external linter. The analyzer inspects `fmt.Errorf` calls whose format argument is a string literal, parses the format string to map each positional verb to its argument index, and reports when a `%v` verb corresponds to an argument whose type implements the `error` interface (verified via `types.Implements` against the universe `error` type, not a syntactic match). It reports at most once per call, honors `//nolint:errorfwrapv` suppression for intentional non-wrapping, skips test files, and is registered in the multichecker in `cmd/linters/main.go`. | ||
|
|
||
| ## Alternatives Considered | ||
|
|
||
| ### Alternative 1: Rely on `go vet` / `golangci-lint` defaults | ||
| The project already runs standard linters, so extending their configuration would avoid new code. Rejected because no default rule distinguishes `%v` from `%w` for error arguments — `go vet`'s `printf` pass considers `%v` on an `error` correct — so the chain-breaking pattern goes uncaught. | ||
|
|
||
| ### Alternative 2: Document the convention and rely on code review | ||
| A contributor guideline plus manual review requires no tooling. Rejected because the `%v` vs `%w` distinction is subtle and easily missed — an instance already reached production with a `nolint` — and manual review provides no repeatable, enforceable guard. | ||
|
|
||
| ### Alternative 3: Syntactic (string-match) detection of `%v` next to an `err` identifier | ||
| A simpler analyzer could flag any `%v` whose argument is named `err`. Rejected because it would both miss errors stored under other names and false-positive on non-error values; type-based `error`-interface checking is more precise at modest extra complexity. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
| - Catches a real, otherwise-undetected correctness pattern (lost error chains) automatically in CI. | ||
| - Follows the established in-repo linter convention, so it composes with the existing `cmd/linters` multichecker and shared `internal` helpers (`astutil`, `filecheck`, `nolint`). | ||
| - Precise: `types.Implements` identity checking and per-argument verb mapping target genuine error wraps while allowing `%v` on non-error arguments. | ||
|
|
||
| ### Negative | ||
| - Adds a custom analyzer the team must maintain, including a hand-rolled format-string verb parser that must track Go's `fmt` flag/width/precision/explicit-index (`%[n]`) syntax as it evolves. | ||
| - Limited to string-literal format arguments: `fmt.Errorf` calls with a non-literal (variable or concatenated) format string are not analyzed, so some misses remain uncaught and may create a false sense of full coverage. | ||
|
|
||
| ### Neutral | ||
| - Suppression is available via `//nolint:errorfwrapv` for intentional non-wrapping cases. | ||
| - Test files are excluded from analysis, matching the suite's existing conventions. | ||
|
|
||
| --- | ||
|
|
||
| *This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27507782045) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,207 @@ | ||
| // Package errorfwrapv implements a Go analysis linter that flags calls to | ||
| // fmt.Errorf that format error arguments with %v instead of %w, which breaks | ||
| // error-chain inspection via errors.Is and errors.As. | ||
| package errorfwrapv | ||
|
|
||
| import ( | ||
| "errors" | ||
| "go/ast" | ||
| "go/token" | ||
| "go/types" | ||
| "strconv" | ||
|
|
||
| "golang.org/x/tools/go/analysis" | ||
| "golang.org/x/tools/go/analysis/passes/inspect" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/internal/astutil" | ||
| "github.com/github/gh-aw/pkg/linters/internal/filecheck" | ||
| "github.com/github/gh-aw/pkg/linters/internal/nolint" | ||
| ) | ||
|
|
||
| var errorIface = universeErrorInterface() | ||
|
|
||
| // universeErrorInterface returns the built-in error interface type, or nil if | ||
| // it cannot be resolved from types.Universe. | ||
| func universeErrorInterface() *types.Interface { | ||
| errorObj := types.Universe.Lookup("error") | ||
| if errorObj == nil { | ||
| return nil | ||
| } | ||
|
|
||
| iface, ok := errorObj.Type().Underlying().(*types.Interface) | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| return iface | ||
| } | ||
|
|
||
| type formatVerb struct { | ||
| argIdx int | ||
| verb rune | ||
| } | ||
|
|
||
| // Analyzer is the errorfwrapv analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "errorfwrapv", | ||
| Doc: "reports fmt.Errorf calls that format error arguments with %v instead of %w", | ||
| URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/errorfwrapv", | ||
| Requires: []*analysis.Analyzer{inspect.Analyzer}, | ||
| Run: 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") | ||
| } | ||
|
|
||
| insp, err := astutil.Inspector(pass) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| noLintLinesByFile := nolint.BuildLineIndex(pass, "errorfwrapv") | ||
|
|
||
| nodeFilter := []ast.Node{ | ||
| (*ast.CallExpr)(nil), | ||
| } | ||
|
|
||
| insp.Preorder(nodeFilter, func(n ast.Node) { | ||
| call, ok := n.(*ast.CallExpr) | ||
| if !ok { | ||
| return | ||
| } | ||
|
|
||
| position := pass.Fset.PositionFor(call.Pos(), false) | ||
| if filecheck.IsTestFile(position.Filename) { | ||
| return | ||
| } | ||
|
|
||
| if !astutil.IsFmtErrorf(pass, call) { | ||
| return | ||
| } | ||
|
|
||
| if len(call.Args) == 0 { | ||
| return | ||
| } | ||
|
|
||
| lit, ok := call.Args[0].(*ast.BasicLit) | ||
| if !ok || lit.Kind != token.STRING { | ||
| return | ||
| } | ||
|
|
||
| verbs := parseFormatVerbs(lit.Value) | ||
| for _, fv := range verbs { | ||
| if fv.verb != 'v' { | ||
| continue | ||
| } | ||
| callArgIdx := fv.argIdx + 1 | ||
| if callArgIdx >= len(call.Args) { | ||
| continue | ||
| } | ||
| tv, ok := pass.TypesInfo.Types[call.Args[callArgIdx]] | ||
| if !ok || tv.Type == nil { | ||
| continue | ||
| } | ||
| if !types.Implements(tv.Type, errorIface) { | ||
| continue | ||
| } | ||
| if nolint.HasDirective(position, noLintLinesByFile) { | ||
| return | ||
| } | ||
| pass.ReportRangef(call, "fmt.Errorf formats an error argument with %%v; use %%w to preserve the error chain") | ||
| return | ||
| } | ||
| }) | ||
|
|
||
| return nil, nil | ||
| } | ||
|
|
||
| func parseFormatVerbs(s string) []formatVerb { | ||
| var verbs []formatVerb | ||
| if len(s) >= 2 { | ||
| s = s[1 : len(s)-1] | ||
| } | ||
|
|
||
| nextArgIdx := 0 | ||
| for i := 0; i < len(s); i++ { | ||
| if s[i] != '%' { | ||
| continue | ||
| } | ||
| i++ | ||
| if i >= len(s) { | ||
| break | ||
| } | ||
| if s[i] == '%' { | ||
| continue | ||
| } | ||
|
|
||
| valueArgIdx := 0 | ||
| hasExplicitValueArg := false | ||
| if idx, nextPos, ok := parseFormatArgIndex(s, i); ok { | ||
| valueArgIdx = idx | ||
| nextArgIdx = idx + 1 | ||
| hasExplicitValueArg = true | ||
| i = nextPos | ||
| } | ||
| for i < len(s) { | ||
| switch s[i] { | ||
| case '-', '+', '#', '0', ' ': | ||
| i++ | ||
| default: | ||
| goto width | ||
| } | ||
| } | ||
|
|
||
| width: | ||
| i = consumeFormatWidthOrPrecision(s, i, &nextArgIdx) | ||
| if i < len(s) && s[i] == '.' { | ||
| i++ | ||
| i = consumeFormatWidthOrPrecision(s, i, &nextArgIdx) | ||
| } | ||
| if i >= len(s) { | ||
| break | ||
| } | ||
| if !hasExplicitValueArg { | ||
| valueArgIdx = nextArgIdx | ||
| nextArgIdx++ | ||
| } | ||
| verbs = append(verbs, formatVerb{argIdx: valueArgIdx, verb: rune(s[i])}) | ||
| } | ||
|
|
||
| return verbs | ||
| } | ||
|
|
||
| func consumeFormatWidthOrPrecision(s string, i int, nextArgIdx *int) int { | ||
| if idx, nextPos, ok := parseFormatArgIndex(s, i); ok && nextPos < len(s) && s[nextPos] == '*' { | ||
| *nextArgIdx = idx + 1 | ||
| return nextPos + 1 | ||
| } | ||
| if i < len(s) && s[i] == '*' { | ||
| *nextArgIdx = *nextArgIdx + 1 | ||
| return i + 1 | ||
| } | ||
| for i < len(s) && s[i] >= '0' && s[i] <= '9' { | ||
| i++ | ||
| } | ||
| return i | ||
| } | ||
|
|
||
| func parseFormatArgIndex(s string, i int) (int, int, bool) { | ||
| if i >= len(s) || s[i] != '[' { | ||
| return 0, i, false | ||
| } | ||
|
|
||
| j := i + 1 | ||
| for j < len(s) && s[j] >= '0' && s[j] <= '9' { | ||
| j++ | ||
| } | ||
| if j == i+1 || j >= len(s) || s[j] != ']' { | ||
| return 0, i, false | ||
| } | ||
|
|
||
| n, err := strconv.Atoi(s[i+1 : j]) | ||
| if err != nil || n <= 0 { | ||
| return 0, i, false | ||
| } | ||
| return n - 1, j + 1, true | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| //go:build !integration | ||
|
|
||
| package errorfwrapv_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "golang.org/x/tools/go/analysis/analysistest" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/errorfwrapv" | ||
| ) | ||
|
|
||
| func TestErrorfWrapV(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, errorfwrapv.Analyzer, "errorfwrapv") | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd]
%*v(width taken from an argument, e.g.fmt.Errorf("%-*v: %v", 10, name, err)) is not handled. The parser exits the width loop on*and records*as the verb, consumingargIdx=0. The actualvverb is then skipped. For the second%v, the code mapsargIdx=1 → call.Args[2] = name— missingerratcall.Args[3]— false negative.💡 Suggested fix
Handle
*in the width (and precision) slot: treat it as consuming one argument index before reading the verb.Add a test fixture: