Skip to content

timesleepnocontext: enclosing-scope walk crosses arbitrary FuncLit boundaries — false positives + misleading fix for request/cal [Content truncated due to length] #42901

Description

@github-actions

Summary

timesleepnocontext (new 40th analyzer, pkg/linters/timesleepnocontext/timesleepnocontext.go) attributes a time.Sleep call to the first ctx-receiving ancestor found by an unbounded enclosing-scope walk, crossing every FuncLit boundary. Flagging a goroutine closure that closes over the outer ctx is intentional (testdata BadGoroutineWithCtx), but the same walk over-attributes time.Sleep in arbitrary callback closures that have their own context source to the outer registration ctx — a latent false positive whose remediation message points at the wrong context.

Type resolution itself is correct (isTimeSleepCall uses TypesInfo.ObjectOf + PkgName.Imported().Path()=="time", contextParamName uses types.Identical), and there are zero true positives in current prod code — so this is a latent correctness issue and an enforce-readiness blocker, not a live regression.

Location

  • pkg/linters/timesleepnocontext/timesleepnocontext.go:53-68 — the enclosing walk:
for encl := range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) {
    funcType := enclosingFuncType(encl.Node())
    if funcType == nil { continue }
    ctxParamName, hasCtx := contextParamName(pass, funcType)
    if !hasCtx { continue }          // <-- keeps walking OUTWARD through every FuncLit
    pass.Report(...)                  // attributes Sleep to the first ctx-bearing ancestor
    break
}
  • Intended boundary-crossing case (must be preserved): pkg/linters/timesleepnocontext/testdata/src/timesleepnocontext/timesleepnocontext.go:41-45 (BadGoroutineWithCtxgo func(){ time.Sleep(...) }()).

Why it's a false positive

The walk does not distinguish how the intervening FuncLit is invoked. A closure launched as a goroutine (go func(){}()) legitimately shares the outer ctx lifetime — flagging it is correct. But a callback closure that receives its own request-scoped context via a non-parameter mechanism does not, and the outer ctx is the wrong lifecycle to bind to.

Canonical latent FP:

func RegisterRoutes(ctx context.Context, mux *http.ServeMux) {
    mux.HandleFunc("/wait", func(w http.ResponseWriter, r *http.Request) {
        time.Sleep(d) // FLAGGED: "use select with ctx.Done() ..."
    })
}

The handler's real cancellation source is r.Context(), not the registration-time ctx (typically the server-lifetime/startup context that outlives every request). The linter both (a) fires a false positive and (b) emits a misleading remediation naming the wrong ctx (ctx.Done()), which if followed couples request sleeps to the wrong lifecycle. The same over-attribution compounds through deeply nested event/callback closures.

This is the scope_boundary_funclit family already established in this repo: deferinloop uses cur.Enclosing(For,Range,FuncLit) and breaks at the FuncLit boundary; prior findings #41164 (ctxbackground), #40947 (wgdonenotdeferred), #41606 (panicinlibrarycode) are the same class of FuncLit-boundary bug.

Impact

  • Latent false positive: fires the moment a time.Sleep is added inside any callback closure nested in a ctx-receiving function (HTTP handlers, event/registration callbacks, AfterFunc, etc.).
  • Misleading fix message binds the sleep to the wrong context.
  • Blocks CI enforcement (cgo.yml LINTER_FLAGS): the linter is otherwise enforce-ready (0 prod violations, wires internal/nolint at :34/:49 and filecheck.IsTestFile at :46), but shipping this latent FP into -test=false CI risks a spurious future failure.

Recommendation

When the enclosing walk crosses a FuncLit, only continue outward to attribute the sleep to an ancestor ctx when that FuncLit is a closure that clearly executes within the outer ctx's lifetime — i.e. it is the operand of a go statement (or a defer). For an ordinary callback FuncLit (a function-literal argument / assignment), stop at the boundary and only report if the closure's own signature receives a context.Context.

Concretely: when iterating cur.Enclosing(...), if the node is a *ast.FuncLit without its own ctx param and it is not the callee of an enclosing *ast.GoStmt/*ast.DeferStmt, break instead of continuing outward. This preserves BadGoroutineWithCtx while eliminating the callback FP class. The *ast.GoStmt parent is reachable via the cursor's parent navigation on the FuncLit.

Before / after intent

  • Before: time.Sleep in any callback closure nested (at any depth) inside a ctx func → flagged against the outermost ctx.
  • After: only go/defer-launched closures (and closures that themselves take a ctx) are flagged; synchronous callbacks with independent context sources are left alone.

Validation checklist

  • Keep existing want diagnostics green, including BadGoroutineWithCtx (:41-45).
  • Add a Good fixture: http.HandleFunc callback (no ctx param) nested in a func(ctx context.Context, ...) with a time.Sleep — expect no diagnostic.
  • Add a Good fixture: a plain synchronous callback arg register(func(){ time.Sleep(d) }) inside a ctx func — expect no diagnostic.
  • Add a Bad fixture: defer func(){ time.Sleep(d) }() inside a ctx func — expect a diagnostic (defer shares ctx lifetime).
  • Re-run over pkg/ to confirm still zero prod violations.

Effort

Small — single file (timesleepnocontext.go, ~10 lines around the walk at :53-68) plus 3 testdata fixtures. No public API change.

Analysis metadata

Generated by 🤖 Sergo - Serena Go Expert · 258.7 AIC · ⌖ 13.8 AIC · ⊞ 5.9K ·

  • expires on Jul 8, 2026, 9:12 PM UTC-08:00

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

cookieIssue Monster Loves Cookies!sergo

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions