fix(gc): coercion-capable operators can collect — four rooting gaps from the #6972 review, plus restore the dark gc-stress gate

[proggeramlug]

Follow-up to #6972 (merged). CodeRabbit's review landed after that PR was merged; all four of its findings are real, and one of them is a soundness hole in the predicate that gates every rooting decision the merged change makes. Three are additional sites of the same #6951 bug class.

1. expr_may_trigger_gc was unsound for coercing operators (the important one)

expr/temp_root.rs documents the predicate's contract explicitly: false must mean "provably allocates nothing", and then violated it.

Expr::Unary { operand, .. } => expr_may_trigger_gc(operand),
Expr::Compare { left, right, .. } => expr_may_trigger_gc(left) || expr_may_trigger_gc(right),
Expr::Binary { op, left, right } =>
    matches!(op, BinaryOp::Add) || expr_may_trigger_gc(left) || expr_may_trigger_gc(right),

o < x, -o and o * 2 all run ToPrimitive / ToNumber on their operands, and a user-defined Symbol.toPrimitive / valueOf / toString is arbitrary JS — it allocates, and it collects. Recursing into the operands does not see that: a < b over two plain LocalGets recursed straight to false. So an argument list of the shape f(freshString(), a < b) would skip rooting freshString() even though a < b can collect — the exact #6951 use-after-free, in a narrower case.

These three now answer "GC-capable" unless every operand is a proven inert primitive, via a new expr_is_inert_primitive: literals, and locals the type analysis proved Number / Int32 / Boolean / Null / Void / Never and that have no reserved shadow slot (a reserved slot means pointer-possible regardless of the refined type). Add is never inert, since concatenation allocates even over two literals.

The predicate now takes &FnCtx to reach local_types / shadow_slot_map; any_later_arg_may_trigger_gc had no callers and is deleted rather than shipped dead.

Cost: unchanged. i < n and x * 2 on proven-numeric locals stay inert, so the hot-loop benchmark from #6972 still emits exactly 12 rooting calls.

2. expr/binary.rs — BigInt dynamic helper, !inline_bitwise branch

A second copy of the let l = lower_expr(..); let r = lower_expr(..); call(fname, l, r) shape that #6972's pass missed (it differed only in indentation). The helper runs ToNumeric on both operands, so a pointer-bearing left operand has to survive the right operand's evaluation. Now uses lower_operand_pair_rooted.

3. lower_canonical_str_self_append — s += rhs

s += rhs must load s before evaluating rhs (a rhs that reassigns s must not be observed), so the pre-rhs value is carried across rhs's evaluation and across js_jsvalue_to_string — both allocate. Re-reading the slot afterwards would take the wrong value, so the loaded box goes into a temp root and is re-read from there.

The coerced rhs handle is rooted too: the cold arm's unbox_str_handle materializes an SSO destination onto the heap, which is another allocation with the bare rhs handle live in a register. Both arms now re-read it, and one truncate in the merge block releases the pair (the index register is defined in the entry block, so it dominates).

4. lower_object_literal — deferred this_patches

Method-closure values are queued and reused after every remaining property has been lowered, so they sit in SSA registers across all of those allocations, and are then passed to js_closure_set_capture_bits as raw pointers. They are now rooted whenever any initializer can collect, and refreshed from their roots before the patch loop.

5. Restores the gc-stress gate, which has been dark since #6925 (#6976)

Not planned scope, but it blocks this PR and every other one.

#6925 added test-files/test_gap_repsel_proven_this_frozen.ts without registering it in test-parity/gc_repsel_corpus.txt, and scripts/gc_repsel_matrix.sh enforces registration by exiting 3 before running anything. So gc-stress has been failing on main with an UNREGISTERED message ever since, and the corpus was never exercised — the gate was dark, not green. Registering the file is one line.

With the gate running again, a genuine red cell appears, and it is Phase 5a's own escape hatch:

repsel_ptr_shape_locals x rep_ptr_shape_off
  compile: PERRY_PTR_SHAPE_LOCALS=0
  run:     PERRY_GC_HEAP_LIMIT=8 PERRY_GC_FORCE_EVACUATE=1
  -> exit 1, TypeError: Cannot read properties of undefined (reading 'area')
     (last five output lines lost)

