refactor(codegen): Layer 1 slice 1 — migrate expr/url_main.rs onto the rooting API, and the campaign map (#7615) - #7617
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 expands construction-based rooting APIs, migrates URL and URLSearchParams code generation to these APIs, adds migration-ledger checks, and updates migration documentation and version records. ChangesRooting-by-construction migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant URLCodegen
participant RootingAPI
participant RootedSlot
participant Runtime
URLCodegen->>RootingAPI: lower URL operands
RootingAPI->>RootedSlot: retain operand roots
RootingAPI->>RootedSlot: reload operands
RootingAPI->>Runtime: invoke URL operation
Runtime-->>RootingAPI: return result
RootingAPI->>RootedSlot: release roots
Possibly related issues
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: 4
🧹 Nitpick comments (3)
crates/perry-codegen/src/rooting.rs (2)
486-495: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe escape-hatch scanner matches substrings anywhere outside a
//comment.Two consequences:
- False positives. A string literal such as
"temp_root", or an identifier such asno_temp_root_needed, fails the build. The failure direction is safe, but the message names a violation that does not exist.- Block comments are not stripped.
code.containssees the body of a/* ... */comment.A word-boundary check on the path segment, for example matching
temp_root::androoted_handle_, narrows both cases without adding a dependency.🤖 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 486 - 495, Update escape_hatch_uses so it only reports actual escape-hatch path references, not arbitrary substrings in identifiers or string literals, by matching the requested path-segment forms such as temp_root:: and rooted_handle_. Extend the scanner’s comment handling to ignore block-comment contents as well as // comments, while preserving the existing line-number and trimmed-line reporting.
340-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a debug-time check for release order and forgotten releases.
releaseconsumesself, so the same slot cannot be released twice. Two failure modes remain writable:
- Releasing an outer slot before an inner one silently drops the inner slot, because
temp_root_truncateis a stack cut.- Dropping a
RootedSlotwithout callingreleaseleaves the group pushed for the rest of the lowering.A
Dropimpl cannot help here, becausereleaseneedsctx. A cheap alternative is a#[cfg(debug_assertions)]flag onRootedSlotplus an assertion inreleasethat the slot is the topmost live one, and a debug panic inDropwhen the flag is still set. This keeps the ledger's guarantee from depending on reviewer discipline.🤖 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 340 - 349, The RootedSlot lifecycle currently permits out-of-order releases and silently forgotten releases. Add a debug-only released-state flag to RootedSlot, assert in RootedSlot::release that the slot is the current topmost live slot before truncating, and mark it released afterward; implement Drop for RootedSlot to panic in debug builds when the flag indicates release was omitted, while preserving release behavior in non-debug builds.crates/perry-codegen/src/expr/url_main.rs (1)
407-429: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe rooted region is correct. The comment block above it is now stale.
The lowering is right: both arms run inside one rooted region,
vals[2]is reachable only whenvalue.is_some(), andrawis a plainDOUBLEthat does not need protection after the release.The preceding comment still states "The guard lives to the end of the arm". After this change the caller holds no guard at all;
with_operands_rootedowns it. The same superseded sentence remains above theUrlSearchParamsSet,UrlSearchParamsAppend,UrlSearchParamsDelete, andUrlSearchParamsGetAllarms. Removing it keeps the reference module consistent with the rule it teaches.🤖 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/url_main.rs` around lines 407 - 429, Remove the stale “guard lives to the end of the arm” comment from the UrlSearchParams lowering arms, including UrlSearchParamsHas and the UrlSearchParamsSet, UrlSearchParamsAppend, UrlSearchParamsDelete, and UrlSearchParamsGetAll arms. Keep the existing with_operands_rooted implementation and update only the superseded comment text.
🤖 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/url_main.rs`:
- Around line 194-201: In the URL setter assignment callback using
with_operands_rooted, do not return the pre-call val_v after runtime_fn may
allocate or invoke user code; reload the RHS from its rooted slot after
call_void using a re-rooted value, or extend RHS rooting through the return.
Preserve assignment-expression semantics by returning the post-call RHS value.
In `@crates/perry-codegen/src/rooting.rs`:
- Around line 530-547: Update the planted source in the test
the_ledger_check_still_reports_a_planted_violation so the rooted_handle_begin
call uses a path that does not contain the temp_root substring. Keep the
expected two hits and assertions, ensuring one hit exercises the temp_root
predicate and the other independently exercises the rooted_handle predicate.
In `@docs/engine-plan.md`:
- Line 88: Clarify the “raw-pointer-across-lowering bug shape eliminated
crate-wide” wording in the migration-status entry, explicitly distinguishing
audited/verified code from code prevented by enforcement. Align the claim with
the stated 262 remaining hazard sites and avoid implying a crate-wide guarantee
that the campaign ledger does not support.
In `@docs/src/internals/rfc-rooting-by-construction.md`:
- Around line 231-234: The migration-ledger documentation overstates its
guarantees. In docs/src/internals/rfc-rooting-by-construction.md lines 231-234,
revise the statement to identify rooting::migration_ledger, its cargo test
execution, and that it checks only registered MIGRATED_MODULES; in
docs/engine-plan.md lines 425-431, qualify the expr::temp_root denial as
applying to modules added to that test’s registry, without implying all
unfinished modules are checked.
---
Nitpick comments:
In `@crates/perry-codegen/src/expr/url_main.rs`:
- Around line 407-429: Remove the stale “guard lives to the end of the arm”
comment from the UrlSearchParams lowering arms, including UrlSearchParamsHas and
the UrlSearchParamsSet, UrlSearchParamsAppend, UrlSearchParamsDelete, and
UrlSearchParamsGetAll arms. Keep the existing with_operands_rooted
implementation and update only the superseded comment text.
In `@crates/perry-codegen/src/rooting.rs`:
- Around line 486-495: Update escape_hatch_uses so it only reports actual
escape-hatch path references, not arbitrary substrings in identifiers or string
literals, by matching the requested path-segment forms such as temp_root:: and
rooted_handle_. Extend the scanner’s comment handling to ignore block-comment
contents as well as // comments, while preserving the existing line-number and
trimmed-line reporting.
- Around line 340-349: The RootedSlot lifecycle currently permits out-of-order
releases and silently forgotten releases. Add a debug-only released-state flag
to RootedSlot, assert in RootedSlot::release that the slot is the current
topmost live slot before truncating, and mark it released afterward; implement
Drop for RootedSlot to panic in debug builds when the flag indicates release was
omitted, while preserving release behavior in non-debug builds.
🪄 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: a4025d57-e670-412d-97a7-ccc264e26833
📒 Files selected for processing (4)
crates/perry-codegen/src/expr/url_main.rscrates/perry-codegen/src/rooting.rsdocs/engine-plan.mddocs/src/internals/rfc-rooting-by-construction.md
| rooting::with_operands_rooted(ctx, &[url, value], |ctx, vals| { | ||
| let (url_v, val_v) = (vals[0].clone(), vals[1].clone()); | ||
| let url_handle = unbox_to_i64(ctx.block(), &url_v); | ||
| ctx.block() | ||
| .call_void(runtime_fn, &[(I64, &url_handle), (DOUBLE, &val_v)]); | ||
| // Assignment expression evaluates to the value on the RHS. | ||
| Ok(val_v) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the URL setter runtime helpers for allocation and user-code re-entry.
fd -t f 'url.rs' crates/perry-runtime/src
rg -n -C12 'pub extern "C" fn js_url_set_(href|pathname|search|hash|protocol|hostname|port|username|password)' crates/perry-runtime/src
# Does the setter path coerce its argument via user-visible toString?
rg -n -C6 'fn js_url_set_' crates/perry-runtime/src | rg -n 'coerce|to_string|ToString|call_user|invoke'Repository: PerryTS/perry
Length of output: 15907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "# Locate stringify/to_string/coerce implementations"
rg -n --glob '*.rs' 'coerce_url_setter_value|js_to_string|to_string|js_coerce|stringifier|Stringer|gc_check_trigger|push_temp|pop_temp|unbox_to_i64' crates/perry-runtime/src crates/perry-codegen/src/expr/url_main.rs | sed -n '1,220p'
echo
echo "# Inspect coerce_url_setter_value and call sites in url_class.rs"
rg -n -A45 'fn coerce_url_setter_value' crates/perry-runtime/src/url/url_class.rs
rg -n -A60 -B10 'fn coerce_url_setter_value' crates/perry-runtime/src/url/url_class.rs crates/perry-runtime/src -g '*.rs'
echo
echo "# Inspect url_main assignment lowering around lines 177-201"
sed -n '130,220p' crates/perry-codegen/src/expr/url_main.rsRepository: PerryTS/perry
Length of output: 40938
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "# js_url_coerce_string definition and all call sites"
rg -n -C12 'pub extern "C" fn js_url_coerce_string|fn js_url_coerce_string|js_url_coerce_string\(' crates/perry-runtime/src crates/perry-codegen/src -g '*.rs'
echo
echo "# Related coercion entrypoints"
rg -n -C8 'js_jsvalue_to_string|js_value_to_string_with_encoding|coerce_to_string|reject_symbol_to_string' crates/perry-runtime/src/builtins crates/perry-runtime/src/value crates/perry-runtime/src/object crates/perry-codegen/src -g '*.rs' | sed -n '1,260p'
echo
echo "# Relevant url_main lowering definitions imports/aux functions"
sed -n '1,120p' crates/perry-codegen/src/expr/url_main.rsRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C5 'gc_check_trigger|evacuate|gc_collect|move|relocate|return_stale|forward|^fn root|pub fn root_nanbox_f64|root_raw_mut_ptr' crates/perry-runtime/src/gc crates/perry-codegen/src/expr/url_main.rsRepository: PerryTS/perry
Length of output: 50370
Reload the RHS value after the URL setter call.
val_v comes from the temp-root slot before js_url_…_set_… runs, and each setter coerces the DOUBLE argument through js_url_coerce_string/js_jsvalue_to_string, which can run user toString code. The root group is released before Ok(val_v) returns, so object-valued RHS operands use a now-unrooted register across potentially moving allocation. Return a reloaded value from a re-rooted slot, or keep rooting the RHS until after the call.
🤖 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/url_main.rs` around lines 194 - 201, In the URL
setter assignment callback using with_operands_rooted, do not return the
pre-call val_v after runtime_fn may allocate or invoke user code; reload the RHS
from its rooted slot after call_void using a re-rooted value, or extend RHS
rooting through the return. Preserve assignment-expression semantics by
returning the post-call RHS value.
Source: Learnings
| #[test] | ||
| fn the_ledger_check_still_reports_a_planted_violation() { | ||
| let planted = "\ | ||
| fn lower(ctx: &mut FnCtx<'_>) { | ||
| let p = ctx.block().call(I64, \"js_url_coerce_string\", &[]); | ||
| let slot = super::temp_root::temp_root_push_i64(ctx, &p); | ||
| let h = super::temp_root::rooted_handle_begin(ctx, &p, true); | ||
| } | ||
| "; | ||
| let hits = escape_hatch_uses(planted); | ||
| assert_eq!( | ||
| hits.len(), | ||
| 2, | ||
| "planted escape-hatch uses must be reported, got {hits:?}" | ||
| ); | ||
| assert!(hits[0].1.contains("temp_root_push_i64")); | ||
| assert!(hits[1].1.contains("rooted_handle_begin")); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The planted-violation test does not exercise the rooted_handle predicate.
The second planted line is super::temp_root::rooted_handle_begin(ctx, &p, true);. It contains the substring temp_root as well, so the first half of the predicate already matches it. If someone deletes || code.contains("rooted_handle") from escape_hatch_uses, this test still passes with hits.len() == 2 and hits[1] still containing rooted_handle_begin.
Plant the second spelling without the temp_root path prefix, so each half of the predicate is required.
💚 Proposed fix to isolate the second predicate
let planted = "\
fn lower(ctx: &mut FnCtx<'_>) {
let p = ctx.block().call(I64, \"js_url_coerce_string\", &[]);
let slot = super::temp_root::temp_root_push_i64(ctx, &p);
- let h = super::temp_root::rooted_handle_begin(ctx, &p, true);
+ let h = rooted_handle_begin(ctx, &p, true);
}
";📝 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.
| #[test] | |
| fn the_ledger_check_still_reports_a_planted_violation() { | |
| let planted = "\ | |
| fn lower(ctx: &mut FnCtx<'_>) { | |
| let p = ctx.block().call(I64, \"js_url_coerce_string\", &[]); | |
| let slot = super::temp_root::temp_root_push_i64(ctx, &p); | |
| let h = super::temp_root::rooted_handle_begin(ctx, &p, true); | |
| } | |
| "; | |
| let hits = escape_hatch_uses(planted); | |
| assert_eq!( | |
| hits.len(), | |
| 2, | |
| "planted escape-hatch uses must be reported, got {hits:?}" | |
| ); | |
| assert!(hits[0].1.contains("temp_root_push_i64")); | |
| assert!(hits[1].1.contains("rooted_handle_begin")); | |
| } | |
| #[test] | |
| fn the_ledger_check_still_reports_a_planted_violation() { | |
| let planted = "\ | |
| fn lower(ctx: &mut FnCtx<'_>) { | |
| let p = ctx.block().call(I64, \"js_url_coerce_string\", &[]); | |
| let slot = super::temp_root::temp_root_push_i64(ctx, &p); | |
| let h = rooted_handle_begin(ctx, &p, true); | |
| } | |
| "; | |
| let hits = escape_hatch_uses(planted); | |
| assert_eq!( | |
| hits.len(), | |
| 2, | |
| "planted escape-hatch uses must be reported, got {hits:?}" | |
| ); | |
| assert!(hits[0].1.contains("temp_root_push_i64")); | |
| assert!(hits[1].1.contains("rooted_handle_begin")); | |
| } |
🤖 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 530 - 547, Update the
planted source in the test the_ledger_check_still_reports_a_planted_violation so
the rooted_handle_begin call uses a path that does not contain the temp_root
substring. Keep the expected two hits and assertions, ensuring one hit exercises
the temp_root predicate and the other independently exercises the rooted_handle
predicate.
| |---|---|---|---| | ||
| | **0** | *enabler* | in-process LLVM | ✅ shipped (#7301), default cargo feature (#7353) | | ||
| | **1** | `perry-codegen` lowering code | `Raw`/`Rooted` discipline | design **validated & corrected** (#7459 — the RFC's own constructor was `E0499`); combinator form proven on the real emitter (#7461); the raw-pointer-across-lowering bug shape **eliminated crate-wide** (#7453, #7462–#7465); full emitter migration **not started** | | ||
| | **1** | `perry-codegen` lowering code | `Raw`/`Rooted` discipline | design **validated & corrected** (#7459 — the RFC's own constructor was `E0499`); combinator form proven on the real emitter (#7461); the raw-pointer-across-lowering bug shape **eliminated crate-wide** (#7453, #7462–#7465). **Migration started**: campaign map + per-module ledger in **#7615**; `expr/url_main.rs` migrated end to end as the template slice (#7617), which found `URL.canParse`/`URL.parse` still carrying #7453's window. 1 of 88 modules; 262 hazard sites remain. **Measured limit, stated once: on the real emitter this does NOT make the bug fail to compile** — `FnCtx` has no interior mutability, so the borrow form is unbuildable on it; the combinator removes the bug from the path of least resistance and the ledger denies the escape hatch, and that is all | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching docs/engine-plan.md =="
git ls-files | rg '(^|/)docs/engine-plan\.md$|engine-plan\.md' || true
echo
echo "== line 88 context =="
if [ -f docs/engine-plan.md ]; then
nl -ba docs/engine-plan.md | sed -n '80,96p'
fi
echo
echo "== search relevant terms =="
rg -n "eliminated crate-wide|raw-pointer-across-lowering|hazard sites|URL.canParse|URL.parse|FnCtx|block\(\)\.call|`#7453`|`#7461`|`#7617`|`#7459`|`#7462`|`#7465`|262" -S .Repository: PerryTS/perry
Length of output: 317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== line 88 context =="
awk '{printf "%6d\t%s\n", NR, $0}' docs/engine-plan.md | sed -n '80,96p'
echo
echo "== relevant terms =="
grep -RInE "eliminated crate-wide|raw-pointer-across-lowering|hazard sites|URL\.canParse|URL\.parse|FnCtx|block\(\)\.call|`#7453`|`#7461`|`#7617`|`#7459`|`#7462`|`#7465`|262" .Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/engine-plan.md line 88 =="
awk '{printf "%6d\t%s\n", NR, $0}' docs/engine-plan.md | sed -n '83,93p'
echo
echo "== docs/engine-plan.md relevant terms (limited) =="
grep -RInE "eliminated crate-wide|hazard sites|raw-pointer-across-lowering|`#7453`" docs/engine-plan.md
echo
echo "== TYPE_LOWERING.md around Line 278 =="
awk '{printf "%6d\t%s\n", NR, $0}' TYPE_LOWERING.md | sed -n '268,288p'
echo
echo "== docs/engine-plan.md around line 278 =="
awk '{printf "%6d\t%s\n", NR, $0}' docs/engine-plan.md | sed -n '273,283p'
echo
echo "== changed/engine-note references =="
grep -RInE "raw-pointer-across-lowering|pre-#7453|`ctx\.block\(\)\.call`|262 hazard" changelog.d docs TYPE_LOWERING.md | head -80Repository: PerryTS/perry
Length of output: 14734
Scope the “eliminated crate-wide” claim.
Line 88 says the raw-pointer-across-lowering bug shape is “eliminated crate-wide,” then immediately says 262 hazard sites remain. State whether “eliminated” means absent from audited code or prevented by enforcement; otherwise readers can read a stronger crate-wide guarantee than the ledger supports.
🤖 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 `@docs/engine-plan.md` at line 88, Clarify the “raw-pointer-across-lowering bug
shape eliminated crate-wide” wording in the migration-status entry, explicitly
distinguishing audited/verified code from code prevented by enforcement. Align
the claim with the stated 262 remaining hazard sites and avoid implying a
crate-wide guarantee that the campaign ledger does not support.
| code cannot regress. **Done, as a test rather than an attribute**: Rust has | ||
| no `#[deny]` for "do not call this `pub(crate)` function from this module", | ||
| so `rooting::migration_ledger` `include_str!`s each finished module and | ||
| fails the build if it names `expr::temp_root`. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(rust-toolchain|Cargo\.toml|.*\.rs|docs/src/internals/rfc-rooting-by-construction\.md|docs/engine-plan\.md)$' || true
echo "== target doc snippets =="
for f in docs/src/internals/rfc-rooting-by-construction.md docs/engine-plan.md; do
if [ -f "$f" ]; then
echo "--- $f"
nl -ba "$f" | sed -n '218,242p;412,438p'
fi
done
echo "== migration_ledger references =="
rg -n "migration_ledger|MIGRATED_MODULES|denies|include_str|deny|build-fail|fail the build|fails the build|pub\(in crate::rooting\)|expr::temp_root|expr/temp_root" .Repository: PerryTS/perry
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read-only behavioral/code-shape probe: map implementation and doc claims without running repo code.
python3 - <<'PY'
from pathlib import Path
import re
files = [Path('docs/src/internals/rfc-rooting-by-construction.md'), Path('docs/engine-plan.md')]
for p in files:
if not p.exists():
print(f"MISSING {p}")
continue
text = p.read_text()
print(f"== {p} stats ==")
print(f"lines={text.count(chr(10))+1}")
for i, line in enumerate(text.splitlines(), 1):
if any(tok in line for tok in ['migration_ledger','MIGRATED_MODULES','include_str!','build fails','fails the build','denies','derive','cfg/test','cargo test','pub(in crate::rooting)']):
print(f"{i}: {line}")
rust_files=list(Path('.').rglob('*.rs'))
matches=[]
for p in rust_files:
hits=[]
txt=p.read_text(errors='ignore')
for tok in ['migration_ledger','MIGRATED_MODULES','include_str!','panic_if_temp_root','deny','expr::temp_root','pub(in crate::rooting)']:
if tok in txt:
hits.append(tok)
if hits:
matches.append((p,hits,txt))
print("== rust files with related tokens ==")
for p,hits,txt in sorted(matches, key=lambda x:x[0]):
print(f"{p}")
print(" tokens:", hits)
lines=txt.splitlines()
for j,l in enumerate(lines,1):
if any(tok in l for tok in hits):
print(f" {j}: {l}")
# Check whether the same identifier is called a test/function and whether the doc says build fail vs test.
needles={
'docs/src/internals/rfc-rooting-by-construction.md': ['migration_ledger','fails the build','cargo test','include_str!'],
'docs/engine-plan.md': ['MIGRATED_MODULES','denies `expr::temp_root`','cargo test']
}
for doc,toks in needles.items():
text=Path(doc).read_text()
print(f"== {doc} token presence ==")
for tok in toks:
print(tok, text.count(tok) if tok not in text.split() else 'count_words')
PYRepository: PerryTS/perry
Length of output: 25134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== crates/perry-codegen/src/rooting.rs relevant sections =="
nl -ba crates/perry-codegen/src/rooting.rs | sed -n '460,525p'
echo "== crates/perry-codegen/src/root_reload.rs relevant section =="
nl -ba crates/perry-codegen/src/root_reload.rs | sed -n '880,895p'
echo "== exact references to migration_ledger in Rust =="
rg -n "migration_ledger|MIGRATED_MODULES|root_reload|rooting::migration_ledger" crates/perry-codegen/srcRepository: PerryTS/perry
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf "== crates/perry-codegen/src/rooting.rs relevant sections ==\n"
awk 'NR>=460 && NR<=525 {printf "%6d\t%s\n", NR, $0}' crates/perry-codegen/src/rooting.rs
printf "== crates/perry-codegen/src/root_reload.rs relevant section ==\n"
awk 'NR>=880 && NR<=895 {printf "%6d\t%s\n", NR, $0}' crates/perry-codegen/src/root_reload.rs
printf "== exact references to migration_ledger in Rust ==\n"
rg -n "migration_ledger|MIGRATED_MODULES|root_reload|rooting::migration_ledger" crates/perry-codegen/srcRepository: PerryTS/perry
Length of output: 5716
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf "== docs engine-plan migration-ledger lines =="
awk 'NR>=424 && NR<=432 {printf "%6d\t%s\n", NR, $0}' docs/engine-plan.md
printf "== docs rfc grounding-by-construction full migration-ledger mention context =="
awk 'NR>=226 && NR<=236 {printf "%6d\t%s\n", NR, $0}' docs/src/internals/rfc-rooting-by-construction.mdRepository: PerryTS/perry
Length of output: 1897
Use precise migration-ledger guarantees.
rooting::migration_ledger is a #[cfg(test)] module, and its scope is limited to MIGRATED_MODULES. It does not make the worklist compile or prove that every unfinished module is checked.
docs/src/internals/rfc-rooting-by-construction.md#L231-L234: replace “fails the build” with wording that namesmigration_ledgerandcargo test, and mention that only registered modules are checked.docs/engine-plan.md#L427-L428: qualify “deniesexpr::temp_rootin them” as applying to added modules by the test, unless a stronger gate is added.
📍 Affects 2 files
docs/src/internals/rfc-rooting-by-construction.md#L231-L234(this comment)docs/engine-plan.md#L425-L431
🤖 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 `@docs/src/internals/rfc-rooting-by-construction.md` around lines 231 - 234,
The migration-ledger documentation overstates its guarantees. In
docs/src/internals/rfc-rooting-by-construction.md lines 231-234, revise the
statement to identify rooting::migration_ledger, its cargo test execution, and
that it checks only registered MIGRATED_MODULES; in docs/engine-plan.md lines
425-431, qualify the expr::temp_root denial as applying to modules added to that
test’s registry, without implying all unfinished modules are checked.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.d/7617-layer1-url-main-migration.md`:
- Line 1: Scope the changelog claim to the paths actually migrated: either
update the single-argument Expr::UrlParse handling to root js_url_coerce_string
and js_url_parse results instead of storing raw I64 values, or revise the
statement to limit the no-raw-pointer guarantee to the base-aware URL paths.
🪄 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: bf5cb276-d27d-42b7-8862-1a77a21bd854
📒 Files selected for processing (2)
changelog.d/7617-layer1-url-main-migration.mdcrates/perry-codegen/src/rooting.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-codegen/src/rooting.rs
| @@ -0,0 +1,3 @@ | |||
| - **The Layer 1 emitter migration is started, and the first module migrated end to end shows both what the discipline buys and what it does not.** `expr/url_main.rs` is now the template slice: it names no `expr::temp_root` symbol, and no raw heap pointer exists in it as a value a lowering can hold — `rooting::call_rooted` returns a slot rather than a register, `rooting::call_with_roots` re-reads each slot as part of emitting the consuming call (so #7461's `RootedSlot::read` is deleted; a register loaded from a root is stale the moment anything else collects, #7114/#7375), and `rooting::with_operands_rooted` owns the operand-group release on every path including `?`, which makes #7462's release-on-one-arm not a program. The migration found `URL.canParse(input, base)` and `URL.parse(input, base)` still carrying #7453's window — the same three lines #7453 fixed in `new URL(input, base)` and #7461 migrated, in the two static forms nobody re-read. IR is byte-identical on 16 of the 18 sources that exercise the module; the two that differ do so in 11 functions, every one of which calls `js_url_can_parse_with_base` or `js_url_parse_with_base`, adding only shadow-frame and root plumbing. **Stated plainly because a partial mechanism believed total is worse than one known partial: on the real emitter this does NOT make the bug fail to compile.** `FnCtx` has no interior mutability, so the RFC's borrow-carrying `Raw<'e>` cannot be built on it (#7459, #7461); the four sabotage arms are recorded in `rooting.rs` with their measured outcomes — the borrow form rejects #7192 with `E0499` (a new `compile_fail` doctest), the two escape-hatch arms fail the new per-module ledger test, and the two bare-builder arms compile silently. The ordered inventory of the remaining 87 modules — 694 raw-pointer sites, 262 hazard sites, ten slices — is #7615, linked from #7294. (#7617) | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Scope the migration claim to the paths that were actually migrated.
Line 1 states that expr/url_main.rs contains no raw heap pointer that a lowering can hold. However, the supplied crates/perry-codegen/src/expr/url_main.rs:237-317 context still shows the single-argument Expr::UrlParse path storing js_url_coerce_string and js_url_parse results in raw I64 values. Either migrate that path too, or state that the guarantee applies only to the base-aware paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@changelog.d/7617-layer1-url-main-migration.md` at line 1, Scope the changelog
claim to the paths actually migrated: either update the single-argument
Expr::UrlParse handling to root js_url_coerce_string and js_url_parse results
instead of storing raw I64 values, or revise the statement to limit the
no-raw-pointer guarantee to the base-aware URL paths.
84fba7d to
b928ec8
Compare
Audit before merge — verified, merged as v0.5.1352The most valuable sentence in this PR is the honest sabotage table. Asked Sabotage re-verified here: injecting a real The migration found two live bugs — Verification re-run here: codegen 688/0 + doctests (both The campaign now has its map: #7615 — 88 modules, 694 raw-pointer sites, |
1 participant
Layer 1 of the GC-correctness plan was the last unstarted track (
docs/engine-plan.md). This is its first slice plus the campaign map: one module migrated end to end onto the rooting-by-construction API as the template every subsequent slice copies, and the ordered inventory of the remaining 87 — #7615, linked from #7294.The module, and why
crates/perry-codegen/src/expr/url_main.rs(669 lines, URL / URLSearchParams / URLPattern).expr::temp_rootAPI. "No half-migrated module" is the rule this campaign has to establish, so the module that already violated it is the right place to establish it.js_url_coerce_stringreturns a bare*mut StringHeader— it is the callee fix(codegen): root the URL constructor's coerced string across base lowering (Layer 1) #7453 was filed against, and the reasonurl_coerce_stringhas a hand-written alternative in the checker'sALLOC_RE.UrlSearchParams.has/delete/forEachbuild their operand list conditionally) — the two shapes the API has to handle.fetch,http,Request,import.meta.urlall route through these lowerings. The template should be proved under real hazard.What the migration found
URL.canParse(input, base)andURL.parse(input, base)still carried #7453's window. Identical three lines to thenew URL(input, base)bug: a raw*mut StringHeaderheld in an SSA register across the lowering ofbase(arbitrary user code) and across a secondjs_url_coerce_stringthat allocates wheneverbaseis not already a string. #7453 fixed one of the three forms and #7461 migrated it; nobody re-read the other two.That repeats #7461's own finding, which is the argument for an API over a checklist.
IR-identity evidence
Program set: the 18
test-files/sources that exercise this module's lowerings, compiledPERRY_RS4GC=0 PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_INLINE_SHADOW_SLOT=0 PERRY_NO_AUTO_OPTIMIZE=1 --trace llvm, both arms..llfiles byte-identical.js_url_can_parse_with_baseorjs_url_parse_with_base, that the only added instructions are shadow-frame / root plumbing, and that nothing was removed. No other lowering changed.The two fixed lowerings gain a two-slot shadow frame, the two root stores, the two re-reads and the two releases. That is the cost of the fix, on those two lowerings only.
Coverage of the identity claim, checked rather than assumed. The 18 sources reach
js_url_new/new_with_base,can_parse/can_parse_with_base,parse/parse_with_base,search_params_{get,has,has2,set,append,delete,delete2,get_all,sort,to_string,entries_arr,new_any,new_empty}and five setters — but notURLPattern,searchParams.forEach, the typedkeys/valuesfamily, or the other four setters. A purpose-built probe covers exactly those, and its IR is byte-identical between arms. So the identity result is not an artefact of what the corpus happens to compile.Sabotage: does it fail to COMPILE?
No — and that is the finding worth more than the slice. Four historic shapes reintroduced into the migrated module, each result measured:
RootingEmitter)E0499compile_fail,E0499doctest added herecall_with_rootsresult across a later loweringctx.block().callexpr::temp_rootFnCtxhas no interior mutability, so the borrow-carryingRaw<'e>cannot be built on it — #7459 and #7461 established that, and this slice confirms it rather than working around it. What the combinator form actually buys is stated in the code and in the RFC, in those words:call_rootedproduces no unrooted register, so fix(codegen): root the URL constructor's coerced string across base lowering (Layer 1) #7453's window has no spelling.RootedSlothas noread— feat(codegen): migrate UrlNew onto the Layer 1 rooting API #7461'sread(&self, ctx) -> Stringis deleted, because a register loaded from a root is stale the moment anything else collects (String literal operand is not GC-rooted across an allocating call in the same expression (stale handle after evacuation) #7114, fix(gc): await polled a promise the event-loop pump had already moved #7375) and aStringremembers nothing.call_with_rootsfuses the re-read into the consuming call.with_operands_rootedowns the release on every path including?, so fix(codegen): root the URLSearchParams receiver across the name lowering #7462's release-on-one-arm is not a program.rooting::migration_ledger, whichinclude_str!s each finished module — the checkable form of the RFC's step 3, which Rust has no attribute for.An unused
root_i64(ctx, reg)combinator was written and then deleted, with the reason recorded in place: it is the one addition that would reopen the window, and it should arrive with a caller, not ahead of one.Second finding: the CI gate is blind to this bug class — #7616
Reintroducing the verbatim pre-#7453 code produces IR that
gc_root_dominance_check.pyreports as clean in all three modes — dominance 0, unrooted-allocas 0, stale-registers identical to the control. Dropping--moving-onlysurfaces 11 stale uses atjs_url_new_with_basein the sabotaged arm and 0 in the migrated one, so the shape is expressible; the--moving-onlyfilter discards the window becausejs_url_coerce_stringis inALLOC_REbut not inPOLL_CAPABLE_RUNTIME. #7453's own fix added it to one list and stopped.The one-line fix is measured on #7616 (curated corpus unchanged at 23/39; the dependency-scale arm is not measurable locally) and deliberately not included here: widening a gate is its own change with its own corpora to measure.
Verification (local — the CI backlog is deep, so this is the evidence)
gc-root-dominancecurated corpus, both gated modes, on the post-change compiler: 129/129 sources, 149 modules, 2452 functions, 9803 root stores → 0 violations, 0 unrooted-alloca violations, and--seeded-violations 40→ 40 planted, 40 caught, 0 missed. Baseline arm identical.--self-test,--audit-alloc-re,--audit-poll-capable,--audit-immovable-sources.cargo test -p perry-codegen --liband--doc(bothcompile_faildoctests reject; both ledger sabotage self-tests fire).cargo test -p perry-runtime --no-fail-fast../run_parity_tests.sh --filter urlagainst the pinned oracle (node 26.5.1): 14/14 PASS, 0 parity fail, 0 compile fail, 0 crashed, 0 skipped — 100%. Identical failure set on both arms, because the set is empty.cargo fmt --all -- --check,check_file_size.sh,addr_class_inventory.py,class_id_collisions.py,raw_handle_debt.py(+ self-tests),gc_store_site_inventory.py,workspace_architecture.py.Not run locally: the dependency-scale (
zod) dominance corpus, which needsnpm ci.Not in this PR
No version bump. No behaviour change beyond the two itemised bug fixes. No gate widening (#7616). No further modules — slice 1 is
lower_array_method.rs, 40 hazard sites, per #7615.Closes nothing; advances #7615.
https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Summary by CodeRabbit
Bug Fixes
Documentation