feat(d5.4): alias_join primitive + per-RID T4 wrap/adopt (step 1)#133
Conversation
D5.4 step 1 — "add the alias_join lowering". Builds on step 0's RID indirection to model T4 (wrap/adopt): a factory returning a wrapper of an argument, or a ctor adopting an argument into an owning field. T4a (return-aliases-arg) and T4b (stored-in-owning-field) are the same fact — a new owning handle joins the arg's alias set — so this is ONE core primitive with two extractor recognisers (the recognisers land in step 2). Core primitive (AST + CFG + analysis): - New `AliasJoin(handle, src)` instruction. `handle` becomes a new owning alias of `src`'s RID: the two handles denote one obligation; `src` stays owning (unlike `move`, which invalidates the source). `analysis.step` just points `handle_rid[id(handle)]` at `src`'s RID — no new resource is minted. - Because state lives on the RID (step 0), the existing per-RID checks now do T4 for free: releasing/escaping through EITHER alias discharges the one resource (clean), a leak of the shared RID is reported ONCE (not per-handle), releasing BOTH is OWN003, and using either after release is OWN002. - Wired through build_cfg (`lower_alias_join`: src must be owned; handle inherits src's origin/type/kind so a per-RID finding attributes to the one resource) and codegen (`var name = src;` owning-alias emission, for exhaustiveness). Bridge (ownir.py): - New `alias_join` flow op (`var` = new owning handle, `src` = adopted local) lowers to the core primitive. An alias_join over an untracked `src` makes no claim — optimistic-silent, the v1 must-only rule (§11). The Dapper precision win is proven: a factory that acquires `inner` locally, alias_joins a wrapper `w`, and RETURNS `w` drops `inner` as a local but its obligation escaped through `w` — so per-RID evaluation reports NO leak. That is the own-only-0-with-a-reason case the whole D5.4 model exists to model. Tests: test_ownir.py gains 7 D5.4 cases (release-wrapper / release-inner clean, drop-both leaks once @ the inner origin, double-release OWN003, use-after OWN002, untracked-src silent, return-wrapper precision). Full suite green; the entire pre-existing corpus is unchanged (AliasJoin is only emitted by the new flow op). Limitation documented: alias_join is straight-line only in v1 (the conflicting- merge raise keeps a branched alias loud rather than silently wrong). Refs docs/notes/d5-ownership-transfer.md §7, §11 (D5.4 step 1). 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)
📝 WalkthroughWalkthroughAdds ChangesAliasJoin ownership flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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://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: 355a932fd6
ℹ️ 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".
| # tracked local; an unknown `src` makes no claim (precision-first). | ||
| name = str(n.get("var", "?")) | ||
| src_h = localmap.get(str(n.get("src", ""))) | ||
| if src_h is not None and name not in hoisted: |
There was a problem hiding this comment.
When an alias_join assigns to a local whose src is untracked, this guard skips the whole operation without removing any existing localmap entry for var. In a flow such as acquire w; alias_join w <- ghost; release w, the release is incorrectly applied to the old w obligation, so the overwritten resource is reported clean instead of leaking. Please kill the stale binding even when the new alias cannot be tracked.
Useful? React with 👍 / 👎.
| # NOT touch `src`'s state (it stays owning, unlike a move). If `src` was | ||
| # already released/escaped, point at its RID anyway so a later use/release | ||
| # of `handle` resolves to that (released) RID and reports correctly. | ||
| st.handle_rid[id(ins.handle)] = st.rid_of(ins.src) |
There was a problem hiding this comment.
After this maps the new handle onto src's RID, active loans are still recorded and queried by the original handle id (Loan.owner / loans_on), not by RID. That means alias_join w inner; borrow inner as b { release w; } is accepted even though releasing another owning alias of the borrowed resource should be OWN008, and the same gap applies to mutable-borrow/use checks through the other alias. The loan owner needs to resolve through the shared RID once aliases exist.
Useful? React with 👍 / 👎.
…ed RID (Codex) Two Codex P2 findings on #133, both real and both precision-safe (they close recall gaps; behaviour in the 1:1 non-alias case is unchanged): 1. alias_join overwrite with an untracked src (ownir.py). `acquire w; alias_join w <- ghost; release w` overwrote `w` with an alias whose src is not tracked, but the guard skipped the whole op WITHOUT dropping the stale `localmap[w]` — so the later `release w` discharged the original obligation and the overwritten resource read clean instead of leaking. Fix: drop the stale binding FIRST (same rule as the D5.2 call-result overwrite), even when the new alias cannot be tracked — the original now leaks OWN001. 2. alias loans follow the shared RID (analysis.py). A loan recorded its owner by handle id, so a borrow of one owning alias was invisible to the other: `alias_join w inner; borrow inner { release w }` missed OWN008, and the mut-borrow/use path missed OWN013. Fix: BorrowStart records the owner by `rid_of(owner)` and loans_on queries by `rid_of(owner)`, so a loan is seen through every owning alias of the resource. In the 1:1 case rid_of is identity, so the whole existing corpus is unchanged. Tests: test_ownir.py gains the untracked-src overwrite leak; test_rid.py gains two direct-CFG cases (release an owning alias of a borrowed RID -> OWN008; use an owning alias of a mut-borrowed RID -> OWN013) — the bridge has no borrow op, so the alias<->loan interaction is exercised on a hand-built CFG. Full suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/codegen.py`:
- Around line 453-463: Clear any existing buffer bookkeeping for `st.name`
before creating the alias in the `A.AliasJoin` handling inside `codegen.py`;
this branch currently copies `buffer_cleanup` and `buffer_vars` from `st.src`
but does not remove stale state already attached to `st.name`, so a later
`release` can use the wrong cleanup. Mirror the shadowing cleanup behavior used
in `_stmt_inline(A.Let)` by dropping any previous `buffer_cleanup`/`buffer_vars`
entry for `st.name` before rebinding it, then continue copying the shared
obligation state from `st.src` as before.
🪄 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: b644f419-dbc4-4afd-b332-424946138a16
📒 Files selected for processing (8)
docs/notes/d5-ownership-transfer.mdownlang/analysis.pyownlang/ast_nodes.pyownlang/cfg.pyownlang/codegen.pyownlang/ownir.pytests/test_ownir.pytests/test_rid.py
…gen (CodeRabbit) CodeRabbit Major on #133: the `A.AliasJoin` codegen branch copied buffer_cleanup/ buffer_vars from `src` but did not drop any stale entry already attached to `st.name`. If the same name previously belonged to a branchy buffer, a later `release {st.name}` could emit that stale buffer cleanup instead of disposing the aliased resource. Mirror the shadowing-cleanup the Let branch already does: pop buffer_cleanup/buffer_vars for `st.name` before rebinding. (Codegen-only; no surface syntax emits AliasJoin yet, but kept consistent and correct.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
2 participants
D5.4 step 1 — the
alias_joinloweringBuilds on step 0's RID indirection (#129) to model T4 wrap/adopt: a factory returning a wrapper of an argument, or a ctor adopting an argument into an owning field. T4a (return-aliases-arg) and T4b (stored-in-owning-field) are the same fact — a new owning handle joins the arg's alias set — so this is one core primitive with two extractor recognisers (the recognisers land in step 2).
Core primitive (AST + CFG + analysis)
AliasJoin(handle, src)instruction.handlebecomes a new owning alias ofsrc's RID: the two handles denote one obligation;srcstays owning (unlikemove, which invalidates the source).analysis.stepjust pointshandle_rid[id(handle)]atsrc's RID — no new resource is minted.OWN003OWN002build_cfg(lower_alias_join:srcmust be owned;handleinheritssrc's origin/type/kind so a per-RID finding attributes to the one resource) andcodegen(var name = src;owning-alias emission, for exhaustiveness).Bridge (
ownir.py)alias_joinflow op (var= new owning handle,src= adopted local) lowers to the core primitive. Analias_joinover an untrackedsrcmakes no claim — optimistic-silent, the v1 must-only rule (§11).The Dapper precision win (proven)
A factory that acquires
innerlocally,alias_joins a wrapperw, and returnswdropsinneras a local — but its obligation escaped throughw, so per-RID evaluation reports no leak. That is the own-only-0-with-a-reason case the whole D5.4 model exists for. Without the alias set this dropped local would be a falseOWN001.Tests
test_ownir.pygains 7 D5.4 cases (185/185 bridge checks): release-wrapper / release-inner clean, drop-both leaks once @ the inner origin, double-releaseOWN003, use-afterOWN002, untracked-src silent, and the return-wrapper precision win. Full suite green; ruff + mypy--strictclean. The entire pre-existing corpus is unchanged —AliasJoinis only emitted by the new flow op.Honest scope
alias_joinforBulkheadSemaphoreFactory-shaped adoption). So this fires on hand-authored facts, not real C# yet.ifthat merges with a non-aliasing path is out of scope; the conflicting-merge raise in_join_handle_ridkeeps that loud, not silently wrong.Refs
docs/notes/d5-ownership-transfer.md§7, §11 (D5.4 step 1).🤖 Generated with Claude Code
https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests