Milestone 8.1b: ownership pass + move tracking#79
Merged
Conversation
The ownership pass (M8.1b) reserves E0020/E0021/E0022 per spec. Move ImmutableAssign/DuplicateDeclaration/UndefinedAssignTarget to E0028/E0029/E0030. Pure label change — pre-alpha, no external consumers of the codes yet.
First token reserved for ownership annotations on parameters. Used in M8.1b to mark parameters as taking ownership.
Defaults to false at every construction site. Parser will emit true when 'move' precedes the parameter name (next task).
fn consume(move s: str): ... — sets is_move=true on the Param. Bare params remain is_move=false (the default-borrow case).
AstGen propagates the AST flag into UIR. Sema and the ownership pass will read it next.
Sema propagates the UIR flag into TIR. The ownership pass will read it to distinguish move-consumption from borrow.
W0002 RedundantMove fires when 'move' precedes a parameter of Copy type (int, float, bool). Move on str remains silent. Warnings now render on the success path of lower_and_analyze.
Module is a no-op for now. Sits between sema and codegen, consumes &[Tir] and a DiagSink, mutates nothing. Subsequent tasks fill in the lattice and forward walk.
Adds is_copy_type/is_move_type/needs_tracking, the OwnerState enum (NotTracked/Valid/Borrowed/Moved), MoveKind, and the per-function Ownership state container. No walk yet — lattice plumbing only.
Walks each TIR statement-by-statement. Allocating instructions (StrConst, StrConcat, str-returning Call) populate `states` with Valid; Var reads populate `origin` with the current owner. Parameter refs use a synthetic high-end TirRef encoding so parameters live in the same lattice as instructions. Consumption logic (VarDecl, Assign, Return, move-Call) lands in subsequent tasks.
E0020 UseAfterMove, E0021 MoveOutOfBorrowedParam, E0022 ReturnBorrowedValue, W0001 DeadStore. Codes are wired through diag_code_str but the ownership pass does not emit them yet.
VarDecl and Assign of Move-typed values consume their initializer. Emits E0020 UseAfterMove on consumption of an already-Moved underlying owner, and E0021 MoveOutOfBorrowedParam on consumption of a Borrowed parameter. Concat reads operands as borrows (no state change), per Rule 2. Var reads now also surface E0020 when the aliased owner is already Moved, matching the spec algorithm at design.md:446. After a consume, the destination binding is reseated to a fresh lattice slot (origin severed, state Valid) so subsequent reads of the new binding are valid while reads of the source binding trip the use-after-move check.
Return of Move-typed expr consumes its operand. Borrowed parameters cannot be returned (E0022). Call arguments check the callee's per-parameter is_move flag: move params consume, default params borrow (and verify the underlying owner is not already Moved). Builtins (print, assert, int_to_str) borrow all str args by convention since they have no entry in by_name.
The borrow-position UAM check in the Call arm fired alongside the Var arm's check, producing two E0020 diagnostics for one fault. The Var arm already covers all current cases (every str arg flows through Var), so drop the call-site duplicate. Tighten test_use_after_move_param to assert exactly one E0020 occurrence so this regression is caught if it returns.
Snapshots state before each branch, walks branches independently, merges Moved conservatively (any branch Moved => merged Moved). Catches conditional use-after-move where one branch consumed and control reconverged before a use.
WhileLoop and ForRange run a 2-iteration fixed point. If any Move-tracked TirRef changed Moved-ness between the entry and post-body state, re-walk the body from the merged state and emit diagnostics on the second pass. Catches use-after-move across the back-edge. Break/continue are no-ops in 8.1b (8.1c attaches Free metadata).
Emits W0001 when a Move-typed binding is declared but never read or consumed. Falls out of the forward walk: pending declarations are cleared on Var reads and on consumption; whatever remains at function end is dead.
E0020/E0021/E0022 messages now name the involved binding, include a secondary label at the move site (Diag::with_note), and a help line suggesting the fix (move keyword for E0021, borrow-instead for E0020, locally-allocated value for E0022).
The Var arm already fires E0020 when reading a moved owner. The consume sites (VarDecl/Assign/Return) emitting again produced double diagnostics for one fault — same class as the Call-arm bug fixed in e3562e8. Also tighten MoveKind: MoveParam no longer carries an unused callee StringId payload, and the doc-comment now reflects that no diagnostic actually reads back the variant today.
- Drop MoveKind enum and OwnerState::Moved.kind (no diagnostic read it). - Add Diag::with_help, replace inline 'help: ' prefix. - Inline clone_snapshot/restore_from; let merge_branches take &[&Ownership] to remove redundant clones in loop fixed-points. - Replace HashSet allocations in merge_branches and states_differ with single-pass iteration. - Hoist Copy-type classification to InternPool::is_copy; eliminates duplicate between sema and ownership. - Drop stale #[allow(dead_code)] annotations on lattice items now that all variants are live.
merge_branches kept pre-branch current_owner mappings untouched via entry().or_insert(), so post-merge reads of a binding resolved to the pre-branch owner regardless of any branch reseats. That produced one false negative (then-branch consumes a reseat, else-branch leaves the original valid -> use after merge missed the conditional move) and two false positives (loop and full conditional reassignments where every path ends Valid still resolved to a Moved pre-branch owner). Fix: snapshot pre-branch (name, owner_pre) pairs before the merge, then for each pair compute the merged state via each branch's end-of-branch owner (b.current_owner[name]) and write it back onto owner_pre in self.states. Branch-local introductions (names declared inside a branch) keep merging via the existing entry().or_insert() path.
lower_and_analyze had duplicated render blocks for the success and error paths after the M8.1b warning-on-success work; ir_command silently dropped warnings on the success path. Refactor both to a single tail block via finalize_diags: drain the sink, render whatever is there, return Err iff any errors are present. Surfaces W0001/W0002 on `ryo ir` runs the same way they appear on `ryo run`. Resolves ISSUES.md I-044.
…ction consume_for_assignment and analyze_return ran the same four-arm state match (Valid/Borrowed/Moved/NotTracked) with only the Borrowed-arm diagnostic differing (E0021 vs E0022). The recent fix in 2c5985d (drop redundant E0020 at consume sites) had to be applied in both places — exactly the divergence risk dedup prevents. Extract consume_underlying parameterized by a BorrowedAction enum; analyze_return delegates instead of duplicating. Diagnostic wording, help text, and binding-name rendering are preserved verbatim. Resolves ISSUES.md I-052.
Tracks the follow-up items surfaced by the four-angle /simplify review of the ownership pass: scratch-sink loop shape (I-045), UIR is_move pass-through (I-047), Call arg_modes in TIR (I-048), Owner enum to replace synthetic_param_ref (I-049), UAM altitude (I-050), loop-helper duplication (I-051), and the parameter-only Borrowed state (I-053). I-044 (ir_command warning render) and I-046 (CompoundAssign no-op arm) and I-052 (consume/return state-match dedup) were resolved in 20cdd02 and 5f04567 and are not added here per the repo convention of removing resolved entries.
Ensure variable reassignments (via Assign) are registered in the pending_dead_store map so that we correctly report dead store warnings (W0001) for Move-typed values that are overwritten or never consumed.
F1: finalize_diags now reads sink.has_errors() before draining instead of re-scanning the returned Vec<Diag>. The sink already maintains an O(1) error counter; the post-drain scan was a redundant linear pass. S3: Replace the documentation-only CompoundAssign no-op arm with an enforcing debug_assert! over the operand type. The previous arm pattern-matched the tag and did nothing; if a future sema relaxation lets a Move-typed compound-assign reach the ownership pass, the assertion fires in debug builds instead of silently falling through to no analysis. File new follow-ups in ISSUES.md: I-054 (parse_source + lex paths still hand-roll the render+wrap pattern that finalize_diags centralizes), I-055 (pending_dead_store.insert duplicated between analyze_var_decl and analyze_assign).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds explicit ChangesMove Semantics and Ownership Analysis
Sequence DiagramsequenceDiagram
participant Check as ownership::check
participant Sema as sema::analyze_function
participant Walk as ownership::visit_expr
participant Consume as ownership::consume_underlying
participant Merge as ownership::merge_two
Check->>Sema: iterate Tir functions
Sema->>Walk: walk statements and expressions
Walk->>Consume: at move sites (VarDecl/Assign/Call/Return)
Consume->>Consume: transition OwnerState (Valid/Borrowed/Moved)
Walk->>Merge: at conditional/loop joins
Merge->>Merge: lattice merge / fixed-point
Check->>Check: emit diagnostics (UseAfterMove/DeadStore/RedundantMove)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (1)
src/ownership.rs (1)
603-604: ⚡ Quick winClarify the 2-iteration fixed-point claim.
The comment states "Converges in at most 2 iterations for the M8.1 pattern set," but the implementation is a 2-pass approximation rather than a true fixed-point loop. While this is likely correct for the current language subset (monotonic merge + no iteration-count-dependent conditionals), a brief inline note explaining why 2 passes suffice would help future maintainers when extending the analysis.
📝 Suggested clarification
- // First pass into a scratch sink — diagnostics are kept only if - // a single iteration suffices. If the lattice changes we re-walk - // and the scratch diags are dropped (the second walk re-emits - // anything that's still wrong). + // Two-pass approximation: first pass discovers moves, second pass + // (if lattice changed) catches use-after-move across the back-edge. + // Sufficient for M8.1 because the merge is monotonic (once Moved, + // always Moved) and the language has no iteration-count-dependent + // conditionals. First-pass diags are kept only if no re-walk needed.🤖 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 `@src/ownership.rs` around lines 603 - 604, The inline comment claiming "Converges in at most 2 iterations for the M8.1 pattern set" is misleading because the implementation is a 2‑pass approximation rather than a true fixed‑point loop; update the comment near that string to (1) rename it to "2‑pass approximation" or similar, (2) state the exact reason two passes suffice for the current M8.1 subset (monotonic merge operations and absence of iteration‑count‑dependent conditionals so propagation stabilizes after one propagation step), and (3) note this assumption so future maintainers updating the analysis (e.g., when adding non‑monotonic merges or iteration‑dependent control flow) know to revert to a full fixed‑point iteration. Include the original phrase "Converges in at most 2 iterations for the M8.1 pattern set" so reviewers can see the prior claim and briefly mention the needed conditions (monotonicity, no iteration-dependent conditionals).
🤖 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 `@ISSUES.md`:
- Around line 249-257: The ISSUES.md ordering is broken: I-053 appears after
I-055; update the file so issue IDs are monotonically increasing by either
moving the I-053 entry to appear before I-055 or renaming I-053 to the next
available ID (e.g., I-056) and update any internal references; ensure the file
preserves the "I-XXX" sequence and adjust any cross-references to I-053/I-055
accordingly.
In `@src/astgen.rs`:
- Line 194: In UIR lowering, UirParam.span is currently set to p.name.span which
drops the span for annotations like move; change the assignment that sets
UirParam.span (in the lowering code handling parameters) to use p.span instead
of p.name.span so the full parsed parameter span (including move annotation) is
preserved for downstream diagnostics.
In `@src/ownership.rs`:
- Around line 250-253: The subtraction in synthetic_param_ref (u32::MAX -
name.raw()) can wrap to 0 and produce TirRef(0); change it to use checked or
saturating arithmetic (e.g., u32::MAX.saturating_sub(name.raw()) or checked_sub
with a fallback) so the result never becomes 0, then call TirRef::from_raw on
the safe non-zero value; ensure you handle the edge case when name.raw() ==
u32::MAX by producing a non-zero sentinel-safe value.
In `@tests/integration_tests.rs`:
- Around line 2461-2468: The test currently only checks that stderr contains
exactly one "E0020" but doesn't assert the process failed; update the tests that
call run_ryo_command (the ones producing the output variable for
uam_vardecl_one.ryo and the similar test mentioned) to also assert the command
exited with a non-success status by checking output.status and asserting it is
not successful (e.g., assert!(!output.status.success(), "...") or equivalent)
before or alongside the existing stderr/E0020 count checks so the test enforces
E0020 remains a failing error.
---
Nitpick comments:
In `@src/ownership.rs`:
- Around line 603-604: The inline comment claiming "Converges in at most 2
iterations for the M8.1 pattern set" is misleading because the implementation is
a 2‑pass approximation rather than a true fixed‑point loop; update the comment
near that string to (1) rename it to "2‑pass approximation" or similar, (2)
state the exact reason two passes suffice for the current M8.1 subset (monotonic
merge operations and absence of iteration‑count‑dependent conditionals so
propagation stabilizes after one propagation step), and (3) note this assumption
so future maintainers updating the analysis (e.g., when adding non‑monotonic
merges or iteration‑dependent control flow) know to revert to a full fixed‑point
iteration. Include the original phrase "Converges in at most 2 iterations for
the M8.1 pattern set" so reviewers can see the prior claim and briefly mention
the needed conditions (monotonicity, no iteration-dependent conditionals).
🪄 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: 8f549881-bc7d-44d9-954f-ff03ba105246
📒 Files selected for processing (14)
ISSUES.mdsrc/ast.rssrc/astgen.rssrc/diag.rssrc/lexer.rssrc/main.rssrc/ownership.rssrc/parser.rssrc/pipeline.rssrc/sema.rssrc/tir.rssrc/types.rssrc/uir.rstests/integration_tests.rs
- ISSUES.md: reorder so I-053 lands before I-054/I-055; the prior placement broke the monotonic-id convention. No external references to update. - astgen.rs: UirParam.span now uses p.span (the full parsed parameter span including a `move` annotation) instead of p.name.span, which dropped the `move` keyword from downstream diagnostics. - ownership.rs: synthetic_param_ref guards against the edge case name.raw() == u32::MAX, which would have produced a zero raw and panicked NonZeroU32. Switch to saturating_sub + .max(1). - ownership.rs: rephrase the analyze_while_loop comment from "converges in at most 2 iterations" (misleading — the impl is a 2-pass approximation, not a true fixed-point loop) to spell out *why* two passes suffice for the M8.1 pattern set (monotonic Moved-ness sub-lattice, no iteration-count-dependent control flow) and what would force a revert to a real fixed-point loop. - integration_tests.rs: test_use_after_move_in_vardecl_one_diag and test_use_after_move_in_return_one_diag now also assert output.status.success() == false; the previous assertions passed even if E0020 had become a warning.
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
1 participant
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.
Adds a post-sema, pre-codegen ownership pass that validates move safety. Catches use-after-move at compile time, introduces the
movekeyword for parameters, and classifies types as Copy or Move.Diagnostics
moveon Copy-typed parameter)Multi-label rendering: binding name in primary message, secondary label at the move site, help text for the suggested fix.
Scope
movekeyword threaded lexer → parser → AST → UIR → TIRTirRefownership lattice (NotTracked / Valid / Borrowed / Moved)E0020/E0021/E0022codes renumbered toE0028/E0029/E0030Strings still leak —
Freeinsertion lands in 8.1c.Test plan
cargo fmt --checkcargo clippy --all-targets -- -Dwarningscargo test(286 unit + 142 integration, 0 failed)m81b_ok.ryo)m81b_bad.ryo)Summary by CodeRabbit
New Features
moveparameter modifier and ownership analysis to validate move/borrow safety and surface related warnings/errors (use-after-move, invalid returns, dead-store).Bug Fixes
Tests
Documentation