From 51902d52e268632eb2374b41d4cee731d2a93627 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 31 May 2026 17:55:03 +0000 Subject: [PATCH 1/2] feat(linters): add fmterrorfnoverbs linter 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> --- cmd/linters/main.go | 2 + .../fmterrorfnoverbs/fmterrorfnoverbs.go | 91 +++++++++++++++++++ .../fmterrorfnoverbs/fmterrorfnoverbs_test.go | 16 ++++ .../src/fmterrorfnoverbs/fmterrorfnoverbs.go | 27 ++++++ 4 files changed, 136 insertions(+) create mode 100644 pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go create mode 100644 pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs_test.go create mode 100644 pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go diff --git a/cmd/linters/main.go b/cmd/linters/main.go index f288339af12..a92f684bf8d 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -22,6 +22,7 @@ import ( "github.com/github/gh-aw/pkg/linters/errstringmatch" "github.com/github/gh-aw/pkg/linters/excessivefuncparams" "github.com/github/gh-aw/pkg/linters/fileclosenotdeferred" + "github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs" "github.com/github/gh-aw/pkg/linters/fprintlnsprintf" "github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror" "github.com/github/gh-aw/pkg/linters/largefunc" @@ -45,6 +46,7 @@ func main() { errstringmatch.Analyzer, excessivefuncparams.Analyzer, fileclosenotdeferred.Analyzer, + fmterrorfnoverbs.Analyzer, largefunc.Analyzer, manualmutexunlock.Analyzer, osexitinlibrary.Analyzer, diff --git a/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go b/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go new file mode 100644 index 00000000000..ebfd239ac29 --- /dev/null +++ b/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go @@ -0,0 +1,91 @@ +// Package fmterrorfnoverbs implements a Go analysis linter that flags calls to +// fmt.Errorf where the format string contains no format verbs, in which case +// errors.New is the idiomatic and cheaper alternative. +package fmterrorfnoverbs + +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" +) + +// Analyzer is the fmterrorfnoverbs analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "fmterrorfnoverbs", + Doc: "reports fmt.Errorf calls whose format string contains no verbs, preferring errors.New", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/fmterrorfnoverbs", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + if !ok { + return nil, nil + } + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + } + + insp.Preorder(nodeFilter, func(n ast.Node) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } + + if !isFmtErrorf(pass, call) { + return + } + + if len(call.Args) == 0 { + return + } + + lit, ok := call.Args[0].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return + } + + // 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) + } + }) + + return nil, nil +} + +// isFmtErrorf returns true if the call expression is a call to fmt.Errorf. +func isFmtErrorf(pass *analysis.Pass, call *ast.CallExpr) bool { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return false + } + if sel.Sel.Name != "Errorf" { + return false + } + pkgIdent, ok := sel.X.(*ast.Ident) + if !ok { + return false + } + obj := pass.TypesInfo.ObjectOf(pkgIdent) + if obj == nil { + return false + } + pkgName, ok := obj.(*types.PkgName) + if !ok { + return false + } + return pkgName.Imported().Path() == "fmt" +} diff --git a/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs_test.go b/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs_test.go new file mode 100644 index 00000000000..766bca471bc --- /dev/null +++ b/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package fmterrorfnoverbs_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs" +) + +func TestFmtErrorfNoVerbs(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, fmterrorfnoverbs.Analyzer, "fmterrorfnoverbs") +} diff --git a/pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go b/pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go new file mode 100644 index 00000000000..676970f76dd --- /dev/null +++ b/pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go @@ -0,0 +1,27 @@ +// Package fmterrorfnoverbs contains test fixtures for the fmterrorfnoverbs linter. +package fmterrorfnoverbs + +import ( + "errors" + "fmt" +) + +func bad() error { + return fmt.Errorf("something went wrong") // want `fmt\.Errorf called with no format verbs; use errors\.New` +} + +func badMultiLine() error { + return fmt.Errorf("interactive input not available in Wasm") // want `fmt\.Errorf called with no format verbs; use errors\.New` +} + +func good() error { + return errors.New("something went wrong") +} + +func goodWithVerb(name string) error { + return fmt.Errorf("failed to process %s", name) +} + +func goodWithWrap(err error) error { + return fmt.Errorf("wrapper: %w", err) +} From cd4f1aa3f2a2eaaa6409eb95b054ff6e71da3c7a Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 31 May 2026 18:19:27 +0000 Subject: [PATCH 2/2] docs(adr): add draft ADR-36146 for fmterrorfnoverbs linter Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/adr/36146-add-fmterrorfnoverbs-linter.md | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 docs/adr/36146-add-fmterrorfnoverbs-linter.md diff --git a/docs/adr/36146-add-fmterrorfnoverbs-linter.md b/docs/adr/36146-add-fmterrorfnoverbs-linter.md new file mode 100644 index 00000000000..f6385adb59e --- /dev/null +++ b/docs/adr/36146-add-fmterrorfnoverbs-linter.md @@ -0,0 +1,40 @@ +# ADR-36146: Add a dedicated `fmterrorfnoverbs` analysis-pass linter + +**Date**: 2026-05-31 +**Status**: Draft + +## Context + +The codebase already maintains a set of custom `golang.org/x/tools/go/analysis` linters under `pkg/linters/`, each as its own package and registered centrally in `cmd/linters/main.go`. A recurring anti-pattern was observed: `fmt.Errorf` is called with a format string that contains no `%` verbs (14 production occurrences across `pkg/console/*`, `pkg/workflow`, and `pkg/parser`), where `errors.New` is the idiomatic and allocation-cheaper alternative. The same issue surfaced independently from the `lint-go` CI step in PR #36140. There was no automated guard preventing this pattern from recurring. + +## Decision + +We will add a new standalone analysis-pass linter, `fmterrorfnoverbs`, in its own package `pkg/linters/fmterrorfnoverbs/`, and register it in the central analyzer list in `cmd/linters/main.go`. The analyzer inspects `*ast.CallExpr` nodes, resolves the callee to `fmt.Errorf` via type information (`pass.TypesInfo`), and reports any call whose first argument is a string literal containing no `%` character. This follows the existing per-linter package convention and uses `analysistest`-based test fixtures, consistent with sibling linters. + +## Alternatives Considered + +### Alternative 1: Rely on an off-the-shelf linter (e.g. `perfsprint` / golangci-lint) +Existing third-party linters such as `perfsprint` cover this exact case. It was not chosen because the repository has deliberately standardized on a homegrown `pkg/linters/` analysis-pass framework, giving full control over rule semantics, messaging, and CI integration without adding an external linter aggregator dependency. *[TODO: verify whether adopting an existing linter was explicitly evaluated and rejected.]* + +### Alternative 2: Add the check to an existing linter package +The verb check could have been folded into a broader existing analyzer rather than introducing a new package. It was not chosen because each rule in `pkg/linters/` is intentionally a single-responsibility, independently testable package, which keeps rules discoverable and individually toggleable. + +## Consequences + +### Positive +- Automated, repeatable prevention of the no-verb `fmt.Errorf` pattern, removing reliance on ad-hoc review. +- Consistent with the established one-package-per-linter convention, so the addition is low-friction for maintainers. +- Encourages idiomatic, marginally cheaper `errors.New` usage across the codebase. + +### Negative +- Detection is limited to direct string-literal first arguments; format strings built from constants, concatenation, or variables are not flagged (false negatives). +- The naive unquoting (stripping first/last byte and a raw `strings.Contains(val, "%")` check) does not account for escaped `%%` or raw string literals, which could in edge cases mis-handle the literal. *[TODO: verify escaped-percent and backtick-literal handling.]* +- Adds yet another custom linter to maintain in-house rather than leveraging an upstream-maintained equivalent. + +### Neutral +- The existing 14 production occurrences are not auto-fixed by this PR; they must be remediated separately once the linter is enforced in CI. +- The analyzer requires the `inspect` pass and type information, matching the resource profile of sibling linters. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26720516192) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*