diff --git a/cmd/linters/main.go b/cmd/linters/main.go index b47cf092bae..586a0c4f0f8 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -18,6 +18,7 @@ import ( "github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred" "github.com/github/gh-aw/pkg/linters/ctxbackground" + "github.com/github/gh-aw/pkg/linters/deferinloop" "github.com/github/gh-aw/pkg/linters/errorfwrapv" "github.com/github/gh-aw/pkg/linters/errormessage" "github.com/github/gh-aw/pkg/linters/errstringmatch" @@ -53,6 +54,7 @@ func main() { multichecker.Main( contextcancelnotdeferred.Analyzer, ctxbackground.Analyzer, + deferinloop.Analyzer, errormessage.Analyzer, fprintlnsprintf.Analyzer, errstringmatch.Analyzer, diff --git a/docs/adr/40679-add-deferinloop-linter.md b/docs/adr/40679-add-deferinloop-linter.md new file mode 100644 index 00000000000..ac9d7d44e91 --- /dev/null +++ b/docs/adr/40679-add-deferinloop-linter.md @@ -0,0 +1,39 @@ +# ADR-40679: Add `deferinloop` linter for `defer` inside `for`/`range` loop bodies + +**Date**: 2026-06-21 +**Status**: Draft + +## Context + +The codebase maintains a custom `go/analysis` linter suite (`pkg/linters/*`, registered in `cmd/linters/main.go`) to enforce house style and catch anti-patterns that off-the-shelf golangci-lint rules miss. A `defer` placed directly inside a `for` or `range` loop body does not run at the end of each iteration — it runs only when the enclosing function returns, which can leak resources (file handles, connections) and produce surprising LIFO cleanup ordering. The upstream `gocritic` rule that covers this is disabled because of golangci-lint v2 bugs, leaving the pattern undetected. 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/deferinloop` and register it in `cmd/linters/main.go`. The analyzer walks each `*ast.DeferStmt` via `inspector.Cursor.Enclosing`, flagging the statement when its nearest relevant ancestor is a `ForStmt` or `RangeStmt`, and exempting it when a `FuncLit` is encountered first (a function literal forms a new scope, so its `defer` runs on the literal's return). Test files are skipped via `filecheck.IsTestFile`, and `//nolint:deferinloop` directives are honored via the shared `nolint` helper. + +## Alternatives Considered + +### Alternative 1: Re-enable `gocritic` / use an off-the-shelf linter +The `gocritic` rule that flags this pattern is disabled because of golangci-lint v2 integration bugs. Re-enabling it would reintroduce those failures and pull in unrelated checks, while waiting on an upstream fix is slow and out of our control. Rejected in favor of a focused local analyzer that fits the existing `pkg/linters` framework. + +### Alternative 2: Flag every `defer` inside a loop, including those in function literals +The analyzer could report any `defer` lexically nested under a loop regardless of intervening `FuncLit` boundaries. This would misreport the common and correct closure idiom (`for ... { func() { defer ... }() }`) used to scope per-iteration cleanup, producing false positives. Rejected; the `FuncLit` boundary check preserves a high signal-to-noise ratio. + +## Consequences + +### Positive +- Catches a real resource-leak anti-pattern that no currently enabled linter detects, restoring coverage lost when `gocritic` was disabled. +- Correctly exempts the function-literal idiom, keeping false positives low. +- Follows the established analyzer pattern, so it is consistent with the rest of the suite and covered by `analysistest`-based tests (range, classic, infinite, and nested loops; closure, explicit-close, and nolint exemptions). + +### Negative +- Adds another analyzer to maintain and run, marginally increasing lint time and the registration surface in `cmd/linters/main.go`. +- Only direct lexical nesting is analyzed; a `defer` whose loop association is obscured through indirection is not in scope. + +### Neutral +- The rule is purely diagnostic (`pass.ReportRangef`); it offers no automated fix/suggested edit. +- Authors who intentionally want a function-scoped `defer` inside a loop can suppress with `//nolint:deferinloop`. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27915387610) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/linters b/linters index ec4b5b3b381..bb66bb9692d 100755 Binary files a/linters and b/linters differ diff --git a/pkg/linters/README.md b/pkg/linters/README.md index ef563841a15..405eda96b4d 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -8,6 +8,7 @@ This package currently provides custom Go analyzers in the following subpackages - `contextcancelnotdeferred` — reports context cancel functions that are called directly instead of deferred. - `ctxbackground` — reports `context.Background()` calls inside functions that already receive a `context.Context` parameter. +- `deferinloop` — reports `defer` statements placed directly inside `for`/`range` loop bodies, which execute when the enclosing function returns rather than each iteration and can cause resource leaks. - `errorfwrapv` — reports `fmt.Errorf` calls that format error arguments with `%v` instead of `%w`. - `excessivefuncparams` — reports function declarations that exceed a configurable parameter-count threshold. - `errormessage` — reports non-actionable error-message patterns in changed files. @@ -45,6 +46,7 @@ This package currently provides custom Go analyzers in the following subpackages |------------|-------------| | `contextcancelnotdeferred` | Custom `go/analysis` analyzer that flags context cancel functions called directly instead of deferred | | `ctxbackground` | Custom `go/analysis` analyzer that flags `context.Background()` calls inside functions that already receive a context parameter | +| `deferinloop` | Custom `go/analysis` analyzer that flags `defer` statements inside `for`/`range` loop bodies that execute when the enclosing function returns rather than each iteration | | `errorfwrapv` | Custom `go/analysis` analyzer that flags `fmt.Errorf` calls that format error arguments with `%v` instead of `%w` | | `excessivefuncparams` | Custom `go/analysis` analyzer that flags function declarations with too many positional parameters | | `errormessage` | Custom `go/analysis` analyzer that flags non-actionable error message patterns in changed files | diff --git a/pkg/linters/deferinloop/deferinloop.go b/pkg/linters/deferinloop/deferinloop.go new file mode 100644 index 00000000000..d88a71c0490 --- /dev/null +++ b/pkg/linters/deferinloop/deferinloop.go @@ -0,0 +1,79 @@ +// Package deferinloop implements a Go analysis linter that flags defer +// statements placed directly inside for or range loop bodies. A defer inside +// a loop does not execute at the end of each iteration — it runs when the +// enclosing function returns, which can cause resource leaks and unexpected +// cleanup ordering. +package deferinloop + +import ( + "go/ast" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + + "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" +) + +// Analyzer is the defer-in-loop analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "deferinloop", + Doc: "reports defer statements enclosed anywhere within a for or range loop body; a function literal between a defer and an enclosing loop is treated as a new scope boundary, making the defer exempt; test files are not checked", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/deferinloop", + 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 + } + noLintLinesByFile := nolint.BuildLineIndex(pass, "deferinloop") + + for cur := range insp.Root().Preorder((*ast.DeferStmt)(nil)) { + deferStmt, ok := cur.Node().(*ast.DeferStmt) + if !ok { + continue + } + + pos := pass.Fset.PositionFor(deferStmt.Pos(), false) + if filecheck.IsTestFile(pos.Filename) { + continue + } + if nolint.HasDirective(pos, noLintLinesByFile) { + continue + } + + if !isInsideLoop(cur) { + continue + } + + pass.ReportRangef(deferStmt, + "defer inside a loop does not execute at the end of each iteration; it runs when the enclosing function returns, which can cause resource leaks") + } + + return nil, nil +} + +// isInsideLoop reports whether cur (a DeferStmt) is enclosed anywhere within a +// for or range loop body, without crossing a function literal boundary. +// Defers inside func literals are exempt because they form a new function scope +// and execute when the literal returns, not the outer function. +func isInsideLoop(cur inspector.Cursor) bool { + for encl := range cur.Enclosing( + (*ast.ForStmt)(nil), + (*ast.RangeStmt)(nil), + (*ast.FuncLit)(nil), + ) { + switch encl.Node().(type) { + case *ast.ForStmt, *ast.RangeStmt: + return true + case *ast.FuncLit: + return false + } + } + return false +} diff --git a/pkg/linters/deferinloop/deferinloop_test.go b/pkg/linters/deferinloop/deferinloop_test.go new file mode 100644 index 00000000000..aecf3af00d5 --- /dev/null +++ b/pkg/linters/deferinloop/deferinloop_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package deferinloop_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/deferinloop" +) + +func TestAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, deferinloop.Analyzer, "deferinloop") +} diff --git a/pkg/linters/deferinloop/testdata/src/deferinloop/deferinloop.go b/pkg/linters/deferinloop/testdata/src/deferinloop/deferinloop.go new file mode 100644 index 00000000000..196dd1273c9 --- /dev/null +++ b/pkg/linters/deferinloop/testdata/src/deferinloop/deferinloop.go @@ -0,0 +1,110 @@ +package deferinloop + +import "os" + +// BadForRange flags defer inside a range loop — resource leak. +func BadForRange(paths []string) { + for _, p := range paths { + f, _ := os.Open(p) + defer f.Close() // want `defer inside a loop does not execute at the end of each iteration; it runs when the enclosing function returns, which can cause resource leaks` + } +} + +// BadForStmt flags defer inside a classic for loop. +func BadForStmt(paths []string) { + for i := 0; i < len(paths); i++ { + f, _ := os.Open(paths[i]) + defer f.Close() // want `defer inside a loop does not execute at the end of each iteration; it runs when the enclosing function returns, which can cause resource leaks` + } +} + +// BadForever flags defer inside an infinite for loop. +func BadForever() { + for { + f, _ := os.Open("file") + defer f.Close() // want `defer inside a loop does not execute at the end of each iteration; it runs when the enclosing function returns, which can cause resource leaks` + break + } +} + +// BadNestedLoop flags defer inside a nested loop. +func BadNestedLoop(matrix [][]string) { + for _, row := range matrix { + for _, p := range row { + f, _ := os.Open(p) + defer f.Close() // want `defer inside a loop does not execute at the end of each iteration; it runs when the enclosing function returns, which can cause resource leaks` + } + } +} + +// GoodExplicitClose is fine — explicit close each iteration. +func GoodExplicitClose(paths []string) { + for _, p := range paths { + f, _ := os.Open(p) + f.Close() + } +} + +// BadIfInLoop flags defer inside an if block within a for loop. +func BadIfInLoop(paths []string, cond bool) { + for _, p := range paths { + if cond { + f, _ := os.Open(p) + defer f.Close() // want `defer inside a loop does not execute at the end of each iteration; it runs when the enclosing function returns, which can cause resource leaks` + } + } +} + +// BadSelectInLoop flags defer inside a select inside a for loop. +func BadSelectInLoop(ch <-chan string) { + for { + select { + case p := <-ch: + f, _ := os.Open(p) + defer f.Close() // want `defer inside a loop does not execute at the end of each iteration; it runs when the enclosing function returns, which can cause resource leaks` + } + } +} + +// GoodFuncLitInsideLoop is fine — defer is inside a closure (new scope). +func GoodFuncLitInsideLoop(paths []string) { + for _, p := range paths { + func() { + f, _ := os.Open(p) + defer f.Close() // FuncLit boundary — not flagged + }() + } +} + +// GoodGoFuncLitInsideLoop is fine — goroutine func literal also forms a new scope. +func GoodGoFuncLitInsideLoop(paths []string) { + for _, p := range paths { + go func() { + f, _ := os.Open(p) + defer f.Close() // FuncLit boundary — not flagged + }() + } +} + +// GoodDeferOutsideLoop is fine — defer is not inside a loop. +func GoodDeferOutsideLoop() { + f, _ := os.Open("file") + defer f.Close() +} + +// GoodNolintSameLine: suppressed with a nolint directive on the same line. +func GoodNolintSameLine(paths []string) { + for _, p := range paths { + f, _ := os.Open(p) + defer f.Close() //nolint:deferinloop + } +} + +// GoodNolintPreviousLine: suppressed with a nolint directive on the previous line. +func GoodNolintPreviousLine(paths []string) { + for _, p := range paths { + f, _ := os.Open(p) + //nolint:deferinloop + defer f.Close() + } +} diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 0bab5358155..fdce0e97c6e 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,9 +1,10 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// The actual analyzers are implemented in subpackages. All 29 active analyzers: +// The actual analyzers are implemented in subpackages. All 30 active analyzers: // // - contextcancelnotdeferred — flags context cancel functions called directly instead of deferred // - ctxbackground — flags context.Background() inside functions that already receive a context +// - deferinloop — flags defer statements placed directly inside for or range loop bodies // - errorfwrapv — flags fmt.Errorf calls that format error arguments with %v instead of %w // - errormessage — flags non-actionable error message patterns in changed files // - errstringmatch — flags brittle strings.Contains(err.Error(), "...") checks diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index f53401eb72d..284e7171e6b 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -13,6 +13,7 @@ import ( "github.com/github/gh-aw/pkg/linters" "github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred" "github.com/github/gh-aw/pkg/linters/ctxbackground" + "github.com/github/gh-aw/pkg/linters/deferinloop" "github.com/github/gh-aw/pkg/linters/errorfwrapv" "github.com/github/gh-aw/pkg/linters/errormessage" "github.com/github/gh-aw/pkg/linters/errstringmatch" @@ -55,13 +56,13 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 29 analyzer +// "Public API > Subpackages" table. The README documents 30 analyzer // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // // Spec (README "Public API > Subpackages"): // -// contextcancelnotdeferred, ctxbackground, errorfwrapv, excessivefuncparams, errormessage, +// contextcancelnotdeferred, ctxbackground, deferinloop, errorfwrapv, excessivefuncparams, errormessage, // errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, // hardcodedfilepath, httpnoctx, jsonmarshalignoredeerror, largefunc, lenstringzero, // manualmutexunlock, osexitinlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib, @@ -71,6 +72,7 @@ func documentedAnalyzers() []docAnalyzer { return []docAnalyzer{ {"contextcancelnotdeferred", contextcancelnotdeferred.Analyzer}, {"ctxbackground", ctxbackground.Analyzer}, + {"deferinloop", deferinloop.Analyzer}, {"errorfwrapv", errorfwrapv.Analyzer}, {"excessivefuncparams", excessivefuncparams.Analyzer}, {"errormessage", errormessage.Analyzer},