fix(codegen): the lowering-window cluster — push order, member updates, ctor args, computed keys (#7634, #7628, #6998, #6986, #7640) - #7699
Conversation
|
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 rooting and re-read logic for member updates, computed access, constructors, and rebindable array pushes. It also corrects ChangesMember update rooting
Push receiver evaluation
Computed access and constructor rooting
Typed-array pointer classification
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Root-dominance, both arms, on the final treeCorpus generated with this branch's compiler ( Shadow — Native — Exactly at the #7679 ratchet — no new hits, and none removed either, so the budget is unchanged. The number that mattered here was
|
![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
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/lower_call/new.rs (1)
282-298: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
lower_constructor_argfor allReadlineoperands.
group.lowerandlower_exprdo not clearctx.discard_expr_value. Ifnew Readline(...)is in a discarded expression statement, a typed-array-store operand can produce0instead of its JavaScript value. This bypasses the constructor-argument behavior thatadopt_constructor_argsenforces.Proposed fix
Some(first) => { let collects = rooting::any_operand_may_collect(ctx, args[1..].iter()); - Some(group.lower(ctx, first, collects)?) + let value = lower_constructor_arg(ctx, first)?; + Some(group.adopt(ctx, first, &value, collects)) } @@ Some(second) => { let collects = rooting::any_operand_may_collect(ctx, args[2..].iter()); - Some(group.lower(ctx, second, collects)?) + let value = lower_constructor_arg(ctx, second)?; + Some(group.adopt(ctx, second, &value, collects)) } @@ for extra in args.iter().skip(2) { - let _ = lower_expr(ctx, extra)?; + let _ = lower_constructor_arg(ctx, extra)?; }🤖 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/lower_call/new.rs` around lines 282 - 298, The Readline constructor argument lowering must use lower_constructor_arg for output, options, and every extra operand instead of group.lower and lower_expr. Update the argument handling around the Readline lowering branch, preserving the existing collects calculations and optional-argument behavior while ensuring all operands follow adopt_constructor_args semantics.
🤖 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/issue7628_rooting_tests.rs`:
- Around line 55-62: Update the stale module references in the doc comments
around the test description and line 178, replacing expr/instance_misc1.rs with
expr/member_update.rs. Keep the surrounding explanation unchanged.
---
Outside diff comments:
In `@crates/perry-codegen/src/lower_call/new.rs`:
- Around line 282-298: The Readline constructor argument lowering must use
lower_constructor_arg for output, options, and every extra operand instead of
group.lower and lower_expr. Update the argument handling around the Readline
lowering branch, preserving the existing collects calculations and
optional-argument behavior while ensuring all operands follow
adopt_constructor_args semantics.
🪄 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: 0c961dac-8141-4dbd-82f3-c8fe4c19757a
📒 Files selected for processing (17)
changelog.d/7699-member-update-rooting.mdchangelog.d/7699-new-and-computed-key-windows.mdchangelog.d/7699-push-receiver-order.mdchangelog.d/7699-uint8array-get-pointer-classification.mdcrates/perry-codegen/src/collectors/pointer_locals.rscrates/perry-codegen/src/expr/array_push.rscrates/perry-codegen/src/expr/index_get.rscrates/perry-codegen/src/expr/index_set.rscrates/perry-codegen/src/expr/instance_misc1.rscrates/perry-codegen/src/expr/issue7628_rooting_tests.rscrates/perry-codegen/src/expr/member_update.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/slice8_rooting_tests.rscrates/perry-codegen/src/lower_call/new.rstest-files/test_gap_7628_index_update_rooted.tstest-files/test_gap_7634_push_receiver_order.tstest-files/test_gap_7640_computed_key_windows.ts
| //! # The remaining test is a PIPELINE assertion, and says so | ||
| //! | ||
| //! [`the_emitted_ir_rereads_both_operands_below_the_read`] holds because of | ||
| //! `root_reload`, not because of this file's source form, and it cannot fail on | ||
| //! a change to `expr/instance_misc1.rs` alone. It is kept anyway because it can | ||
| //! fail on a `root_reload` regression for this shape, which is a property | ||
| //! nothing else pins. It is NOT evidence about the lowering, and naming it | ||
| //! otherwise is how a green gate stops meaning anything. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale module reference in the doc comment.
Line 59 says the test cannot fail on a change to expr/instance_misc1.rs alone. The two arms now live in crates/perry-codegen/src/expr/member_update.rs. The same stale path appears at line 178. Point both at expr/member_update.rs so the reasoning stays traceable after the move.
📝 Proposed doc fix
-//! `root_reload`, not because of this file's source form, and it cannot fail on
-//! a change to `expr/instance_misc1.rs` alone. It is kept anyway because it can
+//! `root_reload`, not because of this file's source form, and it cannot fail on
+//! a change to `expr/member_update.rs` alone. It is kept anyway because it can-/// (measured — see the module header), so it cannot fail on a change to
-/// `expr/instance_misc1.rs` alone. What it can catch is a `root_reload`
+/// (measured — see the module header), so it cannot fail on a change to
+/// `expr/member_update.rs` alone. What it can catch is a `root_reload`🤖 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/issue7628_rooting_tests.rs` around lines 55 -
62, Update the stale module references in the doc comments around the test
description and line 178, replacing expr/instance_misc1.rs with
expr/member_update.rs. Keep the surrounding explanation unchanged.
ES2024 evaluates the MemberExpression `arr.push` to a Reference before the argument list, so the push lands on the array `arr` named at that moment. Both arms of expr/array_push.rs lowered the argument first and read the receiver afterwards, so an argument that rebound the receiver redirected the push onto its replacement. Gated on the divergence being observable: `push_receiver_is_rebindable` is the as-if test, so the hot shape (a plain local nothing else can reach) keeps its historical order, its inline tiers and its `Reuse` verdict, and emits no rooting IR. When it fires, the spec fix and the rooting fix are one change — the receiver becomes an operand of `with_operands_rooted_across`. The reorder alone is not sufficient: every fast tier publishes the reallocated head back into the binding unconditionally, which lands on the wrong array once the argument may have rebound it. The spec-ordered arm guards its write-back on the binding still naming the array that was pushed onto.
The member read-modify-write arms move to RootedGroup, which re-reads at any number of caller-chosen points — the combinator #7628 asked for, already built by #7615 slice 6. The operand half the issue filed is NOT a live bug: with the per-use re-reads collapsed back to one, and with PropertyUpdate's receiver root removed entirely, the emitted IR is unchanged — root_reload (#7280) rematerialises the slot load at each use, including through the ptrtoint + POINTER_MASK handle derivation. Kept anyway (free, and it drops the dependence on a pass with a documented side condition), documented as belt-and-braces, and its two tests are named as pipeline assertions. The repair is the RESULT: for a BigInt element js_to_numeric / js_numeric_step return a heap BigIntHeader, and the value the expression yields is live across a user setter as a bare call result with no slot to reload from. adopt_emitted closes it, gated on is_provably_not_bigint so a typed-array update pays nothing. instance_misc1.rs was 4 lines under the 2000-line cap, so the two arms move to expr/member_update.rs.
…mber (#6998) `const it = u8[Symbol.iterator]` lowers to `Uint8ArrayGet { index: SymbolFor }` — verified on the emitted HIR, which is the reachability the issue left open — and the collector typed it Number unconditionally, so the local got no shadow slot and the value was invisible to the collector. The arm now answers Number only for a structurally numeric key (the same `index_is_definitely_numeric` proof the IndexGet typed-array arm uses) and None otherwise. Structural on purpose: a `number`-declared index local is not evidence, and the sharper `expr_is_known_non_pointer_shadow_value` test needs an FnCtx this collector runs before. The byte-read arm #6996 paid to keep free is unchanged and pinned. Note in the changelog: the runtime consequence is currently masked by a second, behavioural defect — five other collectors make the same unconditional assumption and force the i32 lowering, so `u8[<non-numeric key>]` reads a byte instead of the property. Filed separately; this is a latent-soundness fix that becomes load-bearing when that is repaired.
…dows (#6986, #7640) #6986: three branches of lower_new_impl_inner early-return before the main class loop adopts into the enclosing RootedGroup, so the scope was open around them and empty — new Readline(...), new <importedFn>(...) and new Function(...) held argument registers across each other's lowering. They now adopt as each operand is produced (interleaved, never appended — a rooted finished list publishes an already-dangling argument 0) and re-read at the call. #7640 section D: unbox_str_handle calls js_get_string_pointer_unified, which materialises an SSO value into a fresh heap StringHeader. Six sites took the receiver's raw untagged pointer above it — #7280 taxonomy (a), which the rooting API structurally cannot express. Four are pure statement swaps at zero cost; two are cross-block and re-derive the handle in the string sub-block. #7640 section B: seven arms of index_get.rs lowered a receiver, then an unconstrained index (o[f()]), then used the receiver, with no rooting decision at all. Each is now one with_operands_rooted group over [object, index]; where the index provably cannot collect, operand_protection answers Reuse and the group emits nothing. Also fixes a migration-ledger violation the array_push tests introduced, and drops a js_gc_temp_root_push count that reads zero on the default build (temp roots lower to a plain alloca store in alloca mode) for the discriminating spec-order-block assertion. Still open on #7640: section A's typed-array stores, section C's statepoint claim, section E's callees. Still open on #6986: builtin.rs's ~22 arms, which need rooting threaded through lower_builtin_new's signature.
…7640) Re-deriving `obj_handle` inside the string sub-block SHADOWED the entry block's, which the NUMERIC sibling block still uses — and a definition in the string block does not dominate it. The LLVM verifier rejected the module ("Instruction does not dominate all uses"), which took 10 corpus sources including test_gap_gc_index_get_receiver_rooting out of the root-dominance corpus as silent "skipped" entries. Distinct names; corpus back to 130/130.
a91545c to
a1e1340
Compare
Re-run after rebasing onto
|
Audit — merging as v0.5.1397#7634 verified independently.
The push was landing on the replacement array instead of the original — a wrong answer with no diagnostic. The finding that both the issue and my brief missed
That is the difference between a fix and a half-fix, and it is not visible from the spec text — you only see it by reading what the fast tiers do after the push. Dropping the inline tiers on the spec-ordered arm and guarding the write-back on the binding still naming the pushed array, with aliases staying valid through #233's forwarding pointer, is the right shape. The Gating on #7628: your correction to my taxonomy is the durable part
I have been passing that taxonomy to every agent for a week. Establishing it by sabotage — collapse the re-reads, remove the receiver root, IR unchanged — rather than by argument is what makes it stick. And renaming the two non-discriminating tests as pipeline assertions rather than leaving them looking like gates is exactly right: a test that cannot fail is worse the more it looks like a guard. The real repair being the result (case (d), no slot) for a BigInt element crossing a user setter, gated on #6998: finding the second bug is worth more than the first
The self-caught bug is the most instructive thing in the report
That is #7024's shape exactly, and it means a verifier rejection can shrink the corpus by 8% while the gate reports success. You fixed the code bug; the gate that hid it is still open, and I am closing that separately — a floor of 90 against a 130-source corpus lets a third of it disappear silently. Gates: 24/24 lint, fmt clean, The |
…lowering (#6986) 30 arms of lower_call/builtin.rs's lower_builtin_new lowered args[0] then args[1] (then, for several, discarded the rest for side effects) with plain lower_expr and no rooting decision — the same #6969 shape #7699 fixed in lower_new.rs's three non-class branches, left open by that PR for this file. WeakMap/WeakSet are a variant: the iterable was lowered, then js_weakmap_new (an unconditional allocation) ran, and only then was the iterable's now-possibly-stale register read. lower_builtin_new now takes the caller's RootedGroup (threaded in from lower_new_impl_inner, which already opens one per #6969/#7699) and three helpers adopt each operand into it as it is produced, never after the fact. CronJob needed a bespoke ordering: its raw-pointer derivation can itself allocate, so it has to run before the other two operands are re-read. Left out of scope: the extract_options_fields-based arms (Response, Request, Blob, File, Headers, ReadableStream, WritableStream, TransformStream) share the hazard but are a structurally different shape.
…lowering (#6986) (#7719) * fix(gc): root builtin.rs's constructor arguments across each other's lowering (#6986) 30 arms of lower_call/builtin.rs's lower_builtin_new lowered args[0] then args[1] (then, for several, discarded the rest for side effects) with plain lower_expr and no rooting decision — the same #6969 shape #7699 fixed in lower_new.rs's three non-class branches, left open by that PR for this file. WeakMap/WeakSet are a variant: the iterable was lowered, then js_weakmap_new (an unconditional allocation) ran, and only then was the iterable's now-possibly-stale register read. lower_builtin_new now takes the caller's RootedGroup (threaded in from lower_new_impl_inner, which already opens one per #6969/#7699) and three helpers adopt each operand into it as it is produced, never after the fact. CronJob needed a bespoke ordering: its raw-pointer derivation can itself allocate, so it has to run before the other two operands are re-read. Left out of scope: the extract_options_fields-based arms (Response, Request, Blob, File, Headers, ReadableStream, WritableStream, TransformStream) share the hazard but are a structurally different shape. * chore: key the changelog fragment on PR #7719 * chore: bump version to 0.5.1416 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Closes #7634, #7628, #6998. Advances #6986 (the three
lower_newbranches it names;builtin.rs's ~22 armsstay open, with the inventory on the issue) and #7640 (sections B and D; A, C and E stay open).
Also files #7700 — a behavioural gap found while verifying #6998.
The codegen lowering-window cluster: five issues, all "a lowering holds a GC value across something that can collect, or evaluates operands in the wrong order". One commit per issue, plus one for a bug I introduced and caught.
#7634 —
arr.push(f())evaluated the receiver AFTER the argumentSpec bug first. ES2024 evaluates the
MemberExpressionarr.pushto a Reference before the argument list, so an argument that rebinds the receiver cannot redirect the push. Perry pushed onto whatever the binding named afterwards.The fix is gated on the divergence being observable, and that is the substance of it. A blanket reorder puts every push into a rooting window —
rows.push({...}),out.push(f(x))— on what #7511 measured as the hottest store family in the compiler.push_receiver_is_rebindableis the as-if test: the two orders name the same array unless the argument assigns the receiver's id itself, or the binding is boxed (collect_boxed_vars' rule is "captured AND mutated", so a captured-but-never-assigned array stays on the fast path) or a module global and the argument can reach a collection point.Evidence for the no-cost claim:
--trace llvmover a 1000-iterationout.push(mk(i))loop plus a capturedrows.push(i * 2)arrow contains zerocall i32 @js_gc_temp_root_pushand zero spec-order blocks.One thing the issue did not anticipate: the reorder alone is not sufficient. Every fast tier publishes the reallocated array head back into the binding unconditionally, and once the argument may have rebound it that store lands on the wrong array (
a.push(f())would overwrite[9]with the grown[1,2]). The spec-ordered arm therefore skips the inline tiers and guards its write-back on the binding still naming the array that was pushed onto; when it does not, the store is skipped and aliases stay valid through the forwarding pointerjs_array_push_f64installs (#233).test-files/test_gap_7634_push_receiver_order.tsmatches node byte-for-byte, includingpush's own result and the alias that keeps the array the push landed on.#7628 —
a[i]++/o.f++Both arms move to
RootedGroup, which re-reads at any number of caller-chosen points. The issue asked for that combinator; slice 6 had already built it, so no new primitive arrives with this caller.The operand half the issue filed is not a live bug, and the sabotage arm is how that was established rather than argued. Collapsing the per-use re-reads back to one — and, for
PropertyUpdate, removing the receiver's root outright — leaves the emitted IR unchanged in the relevant respect.root_reload(#7280) rematerialises the slot load at every use a collection point can reach, including through theptrtoint+and POINTER_MASKhandle derivation that #7280's own taxonomy lists as case (a), the class it cannot repair:That taxonomy entry is about a raw handle a helper returns, not one masked out of a NaN-boxed value the pass has spilled. The per-use re-reads are kept — free, and they drop the dependence on a pass with a documented side condition — but they are documented as belt-and-braces and their two tests are named as pipeline assertions rather than lowering assertions.
The repair is the result. For a BigInt element
js_to_numeric/js_numeric_stepreturn a heapBigIntHeader, and the value the expression yields is live across a user setter as a bare call result with no slot forroot_reloadto reload from — the taxonomy's case (d).RootedGroup::adopt_emittedcloses it, gated onis_provably_not_bigintso a typed-arrayta[i]++keeps the IR it had. Sabotaging that gate turns the test red; the typed-array arm is the measured counterfactual.#6998 —
pointer_localstyped everyUint8ArrayGetas a NumberReachability, which the issue explicitly left open, is now established on emitted HIR rather than argued —
const it = u8[Symbol.iterator]givesLet { ty: Any, init: Uint8ArrayGet { index: SymbolFor(…) } }. The arm now answersNumberonly for a structurally numeric key andNoneotherwise; the byte-read arm #6996 paid to keep free is unchanged and pinned.Found while verifying it, and worth more than the fix: five other collectors and
type_analysis/numeric.rsmake the same unconditional assumption and force the i32 lowering, so a non-numeric key on a typed-array-typed local reads a byte instead of the property (typeof u8[Symbol.iterator]isnumber;u8[k]withk = "byteLength"is0). No heap value reaches such a local today, so this is a latent-soundness fix that becomes load-bearing when that is repaired. Filed separately rather than folded in — those collectors decide thebuf[i]i32 fast path.#6986 — constructor arguments on
lower_new's non-class branchesThree branches early-
returnbefore the main class loop ever adopts into the enclosing scope, so the scope was open around them and empty:new Readline(…),new <importedFn>(…),new Function(…). All three now adopt as each operand is produced — interleaved, never appended — and re-read at the call.lower_js_args_arrayis no rescue and is untouched: a plainalloca_entry_arraypack with nojs_shadow_slot_bind.Not closed:
builtin.rs's ~22 arms.lower_builtin_newtakes no rooting context, so they are unreachable from anew.rs-level fix; the inventory is on the issue.#7640 — sections D and B
Section D, all six sites.
unbox_str_handlecallsjs_get_string_pointer_unified, which materialises an SSO value into a fresh heapStringHeader. Six sites took the receiver's raw untagged pointer above it. Four are pure statement swaps at zero cost; two are cross-block and re-derive the handle in the string sub-block.Section B, seven arms of
index_get.rsthat lowered a receiver, then an unconstrained index (o[f()]), then used the receiver, with no rooting decision at all. Each is now onewith_operands_rootedgroup over[object, index]; where the index provably cannot collect,operand_protectionanswersReuseand the group emits nothing.Sections A, C and E stay open.
A bug this PR introduced and caught
The cross-block section-D repair first shadowed the entry block's
obj_handle, which the numeric sibling block still uses — a definition in the string block does not dominate it. The LLVM verifier rejected the module, and the root-dominance corpus recorded that as ten silently "skipped" sources, includingtest_gap_gc_index_get_receiver_rooting. Distinct names; corpus back to 130/130, 0 skipped. Recorded here because "skipped" reading as neutral in a corpus generator is the shape of #7024.Verification
.node-version), byte-for-byte, for all three new gap files.test-files/sources matchingindex|array|buffer|uint8|string|push|typed|symbol|proxy|getter: 69 pass, 0 skip, 1 diff —test_compat_buffers_typed'stoSorted/toReversed, A/B'd against a compiler built fromd1aa968e9and byte-identical there, so pre-existing.cargo test -p perry-codegen --lib --no-fail-fast: 786 pass.cargo test -p perry-runtime --lib: 1935 pass.cargo check --all-targets: clean.native_root_coveragetests: pass.lintcommands extracted fromtest.yml, pluscargo fmt --all -- --check: pass.--moving-only --seeded-violations 40): 40 planted, 40 caught, 0 missed; corpus 130/130.--statepoints --moving-only --max-unrooted 11): see the comment below for the result.Sabotage arms were run for every acceptance test that claims a repair, and the two that could NOT be made to fail are relabelled rather than left looking like gates.
Summary by CodeRabbit
Bug Fixes
Array.prototype.pushwhen arguments can rebind the receiver.Tests
Documentation