P-005 D5.3 / P1a: curated BCL fresh-factory table (producer side of Tier B)#127
Conversation
…ier B) C+D first slice — the producer half of the boundary contract. A leaked BCL factory result (`var s = File.OpenRead(p)`) was invisible because there is no first-party body to infer `fresh` from; now a curated table makes it owned. - ownir.py: `_BCL_FRESH_FACTORIES` (File.OpenRead/OpenText/OpenWrite/Open/Create/ CreateText/AppendText) + `_is_bcl_fresh_factory` (matches `Type.Method`, so a namespace-qualified callee resolves on its last two segments). New `_callee_returns_fresh` centralises the "does this call yield a fresh owned result?" decision (Tier A first-party summary OR Tier B BCL table) and is now the single source of truth shared by the leak pre-scan, the branch-hoist safety walk, and the flow lowering — so all three agree. - A leaked factory result is OWN001 at the call; disposed is clean; use-after-dispose is OWN002; it composes with the P-015 codeFlows slice (the finding anchors at the factory call). Pure factories only — overload-ambiguous wrappers that adopt an arg (`new StreamReader(stream)`) stay out (sink/T4). - The leaveOpen *sink* breadth rides the existing $consume/$borrow channel and is extractor-side (the bool literal is a per-call-site fact), so it is CI/C#-only; documented as the remaining half in d5-ownership-transfer.md §7. Tests: leak / disposed-clean / use-after-dispose / namespace-qualified / a non-disposable `File.ReadAllText` making no claim. ownir 174/174; ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughOwnIR now treats curated BCL factory calls as fresh-owned ChangesTier B BCL factory ownership
Sequence Diagram(s)sequenceDiagram
participant tests as tests/test_ownir.py
participant ownir as ownlang/ownir.py
participant fresh as _callee_returns_fresh
participant hoist as acquires(...)
participant lower as _lower_flow
tests->>ownir: run Tier B BCL scenarios
ownir->>fresh: classify File.OpenRead / System.IO.File.Create
fresh-->>ownir: fresh-owned result or not
ownir->>hoist: reuse fresh classification for hoist safety
ownir->>lower: lower tracked result as acquire
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76d29aff32
ℹ️ 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 (callee in _BCL_FRESH_FACTORIES | ||
| or ".".join(callee.split(".")[-2:]) in _BCL_FRESH_FACTORIES) |
There was a problem hiding this comment.
Require System.IO before applying File factory table
When a solution has any source-visible method/type ending in File.OpenRead/File.Create outside the BCL (for example MyCompany.File.OpenRead returning a plain value), this suffix match still treats it as a BCL factory and _lower_flow mints an owned IDisposable result, producing false OWN001/OWN002 reports. The table is meant to model System.IO.File only, so this should either require the System.IO.File.* identity or let an exact first-party summary override the Tier-B shortcut before applying the suffix match.
Useful? React with 👍 / 👎.
| if _is_bcl_fresh_factory(callee): | ||
| return True |
There was a problem hiding this comment.
Propagate BCL freshness into return summaries
When a first-party method just returns a BCL factory result, e.g. s = File.OpenRead(p); return s;, the callee itself is clean because this helper makes the call acquire-and-return the handle, but _infer_return_skeleton still records the method as a forward to external File.OpenRead; the solver then resolves that external forward to unknown, so callers that drop Make() get no OWN001. The new BCL freshness knowledge needs to be used while building return skeletons as well, otherwise simple wrappers around these factories remain invisible.
Useful? React with 👍 / 👎.
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 `@ownlang/ownir.py`:
- Around line 1114-1129: Restrict the Tier B branch in `_callee_returns_fresh()`
/ `_is_bcl_fresh_factory()` so only real `System.IO.File` factory calls are
treated as fresh, instead of matching any callee whose last two segments look
like `File.OpenRead` or `File.Create`. Update the BCL factory check to require
the full `System.IO.File`-based symbol (or an equivalent strict namespace/type
match) before returning true, so first-party and third-party `File.*` methods
still fall through to the `mos` summary path and do not get mis-lowered or
trigger OWN001/OWN002/hoist behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a1a3d67f-2061-4e4b-8c86-c3a4942fa62c
📒 Files selected for processing (3)
docs/notes/d5-ownership-transfer.mdownlang/ownir.pytests/test_ownir.py
…s (Codex #127) Two Codex P2s on the BCL fresh-factory table: - PRECISION: the match was a loose last-two-segments suffix, so an external `MyCompany.File.OpenRead` returning a plain value would be treated as a BCL factory and fabricate a false OWN001/OWN002. Now matches ONLY the bare `File.Method` or the fully-qualified `System.IO.File.Method` identity. Also flipped `_callee_returns_fresh` so a first-party summary is authoritative and OVERRIDES the table (a callee whose body we can see is never given a fabricated `fresh`). - RECALL: a first-party wrapper `Make(){ return File.OpenRead(p) }` recorded a `forward` to the external factory, which the solver degraded to `unknown`, so a dropped `Make()` leaked invisibly. `_infer_return_skeleton` now classifies such a return as `fresh`, so the caller is charged the leak. Tests: non-System.IO look-alike rejected, first-party override, wrapper-fresh recall. ownir 177/177; ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
…bbit #127) CodeRabbit marked the Tier-B precision concern addressed by 52a562e; this folds in its remaining suggestion — strip an optional `global::` qualifier before the match, so `global::System.IO.File.OpenRead` resolves as the BCL identity while a `global::`-qualified non-System.IO look-alike is still rejected. Tests for both. ownir 177/177; ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
What & why
The "C + D" slice — and it turns out C (D5.3 BCL pack) and D (P1a resource contracts) are the same track: Tier B's curated contract table. This ships the producer half: a leaked BCL factory result was invisible because there's no first-party body to infer
freshfrom.var s = File.OpenRead(p)(never disposed) now surfaces as a real leak.(The sink half — the
leaveOpenconsume/borrow breadth — already rides the existing$consume/$borrowchannel from D5.1b, and its breadth is a C#-extractor table since the bool literal is a per-call-site fact; documented as the remaining, CI/C#-only half.)Changes (
ownlang/ownir.py)_BCL_FRESH_FACTORIES— curated table of pure BCL factories whose return the caller owns (File.OpenRead/OpenText/OpenWrite/Open/Create/CreateText/AppendText). Keyed byType.Method;_is_bcl_fresh_factorymatches a namespace-qualified callee on its last two segments (System.IO.File.OpenRead✓)._callee_returns_fresh— new single source of truth for "does this call yield a fresh owned result?" (Tier A first-party summary or Tier B BCL table), now shared by the leak pre-scan, the branch-hoist safety walk, and the flow lowering — so all three agree.callto a known factory mints the result as afreshacquire via the existing T1 path. Pure factories only — overload-ambiguous wrappers that adopt an arg (new StreamReader(stream)) are excluded (that's the sink / T4 case).Behaviour (verified)
Composes with the P-015 codeFlows slice (the leak anchors at the factory call).
Testing
python tests/run_tests.py→ green (ownir: 174/174,diagnostics: PASS);ruff+mypyclean. New tests cover leak / disposed-clean / use-after-dispose / namespace-qualified / non-disposable-makes-no-claim.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
IDisposable-like results, improving leak and use-after-release detection.Documentation
leaveOpenbehavior.Tests
System.IO.File.*factory call sites, including namespace,global::qualification, non-disposable calls, and first-party override scenarios.