feat(linters): add deferinloop analyzer — flags defer inside for-loop bodies#40679
Conversation
… bodies Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom go/analysis analyzer (deferinloop) to the gh-aw linter suite to detect defer statements inside for/range loop bodies (a common source of resource leaks and unintended cleanup timing), along with analysistest coverage, registration in the multichecker entrypoint, and README documentation.
Changes:
- Added
pkg/linters/deferinloopanalyzer implementation that walksDeferStmtnodes and detects enclosingfor/rangeloops while treating function literals as a scope boundary. - Added analysistest-based tests and
testdata/cases covering loop forms, nesting, closure exemptions, and//nolint:deferinloop. - Registered the analyzer in
cmd/linters/main.goand documented it inpkg/linters/README.md.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/deferinloop/deferinloop.go | New analyzer that reports defer in loop bodies and honors nolint + FuncLit scope boundaries |
| pkg/linters/deferinloop/deferinloop_test.go | analysistest runner for the new analyzer |
| pkg/linters/deferinloop/testdata/src/deferinloop/deferinloop.go | Positive/negative test cases for loop usage, closures, and nolint |
| cmd/linters/main.go | Registers deferinloop.Analyzer in the multichecker |
| pkg/linters/README.md | Documents the new linter in the package overview + table |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/6 changed files
- Comments generated: 2
| // Analyzer is the defer-in-loop analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "deferinloop", | ||
| Doc: "reports defer statements placed directly inside for or range loop bodies; defers inside function literals are exempt because they form a new function scope", |
There was a problem hiding this comment.
Fixed in the latest commit. The Doc string now reads: "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" — no longer implies all defers inside function literals are exempt.
| - `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`. |
There was a problem hiding this comment.
Done — pkg/linters/doc.go updated (count 29→30, deferinloop added alphabetically) and pkg/linters/spec_test.go updated with the import and entry in documentedAnalyzers().
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( Note: The testdata file covers 9 scenarios across 4 bad-case categories (range loop, classic Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — commenting on doc-string accuracy and test coverage gaps. No blocking logic bugs found.
📋 Key Themes & Highlights
Key Themes
-
Doc inaccuracy (
/grill-with-docs): BothAnalyzer.Doc(line 23) and theisInsideLoopcomment (line 61) use the word "directly" — implying the linter only catches defer as an immediate child of a loop body. The implementation correctly catches defer at any depth (insideif/switch/selectwithin a loop), so the docs should say so. -
Test coverage gaps (
/tdd): The testdata covers the six core cases well, but is missing the most prevalent real-world patterns:deferinside aselectcase inside aforloop (ubiquitous in channel-consuming goroutines)deferinside anifblock inside a loopgo func() { defer ... }()inside a loop (goroutine FuncLit — should be exempt)
Compare withtimeafterleak's testdata (163 lines, 13 functions) for a fuller reference.
-
// wantregex precision (/tdd): Backtick-quoted strings inanalysistestare Go regexps; the unescaped.chars are wildcards. Harmless in practice but imprecise.
Positive Highlights
- ✅ Core
isInsideLooplogic is correct:inspector.Cursor.Enclosingwith innermost-first ordering plusFuncLitshort-circuit is the right approach - ✅ Shared
nolintpackage wired up consistently with same-line and previous-line directives - ✅ Registration in
cmd/linters/main.gois clean and follows existing alphabetical order - ✅ PR description clearly motivates why a custom analyzer is needed (gocritic disabled under golangci-lint v2)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 86.8 AIC · ⌖ 7.89 AIC · ⊞ 6.9K
| // Analyzer is the defer-in-loop analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "deferinloop", | ||
| Doc: "reports defer statements placed directly inside for or range loop bodies; defers inside function literals are exempt because they form a new function scope", |
There was a problem hiding this comment.
[/grill-with-docs] The Doc string says "placed directly inside for or range loop bodies" — but the linter catches defer at any depth within a loop body (e.g. inside a nested if, switch, or select).\n\n
💡 Suggested fix
\n\nThe word "directly" implies only immediate children of the loop body, which is inaccurate. A user who writesdefer inside an if within a loop would rightfully expect it to be flagged — and it is — but the doc implies otherwise:\n\ngo\nDoc: "reports defer statements appearing anywhere inside for or range loop bodies, except inside function literals which form a new scope",\n\n\nSame inaccuracy appears in the isInsideLoop doc comment on line 61.\nThere was a problem hiding this comment.
Fixed — Analyzer.Doc now reads "enclosed anywhere within a for or range loop body" rather than "placed directly inside".
| } | ||
|
|
||
| // isInsideLoop reports whether cur (a DeferStmt) is directly enclosed by a | ||
| // for or range loop body, without crossing a function literal boundary. |
There was a problem hiding this comment.
[/grill-with-docs] "Directly enclosed by a for or range loop body" is inaccurate — the function actually flags defer at any ancestor depth before a FuncLit boundary (e.g. a defer inside if { switch { defer } } inside a for is still caught). The word "directly" implies only immediate children.\n\n
💡 Suggested fix
\n\ngo\n// isInsideLoop reports whether cur (a DeferStmt) has a for or range loop\n// anywhere in its ancestor chain before crossing a function literal boundary.\n// Defers inside func literals are exempt because they form a new function scope\n// and execute when the literal returns, not the outer function.\n\nThere was a problem hiding this comment.
Fixed — isInsideLoop comment updated to "enclosed anywhere within a for or range loop body, without crossing a function literal boundary".
| //nolint:deferinloop | ||
| defer f.Close() | ||
| } | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing test cases for defer inside control-flow structures nested within a loop. The most common real-world form is for { select { case <-ch: defer cleanup() } }, but if and switch inside a loop are equally unexercised. Compare with timeafterleak's testdata (163 lines, 13 functions) which is more exhaustive.\n\n
💡 Suggested additions
\n\nAdd these exempt/flagged cases to verify the ancestor-walk is correct regardless of intermediate AST nodes:\n\ngo\n// BadSelectInLoop flags defer inside a select inside a for loop.\nfunc BadSelectInLoop(ch <-chan string) {\n\tfor {\n\t\tselect {\n\t\tcase p := <-ch:\n\t\t\tf, _ := os.Open(p)\n\t\t\tdefer f.Close() // want `defer inside a loop...`\n\t\t}\n\t}\n}\n\n// BadIfInLoop flags defer inside an if block inside a for loop.\nfunc BadIfInLoop(paths []string, cond bool) {\n\tfor _, p := range paths {\n\t\tif cond {\n\t\t\tf, _ := os.Open(p)\n\t\t\tdefer f.Close() // want `defer inside a loop...`\n\t\t}\n\t}\n}\n\n// GoodGoFuncLitInsideLoop is fine — goroutine func literal forms a new scope.\nfunc GoodGoFuncLitInsideLoop(paths []string) {\n\tfor _, p := range paths {\n\t\tgo func() {\n\t\t\tf, _ := os.Open(p)\n\t\t\tdefer f.Close()\n\t\t}()\n\t}\n}\n\n\nThe implementation should handle all these correctly (the ancestor walk skips non-filter nodes), but without tests a future refactor has no safety net.\nThere was a problem hiding this comment.
Added BadIfInLoop, BadSelectInLoop, and GoodGoFuncLitInsideLoop test cases covering those patterns.
| 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` |
There was a problem hiding this comment.
[/tdd] The // want backtick-quoted string is treated as a Go regexp by analysistest. The ." characters in "iteration" and "leaks" are wildcard metacharacters (match any byte), so the regex is slightly more permissive than intended.\n\n<details>\n<summary>💡 Suggested fix</summary>\n\nEscape literal dots with \.":\n\ngo\ndefer 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`\n// becomes:\ndefer 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`\n\n\nIn practice this is harmless (the actual message always matches), but precise regexps make the test intent clearer. This pattern repeats on lines 17, 25, and 35.\n
There was a problem hiding this comment.
The diagnostic message contains no literal dots, so there is nothing to escape. The // want patterns are already exact matches for the message text.
|
@copilot Run pr-finisher skill |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR adds 176 new lines in business-logic directories (a new custom 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. A future contributor reading 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in 🔒 This PR cannot merge until an ADR is linked in the PR body.
|
There was a problem hiding this comment.
The implementation is correct and well-structured. isInsideLoop properly uses cur.Enclosing(...) to stop at FuncLit boundaries, the nolint handling is sound, and the test coverage is reasonable for the documented patterns.
Two non-blocking observations
-
Analyzer.Docomits the test-file exemption —_test.gofiles are silently skipped (consistent with other linters in this package), but the doc string doesn't say so.gocritic's equivalent check doesn't skip test files, so users may be surprised. -
Missing testdata case for defer inside a conditional within a loop —
if cond { defer f.Close() }inside aforloop is the most common real-world trigger for this warning. The logic handles it correctly, but there's no test to prevent a future regression ifisInsideLoopis refactored.
🔎 Code quality review by PR Code Quality Reviewer · 163.7 AIC · ⌖ 7.51 AIC · ⊞ 5.1K
| // Analyzer is the defer-in-loop analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "deferinloop", | ||
| Doc: "reports defer statements placed directly inside for or range loop bodies; defers inside function literals are exempt because they form a new function scope", |
There was a problem hiding this comment.
Analyzer.Doc omits the test-file exemption, which will surprise users when _test.go defers-in-loops are silently not flagged.
💡 Suggested fix
Extend the doc string to document the exemption:
Doc: "reports defer statements placed directly inside for/range loop bodies; defers inside function literals and _test.go files are exempt",gocritic's equivalent check (deferInLoop) flags test files. This analyzer silently diverges from that behavior. A user who wonders why their benchmark or test helper isn't flagged has no indication from the doc string that test files are excluded.
There was a problem hiding this comment.
Added "test files are not checked" to the Analyzer.Doc string to document this exemption.
| //nolint:deferinloop | ||
| defer f.Close() | ||
| } | ||
| } |
There was a problem hiding this comment.
No test case for defer nested inside a conditional (if/switch/select) within a loop — the most common real-world pattern that triggers this lint warning.
💡 Suggested addition
// BadDeferInIfInsideLoop flags defer inside an if block within a loop.
func BadDeferInIfInsideLoop(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`
}
}
}The current implementation is correct — cur.Enclosing(...) traverses all ancestors, so the ForStmt/RangeStmt is found regardless of how many IfStmt/SwitchStmt/SelectStmt nodes are between the defer and the loop. However, without an explicit test for this pattern, a future refactor that naively checks only the direct parent would regress silently. Adding this case closes that gap.
There was a problem hiding this comment.
Added BadIfInLoop (defer inside if inside loop) and BadSelectInLoop (defer inside select inside loop) test cases.
| // Analyzer is the defer-in-loop analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "deferinloop", | ||
| Doc: "reports defer statements placed directly inside for or range loop bodies; defers inside function literals are exempt because they form a new function scope", |
There was a problem hiding this comment.
Analyzer.Doc says "directly inside" but the linter flags defers at any nesting depth within the loop — this is a meaningful documentation inaccuracy.
💡 Suggested fix
The current doc reads:
"reports defer statements placed directly inside for or range loop bodies"
isInsideLoop uses cur.Enclosing(...) which skips all intermediate nodes (IfStmt, SwitchStmt, SelectStmt, BlockStmt). A defer inside for { if cond { defer f() } } is flagged even though it is not a direct child of the loop body.
Change to:
Doc: "reports defer statements enclosed anywhere within a for or range loop body without a function literal boundary; defers inside function literals are exempt because they form a new function scope",Users who read "directly inside" and wonder why their for { switch { case: defer } } is flagged will be confused. The behavior is correct; the description is not.
There was a problem hiding this comment.
Fixed — Analyzer.Doc now reads "enclosed anywhere within" instead of "placed directly inside".
| // isInsideLoop reports whether cur (a DeferStmt) is directly enclosed by 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. |
There was a problem hiding this comment.
isInsideLoop doc comment says "directly enclosed by" — same imprecision as Analyzer.Doc, misleading for future maintainers.
💡 Suggested fix
Current comment:
isInsideLoop reports whether cur (a DeferStmt) is **directly enclosed by** a for or range loop body
A maintainer who reads "directly enclosed" might refactor Enclosing(...) to only check the immediate parent node, breaking detection of defers nested inside if/switch/select inside loops — and the comment would look like justification for that regression.
Suggested:
// 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.There was a problem hiding this comment.
Fixed — isInsideLoop comment updated to "enclosed anywhere within a for or range loop body" to avoid implying only immediate children are checked.
…test.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…/goroutine test cases Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Weekly update covering the week of June 15–22, 2026: - Compiler +320% performance regression fix (#40662) - New deferinloop Go linter (#40679) - gh-aw-detection rollout to 50% of workflows (#40698) - JSON-RPC error handling fix (#40715) - Skillet sparse checkout path fix (#40684) - FNV-1a heredoc delimiter generation (#40696) - Agent of the Week: delight ✨ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
deferinside a loop doesn't execute at end of each iteration — it runs when the enclosing function returns, causing resource leaks (file handles, connections) and unexpected LIFO cleanup ordering.gocriticcovers this but is disabled due to golangci-lint v2 bugs; this custom analyzer fills the gap.Changes
pkg/linters/deferinloop/deferinloop.go— Analyzer usinginspector.Cursor.Enclosingto walk ancestors from eachDeferStmt, stopping atForStmt/RangeStmt(flag) orFuncLit(exempt — new scope). Honors//nolint:deferinloop. Test files are exempt. Doc string accurately describes the ancestor-walk behavior and FuncLit boundary exemption.pkg/linters/deferinloop/deferinloop_test.go+testdata/— Covers range loops, classic for loops, infinite loops, nested loops (flagged), defers nested insideif/selectwithin a loop (flagged), closures, goroutine func literals, explicit-close, and nolint (exempt).cmd/linters/main.go— Registersdeferinloop.Analyzerin the multichecker.pkg/linters/README.md— Documents the new linter.pkg/linters/doc.go— Updated analyzer count (29→30) and addeddeferinloopto the active analyzer list.pkg/linters/spec_test.go— AddeddeferinlooptodocumentedAnalyzers()to keep the spec consistent.Example