refactor(codegen): Layer 1 rooting migration slice 6 — the multi-point re-read scope (#7615) - #7651
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds ChangesRooted call lowering
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CallLowering
participant RootedGroup
participant ExpressionLowering
participant RuntimeDispatch
CallLowering->>RootedGroup: create rooted operand scope
RootedGroup->>ExpressionLowering: lower arguments left to right
ExpressionLowering-->>RootedGroup: return lowered operands
RootedGroup->>RootedGroup: reread operands after allocations
RootedGroup->>RuntimeDispatch: dispatch rooted arguments
RuntimeDispatch-->>CallLowering: complete call lowering
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-codegen/src/rooting.rs (1)
717-722: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the
AccArrayopacity claim, or brand the handle.
AccArrayis a bareusizeindex and isCopy. Nothing ties it to the group that produced it, so a handle from oneRootedGroupcan index another group'saccsand select the wrong slot, or panic on an out-of-range index. Both groups arepub(crate)in the same crate, so the compiler does not prevent this. Either soften the doc claim or add an invariant that ties the handle to its group.♻️ Option: state the actual guarantee
/// A handle on one accumulator array inside a [`RootedGroup`]. /// -/// Opaque and `Copy`: it indexes the group's own list, so it cannot name a slot -/// belonging to a different group or outlive the release. +/// Opaque and `Copy`: it indexes the group's own list, and the release consumes +/// the group, so a handle cannot outlive the scope. It is NOT branded per +/// group — pass a handle only to the group that returned it. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(crate) struct AccArray(usize);🤖 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/rooting.rs` around lines 717 - 722, Correct the documentation on AccArray to state only the guarantee provided by its usize index representation: it identifies an accumulator slot but is not tied to or validated against a particular RootedGroup and must not be used across groups. Do not claim group ownership or lifetime enforcement unless the handle is redesigned to carry a group-specific invariant.
🤖 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/src/rooting.rs`:
- Around line 717-722: Correct the documentation on AccArray to state only the
guarantee provided by its usize index representation: it identifies an
accumulator slot but is not tied to or validated against a particular
RootedGroup and must not be used across groups. Do not claim group ownership or
lifetime enforcement unless the handle is redesigned to carry a group-specific
invariant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b4b04431-973e-42f3-bfcf-655d03de5414
📒 Files selected for processing (12)
changelog.d/7651-layer1-slice6-rooted-group.mdcrates/perry-codegen/src/expr/temp_root.rscrates/perry-codegen/src/lower_call/console_promise.rscrates/perry-codegen/src/lower_call/console_rooting_tests.rscrates/perry-codegen/src/lower_call/early_branches.rscrates/perry-codegen/src/lower_call/extern_func.rscrates/perry-codegen/src/lower_call/func_ref.rscrates/perry-codegen/src/lower_call/method_override.rscrates/perry-codegen/src/lower_call/mod.rscrates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rscrates/perry-codegen/src/lower_call/property_get/static_dispatch.rscrates/perry-codegen/src/rooting.rs
💤 Files with no reviewable changes (1)
- crates/perry-codegen/src/expr/temp_root.rs
Follow-up commit: two more windows, found by auditing the arms the migration did not have to touch
1.
Compiled from ; before
%r55 = invoke i64 @js_array_alloc(i32 3)
%r58 = invoke double @..._churn(...) ; user code: allocates, polls
%r60 = invoke i64 @js_array_push_f64(i64 %r55, double %r58) ; %r55 from above the churn
%r63 = invoke double @..._churn(...)
%r65 = invoke i64 @js_array_push_f64(i64 %r60, double %r63) ; %r60 from above the churn
%r68 = invoke double @..._churn(...)
%r70 = invoke i64 @js_array_push_f64(i64 %r65, double %r68) ; %r65 from above the churn
; after — the accumulator is re-read from its root between every push
%r68 = invoke double @..._churn(...)
%r70 = <load from the root slot>
%r71 = invoke i64 @js_array_push_f64(i64 %r70, double %r68)
%r78 = invoke double @..._churn(...)
%r80 = <load from the root slot>
%r81 = invoke i64 @js_array_push_f64(i64 %r80, double %r78)2. Neither faults at runtime in the arrangements tried —
Gates re-run after this commit: 20/20 lint commands, |
…t re-read scope (#7615) Slice 5 could not take three `lower_call/` modules and named the reason: they need a re-read at more than one point, and every `with_operands_rooted*` form has exactly one. Its hypothesis for the missing combinator was the variadic/rest shape. That turned out to be one instance of it rather than the shape itself. `crate::rooting::RootedGroup` is ONE temp-root scope — already-lowered operands and mutable accumulator arrays together — re-readable at any number of caller-chosen points and released once, for the whole stack. The rest/variadic case is the one that also holds an array. Two entry points, and the asymmetry is argued in the source: `with_rooted_group` owns the release; `open_rooted_group` hands the scope back for the one shape where the release must post-dominate blocks the lowering does not lexically contain (`func_ref.rs`'s four block-splitting specialized-ABI diamonds). What escaping leaves writable is only the SAFE half of guard mismanagement — the scope is not `Clone`, `release` consumes it, and there is no way to obtain the slot index, so an early or mis-ordered truncate is unwritable and a forgotten one is over-retention. `implicit_this_save` / `implicit_this_restore` MOVED into `crate::rooting` rather than being re-exported, so the pair has one spelling. Migrated and listed in the ledger, all three load-bearing on the committed source: `lower_call/mod.rs`, `lower_call/func_ref.rs`, `lower_call/console_promise.rs`. Three live bugs in the `console.*` arms, all A/B'd byte-for-byte against node 26.5.1 (#7649 and two siblings the audit found): * `console.dir(x, y, sideEffect())` lowered `args[2..]` AFTER the call that prints, so the side effect ran second; * `console.time(label, sideEffect())` never lowered the surplus arguments at all, so the side effect never happened; * `console.table(a, b, c)` fell through to the generic multi-arg console.log arm and printed the array instead of a table. Plus #7649's rooting half, demonstrated in IR: `console.table`/`console.dir` held operand 0 in a bare SSA register across operand 1's lowering. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…sync's operand chain (#7615) Two windows the slice-6 audit found in arms the migration did not have to touch. Promise.try threaded a raw *mut ArrayHeader through its push loop in a bare SSA register while the next argument -- arbitrary user code -- was lowered, and held the callback in another bare register across all of it: #7154's accumulator shape verbatim, and the same defect slice 5 found in namespace_call.rs's rest path. Array.fromAsync held each of its three operands across the others' lowering (#7280 taxonomy (c)). Array.fromAsync keeps take(3) so this stays a pure rooting fix: the Promise.* statics and fromAsync alike fail to EVALUATE their surplus arguments, which is a separate node-visible defect. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
#7615) Review caught the doc claiming a handle 'cannot name a slot belonging to a different group'. It is a bare usize index and both types are pub(crate), so nothing enforces that. State what the handle DOES buy — it is not a slot index, so it cannot be truncated or mis-ordered — and say plainly that it is not branded per group. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Audit — merging as v0.5.1369Three node-visible bugs, all reproduced independentlyBuilt a baseline from
The second is the one I'd have missed reviewing this by eye: the argument was never lowered, so the evaluation silently vanished. And the third is a good demonstration that an arity gate is a behavioural decision — The combinator, and slice 5's hypothesis being wrongSlice 5 said the gap was "the variadic/rest shape". You checked and only one of its three blocked modules is variadic — the other two want re-reads at two instructions and at four block-splitting diamonds ~450 lines apart. Generalising to "one temp-root scope, re-readable at any number of caller-chosen points, released once" is the right abstraction, and it is better for having been derived from all three rather than from the one that suggested itself. The two-entry-point argument is the part I'd have pushed back on if it were asserted rather than argued: an early or mis-ordered release is a use-after-free, a forgotten one is over-retention, so Declining to list Verified independentlyLedger sabotage: I planted a compiling Gates: 20/20 from the Cost: 35 lines removed, 0 added, corpus root stores 9846 → 9799 — two unconditional roots on provably non-pointer values that bypassed Your three corrections, all verified
|
aceba5f to
7afaad5
Compare
1 participant
Slice 6 of the Layer 1 rooting migration (#7615). Slices 1–5 are merged; #7648 was slice 5.
The API gap first, because that was the assignment
Slice 5 left three
lower_call/modules unmigrated and recorded why: "three ofthem need a re-read at more than one point, and every
with_operands_rooted*form has exactly one. The concrete missing combinator is the variadic/rest shape
(per-element re-reads between allocating pushes)."
The first half of that is right and the second half is not, and it matters.
Only one of the three modules is variadic. Reading all three:
console_promise.rs's dynamic closure calljs_closure_unbox_callee_checked_rebind(which clones athis-capturing closure), the arguments feedjs_closure_callNbelow itmod.rs'slower_rest_call_args_rootedjs_array_push_f64func_ref.rsThe common request is not "variadic". It is one temp-root scope, re-readable
at any number of caller-chosen points. The variadic case is that scope with an
accumulator array in it, which is why
RootedGroupcarries both operands andarrays rather than there being a second type.
Two entry points, argued rather than assumed
with_rooted_groupowns the release like every other combinator in the file.open_rooted_grouphands the scope back — which the rest ofrooting.rsdeliberately refuses to do — so it needs a justification, and the justification
is that the two halves of guard mismanagement are not equally dangerous:
cut: truncating the wrong slot drops everything above it, which is how a
saved receiver becomes the number
0);stays live, the code is merely conservative.
RootedGroupremoves the dangerous half by construction, for both entrypoints: not
Clone,releaseconsumes it, and no way to obtain the slotindex. Escaping leaves only the safe half writable. That is strictly better than
the
Option<String>slot index it replaces, which the caller could truncateanywhere. Inverting control in
func_ref.rsinstead would not remove the hazard,it would relocate it into a 450-line closure.
implicit_this_save/implicit_this_restoremoved intocrate::rootingrather than being re-exported, so the pair has one spelling — two would be the
drift that produced #7114.
Modules migrated
Three, all load-bearing on the committed source (each named
expr::temp_rootbefore this change):
lower_call/mod.rs,lower_call/func_ref.rs,lower_call/console_promise.rs.early_branches.rs,method_override.rsand bothproperty_getdispatchersare now clean of the escape hatch as a side effect of the
implicit_thismove,and are deliberately not listed: a ledger line asserts that a module makes
every rooting decision through this API, and nobody has read those four for
windows with no decision at all. That is slice 4's "listed ≠ audited"
distinction.
Live bugs
All three behavioural ones are byte-for-byte A/B'd against node 26.5.1 (the
.node-versionoracle) with a baseline compiler built frommainin a separatetarget dir, same
PERRY_GC_MOVING_LOOP_POLLSon both arms.mainconsole.dir(x, y, f())side:dir3then{ a: 1 }{ a: 1 }thenside:dir3console.time(l, f())side:time2console.table(a, b, c)[ { a: 1 } ] [ 'a' ] 1diffof node's output against this branch's is empty; againstmainit isfour hunks.
console.dirsequenced side effects after its own print (console.table / console.dir hold operand 0 unrooted across operand 1's lowering (lower_call/console_promise.rs) #7649's non-GCarm, which the issue flagged as unverified — now verified). It lowered
args[2..]belowjs_console_dir_with_options. Node evaluates the wholeargument list before invoking anything.
console.time/timeEnd/timeLog/count/countResetdroppedtheir surplus arguments entirely — not resequenced, never lowered. A new
find, same root cause.
console.table(a, b, c)stopped beingconsole.table. The arity gate wasargs.len() == 1 || args.len() == 2, so three arguments fell through to thegeneric multi-arg
console.logarm. Node ignores surplus arguments; it doesnot switch renderer.
console.table / console.dir hold operand 0 unrooted across operand 1's lowering (lower_call/console_promise.rs) #7649's rooting half, demonstrated in IR rather than at runtime. With
console.table(makeRows(), [churn(300)]),mainemitsroot_reloadstructurally cannot repair this — a call result has no slot tobe re-read from (GC: #7154's residual is NOT fixed — the loop-polls config is red 0/30, and stock zod alone fails 5/40 #7280 taxonomy (c) + (d)). This branch emits the root store
above
churnand reloads below it.The runtime fault is arrangement-dependent and I could not reproduce one.
Under
PERRY_GC_MOVING_LOOP_POLLS=1+PERRY_GC_ZEAL=1(copied_objects=6005confirmed via
PERRY_GC_DIAG=1, so the subject was live), plusPERRY_GC_PROTECT_FROMSPACE=1 …_DEPTH=800,mainprinted the correct tableand exited 0. The IR is the evidence the window exists; a
TypeErrorwouldonly have been evidence it is reachable in one arrangement. Stated here rather
than claimed away.
Zero cost where the window cannot collect — measured, not asserted
A probe exercising direct calls, rest calls,
arguments,new, method dispatch,dynamic closure calls,
Map.setand eightconsole.*arms, compiled both armswith
--trace llvm. Normalising SSA numbering and block labels, the entiresemantic delta is 35 removed lines and 0 added: two unconditional
temp_root_push_double/ re-read / clear sequences that were protecting valuesexpr_is_known_non_pointer_shadow_valueproves are not heap references — adoublefromb.scale(2)and the literaltrueinconsole.assert. Both armsbypassed the shared
operand_protectiondecision; routing them through it dropsthe traffic. Corpus-wide the root-store count goes 9846 → 9799.
Runtime output over the same probe:
mainand this branch identical.Tests
lower_call/console_rooting_tests.rs, five tests, asserting on IR orderingrather than slot counts (a count lets the other operand's rooting pay for the
assertion). Each asserts by callee name that the arm under test was reached, so
a shape measured over a lowering that never ran cannot pass.
Sabotage-verified against the pre-fix source, with the file reverted to its
HEADcontent and only the movedimplicit_thispath repointed so it stillcompiles:
error[count 0,Running unittestspresent (so the plantcompiled and the binary ran), 4 of 5 red. The fifth is the zero-cost pin,
which correctly passes in both arms.
Ledger sabotage, one arm per newly-listed module — a compiling
temp_root_push_double/temp_root_truncatepair planted in each:error[lower_call/mod.rslower_call/func_ref.rslower_call/console_promise.rsGates
All 20 lint-job commands enumerated from
.github/workflows/test.yml— 20/20pass. Plus
cargo fmt --all -- --check,cargo test -p perry-codegen --lib(711 pass),
cargo test -p perry-runtime --lib(1909 pass, 3 ignored),cargo check --all-targets(clean).scripts/gc_root_dominance_check.pyover the realgc_root_dominance_corpus.shcorpus (149 modules, 2452 functions), in thegate's own mode:
--unrooted-allocas: 6 violations, 0 moving-reachable, identical in botharms (the known #7210 residue).
Gap tests A/B'd (node vs
mainvs this branch): 7 console tests and 15call-shape / GC-rooting tests. All
base-vs-fix: same, allnode-vs-fix: match, excepttest_gap_console_methods, which differs from node in botharms by timer-microsecond values only.
Campaign note: the "24 modules" number is misleading
rg -l "temp_root::" crates/perry-codegen/srcreports 24, but 14 of them makeno rooting decision: five only construct
TempRootPool::default(), three onlycall a predicate (
expr_may_trigger_gc,expr_is_inert_primitive,operand_is_reloadable), and six only used theimplicit_thispair this PRmoved. After this change the modules with real un-migrated rooting decisions are:
lower_call/new.rs,expr/child_proc.rs,expr/dyn_extern_i18n.rs,expr/fs_await.rs,expr/math_simple.rs,expr/proxy_reflect.rs,expr/static_field_meta.rs.new.rsis the big one and now unblocked —RootedGroupis exactly itsrefresh_rooted_argsshape — but it is 1988 linesagainst the 2000-line cap, so it wants its own slice.
Closes #7649.
https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Summary by CodeRabbit
Bug Fixes
console.tablebehavior across supported argument counts.Promise.tryandArray.fromAsync.Documentation