refactor(codegen): migrate arrays_finds.rs + array_methods.rs onto the Layer 1 rooting API (#7615) - #7620
Conversation
…e Layer 1 rooting API (#7615) Layer 1 campaign slice 1b: `expr/arrays_finds.rs` and `expr/array_methods.rs` migrated end to end onto `crate::rooting` and added to `MIGRATED_MODULES`. Adds one combinator, `rooting::with_operands_rooted_across`, with its two callers: the `u8[i]` / `buf[i]` tails root the receiver across an index lowering whose representation the caller picks (`lower_expr_as_i32` vs `fptosi`), which the fixed re-read point of `with_operands_rooted` cannot express. `with_operands_rooted` is now the empty-`across` case of it, so "root, re-derive or reuse?" stays answered in one place.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds ChangesGC-safe operand rooting migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExpressionLowering
participant with_operands_rooted_across
participant OperandLowering
participant RuntimeCall
ExpressionLowering->>with_operands_rooted_across: lower and root operands
with_operands_rooted_across->>OperandLowering: evaluate later operands
OperandLowering-->>with_operands_rooted_across: return lowered values
with_operands_rooted_across->>RuntimeCall: pass re-read operands
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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 `@changelog.d/7620-layer1-slice1b-arrays-finds-rooting.md`:
- Line 1: Update the changelog heading to use PR key `#7620`, matching the
7620-prefixed fragment filename and ensuring the release note references the
correct PR.
🪄 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: 0466fa89-1745-4ad2-bdac-1f03e03e6add
📒 Files selected for processing (4)
changelog.d/7620-layer1-slice1b-arrays-finds-rooting.mdcrates/perry-codegen/src/expr/array_methods.rscrates/perry-codegen/src/expr/arrays_finds.rscrates/perry-codegen/src/rooting.rs
| @@ -0,0 +1,60 @@ | |||
| ### Layer 1 rooting migration, slice 1b — `expr/arrays_finds.rs` + `expr/array_methods.rs` (#7615) | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the current PR key in the heading.
Line 1 says #7615, but the fragment filename is keyed to 7620. Change the heading to #7620 so the release note identifies the same PR as its required fragment.
As per coding guidelines, add a PR-keyed changelog.d/<PR>-<slug>.md fragment. Based on learnings, verify the PR key in each changelog fragment.
Proposed fix
-### Layer 1 rooting migration, slice 1b — `expr/arrays_finds.rs` + `expr/array_methods.rs` (`#7615`)
+### Layer 1 rooting migration, slice 1b — `expr/arrays_finds.rs` + `expr/array_methods.rs` (`#7620`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Layer 1 rooting migration, slice 1b — `expr/arrays_finds.rs` + `expr/array_methods.rs` (#7615) | |
| ### Layer 1 rooting migration, slice 1b — `expr/arrays_finds.rs` + `expr/array_methods.rs` (`#7620`) |
🤖 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 `@changelog.d/7620-layer1-slice1b-arrays-finds-rooting.md` at line 1, Update
the changelog heading to use PR key `#7620`, matching the 7620-prefixed fragment
filename and ensuring the release note references the correct PR.
Sources: Coding guidelines, Learnings
Audit before merge — verified, merged as v0.5.1355Ledger sabotage red on a real The Root-dominance re-run: 129/129, 0 violations, 40/40 seeded caught. Full Three things this slice adds to the campaign's method, worth naming:
The combinator addition ( Fallout handled separately: the harness-fingerprint redness this slice's |
1 participant
Layer 1 campaign slice 1b (#7615):
crates/perry-codegen/src/expr/arrays_finds.rsandcrates/perry-codegen/src/expr/array_methods.rsmigrated end to end ontocrate::rooting, following #7617's template and #7618's precedent. 2 modules, 1562 → 1763 lines; the map counts 40 raw sites and 26 hazard sites. Both land whole; the ledger notes no outstanding boundary.Adds one combinator,
rooting::with_operands_rooted_across, arriving with its two callers and with the argument for why the existing one cannot serve (below).with_operands_rootedis now its empty-acrosscase, so "root, re-derive, or reuse?" is still answered in exactly one place — the drift that produced #7114 was two helper families answering it separately.What the migration found
1.
Expr::BufferSliceheld a RAW, already-unboxed pointer across user code#7453's shape with a twist that matters: what is in flight is no longer NaN-boxed, so #7280's
root_reloadstructurally cannot repair it — that post-pass re-reads a shadow slot into adouble, and the consuming call reads ani64derived above the window. The unbox is now emitted below the group's re-read, which is the only place it can be correct.2. The
arr.find*family, in the shape it is almost always written infind/findIndex/findLast/findLastIndexall lowered the array first and left it in a register while the callback was lowered. A callback literal lowers tojs_closure_new— an allocation — soxs.find(x => x.id === id)is a live window. Same forObject.is(a, b),Object.hasOwn(o, k),path.matchesGlob(p, pat), theMap/Setpositional readers,AggregateError,Buffer.concat(list, total),Object.create(proto, props), bothFinalizationRegistrymutators, bothErrorNew*forms, the three-operandNativeArenaView/NativePodView, the multi-argumentnew Date(y, m, d, …)(each component live across the next), and the polymorphicu8[k] = vstore.3. The index arms needed a re-read point this module controls
u8[i]andbuf[i]lower the receiver withlower_exprand the index withlower_index_i32, which picks between ani32fast path (lower_expr_as_i32) and adoubleplusfptosi. The receiver is live across that choice. Handing the index towith_operands_rootedwould force every such read back onto the NaN-boxed path — a codegen-quality regression, not a rooting fix.with_operands_rooted's re-read point is fixed at the end of its operand list, so an operand lowered before caller-controlled work would be re-read above it and stale again by the time the call runs, which is the #7114 half-measure.with_operands_rooted_acrossroots the group before the caller-controlled lowering and re-reads it after;across_exprsis passed as expressions rather than aboolso the "does this window collect?" question stays insideoperand_protection. Two callers, both here.The new combinator deliberately does not reopen what
rooting.rs's existing note warns about: it never takes a bare register, it still owns the release (now including a bail from the operand lowering itself), and it never hands out an unrooted result.Honest scoping — what is genuinely new vs what #7280 already covered
Following #7618's precedent, this is stated rather than glossed. For a shadow-slotted local receiver,
root_reloadhad already repaired thefindwindow, and the baseline IR shows it as an out-of-sequence re-read:-16 load double, ptrin the net delta is exactly that population: #7280 reloads replaced by root reads. What this PR actually buys is the three shapes #7280 structurally cannot cover:A receiver reassigned by its own argument. GC: #7154's residual is NOT fixed — the loop-polls config is red 0/30, and stock zod alone fails 5/40 #7280 bails there by design (
operand_is_reloadable's documented miscompile: re-loading would observe the assignment). Baseline, verbatim — note the absence of any re-read:After:
A raw, already-unboxed pointer —
BufferSliceabove. Adoublereload cannot repair ani64derived above the window.Operand-to-operand windows and receivers with no slot at all — call results, property chains, the four-operand
finreg.register(t, h, tok).IR-identity evidence
Two corpora, both arms, compiled
PERRY_RS4GC=0 PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_INLINE_SHADOW_SLOT=0 PERRY_NO_AUTO_OPTIMIZE=1 --trace llvm,PERRY_RUNTIME_DIRpinned. Register names and block-label numbers are masked; a function is "identical" iff its instruction sequence matches up to that renaming.A. 8 purpose-built probes reaching 28 of the 30 distinctive callees of the arms this PR changed (the two gaps are named below).
172 functions compared; 164 identical, 8 differ — and all 8 are
main, the only function in each probe containing these lowerings.Whole-corpus net instruction delta, every line of it:
store i64 %r·js_shadow_slot_bind— the root storesload i64, ptr·bitcast i64→double— the re-readsstore i64 0, ptr·js_shadow_slot_set— the releases (the odd one is a grown frame's prologue zero-init)bitcast double→i64— the store side of a pushalloca i64— one more pooled temp slotload double, ptr— GC: #7154's residual is NOT fixed — the loop-polls config is red 0/30, and stock zod alone fails 5/40 #7280's reloads, replaced by root readsNon-root-plumbing added: 0. Non-root-plumbing removed: 0. This PR deletes no instruction kind and adds no callee.
B. The whole
gc-root-dominancecorpus (129 sources → 149 modules) compared the same way, which also re-checks the two already-migrated modules against thewith_operands_rootedrefactor:load double).@.str.Nconstants andjs_register_function_namecalls come out in a different order run to run, and intest_gap_class_expr_dynamic_parent_ctorthreeperry_method_*callees permute. Filed as Codegen is not run-to-run deterministic: string-constant numbering, function-name registration order, and method-callee selection permute #7622; it is a pre-existing build-reproducibility gap, and worth knowing about before anyone else tries a byte-level A/B here.Behavioural A/B: all 8 probes, both arms — identical stdout and exit code, and identical again under
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=64.Coverage the probes do NOT reach, stated rather than glossed:
js_native_pod_view(lower_native_pod_view_with_layout) —NativeArena.podViewneeds aPerryPodView<T>annotation over aPerryPod<…>layout, which has no TS spelling reachable from a standalone probe; it is exercised only bycrates/perry-codegen/tests/native_proof_*(nightly/tag only). Its migration is the same three-operandwith_operands_rootedasNativeArenaView, which IS probed twice.js_buffer_index_get_value(BufferIndexGet's tail) — the structural twin ofUint8ArrayGet's tail, which is probed 6 times through the same newwith_operands_rooted_acrosscall. Five index shapes were tried and all routed tolower_buffer_loador the dynamic helper.Ledger sabotage — result recorded, per module
escape_hatch_usesstrips comments, and both files namedtemp_rootzero times before this PR, so listing them passes vacuously. The arm that makes the listing mean something was run for each module separately (the assert stops at the first offender, so one run cannot speak for both):expr/arrays_finds.rstemp_root_push_double+temp_root_truncatearrays_finds.rs:279and:280expr/array_methods.rstemp_root_push_i64+temp_root_truncatearray_methods.rs:59and:60Same answer #7617 and #7618 measured, now for a third and fourth module: the API does not make the bug fail to compile; the ledger is what denies the escape hatch. Both reverted. That vacuity, and the obligation to run this arm, is now written into
MIGRATED_MODULES' doc comment so the next slice cannot skip it by accident.Verification (local — the CI backlog is deep, so this is the evidence, and it is stated as such)
gc-root-dominance, both gated modes, post-change compiler: corpus 129/129 sources → 149 modules, 2452 functions, 9826 root stores → 0 violations;--seeded-violations 40→ 40 planted, 40 caught, 0 missed;--unrooted-allocas --moving-only→ 0. Baseline arm on the same corpus: 9810 root stores, 7861 gc-capable allocas, also 0/0. The +16 / +2 delta is how you can tell the gate's subject was live rather than absent.--self-test,--audit-alloc-re(71 alternatives),--audit-poll-capable(54 entries),--audit-immovable-sources(4 probes, 0 failing).cargo test -p perry-codegen --lib— 691 pass, 0 fail, including all four ledger tests.--doc— bothcompile_fail,E0499arms still reject.cargo test -p perry-runtime --no-fail-fast— 1886 pass, 0 fail, clean on the first run (no rerun needed; cargo-test: perry-runtime's suite fails a different number of tests on every run #7365's known flake did not appear).array,buffer,path,map,set,date,object,typed,process,iterator,weakref,error,fs,regexp,math,uint8): 60 tests, 60 pass, 0 parity fail, 0 compile fail, 0 crashed, 0 skipped — identical failure sets, because the set is empty on both. (test_gap_finaliz*matches no file; the harness exits 1 on an empty selection, which is selection, not a result.)lintscript set, enumerated from.github/workflows/test.ymlrather than remembered — 27 invocations. All pass except one, which is red onmaintoo (see below).Not run locally: the dependency-scale (
zod) dominance corpus, which needsnpm ci.Pre-existing failures found, flagged rather than smuggled in
1.
benchmarks/ci_public_baseline_check.pyis red onmain.This PR's diff is three files under
crates/perry-codegen/src/; it cannot cause this. #7618 recorded the same step failing on everymainrun from 2026-07-29 onward. Becauselintis a sequence and a failing step skips the rest, this is CLAUDE.md hazard 4 in the costume the workflow's own comment names — except the later steps now carryif: ${{ !cancelled() }}, so they do still speak.2.
path.resolve(base, f(), …)throwsERR_INVALID_ARG_TYPEwhere node returns the path — an SSO bug, not a rooting bug.Expr::PathResolveJoinunboxes its operands withunbox_to_i64, so a short computed string's inline SSO bytes are read as aStringHeader*. Bisected by string length: a segment longer than the SSO threshold works, a short one does not. The correct helper isunbox_str_handle— but that one allocates (SSO materialisation), so applying it opens the #7213 window in the same arm, which is a rooting change with its own combinator question. Filed as #7621. Deliberately not fixed here: this PR is IR-identical-or-justified by construction, and a semantic fix to five path arms is not something to hide inside a refactor. Confirmed present on both arms.3. Codegen output is not run-to-run deterministic (#7622). See the IR-identity section: string-constant numbering,
js_register_function_nameorder, andperry_method_*callee selection permute between two compiles of the same source with the same binary. This does not affect correctness of any arm here, but it does mean a byte-level IR A/B needs the multiset/normalised comparison this PR used, notdiff.Not in this PR
No version bump. No behaviour change beyond the itemised rooting fixes — no instruction kind is deleted. No gate widening, no allowlist entry, no new test file (and therefore no
check_test_registration.pyregistration to make). The SSO bug above, the benchmark-baseline gate, and the nondeterminism are reported, not repaired.Advances #7615. Slice 2 (
expr/instance_misc1.rs,expr/logical_collections.rs,lower_call/property_get/map_set.rs) is next.Summary by CodeRabbit
Bug Fixes
undefinedresults for out-of-range typed-array and buffer access.Documentation
Chores