panicinlibrarycode: enforce FuncLit boundaries for init/doc panic exemptions#41631
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
👋 Nice fix to the The PR description is clear, the logic change is tight (8 lines modified in the analyzer), and the testdata additions cover both new failure cases while preserving the existing exemption scenarios. This looks ready for review. ✅
|
|
✅ 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. No test files were added or modified in this PR. Test Quality Sentinel skipped. The PR modifies only the linter implementation (panic-in-library-code.go) and its testdata corpus (testdata/src/panicinlibrarycode/panicinlibrarycode.go). The existing test file (panic-in-library-code_test.go) was not changed. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41631 does not have the 'implementation' label and has only 24 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
This PR fixes panicinlibrarycode false negatives where panics inside nested closures were incorrectly exempted when the enclosing function was init() or had a documented panic contract. The analyzer now treats FuncLit as an exemption boundary so init()/doc-contract exemptions only apply to panics directly in the function body.
Changes:
- Update
isInInitFunctionto stop exemptions when a*ast.FuncLitis encountered while walking enclosing nodes. - Update
hasDocumentedPanicContractwith the sameFuncLitboundary rule. - Add regression testdata covering panics inside closures nested in
init()and in documented “panics if …” functions (now reported).
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/panic-in-library-code/panic-in-library-code.go | Enforces FuncLit boundaries so init/doc-contract panic exemptions don’t suppress panics inside nested closures. |
| pkg/linters/panic-in-library-code/testdata/src/panicinlibrarycode/panicinlibrarycode.go | Adds test cases to ensure nested-closure panics are reported even when wrapped by init/doc-contract contexts. |
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: 1
- Review effort level: Low
| // isInInitFunction reports whether the panic is inside a top-level init() | ||
| // function. Only top-level (no receiver) init functions are recognized; | ||
| // methods named init are ordinary methods and are not exempt. |
![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 /diagnose, /tdd, and /improve-codebase-architecture — approving with two minor suggestions.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause precisely identified:
Enclosingwas blind toFuncLitboundaries, so the walk silently passed through closures and matched the outerFuncDecl. - ✅ Fix is minimal and correct — adding
(*ast.FuncLit)(nil)to theEnclosingcall and short-circuiting on the firstFuncLitis the right invariant. - ✅ Regression tests added for both affected exemption paths (
isInInitFunctionandhasDocumentedPanicContract). - ✅
isInSyncOnceDoFuncLitis unaffected — it operates on a separate code path that runs before either exemption.
Minor Suggestions
- Duplicate guard pattern (inline comment on
panic-in-library-code.goL185): the 3-line FuncLit check is copied verbatim into both functions. A sharedfindEnclosingFuncDeclhelper would keep the invariant in one place. - Deep nesting test (inline comment on testdata L51): the current tests only cover one closure level. A 2-level test would make the
Enclosingtraversal order contract explicit.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 51.7 AIC · ⌖ 8.14 AIC · ⊞ 6.5K
| for encl := range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) { | ||
| if _, isFuncLit := encl.Node().(*ast.FuncLit); isFuncLit { | ||
| return false | ||
| } |
There was a problem hiding this comment.
[/improve-codebase-architecture] The identical 3-line FuncLit boundary guard is duplicated verbatim in isInInitFunction (lines 183–185) and hasDocumentedPanicContract (lines 200–202). A third exemption function added later would likely miss this invariant.
💡 Suggested: extract a shared helper
// findEnclosingFuncDecl returns the innermost enclosing *ast.FuncDecl,
// or nil if a FuncLit boundary is crossed first.
func findEnclosingFuncDecl(cur inspector.Cursor) *ast.FuncDecl {
for encl := range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) {
if _, ok := encl.Node().(*ast.FuncLit); ok {
return nil
}
if decl, ok := encl.Node().(*ast.FuncDecl); ok {
return decl
}
break
}
return nil
}Both callers become single-expression checks, and the boundary invariant lives in one place.
| panic("handler panic outside init flow") // want `avoid panic in library code; return an error instead` | ||
| } | ||
| _ = handler | ||
| } |
There was a problem hiding this comment.
[/tdd] The test covers one level of closure nesting. Consider adding a two-level case to explicitly document that the boundary check handles deep nesting (the algorithm is correct because Enclosing yields innermost-first, but a test makes that contract visible).
💡 Suggested test case
func init() {
outer := func() {
inner := func() {
panic("deeply nested") // want `avoid panic in library code; return an error instead`
}
_ = inner
}
_ = outer
}Without this, a future change that accidentally breaks inner-to-outer traversal order would not be caught by these tests.
There was a problem hiding this comment.
The FuncLit-boundary fix is logically sound and the two new test cases cover the primary regression scenarios. Two medium-severity findings worth addressing.
Findings summary
Dead code in both boundary functions (lines 187, 204)
After adding the FuncLit early-return, the decl, ok := ...; if !ok { break } defensive branch is permanently unreachable in both isInInitFunction and hasDocumentedPanicContract. The loop only yields FuncDecl or FuncLit nodes; FuncLit is already handled; ok is therefore always true. The residual defensive code misleads readers and should be replaced with a direct type assertion.
Implicit check-ordering dependency, no test coverage
The fix is correct for sync.Once.Do(func(){ panic(...) }) nested inside init() only because isInSyncOnceDoFuncLit short-circuits before isInInitFunction in shouldSkipPanic. That ordering constraint is undocumented and unprotected by any test. A future reorder of those checks would silently convert this exemption into a false positive.
🔎 Code quality review by PR Code Quality Reviewer · 83 AIC · ⌖ 7.62 AIC · ⊞ 5.2K
| return false | ||
| } | ||
| decl, ok := encl.Node().(*ast.FuncDecl) | ||
| if !ok { |
There was a problem hiding this comment.
Dead code: if !ok { break } is unreachable after the new FuncLit early-return, silently misleading future readers about the invariants.
💡 Details and fix
cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) yields only *ast.FuncDecl and *ast.FuncLit nodes. The new early return false handles every *ast.FuncLit, so any code that reaches this type assertion is guaranteed to hold a *ast.FuncDecl — ok is always true. The if !ok { break } block is permanently dead.
The same issue exists at line 204 in hasDocumentedPanicContract.
Replace both with a direct assertion that makes the invariant explicit:
decl := encl.Node().(*ast.FuncDecl)| _ = handler | ||
| } | ||
|
|
||
| // ok: panic inside a sync.Once.Do callback. |
There was a problem hiding this comment.
Missing test for sync.Once.Do callback inside init() — the exemption of this case now depends silently on check-ordering in shouldSkipPanic, with no regression test to protect it.
💡 Details and suggested test
The FuncLit boundary change makes isInInitFunction return false for any panic inside a closure within init(). The sync.Once.Do callback is still correctly exempt only because isInSyncOnceDoFuncLit short-circuits before isInInitFunction in shouldSkipPanic:
return isInSyncOnceDoFuncLit(pass, cur) || // must fire first
panicMessageStartsWithBUG(pass, call) ||
isInInitFunction(cur) || // now returns false for any FuncLit boundary
hasDocumentedPanicContract(cur)This ordering contract is undocumented. If the checks are ever reordered, the sync.Once.Do-inside-init() shape silently breaks (becomes a false positive). No test covers it today.
Add a companion case here:
func init() {
once.Do(func() {
panic("lazy init inside init()") // should not be flagged
})
}
panicinlibrarycodeincorrectly suppressed panics inside nested closures when the enclosingFuncDeclwasinit()or had a documented panic contract (panics if/on ...). The exemption walk skippedFuncLitboundaries, creating false negatives for deferred/registered callbacks that execute outside the parent function’s contract.Analyzer boundary semantics
isInInitFunctionto walkEnclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil))and immediately stop exemption when the innermost enclosing node is a*ast.FuncLit.hasDocumentedPanicContractwith the same boundary rule so documented-contract exemptions only apply to panics directly in the documented function body.Regression coverage
init()(now reported).panics if ...function (now reported).init()panic, direct documented-contract panic, andsync.Once.Docallback exemption).