feat(extractor): exception-edge recall slice — nested try, ctor throw-points, typed catches#33
Conversation
…-points, typed catches Lands the deferred exception-edge recall slice from PR #32. Three sound recall gaps (missed leaks, never false positives) in the try exception-edge model close: - Nested compound statements: edges now recurse into if/loop/block bodies and are injected before the nested LEAF statement — where the resource's ownership is exact (after any in-branch dispose). This catches a throw-before-dispose inside a branch while keeping the dispose-before-throw shape silent (DisposeInsideIfWithThrow 'cif' stays silent for this better reason now, not by skipping compounds). The throw continuation is threaded (onThrow) and composes across nested tries (inner finally, then outer, then return); a canEscape flag propagates an enclosing catch-all's swallow so no edge is injected in a region whose throws never reach method exit. - Constructor (new) as a throw point: a throwing ctor skips a PRIOR owned resource's dispose; StatementMayThrow now treats object creation as may-throw. The edge lands before the resource's own acquire (harmless — not yet owned there). - Typed/filtered catches: a non-tail catch no longer blanket-suppresses edges. Only a genuine catch-all (catch {} / catch (Exception), no when-filter) does; a typed catch lets the uncaught exception types propagate past the post-try dispose and really leak. All three match CodeQL's cs/dispose-not-called-on-throw. Validated end-to-end in CI on new samples (nestedLeak, ctorPrior, typedLeak leak; ctorLater stays silent) and on the core IR via the flow_nested_throw ownir fixture (52/52 bridge checks). Every existing try-sample keeps its verdict. Remaining deferred: finally-before-return and switch/do. 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 (2)
📝 WalkthroughWalkthroughThe PR refines the ChangesException-edge recall: lowering, samples, tests, and docs
Sequence Diagram(s)sequenceDiagram
participant LowerFlowStmt
participant StatementMayThrow
participant InjectThrowEdge
participant TryLower as Try Lowering
participant IREmitter as IR Emitter
LowerFlowStmt->>StatementMayThrow: stmt
StatementMayThrow-->>LowerFlowStmt: mayThrow: true/false
alt mayThrow and canEscape
LowerFlowStmt->>InjectThrowEdge: stmt, onThrow
InjectThrowEdge->>IREmitter: emit if(*) { onThrow continuation }
end
LowerFlowStmt->>IREmitter: emit acquire / flow facts
Note over TryLower: For try blocks
TryLower->>TryLower: compute escapesThisTry
TryLower->>TryLower: build bodyOnThrow = finally + (enclosing | return)
TryLower->>LowerFlowStmt: lower block with bodyOnThrow
LowerFlowStmt->>IREmitter: inject throw edges at leaves
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: fc2192d5e1
ℹ️ 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".
| static bool StatementMayThrow(StatementSyntax st) => | ||
| st.DescendantNodes().OfType<InvocationExpressionSyntax>().Any(i => !IsDisposeShaped(i)); | ||
| st.DescendantNodes().Any(n => | ||
| (n is InvocationExpressionSyntax i && !IsDisposeShaped(i)) | ||
| || n is ObjectCreationExpressionSyntax or ImplicitObjectCreationExpressionSyntax); |
There was a problem hiding this comment.
When constructor throw points are found with DescendantNodes(), the scan also enters anonymous-function bodies, so a statement like Func<Stream> f = () => new MemoryStream(); inside a try is given a synthetic throw edge even though that constructor is not executed when the delegate is created. If a tracked disposable is live before that statement and disposed after the try, the new edge skips the later dispose and reports a leak that cannot occur on that path. Exclude deferred bodies such as lambdas/expression-tree lambdas when looking for object-creation throw points.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 83a9d3e. StatementMayThrow now passes descendIntoChildren: n => n is not AnonymousFunctionExpressionSyntax to DescendantNodes, so a new (or call) inside a lambda / anonymous-method body no longer counts as a throw point at the declaring statement — that body runs on invoke, not on declaration. An immediately-invoked lambda is still caught, since the outer invocation is itself the throw point. This also closes the same latent issue the pre-existing invocation scan had (it descended into lambdas too). Pinned by a new sample CtorInLambdaNotThrow (lamPrior must stay silent in CI).
Generated by Claude Code
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 328-336: The IsCatchAll method incorrectly classifies any
qualified exception type ending in Exception (like Foo.Exception) as a catch-all
by extracting only the rightmost identifier. To fix this, modify the switch
expression to properly validate qualified names: for QualifiedNameSyntax, check
that it specifically represents System.Exception (by verifying the left side is
"System" and the right identifier is "Exception"), not just any qualified name
with Exception as the rightmost identifier. This ensures only truly system-level
exception catches are treated as catch-all, while domain-specific exceptions
like Foo.Exception correctly allow exception edges to be injected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 045fd5f8-952a-4703-b278-36c69451fb03
📒 Files selected for processing (7)
.github/workflows/ci.ymldocs/ROADMAP.mddocs/notes/real-world-mining.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FlowLocalsSample.cstests/fixtures/ownir/flow_nested_throw.facts.jsontests/test_ownir.py
…dge slice - StatementMayThrow: don't descend into lambda / anonymous-method bodies when scanning for throw points. A `new` (or call) inside `() => …` runs when the delegate is invoked, not where it is declared, so the declaring statement is not a throw point; counting it injected a phantom edge that could falsely flag a prior resource disposed after the try (Codex review). An immediately-invoked lambda is still caught via its outer invocation, and this also closes the same latent issue the pre-existing invocation scan had. Pinned by CtorInLambdaNotThrow (lamPrior silent). - IsCatchAll: match the canonical System.Exception spellings by full text instead of the rightmost identifier. `catch (Foo.Exception)` — a domain type whose rightmost name is `Exception` but which is not System.Exception — was misclassified as a catch-all, suppressing edges and missing a real leak (CodeRabbit review). Now only Exception / System.Exception / global::System.Exception are catch-alls; a qualified domain catch is typed and injects edges. Pinned by QualifiedTypedCatchLeaks (qualLeak). Both are no-false-positive-preserving. Tests 52/52; C# end-to-end validated in CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
2 participants
Lands the exception-edge recall slice deferred from #32. Three sound recall gaps in the
tryexception-edge model are closed — all matching CodeQL'scs/dispose-not-called-on-throw, none introducing a false positive (missed leaks become caught; nothing safe starts being flagged).What changed
if/loop/block bodies and are injected before the nested leaf statement — where the resource's ownership is exact (after any in-branch dispose). The throw continuation (onThrow) is threaded through the recursion and composes across nested tries (innerfinally→ outerfinally→return); acanEscapeflag propagates an enclosing catch-all's swallow, so no edge is injected in a region whose throws can never reach method exit.nestedLeakleaks;cif(DisposeInsideIfWithThrow) now stays silent for the right reason — leaf-level placement, not by skipping compoundsnew) as a throw pointStatementMayThrownow treats object creation as may-throw. The edge lands before the resource's ownacquire(harmless — not yet owned there).ctorPriorleaks;ctorLaterstays silentcatch {}/catch (Exception), nowhenfilter) does; a typed catch lets the uncaught exception types propagate past the post-try dispose and really leak.typedLeakleaksValidation
tests/fixtures/ownir/flow_nested_throw.facts.jsonfixture encodes the exact lowered IR the extractor now emits for all three shapes;test_ownir.pyasserts the verdicts (nestedLeak+ctorPriorleak,cif+ctorLatersilent). Suite is 52/52 bridge checks (was 51).ctorLateradded to the must-stay-silent allowlist. Every existingtry-sample keeps its verdict, and the--statscoverage invariant still holds.Deferred (still sound recall gaps)
finally-before-returnthreading (bailed today) andswitch/do. Docs (ROADMAP + real-world-mining note) updated to move the three landed items out of the deferred list.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
Documentation
try/catchblocks, including constructor-throw behavior and how typed/filtered catches affect injected exception paths.Improvements
IDisposablescenarios, especially for nested exception edges andnew/constructor-throw cases where disposal can be skipped.