fix(bridge): hoist cross-branch locals to outer scope (branch-scope OWN030 crash)#120
Merged
Conversation
…elease is clean A local acquired inside both if-branches (or via a fresh factory call) and released after the merge used to crash: the bridge emitted each synthetic Let inside its branch block, so the post-merge reference was out-of-scope and the core raised OWN030 -> OwnIRError. _hoisted_branch_locals now finds locals acquired at depth>=1 whose shallowest reference is at depth 0 (the function top, always reached), and to_module declares each once at the outer scope, skipping the in-branch acquire. A balanced cross-branch release is now CLEAN; an un-released one still leaks OWN001 (precision-safe: both the unconditional acquire and the depth-0 reference run on every path). Covers plain acquire and fresh call-result forms. We do NOT soft-skip OWN030 -- the strict map-or-raise invariant stays intact. A nested reference (released at depth>=1 in an enclosing block) is left as a narrower limitation -- function-top is not the common-dominator scope there, so hoisting would leak on the sibling path; locked by the nested_branch xfail test for a common-dominator follow-up. Tests: branch_merge (clean), branch_leak (OWN001), branch_factory (clean), nested_branch (still raises, locked). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
![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: 8bb8633ca8
ℹ️ 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".
Comment on lines
+1363
to
+1364
| elif op == "while": | ||
| walk(n.get("body"), depth + 1) |
There was a problem hiding this comment.
When a local is acquired in a while body and referenced after the loop, recursing here makes _hoisted_branch_locals mark it for function-scope hoisting. _lower_flow then skips every in-loop acquire, collapsing 0+ loop semantics into one unconditional acquire; for example while { acquire r } ; release r now lowers to a single top-level acquire/release and returns no findings, even though multiple iterations leak all earlier allocations. The existing loop analysis is fixpoint-sensitive, so loop-body acquires should not be hidden by this branch-merge hoist without a loop-aware model.
Useful? React with 👍 / 👎.
…(Codex P1)
The hoist is sound only for mutually-exclusive if branches: exactly one acquire
runs, so a single unconditional one is balanced. A while body is cumulative --
hoisting while { acquire r }; release r to one acquire collapsed 0..N iterations
and HID the per-iteration leak (returned no findings). _hoisted_branch_locals now
tracks loop-enclosed acquires and excludes them, so a loop-acquired local keeps
its pre-existing loud behaviour (OWN030 -> OwnIRError) instead of a silent false-
clean. A loop-aware model is a separate follow-up.
Test: loop_acq locks the raise (not a false-clean) and flips when that lands.
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR hoists selected branch-acquired locals to function scope, updates flow lowering to reuse those handles across branches, and refreshes tests and notes for balanced release, leak, factory-result, guard, one-branch, nested-branch, while, and pool behavior. ChangesBranch-scope hoisting in OwnIR lowering
Sequence Diagram(s)sequenceDiagram
participant to_module
participant _hoisted_branch_locals
participant _lower_flow
participant if_branch
participant release
to_module->>_hoisted_branch_locals: collect hoistable locals
to_module->>_lower_flow: lower flow with hoisted set
_lower_flow->>if_branch: lower branch body without duplicate acquire
if_branch-->>_lower_flow: return merged flow
_lower_flow->>release: emit post-merge release
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 |
![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 `@ownlang/ownir.py`:
- Around line 1309-1318: The hoisting logic in ownir.py is using depth-0
references as the only safety signal, but that is insufficient because a
top-level reference may still be unreachable on some branches. Update the hoist
predicate in the code that decides when to predeclare locals in to_module() so
it only hoists a name when every branch that reaches the merge either acquires
the local or exits, not merely when the shallowest reference is at depth 0. Use
the existing hoist/acquire/reference analysis around the logic described by the
hoisting rules to add a branch-coverage check before declaring the name at outer
scope.
- Around line 854-857: The branch-hoisting path is dropping the original pool
metadata by hardcoding the handle record to "pool": False, so hoisted acquires
no longer behave like normal pool rents. Update the hoist logic in ownir.py
where handles[hh] and hoisted_lets are created so it preserves the acquire’s
kind/mapping from the original branch-local handle, matching the normal acquire
handling used elsewhere (including the path around the standard pool mapping
logic). Ensure branch-hoisted ArrayPool rents remain marked as pooled buffers
instead of generic disposables.
🪄 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: 1d00c3aa-24d2-46f2-b632-f89ccc67e34f
📒 Files selected for processing (3)
docs/notes/d5-ownership-transfer.mdownlang/ownir.pytests/test_ownir.py
… pool kind (CodeRabbit) Two issues in the branch-scope hoist: 1) (Major) depth-0 reference is not sufficient. Hoisting makes a conditional acquire unconditional, so a branch that early-returns before the release on a path that did not acquire the local leaks the hoisted resource -- a FALSE OWN001 (if c: acquire r else: return; release r). Add _branch_hoist_safe, a definite-assignment walk (an if establishes acquisition only when both arms do, a while body never does, a non-discharging return on a not-yet-acquired path is unsafe); a name is hoisted only when it holds. The guard shape now stays a loud OWN030 raise instead of a fabricated finding. 2) (Minor) hoisted acquires hardcoded pool=False, dropping ArrayPool-rent metadata. Track the acquire kind and preserve it on the hoisted handle, so a branch-hoisted Rent still reports as a pooled buffer. Tests: guard (early-return -> not hoisted, no false finding), one_branch (no early return -> clean), pool_branch (pooled kind preserved). branch_merge / branch_factory / branch_leak unchanged. Full suite green (corpus 19/19), ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
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 1602-1605: The bridge branch-scope pool regression check is using
message text and a nonexistent pooled attribute instead of asserting the
structured Finding.kind value. Update the assertion in the test around the
pooled findings to compare the exact finding shape directly using the existing
finding objects (for example, the list of code/kind pairs) so it verifies the
preserved pooled buffer tag rather than wording.
🪄 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: c49c8cc5-7f87-45ac-8b82-2fb9177332a5
📒 Files selected for processing (3)
docs/notes/d5-ownership-transfer.mdownlang/ownir.pytests/test_ownir.py
✅ Files skipped from review due to trivial changes (1)
- docs/notes/d5-ownership-transfer.md
🚧 Files skipped from review as they are similar to previous changes (1)
- ownlang/ownir.py
…ession (CodeRabbit)
The pool-preservation test checked a nonexistent 'pooled' attribute or 'pool' in
the message wording, which could miss a metadata regression while passing on
text. Assert the exact shape instead: [(code, kind)] == [('OWN001', 'pooled
buffer')] -- dropping the pool flag lowers the hoisted handle to 'disposable',
which this now catches directly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bridge branch-scope fix
Closes the pre-existing crash surfaced (but not caused) by D5.2 — Codex P2 on #116.
The bug
A local acquired inside both branches of an
ifand released after the merge(
if c: r = acquire() else: r = acquire(); release r) crashedcheck_factswithOWN030 → OwnIRError. The core resolver is strictly lexical (anifpushes a scope perbranch and pops it at the merge), but the bridge emitted each synthetic
Letinside itsbranch block — so the post-merge
releaseresolved to an out-of-scope handle, the corereported OWN030 (undefined name), and the strict map-or-raise invariant raised.
This predates D5 — it reproduces with a plain
acquire(no factory path). D5.2'scall-result acquire only added another way to reach it. (Locked last PR by the
branch_mergexfail; this flips it.)
The fix
Make
acquirelowering branch-aware._hoisted_branch_localsfinds locals acquired atdepth ≥ 1 whose shallowest reference is at depth 0 (the function top, always reached);
to_moduledeclares each once at the function's outer scope (a singleLet) and skips thein-branch acquire. A balanced cross-branch release is now CLEAN; an un-released one still leaks
OWN001.
Precision-safe by construction: hoisting a conditional acquire to unconditional is balanced
because the depth-0 reference also runs on every path — never a fabricated use/double-release.
On the C# path where the value wasn't assigned it's null and
.Dispose()is a null-safe no-op,so clean is the correct verdict. Covers both the plain
acquireand thefreshcall-result form.We deliberately did not soft-skip OWN030 — that would mask genuine lowering drift; the strict
map-or-raise invariant stays load-bearing.
Scope / honesty
method scope, assigned in a branch, disposed at method level).
nested
if, released at depth 1 in the enclosing block) — function-top isn't thecommon-dominator scope there, so an unconditional top-level acquire would leak on the sibling
path (a false OWN001). The hoist deliberately doesn't fire; the OWN030 raise persists. Correct
fix = hoist to the common-dominator block, tracked for a follow-up and locked by the
nested_branchxfail test.Tests (148 → 151 bridge checks)
branch_merge(clean) ·branch_leak(OWN001 — leak still caught) ·branch_factory(freshcall-result form, clean) ·
nested_branch(still raises, xfail-locked for the follow-up). Fullsuite green (corpus 19/19, wpf, lifetimes, loops, spec),
ruff+mypy --strictclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
Generated by Claude Code
Summary by CodeRabbit