feat(codegen): migrate UrlNew onto the Layer 1 rooting API - #7461
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 |
First production lowering on the layer 1 discipline, and it closed a window the hand-fix in #7453 left behind. FnCtx has no interior mutability -- ctx.block() needs &mut -- so the borrow-carrying Raw from #7459 cannot be built on it: root(self) would need a second borrow while the handle holds the first. The shape that works against a &mut-only emitter is the combinator, which is what the runtime settled on for layer 3 (RuntimeHandle::across_*): never hand out an unrooted handle at all. call_rooted emits the collecting call and roots its result in one step. Emitted window, #7453 vs now: before after coerce url coerce url root url root url mk() <- user call mk() coerce base <- UNROOTED coerce base read url root base <- closed new_with_base(url, base_raw) read url read base new_with_base(read, read) base_ptr was live and unrooted across js_gc_temp_root_get in the hand-written version. Small window, but a window -- and I did not see it when writing that fix by hand, which is the argument for the API. 11/11 URL gap tests byte-identical to node; repro clean under PERRY_GC_HEAP_LIMIT=8 PERRY_GC_FORCE_EVACUATE=1.
c3d355f to
46938d3
Compare
1 participant
First production lowering migrated onto the Layer 1 discipline (#7459), and it closed a window my own hand-fix in #7453 left behind.
Why the combinator, not the borrow form
FnCtxhas no interior mutability —ctx.block()needs&mut— so #7459's borrow-carryingRawcannot be built on it directly:root(self)would need a second borrow while the handle still holds the first. That is the sameE0499the RFC's own API hits.The shape that works against a
&mut-only emitter is the combinator, and it is exactly what the runtime settled on for layer 3 (RuntimeHandle::across_*): never hand out an unrooted handle at all.call_rootedemits the collecting call and roots its result in one step, so there is no window in which an unrooted register exists to be misused.This is weaker than the borrow formulation — it prevents the bug rather than rejecting attempts to write it — but it needs no emitter rewrite, which is what makes the migration incremental instead of all-at-once.
It caught something I missed by hand
mk()— user callmk()new_with_base(url, base_raw)new_with_base(read, read)base_ptrwas live and unrooted acrossjs_gc_temp_root_getin the hand-written version. A small window, but a window — and I did not see it when writing that fix, which is the whole argument for having an API instead of a review checklist.Verification
new URL/URLPatterngap tests byte-identical to the pinned Node oraclePERRY_GC_HEAP_LIMIT=8 PERRY_GC_FORCE_EVACUATE=1rootingunit tests and doctests green;cargo fmt --checkcleanScope
One lowering.
UrlPatternNewand the rest ofperry-codegenare unchanged, and this does not finish Layer 1 — but the migration is now started against real code rather than a mock, and the first site it touched had a residual bug in it.