Skip to content

feat: PoC oficially proven?#15

Merged
PhysShell merged 2 commits into
mainfrom
claude/p016-b0b-b2-flow-locals
Jun 16, 2026
Merged

feat: PoC oficially proven?#15
PhysShell merged 2 commits into
mainfrom
claude/p016-b0b-b2-flow-locals

Conversation

@PhysShell

@PhysShell PhysShell commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • New Features

    • Added experimental --flow-locals analysis for more accurate, path-sensitive tracking of local IDisposable disposal issues.
    • Reports cases like use-after-dispose, double-disposal, and conditional disposal leaks.
    • Extended the analyzer output to include per-method flow facts.
  • Improvements

    • own-check scripts now enable --flow-locals by default, with --legacy / -Legacy to fall back to the older flat detector.
  • Documentation / Tests

    • Updated the proposal status and expanded CI validation to cover flow-sensitive scenarios.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 31108fc2-90d1-4210-a3ed-a95726fa6343

📥 Commits

Reviewing files that changed from the base of the PR and between 5d8ca9b and 336281e.

📒 Files selected for processing (6)
  • .github/workflows/ci.yml
  • docs/proposals/P-016-deep-fact-extraction.md
  • frontend/roslyn/OwnSharp.Extractor/Program.cs
  • frontend/roslyn/samples/FlowLocalsSample.cs
  • scripts/own-check.ps1
  • scripts/own-check.sh
✅ Files skipped from review due to trivial changes (1)
  • docs/proposals/P-016-deep-fact-extraction.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • .github/workflows/ci.yml
  • frontend/roslyn/OwnSharp.Extractor/Program.cs

📝 Walkthrough

Walkthrough

Adds an experimental --flow-locals mode to the Roslyn extractor that emits per-method CFG flow facts (acquire/use/release/if/return nodes) for non-escaping local IDisposable objects. The OwnIR Python pipeline is extended to validate, lower, and diagnose these facts, producing path-sensitive OWN001/002/003 findings. Wrapper scripts toggle the feature, and CI asserts the expected findings end-to-end.

Changes

Flow-sensitive local IDisposable tracking (--flow-locals spike)

Layer / File(s) Summary
Sample test cases and --flow-locals CLI flag
frontend/roslyn/samples/FlowLocalsSample.cs, frontend/roslyn/OwnSharp.Extractor/Program.cs
FlowLocalsSample.cs defines the canonical disposal scenarios (use-after-dispose, leak-on-else, double-dispose, clean, loop-skipped, escaping return, task-exempt, timer-leak). Program.cs adds the --flow-locals CLI flag as an experimental opt-in parameter.
Extractor flow-lowering helpers and per-method emission
frontend/roslyn/OwnSharp.Extractor/Program.cs
Adds IsDisposable detection, method-name extraction, and statement-to-CFG-node lowering helpers (bailing on loops/try/switch). Initializes flowFunctions accumulator, suppresses legacy D1 syntactic detector when --flow-locals is active, implements per-method emission loop that filters escaping locals and lowers bodies, and emits flowFunctions into output JSON schema.
OwnIR schema validation, flow lowering, and diagnostic mapping
ownlang/ownir.py
Imports new AST node types for control flow; extends load() to validate optional functions array; extends to_module() to invoke _lower_flow and append Function nodes; implements _lower_flow() allocating loc_<n> handles and emitting Let/Use/Release/Return/If statements; extends check_facts() to emit Finding(kind="disposable") for OWN001/002/003/009 on flow-local subjects.
Wrapper scripts: --legacy flag to toggle --flow-locals
scripts/own-check.ps1, scripts/own-check.sh
own-check.ps1 adds -Legacy switch to select flat detector instead of default path-sensitive analysis; conditionally passes --flow-locals to extractor. own-check.sh adds --legacy flag with equivalent logic, initializing legacy variable and conditionally appending --flow-locals to extractor_args. Both document the tradeoff between old name-based and new flow-sensitive modes.
CI assertion step and proposal status update
.github/workflows/ci.yml, docs/proposals/P-016-deep-fact-extraction.md
CI adds wpf-extractor step that runs --flow-locals on the sample, pipes facts through ownir, and asserts OWN002/OWN001/OWN003 are present while clean/looped/esc/exemptTask are absent. Proposal P-016 status updated with B0a/B0b+B2 completion markers, current graduation caveats, and next steps.

Sequence Diagram(s)

