httprespbodyclose: analyze function literals as first-class scopes#43526
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The only changes are to the production linter code (httprespbodyclose.go) and linter testdata input (testdata/src/httprespbodyclose/httprespbodyclose.go). Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43526 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (67 additions detected). |
There was a problem hiding this comment.
Pull request overview
This PR improves the httprespbodyclose Go analyzer so it treats function literals (func(...) { ... }) as first-class, independently analyzed scopes—fixing false negatives where both the HTTP response assignment and a non-deferred resp.Body.Close() occur inside a closure (e.g., goroutines, handlers, IIFEs).
Changes:
- Extend the inspector traversal to analyze both
*ast.FuncDecland*ast.FuncLitnodes. - Refactor the per-scope analysis into a shared
inspectFunchelper that operates on either node type’sBody. - Add testdata cases covering closure-local true-positives (manual close) and a closure-local non-violation (deferred close).
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/httprespbodyclose/httprespbodyclose.go | Adds FuncLit scope analysis via a unified function-body inspection path while preserving nested FuncLit short-circuiting. |
| pkg/linters/httprespbodyclose/testdata/src/httprespbodyclose/httprespbodyclose.go | Adds closure-based positive/negative test cases to prevent regressions on the previously missed leak pattern. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: httprespbodyclose — analyze function literals as first-class scopes
The fix is correct and the approach is sound.
Scope traversal design ✅
insp.Preorder visits both *ast.FuncDecl and *ast.FuncLit nodes. Within each scope, inspectNode returns false on any nested FuncLit, ensuring each closure scope is analyzed exactly once — no duplicate diagnostics, no bleed across scopes.
Captured-variable safety ✅
Variables from outer scopes do not appear in an inner scope's respVars map, so cross-scope closes (as in the preserved GoodCloseInsideClosure case) remain unflagged.
Test coverage ✅
IIFE, goroutine, and handler closures are all exercised; the want-annotation pattern is correctly placed on the assignment line.
Minor observation (non-blocking)
In inspectFunc, the FuncLit nil-body guard (if fn.Body == nil { return }) is logically unreachable — a FuncLit always has a body in valid Go AST. Removing it would marginally reduce noise, but leaving it is harmless defensive code and not worth a change request.
No blocking issues. Approving.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 40.6 AIC · ⌖ 5.82 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — this is a well-targeted bug fix with good test coverage. Leaving a few suggestions as comments; no blocking issues.
📋 Key Themes & Highlights
Key Themes
FuncLitnil guard is dead code (line 55):*ast.FuncLit.Bodyis nevernilin a valid AST; the guard is misleading and both switch arms are identical. Minor but worth cleaning up.- Short-circuit deserves a comment (line 87): The
inspectNodebail-out on*ast.FuncLitis the correctness linchpin that prevents double-counting, but it reads like a bug without a comment. Future maintainers need to understand why it's there. - Missing boundary test (line 48 of testdata): The new
FuncLitscope doesn't cover the case where an IIFE assigns a*http.Responseto an outer variable and returns without closing — a genuine non-violation that's worth pinning. - Naming (line 46):
inspectFuncis generic;inspectFuncScopeor a doc comment would telegraph the dual-form contract.
Positive Highlights
- ✅ Root cause is correctly diagnosed —
Preorder+FuncLitinclusion is the minimal fix - ✅ Inner walk correctly short-circuits at nested
FuncLitto prevent double-reporting - ✅ Test cases cover all three real-world closure shapes (IIFE, goroutine, HTTP handler) plus the defer-good-path
- ✅
GoodCloseInsideClosure(outer-assigned, inner-closed) is preserved as an explicit non-violation - ✅ No changes to unrelated linter logic
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 76.3 AIC · ⌖ 5.55 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/linters/httprespbodyclose/httprespbodyclose.go:55
[/diagnosing-bugs] *ast.FuncLit.Body is never nil in a valid Go AST — the grammar requires a body for function literals. This nil guard is dead code and adds misleading defensive noise that could cause future readers to wonder what scenario it protects against.
<details>
<summary>💡 Suggested simplification</summary>
Collapse the two identical case arms into one check after the switch:
var body *ast.BlockStmt
switch fn := n.(type) {
case *ast.FuncDecl:
body = fn.Body
case *a…
</details>
<details><summary>pkg/linters/httprespbodyclose/httprespbodyclose.go:87</summary>
**[/diagnosing-bugs]** `inspectNode` short-circuits on any `*ast.FuncLit` to prevent double-counting, but now that `Preorder` visits every `FuncLit` independently, the short-circuit only needs to skip nested literals during the *inner* `ast.Inspect` walk — this part is correct. However, the short-circuit also means a **deferred close defined in an outer function that closes over a variable assigned in an inner closure would not be tracked**. A comment here explaining this is intentional would p…
</details>
<details><summary>pkg/linters/httprespbodyclose/testdata/src/httprespbodyclose/httprespbodyclose.go:48</summary>
**[/tdd]** The test suite covers detection in IIFE, goroutine, and handler closures, which is great. A missing boundary case: when an IIFE assigns a `*http.Response` into an outer variable and returns without closing it — this should not be flagged (caller owns the body), but it's untested and close to the new code path.
<details>
<summary>💡 Suggested test case</summary>
```go
// GoodIIFEReturnsResp returns the response from the IIFE — caller is responsible for closing.
func GoodIIFEReturnsR…
</details>
<details><summary>pkg/linters/httprespbodyclose/httprespbodyclose.go:46</summary>
**[/codebase-design]** The function name `inspectFunc` is now shared between `FuncDecl` and `FuncLit`, but the test-file guard (`filecheck.IsTestFile`) uses `n.Pos()` to get the filename. For `FuncLit` nodes nested inside a non-test file, this will correctly skip test files — but the intent is less obvious than for `FuncDecl`. A renaming to something like `inspectFuncScope` and a short doc comment would clarify that this handles both declaration and literal forms.
<details>
<summary>💡 Suggest…
</details>
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 129.7 AIC · ⌖ 5.19 AIC · ⊞ 5.4K
Comment /review to run again
| return | ||
| } | ||
| body = fn.Body | ||
| default: |
There was a problem hiding this comment.
Dead default: branch is unreachable and silently masks future type-contract violations.
💡 Details
insp.Preorder is invoked with an explicit node filter of [*ast.FuncDecl, *ast.FuncLit]. The framework guarantees the callback is only invoked for those two types, making the default: return permanently dead.
This creates two compounding risks:
- Readers are misled into thinking
inspectFuncmay receive other node types, obscuring the real caller contract. - If
inspectFuncis ever called directly (test helper, new filter entry), a wrong type silently produces no diagnostic — no panic, no error, just a quiet miss.
Suggestion — remove default entirely so the exhaustive type switch documents the contract:
switch fn := n.(type) {
case *ast.FuncDecl:
if fn.Body == nil {
return
}
body = fn.Body
case *ast.FuncLit:
if fn.Body == nil {
return
}
body = fn.Body
}If forward-compatibility matters, use a compile-time exhaustiveness check or a panic.
🤖 PR Triage — Run §28741077056
Rationale: Fixes false-negative leak detection in
|
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the remaining actionable review feedback in |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey The test coverage is thorough — IIFEs, goroutine closures, and HTTP handler closures all covered, plus the non-violation
|
|
🎉 This pull request is included in a new release. Release: |
httprespbodyclosemissed a real leak pattern when bothrespassignment and manualresp.Body.Close()occurred inside a closure. The analyzer only enteredFuncDecland skippedFuncLitbodies entirely, creating false negatives in goroutines/handlers/IIFEs.Scope traversal fix
*ast.FuncDecland*ast.FuncLit.FuncLitshort-circuiting during inner walks so each scope is analyzed exactly once (prevents duplicate diagnostics).Behavioral coverage in testdata
go func()closure with manual non-deferred closehttp.HandleFunc-style closure with manual non-deferred closedefer resp.Body.Close()