fix: P0 issues for m8.4.2 (I-112/I-113, I-081, I-083, I-114, I-115) - #106
Conversation
…2, I-113) Extract emit_sret_str_call helper shared by all four sret arms (eval_inst_str + emit_call, __ryo_str_from_view + int/float/bool formatters). The formatter bare-statement arm had drifted: it never cached the loaded triple in inst_values, so the ownership pass's scheduled anon-temp Free could not resolve the buffer and tripped the scalar-Free guard. Both statement arms now cache the triple. Regression tests: bare int_to_str/float_to_str/bool_to_str statements. Entries removed from ISSUES.md per convention.
emit_stmt/emit_body/emit_scoped_body/generate_if_stmt now return
Terminator::{None, Return, Break, Continue} instead of a bool that
treated break/continue as 'returns'. generate_if_stmt tracks
all_terminated (merge-block reachability) and all_return (what the if
reports to its caller) as separate questions; emit_body stops the walk
on any terminator, and the function-level fallthrough check tests
== Return only.
Behavior-preserving: full integration suite 187/187. Entry removed
from ISSUES.md per convention.
…I-083) The scalar entry point silently returned a str's data pointer for str-typed insts. Now eval_inst rejects them with an internal error at three points: the memo hit, an entry guard on inst type, and the Call arm. The two legitimate multi-word consumers are rerouted: - __ryo_panic's StrConst message goes through the new emit_strconst_rodata_ptr helper (the one deliberate exception: the runtime's scalar ptr+len ABI) - ExprStmt of str-typed operands goes through eval_inst_str, and eval_inst_str's user-call arm calls emit_call directly With the dummy gone the shared-memo ordering hazard is enforced loudly (a wrong-entry hit errors instead of corrupting), so the memo split from the original resolution is unnecessary. The memo insert is now a plain insert since no scalar path can cache mid-evaluation. Verified: integration 187/187, frontend+backend 402/402, asan 27/27. Entry removed from ISSUES.md per convention.
Mirror the Rule-7 partition's P6' look-through in the E0031 note-span finder: when the arg is a ViewAsStr borrow, resolve projection_root first, falling back to underlying_owner. Regression test pins both notes for two(move s, s[0:1]). Verified: ryo-frontend lib 394 passed. Removes I-114 from ISSUES.md.
…I-115) Promote the debug_assert in both accessors to a hard assert! whose message names the contract and includes the raw ref, so a violation in a release build fails clearly instead of a cryptic index-out-of-bounds. Doc comments pin the invariant; two should_panic tests guard the message. Verified: ryo-core 62, ryo-frontend 394, ryo-backend 9, integration 187/187. Removes I-115 from ISSUES.md.
ISSUES.md entries are removed as they get fixed, so I-0xx references in comments, doc comments, and panic/assert messages would dangle. Where an ID was the whole explanation, the text now states the why directly; message strings only lost their ID tails. Verified: cargo check --workspace, ryo-core 62, ryo-frontend 394, ryo-backend 9, integration 187/187.
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe compiler now propagates explicit termination states, handles string-returning calls through dedicated multi-value paths, validates TIR parameter sentinels unconditionally, and resolves ChangesCode generation and semantic invariants
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant compile_function
participant emit_body
participant emit_stmt
participant cleanup
compile_function->>emit_body: Emit function body
emit_body->>emit_stmt: Emit each statement
emit_stmt->>cleanup: Schedule cleanup before loop jumps
emit_stmt-->>emit_body: Return a Terminator
emit_body-->>compile_function: Return final Terminator
compile_function->>compile_function: Add implicit return only for normal completion
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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: 2
🧹 Nitpick comments (3)
ryo-backend/src/codegen.rs (1)
2338-2375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate bare-statement str-call branches in
emit_call.
emit_stmtroutes str-typed expression statements througheval_inst_str, andeval_inst_stralready handles__ryo_str_from_viewand the formatter builtins.emit_stmtno longer callsemit_call, so these branches inryo-backend/src/codegen.rsare dead code. Keep the dispatch ineval_inst_stronly.🤖 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 2338 - 2375, Remove the __ryo_str_from_view and formatter-builtin branches from emit_call, since emit_stmt now routes string expression statements through eval_inst_str. Keep their existing handling exclusively in eval_inst_str and leave other emit_call dispatch behavior unchanged.ryo-frontend/src/ownership.rs (1)
5749-5795: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
borrowed herenote span.All instructions use
0..0, so the test verifies only the note text. A regression that attaches the note to the move argument would still pass. Give the move argument andreborrowdistinct spans, then assert that theborrowed herenote usestir.span(reborrow).🤖 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 5749 - 5795, Update move_and_viewasstr_borrow_of_one_owner_reports_borrow_note to assign distinct spans to the move argument and reborrow expressions, then assert the “borrowed here” note span equals tir.span(reborrow). Keep the existing diagnostic-count and note-message assertions while ensuring the note is attached to the reborrow rather than the move argument.ryo-core/src/tir.rs (1)
1358-1380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify the release-profile regression coverage.
These
#[should_panic]tests pass with bothassert!anddebug_assert!in the default debug test profile. They do not prove the release-mode contract. If CI does not already run the workspace tests withcargo test --release, add that check or a dedicated release-profile test.The PR objective requires these checks to remain active in release 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-core/src/tir.rs` around lines 1358 - 1380, Ensure the regression tests inst_rejects_param_sentinel_with_clear_message and span_rejects_param_sentinel_with_clear_message execute under a release profile, such as by adding a cargo test --release workspace CI check or equivalent dedicated release test. Verify the sentinel rejection remains active and produces the expected panic messages in release builds.
🤖 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 `@ryo-backend/src/codegen.rs`:
- Around line 896-900: Update the operand dispatch in the ExprStmt lowering path
to detect view-typed expressions and route them through eval_inst_view, while
preserving eval_inst_str for string types and eval_inst for other scalar types.
Use the operand type available through ctx.tir.inst(operand).ty and ensure
discarded view expressions are evaluated without triggering scalar
ValueRepr::View errors.
In `@ryo-core/src/tir.rs`:
- Around line 377-395: Update Tir::inst and Tir::span to assert that the TirRef
raw value is nonzero before indexing, while preserving their existing
param-sentinel checks. Add regression tests covering TirRef::from_raw(0) for
both accessors, verifying they reject the reserved arena slot in debug/test
execution.
---
Nitpick comments:
In `@ryo-backend/src/codegen.rs`:
- Around line 2338-2375: Remove the __ryo_str_from_view and formatter-builtin
branches from emit_call, since emit_stmt now routes string expression statements
through eval_inst_str. Keep their existing handling exclusively in eval_inst_str
and leave other emit_call dispatch behavior unchanged.
In `@ryo-core/src/tir.rs`:
- Around line 1358-1380: Ensure the regression tests
inst_rejects_param_sentinel_with_clear_message and
span_rejects_param_sentinel_with_clear_message execute under a release profile,
such as by adding a cargo test --release workspace CI check or equivalent
dedicated release test. Verify the sentinel rejection remains active and
produces the expected panic messages in release builds.
In `@ryo-frontend/src/ownership.rs`:
- Around line 5749-5795: Update
move_and_viewasstr_borrow_of_one_owner_reports_borrow_note to assign distinct
spans to the move argument and reborrow expressions, then assert the “borrowed
here” note span equals tir.span(reborrow). Keep the existing diagnostic-count
and note-message assertions while ensuring the note is attached to the reborrow
rather than the move argument.
🪄 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 Plus
Run ID: 60d4c64e-2220-4370-b842-d3c5dcdb7688
📒 Files selected for processing (9)
ISSUES.mdryo-backend/src/codegen.rsryo-core/src/tir.rsryo-core/src/types.rsryo-core/src/uir.rsryo-driver/src/pipeline.rsryo-frontend/src/builtins.rsryo-frontend/src/ownership.rsryo/tests/integration_tests.rs
💤 Files with no reviewable changes (1)
- ISSUES.md
| pub fn inst(&self, r: TirRef) -> &TypedInst { | ||
| debug_assert!(!r.is_param(), "Tir::inst called with a param sentinel ref"); | ||
| assert!( | ||
| !r.is_param(), | ||
| "Tir::inst called with a param sentinel ref (raw={}); \ | ||
| param sentinels are not instruction indices", | ||
| r.raw() | ||
| ); | ||
| &self.instructions[r.index()] | ||
| } | ||
|
|
||
| /// Look up the source span for `r`. Same param-sentinel contract | ||
| /// as [`Tir::inst`]. | ||
| pub fn span(&self, r: TirRef) -> Span { | ||
| debug_assert!(!r.is_param(), "Tir::span called with a param sentinel ref"); | ||
| assert!( | ||
| !r.is_param(), | ||
| "Tir::span called with a param sentinel ref (raw={}); \ | ||
| param sentinels are not instruction indices", | ||
| r.raw() | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject the reserved zero reference before indexing.
The new assertion rejects TirRef::param, but it does not reject TirRef::from_raw(0). The TIR contract reserves arena slot 0. If raw zero reaches either accessor, r.index() can read the reserved entry as a real instruction or span in release builds. Add a hard zero-sentinel assertion to both methods and add regression tests.
Proposed fix
assert!(
!r.is_param(),
"Tir::inst called with a param sentinel ref (raw={}); \
param sentinels are not instruction indices",
r.raw()
);
+ assert!(
+ r.raw() != 0,
+ "Tir::inst called with the reserved sentinel ref (raw=0)"
+ );
&self.instructions[r.index()]
@@
assert!(
!r.is_param(),
"Tir::span called with a param sentinel ref (raw={}); \
param sentinels are not instruction indices",
r.raw()
);
+ assert!(
+ r.raw() != 0,
+ "Tir::span called with the reserved sentinel ref (raw=0)"
+ );
self.spans[r.index()]As per coding guidelines: “never emit or treat slot 0 as a real instruction.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn inst(&self, r: TirRef) -> &TypedInst { | |
| debug_assert!(!r.is_param(), "Tir::inst called with a param sentinel ref"); | |
| assert!( | |
| !r.is_param(), | |
| "Tir::inst called with a param sentinel ref (raw={}); \ | |
| param sentinels are not instruction indices", | |
| r.raw() | |
| ); | |
| &self.instructions[r.index()] | |
| } | |
| /// Look up the source span for `r`. Same param-sentinel contract | |
| /// as [`Tir::inst`]. | |
| pub fn span(&self, r: TirRef) -> Span { | |
| debug_assert!(!r.is_param(), "Tir::span called with a param sentinel ref"); | |
| assert!( | |
| !r.is_param(), | |
| "Tir::span called with a param sentinel ref (raw={}); \ | |
| param sentinels are not instruction indices", | |
| r.raw() | |
| ); | |
| pub fn inst(&self, r: TirRef) -> &TypedInst { | |
| assert!( | |
| !r.is_param(), | |
| "Tir::inst called with a param sentinel ref (raw={}); \ | |
| param sentinels are not instruction indices", | |
| r.raw() | |
| ); | |
| assert!( | |
| r.raw() != 0, | |
| "Tir::inst called with the reserved sentinel ref (raw=0)" | |
| ); | |
| &self.instructions[r.index()] | |
| } | |
| /// Look up the source span for `r`. Same param-sentinel contract | |
| /// as [`Tir::inst`]. | |
| pub fn span(&self, r: TirRef) -> Span { | |
| assert!( | |
| !r.is_param(), | |
| "Tir::span called with a param sentinel ref (raw={}); \ | |
| param sentinels are not instruction indices", | |
| r.raw() | |
| ); | |
| assert!( | |
| r.raw() != 0, | |
| "Tir::span called with the reserved sentinel ref (raw=0)" | |
| ); | |
| self.spans[r.index()] |
🤖 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 377 - 395, Update Tir::inst and Tir::span
to assert that the TirRef raw value is nonzero before indexing, while preserving
their existing param-sentinel checks. Add regression tests covering
TirRef::from_raw(0) for both accessors, verifying they reject the reserved arena
slot in debug/test execution.
Source: Coding guidelines
…st hardening - ExprStmt lowering routes view-typed operands (bare slices) through eval_inst_view; previously they hit the scalar-path rejection. Regression: test_bare_slice_statement (red before the fix). - emit_call: remove the provably dead __ryo_str_from_view and formatter-builtin branches — eval_inst guards str/view-returning calls out of the scalar path, and eval_inst_str's Call arm handles those builtins inline, so no call site can reach them. - Tir::inst docs note the NonZeroU32 newtype makes raw-0 refs unconstructible (no in-accessor guard needed); new test pins from_raw(0) rejection at construction. CI gains a release-profile run of the tir sentinel tests on the ubuntu lane so the assert!-in- release guarantee is exercised. - move_and_viewasstr_borrow_of_one_owner_reports_borrow_note: distinct spans for the move arg and reborrow chain; assert the 'borrowed here' note attaches to tir.span(reborrow). - cargo fmt rewraps. Verified: fmt/clippy(-Dwarnings) clean, ryo-core 63, ryo-frontend 394, ryo-backend 9, integration 188/188, release tir::tests 14.
One conflict in ownership.rs (check_use_moved doc): kept the spec citation from main without the issue-ID reference, per this branch's convention. The I-127 SAFETY-comment links stay — R5 requires a linked issue for in-tree unsafe and I-127 is still open. Verified post-merge: fmt/clippy(-Dwarnings, new workspace deny lints) clean, ryo-core 63, ryo-frontend 394, ryo-backend 9, integration 188/188.
1 participant
Summary
Fixes all P0 issues from the m8.4.2 audit on one branch, one commit per issue:
emit_sret_str_callhelper in codegen (dedup of 4 sret arms); the formatter bare-statement arm now caches the triple ininst_values(the actual I-112 bug). 3 regression integration tests.Terminator {None, Return, Break, Continue}enum replaces the bool that conflated block-terminated with returned;generate_if_stmtnow tracksall_terminatedvsall_returnseparately.eval_instrejects str/view-typed insts withErrinstead of a dummy scalar;__ryo_panic's StrConst arg goes throughemit_strconst_rodata_ptr;ExprStmtroutes str-typed operands toeval_inst_str.two(move s, s[0:1]).Tir::inst/Tir::spanpromote the param-sentineldebug_assert!to a hardassert!with a clear release-mode message.I-0xxreferences from comments/messages (entries are removed from ISSUES.md as fixed).Verification
cargo test -p ryo --test integration_tests— 187/187cargo test -p ryo-frontend --lib— 394/394cargo test -p ryo-core --lib— 62/62cargo test -p ryo-backend --lib— 9/9cargo test -p ryo --test asan_smoke— 27/27Summary by CodeRabbit
Bug Fixes
Documentation
Tests