Recognize deferred named recover helpers in goroutinemissingrecover - #50848
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
🟡 Changes recommended
The analyzer can incorrectly accept helpers where recover() executes through a nested go or defer statement.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Extends goroutinemissingrecover to recognize same-package named recovery helpers and methods.
Changes:
- Resolves deferred named functions, methods, and generic instantiations.
- Indexes function bodies for recovery analysis.
- Adds positive and conservative negative fixtures.
File summaries
| File | Description |
|---|---|
goroutinemissingrecover.go |
Implements named-helper resolution and analysis. |
testdata/src/a/a.go |
Adds named-helper and method fixtures. |
Review details
Suppressed comments (1)
pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go:178
- The new generic-instantiation paths are not exercised by the fixture: all added accepted calls are non-generic. Please add cases for
defer helper[T]()(and preferably a generic-receiver method) so theIndexExpr/IndexListExprunwrapping andOrigin()lookup cannot regress unnoticed.
case *ast.IndexExpr:
// Explicit instantiation of a generic function: f[T].
return resolveFuncBody(unwrapParens(target.X), typesInfo, funcBodies)
case *ast.IndexListExpr:
return resolveFuncBody(unwrapParens(target.X), typesInfo, funcBodies)
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| continue | ||
| } | ||
| if containsRecoverCall(fn.Body, typesInfo) { | ||
| if containsRecoverCall(deferredBody, typesInfo) { |
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32 AIC · ⊞ 7.1K
Comment /matt to run again
| // Package-qualified function (pkg.Fn) — no selection recorded and the | ||
| // body lives in another package, so it cannot be inspected. | ||
| return nil, false | ||
| case *ast.IndexExpr: |
There was a problem hiding this comment.
[/tdd] The *ast.IndexExpr and *ast.IndexListExpr branches (generic function instantiations like defer f[T]()) are not covered by any test fixture — a regression here would go undetected.
💡 Suggested test fixture
Add to testdata/src/a/a.go:
func genericRecoverHelper[T any]() {
if r := recover(); r != nil {
_ = r
}
}
func genericRecoverDeferGoroutine() {
go func() {
defer genericRecoverHelper[int]()
panic("oops")
}()
}Include a parallel case without recover() to cover the negative path too.
@copilot please address this.
|
@copilot merge main and recompile |
|
🎉 This pull request is included in a new release. Release: |
goroutinemissingrecoveronly accepted adeferwhose target is a function literal, so a goroutine deferring a named helper that callsrecover()directly was flagged as unprotected — even though the Go spec counts any function called directly bydeferas a valid recovery point.Changes
hasTopLevelRecoverDefer: when the deferred target is not an*ast.FuncLit, resolve it to a*types.Funcand run the existingcontainsRecoverCallcheck against its body.resolveFuncBody(new): resolves*ast.IdentviaTypesInfo.Uses, methods viaTypesInfo.Selections, and unwraps generic instantiations (f[T],f[T1, T2]). Returns false for anything not statically resolvable.indexFuncBodies(new): one pass overpass.Filesbuilding*types.Func→ body (keyed byOrigin()so generic instantiations map back to the declaration).recover()buried in a nested closure inside the helper are still flagged.Test fixtures
Added to
testdata/src/a/a.go: named helper withrecover()and method withrecover()(no diagnostic); named helper withoutrecover(), deferred func value, and helper whoserecover()is nested in a closure (still flagged).Note: the fixture package reports pre-existing
unusedlint findings (every fixture function, old and new, is unreferenced); these are not introduced here.