perf(repsel): admit conditional return-shape producers (#7170 R2) - #8007
Conversation
📝 WalkthroughWalkthroughConditional return-shape inference now supports recursively nested conditional arms when each arm creates a fresh allocation of the same class. Optimization reports mark only proven return-shape sources as served. ChangesConditional return-shape support
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change broadens conditional return-shape optimization only for proven fresh allocations of one admissible class, with focused regression coverage and validation checks documented. No actionable merge-blocking risk remains beyond normal review follow-up. Sequence Diagram(s)sequenceDiagram
participant ReturnExpression
participant producer_return_class
participant collect_fresh_return_sources
participant CallerSeeding
ReturnExpression->>producer_return_class: provide conditional return expression
producer_return_class->>collect_fresh_return_sources: collect fresh branch sources
collect_fresh_return_sources->>producer_return_class: return fresh sources
producer_return_class->>CallerSeeding: seed caller from one agreed class
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/perry-codegen/src/collectors/ptr_shape_returns_tests.rs (1)
344-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover a seeded local in a conditional arm.
Expr::LocalGet(999)fails because it has no seeded class. The test also passes if recursive traversal incorrectly treats a conditional arm as a direct return. Add a local initialized withnew C()and return it from one conditional arm. Assert that no fact is produced.Based on learnings: Phase 3b permits
LocalGetonly for a barereturn local; a conditional arm must remain rejected.🤖 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/perry-codegen/src/collectors/ptr_shape_returns_tests.rs` around lines 344 - 348, Update conditional_return_with_any_non_fresh_arm_gets_no_fact to seed a local with new_c(), return that local from one conditional arm, and keep the other arm fresh/non-fresh as needed to exercise the rejection. Assert that no fact is produced, ensuring LocalGet is accepted only for a bare return local and not when nested in a conditional arm.Source: Learnings
crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs (1)
817-839: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIdentify the condition allocation in the assertion.
The served and unserved counts do not prove that the condition stays unserved. A defect that serves the condition and skips one result arm still passes this test. Give the three allocations distinct report identities and assert that the condition row is unserved and both result-arm rows are served.
🤖 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/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs` around lines 817 - 839, Update the conditional allocation test around run_as_producer to give the condition and both result arms distinct report identities, then assert each identity’s tier explicitly: the condition allocation must be unserved, while both then_expr and else_expr allocations must be served. Replace the aggregate served/unserved count assertions so a swapped allocation classification cannot pass.
🤖 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/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs`:
- Around line 817-839: Update the conditional allocation test around
run_as_producer to give the condition and both result arms distinct report
identities, then assert each identity’s tier explicitly: the condition
allocation must be unserved, while both then_expr and else_expr allocations must
be served. Replace the aggregate served/unserved count assertions so a swapped
allocation classification cannot pass.
In `@crates/perry-codegen/src/collectors/ptr_shape_returns_tests.rs`:
- Around line 344-348: Update
conditional_return_with_any_non_fresh_arm_gets_no_fact to seed a local with
new_c(), return that local from one conditional arm, and keep the other arm
fresh/non-fresh as needed to exercise the rejection. Assert that no fact is
produced, ensuring LocalGet is accepted only for a bare return local and not
when nested in a conditional arm.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a37df1ff-4bc8-41da-ac21-5322ee6ca8ea
📒 Files selected for processing (5)
changelog.d/8007-conditional-return-shapes.mdcrates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rscrates/perry-codegen/src/collectors/ptr_shape_report.rscrates/perry-codegen/src/collectors/ptr_shape_returns.rscrates/perry-codegen/src/collectors/ptr_shape_returns_tests.rs
1 participant
Summary
Admit returned conditional expressions as
Ptr<Shape>producers when every recursively reachable result arm is a fresh allocation of the same admissible class. This is the first producer-side R2 increment identified by #7170's post-R1 measurement.Changes
producer_return_class, while keeping conditions and unsupported expression forms out of the proof.return localretains its existing Phase 3b proof.returned expression operandsyntax bucket while marking only proven conditional result allocations as served.The proof continues to use the existing class-admission, containment, module-barrier, and caller-slot/rooting machinery; this introduces no new pointer position.
Related issue
Refs #7170 (R2 producer increment; the broader issue remains open for logical expressions and cross-module/method consumers).
Test plan
cargo check --profile perry-dev -p perry-codegencargo test --profile perry-dev -p perry-codegen --lib ptr_shape_returns --no-fail-fast(31 passed)cargo test --profile perry-dev -p perry-codegen --lib opt_report_tests --no-fail-fast(21 passed)cargo fmt -p perry-codegen -- --checkgit diff --checkcargo build --releaseclean#[test]in the affected crateThe focused test executables were linked against the installed Windows LLVM package with only its available x86/AArch64 targets enabled locally; that temporary manifest adjustment was restored and is not part of this PR.
Screenshots / output
Not applicable; compiler analysis/report behavior only.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests