feat(extractor): close remaining flow-lowering gaps — finally-before-return, do, switch#34
Conversation
…return, do, switch
Closes the last deferred control-flow recall gaps in the --flow-locals lowering, so the
flow detector now models every common construct (only goto/labeled statements and a few
exotic forms still honestly bail).
- finally-before-return: an `onReturn` continuation is threaded alongside `onThrow`
(the finally-stack + return), so a `return` inside a try-with-finally runs the finally
FIRST — a finally-disposed resource is released on the return path. Unlike `onThrow`,
`onReturn` is never suppressed by catches (a return is never caught). The old "bail the
whole method if a try-with-finally has a return" is removed; such methods are analysed,
and an early return that skips a later dispose is now caught.
- do-while: desugared `do { B } while(c)` -> `B; while(c){ B }` (the body runs 1+ times).
Modelling it as a bare 0+-trip `while` would be unsound — a resource released only in the
body but acquired outside would falsely leak on the phantom 0-trip path.
- switch: lowered to a chain of opaque, mutually-exclusive `if(*)` branches, one per
section, trailing `break` stripped (anything else — nested break, goto case, throw —
bails). With NO default the LAST case becomes the tail rather than an empty no-match
branch: that avoids a FALSE POSITIVE on an exhaustive switch (e.g. over an enum) where
every case disposes; the cost is a sound recall gap (a non-exhaustive no-match leak is
only missed when every case disposes).
All no-false-positive-preserving. Validated on the core IR via a new flow_finally_switch
fixture (silent: finally-before-return clean, switch-all-dispose; leak: early-return,
switch-else) — tests 53/53 — and end-to-end in CI on new samples (earlyRet/doLeak/swLeak
leak; other/doClean/swAll/tfRet silent). Existing samples keep their verdicts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR threads an Changesfinally/switch/do onReturn Lowering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95af05cfc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { new { op = "return", var = (string?)null, line = LineOf(trys) } }); | ||
| foreach (var stmt in trys.Block.Statements) | ||
| if (!LowerFlowStmt(stmt, tracked, nodes, bodyCanEscape, bodyOnThrow)) | ||
| if (!LowerFlowStmt(stmt, tracked, nodes, bodyCanEscape, bodyOnThrow, bodyOnReturn)) |
There was a problem hiding this comment.
When a try/finally containing a return uses a null-conditional dispose such as finally { r?.Dispose(); }, this new path no longer bails and threads the return through finallyNodes, but those nodes are produced by EmitFlowExpr, which only emits releases for direct member-access calls and treats r?.Dispose() as a use. The analyzer therefore reports OWN001 for a resource that the finally releases on both normal and return paths; either model conditional-access disposals before lowering these returns or keep skipping that shape.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in bb97a5d. EmitFlowExpr now recognizes x?.Dispose() / x?.Close() / x?.DisposeAsync() (the member-binding form under a conditional access) as a release, mirroring IsDisposeShaped (which already handled it for the may-throw / catch checks). So finally { x?.Dispose(); } releases x on both the normal completion and the threaded return path — no false OWN001. This also closes the same latent issue for a finally { x?.Dispose(); } without a return, which the finally-before-return threading just made reachable. Pinned by a new sample NullCondFinallyDispose (ncf must stay silent in CI).
Generated by Claude Code
…e in EmitFlowExpr
`EmitFlowExpr` only treated `x.Dispose()` (member access) as a release; `x?.Dispose()`
(null-conditional — a member BINDING under a conditional access) fell through to "use".
So a `finally { x?.Dispose(); }` modelled the dispose as a use, never releasing x. The
finally-before-return threading (this PR) widened that latent false positive onto the
return path: a try-with-finally + return that disposes via `?.` was bailed before, but is
now analysed and would falsely report OWN001 (Codex review).
Fix: recognize `x?.Dispose()`/`x?.Close()`/`x?.DisposeAsync()` as a release too, mirroring
IsDisposeShaped (which already handled the member-binding form for the may-throw / catch
checks). Pinned by a new sample NullCondFinallyDispose (`ncf` must stay silent in CI).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
2 participants
Closes the last deferred control-flow recall gaps in the
--flow-localslowering. The flow detector now models every common construct — onlygoto/labeled statements and a few exotic forms still honestly bail. All three changes preserve the no-false-positive invariant.What changed
finally-before-returnonReturncontinuation is threaded alongsideonThrow(finally-stack + return), so areturninside a try-with-finally runs the finally first — a finally-disposed resource is released on the return path. UnlikeonThrow,onReturnis never suppressed by catches (areturnis never caught). The old "bail the whole method if a try-with-finally has a return" is removed.earlyRetleaks (early return skips a later dispose);othersilent (finally disposes it on the return path);tfRetstays silentdo/whiledo { B } while(c)→B; while(c){ B }(body runs 1+ times). A bare 0+-tripwhilewould be unsound — a resource released only in the body but acquired outside would falsely leak on the phantom 0-trip path.doLeakleaks;doCleansilentswitchif(*)branches (one per section, trailingbreakstripped; nested break /goto case/throwbail). With nodefaultthe last case becomes the tail rather than an empty no-match branch — avoiding a false positive on an exhaustive switch (e.g. over an enum) where every case disposes.swLeakleaks (default branch doesn't dispose);swAllsilent (all cases dispose, no default)Validation
tests/fixtures/ownir/flow_finally_switch.facts.jsonencodes the exact lowered IR;test_ownir.pyasserts the verdicts (clean finally-return + switch-all-dispose stay silent; early-return + switch-else leak). Suite 53/53 (was 52).earlyRet,doLeak,swLeak) and silent-allowlist additions (other,doClean,swAll). Every existing sample keeps its verdict; the--statsinvariant still holds.Notes
switchno-defaultmodel trades a sound recall gap (a non-exhaustive no-match leak is missed only when every case disposes) for never false-flagging an exhaustive switch. Enum-exhaustiveness-aware modelling (using the semantic model) is a possible future refinement.With this, the flow-lowering recall work is complete — next stop is the lifetimes / borrow model.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
try/finallypaths that include early returns, reducing missed leaks.switchstatements anddo-whileloops in disposal scenarios.x?.Dispose()), treating it consistently with other tracked releases.