Skip to content

[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.

Sign up for GitHub

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
pelikhan merged 8 commits into from
Jun 14, 2026
Merged

[linter-miner] linter: add errorfwrapv — flag fmt.Errorf using %v to wrap errors instead of %w #39263

Show file tree
Hide file tree
Changes from 1 commit
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] Jun 14, 2026
3b21d26
docs(adr): add draft ADR-39263 for errorfwrapv linter
github-actions[bot] Jun 14, 2026
ce189fd
fix errorfwrapv parsing and docs
Copilot Jun 14, 2026
bdda094
fix: suppress intentional errorfwrapv violation
Copilot Jun 14, 2026
7db30f8
fix: avoid unchecked error interface assertion
Copilot Jun 14, 2026
1eafd1a
refactor: avoid errorfwrapv variable shadowing
Copilot Jun 14, 2026
551cfee
fix: return static error for nil error interface
Copilot Jun 14, 2026
3b971b9
chore: clarify errorfwrapv init error
Copilot Jun 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/linters/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (

"github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred"
"github.com/github/gh-aw/pkg/linters/ctxbackground"
"github.com/github/gh-aw/pkg/linters/errorfwrapv"
"github.com/github/gh-aw/pkg/linters/errormessage"
"github.com/github/gh-aw/pkg/linters/errstringmatch"
"github.com/github/gh-aw/pkg/linters/excessivefuncparams"
Expand Down Expand Up @@ -53,6 +54,7 @@ func main() {
errormessage.Analyzer,
fprintlnsprintf.Analyzer,
errstringmatch.Analyzer,
errorfwrapv.Analyzer,
execcommandwithoutcontext.Analyzer,
Comment on lines 51 to 58
excessivefuncparams.Analyzer,
fileclosenotdeferred.Analyzer,
Expand Down
146 changes: 146 additions & 0 deletions pkg/linters/errorfwrapv/errorfwrapv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
// 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 (
"go/ast"
"go/token"
"go/types"

"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 = types.Universe.Lookup("error").Type().Underlying().(*types.Interface)

Check failure on line 19 in pkg/linters/errorfwrapv/errorfwrapv.go

View workflow job for this annotation

GitHub Actions / lint-go 

type assertion x.(*go/types.Interface) is unchecked and may panic; use the two-value form v, ok := x.(*go/types.Interface) instead

// 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) {
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 argIdx, verb := range verbs {
if verb != 'v' {
continue
}
callArgIdx := 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) map[int]rune {
verbs := make(map[int]rune)
if len(s) >= 2 {
s = s[1 : len(s)-1]
}

argIdx := 0
for i := 0; i < len(s); i++ {
if s[i] != '%' {
continue
}
i++
if i >= len(s) {
break
}
if s[i] == '%' {
continue
}
if s[i] == '[' {
for i < len(s) && s[i] != ']' {
i++
}
i++
if i >= len(s) {
break

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] parseFormatVerbs scans past [n] but never extracts the numeric index — argIdx still increments sequentially. For fmt.Errorf("%[2]v occurred: %[1]s", name, err), the verb %[2]v is recorded at argIdx=0 → callArgIdx=1 = name (not an error), silently missing the real error arg — a false negative.

💡 Suggested fix

Extract the integer between [ and ] and use it (0-based) as the argument index for this verb, and reset the sequential counter:

if s[i] == '[' {
    start := i + 1
    for i < len(s) && s[i] != ']' {
        i++
    }
    n, _ := strconv.Atoi(s[start:i]) // 1-based explicit index
    i++                                // skip ']'
    if i >= len(s) {
        break
    }
    // ... parse flags/width/precision, then:
    verbs[n-1] = rune(s[verbPos]) // use explicit 0-based index
    argIdx = n                    // reset sequential counter per Go fmt spec
    continue
}

Add a test fixture for the explicit-index case:

// BadExplicitIndexV uses a positional index with %v on an error arg.
func BadExplicitIndexV(err error) error {
    return fmt.Errorf("%[1]v: failed", err) // want `fmt\.Errorf formats an error argument with %v`
}

}
}
for i < len(s) {
switch s[i] {
case '-', '+', '#', '0', ' ':
i++
default:
goto width
}
}

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

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, consuming argIdx=0. The actual v verb is then skipped. For the second %v, the code maps argIdx=1 → call.Args[2] = name — missing err at call.Args[3] — false negative.

💡 Suggested fix

Handle * in the width (and precision) slot: treat it as consuming one argument index before reading the verb.

// width:
if i < len(s) && s[i] == '*' {
    argIdx++ // '*' consumes one call.Args slot
    i++
} else {
    for i < len(s) && s[i] >= '0' && s[i] <= '9' {
        i++
    }
}
// precision:
if i < len(s) && s[i] == '.' {
    i++
    if i < len(s) && s[i] == '*' {
        argIdx++
        i++
    } else {
        for i < len(s) && s[i] >= '0' && s[i] <= '9' {
            i++
        }
    }
}

Add a test fixture:

// BadDynamicWidthWrap uses dynamic width and %v for the error.
func BadDynamicWidthWrap(err error) error {
    return fmt.Errorf("%-*v", 10, err) // want `fmt\.Errorf formats an error argument with %v`
}

width:
for i < len(s) && s[i] >= '0' && s[i] <= '9' {
i++
}
if i < len(s) && s[i] == '.' {
i++
for i < len(s) && s[i] >= '0' && s[i] <= '9' {
i++
}
}
if i >= len(s) {
break
}
verbs[argIdx] = rune(s[i])
argIdx++
}

return verbs
}
16 changes: 16 additions & 0 deletions pkg/linters/errorfwrapv/errorfwrapv_test.go
View file
Open in desktop
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")
}
33 changes: 33 additions & 0 deletions pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package errorfwrapv