sequenceDiagram
  participant CI as CI Job
  participant Extractor as OwnSharp.Extractor
  participant Sample as FlowLocalsSample.cs
  participant OwnIR as ownir.py
  participant Core as OwnIR Core Checker

  CI->>Extractor: run --flow-locals FlowLocalsSample.cs
  Extractor->>Sample: Roslyn semantic walk of method bodies
  Sample-->>Extractor: IDisposable locals + CFG structure
  Extractor-->>CI: OwnIR JSON { components[], functions[] }
  CI->>OwnIR: load + to_module(facts)
  OwnIR->>OwnIR: _lower_flow() → Let/Use/Release/Return/If AST nodes
  OwnIR-->>Core: Module with flow Function nodes
  Core-->>OwnIR: diagnostics (OWN001/002/003 on loc_n handles)
  OwnIR-->>CI: Finding(kind="disposable", code=OWN00x)
  CI->>CI: assert OWN002/OWN001/OWN003 present, clean/looped/esc absent
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

  • PhysShell/Own.NET#14: Both PRs modify ownlang/ownir.py's OwnIR-to-Module lowering path (to_module/AST construction) and its subsequent checking logic, with this PR extending that AST lowering to additionally handle the new functions flow-bodies.
  • PhysShell/Own.NET#10: Both PRs directly modify the CI/CLI orchestration layer—specifically scripts/own-check.sh (prior PR introduces the pipeline; this PR extends the extractor args to toggle path-sensitive --flow-locals via --legacy).

Poem

🐇 A local stream was opened wide,
disposed on one path, leaked beside—
CFG nodes flow, loc_0 takes hold,
OWN002 found, the tale is told.
--flow-locals hops the happy track,
no escaped stream sneaks out the back! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is vague and unclear. 'PoC oficially proven?' uses non-descriptive language and does not convey what the changeset actually accomplishes. Replace with a clear, descriptive title that summarizes the main change, such as 'feat: Add flow-sensitive local disposal tracking with --flow-locals flag' or similar.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/p016-b0b-b2-flow-locals

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
ownlang/ownir.py (1)

438-445: 💤 Low value

Consider isolating localmap for branch-scoped locals in future graduation.

The if handling passes the same localmap reference to both then and else branches. If a local with the same name is declared in both branches (different declarations, same name), the second branch's acquire would overwrite the first in localmap, and the handles dict would have both entries correctly, but cross-branch references could resolve to the wrong handle.

For the current samples (all method-scoped locals), this isn't exercised. For graduation with block-scoped locals, consider copying localmap before each branch.

🤖 Prompt for 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.

In `@ownlang/ownir.py` around lines 438 - 445, The `if` handling in `_lower_flow`
passes the same `localmap` reference to both `then` and `else` branches, which
will cause issues when block-scoped locals are supported in future graduation.
If the same variable name is declared in both branches, the second branch would
overwrite the first in the shared `localmap`. To prepare for block-scoped local
support, create a shallow copy of `localmap` before each branch and pass the
copy to the respective `_lower_flow` calls for `then_b` and `else_b`, ensuring
each branch has its own isolated local namespace that cannot interfere with the
other.
🤖 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.

Nitpick comments:
In `@ownlang/ownir.py`:
- Around line 438-445: The `if` handling in `_lower_flow` passes the same
`localmap` reference to both `then` and `else` branches, which will cause issues
when block-scoped locals are supported in future graduation. If the same
variable name is declared in both branches, the second branch would overwrite
the first in the shared `localmap`. To prepare for block-scoped local support,
create a shallow copy of `localmap` before each branch and pass the copy to the
respective `_lower_flow` calls for `then_b` and `else_b`, ensuring each branch
has its own isolated local namespace that cannot interfere with the other.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1b4fac91-cfe9-46b4-8764-c49d19f2a0f8

📥 Commits

Reviewing files that changed from the base of the PR and between 8323c77 and 5d8ca9b.

📒 Files selected for processing (5)
  • .github/workflows/ci.yml
  • docs/proposals/P-016-deep-fact-extraction.md
  • frontend/roslyn/OwnSharp.Extractor/Program.cs
  • frontend/roslyn/samples/FlowLocalsSample.cs
  • ownlang/ownir.py

…e), exempt dispose-optional types, default the wrappers to flow
@PhysShell PhysShell merged commit 336281e into main Jun 16, 2026
17 checks passed
This was referenced Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant