Add walkfuncerrshadow analyzer for filepath.Walk err shadowing #49633
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.* |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 { | ||
|
Contributor
There was a problem hiding this comment. Testdata has no multi-line callback nolint suppression test. The existing Please add a test case like: func GoodNolintMultiLine(root string) error {
(nolint/redacted):walkfuncerrshadow
err := filepath.Walk(root, func(
path string,
info os.FileInfo,
err error,
) error {
return err
})
return err
}This would expose the position-anchor bug described in the implementation comment. @copilot please address this. |
||
| 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 | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] The guard 💡 Suggested fixSearch all LHS identifiers for one named var outerErr *ast.Ident
for _, lhs := range assign.Lhs {
if id, ok := lhs.(*ast.Ident); ok && id.Name == "err" {
outerErr = id
break
}
}
if outerErr == nil {
return
}Add a corresponding test fixture covering @copilot please address this. |
||
| } | ||
| 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 | ||
| } | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The 💡 DetailsThe nolint directive is conventionally placed on the line that triggers the diagnostic — which a user would naturally place on the Add a test fixture with the directive on the assignment line (not the callback) to confirm it either works or fails intentionally. @copilot please address this. |
||
|
|
||
| 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, | ||
| ) | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. [/tdd] The type check in the outer @copilot please address this. |
||
| 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] | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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") | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The
BadWalkDirfixture usesvar err error; err = ...(an assign statement=) rather than the short-variable-declarationerr := ...used inBadWalk. This is a good additional shape to cover, but there's no fixture testingerr := filepath.WalkDir(...)with:=. Add one to make coverage symmetric.@copilot please address this.