import (
"errors"
"fmt"
)

var ErrBase = errors.New("base error")

// BadVWrap uses %v to format an error — should be %w.
func BadVWrap(err error) error {
return fmt.Errorf("operation failed: %v", err) // want `fmt\.Errorf formats an error argument with %v`

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Good that BadVWrapExtra shows %v alongside a non-error arg — that's a useful positive case. Consider also adding a fixture for a concrete error-implementing type passed directly (not via the error interface variable) to confirm types.Implements handles pointer receivers correctly.

💡 Suggested fixture 
type myError struct{ msg string }

func (e *myError) Error() string { return e.msg }

// BadConcretePointerWrap passes *myError directly with %v.
func BadConcretePointerWrap(err *myError) error {
	return fmt.Errorf("wrapped: %v", err) // want `fmt\.Errorf formats an error argument with %v`
}

}

// BadVWrapExtra has %v for an error arg alongside a non-error arg.
func BadVWrapExtra(err error, count int) error {
return fmt.Errorf("error %v occurred %d times", err, count) // want `fmt\.Errorf formats an error argument with %v`
}

// GoodWWrap uses %w to properly wrap the error.
func GoodWWrap(err error) error {
return fmt.Errorf("operation failed: %w", err)
}

// GoodNonErrorVerb uses %v for a non-error argument only.
func GoodNonErrorVerb(name string) error {
return fmt.Errorf("operation %v failed", name)
}

// GoodMixedVerbs uses %w for the error and %v for a non-error.
func GoodMixedVerbs(name string, err error) error {
return fmt.Errorf("operation %v failed: %w", name, err)
}

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The nolint suppression path (analyzer lines 79–81) has no fixture. If nolint.HasDirective were accidentally removed or inverted, no test would catch it.

💡 Suggested fixture to add 
// SuppressedByNolint should emit no diagnostic due to the directive.
func SuppressedByNolint(err error) error {
	return fmt.Errorf("operation failed: %v", err) (nolint/redacted):errorfwrapv
}

Loading