Claude/zen pasteur 76hfs1#31
Conversation
…y-method recall slice)
The oracle's "Oracle only" Dispose-class bucket included plain undisposed locals the
--flow-locals detector skipped only because their method had a `try`. Lower
`try { A } [catch...] [finally { B }]` as sequential `A; B`: a local acquired in try
and disposed in finally stays balanced (silent), one never disposed leaks (now caught),
and try-methods are no longer skipped wholesale. Catch bodies aren't lowered; to stay
sound, bail (skip the method) if a catch disposes, so a release that only happens in a
catch is never missed -> no false leak.
Not yet modelled (documented next step): dispose-not-called-on-throw (disposed in try,
not finally) reads as released here — needs per-statement exceptional exits; switch/do
likewise stay skipped.
FlowLocalsSample gains TryFinallyClean (silent), TryNeverDisposed (flagged), and
CatchDisposesSkipped (soundly skipped -> silent); CI asserts each + the coverage
invariant still holds.
…y-lowering slice Third leak in the cross-tool fixture: a FileStream never disposed, inside a try-method. Before try/finally lowering it was "Oracle only" (Own.NET skipped any method with a `try`); now Own.NET lowers it and should join the FileStream control in "Agree" across Own.NET + CodeQL + Infer#. Bump the oracle sentinel to re-run.
The flow detector now lowers try/finally (sequential A;B, catch-disposes bailed for soundness), so a plain undisposed local in a try-method is caught. The cross-tool fixture's try-method FileStream leak moved from "Oracle only" into "Agree" (Own.NET + Infer#), validating the slice. Note the still-deferred dispose-not-called-on-throw (try-not-finally) + switch/do.
… CodeQL arm)
The cross-tool re-run lost CodeQL to JOB_STATUS_CONFIGURATION_ERROR ("Loaded a
configuration file for version '4.36.2', but running version '3.36.2'"): init was
github/codeql-action/init@v4 but analyze was still @V3, so init wrote a v4 config
the v3 analyze couldn't read. Bump analyze to @v4 to match. Bump the sentinel to
re-run and confirm all three tools (incl. CodeQL) return on the fixture.
|
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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughExtends the Changestry/finally Lowering in Flow-Locals
CodeQL Action Version Bump
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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://avatars.githubusercontent.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: 1470834192
ℹ️ 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".
| return false; | ||
| if (!LowerFlowStmt(trys.Block, tracked, nodes)) | ||
| return false; | ||
| if (trys.Finally is { } fin && !LowerFlowStmt(fin.Block, tracked, nodes)) |
There was a problem hiding this comment.
When a try block contains return, this lowers the finally body by appending it after the return op. The OwnIR CFG treats return as terminal, so a real cleanup such as try { return; } finally { stream.Dispose(); } becomes unreachable in the model and --flow-locals reports stream as leaked even though C# always runs the finally before exiting. This affects try/finally cleanup patterns with early returns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in dc983a6. You're right: the finally release lands after the terminal return op and becomes unreachable in the sequential model, so a resource the finally disposes was falsely flagged. Until finally-before-return is modelled, I bail the method when a try-with-finally contains a return. The common try { …; return x; } finally { r.Dispose(); } is safe anyway, so skipping it is sound — no false leak, and no false negative on that shape (a real leak there is rare). Locked with a TryFinallyReturn sample that must stay silent (CI asserts it).
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@corpus/fixtures/systemevents-console/README.md`:
- Line 17: The line starting with `#2 is the agreement...` in the README.md file
is triggering a Markdown linting error (MD018) because `#` followed by text
without a space is being interpreted as a heading marker. Reword the sentence so
that `#2` is not at the beginning of the line (for example, by moving it to the
middle like "Leak `#2` is the agreement..."), or escape the hash character to
prevent it from being interpreted as a Markdown heading token.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 371-375: The catch block disposal detection logic in the foreach
loop iterating over trys.Catches only checks for MemberAccessExpressionSyntax
(matching patterns like x.Dispose()), but fails to detect conditional-access
patterns like x?.Dispose() which use MemberBindingExpressionSyntax. Extend the
condition that currently checks "i.Expression is MemberAccessExpressionSyntax
cm" to also handle MemberBindingExpressionSyntax cases, ensuring both expression
types are checked for the method names "Dispose", "Close", or "DisposeAsync" to
prevent false positive OWN001 diagnostics in catch-only disposal scenarios.
🪄 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: 3f78e60e-a06e-4dab-9658-490a9d14a94b
📒 Files selected for processing (8)
.github/workflows/ci.yml.github/workflows/oracle.ymlcorpus/fixtures/systemevents-console/Program.cscorpus/fixtures/systemevents-console/README.mdcorpus/oracle-target.txtdocs/notes/real-world-mining.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FlowLocalsSample.cs
…n finally + ?.Dispose catch)
Two false positives in the try/finally lowering, caught in review:
* Codex (P2): a `return` inside a try-with-finally made the finally's release
unreachable after the terminal `return` op, so the model falsely flagged a resource
the finally disposes (the common `try { …; return x; } finally { r.Dispose(); }`).
Until finally-before-return is modelled, bail when a try-with-finally contains a
return — that shape is safe anyway, so skipping it is sound.
* CodeRabbit: the catch-dispose bail only matched `x.Dispose()` (member access), not
`x?.Dispose()` (member binding), so a catch-only conditional dispose got lowered and
could false-flag. Match both expression shapes.
FlowLocalsSample gains TryFinallyReturn and CatchNullCondDispose (both must stay
silent); CI asserts them. Also reword the fixture README to avoid MD018 (CodeRabbit).
2 participants
Summary by CodeRabbit
IDisposableresources withintry/catchandtry/finally, aligning results across tools and reducing false leak reports.systemevents-consolefixture README to cover new try-related leak coverage and behavior.