P-016: extractor lowers while/foreach to flow facts (A1 reaches real C#)#18
Conversation
…(A1 reaches C#)
A1 gave the core loop support, but the flow extractor still bailed on any loop
body (LowerFlowStmt -> false), so loopy C# methods were honestly skipped. Now
`while` and `foreach` bodies lower to a `while` flow op (both are the
0+-iteration, opaque-condition shape); the bridge maps it to the core `While`
node, so the worklist fixpoint analyses real loopy C# end-to-end — including
cross-iteration leak / use-after-release / double-release.
- Program.cs: WhileStatementSyntax + ForEachStatementSyntax -> {op:"while",body}.
`for` (can declare a resource in its initializer) and `do` (runs 1+ times)
still bail honestly.
- ownir.py: import While; _lower_flow maps the `while` op to the core While node;
_released_vars descends into while bodies so the OWN001 wording split holds.
- FlowLocalsSample.cs: whileLeak/foreachLeak (-> OWN001) + whileClean (balanced ->
silent); the HasLoop comment is clarified (it's a `for`, still skipped).
- tests/fixtures/ownir/flow_while.facts.json + test_ownir: pin the cross-iteration
OWN001+OWN003 through the bridge, dotnet-free.
- ci.yml: assert the while/foreach leaks + whileClean silence on real C#.
- docs/proposals/P-016 updated.
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Changeswhile/foreach Loop Lowering for --flow-locals
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
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 `@tests/test_ownir.py`:
- Around line 313-316: The validation logic in the wlfindings assertion is
missing a cardinality check, allowing duplicate or extra findings to go
undetected. Add a check to ensure len(wlfindings) == 2 in addition to the
existing code and event validations to confirm exactly two findings are present
in the wlfindings list.
🪄 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: bfc43ebf-1e3b-4187-8504-fb3159383514
📒 Files selected for processing (7)
.github/workflows/ci.ymldocs/proposals/P-016-deep-fact-extraction.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FlowLocalsSample.csownlang/ownir.pytests/fixtures/ownir/flow_while.facts.jsontests/test_ownir.py
…N003) The wlfindings assertion checked the code set (which collapses duplicates) and events but not cardinality, so a duplicate/extra finding could slip through. Add `len(wlfindings) != 2`, matching the other test_ownir blocks. Addresses a CodeRabbit review nitpick. https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
A1 reaches the frontend
A1 (#17) gave the core loop support, but the flow extractor still bailed on any loop body (
LowerFlowStmt → false), so loopy C# methods were honestly skipped at the frontend. This wires the two together:whileandforeachbodies now lower to awhileback-edge flow op, the bridge maps it to the coreWhilenode, and the worklist fixpoint analyses real loopy C# end-to-end — including the cross-iteration faults a single pass can't see.Why
while+foreach(and notfor/do)Both
whileandforeachare the same ownership shape: a body that runs 0+ times over an opaque condition/collection.foreach's loop variable is never anew'd candidate and its hidden enumerator is auto-disposed, so modelling its body as awhileis sound.for(can declare a resource in its initializer) anddo(runs 1+ times, so a 0-tripwhilemodel would false-positive) still bail honestly — left as the next increment.Changes
Program.cs—WhileStatementSyntax+ForEachStatementSyntax→{op:"while", line, body}(recursively lowered; bails if the body has an unmodelled statement).ownir.py— importWhile;_lower_flowmaps thewhileop to the coreWhilenode;_released_varsdescends intowhilebodies so the OWN001 never-disposed vs not-on-every-path wording split still holds inside loops.FlowLocalsSample.cs—whileLeak/foreachLeak(→ OWN001) andwhileClean(balanced acquire+dispose in the body → silent, no false positive); theHasLoopcomment is clarified (it's afor, still skipped).tests/fixtures/ownir/flow_while.facts.json+test_ownir— pin the cross-iteration OWN001 + OWN003 (acquire before the loop,releaseinside it) through the bridge, dotnet-free.ci.yml— assert thewhile/foreachleaks andwhileCleansilence on real C#.docs/proposals/P-016updated.What it now catches (verified end-to-end through the bridge)
newIDisposable acquired eachwhile/foreachturn, never disposedreleased inside itGTM impact
Loopy methods (in EF/business code, mostly
foreachover collections) were skipped wholesale; they're now analysed, so leaks/double-disposes inside loops surface.Tests / lint
Full suite green locally —
ownir 47/47,analysis 125/125,loops 20/20,gallery 11/11,fuzz 3000 clean,spec 22/22— andruff check .clean. The extractor itself needsdotnet(CI); its lowering was verified by hand and by simulating the emitted facts through the bridge.Follow-ons (not here)
for/foreach-with-disposable-iterator lowering, escape-via-projection hardening, then full graduation (raw extractor flag default-on +OWNIR_VERSIONbump).https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
New Features
IDisposablevalues insidewhileandforeachloops, with per-iteration analysis to surface leaks that previously could be missed.Documentation
while/foreachbodies.Tests
whilefixture, and extended verification to ensure expected findings are produced consistently across iterations.