From a4e895fe2e23704f90f8f61317c09068b5322d69 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 19 Jun 2026 18:19:26 +0000 Subject: [PATCH 1/2] feat(linters): add sprintferrdot linter for redundant .Error() in fmt format calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- cmd/linters/main.go | 2 + pkg/linters/sprintferrdot/sprintferrdot.go | 208 ++++++++++++++++++ .../sprintferrdot/sprintferrdot_test.go | 17 ++ .../src/sprintferrdot/sprintferrdot.go | 62 ++++++ 4 files changed, 289 insertions(+) create mode 100644 pkg/linters/sprintferrdot/sprintferrdot.go create mode 100644 pkg/linters/sprintferrdot/sprintferrdot_test.go create mode 100644 pkg/linters/sprintferrdot/testdata/src/sprintferrdot/sprintferrdot.go diff --git a/cmd/linters/main.go b/cmd/linters/main.go index fd3486fe000..acb34ce5a71 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -39,6 +39,7 @@ import ( "github.com/github/gh-aw/pkg/linters/regexpcompileinfunction" "github.com/github/gh-aw/pkg/linters/seenmapbool" "github.com/github/gh-aw/pkg/linters/sortslice" + "github.com/github/gh-aw/pkg/linters/sprintferrdot" "github.com/github/gh-aw/pkg/linters/ssljson" "github.com/github/gh-aw/pkg/linters/strconvparseignorederror" "github.com/github/gh-aw/pkg/linters/timeafterleak" @@ -71,6 +72,7 @@ func main() { ssljson.Analyzer, seenmapbool.Analyzer, sortslice.Analyzer, + sprintferrdot.Analyzer, strconvparseignorederror.Analyzer, jsonmarshalignoredeerror.Analyzer, lenstringzero.Analyzer, diff --git a/pkg/linters/sprintferrdot/sprintferrdot.go b/pkg/linters/sprintferrdot/sprintferrdot.go new file mode 100644 index 00000000000..7a39ffeca18 --- /dev/null +++ b/pkg/linters/sprintferrdot/sprintferrdot.go @@ -0,0 +1,208 @@ +// Package sprintferrdot implements a Go analysis linter that flags redundant +// .Error() calls on error values passed to fmt format functions. +// +// When an error is formatted with %s or %v, the fmt package calls .Error() +// automatically, so calling .Error() explicitly before passing the value +// to the format function is redundant. +package sprintferrdot + +import ( + "go/ast" + "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" +) + +var errorIface = universeErrorInterface() + +// universeErrorInterface returns the built-in error interface type, or nil. +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 +} + +// Analyzer is the sprintf-err-dot analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "sprintferrdot", + Doc: "reports redundant .Error() calls on error arguments passed to fmt format functions", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/sprintferrdot", + 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 + } + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + } + + insp.Preorder(nodeFilter, func(n ast.Node) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } + + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.IsTestFile(pos.Filename) { + return + } + + formatArgIdx, variadicStart, ok := fmtFormatCallInfo(pass, call) + if !ok { + return + } + if formatArgIdx >= len(call.Args) || variadicStart > len(call.Args) { + return + } + + formatStr, ok := extractStringLit(call.Args[formatArgIdx]) + if !ok { + return + } + + verbs := parseSimpleFormatVerbs(formatStr) + if verbs == nil { + return + } + + variadicArgs := call.Args[variadicStart:] + for i, arg := range variadicArgs { + if i >= len(verbs) { + break + } + if verbs[i] != 's' && verbs[i] != 'v' { + continue + } + if isErrorDotCall(pass, arg) { + pass.Reportf(arg.Pos(), + "redundant .Error() call: pass the error value directly with %%%c", verbs[i]) + } + } + }) + + return nil, nil +} + +// fmtFormatCallInfo returns the format-string argument index and the +// variadic-args start index for recognised fmt format functions. +func fmtFormatCallInfo(pass *analysis.Pass, call *ast.CallExpr) (formatArgIdx, variadicStart int, ok bool) { + sel, isSel := call.Fun.(*ast.SelectorExpr) + if !isSel { + return 0, 0, false + } + if !astutil.IsPkgSelector(pass, sel, "fmt") { + return 0, 0, false + } + switch sel.Sel.Name { + case "Sprintf", "Errorf", "Printf": + return 0, 1, true + case "Fprintf", "Fscanf": + return 1, 2, true + default: + return 0, 0, false + } +} + +// extractStringLit returns the unquoted value of a string literal expression. +func extractStringLit(expr ast.Expr) (string, bool) { + lit, ok := expr.(*ast.BasicLit) + if !ok { + return "", false + } + s, err := strconv.Unquote(lit.Value) + if err != nil { + return "", false + } + return s, true +} + +// parseSimpleFormatVerbs returns the list of format verbs in sequential order. +// It returns nil when the format string uses complex features such as explicit +// argument indices (%[n]verb) or * widths/precisions, since those prevent +// reliable positional mapping. +func parseSimpleFormatVerbs(format string) []rune { + var verbs []rune + i := 0 + for i < len(format) { + if format[i] != '%' { + i++ + continue + } + i++ + if i >= len(format) { + break + } + if format[i] == '%' { + i++ + continue + } + // Skip flags: -, +, #, space, 0 + for i < len(format) && (format[i] == '-' || format[i] == '+' || format[i] == '#' || format[i] == ' ' || format[i] == '0') { + i++ + } + if i >= len(format) { + break + } + // Explicit arg index or * width/precision: too complex to analyse. + if format[i] == '[' || format[i] == '*' { + return nil + } + // Skip width digits. + for i < len(format) && format[i] >= '0' && format[i] <= '9' { + i++ + } + // Skip precision. + if i < len(format) && format[i] == '.' { + i++ + if i < len(format) && format[i] == '*' { + return nil + } + for i < len(format) && format[i] >= '0' && format[i] <= '9' { + i++ + } + } + if i >= len(format) { + break + } + verbs = append(verbs, rune(format[i])) + i++ + } + return verbs +} + +// isErrorDotCall reports whether expr is a zero-argument .Error() call on a +// value that implements the error interface. +func isErrorDotCall(pass *analysis.Pass, expr ast.Expr) bool { + call, ok := expr.(*ast.CallExpr) + if !ok || len(call.Args) != 0 { + return false + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Error" { + return false + } + if errorIface == nil { + return false + } + t := pass.TypesInfo.TypeOf(sel.X) + if t == nil { + return false + } + return types.Implements(t, errorIface) || types.Implements(types.NewPointer(t), errorIface) +} diff --git a/pkg/linters/sprintferrdot/sprintferrdot_test.go b/pkg/linters/sprintferrdot/sprintferrdot_test.go new file mode 100644 index 00000000000..32d0812bb5c --- /dev/null +++ b/pkg/linters/sprintferrdot/sprintferrdot_test.go @@ -0,0 +1,17 @@ +//go:build !integration + +// Package sprintferrdot_test provides tests for the sprintferrdot analyzer. +package sprintferrdot_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/sprintferrdot" +) + +func TestSprintfErrDot(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, sprintferrdot.Analyzer, "sprintferrdot") +} diff --git a/pkg/linters/sprintferrdot/testdata/src/sprintferrdot/sprintferrdot.go b/pkg/linters/sprintferrdot/testdata/src/sprintferrdot/sprintferrdot.go new file mode 100644 index 00000000000..4b6e9de1d60 --- /dev/null +++ b/pkg/linters/sprintferrdot/testdata/src/sprintferrdot/sprintferrdot.go @@ -0,0 +1,62 @@ +// Package sprintferrdot is the test fixture for the sprintferrdot analyzer. +package sprintferrdot + +import ( + "errors" + "fmt" + "io" + "os" +) + +var sentinel = errors.New("sentinel") + +// badSprintfS calls fmt.Sprintf with err.Error() and a %s verb — should be flagged. +func badSprintfS(err error) string { + return fmt.Sprintf("operation failed: %s", err.Error()) // want `redundant \.Error\(\) call: pass the error value directly with %s` +} + +// badSprintfV calls fmt.Sprintf with err.Error() and a %v verb — should be flagged. +func badSprintfV(err error) string { + return fmt.Sprintf("operation failed: %v", err.Error()) // want `redundant \.Error\(\) call: pass the error value directly with %v` +} + +// badErrorf calls fmt.Errorf with err.Error() and a %s verb — should be flagged. +func badErrorf(err error) error { + return fmt.Errorf("wrapped: %s", err.Error()) // want `redundant \.Error\(\) call: pass the error value directly with %s` +} + +// 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` +} + +// goodSprintfS passes the error value directly — no diagnostic expected. +func goodSprintfS(err error) string { + return fmt.Sprintf("operation failed: %s", err) +} + +// goodSprintfW wraps with %w — no diagnostic expected. +func goodSprintfW(err error) error { + return fmt.Errorf("wrapped: %w", err) +} + +// goodStandaloneError calls .Error() outside of a format function — no diagnostic expected. +func goodStandaloneError(err error) string { + return err.Error() +} + +// 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()) +} + +// goodNonErrorDot calls .SomeMethod() that is not Error() — no diagnostic expected. +func goodNonErrorDot(f *os.File) string { + name := f.Name() + return fmt.Sprintf("file: %s", name) +} + +// goodNilErr explicitly avoids the pattern. +func goodNilErr() string { + return fmt.Sprintf("sentinel: %s", sentinel) +} From fdaa1b5bed758b19f23f3e34daebe9e95398aa0e Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 19 Jun 2026 18:31:04 +0000 Subject: [PATCH 2/2] docs(adr): add draft ADR-40371 for sprintferrdot linter Co-Authored-By: Claude Opus 4.8 (1M context) --- ...lag-redundant-error-calls-in-fmt-linter.md | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 docs/adr/40371-flag-redundant-error-calls-in-fmt-linter.md diff --git a/docs/adr/40371-flag-redundant-error-calls-in-fmt-linter.md b/docs/adr/40371-flag-redundant-error-calls-in-fmt-linter.md new file mode 100644 index 00000000000..5bf2c65872a --- /dev/null +++ b/docs/adr/40371-flag-redundant-error-calls-in-fmt-linter.md @@ -0,0 +1,38 @@ +# ADR-40371: Flag Redundant `.Error()` Calls in `fmt` Format Functions via a Dedicated Linter + +**Date**: 2026-06-19 +**Status**: Draft + +## Context + +The codebase maintains a suite of custom `go/analysis` analyzers under `pkg/linters/`, each registered in `cmd/linters/main.go` and targeting a single, narrowly-scoped code smell that the default `golangci-lint`/`staticcheck`/`govet` rule sets do not catch. A source scan found 6 live production instances of `err.Error()` being passed to `fmt` format functions (`Sprintf`, `Errorf`, `Fprintf`) under `%s`/`%v` verbs, where `fmt` already invokes `.Error()` implicitly through the error/`Stringer` interface. The explicit call is redundant noise that obscures error-chain inspection intent, and no existing lint rule flags it. The project needs a mechanical, CI-enforceable way to detect and prevent this pattern. + +## Decision + +We will add a new dedicated `go/analysis` analyzer package, `pkg/linters/sprintferrdot`, that reports redundant zero-argument `.Error()` calls on error-typed values passed to recognized `fmt` format functions when the positionally-mapped verb is `%s` or `%v`. The analyzer follows the established `pkg/linters/` pattern — type-based error-interface detection via `go/types`, shared helpers from `internal/astutil` and `internal/filecheck`, and a fixture-driven `analysistest` suite — and is registered in `cmd/linters/main.go` alongside the other analyzers. Format strings using explicit argument indices (`%[n]v`) or `*` width/precision are deliberately skipped to avoid unreliable positional mapping. + +## Alternatives Considered + +### Alternative 1: Rely on existing `golangci-lint` / `staticcheck` configuration +We could attempt to express this rule through existing linter configuration. Rejected because no rule in `errcheck`, `govet`, or default `staticcheck` flags this specific redundancy; there is no configuration knob that captures it, so a custom analyzer is required. + +### Alternative 2: Extend the existing `errorfwrapv` linter instead of a new package +Since `errorfwrapv` already inspects `fmt.Errorf` error-formatting, we could fold this check into it. Rejected to preserve the project's one-analyzer-per-smell convention: the two checks have different scopes (`errorfwrapv` covers `%v`→`%w` in `Errorf` only; `sprintferrdot` covers `.Error()` redundancy across multiple `fmt` functions), and keeping them separate keeps each analyzer's diagnostics and tests focused. + +## Consequences + +### Positive +- Mechanically catches a redundancy class that no existing linter detects, across `Sprintf`/`Errorf`/`Printf`/`Fprintf`. +- Improves readability and consistency by encouraging callers to pass the error value directly; complements `errorfwrapv`. + +### Negative +- Adds another analyzer to maintain, register, and keep fast in the lint pass. +- Positional verb parsing is heuristic and intentionally bails on complex format strings (`%[n]v`, `*` widths), producing false negatives in those cases rather than risking false positives. + +### Neutral +- The analyzer reports only; it does not auto-fix. Flagged sites must be remediated manually (or by a follow-up fixer). +- Test files are skipped via `filecheck.IsTestFile`, matching the convention of sibling linters. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27842164958) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*