refactor(codegen): migrate the computed read/write modules onto the Layer 1 rooting API (#7615) - #7642
Conversation
📝 WalkthroughWalkthroughComputed index reads and writes now use grouped rooting combinators. Property-set paths cover array length and dynamic stores. The migration ledger and HIR tests document and verify the changes. ChangesComputed access rooting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Codegen as index_set.rs/property_set.rs
participant Rooting as rooting API
participant RHS as right-hand-side lowering
participant Setter as runtime setter
Codegen->>Rooting: register receiver, key, and value operands
Rooting->>RHS: lower operands across collection windows
RHS-->>Rooting: return lowered values
Rooting->>Setter: emit setter call with reread rooted operands
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
…ayer 1 rooting API (#7615) Slice 4 of the Layer 1 campaign: `expr/index_get.rs`, `expr/index_set.rs`, `expr/property_set.rs` and their five sibling submodules move off the raw `expr::temp_root` API and onto `crate::rooting`'s combinators. All nine are listed in `MIGRATED_MODULES`. The whole slice is the `StoreOperandGuard` family — lower the receiver, guard it, lower the rest, re-read, store, release — and it is exactly `with_operands_rooted` / `with_operands_rooted_across`. No fourth combinator. Three live bugs, all the shape the sibling arms already guard: - #7645 `arr.length = f()` had no guard at all. - #7646 `arr[k] = f()` on an array with a non-numeric key rooted the receiver but not the KEY, which is a heap string by construction on that arm; and `arr[stringKey] = f()` guarded neither operand. - #7647 the polymorphic `o[k] = f()` fallback guarded neither operand, and the dynamic-string-key arm derived the receiver's window from `value` alone — the half-measure #7201 named, so `o[f()] = 1` was unguarded. Fixed here because each is an already-slow arm whose store routes through a runtime helper, so the root is noise beside the call. The rest of what the migration surfaced is filed, not fixed: they sit on inline fast paths where a root is a measured cost.
…7615) The TS corpora structurally cannot reach them, and that was measured rather than assumed: over the whole curated root-dominance corpus (129 sources, 149 modules) `js_array_set_length_strict` is called ZERO times, and the other two arms are reached once each by a source whose operands provably cannot collect — so the emitted IR is identical with and without the repair. Six hand-written probe shapes did not close the gap either, for a structural reason: an ordinary `o.k = v` / `o[k] = v` assignment lowers to `Expr::PutValueSet` and reaches the dynamic IC, not the `Expr::PropertySet` / `Expr::IndexSet` arms in these modules. So the coverage is built from HIR, and each test is DIFFERENTIAL: the same store compiled twice, once with an allocating RHS and once with an inert one, asserting the allocating one reserves strictly more root slots. An inert RHS makes `operand_protection` return `Reuse` and the combinator emit nothing, which is exactly the property that keeps this slice byte-identical on the corpus — so the two runs bracket the repair and no corpus drift can make the assertion vacuous. Each test also asserts by name that its arm was reached. Also fixes up the issue references in the slice's comments to the numbers the tracker actually assigned (#7637, #7638, #7639, #7640).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/expr/computed_store_rooting_tests.rs`:
- Around line 156-222: Add two HIR differential tests alongside the existing
store-rooting tests: one for the non-numeric array-key `#7638` arm and one for the
dynamic-string-key `#7639` arm representing o[f()] = 1. Build each test with the
appropriate receiver/key types and assert its expected runtime callee, comparing
both allocating and inert right-hand-side expressions through the existing
helper.
🪄 Autofix
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: 6f0d16f1-e303-4c74-be2a-c1a835d8b012
📒 Files selected for processing (13)
changelog.d/7642-layer1-slice4-computed-reads-writes.mdcrates/perry-codegen/src/expr/computed_store_rooting_tests.rscrates/perry-codegen/src/expr/index_get.rscrates/perry-codegen/src/expr/index_get/guarded_array.rscrates/perry-codegen/src/expr/index_get/inline_dyn_typed_array.rscrates/perry-codegen/src/expr/index_set.rscrates/perry-codegen/src/expr/index_set_typed_array.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/property_get.rscrates/perry-codegen/src/expr/property_get/globalget.rscrates/perry-codegen/src/expr/property_get/helpers.rscrates/perry-codegen/src/expr/property_set.rscrates/perry-codegen/src/rooting.rs
| /// #7638 arm 2 — `arr[stringKey] = f()`. The key is a heap string by | ||
| /// construction on this arm and `unbox_str_handle` below the RHS would hand the | ||
| /// setter a pre-move `StringHeader*`. | ||
| #[test] | ||
| fn array_string_key_store_roots_both_operands_across_an_allocating_rhs() { | ||
| assert_operands_rooted_only_when_the_window_collects( | ||
| "array_string_key", | ||
| "js_typed_feedback_array_set_string_key", | ||
| |value| { | ||
| vec![ | ||
| Stmt::Let { | ||
| id: 1, | ||
| name: "arr".to_string(), | ||
| ty: Type::Array(Box::new(Type::Any)), | ||
| mutable: false, | ||
| init: Some(Expr::Array(vec![Expr::Number(1.0)])), | ||
| }, | ||
| Stmt::Let { | ||
| id: 2, | ||
| name: "key".to_string(), | ||
| ty: Type::String, | ||
| mutable: false, | ||
| init: Some(Expr::String("k".to_string())), | ||
| }, | ||
| Stmt::Expr(Expr::IndexSet { | ||
| object: Box::new(Expr::LocalGet(1)), | ||
| index: Box::new(Expr::LocalGet(2)), | ||
| value: Box::new(value), | ||
| }), | ||
| ] | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| /// #7639 arm 1 — the polymorphic `o[k] = f()` fallback, reached when nothing | ||
| /// about the receiver or the key is statically known. It guarded neither | ||
| /// operand, and it is the arm where both are heap values by default. | ||
| #[test] | ||
| fn polymorphic_index_store_roots_both_operands_across_an_allocating_rhs() { | ||
| assert_operands_rooted_only_when_the_window_collects( | ||
| "polymorphic_index", | ||
| "js_typed_feedback_object_set_index_polymorphic", | ||
| |value| { | ||
| vec![ | ||
| Stmt::Let { | ||
| id: 1, | ||
| name: "recv".to_string(), | ||
| ty: Type::Any, | ||
| mutable: false, | ||
| init: Some(Expr::Object(Vec::new())), | ||
| }, | ||
| Stmt::Let { | ||
| id: 2, | ||
| name: "key".to_string(), | ||
| ty: Type::Any, | ||
| mutable: false, | ||
| init: Some(Expr::Object(Vec::new())), | ||
| }, | ||
| Stmt::Expr(Expr::IndexSet { | ||
| object: Box::new(Expr::LocalGet(1)), | ||
| index: Box::new(Expr::LocalGet(2)), | ||
| value: Box::new(value), | ||
| }), | ||
| ] | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cover the remaining repaired store arms.
These tests cover js_typed_feedback_array_set_string_key and js_typed_feedback_object_set_index_polymorphic. They do not cover the separate #7638 non-numeric array-key arm or the #7639 dynamic-string-key arm for o[f()] = 1.
A regression in either arm leaves this suite green. Add one HIR differential test per arm. Each test must assert the expected runtime callee and compare allocating and inert right-hand sides.
🤖 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/computed_store_rooting_tests.rs` around lines
156 - 222, Add two HIR differential tests alongside the existing store-rooting
tests: one for the non-numeric array-key `#7638` arm and one for the
dynamic-string-key `#7639` arm representing o[f()] = 1. Build each test with the
appropriate receiver/key types and assert its expected runtime callee, comparing
both allocating and inert right-hand-side expressions through the existing
helper.
c388342 to
d129279
Compare
Audit before merge — verified, merged as v0.5.1364All three in-slice fixes verified behaviourally against node 26.5.1 — Ledger sabotage red. Root-dominance both modes on the corpus: 129/129, 0 One correction to your lint line: Three things from this slice worth carrying forward
#7640 filed rather than fixed is the right call and matches #7634's Housekeeping: I'll clear the |
1 participant
Slice 4 of the Layer 1 campaign (#7615): the computed read/write lowerings.
Nine modules, all migrated end to end and listed in
MIGRATED_MODULES—expr/index_get.rs,expr/index_get/guarded_array.rs,expr/index_get/inline_dyn_typed_array.rs,expr/index_set.rs,expr/index_set_typed_array.rs,expr/property_get.rs,expr/property_get/globalget.rs,expr/property_get/helpers.rs,expr/property_set.rs. Nothing namesexpr::temp_rootafter this, and nomodule is left half-migrated.
One shape, no fourth combinator
Every rooting decision in this slice was the
StoreOperandGuardfamily — lowerthe receiver, guard it, lower the rest, re-read, store, release — and that is
exactly what
with_operands_rooted/with_operands_rooted_acrossalready say.The
acrossform is needed wherever the value is lowered bylower_value_for_dynamic_{index,property}_setorlower_value_for_optional_barrier, native-rep lowerings the operand list cannotproduce; everywhere else the plain form serves.
Honest scoping: three load-bearing, six vacuous
index_get.rs,index_set.rsandproperty_set.rsnamed the escape hatchbefore the migration (
guard_store_operand,guard_store_operand_across,reread_store_operand,release_store_operand,expr_may_trigger_gc), sotheir ledger lines hold on the committed source.
The other six named nothing, so their lines are vacuous and only the
sabotage arm makes them assertions. Each carries the audit that earned it in its
own file header rather than banking the count here. The one worth repeating:
the campaign map credits
property_get/globalget.rswith 6 hazard sites andall six are the same false positive —
js_get_global_this_builtin_value→ctx.strings.intern→unbox_to_i64→load.internandformat!emit noIR. A window is measured in emissions, and there are none. That module lowers
no user expression at all.
property_get.rs's three.call(I64, "js_*")sites each hand their raw pointerto a
nanbox_*_inlinein the same block, socall_rootedhas no site there —rooting them would add temp-root traffic to close a window that does not exist.
Three live bugs, fixed here
Each is the same window every sibling arm has guarded since #7154, and each is
on an arm whose store already routes through a runtime helper, so the root is
noise beside the call.
arr.length = f()holds the array receiver across the value with no root #7637 —arr.length = f()had no guard at all. Receiver lowered first(spec order),
f()allocates, andjs_array_set_length_strictthen truncatesthe abandoned from-space copy.
arr[k] = f()with a non-numeric index rooted the receiver but not the key —on the one arm whose entire purpose is keys that are not proven numeric —
and
arr[stringKey] = f()guarded neither operand.valuealone (#7201 residue) #7639 — the polymorphico[k] = f()fallback guarded neither operand,and it is reached precisely when nothing about receiver or key is known, so
both are heap values by default. Separately the dynamic-string-key arm derived
the receiver's window from
valuealone — the half-measure GC: a class expression with astatic { … }block SIGSEGVs under PERRY_GC_MOVING_LOOP_POLLS=1 (static-thiscell is not a rewritten root) #7201 named inprose — leaving
o[f()] = 1unguarded.That last one is the argument for the combinator rather than for a better flag.
with_operands_rooted_windowcomputes each operand's window asacross_collects || any_may_trigger_gc(exprs[i+1..]), so "the receiver is liveacross everything after it" becomes a property of the operand list instead of
something an author restates per site.
guard_store_operand's two-argument formstructurally could not say it. The same arm's two nested guards also collapse to
one group, retiring the release-inner-to-outer obligation that
temp_root_truncate's stack-cut semantics imposed.What the migration surfaced and did NOT fix — #7640
Translating the guarded arms made visible that ~20 arms in the same files make
no rooting decision at all: the bounded-index array store (which sits
immediately above the arm #7341 fixed, and whose loop predicate explicitly
accepts allocating object/array RHSs), the
#5525inline typed-array stores,globalThis[k] = v, ten read-side arms ofindex_get.rswhere the base is liveacross the key, an unsubstantiated "statepoint re-read" claim above the
class-field store, and a
unbox_str_handleordering hazard that is #7280'scase (a) — a raw
i64no temp root can name.Filed with repros and a suggested order rather than fixed, because they sit on
inline fast paths and guard diamonds where a root is a measured cost, and
this slice was not permitted to benchmark. Slice 3's
arr.pushprecedent(#7634) is the standard: a behaviour-preserving refactor does not smuggle in a
change whose cost nobody measured.
That gap is now written into the ledger itself, because it is the campaign's
most misreadable result:
Verification
IR identity. Byte-identical on all 149 modules of the curated corpus. On the
dependency-scale (zod) corpus, 80 of 81 byte-identical; the single difference is
one function,
perry_closure_..._schemas_ts__126, and it is root-plumbing only:the dynamic-string-key
IndexSetarm's two operands are now re-read andreleased in operand-list order (receiver, key) instead of the hand-written
inner-to-outer order (key, receiver). Same two slots, same set of clears, no
root added or removed — 32 diff lines, all register renumbering downstream of
that swap.
Probe vacuity checked before trusting the A/B. Every migrated arm's call
sites were counted in the emitted IR first: 543
object_get_field_by_name_f64,37
object_set_field_by_name, 16object_get_index_polymorphic, 10array_set_index_or_string, and so on. The one arm the curated corpus nevercalls is
js_array_set_length_strict— zero calls across all 129 sources —which is why the repairs get unit tests instead.
Gates. Both gated modes green on both corpora with an empty allowlist;
--seeded-violations 40at 40/40 on each;--unrooted-allocas --moving-onlyat0 over 7867 (curated) and 15242 (dep) gc-capable allocas; stale-register ratchet
unchanged at 23 / 115. Checker
--self-test,--audit-alloc-re,--audit-poll-capable,--audit-immovable-sourcesall clean.Ledger sabotage, per module. A real, compiling
temp_root_push_double/temp_root_truncatepair was planted in each of the nine in turn — the assertstops at the first offender, so one run cannot speak for nine — and each was
recorded red naming its own file and lines. Two of the nested submodules needed
crate::expr::temp_root::rather thansuper::temp_root::; withsuper::theplant did not compile, which is a build failure and not a sabotage, so those two
were re-run with the resolving path.
Tests.
-p perry-codegen --lib702 passed;--doc3 passed (bothcompile_faildoctests still reject);-p perry-runtime --no-fail-fast1902passed, 0 failed, no rerun needed.
cargo check --all-targetsclean — the#[cfg(test)]check #7631 asked for.Gap families, both arms,
PERRY_NO_AUTO_OPTIMIZE=1, prebuilt releasebinaries per arm:
test_gap_computed(5),test_gap_prop(1),test_gap_array(13),test_gap_object(9),test_gap_gc_(30). Verdict setsidentical on both arms, including the two pre-existing failures —
test_gap_prop_plan_cache_invalidation(parity) andtest_gap_gc_same_module_call_argument_rooting(crash) — which reproduce on abaseline build of
e4b298946and are not this PR's.Lint: 23 of 24 enumerated steps pass. The one red is
benchmarks/ci_public_baseline_check.py, red on pristinemainsince2026-07-29 and reported by #7618, #7620, #7627 and #7636 alike.
Coverage for the repaired arms
They are not reachable from ordinary TypeScript, and that was measured rather
than assumed. Six probe shapes — module-const and parameter receivers, string /
symbol /
anykeys, inside and outside a function — all routed past thesubject, because an ordinary
o.k = v/o[k] = vassignment lowers toExpr::PutValueSetand reaches the dynamic IC, not theExpr::PropertySet/Expr::IndexSetarms in these modules.So the coverage is built from HIR, and each test is differential: the same
store compiled twice, once with an allocating RHS and once with an inert one,
asserting the allocating one reserves strictly more root slots. An inert RHS
makes
operand_protectionreturnReuseand the combinator emit nothing —which is the property keeping this slice byte-identical on the corpus — so the
two runs bracket the repair and no corpus drift can make the assertion vacuous.
Each test also asserts by name that its arm was reached, and the slot count is
read under both root lowerings so it cannot silently measure zero. Sabotaging
the
arr.lengthrepair turns exactly that one test red, and only it.Closes #7637, closes #7638, closes #7639. Map comment on #7615 follows.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation