refactor(codegen): Layer 1 rooting migration slice 5 — the timer and namespace-call lowerings (#7615) - #7648
Conversation
…e Layer 1 rooting API (#7615) Slice 5. Also fixes the unrooted 2-arg setTimeout/setInterval callback window and five unprotected windows in namespace_call.rs.
…rms (#7615) Asserts an ordering rather than a slot count, so it cannot pass vacuously: verified red on the pre-fix source for the two hazard arms and green for the two zero-cost arms.
📝 WalkthroughWalkthroughTimer and namespace-call lowering now use ChangesLower-call rooting migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 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
🧹 Nitpick comments (1)
crates/perry-codegen/src/lower_call/namespace_call.rs (1)
62-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
undefined_litduplicates the same helper in the parent module.
crates/perry-codegen/src/lower_call/mod.rslines 63-65 defines an identicalundefined_lit. Promote the parent one topub(super)/pub(crate)and import it here instead of redefining it.🤖 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/namespace_call.rs` around lines 62 - 65, Remove the duplicate undefined_lit helper from the namespace-call module, change the parent module’s undefined_lit visibility to pub(super) or pub(crate), and import and reuse that helper where needed in namespace_call.rs.
🤖 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/timer_rooting_tests.rs`:
- Around line 137-144: Update last_line_containing to exclude IR lines beginning
with or containing declare, matching the filtering behavior in
validate_call_line and the temp-root counter, so it returns only the relevant
call-site line for delay_alloc ordering assertions.
---
Nitpick comments:
In `@crates/perry-codegen/src/lower_call/namespace_call.rs`:
- Around line 62-65: Remove the duplicate undefined_lit helper from the
namespace-call module, change the parent module’s undefined_lit visibility to
pub(super) or pub(crate), and import and reuse that helper where needed in
namespace_call.rs.
🪄 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: b052a02e-6346-4a94-b2f6-f325cd9a9990
📒 Files selected for processing (6)
changelog.d/7648-layer1-slice5-lower-call.mdcrates/perry-codegen/src/lower_call/extern_timers.rscrates/perry-codegen/src/lower_call/mod.rscrates/perry-codegen/src/lower_call/namespace_call.rscrates/perry-codegen/src/lower_call/timer_rooting_tests.rscrates/perry-codegen/src/rooting.rs
| fn last_line_containing(ir: &str, needle: &str) -> usize { | ||
| ir.lines() | ||
| .enumerate() | ||
| .filter(|(_, l)| l.contains(needle)) | ||
| .map(|(i, _)| i) | ||
| .last() | ||
| .unwrap_or_else(|| panic!("no line containing {needle} in:\n{ir}")) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Exclude declare lines in last_line_containing, as the other two helpers do.
validate_call_line and the temp-root counter both drop declare lines. last_line_containing does not. The module carries declare i64 @js_object_alloc(...). If the emitter ever places declarations after function definitions, delay_alloc becomes the declaration index and the ordering assertion in assert_callback_survives_an_allocating_delay fails for a reason unrelated to rooting.
🛡️ Proposed fix to restrict the search to call sites
-/// Line index of the LAST occurrence of `needle`.
+/// Line index of the LAST occurrence of `needle`, ignoring `declare` lines —
+/// the module names the helper whether or not anything calls it.
fn last_line_containing(ir: &str, needle: &str) -> usize {
ir.lines()
.enumerate()
+ .filter(|(_, l)| !l.trim_start().starts_with("declare"))
.filter(|(_, l)| l.contains(needle))
.map(|(i, _)| i)
.last()
.unwrap_or_else(|| panic!("no line containing {needle} in:\n{ir}"))
}📝 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.
| fn last_line_containing(ir: &str, needle: &str) -> usize { | |
| ir.lines() | |
| .enumerate() | |
| .filter(|(_, l)| l.contains(needle)) | |
| .map(|(i, _)| i) | |
| .last() | |
| .unwrap_or_else(|| panic!("no line containing {needle} in:\n{ir}")) | |
| } | |
| fn last_line_containing(ir: &str, needle: &str) -> usize { | |
| ir.lines() | |
| .enumerate() | |
| .filter(|(_, l)| !l.trim_start().starts_with("declare")) | |
| .filter(|(_, l)| l.contains(needle)) | |
| .map(|(i, _)| i) | |
| .last() | |
| .unwrap_or_else(|| panic!("no line containing {needle} in:\n{ir}")) | |
| } |
🤖 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/timer_rooting_tests.rs` around lines 137
- 144, Update last_line_containing to exclude IR lines beginning with or
containing declare, matching the filtering behavior in validate_call_line and
the temp-root counter, so it returns only the relevant call-site line for
delay_alloc ordering assertions.
Also records, in the slice 5 fragment, what review could and could not reproduce: bug 3 reproduces on the plain default build with no GC knobs (independently, with a different repro shape, printing nothing at exit 0), while bugs 1 and 2 are arrangement-dependent at runtime and rest on the IR window, which review confirmed directly in --trace llvm output. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Cargo.toml`:
- Line 318: Synchronize the package metadata by updating Cargo.lock’s perry
package version to match the workspace version declared in Cargo.toml as
0.5.1368, preferably by regenerating the lockfile and preserving the manifest
bump.
In `@CLAUDE.md`:
- Line 11: Restore the release metadata to version 0.5.1367: update the Current
Version entry in CLAUDE.md at line 11 and the [workspace.package].version value
in Cargo.toml at line 318.
🪄 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: 6a6b2608-fea9-456d-a833-71a5c3acf4ac
📒 Files selected for processing (3)
CLAUDE.mdCargo.tomlchangelog.d/7648-layer1-slice5-lower-call.md
🚧 Files skipped from review as they are similar to previous changes (1)
- changelog.d/7648-layer1-slice5-lower-call.md
|
|
||
| [workspace.package] | ||
| version = "0.5.1367" | ||
| version = "0.5.1368" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Synchronize Cargo.lock with the workspace version.
Cargo.lock still records perry as 0.5.1367, while this manifest declares 0.5.1368. If the version bump is retained, regenerate Cargo.lock; otherwise, restore the manifest version. The mismatch can break locked builds and package metadata consistency.
🤖 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 `@Cargo.toml` at line 318, Synchronize the package metadata by updating
Cargo.lock’s perry package version to match the workspace version declared in
Cargo.toml as 0.5.1368, preferably by regenerating the lockfile and preserving
the manifest bump.
| Perry is a native TypeScript compiler written in Rust that compiles TypeScript source code directly to native executables. It uses SWC for TypeScript parsing and LLVM for code generation. | ||
|
|
||
| **Current Version:** 0.5.1367 | ||
| **Current Version:** 0.5.1368 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep release metadata in the maintainer-owned release flow.
External contributor PRs must not update the repository version fields.
CLAUDE.md#L11-L11: restoreCurrent Versionto0.5.1367.Cargo.toml#L318-L318: restore[workspace.package].versionto0.5.1367.
📍 Affects 2 files
CLAUDE.md#L11-L11(this comment)Cargo.toml#L318-L318
🤖 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 `@CLAUDE.md` at line 11, Restore the release metadata to version 0.5.1367:
update the Current Version entry in CLAUDE.md at line 11 and the
[workspace.package].version value in Cargo.toml at line 318.
Sources: Coding guidelines, Learnings
Audit — merging as v0.5.1368Bug 3 confirmed independently, and it is worse than the fragment saidI built a baseline from Bugs 1 and 2: I could not reproduce the runtime fault — and you were right anywayYour files, your commands, my %r7 = call i64 @js_closure_alloc_with_captures_singleton(...) ; the callback
%r9 = bitcast i64 %r8 to double ; BARE register
%r10 = call double @perry_fn_timer2b_ts__churn() ; user code, collects
%r11 = call i64 @js_timer_validate_callback(double %r9, i32 0) ; reads the register defined above the calland the fixed arm stores Your correction #3 is the one I'd want future slices to inherit: a window that does not fault is not a window that is absent — it is one whose victim happened to survive. I've written that into the fragment next to the claim, along with the fact that review could not reproduce it. A changelog that says "the baseline throws X" where a reader following the instructions gets exit 0 costs more trust than the finding is worth; a changelog that says "here is the window, here is why the fault is arrangement-dependent" is checkable forever. Your two corrections to my brief — both verified, both mine to own
And I did briefly suspect the Scope2 of 6 modules is the right answer given what you found, and the reason is the valuable part: three of the four you left need a re-read at more than one point, and every #7649 captures the missing variadic/rest combinator. Gates: 20/20 from the |
The version bumps in #7646/#7648 edited Cargo.toml and staged Cargo.lock without a cargo invocation in between, so the lock kept 0.5.1367 and every build dirtied the tree. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
1 participant
Slice 5 of the Layer 1 rooting migration (#7615). Two modules of the
lower_call/family migrated end to end, four live bugs found and fixed, and the reason the other four modules were left out recorded in the ledger because it is a statement about the API rather than about those files.Migrated
lower_call/extern_timers.rslower_exprs_rooted/temp_root_release)lower_call/namespace_call.rsSabotage arm run per module — the assert stops at the first offender, so one run cannot speak for two. A compiling
temp_root_push_double/temp_root_truncatepair was planted in each; the ledger test went red and named both lines byfile:line; the build output was checked forerror[andcould not compile, both 0, andRunning unittestsconfirmed the test binary was actually reached. (A plant that fails to compile also makes the command non-zero, and a check grepping only forFAILEDscores that build error as a successful sabotage.)Live bugs
All four demonstrated against node 26.5.1 (the
.node-versionoracle) with a baseline compiler built frommainin a separate target directory, and re-verified underPERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800, compiled withPERRY_GC_MOVING_LOOP_POLLS=1.1. Two-argument
setTimeout/setIntervalheld the callback unrooted across the delay. #7210 rooted the trailing-argument forms and left the two-argument siblings alone, though the comment it wrote names the window exactly. The delay is an arbitrary expression:setIntervalthrows the same. A capturing callback is required — a non-capturing arrow lowers tojs_closure_alloc_singleton, which is never moved, so the first reproducer I wrote passed while the IR clearly showed the window.2. The var-shaped namespace export dispatched through a stale closure.
ns.arrow(churn())fetches the closure from its zero-arg getter first (spec order — the callee reference before the arguments), holds it in a bare register across every argument's lowering, thenunbox_to_i64s it. #7280 taxonomy (a) and (c) at once:root_reloadcould not have repaired it, because the pointer is derived below the window from a register captured above it.3. The
has_restnamespace direct call lost every rest element — silently. The #7154 accumulator shape verbatim:currentwas a raw*mut ArrayHeaderthreaded through a push loop while the next argument's expression ran, holding the only reference to everything pushed so far.lower_rest_call_args_rootedwas written for exactly this and this path never adopted it.A wrong answer, not a crash. Delegating to the audited helper also pads fixed parameters to the declared arity, which the hand-rolled loop did not.
4. Three more unprotected windows, repaired in passing, real by inspection but without a reproducer that faults today: the
fs/promiseswriteFile/appendFile/rmdirarms (pathheld acrosscontentandoptions), both V8-bridge arms (a barefor a in args { lower_expr }— #7240's shape in a path that post-dates the fix), and the plain namespace direct-call argument loop.False positives, counted in emissions
The one-argument
setTimeout/setImmediateand all threeclear*arms lower a single operand and consume it in the very next emission. No emission between production and use means no window, so they stay on barelower_expr; routing them throughwith_operands_rootedwould emit nothing anyway (any_may_trigger_gcover an empty tail isfalse) and would advertise a protection that is not there.Cost
Zero where the window cannot collect, and now pinned rather than asserted. With a literal delay the emitted module contains no temp-root traffic at all and the callback register feeds
js_timer_validate_callbackdirectly. Across the whole 149-modulegc_root_dominance_corpus.shcorpus, baseline and fixed IR are byte-identical (diff -rq→ 0 differing files).Tests
lower_call/timer_rooting_tests.rs, asserting on emitted IR rather than runtime behaviour. The runtime fault needs a capturing callback, a polling delay and the compile-timePERRY_GC_MOVING_LOOP_POLLS=1(off by default since #7161) — a gap test would be green on the default build whether or not the fix is present, which is hazard 4.The assertion is an ordering, not a slot count: a count across two programs that differ in an operand lets the delay's own rooting pay for the assertion. With an allocating delay the register
js_timer_validate_callbackreads must be defined below the delay's allocation; with a literal delay it must be the original register with zero temp-root traffic. Checked against the pre-fix source: the two ordering tests fail, the two zero-cost tests pass. Neither is vacuous. The liveness check excludesdeclarelines — the module always carriesdeclare @js_timer_validate_callbackwhether or not anything calls it.Not migrated, and why
Four of the six modules named on the tracker are left, and the reason is one sentence about this API: three of them need a re-read at more than one point, and every
with_operands_rooted*form has exactly one.lower_call/mod.rs—lower_call_args_rooted/lower_rest_call_args_rootedreturn a guard deliberately: their consumers infunc_ref.rsare block-splitting specialized-ABI diamonds whose release must sit in a merge block post-dominating four dispatch paths, ~200 lines below the lowering. A closure form can express that only by swallowing the whole dispatch chain, in a file outside the slice.lower_call/new.rs—refresh_rooted_argsre-reads the same operand group at three caller-chosen points, under atemp_root_scope_begin/_endmarker spanning ~20 return paths.lower_call/console_promise.rs—lower_dynamic_closure_callre-reads receiver and callee below the arguments, then re-reads the arguments again below the allocating rebind unbox. Two stages, one combinator.lower_call/early_branches.rs— its only escape-hatch uses areimplicit_this_save/implicit_this_restore, already a paired combinator rather than the raw ordering API. Migrating it means re-exporting that pair throughcrate::rooting: a rename that would make the ledger line look substantive while asserting nothing new.The concrete missing combinator is the variadic/rest shape — per-element re-reads between allocating pushes.
namespace_call.rs's rest arm therefore delegates tosuper::lower_rest_call_args_rooted, which still names the raw API; that boundary is stated in the file header rather than hidden, and it is the same posture as callinglower_expr.Two hazards in
console_promise.rswere found while reading it and are not fixed here:console.table(a, b)andconsole.dir(a, b)both hold operand 0 in a bare register across operand 1's lowering. They are one-line fixes with the same combinator, but they belong with that module's migration rather than ahead of it.Gates
The lint list was extracted from
.github/workflows/test.yml, not recalled — 20 commands, all run individually:Plus:
rustup run stable cargo fmt --all -- --check— cleancargo test -p perry-codegen --lib --no-fail-fast— 706 passed, 0 failedcargo test -p perry-runtime --lib --no-fail-fast— 1908 passed, 0 failed, 3 ignoredcargo check --all-targets— cleangc_root_dominance_check.py --self-test— OKgc_root_dominance_corpus.sh— 129/129 sources, 149.ll--moving-only --seeded-violations 40) — 40 planted, 40 caught, 0 MISSED, rc=0--unrooted-allocas --moving-only— 0 violations, rc=0Run without
--moving-onlyboth arms report the same 171 non-moving leads, all one fingerprint (js_object_alloc_class_inline_keys→js_gc_declare_typed_shape_layout). A/B'd against a baseline corpus: 171 on both, so they are pre-existing and untouched by this change; the gate uses--moving-onlyand sees 0 either way.No version bump, no
CHANGELOG.mdedit; changelog fragment added underchangelog.d/.Summary by CodeRabbit
Bug Fixes
Tests