diff --git a/docs/adr/49633-add-walkfuncerrshadow-linter-for-filepath-walk-err-shadow.md b/docs/adr/49633-add-walkfuncerrshadow-linter-for-filepath-walk-err-shadow.md new file mode 100644 index 00000000000..3312c8bd30f --- /dev/null +++ b/docs/adr/49633-add-walkfuncerrshadow-linter-for-filepath-walk-err-shadow.md @@ -0,0 +1,45 @@ +# ADR-49633: Add walkfuncerrshadow Linter for filepath.Walk/WalkDir err Parameter Shadowing + +**Date**: 2026-08-01 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +The `filepath.Walk` and `filepath.WalkDir` APIs take a callback whose third parameter is conventionally named `err`. When the outer variable receiving the walk's return value is also named `err`, the callback parameter silently shadows it. This is a narrow but recurring source of confusion during refactors — the inner `err` and the outer `err` are distinct variables, but their identical names make it easy to misread call sites and introduce bugs. The project already has a collection of custom `go/analysis` analyzers in `pkg/linters/` that target specific, high-signal patterns; this is another instance of the same category. + +### Decision + +We will add a new focused `go/analysis` analyzer (`walkfuncerrshadow`) in `pkg/linters/walkfuncerrshadow/` that flags exactly the case where a `filepath.Walk` or `filepath.WalkDir` call assigns its result to an outer variable named `err` and the callback's third parameter is also named `err`. The analyzer will be registered in the existing linter registry (`pkg/linters/registry.go`) so it runs through the project's standard linter driver. It will be narrowly scoped: it does not flag distinct outer names (e.g. `walkErr`), distinct callback parameter names, or non-`filepath` walkers. + +### Alternatives Considered + +#### Alternative 1: Rely on a Generic Shadow Checker (e.g., govet's `shadow` analyzer) + +Generic shadow analyzers like `go vet -shadow` or golangci-lint's `govet` shadow pass would catch this pattern but also flag every other variable shadowing in the codebase. This produces a high volume of diagnostics that are not all bugs, making it impractical to enable globally. The decision was to write a focused analyzer to achieve zero false positives for the common non-problematic cases (distinct names, non-`filepath` walkers). + +#### Alternative 2: Lint via a golangci-lint Plugin Wrapping an Existing Analyzer + +The project could integrate a third-party golangci-lint plugin for shadow detection. This adds an external dependency that must be vendored and kept compatible, and the detection scope is still broader than needed. The in-tree custom analyzer approach avoids new external dependencies and keeps the detection logic within the project's standard analysis framework, consistent with all other custom analyzers in `pkg/linters/`. + +### Consequences + +#### Positive +- Provides precise, zero-false-positive detection of the specific `filepath.Walk`/`WalkDir` err-shadowing pattern without noise from unrelated variable shadowing. +- Reduces the risk of a class of subtle bugs introduced during refactors, where developers misread `err` in the callback as the outer walk-result `err`. +- Integrates cleanly into the existing linter driver and registry with minimal surface area. +- Provides an escape hatch (`//nolint:walkfuncerrshadow`) for intentional cases. + +#### Negative +- Any existing code that uses the flagged pattern must be reviewed and potentially renamed; this is a one-time migration cost. +- The analyzer is intentionally narrow — it will not catch analogous patterns with `fs.WalkDir` or other walk-like APIs, so developers might not expect the asymmetry. + +#### Neutral +- Increases the active analyzer count from 60 to 61; documentation and spec tests are updated accordingly. +- The analyzer follows the exact same structural pattern as all other analyzers in `pkg/linters/`, requiring no new conventions. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/linters/README.md b/pkg/linters/README.md index 949c27a0938..6ee1ea8b056 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -132,6 +132,7 @@ This package currently provides custom Go analyzers in the following subpackages | `tolowerequalfold` | Custom `go/analysis` analyzer that flags case-insensitive comparisons via `strings.ToLower`/`ToUpper` that should use `strings.EqualFold` | | `trimleftright` | Custom `go/analysis` analyzer that flags `strings.TrimLeft`/`TrimRight` calls with a multi-character literal cutset where `TrimPrefix`/`TrimSuffix` was likely intended | | `uncheckedtypeassertion` | Custom `go/analysis` analyzer that flags unchecked single-value type assertions | +| `walkfuncerrshadow` | Custom `go/analysis` analyzer that flags `filepath.Walk`/`filepath.WalkDir` callbacks whose `err` parameter shadows an outer `err` variable assigned from the walk call | | `wgdonenotdeferred` | Custom `go/analysis` analyzer that flags non-deferred `sync.WaitGroup.Done()` calls | | `writebytestring` | Custom `go/analysis` analyzer that flags `w.Write([]byte(s))` calls where `s` is a string that can be replaced with `io.WriteString` | | `internal` | Shared helper subpackages used by analyzers (`internal/filecheck`, `internal/nolint`) | @@ -261,6 +262,7 @@ _ = trimleftright.Analyzer - `github.com/github/gh-aw/pkg/linters/tolowerequalfold` — to-lower-equal-fold analyzer subpackage - `github.com/github/gh-aw/pkg/linters/trimleftright` — trim-left-right analyzer subpackage - `github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion` — unchecked-type-assertion analyzer subpackage +- `github.com/github/gh-aw/pkg/linters/walkfuncerrshadow` — walk-func-err-shadow analyzer subpackage - `github.com/github/gh-aw/pkg/linters/wgdonenotdeferred` — wg-done-not-deferred analyzer subpackage - `github.com/github/gh-aw/pkg/linters/writebytestring` — write-byte-string analyzer subpackage diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 33576a41762..53ef264705b 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,6 +1,6 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// All 60 active analyzers: +// All 61 active analyzers: // // - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...) // - appendoneelement — flags append(s, []T{x}...) calls where a single-element slice literal is spread and can be simplified to append(s, x) @@ -60,6 +60,7 @@ // - tolowerequalfold — flags case-insensitive comparisons via ToLower/ToUpper that should use EqualFold // - trimleftright — flags strings.TrimLeft/TrimRight calls with a multi-character literal cutset where TrimPrefix/TrimSuffix was likely intended // - uncheckedtypeassertion — flags unchecked single-value type assertions +// - walkfuncerrshadow — flags filepath.Walk/WalkDir callbacks whose err parameter shadows an outer err variable assigned from the walk call // - wgdonenotdeferred — flags non-deferred sync.WaitGroup.Done() calls // - writebytestring — flags w.Write([]byte(s)) calls where s is a string that can be replaced with io.WriteString(w, s) // diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index 5406f94494c..f20b15e227d 100644 --- a/pkg/linters/registry.go +++ b/pkg/linters/registry.go @@ -61,6 +61,7 @@ import ( "github.com/github/gh-aw/pkg/linters/tolowerequalfold" "github.com/github/gh-aw/pkg/linters/trimleftright" "github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion" + "github.com/github/gh-aw/pkg/linters/walkfuncerrshadow" "github.com/github/gh-aw/pkg/linters/wgdonenotdeferred" "github.com/github/gh-aw/pkg/linters/writebytestring" ) @@ -133,6 +134,7 @@ var allAnalyzers = []*analysis.Analyzer{ tolowerequalfold.Analyzer, trimleftright.Analyzer, uncheckedtypeassertion.Analyzer, + walkfuncerrshadow.Analyzer, wgdonenotdeferred.Analyzer, writebytestring.Analyzer, } diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 6fb3590b4fd..ccac35b7ad2 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -69,6 +69,7 @@ import ( "github.com/github/gh-aw/pkg/linters/tolowerequalfold" "github.com/github/gh-aw/pkg/linters/trimleftright" "github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion" + "github.com/github/gh-aw/pkg/linters/walkfuncerrshadow" "github.com/github/gh-aw/pkg/linters/wgdonenotdeferred" "github.com/github/gh-aw/pkg/linters/writebytestring" ) @@ -86,7 +87,7 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 60 analyzers +// "Public API > Subpackages" table. The README documents 61 analyzers // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // @@ -98,7 +99,7 @@ type docAnalyzer struct { // logfatallibrary, manualmutexunlock, mapclearloop, mapdeletecheck, nilctxpassed, osexitinlibrary, osgetenvlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib, // regexpcompileinfunction, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, sprintfbool, sprintfint, ssljson, // strconvparseignorederror, stringbytesroundtrip, stringreplaceminusone, stringsconcatloop, stringscountcontains, stringsindexcontains, stringsindexhasprefix, stringsjoinone, timeafterleak, timesleepnocontext, timenowsub, -// tolowerequalfold, trimleftright, uncheckedtypeassertion, wgdonenotdeferred, writebytestring +// tolowerequalfold, trimleftright, uncheckedtypeassertion, walkfuncerrshadow, wgdonenotdeferred, writebytestring func documentedAnalyzers() []docAnalyzer { return []docAnalyzer{ {"appendbytestring", appendbytestring.Analyzer}, @@ -159,6 +160,7 @@ func documentedAnalyzers() []docAnalyzer { {"tolowerequalfold", tolowerequalfold.Analyzer}, {"trimleftright", trimleftright.Analyzer}, {"uncheckedtypeassertion", uncheckedtypeassertion.Analyzer}, + {"walkfuncerrshadow", walkfuncerrshadow.Analyzer}, {"wgdonenotdeferred", wgdonenotdeferred.Analyzer}, {"writebytestring", writebytestring.Analyzer}, } diff --git a/pkg/linters/walkfuncerrshadow/testdata/src/walkfuncerrshadow/walkfuncerrshadow.go b/pkg/linters/walkfuncerrshadow/testdata/src/walkfuncerrshadow/walkfuncerrshadow.go new file mode 100644 index 00000000000..6391355a66b --- /dev/null +++ b/pkg/linters/walkfuncerrshadow/testdata/src/walkfuncerrshadow/walkfuncerrshadow.go @@ -0,0 +1,94 @@ +package walkfuncerrshadow + +import ( + "io/fs" + "os" + "path/filepath" +) + +func BadWalk(root string) error { + err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { // want `callback parameter err shadows outer err assigned from filepath\.Walk; rename the callback parameter \(for example walkErr\)` + if err != nil { + return err + } + return nil + }) + return err +} + +func BadWalkDir(root string) error { + var err error + err = filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { // want `callback parameter err shadows outer err assigned from filepath\.WalkDir; rename the callback parameter \(for example walkErr\)` + if err != nil { + return err + } + return nil + }) + return err +} + +func BadWalkDirShort(root string) error { + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { // want `callback parameter err shadows outer err assigned from filepath\.WalkDir; rename the callback parameter \(for example walkErr\)` + if err != nil { + return err + } + return nil + }) + return err +} + +func GoodDistinctParam(root string) error { + err := filepath.Walk(root, func(path string, info os.FileInfo, walkErr error) error { + if walkErr != nil { + return walkErr + } + return nil + }) + return err +} + +func GoodDistinctOuter(root string) error { + walkErr := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + return nil + }) + return walkErr +} + +func GoodOtherWalker(root string) error { + err := fs.WalkDir(os.DirFS(root), ".", func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + return nil + }) + return err +} + +func GoodNolint(root string) error { + //nolint:walkfuncerrshadow + err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + return nil + }) + return err +} + +func GoodNolintMultiLine(root string) error { + //nolint:walkfuncerrshadow + err := filepath.Walk(root, func( + path string, + info os.FileInfo, + err error, + ) error { + if err != nil { + return err + } + return nil + }) + return err +} diff --git a/pkg/linters/walkfuncerrshadow/walkfuncerrshadow.go b/pkg/linters/walkfuncerrshadow/walkfuncerrshadow.go new file mode 100644 index 00000000000..27c81733b40 --- /dev/null +++ b/pkg/linters/walkfuncerrshadow/walkfuncerrshadow.go @@ -0,0 +1,126 @@ +// Package walkfuncerrshadow implements a Go analysis linter that flags +// filepath.Walk and filepath.WalkDir callbacks whose err parameter shadows an +// outer err variable assigned from the walk call itself. +package walkfuncerrshadow + +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" + "github.com/github/gh-aw/pkg/linters/internal/nolint" + "github.com/github/gh-aw/pkg/logger" +) + +var pkgLog = logger.New("linters:walkfuncerrshadow") + +const analyzerName = "walkfuncerrshadow" + +// Analyzer is the walkfuncerrshadow analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: analyzerName, + Doc: "reports filepath.Walk/WalkDir callbacks whose err parameter shadows an outer err variable assigned from the walk call", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/walkfuncerrshadow", + Requires: []*analysis.Analyzer{inspect.Analyzer, nolint.Analyzer, filecheck.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + pkgLog.Printf("analyzing package %s", pass.Pkg.Path()) + + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err + } + noLintIndex, err := nolint.Index(pass) + if err != nil { + return nil, err + } + generatedFiles, err := filecheck.Index(pass) + if err != nil { + return nil, err + } + + insp.Preorder([]ast.Node{(*ast.AssignStmt)(nil)}, func(n ast.Node) { + assign, ok := n.(*ast.AssignStmt) + if !ok { + return + } + checkAssign(pass, assign, generatedFiles, noLintIndex) + }) + + return nil, nil +} + +func checkAssign(pass *analysis.Pass, assign *ast.AssignStmt, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + if len(assign.Lhs) != 1 || len(assign.Rhs) != 1 { + return + } + + outerErr, ok := assign.Lhs[0].(*ast.Ident) + if !ok || outerErr.Name != "err" { + return + } + + call, ok := astutil.UnwrapParenExpr(assign.Rhs[0]).(*ast.CallExpr) + if !ok { + return + } + sel, ok := astutil.UnwrapParenExpr(call.Fun).(*ast.SelectorExpr) + if !ok || !astutil.IsPkgSelector(pass, sel, "path/filepath") { + return + } + if sel.Sel.Name != "Walk" && sel.Sel.Name != "WalkDir" { + return + } + if len(call.Args) < 2 { + return + } + + callback, ok := astutil.UnwrapParenExpr(call.Args[1]).(*ast.FuncLit) + if !ok { + return + } + callbackErr := callbackErrParam(pass, callback) + if callbackErr == nil { + return + } + + assignPos := pass.Fset.PositionFor(assign.Pos(), false) + if filecheck.ShouldSkipFilename(assignPos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(assignPos, noLintIndex, analyzerName) { + return + } + + pkgLog.Printf("flagging filepath.%s callback err shadow at %s", sel.Sel.Name, assignPos) + pass.ReportRangef( + callbackErr, + "callback parameter err shadows outer err assigned from filepath.%s; rename the callback parameter (for example walkErr)", + sel.Sel.Name, + ) +} + +func callbackErrParam(pass *analysis.Pass, callback *ast.FuncLit) *ast.Ident { + if callback == nil || callback.Type == nil || callback.Type.Params == nil { + return nil + } + params := callback.Type.Params.List + // checkAssign narrows to filepath.Walk / WalkDir callback literals, both of + // which require exactly three parameters. + if len(params) != 3 { + return nil + } + last := params[2] + if len(last.Names) != 1 || last.Names[0].Name != "err" { + return nil + } + if pass.TypesInfo == nil || !nolint.ImplementsError(pass.TypesInfo.TypeOf(last.Type)) { + return nil + } + return last.Names[0] +} diff --git a/pkg/linters/walkfuncerrshadow/walkfuncerrshadow_test.go b/pkg/linters/walkfuncerrshadow/walkfuncerrshadow_test.go new file mode 100644 index 00000000000..a1d30a22627 --- /dev/null +++ b/pkg/linters/walkfuncerrshadow/walkfuncerrshadow_test.go @@ -0,0 +1,15 @@ +//go:build !integration + +package walkfuncerrshadow_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/walkfuncerrshadow" +) + +func TestWalkFuncErrShadow(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), walkfuncerrshadow.Analyzer, "walkfuncerrshadow") +}