feat(d5): D5.2 — T1 fresh-returning factories become acquire sites#116
Conversation
Infer a method's owned-return kind in _build_skeletons: a body that acquires a local and returns it is fresh (a factory); a single returned local that is the result of a first-party call is a forward-return the solver propagates. Caller- side, a call op that binds a result whose callee summary returns fresh is also lowered to an acquire of that local, so the existing leak / double-release / use-after-release checks apply at the call site -- the factory-leak class, lost before D5.2. Precision-first: a returned parameter is never fresh (that is wrap/alias, T4/D5.4), and a non-fresh/unknown return makes no claim, so a result is never falsely owned. The out/ref-owned door is noted as a later T1 slice. Tests: factory-result leak (OWN001 at the call), disposed-clean, use-after- dispose (OWN002), forward-return propagation, param-return precision guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
|
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)
📝 WalkthroughWalkthroughThe D5.2 slice adds return-ownership inference, lowers fresh call results to call-site acquires, and expands the roadmap note and tests for forwarding, disposal, use-after-release, and parameter-return precision. ChangesFresh-return ownership transfer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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: 0bf537b3ee
ℹ️ 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".
| localmap: dict[str, str], | ||
| released_vars: set[str]) -> list[Stmt]: | ||
| released_vars: set[str], | ||
| mos: dict[str, Any] | None = None) -> list[Stmt]: |
There was a problem hiding this comment.
When a fresh-returning factory call is inside an if or while body, the recursive _lower_flow calls still omit this new mos argument, so those nested invocations run with mos=None and skip the D5.2 acquire for call ... result. I checked an if branch containing {"op":"call","callee":"make","result":"r"} where make returns a freshly acquired local, and check_facts returns no OWN001, while the same top-level call is reported; pass mos through the recursive branch/loop calls so factory leaks in control-flow bodies are not silently missed.
Useful? React with 👍 / 👎.
…while are caught (Codex P2) The recursive _lower_flow calls for if/then/else and while bodies dropped the new mos argument, so a fresh-returning factory call inside control flow ran with mos=None and silently skipped the D5.2 acquire -- a factory leak in a branch/loop went unreported. Pass mos through all three recursive calls. Adds a regression test: a fresh factory call in an if branch whose result is never disposed must leak OWN001 at the call site, like the top-level case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
![coderabbitai[bot]](https://cdn.github.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 `@ownlang/ownir.py`:
- Around line 974-984: The freshness inference in ownir.py’s return
classification is too permissive because it treats any returned variable found
in acquire tracking as fresh even when that local can also come from a call
result on another path. Tighten the logic in the returned/acquired check before
returning ReturnSkeleton("fresh") so it only marks fresh when the returned
locals are exclusively acquired and not mixed with _call_result_callees-derived
values or other origins; use the existing returned, acquired, and
_call_result_callees helpers around the ReturnSkeleton branch to make the
decision path-sensitive.
- Around line 1253-1265: Pass the solved summary map through recursive lowering
in `_lower_flow()` so fresh-result handling works in nested bodies. The `if` and
`while` branches currently recurse without `mos`, which means `call` operations
with a `result` inside nested flows never become `acquire` statements. Update
the recursive calls in `_lower_flow()` to forward `mos` consistently, and verify
the fresh-result logic still applies for nested `then`/`else` and loop bodies.
🪄 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: 765d8f3e-fad8-4e56-b06d-a364327e8584
📒 Files selected for processing (3)
docs/notes/d5-ownership-transfer.mdownlang/ownir.pytests/test_ownir.py
…it Major) _infer_return_skeleton classified a returned local as fresh whenever it appeared in any acquire, even if the same local is also a call result on another path (if c: x = acquire() else: x = other()). That would make a caller acquire a value it does not own on the non-acquire path, fabricating OWN001/OWN002 there. Require every returned local to be acquired AND not also a call result before claiming fresh; otherwise degrade to no-claim (precision-first). Adds a regression test: a method that acquires x then overwrites it with a non- owned call result and returns it is not fresh, so a caller's dropped result is silent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
|
Both Major findings from the earlier reviews are addressed on the latest commit
CI is fully green on @coderabbitai review Generated by Claude Code |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f85ecea1cd
ℹ️ 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".
| if all(v in acquired and v not in param_names and v not in call_results | ||
| for v in returned): |
There was a problem hiding this comment.
When a method has one branch returning an acquired local and another branch returning no owned value (for example a null/default result represented as return without var), _collect_vars only records the acquired branch, so this condition marks the whole method as fresh. A caller that drops that result is then reported as OWN001 even though the callee can return a non-owned value on another normal path; this violates the precision-first rule that fresh means every returned value is newly owned.
Useful? React with 👍 / 👎.
| and summ is not None and getattr(summ, "returns", None) == "fresh"): | ||
| handle = f"loc_{loc[0]}" | ||
| loc[0] += 1 | ||
| localmap[result] = handle |
There was a problem hiding this comment.
When a fresh factory result is assigned inside an if/else and then disposed after the branch (e.g. both branches assign r = make(); followed by release r), this assignment leaves localmap pointing at a synthetic Let declared inside one branch. The later release is emitted outside that branch scope, so CFG lowering reports OWN030 undefined name and check_facts raises OwnIRError instead of accepting a balanced factory result.
Useful? React with 👍 / 👎.
_infer_return_skeleton only collected returns carrying a var, so a method that returns an acquired local on one branch but a bare return (return null / non- owned) on another was still marked fresh -- a caller dropping its result was charged a bogus OWN001 on the null path. Add _has_bare_return: if any return path yields a non-owned value, make no fresh/forward claim (precision-first). Regression test: a method with a return-null branch is not fresh, so a caller's dropped result is silent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
…l regression Codex flagged that a local acquired in both if-branches and released after the merge crashes (OWN030 -> OwnIRError). Confirmed pre-existing: it reproduces with a plain acquire, no fresh/factory path, so it is a bridge flat-localmap branch- scope limitation, not a D5.2 regression. Per the decision, land D5.2 as-is and track the bridge fix as its own slice (make acquire lowering branch-aware / hoist synthetic handles to the merge scope) rather than soft-skipping OWN030, which would mask real lowering drift. Records the limitation in the D5 note and adds an xfail-style lock (branch_merge) asserting the current raise, to flip to a clean-balanced-release assertion once the bridge fix lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
|
Both Codex findings addressed: 1. "Require every return path to be fresh" (line 984) — valid, fixed in 2. "Preserve factory result locals across branch merges" (line 1348) — confirmed real, but pre-existing and not a D5.2 regression: the same Decision (per maintainer): land D5.2 as-is and track the bridge fix as its own slice — make Thanks for both catches — #1 was a genuine precision bug. Generated by Claude Code |
2 participants
P-005 D5.2 — T1 return-value door
A factory leak —
var r = MakeReader(); /* never disposed */— was silently lost before this slice. T1 closes it: afresh-returning call becomes an acquire site, so the existing leak / double-release / use-after-release checks apply to the result.What ships
_build_skeletonsnow derives a method's owned-return kind via_infer_return_skeleton:acquires a local and returns it →fresh(a factory)call→forward-return, which the D5.0 solver propagates (factory-of-factory)_lower_flow, acallop that binds aresultwhose callee summary returnsfreshis also lowered to anacquireof that local. The MOS is threaded into_lower_flowexactly as_lower_fn_paramsalready receives it. The args' effects (if any) are still applied by the emittedCall; the new acquire models the return.new T()is already anacquireop from the extractor; this slice adds the first-party factory-method door.Precision-first (the floor stays
own-only 0)fresh— that is wrap/alias (T4 / D5.4). No false acquire of the result, no consume of the arg.Tests (139 → 144 bridge checks)
freshpropagates through a forward-return factory-of-factory → caller OWN001fresh→ silent (precision guard)Full suite green (
run_tests.pyexit 0),ruff+mypy --strictclean.Out of scope (noted in the design note)
The
out/ref-owned parameter — the otherfreshsource — needs extractor-side recognition of an out-assignment as a fresh acquire; it rides into a later T1 slice before async.🤖 Generated with Claude Code
https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
Generated by Claude Code
Summary by CodeRabbit