jit: fix recursive call return value type tagging - #8499
Conversation
Assisted-by: Codex:5.6-sol
📝 WalkthroughWalkthroughRecursive JIT calls now derive results from the compiled function return signature. The change supports no-result and single-result calls, rejects unsupported result shapes, and adds boolean and floating-point recursion tests. ChangesRecursive JIT result handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RecursiveJITCall
participant CompiledFunction
participant JitValue
RecursiveJITCall->>CompiledFunction: execute recursive call
CompiledFunction-->>RecursiveJITCall: return result shape and value
RecursiveJITCall->>JitValue: convert value using self.sig.ret
Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/jit/src/instructions.rs (1)
561-564: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd recursive
Nonecoverage.Line 562 adds the no-result recursive-call path. The added tests cover only
BoolandFloat. Add a recursive function declared withNonethat reachesreturn recursive_none(n - 1).🤖 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 `@crates/jit/src/instructions.rs` around lines 561 - 564, Extend the recursive-call test coverage around the return handling in the JIT instruction flow to include a function declared with return type None that executes return recursive_none(n - 1). Verify this path compiles and returns JitValue::None, preserving the existing Bool and Float recursive cases.
🤖 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.
Nitpick comments:
In `@crates/jit/src/instructions.rs`:
- Around line 561-564: Extend the recursive-call test coverage around the return
handling in the JIT instruction flow to include a function declared with return
type None that executes return recursive_none(n - 1). Verify this path compiles
and returns JitValue::None, preserving the existing Bool and Float recursive
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f87064a-97fe-4b9f-8cd2-e433f39bd40b
📒 Files selected for processing (3)
crates/jit/src/instructions.rscrates/jit/tests/bool_tests.rscrates/jit/tests/float_tests.rs
Assisted-by: Codex:5.6-sol
Summary
Recursive call results were previously hardcoded as
JitValue::Int(...). Even when Cranelift gave back a value with the right machine type, the JIT still wrote it down as an integer at compile time.As a result, recursive functions returning
boolorfloatwere incorrectly typed. Functions with no return value could also try to read a return slot that was not there.self.sig.ret) with the actual return slot to preservebool,float, andNonereturn typesNotSupportedwhen the declared return type and actual return slot do not matchboolandfloatrecursive_float: verifies that a recursive call result remains tagged asFloat, allowing the following/ 2.0operation to consume the underlyingF64value correctlyrecursive_bool: verifies that a recursive call result remains tagged asBool(I8), allowing the followingnotoperation to compile and execute correctlySummary by CodeRabbit
Bug Fixes
Tests