diff --git a/cmd/linters/main.go b/cmd/linters/main.go index acb34ce5a71..b47cf092bae 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -40,6 +40,7 @@ import ( "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/sprintferrorsnew" "github.com/github/gh-aw/pkg/linters/ssljson" "github.com/github/gh-aw/pkg/linters/strconvparseignorederror" "github.com/github/gh-aw/pkg/linters/timeafterleak" @@ -73,6 +74,7 @@ func main() { seenmapbool.Analyzer, sortslice.Analyzer, sprintferrdot.Analyzer, + sprintferrorsnew.Analyzer, strconvparseignorederror.Analyzer, jsonmarshalignoredeerror.Analyzer, lenstringzero.Analyzer, diff --git a/docs/adr/40490-add-sprintferrorsnew-linter.md b/docs/adr/40490-add-sprintferrorsnew-linter.md new file mode 100644 index 00000000000..5d7a88fdf13 --- /dev/null +++ b/docs/adr/40490-add-sprintferrorsnew-linter.md @@ -0,0 +1,39 @@ +# ADR-40490: Add `sprintferrorsnew` linter for `errors.New(fmt.Sprintf(...))` + +**Date**: 2026-06-20 +**Status**: Draft + +## Context + +The codebase contains a custom `go/analysis` linter suite (`pkg/linters/*`, registered in `cmd/linters/main.go`) used to enforce house style and catch anti-patterns that off-the-shelf golangci-lint rules miss. A recurring pattern was found in `pkg/workflow` where error values are built with `errors.New(fmt.Sprintf(...))` instead of the idiomatic `fmt.Errorf(...)`. The inline form is longer, allocates an intermediate string on the heap, and cannot use `%w` for error wrapping. Existing enabled rules (`errcheck`, `errorlint`, `nilerr`) do not detect it. The constraint is that any new rule must integrate with the established analyzer framework and keep false positives low. + +## Decision + +We will add a new `go/analysis` analyzer package `pkg/linters/sprintferrorsnew` and register it in `cmd/linters/main.go`. The analyzer flags only the **inline** form `errors.New(fmt.Sprintf(...))` — a single-argument `errors.New` call whose sole argument is a direct `fmt.Sprintf` call — using AST inspection plus the shared `astutil.IsPkgSelector` helper to confirm package identity. Test files are skipped via `filecheck.IsTestFile`, and the variable-assignment form (`msg := fmt.Sprintf(...); errors.New(msg)`) is deliberately not flagged to preserve a high signal-to-noise ratio. + +## Alternatives Considered + +### Alternative 1: Extend an existing golangci-lint rule / use an off-the-shelf linter +No commonly enabled rule (`errcheck`, `errorlint`, `nilerr`, etc.) detects this pattern, and adding a third-party dependency or upstream change is heavier and slower than a focused local analyzer that fits the existing `pkg/linters` framework. Rejected as higher cost with less control. + +### Alternative 2: Also flag the variable-assignment form +The analyzer could trace `errors.New(msg)` back to a nearby `msg := fmt.Sprintf(...)`. This requires dataflow tracking, increases false-positive risk (the variable may be reused or conditionally assigned), and adds complexity. Rejected to keep the rule simple and high-precision; only the unambiguous inline form is flagged. + +## Consequences + +### Positive +- Catches a real, repeated anti-pattern (4 instances in `pkg/workflow`) that existing linters miss. +- Encourages `fmt.Errorf`, enabling `%w` wrapping and avoiding a redundant allocation. +- Follows the established analyzer pattern, so it is consistent with the rest of the suite and covered by `analysistest`-based tests. + +### Negative +- Adds another analyzer to maintain and run, marginally increasing lint time and the registration surface in `cmd/linters/main.go`. +- The deliberate scope limit means the variable-assignment form goes undetected, so some occurrences of the anti-pattern remain unflagged. + +### Neutral +- Only the inline form is reported; authors who prefer the assignment form for readability are unaffected. +- The rule is purely diagnostic (`pass.Reportf`); it offers no automated fix/suggested edit. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27880137982) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/linters/sprintferrorsnew/sprintferrorsnew.go b/pkg/linters/sprintferrorsnew/sprintferrorsnew.go new file mode 100644 index 00000000000..4352a677a6c --- /dev/null +++ b/pkg/linters/sprintferrorsnew/sprintferrorsnew.go @@ -0,0 +1,73 @@ +// Package sprintferrorsnew implements a Go analysis linter that flags +// errors.New(fmt.Sprintf(...)) calls that should use fmt.Errorf instead. +package sprintferrorsnew + +import ( + "go/ast" + + "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" +) + +// Analyzer is the sprintferrorsnew analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "sprintferrorsnew", + Doc: "reports errors.New(fmt.Sprintf(...)) calls that should use fmt.Errorf instead", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/sprintferrorsnew", + 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 + } + + if filecheck.IsTestFile(pass.Fset.Position(call.Pos()).Filename) { + return + } + + // Match errors.New(...) + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "New" { + return + } + if !astutil.IsPkgSelector(pass, sel, "errors") { + return + } + if len(call.Args) != 1 { + return + } + + // Check if the sole argument is a direct fmt.Sprintf(...) call. + argCall, ok := call.Args[0].(*ast.CallExpr) + if !ok { + return + } + argSel, ok := argCall.Fun.(*ast.SelectorExpr) + if !ok || argSel.Sel.Name != "Sprintf" { + return + } + if !astutil.IsPkgSelector(pass, argSel, "fmt") { + return + } + + pass.Reportf(call.Pos(), "use fmt.Errorf instead of errors.New(fmt.Sprintf(...))") + }) + + return nil, nil +} diff --git a/pkg/linters/sprintferrorsnew/sprintferrorsnew_test.go b/pkg/linters/sprintferrorsnew/sprintferrorsnew_test.go new file mode 100644 index 00000000000..4ee2542805c --- /dev/null +++ b/pkg/linters/sprintferrorsnew/sprintferrorsnew_test.go @@ -0,0 +1,17 @@ +//go:build !integration + +// Package sprintferrorsnew_test provides tests for the sprintferrorsnew analyzer. +package sprintferrorsnew_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/sprintferrorsnew" +) + +func TestSprintfErrorsNew(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, sprintferrorsnew.Analyzer, "sprintferrorsnew") +} diff --git a/pkg/linters/sprintferrorsnew/testdata/src/sprintferrorsnew/sprintferrorsnew.go b/pkg/linters/sprintferrorsnew/testdata/src/sprintferrorsnew/sprintferrorsnew.go new file mode 100644 index 00000000000..940deebe67c --- /dev/null +++ b/pkg/linters/sprintferrorsnew/testdata/src/sprintferrorsnew/sprintferrorsnew.go @@ -0,0 +1,33 @@ +// Package sprintferrorsnew is the test fixture for the sprintferrorsnew analyzer. +package sprintferrorsnew + +import ( + "errors" + "fmt" +) + +// bad demonstrates the flagged pattern: errors.New wrapping a fmt.Sprintf call. +func bad() error { + return errors.New(fmt.Sprintf("invalid engine: %s", "claude")) // want `use fmt\.Errorf instead of errors\.New\(fmt\.Sprintf\(\.\.\.\)\)` +} + +// badWithMultipleArgs demonstrates the flagged pattern with multiple format args. +func badWithMultipleArgs() error { + return errors.New(fmt.Sprintf("invalid value %q for flag %s", "x", "-n")) // want `use fmt\.Errorf instead of errors\.New\(fmt\.Sprintf\(\.\.\.\)\)` +} + +// goodErrorf uses fmt.Errorf directly — no diagnostic expected. +func goodErrorf() error { + return fmt.Errorf("invalid engine: %s", "claude") +} + +// goodVariable uses a pre-built string variable — not flagged by this linter. +func goodVariable() error { + msg := fmt.Sprintf("invalid engine: %s", "claude") + return errors.New(msg) +} + +// goodPlainString uses a string literal — no diagnostic expected. +func goodPlainString() error { + return errors.New("something went wrong") +}