Bisected: rc=0 at 8327ced52 (parent of #6925), rc=1 at 1a533a3a8 (#6925), rc=1 on current main. Not #6972 — a full --arms all run on that branch (based on 8327ced52) was 361/361 byte-exact with FAIL=0, and that run included this exact cell. Filed as #6976 and triaged against it, with an explicit note to remove the entry when fixed: an arm that turns a representation off is supposed to be the safest cell in the matrix.

Matrix after this PR: 380 cells, 379 byte-exact, PASS=29 UNVER=350 XFAIL=1 FAIL=0, with repsel_gc_stress x cons_scan_off and x cons_scan_off_force still PASS and the arm measurably live.

Verification

Everything from #6972 re-run on this branch, against pinned Node 26.5.0:

• gc: console.log argument temporaries are not precise roots — a precise-roots-only collection drops string-literal args (minimal repro, no evacuation needed) #6951 repro: still fixed under PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_HEAP_LIMIT=8.
• 20-shape probe suite: unchanged — the same 7 shapes remain red, all four already filed (gc: scalar-replaced object/array locals holding heap values are not precise roots #6968 scalar-replaced locals, gc: constructor arguments (new C(a, b)) are not precise roots across the instance allocation #6969 ctor args, gc: native-method-call arguments (map.set(a, b)) are not precise roots -- aborts under precise-roots-only #6970 native-method args, gc: string-method receiver and arguments are not precise roots across an allocating argument #6971 string-method receiver/args).
• Gap corpus: 431 test_gap_*.ts byte-exact vs Node 26.5.0 — identical result set to the origin/main baseline.
• scripts/gc_repsel_matrix.sh --arms all: 380 cells, 379 byte-exact, FAIL=0, XFAIL=1 (the gc-stress gate is dark on main: #6925 left its gap file unregistered, and regressed repsel_ptr_shape_locals under PERRY_PTR_SHAPE_LOCALS=0 #6976 cell above); test_gap_repsel_gc_stress × cons_scan_off and × cons_scan_off_force still PASS with the arm live.
• Throw through a protected argument list: byte-exact under both the default and precise-roots-only arms.
• New unit + IR tests from fix(gc): make argument temporaries precise roots (#6951) #6972 all still pass; cargo fmt --all -- --check clean.

Summary by CodeRabbit

• Bug Fixes
  • Improved garbage-collection safety for coercion-capable operations, preventing rare misbehavior when coercions invoke user-defined conversions.
  • Fixed rooting gaps affecting BigInt non-inlined fallbacks, string self-appending (+=), and object literal method closure handling.
  • Preserved prior behavior/performance for proven-numeric locals.
• Tests
  • Re-verified fixes on pinned Node 26.5.0 with targeted repros and updated corpus/triage coverage, including conservative stack-scanning variants.

[coderabbitai]

📝 Walkthrough

Walkthrough

The compiler’s GC-trigger prediction now accounts for user-defined coercions. Additional rooting protects BigInt operands, string self-append values, and deferred object-literal closures across allocating operations.

Changes

GC-safe codegen temporaries

Layer / File(s)	Summary
Context-aware GC prediction crates/perry-codegen/src/expr/temp_root.rs, changelog.d/...	GC checks use function context and inert-primitive analysis for coercion-capable expressions, with verification results documented.
Rooted BigInt helper operands crates/perry-codegen/src/expr/binary.rs	The dynamic BigInt helper lowers both operands with temporary rooting across ToNumeric evaluation, then releases the root.
Rooted string self-append values crates/perry-codegen/src/lower_string_method.rs	Canonical string self-append preserves the pre-coercion left value and roots the coerced right handle across heap and cold paths.
Deferred closure rooting crates/perry-codegen/src/expr/object_literal.rs, test-parity/*	Object-literal method closures are optionally rooted and refreshed before deferred this capture patching; GC representation-selection coverage and triage are updated.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ExpressionLowering
  participant TempRootAnalysis
  participant RuntimeCoercion
  participant GC
  ExpressionLowering->>TempRootAnalysis: classify operands and later expressions
  TempRootAnalysis-->>ExpressionLowering: require temporary roots when coercion may allocate
  ExpressionLowering->>RuntimeCoercion: evaluate ToNumeric or ToString
  RuntimeCoercion->>GC: allocate or trigger collection
  GC-->>ExpressionLowering: preserve rooted and refreshed values

Loading

Possibly related issues

• PerryTS/perry issue 6949 — Addresses a related GC-rooting gap involving values spanning potentially allocating coercions.
• PerryTS/perry issue 6976 — Tracks the representation-selection gap and matrix cell triaged by this change.
• PerryTS/perry issue 6971 — Addresses related string-lowering GC rooting across allocating operations.

Possibly related PRs

• PerryTS/perry#6860 — Overlaps at the BigInt bitwise code-generation boundary.
• PerryTS/perry#6972 — Overlaps in temporary-root infrastructure and binary/object-literal lowering paths.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Title check	✅ Passed	The title accurately summarizes the main GC rooting fixes and the gc-stress gate restoration.
Description check	✅ Passed	The description covers the summary, concrete changes, related issues, and verification, though it doesn't use the template headings exactly.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/6951-followup-coercion-gate

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/perry-codegen/src/expr/object_literal.rs (1)

473-538: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Release this-closure temp roots before the final rooted handle release.

rooted_handle_release(ctx, rooted) only truncates the root pushed by rooted_handle_begin, so temp_root_push_double entries added for captures_this: true closures in this_patches remain on the temp-root stack. Add/release or truncate each closure_root when it is consumed.

🤖 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/expr/object_literal.rs` around lines 473 - 538,
Release each non-None closure root from this_patches after it has been consumed
to refresh the deferred closure value, before rooted_handle_release(ctx, rooted)
runs. Update the final this_patches transformation or patch-consumption flow to
call the existing temp-root release/truncate mechanism for every closure_root,
while preserving the refreshed closure values and None behavior.

🤖 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.

Outside diff comments:
In `@crates/perry-codegen/src/expr/object_literal.rs`:
- Around line 473-538: Release each non-None closure root from this_patches
after it has been consumed to refresh the deferred closure value, before
rooted_handle_release(ctx, rooted) runs. Update the final this_patches
transformation or patch-consumption flow to call the existing temp-root
release/truncate mechanism for every closure_root, while preserving the
refreshed closure values and None behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 69482090-1e53-4354-a75e-9d6020bf4715

📥 Commits

Reviewing files that changed from the base of the PR and between 760db2f and a562173.

📒 Files selected for processing (5)

• changelog.d/6975-temp-root-coercion-gate.md
• crates/perry-codegen/src/expr/binary.rs
• crates/perry-codegen/src/expr/object_literal.rs
• crates/perry-codegen/src/expr/temp_root.rs
• crates/perry-codegen/src/lower_string_method.rs