fix(gc): operand temporaries are precise roots — collection methods, constructor args, string-method receiver (#6970, #6969, #6971)

[proggeramlug]

Fixes #6970. Fixes #6969. Fixes #6971.

Three sibling lowering paths that #6972 named and filed. Each keeps an
evaluated operand in a bare LLVM SSA register across a collection point, which
under precise-roots-only (PERRY_CONSERVATIVE_STACK_SCAN=off) is a live
use-after-free.

First: is #6970 a live production abort today?

No — but the premise behind the question is right, and PR #6972's stated
reason is stale. Both halves matter, so both are measured rather than
asserted.

gc/roots.rs does resolve Auto -> SkipDisabled in shipped builds. #6972 said
this was harmless because gc_check_trigger "forces the scan on both automatic
arms". That is no longer true. gc_incremental_enabled() (policy.rs:302) and
gc_moving_safepoint_enabled() (policy.rs:279) are both default ON — the
doc comments above them still say "EXPERIMENTAL — default OFF" and "default
off", but the bodies are !matches!(env, "0"|"off"|"false"). With incremental
on, registered_root_scanners_block_budgeted_gc() is false for an ordinary
compiled program, so the guarded direct-minor arm (policy.rs:1207) is mostly
bypassed and the budgeted stepper runs instead — and budgeted cycles skip the
conservative subphase structurally, regardless of any guard
(cycle.rs:568-573, classifier mode).

Measured, not inferred. PERRY_GC_TRACE=1 on the #6970 reproducer, no other
env vars at all:

3 completed cycles
  1 full  / mutator_assist      conservative_root_count = 0   <- UNGUARDED
  1 minor / mutator_assist      conservative_root_count = 0   <- UNGUARDED
  1 full  / legacy_synchronous  conservative_root_count > 0      guarded

So production genuinely does complete precise-roots-only cycles today.

But the reproducer still does not fail there, and I could not make it fail:
80 sequential iterations under the pure default config, three runs, all
byte-exact against node. Two things keep it alive. BlockPersistence
(cycle.rs:1208-1247) runs precisely when the conservative scan is skipped
and retains everything in the recent-block window; and at the default heap size
so few cycles complete that landing one inside the argument-evaluation window
is unlikely. Raising pressure (PERRY_GC_HEAP_LIMIT=8) does not expose it
either — it shifts the mix to the OldReclaim arm, which is guarded (20 of 21
cycles).

Conclusion: latent, not live. What holds it latent is a recent-block
retention heuristic plus timing, not a rooting invariant — and #6950, by making
an evacuating minor reachable, is exactly what would remove the cushion. The
fix is worth landing on those terms rather than as a shipped-crash fix.

Reproduction (all three confirmed before fixing)

PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_HEAP_LIMIT=8, --profile perry-dev, macOS arm64:

issue	shape	before	after (4 runs)
#6970	m.set(fresh(k), churn(N))	abort, exit 134 — grown Map must retain its side-allocation owner record	4/4 byte-exact
#6969	new Pair(fresh(k), churn(N))	silent DIFF, 3 output lines lost	4/4 byte-exact
#6971	fresh(k).concat("|" + churn(N))	DIFF — receiver replaced by blanks	4/4 byte-exact

The arm is live, not vacuous: PERRY_GC_TRACE=1 reports 22 completed
cycles on the #6970 run. This arm collects but does not relocate, so it is
evidence about sweeping only; the separate evacuating measurement is below.

The bugs, from the IR

#6970 — the key is finished, then the value's lowering collects:

%r15 = call double @fresh__spec_i32(i32 0)     ; heap string, SSA register only
%r20 = call double @churn(double %r19)          ; collects — %r15 is freed here
%r21 = call i64 @js_map_set(i64 %r17, double %r15, double %r20)

#6971 — the receiver is unboxed to a bare address before the argument
is lowered:

%r25 = call i64 @js_get_string_pointer_unified(double %r24)  ; bare StringHeader*
%r29 = call double @churn(double %r28)                        ; collects
%r36 = call i64 @js_string_concat(i64 %r25, i64 %r35)         ; %r25 dangling

#6969 — argument 0 is live across argument 1 and across the instance
allocation, which always collects.

Mechanism

Reuses #6972's primitives; two additions, both in expr/temp_root.rs.

RootedOperands — root already-lowered operands where the caller, not
the helper, knows what comes next. lower_exprs_rooted cannot serve these:
it decides what to protect from the expression list it is handed and re-reads
immediately, whereas MapSet chooses the value's representation per branch
(~10 of them) and new allocates after the whole list is lowered. The caller
supplies the protection decision and picks the re-read point.

temp_root_scope_begin / _end — an expression-scope barrier, and the
thing that makes #6969 tractable at all. Truncation is a stack cut, so
pushing one null marker slot (a null word decodes to nothing, so it roots
nothing) lets a single truncate release the whole group on whichever of
lower_new's ~20 return paths ran. #6969 called this out as "the design
question to settle first"; the alternative is a temp_root_release at each
consumption site, which is precisely the bookkeeping that gets missed. lower_new_impl
becomes a six-line wrapper around the existing body.

lower_string_method is split the same way: the receiver is rooted once for
the whole dispatch and released in the wrapper, so none of its ~60 arms carries
release bookkeeping. Its 25 receiver uses each re-read through reread_recv,
which returns the original register when nothing was rooted.

concat needs more than a re-read: its accumulator is a bare StringHeader*
that changes address every round, so the result is written back with
js_gc_temp_root_set before the next argument is lowered. Rooting the old
address would sweep the string under construction.

A note on what a green reproducer proves

Worth stating up front, because it bit this PR after review and has now bitten
this campaign three separate ways.

A passing reproducer proves the window you happened to exercise, not the
window you reasoned about. All three reproducers here were 4/4 byte-exact
while the MapSet receiver was still unrooted across the key's lowering —
they passed because fresh(k) is a small allocation that rarely triggers a
cycle, not because the window was covered. Luck, not correctness. CodeRabbit
found it by reading; no amount of re-running would have.

It happened twice more in this PR alone. The receiver-rooting hole above, and
then a miscompile: re-loading a local/global operand to recover its address
after relocation also reads its value now, so
new C(g, bump()) where bump() reassigns g captured the post-bump()
value. All three reproducers stayed 4/4 green through both — they never mutate a
variable across an argument list, so the defect was outside every window they
exercise. Both were found by reading, not running.

The same shape across the wider campaign: FORCE_EVACUATE arms that passed while
moved_objects was 0 and nothing had ever relocated (#6950); gap sweeps that
were green off a warm object cache; and these. In each case the arm ran, the
output matched, and the thing being claimed was never exercised. The habit that
catches it is to state which collection point a test puts pressure on and check
that it is the one the fix is about — PERRY_GC_TRACE=1 cycle counts and
copied_objects exist for exactly that, and every measurement below quotes
them.

Two things that were wrong on the way, both now pinned by tests

Rooting a list after the loop is worse than not rooting it. My first #6969
attempt pushed all of lowered_args after the lowering loop. Argument 0 is
already dead by then, so this published a dangling pointer to the scanner and
turned the silent DIFF into a SIGSEGV. The push must interleave with the
lowering; constructor_arguments_are_rooted_across_the_instance_allocation
asserts push(arg0) < arg1's own allocation.

The receiver was rooted too late. RootedOperands originally took the
whole operand list at once, so m.set(k, v) lowered both map and key
before rooting either — leaving the receiver exposed across the key's lowering,
and (worse) able to push an already-dead register into a slot the collector
scans. It is now built incrementally, one operand at a time, so each is rooted
before the next is lowered. Same root cause as the constructor-list mistake
below; the two are one lesson.

Re-loading is not a free substitute for rooting. A registered root supplies
liveness, which tempts you to skip the slot and just re-emit the load. A temp
root actually buys three things — liveness, a location the collector rewrites,
and the value the call observed — and a re-load gives up the third. Only
immutable sources (string literals) are exempt; locals and globals get a real
slot. Covered by test_gap_ctor_arg_capture_order.ts, verified to fail with the
old predicate.

A blanket LocalGet suppression is unsound. The obvious reading of "locals
are shadow-rooted already" regressed #6970 straight back to an abort: the Map
receiver is a local with no shadow slot, so it is pointer-valued and not a
precise root at all (that is #6968). operand_needs_root checks
ctx.shadow_slot_map.contains_key(id), and the comment says why.

Cost

Gated by operand_needs_root: literals, module globals, provable
non-pointers, and locals with a reserved shadow slot pay nothing.

Byte-identical IR on already-safe shapes, verified by compiling the same
probe with a main build and this branch and diffing — "user_" + i,
[1,2,3], {a:i,b:total}, all-local argument lists, m.set(k, 1),
m.get(k), label.slice(1,3), label.indexOf("s"), new Pair(label, i):

md5  820e5ac515324b1f7d5368512d8a58a0  base/safe_shapes_ts.ll
md5  820e5ac515324b1f7d5368512d8a58a0  branch/safe_shapes_ts.ll

The 15 rooting calls in that file are present in both — they are #6972's
and #6975's, not new. On a probe of the three protected shapes the delta is
push 1→5, get 1→4, set 0→1, truncate 1→4: +11 calls, +14 IR lines.

Tests

• gc::tests::temp_roots::rewriting_a_slot_roots_the_new_value_and_releases_the_replaced_one
  — a real collection, pinning ConservativeStackScanMode::Disabled (the
  unit-test default is Full, whose native scan finds the test's own raw Rust
  locals and would make the test prove nothing). Asserts the sweep actually
  ran, that the written-back value is marked, and that the replaced one is not
  retained. Teeth: neutering js_gc_temp_root_set's body makes it fail.
• Six codegen IR tests in temp_root_operand_temporaries.rs — three pinning the
  emission contract and ordering, three pinning the absence of rooting on the
  shapes that were never broken. Teeth: with the three protection decisions
  forced to false, exactly the three positive tests fail and the three gate
  tests still pass.

Verification / what is pre-existing

• cargo test -p perry-codegen: 3 failures in native_proof_regressions.rs
  (mutable_string_key_rejects_static_write_pic,
  static_put_value_rejects_write_pic_when_rhs_can_allocate,
  nested_same_shape_object_writes_version_one_through_four_fields) — the same
  three fix(gc): make argument temporaries precise roots (#6951) #6972 reported. Verified identical on a main build of my own, not
  taken on trust.

• cargo test -p perry-runtime --lib gc::: gc::tests::teardown::map_set_*
  fail only under parallel execution (load was ~38) and pass with
  --test-threads=1; gc::tests::copying::large_object_old_born_array_slot_write_keeps_young_child_alive
  fails on main too.

• scripts/addr_class_inventory.py exits 1 on this branch and on main
  (native_module/constants.rs:2088, a file this PR does not touch).

• check_file_size.sh clean; cargo fmt --all -- --check clean; cargo clippy -p perry-codegen adds no new warnings.

• 432-file corpus A/B (every test_gap_*.ts, same sources, main compiler
  vs this branch, diffing program output and exit code): SAME=422, DIFF=1,
  SKIP=9. The single DIFF is test_gap_console_methods, and it is
  console.time wall-clock jitter (timer1: 0.010ms vs 0.021ms) — not a
  behavioural change. The 9 skips fail to compile in both arms.

Not verified: the local node is 26.3.0, not the pinned 26.5.0 (the mini
that has it is down to ~5 GB free). These reproducers print only numbers and
plain strings, so the oracle difference is not material for them, but I did not
run a pinned-node gap sweep.

Rebased onto #6977 — tested where objects actually move

#6977 landed while this was in review, so the evacuating arm
(PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off) is now real. This
branch is rebased onto it and re-measured there, with 786 490 objects actually
copied over 12 completed cycles — not an inert "passes under FORCE_EVACUATE".

repro	main @ e70ffe2	this branch
#6969 new Pair(...)	SIGSEGV 2/3, wrong 1/3	3/3 byte-exact
#6970 m.set(...)	SIGSEGV 3/3	SIGSEGV 3/3
#6971 .concat(...)	SIGSEGV 3/3	SIGSEGV 3/3

Stated plainly: under evacuation these programs are broken on main too, and
this branch fixes one of the three there and neither harms nor helps the other
two. What is still crashing is #6981 — the same defect class at sites this PR
does not touch (an ordinary user-function call argument passed raw under the
specialized ABI). The reproducers exercise a lot more than the shape they are
named for: churn alone does sink.push({...}), an object literal, a template
concat and an array literal per iteration. The arm these three issues actually
specify — precise roots, non-moving — is 4/4 on all three, main 0/4.

The one thing evacuation did find in this PR

Reviewing against #6981's mechanism exposed a real hole in my own gate. A temp
root buys two separate things:

1. liveness — the object is marked instead of swept;
2. a location the collector rewrites — so the value survives relocation.

operand_needs_root's suppressions only ever justified giving up the first: a
shadow-slotted local / module global / string literal is a registered root, so
it cannot be swept. But evacuation rewrites that storage, and the register
loaded beforehand still holds the pre-move address — precisely #6981, one layer
out. Suppressing the root and then reusing the cached register would have
shipped that.

Fixed by operand_is_reloadable: a suppressed operand is re-loaded at the
re-read point rather than reused. Correct under relocation, and it is a plain
load, not a runtime call — verified in the IR, where an escaping
new Pair(s, s) gains exactly two load double, ptr %r7 after the instance
allocation and zero extra calls. Pinned by
registered_root_operands_are_reloaded_rather_than_rooted.

Relationship to #6981 — orthogonal site, same class

Not the same bug; do not expect this PR to close it. #6981's repro
(first(P) where P is an Int32Array passed to an ordinary user function) goes
through the specialized-ABI call path, which this PR does not touch — none of
lower_new, lower_string_method or the collection-method lowering is involved.

It is the same class, and the connection is load-bearing rather than
decorative: #6981 is "the pre-collection register is stale after relocation",
which is exactly the second half of the rooting contract above. That is why it
changed this PR's gate. Folding #6981 in would mean rooting/reloading every
specialized-ABI argument — a different site, a different mechanism (raw unboxed
pointers, not NaN-boxed operands), and worth its own change.

Not in scope

#6968 (scalar-replaced object/array locals) is deliberately untouched — it is a
different defect, with no container and no shadow binding. This PR's
operand_needs_root comment records the concrete evidence that it is real and
distinct: the shadow-slot check exists because a pointer-valued local without
a slot is not a root.

Related: #6951, #6972, #6975, #6950, #6968.

Summary by CodeRabbit

• Bug Fixes
  • Fixed GC hazards in Map/Set and URLSearchParams lowering by introducing more precise temp-rooting for receiver/key/value and by reloading values after GC-visible steps.
  • Improved GC-safe new ... lowering so constructor argument values are captured correctly across allocation and later consumption.
  • Made string method lowering GC-safe, including concat accumulator rooting/reloading and re-reading the receiver per dispatch arm.
• Tests
  • Added regression tests validating operand temporary rooting via LLVM IR checks.
  • Added GC runtime coverage for temp-root slot rewriting semantics.
  • Added a constructor argument capture-order regression test.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Operand temporaries are rooted across potentially collecting lowering and runtime calls for Map/Set operations, constructors, and string methods. Grouped and scoped temp-root helpers re-read evacuated values, with codegen, runtime, and integration regression tests.

Changes

Precise operand roots

Layer / File(s)	Summary
Temporary-root infrastructure crates/perry-codegen/src/expr/temp_root.rs, changelog.d/...	Adds root-slot rewriting, grouped operand rooting, selective suppression, reloading, and scoped root barriers.
Map and collection lowering crates/perry-codegen/src/expr/math_simple.rs, crates/perry-codegen/src/lower_call/property_get/map_set.rs	Roots Map, Set, and URLSearchParams operands across argument lowering and runtime calls, then releases or re-reads them.
Constructor argument rooting crates/perry-codegen/src/lower_call/new.rs, test-files/test_gap_ctor_arg_capture_order.ts	Roots constructor arguments before instance allocation, refreshes them at consumption points, and tests capture ordering.
String receiver and accumulator rooting crates/perry-codegen/src/lower_string_method.rs	Roots string receivers and concat accumulators across allocating arguments and re-reads moved values.
Rooting contract validation crates/perry-codegen/tests/temp_root_operand_temporaries.rs, crates/perry-runtime/src/gc/tests/temp_roots.rs	Adds IR assertions for required and suppressed rooting, registered-root reloads, rewritten slot semantics, and constructor capture behavior.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

• Issue 6986 — Extends constructor argument rooting to previously unhandled constructor branches.
• Issue 6988 — Concerns test coverage and placement for the added temp-root codegen assertions.
• Issue 6991 — Addresses stale constructor values in the constructor receiver path, adjacent to this constructor-argument fix.

Possibly related PRs

• PerryTS/perry#6972 — Shares the temp-root infrastructure and precise operand-rooting patterns.
• PerryTS/perry#6975 — Modifies shared temp-root eligibility and string-method emission logic.
• PerryTS/perry#6997 — Updates non-pointer operand classification used by temporary-root suppression.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the main GC rooting changes across collection methods, constructors, and string receivers.
Description check	✅ Passed	The description covers summary, related issues, tests, and verification details; only the repo's exact template headings are missing.
Linked Issues check	✅ Passed	The code changes implement the requested precise-root fixes for #6970, #6969, and #6971, including rooting, rereads, and release barriers.
Out of Scope Changes check	✅ Passed	The changes stay within the GC rooting fixes and their tests/changelog entry, with no clearly unrelated functionality introduced.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ 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 fix/6970-6969-6971-temp-roots

---

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]

Actionable comments posted: 5

🧹 Nitpick comments (2)

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

305-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pin the values/protect length contract.

protect.get(i).copied().unwrap_or(false) silently degrades a short protect slice into "root nothing" — exactly the failure mode this module exists to prevent, and invisible at the call site. A debug_assert_eq! makes a future caller mismatch fail loudly in tests instead of reintroducing a use-after-free.

♻️ Proposed assertion

 ) -> RootedOperands {
+    debug_assert_eq!(
+        values.len(),
+        protect.len(),
+        "root_operands: one protect flag per operand"
+    );
     let mut slots = Vec::with_capacity(values.len());

🤖 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/temp_root.rs` around lines 305 - 328, Add a
debug_assert_eq! at the start of root_operands to require protect.len() ==
values.len(), then index protect directly in the loop instead of using
get(...).unwrap_or(false), preserving the existing rooting behavior for valid
inputs.

crates/perry-codegen/tests/temp_root_operand_temporaries.rs (1)

1-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mirror the ordering contracts into a cargo-test-visible unit test.

These six assertions are the only thing pinning the push → re-read → consume → release ordering, and integration suites under crates/*/tests/*.rs are not guaranteed to run on every PR. Consider duplicating at least the ordering checks (map-set and constructor interleaving) as unit tests inside perry-codegen so a regression in the rooting order fails the default test run.

As per path instructions: "Prefer acceptance coverage in cargo-test-visible unit tests because integration suites under crates/*/tests/*.rs do not run on every PR."

🤖 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/tests/temp_root_operand_temporaries.rs` around lines 1 -
21, Add cargo-test-visible unit tests within perry-codegen that mirror the
critical rooting-order assertions from the temporary-operand coverage, including
map-set and constructor interleaving. Verify push → re-read → consume → release
ordering, while preserving the existing negative checks for shapes that require
no rooting calls.

Source: Path instructions

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

Inline comments:
In `@changelog.d/6983-operand-temporaries-precise-roots.md`:
- Around line 4-5: The changelog predecessor reference for the description of
rooted variadic argument accumulators, concat operand pairs, and literal element
lists is incorrect. Update the issue number in the changelog entry to match the
`#6951` attribution used by the related temp-root tests and the RootedOperands
documentation, while leaving the other issue reference unchanged.

In `@crates/perry-codegen/src/expr/math_simple.rs`:
- Around line 552-575: Update the MapSet lowering flow around m_box, k_box, and
RootedOperands so the map receiver is rooted before lowering key, and the key is
rooted whenever value lowering may collect. Construct the RootedOperands
protection before key lowering, using the existing temp_root::lower_exprs_rooted
ordering or equivalent RootedOperands support, while preserving value-window
protection and deferred m_handle_unbox behavior.

In `@crates/perry-codegen/src/lower_call/new.rs`:
- Around line 806-815: The single re-read in the `lower_new_impl`
argument-lowering flow does not cover later allocations before argument
consumption. Add a small helper that refreshes each `lowered_args` entry from
its corresponding `arg_roots` slot, then invoke it immediately before late
consumers including `marshal_imported_ctor_args` and the dynamic-parent
`super(...)` args-buffer write, while retaining the initial refresh where
needed.

In `@crates/perry-codegen/src/lower_string_method.rs`:
- Around line 168-174: Guard the cleanup in the string-method lowering flow
around lower_string_method_dispatch: before calling temp_root_truncate, verify
that the current block is not terminated, using the existing is_terminated()
pattern. Preserve cleanup on reachable paths while avoiding emission after the
unknown-property arm’s blk.unreachable() terminator.

In `@crates/perry-codegen/tests/temp_root_operand_temporaries.rs`:
- Around line 369-386: The test
constructor_arguments_on_plain_locals_emit_no_rooting_calls does not exercise
LocalGet operands or shadow-slot suppression in operand_needs_root. Add
arguments that reference pointer-typed locals bound to shadow-stack slots and
retain the no-temp-root assertion, or rename the test and documentation to
describe literal arguments if that is the intended coverage.

---

Nitpick comments:
In `@crates/perry-codegen/src/expr/temp_root.rs`:
- Around line 305-328: Add a debug_assert_eq! at the start of root_operands to
require protect.len() == values.len(), then index protect directly in the loop
instead of using get(...).unwrap_or(false), preserving the existing rooting
behavior for valid inputs.

In `@crates/perry-codegen/tests/temp_root_operand_temporaries.rs`:
- Around line 1-21: Add cargo-test-visible unit tests within perry-codegen that
mirror the critical rooting-order assertions from the temporary-operand
coverage, including map-set and constructor interleaving. Verify push → re-read
→ consume → release ordering, while preserving the existing negative checks for
shapes that require no rooting calls.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fc4235eb-f2d5-49b9-a6eb-492934045d4d

📥 Commits

Reviewing files that changed from the base of the PR and between 68fab3a and fa4127d.

📒 Files selected for processing (8)

• changelog.d/6983-operand-temporaries-precise-roots.md
• crates/perry-codegen/src/expr/math_simple.rs
• crates/perry-codegen/src/expr/temp_root.rs
• crates/perry-codegen/src/lower_call/new.rs
• crates/perry-codegen/src/lower_call/property_get/map_set.rs
• crates/perry-codegen/src/lower_string_method.rs
• crates/perry-codegen/tests/temp_root_operand_temporaries.rs
• crates/perry-runtime/src/gc/tests/temp_roots.rs

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

This test does not cover the "plain locals" case it is named for.

The name and doc comment describe the shadow-slot suppression in operand_needs_root, but the arguments are Expr::Number and Expr::String — neither is a LocalGet, so the branch temp_root.rs calls "load-bearing, not decoration" (and whose blanket form regressed #6970) has no coverage here. Either rename to reflect the literal-argument gate, or add a LocalGet argument bound to a shadow-slotted local so the suppression itself is pinned.

🤖 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/tests/temp_root_operand_temporaries.rs` around lines 369
- 386, The test constructor_arguments_on_plain_locals_emit_no_rooting_calls does
not exercise LocalGet operands or shadow-slot suppression in operand_needs_root.
Add arguments that reference pointer-typed locals bound to shadow-stack slots
and retain the no-temp-root assertion, or rename the test and documentation to
describe literal arguments if that is the intended coverage.

[coderabbitai]

🧹 Nitpick comments (3)

crates/perry-codegen/tests/temp_root_operand_temporaries.rs (3)

185-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No positive test for a plain (non-mutating) string-method receiver rooted across an allocating argument.

The #6971 section covers the concat accumulator's push+set+get (mutating case) and the negative gate for non-allocating args, but there's no test analogous to map_set_key_is_rooted_across_an_allocating_value that verifies a non-mutating receiver method (e.g. slice) with an allocating argument gets pushed and re-read (push+get, no set). The PR objective explicitly calls out "string-method receivers ... across allocating expressions" as distinct from the changing accumulator case, so this path is currently only indirectly implied, not directly pinned.

🤖 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/tests/temp_root_operand_temporaries.rs` around lines 185
- 254, Add a positive test alongside
concat_accumulator_is_rooted_and_written_back for a non-mutating string method
such as slice with an allocating argument. Assert the generated IR contains
js_gc_temp_root_push and js_gc_temp_root_get, while asserting it does not
contain js_gc_temp_root_set, covering receiver rooting without accumulator
write-back.

---

1-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Consider whether this contract-pinning coverage belongs in a cargo-test-visible unit test instead.

This entire suite lives under crates/perry-codegen/tests/, which the repo's coding guidelines call out explicitly for integration suites that don't run on every PR by default. Since this file exists specifically to pin a subtle GC-rooting contract (push→get/set ordering, negative gates), losing default-PR coverage for it on future unrelated changes to temp_root.rs/lowering paths would weaken the regression protection this PR is trying to establish. Based on retrieved learnings, the diff-scoped e2e-scoped CI job does pick up modified suites like this one for the current PR, so this specific PR's coverage isn't at risk — but subsequent PRs that touch only the lowering files (and not this test file) may not re-run it.

Consider whether the core contract assertions (at least the ordering/gate checks) can be duplicated or migrated into a #[cfg(test)] module inside crates/perry-codegen/src so they run unconditionally on every PR.

Based on learnings, "the PR-only e2e-scoped job selects integration test suites based on the diff... So a modified file like crates/*/tests/*.rs is likely to be executed in the PR's e2e-scoped job as well," and as per coding guidelines, "Prefer acceptance coverage in cargo-test-visible unit tests because integration suites under crates/*/tests/*.rs do not run on every PR."

🤖 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/tests/temp_root_operand_temporaries.rs` around lines 1 -
21, Move or duplicate the core contract assertions from the
temp_root_operand_temporaries integration suite into a #[cfg(test)] module
within the temp_root lowering implementation. Cover push→get/set ordering and
the negative rooting gates, preserving the existing expected behavior while
ensuring these checks run in default cargo-test PR coverage. Keep end-to-end
tests only for runtime GC validation that cannot be exercised by unit tests.

Sources: Coding guidelines, Learnings

---

190-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add ordering assertions for push → set → get, matching the sibling tests.

Unlike map_set_key_is_rooted_across_an_allocating_value and constructor_arguments_are_rooted_across_the_instance_allocation, this test only checks presence of js_gc_temp_root_push/_set/_get, not their relative order. Given the doc comment stresses that the write-back must happen before the re-read (otherwise "the next argument's lowering keeps the INPUT alive and sweeps the string under construction"), pinning push < set < get (relative to the allocating argument's lowering) would make this test as strict as its siblings and catch an ordering regression that presence-only checks would miss.

♻️ Suggested ordering assertions

     assert!(
         ir.contains("call i64 `@js_gc_temp_root_get`"),
         "the accumulator must be re-read after the argument (and its ToString \
          coercion, which also allocates):\n{ir}"
     );
+
+    let push = ir.find("call i32 `@js_gc_temp_root_push`").unwrap();
+    let set = ir.find("call void `@js_gc_temp_root_set`").unwrap();
+    let get = ir.find("call i64 `@js_gc_temp_root_get`").unwrap();
+    assert!(
+        push < set && set < get,
+        "order must be push → write-back → re-read:\n{ir}"
+    );
 }

🤖 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/tests/temp_root_operand_temporaries.rs` around lines 190
- 226, Strengthen concat_accumulator_is_rooted_and_written_back by asserting the
IR ordering of js_gc_temp_root_push, js_gc_temp_root_set, and
js_gc_temp_root_get. Verify push occurs before set and set before get, matching
the ordering checks in the sibling rooting tests while preserving the existing
presence assertions.

🤖 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/tests/temp_root_operand_temporaries.rs`:
- Around line 185-254: Add a positive test alongside
concat_accumulator_is_rooted_and_written_back for a non-mutating string method
such as slice with an allocating argument. Assert the generated IR contains
js_gc_temp_root_push and js_gc_temp_root_get, while asserting it does not
contain js_gc_temp_root_set, covering receiver rooting without accumulator
write-back.
- Around line 1-21: Move or duplicate the core contract assertions from the
temp_root_operand_temporaries integration suite into a #[cfg(test)] module
within the temp_root lowering implementation. Cover push→get/set ordering and
the negative rooting gates, preserving the existing expected behavior while
ensuring these checks run in default cargo-test PR coverage. Keep end-to-end
tests only for runtime GC validation that cannot be exercised by unit tests.
- Around line 190-226: Strengthen concat_accumulator_is_rooted_and_written_back
by asserting the IR ordering of js_gc_temp_root_push, js_gc_temp_root_set, and
js_gc_temp_root_get. Verify push occurs before set and set before get, matching
the ordering checks in the sibling rooting tests while preserving the existing
presence assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ca56bcc4-7d7b-4e21-a2be-c527e99feac3

📥 Commits

Reviewing files that changed from the base of the PR and between fa4127d and 8a635b8.

📒 Files selected for processing (8)

• changelog.d/6983-operand-temporaries-precise-roots.md
• crates/perry-codegen/src/expr/math_simple.rs
• crates/perry-codegen/src/expr/temp_root.rs
• crates/perry-codegen/src/lower_call/new.rs
• crates/perry-codegen/src/lower_call/property_get/map_set.rs
• crates/perry-codegen/src/lower_string_method.rs
• crates/perry-codegen/tests/temp_root_operand_temporaries.rs
• crates/perry-runtime/src/gc/tests/temp_roots.rs

🚧 Files skipped from review as they are similar to previous changes (7)

• crates/perry-codegen/src/lower_call/new.rs
• changelog.d/6983-operand-temporaries-precise-roots.md
• crates/perry-runtime/src/gc/tests/temp_roots.rs
• crates/perry-codegen/src/lower_call/property_get/map_set.rs
• crates/perry-codegen/src/expr/temp_root.rs
• crates/perry-codegen/src/expr/math_simple.rs
• crates/perry-codegen/src/lower_string_method.rs

[proggeramlug]

Thanks — both substantive findings were real and are fixed in 7f345d3.

1. MapSet receiver unrooted across the key's lowering (critical) — correct, fixed.
Confirmed by reading: m_box and k_box were both lowered before root_operands ran, so the receiver's exposure window (the key's own lowering — fresh(k) in the repro, which allocates) was unprotected, and the subsequent push could publish an already-dangling pointer into a scanned slot. That is the exact escalation this PR's changelog documents for #6969, so shipping it here would have been the same mistake one file over.

RootedOperands is now built incrementally (root_operands_begin + push), so map is rooted before key is lowered and key before value:

let mut roots = temp_root::root_operands_begin(2);
let m_box = lower_expr(ctx, map)?;
roots.push(ctx, map, &m_box, key_collects || value_collects);
let k_box = lower_expr(ctx, key)?;
roots.push(ctx, key, &k_box, value_collects);

I kept RootedOperands rather than switching to lower_exprs_rooted because this site still needs the deferred re-read (the value's representation is chosen per branch, ~10 of them) and the deferred m_handle unbox; the builder gives the same interleaving guarantee.

2. lowered_args consumed after further collection points (major) — correct, fixed.
Your reading is right, including that it was staleness rather than a use-after-free (the scope cut keeps the slots alive). Extracted refresh_rooted_args and called it at the sites behind further lowering: both marshal_imported_ctor_args callsites and the dynamic-parent super(...) args buffer. The over-claiming comment ("every consumer below this point") is gone.

3. Changelog issue number — fixed, now #6951 (via #6972). Both were accurate in isolation (#6951 the issue, #6972 the PR); made consistent.

4. debug_assert_eq! on the values/protect lengths — no longer applicable. The parallel-slice API is gone with the builder, so operand/value/flag can't disagree by construction, which is the stronger version of what you asked for.

5. Mirroring the ordering contracts into cargo-test-visible unit tests — acknowledged and not done here; flagging rather than silently skipping. The assertions need a built Module + compile_module, so moving them into src/ unit tests means either constructing a FnCtx by hand or duplicating the harness. Worth doing properly for the whole temp_root family rather than piecemeal in this PR. The end-to-end gate meanwhile is the cons_scan_off matrix arm plus the reproducers.

Re-verified after the changes: reproducers 4/4 byte-exact on the precise-roots arm (#6969 also 3/3 on the evacuating arm, where main is 0/3); safe-shape IR still byte-identical to main (same md5); cargo test -p perry-codegen unchanged at the same 3 pre-existing native_proof_regressions failures.

[coderabbitai]

Actionable comments posted: 1

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

Inline comments:
In `@crates/perry-codegen/src/lower_call/new.rs`:
- Around line 162-164: Update constructor-argument refresh handling around
lower_constructor_arg and all listed refresh sites so mutable reloadable
operands, including shadow-slotted locals and module globals, are never
re-lowered after the new call. Reload only provably immutable sources; otherwise
snapshot the evaluated value into a temp-root slot and refresh from that slot,
preserving the original call-time value across argument evaluation, field
initialization, and constructor lowering.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a2347af9-03b1-4061-a746-df9cf7fcfc73

📥 Commits

Reviewing files that changed from the base of the PR and between 8a635b8 and 7f345d3.

📒 Files selected for processing (4)

• changelog.d/6983-operand-temporaries-precise-roots.md
• crates/perry-codegen/src/expr/math_simple.rs
• crates/perry-codegen/src/expr/temp_root.rs
• crates/perry-codegen/src/lower_call/new.rs

🚧 Files skipped from review as they are similar to previous changes (3)

• changelog.d/6983-operand-temporaries-precise-roots.md
• crates/perry-codegen/src/expr/temp_root.rs
• crates/perry-codegen/src/expr/math_simple.rs

[proggeramlug]

Confirmed and fixed in 4eec048 — and you were right that it is worse than the staleness it was meant to cure. This was a miscompile I introduced, not a rooting gap.

Reproduced before fixing:

let g = "before";
function bump(): number { g = "after"; return 1; }
class C { p: unknown; q: unknown; constructor(p, q) { this.p = p; this.q = q; } }
const sink: unknown[] = [];
const c = new C(g, bump());
sink.push(c);                      // defeat scalar replacement
console.log("c.p =", c.p, "| g =", g);

node:   c.p = before | g = after
main:   c.p = before | g = after
branch: c.p = after  | g = after      <- wrong

The framing I had was wrong. I described a temp root as buying two things — liveness, and a location the collector rewrites — and concluded a registered root supplies the first so the slot could be skipped. It buys three: liveness, relocation-correctness, and the value the call actually observed. Re-loading gives up the third, and only an immutable source is exempt.

So operand_is_reloadable is now string literals alone; locals and module globals get a real slot, which preserves the call-time value and is rewritten on evacuation. That is the "snapshot into a temp-root slot and refresh from that slot" you proposed — I took it as written rather than trying to narrow it with a mutation analysis, because the refresh sites you listed (843, 1563, 1603, 1692) sit behind arbitrary user lowering and a "provably not reassigned between these two points" query does not exist here.

Cost of the correction, measured: new C(local) and m.set(local, f()) now emit a push/get pair they did not before. I would rather pay that than ship the wrong value.

Regression coverage: test-files/test_gap_ctor_arg_capture_order.ts, covering both a module global reassigned by a later argument and a local reassigned by the constructor body. Verified to have teeth — rebuilt with the old predicate restored, it prints global captured: after; with the fix, before.

Re-verified after the change: all three reproducers 4/4 byte-exact on the precise-roots arm, #6969 still 3/3 on the evacuating arm, 7/7 codegen IR tests green. Full 432-file corpus A/B is re-running against this build — the earlier SAME=422 predates two restructures and I am not carrying it forward.

Two notes on the earlier round, for the record: the debug_assert_eq! on slice lengths is moot (the parallel-slice API is gone with the incremental builder, so the operand/value/flag triple cannot disagree by construction), and the "mirror into cargo-test-visible unit tests" suggestion is now tracked as #6988 rather than dropped — per CLAUDE.md crates/*/tests/*.rs does not run per-PR, so this family's emission-contract coverage is genuinely CI-invisible and deserves its own change.

[proggeramlug]

Corpus A/B verdict — final build

SAME=423  DIFF=1  SKIP=9      (433 files)
DIFFERING: test_gap_console_methods (exit 0/0)

Every test-files/test_gap_*.ts compiled with the main compiler (e70ffe29d) and this branch's, run, and compared on stdout+stderr and exit code.

These numbers are from the final build, 4eec04829 — post-miscompile-fix. Not the SAME=422 run (which predates both the incremental-rooting restructure and the miscompile fix) and not the run I killed at exit 144 mid-rebuild. I verified the compiler's provenance functionally rather than by mtime: the binary that produced this table compiles test_gap_ctor_arg_capture_order.ts to global captured: before, which only the fixed predicate does. (423 rather than 422 because this PR adds that regression test to test-files/; 423 + 1 + 9 = 433.)

The single DIFF is console.time wall-clock jitter, not a behavioural change:

26c26
<   timer1: 0.012ms
>   timer1: 0.013ms
28c28
<   timer1: 0.027ms
>   timer1: 0.023ms

Same file, same shape of noise as the pre-restructure run — it differs run-to-run on the same binary. The 9 SKIPs fail to compile in both arms.

No DIFF resembling the spec-ABI raw-pointer class (#6981) surfaced here; this A/B runs the default configuration, where that class is masked by the conservative stack scan, so it would not be expected to and this is not evidence either way about it.