Skip to content

chore(gc): delete the GcCallEffect::NeverReturns variant that is never constructed - #7347

Merged
proggeramlug merged 1 commit into
mainfrom
chore/delete-dead-neverreturns
Aug 4, 2026
Merged

chore(gc): delete the GcCallEffect::NeverReturns variant that is never constructed#7347
proggeramlug merged 1 commit into
mainfrom
chore/delete-dead-neverreturns

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Never constructed since #7314 added it, so its three match arms in precise_roots.rs were dead and rustc warned on every build — one of the warnings currently failing the Warnings gate on main.

Deleted rather than wired up, because the classifier's own comment already records why it can never be enabled:

The audit it rested on is already 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 these helpers allocate the Error they raise and can therefore collect.

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_churn reports 62 statepoints / 88 relocations before and after. cargo test -p perry-codegen --lib green.

Summary by CodeRabbit

  • Bug Fixes
    • Improved garbage-collection safety around calls that may throw or allocate.
    • Ensured calls that never return are handled conservatively unless explicitly approved as non-collecting.
    • Preserved safepoint handling for throwing calls that may allocate or unwind.

…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.
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change removes the NeverReturns GC call effect. Throwing helpers now use Unknown, and precise-root lowering processes affected invokes and ordinary calls through normal safepoint handling.

Changes

GC call classification

Layer / File(s) Summary
Remove NeverReturns effect
crates/perry-codegen/src/gc_call_effects.rs
GcCallEffect::NeverReturns is removed. js_throw* calls now use Unknown.
Update precise-root handling
crates/perry-codegen/src/function/precise_roots.rs
NeverReturns calls are no longer marked as leaf calls or skipped during invoke and ordinary-call safepoint lowering.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: removal of the unused GcCallEffect::NeverReturns variant.
Description check ✅ Passed The description explains the change, rationale, preserved behavior, and test results, although it does not use all template headings.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/delete-dead-neverreturns

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/perry-codegen/src/gc_call_effects.rs (1)

108-113: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add direct regression tests for js_throw* classification.

The fallback classifies js_throw* helpers as GcCallEffect::Unknown, and no prefix arm remains. Add assertions for representative throw helpers such as js_throw_reference_error_tdz and js_throw_not_a_constructor so 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

📥 Commits

Reviewing files that changed from the base of the PR and between 494083f and f2c6f0b.

📒 Files selected for processing (2)
  • crates/perry-codegen/src/function/precise_roots.rs
  • crates/perry-codegen/src/gc_call_effects.rs

@proggeramlug
proggeramlug merged commit 07c8f33 into main Aug 4, 2026
26 of 44 checks passed
@proggeramlug
proggeramlug deleted the chore/delete-dead-neverreturns branch August 4, 2026 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant