Repository navigation
Detect missing recovery in goroutines launched through local closures - #71897
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
…inemissingrecover
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (163 new lines under 📄 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 Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: no blocking findings
The change correctly resolves local function-literal variables by type-object identity and conservatively excludes reassigned/address-taken targets. I found no actionable correctness or security issue on the changed lines. Static review only; tests were not run.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is well-scoped (mutable/address-taken exclusions, suppression, named-function exclusion are all correctly preserved), but one reassignment pattern still slips through the new detection, undercutting the PR's core claim of closing the local-closure loophole.
📋 Key Themes & Highlights
Key Finding
- Declare-then-assign variables (
var fn func(); fn = func(){...}) are never tracked. Therecordhelper's mutable-detection treats any non-:=/ValueSpecwrite recorded viaUsesas disqualifying, even when it's the variable's only write. I verified this with a standaloneanalysistestprobe:go fn()aftervar fn func(); fn = func(){ panic("oops") }is silently not flagged. See inline comment for repro and suggested direction.
Positive Highlights
- ✅ Good defensive exclusions for reassigned, range-assigned, and address-taken variables — each backed by a dedicated regression test (
reassignedLocalClosureGoroutine,rangeAssignedLocalClosureGoroutine,addressTakenLocalClosureGoroutine). - ✅ Shadowing test (
shadowedLocalClosureGoroutine) correctly validates object-identity resolution rather than name matching. - ✅ Doc comments at the package and function level were updated in lockstep with the behavior change.
Suggested Follow-up
- Add a regression test for the
var+ later=pattern (single assignment, no intervening declaration-time initializer) and adjustindexLocalFuncLiteralsto only mark a variable mutable when it has more than one write, rather than any non-:=write.
No other blocking issues found in the diff — the rest of the index/resolve logic is sound and the test matrix is thorough for the cases it does cover.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~53 AIC · ⌖ ~14 AIC · ⊞ 10.2K
Comment /matt to run again
| return | ||
| } | ||
| variable, ok := pass.TypesInfo.Defs[ident].(*types.Var) | ||
| if !ok { |
There was a problem hiding this comment.
[/tdd] False negative: a var fn func() declared then assigned (fn = func(){...}) is never tracked as a literal — the first write via = lands in the Uses branch and gets marked mutable, so the only initializer is discarded even though go fn() runs after it and never writes again.
💡 Repro and suggested fix
func twoStepVarAssignGoroutine() {
var fn func()
fn = func() {
panic("oops")
}
go fn() // currently NOT flagged, should be
}Confirmed via a standalone analysistest probe against this exact pattern — no diagnostic is reported. This is a common idiom (declare-then-assign, e.g. for recursive closures or to satisfy a pre-declared signature) and currently bypasses the check entirely, which undercuts the PR's stated goal of closing the local-closure loophole.
Consider tracking assignment count per variable instead of binary mutable/defs-only: if a *types.Var is written to exactly once via = (and never via :=/ValueSpec with a separate literal, never range-assigned, never address-taken), that single write should still be eligible as the literal initializer. The current logic conflates "reassigned after first literal" with "first and only assignment," both landing in the same mutable bucket.
@copilot please address this.
There was a problem hiding this comment.
🟡 Changes recommended
Confirmed false-positive diagnostics remain for protected goroutines.
2 open issues
What changed in this PR
Extends goroutinemissingrecover so extracting a goroutine’s inline function into a local closure variable does not bypass panic-safety checks.
Changes:
- Resolve local closure initializers by variable identity while excluding reassigned and address-taken variables.
- Add regression coverage for recovery guards, parentheses, shadowing, suppression, and mutable targets.
| File | Description |
|---|---|
pkg/linters/goroutinemissingrecover/testdata/src/a/a.go |
Adds closure-detection and exclusion regression cases. |
pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go |
Indexes local closures and checks their recovery guards. |
Review effort: Max
| if variable, ok := pass.TypesInfo.Uses[fun].(*types.Var); ok { | ||
| call = localLiterals[variable] |
| case *ast.UnaryExpr: | ||
| if stmt.Op == token.AND { | ||
| record(stmt.X, nil) | ||
| } |
|
@copilot run pr-finisher skill |
Ran |
|
🎉 This pull request is included in a new release. Release: |
goroutinemissingrecovermissedgo fn()whenfnwas a local closure, allowing extraction of an inline literal to silently bypass panic-safety checks.:=andvarliteral initializers by type-object identity, then apply the existing top-level defer/recover check.