feat(d5.4): RID indirection in the core flow analysis (step 0, no-op)#129
Merged
Conversation
D5.4 (T4 wrap/adopt) lands in a three-commit cadence so the core change is
de-risked. This is step 0: the no-op identity refactor that moves resource
state from per-binding to per-RID, with a 1:1 handle->RID mapping. Behaviour
is byte-for-byte unchanged — the whole green corpus is the proof — and step 1
(alias_join: a second owning handle joins an existing RID) now has a tested
foundation instead of having to re-derive the invariant under a behavioural
change.
What changed in ownlang/analysis.py:
- State.var is now keyed by RID (resource id), not by handle identity. A RID is
the obligation carrying the {OWNED,MOVED,RELEASED,ESCAPED} state; a handle
(local/param Symbol) denotes a RID through the new State.handle_rid map.
- State.rid_of(sym) resolves a handle to its RID, defaulting to id(sym) for an
un-aliased handle. Choosing RID == id(sym) is what makes this a no-op: every
var key is the same int it was before, so _sym_by_id resolves a RID straight
back to its symbol and leak attribution is unchanged.
- State.mint(sym) binds a handle to a fresh resource (its own RID) — the 1:1
acquire. Every var write (Acquire/AcquireBuffer/MoveInto-dst/owned-param)
goes through mint; every var read/overwrite (Release/Return/consume/state
checks) through rid_of. Loans stay keyed by handle id (borrow machinery is
not in the alias-set model in v1, per the design's owning-only constraint).
- join() now also joins handle_rid via _join_handle_rid, which unions agreeing
maps and asserts on a conflicting one — locking the step-0 single-mapping
invariant the same way join already locks the block-scoped-loan invariant.
tests/test_rid.py (new, wired into run_tests): pins rid_of's 1:1 default, mint's
recording, the join union/assert, and end-to-end that a 1:1 acquire still leaks
(OWN001) / releases clean / double-releases (OWN003) — i.e. the indirection
changed nothing observable.
Refs docs/notes/d5-ownership-transfer.md sections 7 and 11 (D5.4 step 0).
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)
📝 WalkthroughWalkthroughOwnership analysis now resolves symbol state by RID instead of handle identity. The transfer, release, return, leak, and escape paths were updated, and a new RID test suite was added to the test runner. ChangesRID indirection in ownership analysis
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 1
🧹 Nitpick comments (1)
tests/test_rid.py (1)
118-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a RID regression for the return/escape path.
These end-to-end checks cover leak/release, but the refactor also changed
Returnand the leak exclusion logic inownlang/analysis.py. A bug in the RID-based escape path would still pass this suite, despite the step-0 “no behavior change” goal.🤖 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 `@tests/test_rid.py` around lines 118 - 129, Add a regression in tests/test_rid.py that exercises the RID-based return/escape path, since the current end-to-end cases only cover acquire/release and can miss bugs in the Return flow and leak exclusion logic in ownlang/analysis.py. Extend the existing _codes/_check-style assertions with a small function that returns or escapes a connection so you can verify the expected RID outcome stays unchanged after the refactor. Reference the Return handling in ownlang/analysis.py and the test helpers _codes and _check to keep the new case aligned with the current suite.
🤖 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/analysis.py`:
- Around line 137-141: The RID-merge check in the handle-to-RID merge logic
relies on assert, which is stripped under optimized Python and can silently
preserve the wrong mapping. Replace the assert in the merge path around the
handle/out check with an explicit runtime validation that always executes and
raises a clear exception when a handle maps to two different RIDs, using the
existing merge code in analysis.py to keep the D5.4 step-0 invariant loud.
---
Nitpick comments:
In `@tests/test_rid.py`:
- Around line 118-129: Add a regression in tests/test_rid.py that exercises the
RID-based return/escape path, since the current end-to-end cases only cover
acquire/release and can miss bugs in the Return flow and leak exclusion logic in
ownlang/analysis.py. Extend the existing _codes/_check-style assertions with a
small function that returns or escapes a connection so you can verify the
expected RID outcome stays unchanged after the refactor. Reference the Return
handling in ownlang/analysis.py and the test helpers _codes and _check to keep
the new case aligned with the current suite.
🪄 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: 5013c9a4-4180-42a3-acca-fc797fd2dfa4
📒 Files selected for processing (3)
ownlang/analysis.pytests/run_tests.pytests/test_rid.py
…(CodeRabbit) CodeRabbit review on #129: - Minor: `_join_handle_rid` used `assert` for the conflicting-mapping invariant, which `python -O` strips — a wrong mapping would then be kept silently, defeating the "must be loud" step-0 contract. Replace with an explicit always-on `raise AssertionError` (same exception type, so any caller is unaffected). Verified it still raises under `python -O`. - Nitpick: the end-to-end RID checks only covered acquire/release, but the refactor also routes the `Return` state-write and `leak_check`'s `exclude` through `rid_of`. Add two regressions: returning an owned resource escapes its RID clean, and a *sibling* RID left owned still leaks OWN001 before the return — proving the exclude spares only the returned RID, not all of them. No behaviour change to the analysis; this hardens the invariant and the test net. 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_rid.py`:
- Line 145: The check-count summary is stale because `run()` now makes 12
`_check(...)` calls while `n` is still hardcoded to 13. Update the total used by
the `test_rid.py` runner logic to match the actual number of `_check`
invocations in `run()`, and preferably derive it from the executed checks
instead of keeping a fixed constant so it cannot drift again.
🪄 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: 47b8952f-b740-4b3c-93b3-52d8a204308f
📒 Files selected for processing (2)
ownlang/analysis.pytests/test_rid.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ownlang/analysis.py
…abbit) The return/escape regressions took the suite to 12 `_check` calls but the summary still printed a hardcoded `n = 13` — reporting a pass count that never ran. Per CodeRabbit's "better" suggestion, derive the total from the checks that actually executed: `_check` records each outcome in a module list and `run()` reports `len(_RAN)`, so adding or removing a check can never desync the count. 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.
D5.4 step 0 — the de-risking refactor (deliberately a no-op)
D5.4 (T4 wrap/adopt) is where Dapper/Polly-style "a factory returns a fresh disposable, the caller adopts it into an owning field" gets modeled. Per the design's three-commit cadence (
docs/notes/d5-ownership-transfer.md§7 + §11), the core change is split so it can't silently break everything. This is step 0: move resource state from per-binding to per-RID, 1:1, behaviour byte-for-byte unchanged.What changed (
ownlang/analysis.py)State.varis now keyed by RID (resource id), not by handle identity. A RID is the obligation carrying the{OWNED,MOVED,RELEASED,ESCAPED}state; a handle (local/paramSymbol) denotes a RID through the newState.handle_ridmap.State.rid_of(sym)resolves a handle to its RID, defaulting toid(sym)for an un-aliased handle. ChoosingRID == id(sym)is what makes this a no-op: everyvarkey is the same int it was before, so_sym_by_idresolves a RID straight back to its symbol and leak attribution is unchanged.State.mint(sym)binds a handle to a fresh resource (its own RID) — the 1:1 acquire. Everyvarwrite (Acquire/AcquireBuffer/MoveInto-dst/owned-param) goes throughmint; everyvarread/overwrite (Release/Return/consume/state checks) throughrid_of.join()now also joinshandle_ridvia_join_handle_rid, which unions agreeing maps and asserts on a conflicting one — locking the step-0 single-mapping invariant the same wayjoinalready locks the block-scoped-loan invariant.Why a no-op is the point
This PR delivers zero new detection — by design. It's pure indirection so step 1 (
alias_join: a second owning handle joins an existing RID; per-RID leak/double/use-after evaluation) is a small, safe diff against a tested invariant rather than a behavioural change. Step 2 then turns on the extractor branches that emitaliasOf:ifor verified wrapper/ctor-adopt sites (the BulkheadSemaphoreFactory shape the Polly oracle surfaced).Proof it changed nothing
The entire green corpus passes unchanged — 19/19 corpus, 6/6 wpf, 20/20 loops, 23/23 spec, 169 bridge, 37 D5.0. ruff + mypy
--strictclean.tests/test_rid.py(new, 11 checks, wired intorun_tests) pins the RID layer directly:rid_of1:1 default + no recording untilmintmintrecords the mapping and returnsid(sym); distinct handles → distinct RIDscopy()preserveshandle_rid_join_handle_ridunions disjoint/agreeing maps and asserts on a conflictOWN001, releases clean, double-releasesOWN003Refs
docs/notes/d5-ownership-transfer.md§7, §11 (D5.4 step 0).🤖 Generated with Claude Code
https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Tests