fix: return-flow analysis — MissingReturn (E0036) for non-void falloff (I-031+I-071) - #109
Merged
Conversation
…f (I-031+I-071) Sema now verifies that a non-void function returns (or diverges via `never`) on every path through its body, closing both directions of the gap: - I-071: a non-void fn that falls off the end got no diagnostic and codegen silently fell through — now rejected with E0036. - I-031: the analysis is branch-aware, so exhaustive if/elif/else chains where every arm returns are accepted (no spurious diag). Implementation: `Tir::block_definitely_returns` (structural, lives with the other TIR reachability helpers) + a DiagCode::MissingReturn check after TirBuilder::finish in analyze_function_body. Loops are conservative (body can run zero times); `panic` tails count as diverging via the never-typed ExprStmt operand; the Unreachable error-recovery sentinel suppresses cascades, while genuine falloff is still reported alongside unrelated errors. Removes the I-031 and I-071 entries from ISSUES.md (incl. the Fix Order section).
|
Warning Review limit reached
Next review available in: 53 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. 📝 WalkthroughWalkthroughThe compiler now validates ChangesNever-value and return-flow validation
Windows CI workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant analyze_function
participant Tir
participant Diagnostic
analyze_function->>Tir: Finish function TIR
analyze_function->>Tir: Call block_definitely_returns
Tir-->>analyze_function: Return-flow result
analyze_function->>Diagnostic: Emit MissingReturn for fall-through
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
Merging this PR will improve performance by 11.61%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
|
Contributor
Author
|
@coderabbitai 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 `@ryo-core/src/tir.rs`:
- Around line 1372-1395: Extend stmt_definitely_returns to classify VarDecl,
Assign, and CompoundAssign statements as definitely returning when their
evaluated initializer or value operand has a never type, matching the existing
ExprStmt handling. Preserve current behavior for non-never operands and add
regression coverage for never-typed initializer and assignment forms.
🪄 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: 2e12bc46-69a1-44f0-9804-ee23b0448707
📒 Files selected for processing (5)
ISSUES.mdryo-core/src/diag.rsryo-core/src/tir.rsryo-driver/src/pipeline.rsryo-frontend/src/sema.rs
… guards Review feedback on the return-flow change, verified against current code (both directions were real): - `stmt_definitely_returns` now also treats VarDecl / Assign / CompoundAssign with a `never`-typed initializer/value as diverging (matching the existing ExprStmt rule) — previously `x = panic(..)` in a non-void fn drew a spurious MissingReturn (E0036). - Accepting those forms unmasked a latent codegen crash family: the never-call eval emits an I8 dummy in a sealed dead block post-trap, and storing it into an int/float/str/strview local tripped the Cranelift verifier (or hit unreachable arms in eval_inst_str / eval_inst_view). emit_stmt now guards all binding-statement paths: evaluate for the trap, then bind zero placeholders (VarDecl) or skip the dead store (Assign / CompoundAssign). Tests: 3 sema regression tests for the divergence classification; 6 integration tests (red-first) covering scalar/str/strview VarDecl, scalar/str/strview Assign, and CompoundAssign forms end-to-end. The integration helper asserts on the user panic message — "panicked" alone also matches a crashing compiler's Rust panic text.
Language decision: a `never` value cannot be bound or assigned —
`x = panic("boom")`, `x: int = panic(...)`, `x = panic(...)`,
`x += panic(...)` are compile errors, superseding the previous
commit's accept-and-guard approach.
- New `check_bindable_value` helper in sema extends the existing
void-RHS rejection to `never` at all four binding sites (var decl,
assign-to-existing, implicit decl, compound assign), reusing
DiagCode::VoidValueInExpression (doc updated to cover both).
`return panic(...)` stays legal — the idiomatic diverging form.
- Reverted the emit_stmt dead-block guards and zero_const: sema's
rejection makes those TIR shapes unreachable in codegen (the
driver short-circuits on errors), and codegen.rs is byte-identical
to main again.
- Kept the tir.rs VarDecl/Assign/CompoundAssign divergence arms:
error-recovery TIR still contains a never-initialized VarDecl, and
the arms suppress a cascading MissingReturn on top of the real
error (locked in by the sema tests).
- Tests: 3 sema divergence tests rewritten as rejection tests (+ a
new return_panic_accepted); the 6 integration tests now assert the
E0017 compile error across scalar/str/strview forms.
- Filed I-130: `never` in other operand positions (binop operands,
call args) still reaches codegen and dies with an internal error —
needs a reject-or-define decision.
Language decision (extending the never-binding rejection): a
`never` value may ONLY appear as a bare statement. `return
panic("x")`, `return 1 + panic("x")`, `f(panic("x"))` —
and uniformly every other operand position (conditions, slice/range
bounds) — are compile errors, resolving I-130 with the reject option
(the entry is removed; it never left this branch).
Implementation: `analyze_expr` is now a thin wrapper that rejects a
never-typed result with E0017 ("a 'never' value (e.g.
`panic(...)`) can only be used as a statement") and recovers with
the error sentinel. All recursive descent goes through it, so the
rule covers every operand position with one check. The old body is
`analyze_expr_allow_never`, called from exactly four sites: the
bare ExprStmt (the one legal never position) and the three binding
arms, which run their own bind-specific check_bindable_value so
`x = panic(...)` keeps the 'cannot bind' message and its
cascade-free recovery shape.
Tests: return_panic_accepted flipped to return_panic_rejected; new
sema tests for binop-operand and call-arg rejection; 3 new
integration tests (return / binop / call-arg) reusing the renamed
assert_never_rejected helper. All existing panic tests already used
statement form — no other fallout.
…§4.2, §7.6) Records the language decisions from this branch in the spec, per the ISSUES.md convention that language-visible resolutions live there: - New §6.1.3 Return Checking: non-void functions must return on every path (E0036); the exhaustive if/elif/else, trailing-return, loop conservatism, and panic-divergence rules with examples. - §4.2 never entry and §7.6 Panic Behavior: a never value is a bare statement only — it cannot be bound, returned, passed, or used as an operand (E0017) — and a bare panic satisfies return-flow.
Quick win (~50s of the windows leg's ~4:30): delete the windows-only `cargo check --workspace --all-targets` step; `cargo test --workspace` covers the same compilation. Entry notes the remaining gap is intrinsic (Zig cache extraction, Defender/NTFS process costs) — caches themselves hit correctly.
`cargo test --workspace` compiles the same targets with full codegen; the check step was ~50s of pure overhead on the slowest CI leg. Removes the I-130 entry from ISSUES.md.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/specification.md`:
- Line 368: Update the affected Markdown list items in docs/specification.md,
including the entry defining `never` and the additional occurrences noted by the
review, so each list marker is followed by exactly one space. Preserve the
existing item text and formatting otherwise.
In `@ryo-core/src/diag.rs`:
- Around line 73-78: Update the documentation comment for VoidValueInExpression
in analyze_expr to describe every prohibited value position, including return
operands, operator operands, call arguments, conditions, bounds, bindings, and
assignments, rather than limiting it to binding and assignment contexts.
🪄 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: 7badb8c5-301f-4356-8319-83375388253f
📒 Files selected for processing (6)
.github/workflows/ci.ymldocs/specification.mdryo-core/src/diag.rsryo-core/src/tir.rsryo-frontend/src/sema.rsryo/tests/integration_tests.rs
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- ryo-core/src/tir.rs
| * `char`: Unicode Scalar Value. Literal: `'a'`. | ||
| * `void`: Unit type. Represents a value with no data. Used for functions that return no meaningful value. *(Rationale: Provides explicit way to represent "no return value" concept, common in many programming languages for side-effecting functions)*. | ||
| * `never`: Bottom type. Represents a computation that never completes (e.g., `panic`, infinite loop, `exit`). *(Rationale: Useful for control flow analysis and type theory completeness).* | ||
| * `never`: Bottom type. Represents a computation that never completes (e.g., `panic`, infinite loop, `exit`). A `never` value may only appear as a **bare expression statement** — it cannot be bound to a variable, returned, passed as an argument, or used as an operand (error **E0017**, `VoidValueInExpression`); see §6.1.3 and §7.6. *(Rationale: Useful for control flow analysis and type theory completeness).* |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one space after each list marker.
markdownlint reports MD030 for these changed list items. This can fail documentation linting.
Proposed fix
-* `never`: Bottom type.
+* `never`: Bottom type.Also applies to: 1688-1691, 2177-2177
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 368-368: Spaces after list markers
Expected: 1; Actual: 3
(MD030, list-marker-space)
🤖 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/specification.md` at line 368, Update the affected Markdown list items
in docs/specification.md, including the entry defining `never` and the
additional occurrences noted by the review, so each list marker is followed by
exactly one space. Preserve the existing item text and formatting otherwise.
Source: Linters/SAST tools
…pression Review feedback: the diag doc listed only binding positions, but the analyze_expr wrapper also fires it for return operands, operator operands, call arguments, conditions, and slice/range bounds. (Skipped the companion markdown finding: specification.md's list convention is 3-space markers — 307 occurrences vs 3 — and the lines this PR added match it; normalizing to one space belongs in a dedicated whole-file pass, not a 5-line inconsistency here.)
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.
What
First item of the m8.4.2 Fix Order (I-031 + I-071): sema now verifies that a non-void function returns (or diverges via
never) on every path through its body.MissingReturn.How
Tir::block_definitely_returnsinryo-core/src/tir.rs— structural analysis, lives with the other TIR reachability helpers. Rules: Return/ReturnVoid return;Unreachablesentinel suppresses cascades;never-typed ExprStmt operand (e.g.panic) diverges; IfStmt requires every arm + an else; loops are conservative (body can run zero times).DiagCode::MissingReturn→ E0036 (diag.rs, pipeline.rs mapping + tripwire test).analyze_function_bodyafterTirBuilder::finish; skipped for void/error-typed returns.Tests
9 new sema tests (red-first): falloff, exhaustive if/else + elif chains, if-without-else, while-body conservatism, trailing return, panic tail, error-cascade behavior both ways.
Validation
fmt clean · clippy
-Dwarningsclean · ryo-core 63 · ryo-frontend 408 · ryo-backend 9 · ryo-driver 1 · integration 188/188Removes the I-031 and I-071 entries from ISSUES.md (incl. the Fix Order section).
Summary by CodeRabbit
New Features
E0036when a function may finish without returning a value.panicstatements.nevervalues to standalone expression statements.Bug Fixes
neverusage in bindings, assignments, returns, operands, and call arguments.Documentation
neverbehavior.