refactor: Milestone 8.2 — Implicit Borrow Liveness & Ownership Pass Refactors - #88
Conversation
…ncoding (I-049)
Replaces the u32::MAX - name.raw() param-key encoding with an explicit
Owner { Param(StringId), Inst(TirRef) } enum. Rekeys states,
current_owner, origin, pending_dead_store, owner_at_read, temp_owners.
Behavior-preserving; named_owners retained pending Task A2 (I-060).
…assifier (I-060) Replaces the sticky named_owners side-set with a stateless static derivation: collect_named_inits gathers the init/value TirRef of every VarDecl/Assign (recursing into control flow), and the anon-temp pass skips any temp in that set. Merge-immune where the current_owner.values() draft double-freed loop-rebound temps.
…eassign positive check
Encoding-only: call_extra gains a MODES slot per arg, CallView exposes modes, TirBuilder::call takes a &[ParamMode]. Sema stamps Move/Borrow from the callee signature; builtins are all-Borrow. Ownership still reads by_name until the next task.
…g (I-048) Drops the HashMap<StringId, &Tir> threaded through the ownership-pass signatures. The Call arm reads view.modes[i] directly. Builtins uniform. The __ryo_panic borrowed-scalar name-match is retained pending Task 5's ABI registry.
…(I-059) BuiltinFunction carries borrowed_scalar_params; ownership + codegen consult is_borrowed_scalar_param instead of pool.str(name) matching. Codegen's borrowed-scalar Scalar-target tripwire downgrades to debug_assert.
…ved (I-050) Introduces check_use_moved invoked at every use site (consume, borrow-arg, StrConcat, and recurse_operands UnOp/BinOp). The Var arm demotes to pure bookkeeping. Closes the gap where non-Var moved operands bypassed the check.
…_loop_body (I-061, I-051) Loop helpers and if-arm analysis snapshot only the non-monotone fields (states, current_owner, pending_dead_store); union-only fields (temp_owners, origin, owner_at_read, next_branch_id) stay live. The while/for near-clones collapse into analyze_loop_body.
The pre-loop owner defensive-emit fired on both break and continue, but freeing on continue is a use-after-free (next iteration re-reads the freed buffer). Gate on is_break; accept a potential leak over UAF until path-relative liveness lands.
Two-phase Call arm: materialise args, partition by underlying owner into borrowed/moved sets, reject overlap (E0023), then commit moves. Catches both f(read(x), consume(x)) and f(consume(x), read(x)) order-independently.
M8.2 delivered as implicit-borrow liveness + ownership-pass refactors (E0023 move-while-borrowed-in-call). Explicit & syntax is M8.3 per spec §5.2.
Adds #[allow(clippy::too_many_arguments)] on consume_underlying, and updates stale comments in ownership.rs and tir.rs to reflect the completed M8.2 architecture.
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds ownership modes to TIR calls, propagates them through semantic analysis and builtins, refactors ownership tracking around allocation owners, updates borrowed-scalar code generation, and substantially revises issue and roadmap documentation. ChangesOwnership-aware calls
Issue and roadmap documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SemanticAnalysis
participant TirBuilder
participant OwnershipAnalysis
participant Codegen
SemanticAnalysis->>TirBuilder: emit arguments with ParamMode values
TirBuilder->>OwnershipAnalysis: expose call modes
OwnershipAnalysis->>Codegen: schedule ownership-based frees
Codegen->>Codegen: handle borrowed-scalar free targets
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/dev/implementation_roadmap.md (3)
3391-3398: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd the required References footer.
This
docs/devdocument ends without a References section containing Spec, Dev, and Milestone links.As per coding guidelines, every docs/dev/**/*.md file must include a “References” footer with entries for Spec, Dev, and Milestone when applicable.
🤖 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 `@docs/dev/implementation_roadmap.md` around lines 3391 - 3398, Add a References footer to implementation_roadmap.md after the existing closing content, including the required Spec, Dev, and Milestone links using the document’s applicable project references and standard documentation format.Source: Coding guidelines
2117-2141: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the fenced Ryo example’s syntax and indentation.
The
if n <= 1line is missing its colon, and the example uses spaces instead of the required tab indentation, so it is not a valid copyable Ryo example.Proposed documentation fix
- if n <= 1 - return 1 + if n <= 1: + return 1As per coding guidelines, docs/**/*.md code examples must follow root CLAUDE.md with Python-style colons and tab indentation.
🤖 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 `@docs/dev/implementation_roadmap.md` around lines 2117 - 2141, Update the fenced Ryo example containing factorial and its tests: add the required colon to the if n <= 1 statement and convert all indentation within the code block from spaces to tabs, preserving the example’s behavior and structure.Source: Coding guidelines
888-894: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReconcile the documented
range()arities.This roadmap says both
range(end)andrange(start, end)are supported, whileISSUES.mdI-040 still says only the two-argument form is supported. Align the two documents with the implemented parser behavior.🤖 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 `@docs/dev/implementation_roadmap.md` around lines 888 - 894, Reconcile the documented range() arities between implementation_roadmap.md and ISSUES.md I-040. Inspect the parser behavior to determine the implemented supported forms, then update both documents consistently to describe only those forms and their loop-header-only usage.ryo-backend/src/codegen.rs (1)
1394-1435: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Scalartargets should not be debug-only. This branch is documented as an ownership-pass bug, butdebug_assert!disappears in release builds, so a bad schedule can be skipped without any diagnostic there. Keep the check unconditional or returnErrif you want the invariant enforced outside debug builds.🤖 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 `@ryo-backend/src/codegen.rs` around lines 1394 - 1435, Make the Scalar branch in emit_frees enforce the ownership invariant in all build modes instead of relying on debug_assert!. Return an Err with the existing diagnostic context, or otherwise perform an unconditional validation, and do not silently continue when a borrowed-scalar target is scheduled for freeing.ryo-driver/src/pipeline.rs (1)
237-247: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
E0023is duplicated.DiagCode::MoveWhileBorrowedInCallreuses the same stable code already assigned toDiagCode::FloatModulo; use a different unused code so the diagnostics stay unique.🤖 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 `@ryo-driver/src/pipeline.rs` around lines 237 - 247, Update the diagnostic-code mapping near DiagCode::MoveWhileBorrowedInCall so it no longer uses E0023, which is already assigned to DiagCode::FloatModulo. Assign a different unused stable code and preserve uniqueness across all DiagCode mappings.ryo-frontend/src/ownership.rs (1)
24-113: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMove parameters still need a free path
Owner::Paramis excluded from every free-scheduling pass, andpending_dead_storeis only populated forVarDecl/Assign. Amoveparameter that is never consumed — or only consumed on some branches — ends the function inValidstate with no scheduledFree, so the callee leaks the transferred allocation.🤖 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 `@ryo-frontend/src/ownership.rs` around lines 24 - 113, Add function-end free scheduling for unconsumed move parameters represented by Owner::Param, including parameters consumed only on some control-flow branches. Update the ownership finalization logic rather than relying solely on pending_dead_store, and schedule a Free only for paths where the parameter remains Valid, avoiding duplicate frees for Moved parameters.
🧹 Nitpick comments (2)
docs/dev/implementation_roadmap.md (1)
864-865: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse ATX syntax for this changed heading.
The changed setext-style heading is flagged by markdownlint (MD003). Convert it to an equivalent
#/##heading while preserving its level.🤖 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 `@docs/dev/implementation_roadmap.md` around lines 864 - 865, Convert the changed setext-style heading in the milestone validation section to equivalent ATX heading syntax, preserving its current heading level and text.Source: Linters/SAST tools
ryo-core/src/tir.rs (1)
568-594: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider
assert_eq!instead ofdebug_assert_eq!for the modes/args length invariant.
debug_assert_eq!is compiled out in release/optimized builds by default. If a future caller ever violatesmodes.len() == args.len()in a release build,lenhere is still computed ascall_extra::ARGS + 2 * args.len()— i.e. fromargs.len(), not from however many mode words were actually pushed. The declaredExtraRangewould then overrun into whatever gets appended toself.extranext, silently corrupting the decode of a subsequent instruction rather than failing loudly. All current callers (sema.rs) already sizemodescorrectly, so this is latent rather than actively triggered, but for a foundational,pubencoding primitive this seems worth hardening unconditionally.♻️ Proposed hardening
- debug_assert_eq!( - modes.len(), - args.len(), - "TirBuilder::call: one ParamMode per arg" - ); + assert_eq!( + modes.len(), + args.len(), + "TirBuilder::call: one ParamMode per arg" + );🤖 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 `@ryo-core/src/tir.rs` around lines 568 - 594, Replace the debug-only modes/args length check in TirBuilder::call with an unconditional assert_eq! so the invariant is enforced in release builds before encoding the call extra data. Keep the existing length calculation and argument/mode serialization unchanged.
🤖 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 `@docs/dev/implementation_roadmap.md`:
- Around line 1155-1165: Add Milestone 8.3 to the Dependencies list for the
milestone documenting the explicit &str borrow example, preserving the existing
M8.2 dependency and accurately reflecting the use of explicit & syntax in
first_word(&s).
- Around line 1674-1676: Update the Milestone 21 goal text so it does not claim
string slices shipped; describe `&str` as planned in M8.4, or consistently
update the earlier M8.4 status if it is actually complete.
- Around line 2273-2297: The v0.1 roadmap example uses deferred `catch` and
f-string syntax. Update the `main` example to handle `find_user(42)` with
v0.1-supported `match` and build the greeting with string concatenation,
preserving the demonstrated optional handling and error result.
- Around line 1059-1072: Update the consume_and_print example so its first
parameter is explicitly move-mode or otherwise owning, while keeping the second
parameter borrowing. Ensure the main call consume_and_print(msg, msg) accurately
demonstrates E0023 by moving msg into the first argument while it is still
implicitly borrowed for the second.
---
Outside diff comments:
In `@docs/dev/implementation_roadmap.md`:
- Around line 3391-3398: Add a References footer to implementation_roadmap.md
after the existing closing content, including the required Spec, Dev, and
Milestone links using the document’s applicable project references and standard
documentation format.
- Around line 2117-2141: Update the fenced Ryo example containing factorial and
its tests: add the required colon to the if n <= 1 statement and convert all
indentation within the code block from spaces to tabs, preserving the example’s
behavior and structure.
- Around line 888-894: Reconcile the documented range() arities between
implementation_roadmap.md and ISSUES.md I-040. Inspect the parser behavior to
determine the implemented supported forms, then update both documents
consistently to describe only those forms and their loop-header-only usage.
In `@ryo-backend/src/codegen.rs`:
- Around line 1394-1435: Make the Scalar branch in emit_frees enforce the
ownership invariant in all build modes instead of relying on debug_assert!.
Return an Err with the existing diagnostic context, or otherwise perform an
unconditional validation, and do not silently continue when a borrowed-scalar
target is scheduled for freeing.
In `@ryo-driver/src/pipeline.rs`:
- Around line 237-247: Update the diagnostic-code mapping near
DiagCode::MoveWhileBorrowedInCall so it no longer uses E0023, which is already
assigned to DiagCode::FloatModulo. Assign a different unused stable code and
preserve uniqueness across all DiagCode mappings.
In `@ryo-frontend/src/ownership.rs`:
- Around line 24-113: Add function-end free scheduling for unconsumed move
parameters represented by Owner::Param, including parameters consumed only on
some control-flow branches. Update the ownership finalization logic rather than
relying solely on pending_dead_store, and schedule a Free only for paths where
the parameter remains Valid, avoiding duplicate frees for Moved parameters.
---
Nitpick comments:
In `@docs/dev/implementation_roadmap.md`:
- Around line 864-865: Convert the changed setext-style heading in the milestone
validation section to equivalent ATX heading syntax, preserving its current
heading level and text.
In `@ryo-core/src/tir.rs`:
- Around line 568-594: Replace the debug-only modes/args length check in
TirBuilder::call with an unconditional assert_eq! so the invariant is enforced
in release builds before encoding the call extra data. Keep the existing length
calculation and argument/mode serialization unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe676e5c-2166-406d-888e-0329c6c92b7c
📒 Files selected for processing (9)
ISSUES.mddocs/dev/implementation_roadmap.mdryo-backend/src/codegen.rsryo-core/src/diag.rsryo-core/src/tir.rsryo-driver/src/pipeline.rsryo-frontend/src/builtins.rsryo-frontend/src/ownership.rsryo-frontend/src/sema.rs
…olve move parameter leak
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ryo-backend/src/codegen.rs (1)
1410-1413: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the documentation to reflect the new error behavior.
The doc comment explicitly claims that a
Scalar-cached target "trips adebug_assert!and is skipped rather than aborting codegen". However, the code below was changed to return anErr(...), which aborts compilation. Please update the documentation to match the current abort behavior.📝 Proposed fix for the doc comment
/// each index as fired in `ctx.freed_at`. A `Scalar`-cached target - /// (borrowed-scalar ABI, never heap-owned) trips a `debug_assert!` and - /// is skipped rather than aborting codegen — the ABI registry is + /// (borrowed-scalar ABI, never heap-owned) returns an `Err` and aborts + /// codegen — the ABI registry is /// supposed to keep such args out of `temp_owners`. See I-057/I-059.🤖 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 `@ryo-backend/src/codegen.rs` around lines 1410 - 1413, Update the doc comment immediately above the affected code to state that encountering a Scalar-cached target returns an error and aborts code generation, rather than triggering a debug assertion and being skipped. Preserve the existing ABI registry context and references while removing the inaccurate skip behavior description.
🧹 Nitpick comments (1)
ryo-backend/src/codegen.rs (1)
521-521: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEncapsulate the parameter
TirRefmapping.u32::MAX - idx as u32is a hidden convention here; a dedicated helper such asTirRef::param(idx)would make the ownership/codegen contract explicit and reduce the chance of divergence.🤖 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 `@ryo-backend/src/codegen.rs` at line 521, Encapsulate the parameter reference mapping used in the codegen path by adding or reusing a dedicated TirRef helper such as TirRef::param, and replace the inline u32::MAX - idx as u32 construction in the virtual_ref assignment with that helper. Keep the resulting parameter mapping unchanged while making the convention explicit.
🤖 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.
Outside diff comments:
In `@ryo-backend/src/codegen.rs`:
- Around line 1410-1413: Update the doc comment immediately above the affected
code to state that encountering a Scalar-cached target returns an error and
aborts code generation, rather than triggering a debug assertion and being
skipped. Preserve the existing ABI registry context and references while
removing the inaccurate skip behavior description.
---
Nitpick comments:
In `@ryo-backend/src/codegen.rs`:
- Line 521: Encapsulate the parameter reference mapping used in the codegen path
by adding or reusing a dedicated TirRef helper such as TirRef::param, and
replace the inline u32::MAX - idx as u32 construction in the virtual_ref
assignment with that helper. Keep the resulting parameter mapping unchanged
while making the convention explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 458a5500-11fe-46ca-a905-c8e4e51d26dd
📒 Files selected for processing (5)
docs/dev/implementation_roadmap.mdryo-backend/src/codegen.rsryo-core/src/tir.rsryo-driver/src/pipeline.rsryo-frontend/src/ownership.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- ryo-driver/src/pipeline.rs
- ryo-core/src/tir.rs
- docs/dev/implementation_roadmap.md
- ryo-frontend/src/ownership.rs
…rrow-liveness # Conflicts: # ISSUES.md # docs/dev/implementation_roadmap.md
6b5c7f3 to
57db0fd
Compare
1 participant
This PR fully implements Milestone 8.2 (Implicit Borrow Liveness & Ownership Pass Refactors) under Ryo’s implicit Rule 2 borrow model, delivering the core dataflow substrate for borrow-liveness and resolving 7 major architectural issues (
ISSUES.mdI-048/049/050/059/060/061/067).Key Deliverables:
Phase A — Architectural Refactors (8.2a):
OwnerEnum (I-049): Replaces brittle synthetic param-ref encodings with a type-safeOwnerenum.named_initsClassifier (I-060): Purgesnamed_ownersside-set, replacing it with a stateless, recursive, merge-immune scan ofVarDecl/Assigninitializers.ParamMode& Deletingby_name(I-048 / I-059): Carries parameter conventions in TIRCallpayloads; deletes theby_nameHashMap; replaces__ryo_panicstring-matching with a clean builtin ABI registry.Varreads to use-sitecheck_use_moved.analyze_loop_body.continueUAF Gate (I-067): Gates trailing defensive loop Free emits onis_break, preventing UAF oncontinuejumps.Phase B — Headline Check (8.2b):
Callargument evaluation (materialize first, partition, check overlap, then commit moves) to reject move-while-borrowed in the same call order-independently, with precise, multi-label Ariadne span notes pointing at the conflicting arguments.Closing:
ISSUES.mdand marked M8.2 complete inimplementation_roadmap.md.Verification:
cargo clippy --all-targets(zero warnings) andcargo fmt --checkare 100% green.Summary by CodeRabbit