gc: Layer 1 rooting slice 8 — the raw API becomes unreachable (#7615) - #7670
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)
📝 WalkthroughWalkthroughLayer 1 rooting migration is complete for the listed codegen slices. Raw rooting APIs moved into a private module, affected lowering paths use ChangesRooting migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/perry-codegen/src/expr/slice7_rooting_tests.rs (2)
133-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
assert_operand_survives_the_windowhas no cross-module consumer.
slice8_rooting_tests.rsimports onlyallocating,require_call_lineandtemp_root_calls. Its module header states explicitly that it does not reuse this helper, because a one-level definition check cannot see theExpr::ArrayMapwindow. Keeping the widened visibility suggests the opposite. Consider reverting this one to private.♻️ Proposed change
-pub(super) fn assert_operand_survives_the_window(ir: &str, callee: &str, n: usize, what: &str) { +fn assert_operand_survives_the_window(ir: &str, callee: &str, n: usize, what: &str) {🤖 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/slice7_rooting_tests.rs` at line 133, Revert the visibility of assert_operand_survives_the_window to private, since no other module consumes it and slice8_rooting_tests.rs explicitly does not reuse this helper.
133-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe slice 7 → slice 8 test-helper visibility split is applied inconsistently. Five helpers were involved. Three (
allocating,require_call_line,temp_root_calls) were widened and are imported. One (assert_operand_survives_the_window) was widened and has no consumer. One (compile_body) was not widened and was copied verbatim into slice 8 instead, under a rationale that describes a semantic difference that does not exist. Pick one rule — export what slice 8 uses, keep the rest private — and apply it to all five.
crates/perry-codegen/src/expr/slice7_rooting_tests.rs#L133-L133: revertassert_operand_survives_the_windowto private, sinceslice8_rooting_tests.rsdeliberately does not use it.crates/perry-codegen/src/expr/slice8_rooting_tests.rs#L172-L201: widenslice7_rooting_tests::compile_bodytopub(super)and import it, deleting this byte-identical copy; if the copy is kept, replace the "these arms need a local" rationale with the real reason.🤖 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/slice7_rooting_tests.rs` at line 133, The slice 7-to-slice 8 helper visibility and reuse rules are inconsistent. In crates/perry-codegen/src/expr/slice7_rooting_tests.rs:133-133, make assert_operand_survives_the_window private again because slice 8 does not use it; in crates/perry-codegen/src/expr/slice8_rooting_tests.rs:172-201, widen slice7_rooting_tests::compile_body to pub(super), import and reuse it, and remove the byte-identical local copy.crates/perry-codegen/src/lower_string_concat.rs (1)
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
_lhsbinding.
_lhsis never read. The underscore prefix hides it from the rustc unused warning, so it will stay indefinitely. The file move is the cheapest moment to drop it.♻️ Proposed cleanup
let lhs_val = ctx.block().load(DOUBLE, &slot); - let _lhs = lhs_val.clone(); let rhs_val = lower_expr(ctx, rhs)?;🤖 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_string_concat.rs` around lines 56 - 58, Remove the unused _lhs binding from the string-concatenation lowering flow, while retaining lhs_val and the subsequent lower_expr call used by lower_string_concat.crates/perry-codegen/src/lower_string_method.rs (1)
228-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider narrowing the raw
recv_boxparameter.
lower_string_method_dispatchnow receives both the raw receiver register and the group. Only theconcatarm reads the raw register, and it is safe there because it unboxes before any argument is lowered. Every other arm must usereread_recv. This keeps a stale register in scope for ~60 arms, which is the exact hazardRootedGroupremoves by never handing the lowered register back.One option: hoist the
concatarm's eager unbox intolower_string_methodnext toadopt_emitted, pass the resulting handle instead ofrecv_box, and give it a name that states it is pre-window (for examplerecv_handle_prewindow). A future arm then cannot pick up a boxed receiver by accident.Not blocking. The current code is correct.
🤖 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_string_method.rs` around lines 228 - 232, Narrow lower_string_method_dispatch by removing the raw recv_box parameter and eagerly unboxing the receiver in lower_string_method alongside adopt_emitted. Pass the resulting pre-window receiver handle with a name such as recv_handle_prewindow, update only the concat arm to use it, and keep all other arms on reread_recv.crates/perry-codegen/src/expr/slice8_rooting_tests.rs (1)
314-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe section header names
Expr::MapHas, but noMapHastest follows.
math_simple.rsmigratedMapGetandMapHastowith_operands_rootedin the same slice. OnlyMapGetis covered here. Either add the mirroringMapHascase or dropMapHasfrom the header at line 315.The
MapHascase is a near-copy ofmap_get_receiver_survives_an_allocating_keywithjs_map_hasas the consumer. Do you want me to generate it?🧪 Proposed test
/// `m.has(k)` has the same receiver-before-key ordering as `m.get(k)`. #[test] fn map_has_receiver_survives_an_allocating_key() { let ir = compile_body( "map_has_window", with_object_local( 1, Stmt::Expr(Expr::MapHas { map: Box::new(field_receiver(1)), key: Box::new(allocating("k")), }), ), ); assert_reread_below_operand( &ir, "js_map_has", 0, "js_map_has", 1, "Map.has evaluates the receiver first and the key second", ); }🤖 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/slice8_rooting_tests.rs` around lines 314 - 342, Add a mirroring test beside map_get_receiver_survives_an_allocating_key for Expr::MapHas, using the map_has_window fixture and asserting receiver rereading around the js_map_has consumer. Keep the receiver-before-key ordering assertion and update the test documentation to describe Map.has.
🤖 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 216-219: Update the tail of the lowering flow around
lower_new_impl_inner to call group.release(ctx) only when
ctx.block().is_terminated() is false; preserve returning result unchanged and
avoid emitting the release operation after lower_new_impl_inner has terminated
the block.
In `@crates/perry-codegen/src/rooting/temp_root.rs`:
- Around line 655-662: Update the doc comments for temp_root_release and guard
to remove the deleted lower_exprs_rooted reference and replace the obsolete
rest-argument-lowering wording with the current RootedOperands::guard,
RootedGroup::first_slot, and RootedGroup::adopt terminology. Keep the
documentation focused on the existing guard lifecycle and caller behavior.
---
Nitpick comments:
In `@crates/perry-codegen/src/expr/slice7_rooting_tests.rs`:
- Line 133: Revert the visibility of assert_operand_survives_the_window to
private, since no other module consumes it and slice8_rooting_tests.rs
explicitly does not reuse this helper.
- Line 133: The slice 7-to-slice 8 helper visibility and reuse rules are
inconsistent. In crates/perry-codegen/src/expr/slice7_rooting_tests.rs:133-133,
make assert_operand_survives_the_window private again because slice 8 does not
use it; in crates/perry-codegen/src/expr/slice8_rooting_tests.rs:172-201, widen
slice7_rooting_tests::compile_body to pub(super), import and reuse it, and
remove the byte-identical local copy.
In `@crates/perry-codegen/src/expr/slice8_rooting_tests.rs`:
- Around line 314-342: Add a mirroring test beside
map_get_receiver_survives_an_allocating_key for Expr::MapHas, using the
map_has_window fixture and asserting receiver rereading around the js_map_has
consumer. Keep the receiver-before-key ordering assertion and update the test
documentation to describe Map.has.
In `@crates/perry-codegen/src/lower_string_concat.rs`:
- Around line 56-58: Remove the unused _lhs binding from the
string-concatenation lowering flow, while retaining lhs_val and the subsequent
lower_expr call used by lower_string_concat.
In `@crates/perry-codegen/src/lower_string_method.rs`:
- Around line 228-232: Narrow lower_string_method_dispatch by removing the raw
recv_box parameter and eagerly unboxing the receiver in lower_string_method
alongside adopt_emitted. Pass the resulting pre-window receiver handle with a
name such as recv_handle_prewindow, update only the concat arm to use it, and
keep all other arms on reread_recv.
🪄 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: 6fc5641b-875a-4e5f-bd83-ffc7e6bcb26b
📒 Files selected for processing (29)
changelog.d/7670-layer1-rooting-slice8-terminal-condition.mdcrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/expr/binary.rscrates/perry-codegen/src/expr/dyn_extern_i18n.rscrates/perry-codegen/src/expr/fs_await.rscrates/perry-codegen/src/expr/literals_vars.rscrates/perry-codegen/src/expr/math_simple.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/proxy_reflect.rscrates/perry-codegen/src/expr/shadow_slot.rscrates/perry-codegen/src/expr/slice7_rooting_tests.rscrates/perry-codegen/src/expr/slice8_rooting_tests.rscrates/perry-codegen/src/expr/static_field_meta.rscrates/perry-codegen/src/lib.rscrates/perry-codegen/src/loop_purity.rscrates/perry-codegen/src/lower_call/mod.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_call/new_alloc.rscrates/perry-codegen/src/lower_string_concat.rscrates/perry-codegen/src/lower_string_method.rscrates/perry-codegen/src/lower_string_method/char_code_at.rscrates/perry-codegen/src/root_reload.rscrates/perry-codegen/src/rooting/mod.rscrates/perry-codegen/src/rooting/temp_root.rscrates/perry-codegen/src/stmt/loops.rsdocs/engine-plan.md
| let mut group = open_rooted_group(args.len() + 1); | ||
| let result = lower_new_impl_inner(ctx, class_name, args, caps_absent_from_args, &mut group); | ||
| group.release(ctx); | ||
| result |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether temp_root_release / temp_root_truncate guard on block termination.
set -euo pipefail
fd -t f 'temp_root.rs' crates/perry-codegen/src/rooting --exec ast-grep outline {} --items all
fd -t f 'temp_root.rs' crates/perry-codegen/src/rooting \
--exec rg -n -C 12 'fn temp_root_release|fn temp_root_truncate' {}
# Compare with the guarded caller and any other open_rooted_group users.
rg -n -C 4 'open_rooted_group\(' crates/perry-codegen/src
rg -n -C 3 'is_terminated\(\)' crates/perry-codegen/src/rootingRepository: PerryTS/perry
Length of output: 8736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== new.rs tail and release =="
sed -n '200,222p' crates/perry-codegen/src/lower_call/new.rs
echo "== string guarded release context =="
sed -n '160,205p' crates/perry-codegen/src/lower_string_method.rs
echo "== temp_root_truncate implementation =="
sed -n '220,255p' crates/perry-codegen/src/rooting/temp_root.rs
echo "== RootedGroup release and open_rooted_group definitions =="
rg -n -C 8 'struct RootedGroup|impl RootedGroup|fn open_rooted_group|pub fn release|fn release' crates/perry-codegen/src/rooting/mod.rs crates/perry-codegen/src/rooting
echo "== temp_root_release implementation =="
sed -n '650,662p' crates/perry-codegen/src/rooting/temp_root.rs
sed -n '630,662p' crates/perry-codegen/src/rooting/temp_root.rs
echo "== any_may_emit_unreachable / is_terminated usage in new/string =="
rg -n -C 3 'unreachable\(|is_terminated\(\)|Release|release' crates/perry-codegen/src/lower_call/new.rs crates/perry-codegen/src/lower_string_method.rsRepository: PerryTS/perry
Length of output: 31650
Guard group.release on terminated blocks.
lower_new_impl_inner can emit unreachable, and then the body returns Ok(...) before reaching this tail. group.release(ctx) calls temp_root_truncate, which emits a runtime call and is invalid after a terminator. Add the !ctx.block().is_terminated() guard before releasing the RootedGroup.
🤖 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 216 - 219, Update
the tail of the lowering flow around lower_new_impl_inner to call
group.release(ctx) only when ctx.block().is_terminated() is false; preserve
returning result unchanged and avoid emitting the release operation after
lower_new_impl_inner has terminated the block.
| /// Release a guard returned by [`lower_exprs_rooted`]. Call it *after* the | ||
| /// consuming call, not before: the consumer allocates while reading these | ||
| /// values. | ||
| pub(crate) fn temp_root_release(ctx: &mut FnCtx<'_>, guard: Option<String>) { | ||
| pub(in crate::rooting) fn temp_root_release(ctx: &mut FnCtx<'_>, guard: Option<String>) { | ||
| if let Some(idx) = guard { | ||
| temp_root_truncate(ctx, &idx); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the doc comment: lower_exprs_rooted was deleted in this change.
The doc on temp_root_release links to [lower_exprs_rooted], which this slice removed (the ledger doc in rooting/mod.rs lists it among the fourteen deleted items). The guard now comes from RootedOperands::guard or RootedGroup::first_slot. Rustdoc cannot resolve the link, and the prose points a reader at a symbol that no longer exists.
The same staleness applies to the guard() doc just above: it names "the rest-argument lowering" as the caller, but the in-module caller is now RootedGroup::adopt.
📝 Proposed doc fix
-/// Release a guard returned by [`lower_exprs_rooted`]. Call it *after* the
-/// consuming call, not before: the consumer allocates while reading these
-/// values.
+/// Release a guard returned by [`RootedOperands::guard`]. Call it *after* the
+/// consuming call, not before: the consumer allocates while reading these
+/// values.
pub(in crate::rooting) fn temp_root_release(ctx: &mut FnCtx<'_>, guard: Option<String>) {🤖 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/temp_root.rs` around lines 655 - 662, Update
the doc comments for temp_root_release and guard to remove the deleted
lower_exprs_rooted reference and replace the obsolete rest-argument-lowering
wording with the current RootedOperands::guard, RootedGroup::first_slot, and
RootedGroup::adopt terminology. Keep the documentation focused on the existing
guard lifecycle and caller behavior.
…w.rs (#7615) `new.rs` was 1,988 lines against `scripts/check_file_size.sh`'s 2,000-line cap, which blocked the Layer 1 rooting migration (#7615 slice 8) — that migration has to ADD lines to the file, replacing `refresh_rooted_args` and the `temp_root_scope_*` marker with a `RootedGroup`. Pure move, no behaviour change: `lower_new_impl_inner`'s field-count computation and its three-arm object allocation become `new_alloc::emit_instance_alloc(ctx, class_name, class) -> String`. `new_site_is_in_loop` moves with them (its only caller is the inline bump-allocator arm). The boundary is a boundary rather than a cut because none of the locals the block defines — `field_count`, `cid_str`, `parent_cid_str`, `n_str`, `packed_keys`, `alloc_field_count` — is read anywhere below the allocation. No rooting decision moves with it: everything the extracted block emits sits ABOVE the instance root, whose push is the caller's next act on the returned handle. new.rs 1,988 -> 1,501; new_alloc.rs 531. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…7615) `lower_string_method.rs` was 1,957 lines against the 2,000-line cap, and the Layer 1 rooting migration adds closure scopes to five of its functions — `with_operands_rooted` and `with_rooted_accumulator` both re-indent the body they own, which is line growth on a file with 43 lines of headroom. Pure move at the boundary the file already had: everything above `lower_string_self_append` dispatches a `str.<method>(...)` call, everything from it down lowers `a + b` / `s += x` on strings. `str_operand_handle_tag_dispatched` becomes `pub(crate)` because three dispatch arms above still call it. lower_string_method.rs 1,957 -> 1,368; lower_string_concat.rs 612. Also lands the first four module migrations of slice 8 (they share the `expr/binary.rs` import line with the move): * `expr/binary.rs` — five `lower_operand_pair_rooted` + `temp_root_release` pairs collapse into one `lower_rooted_dynamic_binary` helper over `with_operands_rooted`. * `expr/math_simple.rs` — `MapSet` becomes a `RootedGroup` (two operands, unequal windows, eight arm-specific re-read points); `MapGet`/`MapHas` become `with_operands_rooted`. `Expr::ArrayMap` gains the root it never had: the receiver was lowered, the callback was lowered, and only THEN was the receiver unboxed — the unbox sat below its own window. * `expr/static_field_meta.rs` — `ClassExprFresh` becomes a `RootedGroup` over the class object plus a nested `with_rooted_accumulator` for the `__perry_ctor_caps` snapshot array, which was threaded through a bare SSA register. * `expr/dyn_extern_i18n.rs` — the namespace-object build becomes `with_rooted_accumulator`. * `lower_call/new.rs` — `refresh_rooted_args` and the `temp_root_scope_begin`/`_end` marker become one escaping `RootedGroup`; the null marker slot is gone with them. `RootedGroup::adopt_emitted` gains a `protect` flag (the WINDOW, not the strategy) and `RootedGroup::is_rooted` returns whether a slot exists. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…7615) The campaign's terminal condition, made true: `expr/temp_root.rs` is now `crate::rooting::temp_root`, declared with a PRIVATE `mod temp_root;` and with every accessor additionally carrying `pub(in crate::rooting)`. The plan spelled the condition as "`expr/temp_root.rs` going `pub(in crate::rooting)`", which is not expressible in Rust — `pub(in path)` requires `path` to be an ancestor module of the item (E0742), and `crate::rooting` is not an ancestor of `crate::expr::temp_root`. Hence the move. Both belts are worn because either alone is one keyword from being undone. Two items keep `pub(crate)` and are re-exported from `rooting/mod.rs`; neither is an accessor and neither can be called in the wrong order: `TempRootPool` (compile-time slot bookkeeping `FnCtx` owns) and `expr_is_inert_primitive` (the shared "can evaluating this run user code?" predicate the loop back-edge poll consults). Fourteen items are DELETED rather than narrowed, because the migration left them with no caller: `lower_exprs_rooted`, `lower_operand_pair_rooted`, `any_later_ref_may_trigger_gc`, `RootedOperands::is_rooted`, the whole `StoreOperandGuard` family and the whole `RootedHandle` family, and `temp_root_scope_begin`/`_end`. CLAUDE.md's kill-policy: the losing mode should stop compiling. Eight modules migrate (seven load-bearing on the committed source, one — `lower_call/new_alloc.rs` — vacuous and listed anyway so an unlisted sibling of a listed module cannot become the place a raw push goes): `expr/binary.rs`, `expr/math_simple.rs`, `expr/static_field_meta.rs`, `expr/dyn_extern_i18n.rs`, `lower_string_method.rs`, `lower_string_concat.rs`, `lower_call/new.rs`, `lower_call/new_alloc.rs`. Nine further files mention the raw API and make no rooting decision, so they are deliberately NOT listed: `expr/mod.rs` (module declaration and a field type, both gone with the move), the four `FnCtx` constructors (`TempRootPool::default()`), `stmt/loops.rs` (one purity predicate), `loop_purity.rs` (a doc link only), and `root_reload.rs` / `gc_call_effects.rs` / `runtime_decls/arrays.rs` plus five test files, whose `js_gc_temp_root_*` occurrences are runtime SYMBOL NAMES. One live bug fixed: `Expr::ArrayMap` lowered the receiver, lowered the callback, and only then unboxed the receiver — the unbox sat below its own window and masked a stale box rather than repairing it (#7280 taxonomy (c)). New: a terminal-condition test over `temp_root.rs`'s own source, with its own sabotage arm. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…cuous (#7615) Four lowering tests over emitted IR, plus the terminal-condition test's ledger entry and the campaign's close-out in docs/engine-plan.md. ★ Both vacuities were MEASURED by the sabotage arm (restore the pre-fix `Expr::ArrayMap` lowering, require red), not reasoned about: 1. Slice 7's `assert_operand_survives_the_window` compares the operand register's OWN definition line against the window. For `ArrayMap` that register is `and i64 %stale, POINTER_MASK` — emitted BELOW the window while masking a value loaded above it. A one-level check cannot see "the unbox sits below its own window", which is the bug. These tests chase the definition chain through pure bit-twiddling to the first real producer. 2. An array-typed LOCAL receiver has no window at all: codegen's `ptr addrspace(1)` retype pass rematerialises the load from the local's own root slot at the use site, so the pre-fix code re-read the receiver by accident. The windows that are real — verified by A/B on emitted IR against a `main` baseline — are the receivers with no slot to rematerialise from: a module global, a class-field read and a closure capture. The tests use the field read. The window is anchored on the LATER OPERAND'S producer rather than on "the last object allocation above the call", because which helper an `Expr::Object` lowering reaches for is not this module's property. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
#7667 added new_target_save using crate::expr::temp_root while this slice moved the module to crate::rooting::temp_root. The two PRs were developed in parallel; the break only appears once both are on the same tree. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Audit — merging as v0.5.1384. The Layer-1 campaign is finished.The terminal condition, verified two waysI planted a raw call in a migrated module — once at top level ( Your correction to the plan's own wording is right and I'd have got it wrong. Fourteen entry points deleted rather than narrowed, per the kill-policy. A narrowed-but-live API is a decision nobody has made. An integration break I caught building itThe rebase surfaced a genuine one: Also resolved two rebase conflicts: Three corrections to my brief, all mine
The leads, and the discipline in how you closed them
★ The trap worth carrying
It presented as a 1-in-10 lowering-test failure with byte-identical IR between green and red runs — indistinguishable from #7665's global sinks, which I had just spent hours on. Gates: 22/22 lint, fmt clean, What you explicitly do not claim — that a listed module cannot make an ordering mistake, while a window with no decision at all stays invisible to the ledger — is the right caveat to leave standing, with #7640 and Layer 3 named as the open half. |
d21c41f to
293e49e
Compare
1 participant
Slice 8 — the last slice of the Layer 1 emitter-migration campaign (#7615).
It reaches the terminal condition the plan stated, and the interesting parts of
the work are the three places the brief was wrong and the two places my own
verification was vacuous.
The terminal condition, and why the file had to move
As literally spelled that is not expressible in Rust.
pub(in path)requires
pathto be an ANCESTOR module of the item (E0742), andcrate::rootingis not an ancestor ofcrate::expr::temp_root. So the filemoved: the raw API is now
crate::rooting::temp_root, declared with aprivate
mod temp_root;and withpub(in crate::rooting)on everyaccessor. Either alone would do it; both are here because the module
declaration is one keyword from re-widening twenty-five items at once.
A raw call planted in a migrated module now fails to compile:
That is the difference between this and a ledger line, and it is the second of
ten sabotage arms below.
Two items keep
pub(crate)and are re-exported fromrooting/mod.rs. Neitheris an accessor and neither can be called in the wrong order:
TempRootPool(compile-time slot bookkeeping) and
expr_is_inert_primitive(the puritypredicate
crate::loop_purityshares).Fourteen entry points were deleted rather than narrowed, because the
migration left them with no caller —
lower_exprs_rooted,lower_operand_pair_rooted,any_later_ref_may_trigger_gc,RootedOperands::is_rooted, the wholeStoreOperandGuardfamily, the wholeRootedHandlefamily,temp_root_scope_begin/_end. CLAUDE.md's kill-policy:the losing mode should stop compiling.
Three corrections to the brief
1.
lower_string_method.rshas 27 escape-hatch sites, not 3 — and it neededa split too. The brief's per-file counts come from a predicate that misses
use crate::expr::temp_root::{…}imports. Under the ledger's own predicate(non-comment lines naming
temp_root/rooted_handle) the real distribution islower_string_method.rs27,expr/static_field_meta.rs10,lower_call/new.rs9,
expr/math_simple.rs9,expr/binary.rs6,expr/dyn_extern_i18n.rs4.loop_purity.rshas zero (its one hit is a doc link), and two files thebrief omits —
gc_call_effects.rsandruntime_decls/arrays.rs— have fiveeach, all of them the runtime SYMBOL NAME
"js_gc_temp_root_push".2. So
new.rswas not the one blocker.lower_string_method.rswas 1,957lines against the 2,000-line cap with 27 sites to migrate — 43 lines of
headroom for five closure scopes. It got its own split, same as
new.rs.3. The
dyn_extern_i18n.rslead is wrong about the CFG — see below.Modules migrated vs classified decision-free
Telling them apart is half the work, because a ledger line on a module that
never had a decision to make looks substantive and asserts nothing.
Eight listed (seven load-bearing on the committed source):
expr/binary.rslower_operand_pair_rooted+temp_root_releasepairs, each releasing on its ownreturnlower_rooted_dynamic_binaryoverwith_operands_rootedexpr/math_simple.rsMapSet's two operands with unequal windows, re-read at eight arm-specific pointsRootedGroup;MapGet/MapHasare the plain shapeexpr/static_field_meta.rsClassExprFresh'srooted_handle_*+ a per-static store guardRootedGroupover the class object, nestedwith_rooted_accumulatorforcaps_arr, nestedwith_operands_rootedper symbol staticexpr/dyn_extern_i18n.rswith_rooted_accumulatorlower_string_method.rsopen_rooted_grouplower_string_concat.rswith_operands_rooted/with_rooted_grouplower_call/new.rsrefresh_rooted_argsat three points under a scope marker over ~20 return pathsRootedGroup; the null marker slot is gone with itlower_call/new_alloc.rsNine classified decision-free and deliberately NOT listed:
expr/mod.rs(amodule declaration and a field type — both gone with the move); the four
FnCtxconstructors (TempRootPool::default()— constructing the pool is notusing it);
stmt/loops.rs(one call to a purity predicate);loop_purity.rs(a doc link); and
root_reload.rs/gc_call_effects.rs/runtime_decls/arrays.rsplus five test files, whosejs_gc_temp_root_*occurrences are runtime symbol names, not this module.
(
linker_temp_lifecycle_tests.rs'stemp_root_if_clang_availableis about atemporary directory.)
The three leads
static_field_meta.rs'scaps_arr— VERIFIED as a real accumulator shapewith a provably empty window. The
__perry_ctor_capsarray held the onlyreference to everything pushed so far across the next element's lowering, in
a bare SSA register — gc: console.log argument temporaries are not precise roots — a precise-roots-only collection drops string-literal args (minimal repro, no evacuation needed) #6951 exactly. But
captured_argsis built at ONE site(
lower/lower_expr/arm_class.rs:153) asids.iter().map(|id| Expr::LocalGet(*id)), andexpr_may_trigger_gcanswersfalsefor everyLocalGet. It is now awith_rooted_accumulatorwhoseprotectis computed rather than assumed: todayfalse, IR byte for byteunchanged, and automatically correct the day a non-inert expression reaches
the list.
math_simple.rs'sArrayMap— VERIFIED live. Below.dyn_extern_i18n.rs'spath_handle— DISMISSED; the premise is wrong aboutthe CFG. Each
<prefix>__init()is emitted into that iteration's matchblock, which branches straight to the join — no
__initdominates any lateruse of
path_handle. Along the fallthrough chain the only emissions betweenthe handle's production and its last use are
js_get_string_pointer_unifiedand
js_string_equals, neither of which re-enters user code or enumerates anobject — the standard
with_operands_rooted_across_call's doc sets for anemitted step (fix(codegen): close #7192's own residual root-store hole, and make the dominance checker able to fail #7198). What that module DID have is the namespace-object
accumulator, which is migrated.
The live bug:
Expr::ArrayMaparr.map(cb)lowered the receiver, lowered the callback, and only THEN unboxedthe receiver — the unbox sat below its own window and masked a stale box
rather than repairing it (#7280 taxonomy (c)).
IR evidence,
mainbaseline vs this branch, both--profile perry-dev,PERRY_NO_AUTO_OPTIMIZE=1,PERRY_GC_MOVING_LOOP_POLLS=1, separate targetdirs. Module-global receiver, baseline:
this branch:
The same shape appears for a class-field read and for a closure capture
(
js_closure_get_capture_bits, a rawi64— taxonomy (a), whichroot_reloadstructurally cannot repair). Frame slot count is unchanged: thepooled slot is reused by the watermark for the next temp.
Where the window is NOT real, and why it matters: for an array-typed
local receiver, codegen's
ptr addrspace(1)retype pass rematerialises theload from the local's own root slot at the use site, so the pre-fix code
re-read the receiver by accident. Measured, not assumed — the baseline arm
emits
%r65.rs4p = load ptr addrspace(1), ptr %r1below the closure alloc allby itself.
Two ways my own verification was vacuous
Both were found by the sabotage arm (restore the pre-fix lowering, require red),
not by reasoning:
assert_operand_survives_the_windowcannot see this bug. Itcompares the operand register's OWN definition line against the window, and
for
ArrayMapthat register isand i64 %stale, POINTER_MASK— emittedbelow the window while masking a value loaded above it. "The unbox sits
below its own window" is exactly the shape a one-level check misses. These
tests chase the definition chain through pure bit-twiddling to the first real
producer. (Slice 7's own tests are not wrong — their subjects are raw
pointers stripped above the window — but the helper is weaker than its
name.)
The tests use a field read.
★ A trap worth recording: a sabotage harness that restores a
.bakcp f f.bak; <patch f>; cargo test; mv f.bak fleavesfwith an oldermtime than the sabotaged build, so cargo keeps the sabotaged binary and every
later run measures the sabotage. It presented as an intermittent lowering-test
failure — 1 run in 10, then 20 in 20 — with byte-identical IR between the
green and red runs, which reads exactly like the process-global sinks #7665
fixed and is not.
touchafter the restore. Diagnosing from the wrong VALUE iswhat settled it: "the producer is one line above the window" is precisely the
pre-fix lowering, not a race.
Verification
mainbaseline, this branch} × {
PERRY_GC_MOVING_LOOP_POLLS0, 1} — over a probecovering
ArrayMap(including evaluation order), the five dynamic binaryarms, string append/concat/chain/methods, Map set/get/has (typed and
generic), class expressions with statics + captures +
static {}blocks,newwith allocating and literal arguments, subclassing, and a 4,000-objectchurn between them. All four MATCH; baselines built in separate target
dirs,
PERRY_NO_AUTO_OPTIMIZE=1throughout..github/workflows/test.yml)plus
rustup run stable cargo fmt --all -- --checkand./scripts/check_file_size.sh(largest file nowrooting/mod.rsat 1,880).cargo test -p perry-codegen --lib --no-fail-fast747 passed, and 20consecutive full-suite runs green after the mtime fix.
cargo test -p perry-runtime --lib --no-fail-fast1,917 passed.cargo check --all-targetsclean.native_root_coverage.--moving-only --seeded-violations 40: 129/129 sources, 149 modules, 40planted / 40 caught / 0 missed, 0 violations.
--unrooted-allocas --moving-only: 7,695 gc-capable allocas, 0.--statepoints --moving-only --max-unrooted 21 --max-stale 0: 2,458functions, 30,080 safepoints, 17,516 live bundles, 40,819 relocates, 40
planted / 40 caught, hazards 21 unrooted / 0 stale — unchanged, and
deliberately not touched while gc: 21 unrooted-across-safepoint hazards in the NATIVE root lowering (the --max-unrooted budget's referent) #7664's budget is being lowered separately.
each newly listed module, require the ledger red AND naming the
file:line)and two visibility (plant a real raw call, require
E0603).What this does NOT claim
The ledger's own caveat, drawn the hard way in slice 4: a listed module cannot
make an ORDERING mistake against the raw API, because it no longer names it. A
window with no rooting decision at all is invisible to that check, and the
only instrument for it is reading the module. 36 modules are listed;
docs/engine-plan.mditem 7 records the remaining audit as the open half, with#7640 holding the deferred sites, and Layer 3 as the rest of the rooting work.
Summary by CodeRabbit
Bug Fixes
Performance
Tests
Documentation