wgdonenotdeferred precision: non-deferred wg.Done() inside a goroutine launched in a loop escapes detection (FuncLit scope bound [Content truncated due to length]

[github-actions]

Summary

The wgdonenotdeferred linter (33rd registered analyzer) has a false negative on the single most common real-world WaitGroup pattern: a non-deferred wg.Done() inside a goroutine that is launched from a for/range loop. The loop-membership check walks the AST ancestor stack without stopping at function-literal boundaries, so it treats the goroutine body as "inside the loop" and suppresses the diagnostic — even though defer wg.Done() inside that goroutine is correct and is precisely what the linter exists to recommend.

Evidence

pkg/linters/wgdonenotdeferred/wgdonenotdeferred.go:73-97:

inLoop := false
for i := range slices.Backward(stack) {
    switch stack[i].(type) {
    case *ast.ForStmt, *ast.RangeStmt:
        inLoop = true
    }
}

// Skip diagnostics for loop-body calls where defer would be semantically wrong.
if !inLoop {
    if exprStmt, ok := node.(*ast.ExprStmt); ok {
        // ... report wg.Done() ...
    }
}

The backward walk over stack sets inLoop = true if any ancestor is a ForStmt/RangeStmt, and never breaks when it crosses an *ast.FuncLit. For:

for range items {
    wg.Add(1)
    go func() {
        wg.Done()   // BUG: not deferred — a panic before this point deadlocks the WaitGroup
        doWork()
    }()
}

the wg.Done() ExprStmt's ancestor stack contains the enclosing RangeStmt, so inLoop becomes true and the diagnostic is suppressed. But the defer scope is the goroutine's function literal, not the loop iteration — defer wg.Done() here is both legal and the recommended fix. The linter misses exactly the case it was designed to catch.

Why the loop-skip is correct only within the same function scope

The loop-skip itself is intentional and correct for a wg.Done() placed directly in a loop body (deferring there would accumulate to function end). The defect is that the skip leaks across FuncLit boundaries.

The sibling linter deferinloop models the identical concept correctly. Its doc (pkg/linters/deferinloop/deferinloop.go:23) states: "a function literal between a defer and an enclosing loop is treated as a new scope boundary," and its enclosure walk breaks on *ast.FuncLit (deferinloop.go:66-74):

for encl := range cur.Enclosing(
    (*ast.ForStmt)(nil),
    (*ast.RangeStmt)(nil),
    (*ast.FuncLit)(nil),
) {
    switch encl.Node().(type) {
    case *ast.FuncLit:
        // new scope boundary -> stop
    ...

wgdonenotdeferred and deferinloop are conceptual inverses (one forbids defer-in-loop, the other recommends defer-for-Done) and must agree on the FuncLit boundary semantics. They currently disagree.

Test-coverage gap that hid this

pkg/linters/wgdonenotdeferred/testdata/src/wgdonenotdeferred/wgdonenotdeferred.go tests:

• BadGoroutine — go func(){ wg.Done() }() not in a loop -> correctly flagged (line 8).
• GoodLoopDone — wg.Done() directly in a for body -> correctly skipped (line 68).

but never the intersection (goroutine launched inside a loop), which is where the boundary bug lives.

Impact

• False negative on the dominant WaitGroup idiom. Launching wg.Add(1); go func(){ ... }() inside a loop is the textbook fan-out pattern; an un-deferred Done() there is exactly the deadlock-on-panic hazard the linter targets, and it is silently allowed.
• Blocks enforce-readiness. A linter that misses its primary target case cannot be trusted as a CI gate. (Current prod has only one wg.Done() site, pkg/console/spinner.go:142, which is correctly deferred — so this is a precision/correctness defect, not a present production leak.)

Recommendation

When computing inLoop, stop the ancestor walk at the nearest enclosing *ast.FuncLit so loop membership is evaluated relative to the innermost function scope (mirroring deferinloop):

inLoop := false
walk:
for i := range slices.Backward(stack) {
    switch stack[i].(type) {
    case *ast.FuncLit:
        // Closure resets the defer scope; loop membership only counts
        // within the same function literal.
        break walk
    case *ast.ForStmt, *ast.RangeStmt:
        inLoop = true
        break walk
    }
}

Validation checklist

• Add a BadGoroutineInLoop testdata case: for { wg.Add(1); go func(){ wg.Done(); doWork() }() } with a // want directive — confirm it is now flagged.
• Add a GoodDeferredGoroutineInLoop case: same but defer wg.Done() — confirm it is not flagged.
• Keep GoodLoopDone (direct loop-body wg.Done()) passing (still skipped).
• Run go test ./pkg/linters/wgdonenotdeferred/....

Effort

Small — single-file change in wgdonenotdeferred.go plus two testdata cases.

_{Pattern: scope-boundary-not-honored (FuncLit). Related precision findings: deferinloop (correct reference impl), #40733 (seenmapbool traversal).}

Generated by 🤖 Sergo - Serena Go Expert · 229.5 AIC · ⌖ 13.6 AIC · ⊞ 5.8K · ◷

• expires on Jun 29, 2026, 9:11 PM UTC-08:00