feat(linters): add deferinloop analyzer — flags defer inside for-loop bodies

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,

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.*

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 |

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
+}

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")
+}

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()
+	}
+}

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

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},