chore(gc): delete the GcCallEffect::NeverReturns variant that is never constructed - #7347
Conversation
…r constructed It has never been constructed since #7314 introduced it, so its three match arms in precise_roots.rs were dead and rustc warned about the variant on every build -- one of the warnings failing the Warnings gate. Deleting rather than wiring it up, because the classifier's own comment already records why it can never be enabled: the audit it rested on is false (js_throw_reference_error_tdz, js_throw_not_a_constructor and others are declared -> f64, not -> !), and since #7302 a throw UNWINDS rather than longjmps, so the call site is an invoke whose unwind edge needs relocations -- while those helpers allocate the Error they raise and can therefore collect. Suppressing the safepoint would leave the catch handler's roots stale after a move. That reasoning is preserved at the site, in the past tense. CLAUDE.md's kill-policy is the rule being applied: an unexercised mode is a decision nobody has made, and the losing mode should stop compiling. No behaviour change, and asserted rather than assumed: 01_nursery_churn reports 62 statepoints / 88 relocations before and after.
📝 WalkthroughWalkthroughThe change removes the ChangesGC call classification
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-codegen/src/gc_call_effects.rs (1)
108-113: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd direct regression tests for
js_throw*classification.The fallback classifies
js_throw*helpers asGcCallEffect::Unknown, and no prefix arm remains. Add assertions for representative throw helpers such asjs_throw_reference_error_tdzandjs_throw_not_a_constructorso a future prefix rule cannot reintroduce unsafe safepoint suppression.🤖 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/gc_call_effects.rs` around lines 108 - 113, Add direct regression tests for the js_throw* helper classification, covering representative names such as js_throw_reference_error_tdz and js_throw_not_a_constructor. Assert that each resolves to GcCallEffect::Unknown through the existing classification test path, preserving the fallback behavior and preventing a future prefix rule from suppressing safepoints.
🤖 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/gc_call_effects.rs`:
- Around line 108-113: Add direct regression tests for the js_throw* helper
classification, covering representative names such as
js_throw_reference_error_tdz and js_throw_not_a_constructor. Assert that each
resolves to GcCallEffect::Unknown through the existing classification test path,
preserving the fallback behavior and preventing a future prefix rule from
suppressing safepoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ca31a5f1-46b6-4645-a278-722b102a3098
📒 Files selected for processing (2)
crates/perry-codegen/src/function/precise_roots.rscrates/perry-codegen/src/gc_call_effects.rs
Never constructed since #7314 added it, so its three match arms in
precise_roots.rswere dead and rustc warned on every build — one of the warnings currently failing theWarningsgate on main.Deleted rather than wired up, because the classifier's own comment already records why it can never be enabled:
I went looking at this as a root-density lever (throw helpers take 5 statepoints in one probe alone, and a call that never returns can't consume a relocation). The comment is the answer: it's a closed dead-end, not an unfired optimization. The rationale is preserved at the site in the past tense so the next person doesn't re-derive it.
CLAUDE.md's kill-policy is the rule: an unexercised mode is a decision nobody has made, and the losing mode should stop compiling.
No behaviour change, asserted not assumed:
01_nursery_churnreports 62 statepoints / 88 relocations before and after.cargo test -p perry-codegen --libgreen.Summary by CodeRabbit