fix(hir,runtime): #5989 — Next.js cacheComponents boot-chain subset (strict global-builtin assignment + Date-extension construct + URLSearchParams shape-check SIGSEGV)#6014
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds a strict-mode runtime helper for assigning to existing global bindings (throwing ReferenceError otherwise), wires HIR lowering to use it, implements a real Reflect.construct thunk with Date-specific newTarget handling and direct builtin constructor identification, and hardens URL search params array validation. ChangesStrict-mode global assignment semantics
Reflect.construct real implementation and Date construction via newTarget
URL search params safety check
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HIRLowering
participant js_global_assign_existing_or_throw
participant globalThis
HIRLowering->>js_global_assign_existing_or_throw: call(name, value)
js_global_assign_existing_or_throw->>globalThis: probe existing binding/property
alt exists
js_global_assign_existing_or_throw->>globalThis: write value
js_global_assign_existing_or_throw-->>HIRLowering: return value
else absent
js_global_assign_existing_or_throw-->>HIRLowering: throw ReferenceError
end
sequenceDiagram
participant globalThis
participant reflect_construct_thunk
participant js_reflect_construct
globalThis->>reflect_construct_thunk: captured Reflect.construct call
reflect_construct_thunk->>js_reflect_construct: target, args_like, new_target
js_reflect_construct-->>reflect_construct_thunk: constructed value
reflect_construct_thunk-->>globalThis: return constructed value
Related issues: Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: perry-runtime maintainers, perry-hir lowering maintainers Poem
🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/url/search_params.rs (1)
849-859: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMaterialize lazy arrays before the
.lengthread
GC_TYPE_LAZY_ARRAYdoes not share the eagerArrayHeaderlayout, so(*keys_arr).lengthcan read the wrong field or fault. Use the same materialization/length helper used elsewhere before touchingkeys_arr, or keep lazy arrays out of this path.🤖 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-runtime/src/url/search_params.rs` around lines 849 - 859, The `keys_arr` handling in `search_params.rs` reads `(*keys_arr).length` even when the object may be a `GC_TYPE_LAZY_ARRAY`, which can access the wrong layout. Update this branch to use the same lazy-array materialization or safe length helper used elsewhere in the runtime before any length or element access, or reject lazy arrays here entirely; keep the existing `try_read_gc_header` and `js_array_get_f64` flow tied to the materialized array path.
🧹 Nitpick comments (1)
crates/perry-hir/src/lower/expr_assign.rs (1)
450-474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect lowering; consider deduplicating with
lower_expr/assignment.rs.The strict-mode branch here is functionally sound and matches the runtime helper's contract. However, the same ~20-line block (comment +
Expr::Callconstruction) is duplicated verbatim incrates/perry-hir/src/lower/lower_expr/assignment.rs(Lines 44-67). Extracting a small shared helper (e.g.,fn strict_global_assign_existing_or_throw(name: String, value: Box<Expr>) -> Expr) would prevent the two call sites from drifting apart.♻️ Suggested extraction
pub(crate) fn strict_global_assign_existing_or_throw(name: String, value: Box<Expr>) -> Expr { Expr::Call { callee: Box::new(Expr::ExternFuncRef { name: "js_global_assign_existing_or_throw".to_string(), param_types: vec![Type::Any, Type::Any], return_type: Type::Any, }), args: vec![Expr::String(name), *value], type_args: vec![], byte_offset: 0, } }🤖 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-hir/src/lower/expr_assign.rs` around lines 450 - 474, The strict-mode global assignment lowering in Expr::Call is duplicated between expr_assign.rs and lower_expr/assignment.rs, so extract the shared construction into a helper and reuse it from both call sites. Create a small shared function around js_global_assign_existing_or_throw that takes the identifier name and RHS value, returns the Expr::Call, and update the strict branch in the ExprAssign lowering and the assignment lowering to call that helper so the two implementations cannot drift.
🤖 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-hir/src/lower_decl/block.rs`:
- Around line 121-145: The forward stub for heritage classes is being registered
under the raw class identifier, while the class-body lowering path resolves and
looks up the renamed symbol, so collisions can leave captures pointing at an
undefined stub. Update the class-declaration handling in block lowering to use
the resolved class name consistently when creating the forward boxed local and
inserting into lexical_forward_decls, matching the lookup logic used in
body_stmt lowering. Make sure the fix keeps the superclass-only gating and
existing scope check behavior intact.
---
Outside diff comments:
In `@crates/perry-runtime/src/url/search_params.rs`:
- Around line 849-859: The `keys_arr` handling in `search_params.rs` reads
`(*keys_arr).length` even when the object may be a `GC_TYPE_LAZY_ARRAY`, which
can access the wrong layout. Update this branch to use the same lazy-array
materialization or safe length helper used elsewhere in the runtime before any
length or element access, or reject lazy arrays here entirely; keep the existing
`try_read_gc_header` and `js_array_get_f64` flow tied to the materialized array
path.
---
Nitpick comments:
In `@crates/perry-hir/src/lower/expr_assign.rs`:
- Around line 450-474: The strict-mode global assignment lowering in Expr::Call
is duplicated between expr_assign.rs and lower_expr/assignment.rs, so extract
the shared construction into a helper and reuse it from both call sites. Create
a small shared function around js_global_assign_existing_or_throw that takes the
identifier name and RHS value, returns the Expr::Call, and update the strict
branch in the ExprAssign lowering and the assignment lowering to call that
helper so the two implementations cannot drift.
🪄 Autofix (Beta)
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: 3cfdcfcb-07f9-4a46-874a-9d0f5328b204
📒 Files selected for processing (14)
crates/perry-codegen/src/runtime_decls/strings.rscrates/perry-hir/src/lower/expr_assign.rscrates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower/lower_expr/assignment.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-runtime/src/error.rscrates/perry-runtime/src/object/class_registry/class_meta.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this/bigint_promise.rscrates/perry-runtime/src/object/global_this/install_static.rscrates/perry-runtime/src/url/search_params.rscrates/perry/tests/issue_5989_strict_global_builtin_assignment.rs
Comment on lines
+121
to
+145
| if let ast::Stmt::Decl(ast::Decl::Class(class_decl)) = stmt { | ||
| let name = class_decl.ident.sym.to_string(); | ||
| // Heritage classes ONLY: a class with a superclass binds its | ||
| // runtime value through a fresh-class-value `Let` | ||
| // (`ClassExprFresh` / dynamic parent), so earlier closures must | ||
| // capture the boxed local. A plain class's value IS the static | ||
| // `ClassRef` — pre-registering a local for it would make the | ||
| // class's OWN body references (e.g. render-result.js's | ||
| // `static #_ = this.EMPTY = new RenderResult(...)` self-construct | ||
| // during class initialization) resolve to the still-undefined | ||
| // stub instead of the class itself. | ||
| if class_decl.class.super_class.is_some() && seen_closure_refs.contains(&name) { | ||
| let already_in_scope = ctx | ||
| .locals | ||
| .lookup_index_in_scope(&name, body_entry_locals_len) | ||
| .is_some(); | ||
| if !already_in_scope { | ||
| let span_lo = class_decl.ident.span.lo.0; | ||
| let id = ctx.define_local(name, Type::Any); | ||
| ctx.var_hoisted_ids.insert(id); | ||
| forward_boxed_ids.push(id); | ||
| ctx.lexical_forward_decls.insert(span_lo, id); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Trace ordering: pre_register_forward_captured_lets vs maybe_rename_colliding_class,
# and how body_stmt resolves the class local for reuse.
rg -nP '\b(pre_register_forward_captured_lets|maybe_rename_colliding_class|resolve_class_name|lookup_local_in_current_scope)\b' crates/perry-hir/srcRepository: PerryTS/perry
Length of output: 4301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant lowering flow and name-resolution helpers.
sed -n '1035,1110p' crates/perry-hir/src/lower_decl/block.rs
printf '\n--- body_stmt.rs ---\n'
sed -n '260,430p' crates/perry-hir/src/lower_decl/body_stmt.rs
printf '\n--- context.rs ---\n'
sed -n '400,440p' crates/perry-hir/src/lower/context.rs
printf '\n--- expr_function.rs ---\n'
sed -n '1000,1095p' crates/perry-hir/src/lower/expr_function.rsRepository: PerryTS/perry
Length of output: 22458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- crates/perry-hir/src/lower_decl/block.rs (960-1115) ---\n'
sed -n '960,1115p' crates/perry-hir/src/lower_decl/block.rs
printf '\n--- crates/perry-hir/src/lower_decl/body_stmt.rs (390-430) ---\n'
sed -n '390,430p' crates/perry-hir/src/lower_decl/body_stmt.rs
printf '\n--- crates/perry-hir/src/lower_decl/mod.rs ---\n'
sed -n '1,80p' crates/perry-hir/src/lower_decl/mod.rsRepository: PerryTS/perry
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- crates/perry-hir/src/lower_decl/block.rs (960-1115) ---'
sed -n '960,1115p' crates/perry-hir/src/lower_decl/block.rs
printf '%s\n' ''
printf '%s\n' '--- crates/perry-hir/src/lower_decl/body_stmt.rs (390-430) ---'
sed -n '390,430p' crates/perry-hir/src/lower_decl/body_stmt.rsRepository: PerryTS/perry
Length of output: 12677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '430,520p' crates/perry-hir/src/lower_decl/body_stmt.rsRepository: PerryTS/perry
Length of output: 5714
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' crates/perry-hir/src/lower/lower_expr/arm_ident.rsRepository: PerryTS/perry
Length of output: 11665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP '\b(lookup_class_captures|RegisterClassCaptures|append_new_args_stmt|class_captures|pending_classes|class.name)\b' crates/perry-hir/src/lower_decl crates/perry-hir/src/lowerRepository: PerryTS/perry
Length of output: 50369
Use the resolved class name for the forward stub too
crates/perry-hir/src/lower_decl/block.rs:121-145 pre-registers the stub under the raw class name, but the class-body path in crates/perry-hir/src/lower_decl/body_stmt.rs:405 looks up the resolved name. For a colliding renamed heritage class (X → X$0), the earlier capture keeps pointing at the raw stub and stays undefined.
🤖 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-hir/src/lower_decl/block.rs` around lines 121 - 145, The forward
stub for heritage classes is being registered under the raw class identifier,
while the class-body lowering path resolves and looks up the renamed symbol, so
collisions can leave captures pointing at an undefined stub. Update the
class-declaration handling in block lowering to use the resolved class name
consistently when creating the forward boxed local and inserting into
lexical_forward_decls, matching the lookup logic used in body_stmt lowering.
Make sure the fix keeps the superclass-only gating and existing scope check
behavior intact.
Contributor
Author
There was a problem hiding this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git log --oneline -5 --all | head -20 || true
git show --stat 033ecf857 2>/dev/null | head -30 || echo "commit 033ecf857 not found"
git show --stat 2a8179534 2>/dev/null | head -30 || echo "commit 2a8179534 not found"Length of output: 2527
Confirmed: these two commits address different review threads (the shape_is_url_search_params SIGSEGV hardening and the duplicated strict-assign lowering), not this thread's concern about pre_register_forward_captured_lets using the raw class name instead of the resolved/renamed name for the forward-captured heritage-class stub in crates/perry-hir/src/lower_decl/block.rs (Lines 121–145).
@proggeramlug Thanks for the update — nice cleanup on both. To confirm: those two commits (033ecf8, 2a81795) don't touch crates/perry-hir/src/lower_decl/block.rs, so this particular finding about using ctx.resolve_class_name(...) for the forward stub (to match the lookup in body_stmt.rs around Line 405) is still open. Let me know if you'd like me to hold this thread separately or if it's already tracked elsewhere.
proggeramlug
pushed a commit
that referenced
this pull request
Jul 5, 2026
…s keys_array gate CodeRabbit: a lazy array does not share the eager ArrayHeader layout, so reading (*keys_arr).length / js_array_get_f64 on one would access the wrong field. An object's own key list is always an eager GC_TYPE_ARRAY (the pre-fix code read .length directly, proving it is never lazy here), so require GC_TYPE_ARRAY specifically and reject lazy arrays — a real URLSearchParams shape never has a lazy keys_array, so returning false is correct.
proggeramlug
pushed a commit
that referenced
this pull request
Jul 5, 2026
…helper CodeRabbit: the ~20-line strict-mode global-assignment lowering was duplicated verbatim in both assignment arms (expr_assign.rs and lower_expr/assignment.rs). Extract to a shared pub(crate) helper in lower_expr/helpers.rs so the two call sites can't drift.
added 6 commits
July 5, 2026 09:47
…ltin is a property write, not a ReferenceError
Strict-mode 'X = v' with no lexical binding for X lowered to an
unconditional ReferenceError in both assignment arms. Per spec (PutValue
on a reference that resolves to the global environment), an EXISTING
global property is a normal property write; only a genuinely absent
binding throws.
Next.js 16's cacheComponents node-environment extensions install their
dynamic-IO clock interceptor exactly this way — strict CJS
'Date = createDate(Date)' — so every server boot printed "Failed to
install `Date` class extension" (the install's catch{} swallowed the
throw) and time-read dynamic detection never armed, one layer of the
dynamic-RSC route hang (#5989). Perry's globalThis machinery already
handles the write and bare-identifier reads observe the override
(verified byte-identical to node); only the assignment lowering blocked
it.
New runtime helper js_global_assign_existing_or_throw does the presence
probe + write-back (mirroring js_global_update's shape for ++x on
globals) and throws the named ReferenceError when absent; both lowering
arms (expr_assign.rs lower_assignment_target + lower_expr/assignment.rs)
now emit it in strict mode. RHS-before-throw evaluation order preserved.
Integration tests cover the Next.js date.js shape end-to-end and the
absent-name ReferenceError.
…l_search_params The receiver gate (#5964) validates the OBJECT header, but keys_array was dereferenced raw — a GC_TYPE_OBJECT reached mid-transition (or with a typed layout) can carry a non-heap word there, and reading (*keys_arr).length SIGSEGV'd during Next.js request handling (config.js method dispatch probes arbitrary receivers through this shape check; first request killed the server). Gate keys_array with the same try_read_gc_header check (#5429/#5943 family).
…s keys_array gate CodeRabbit: a lazy array does not share the eager ArrayHeader layout, so reading (*keys_arr).length / js_array_get_f64 on one would access the wrong field. An object's own key list is always an eager GC_TYPE_ARRAY (the pre-fix code read .length directly, proving it is never lazy here), so require GC_TYPE_ARRAY specifically and reject lazy arrays — a real URLSearchParams shape never has a lazy keys_array, so returning false is correct.
…helper CodeRabbit: the ~20-line strict-mode global-assignment lowering was duplicated verbatim in both assignment arms (expr_assign.rs and lower_expr/assignment.rs). Extract to a shared pub(crate) helper in lower_expr/helpers.rs so the two call sites can't drift.
… real Dates
Three coupled gaps broke Next.js 16's installed Date wrapper (every
construction through it yielded unbranded 'Invalid Date' objects):
1. The reified Reflect.construct VALUE was a no-op stub returning
undefined — the wrapper calls it through a captured binding
('const construct = Reflect.construct'). Route it to the real
js_reflect_construct (reflect_construct_thunk, mirroring
reflect_apply_thunk).
2. Reflect.construct(Date, args, newTarget) with a distinct newTarget
fell to the generic construct tail, which allocates a PLAIN object
and invokes the Date thunk against it. Build the real branded Date
and honor GetPrototypeFromConstructor like the typed-array arm.
3. identify_global_builtin_constructor recovered names via the
globalThis singleton walk, which breaks the moment user code
REASSIGNS the global binding (the whole point of the extension) —
the wrapper's captured ORIGINAL constructor stopped being
identifiable. Dedicated per-builtin thunks now map to their names
directly, without consulting globalThis.
Syntactic Date.now() sites compile to the builtin intrinsic and bypass an installed global override (known, separately-tracked gap) — assert interception through a captured Date.now binding instead.
proggeramlug
force-pushed
the
fix/5989-global-builtin-assignment-clean
branch
from
2a81795 to
90ad524
Compare
proggeramlug
changed the title
Contributor
Author
|
Retargeted to the #5874-independent subset. While this PR was in review, main merged #6015 (revert of #5874 "Improve Zod compatibility support", which regressed 322 test262 tests). That revert deleted the |
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 `@crates/perry-runtime/src/object/class_registry/construct.rs`:
- Around line 912-928: The heap-pointer check in construct.rs is using a raw
address threshold that can still admit small handle values before the unsafe
GcHeader dereference. Update the has_user_proto logic to reuse the canonical
is_plausible_heap_addr(raw) guard, or an equivalent handle-band plus heap-range
check, before reading hdr in the Object/Array prototype detection path. Keep the
existing is_fn fast path and the hdr.obj_type match, but ensure the unsafe deref
only happens for plausible heap addresses.
🪄 Autofix (Beta)
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: 515daca8-3ff1-4f5d-8fa7-da30cf441b57
📒 Files selected for processing (14)
crates/perry-codegen/src/runtime_decls/strings.rscrates/perry-hir/src/lower/expr_assign.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-hir/src/lower/lower_expr/assignment.rscrates/perry-hir/src/lower/lower_expr/helpers.rscrates/perry-hir/src/lower/mod.rscrates/perry-runtime/src/error.rscrates/perry-runtime/src/object/class_registry/class_meta.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this/bigint_promise.rscrates/perry-runtime/src/object/global_this/install_static.rscrates/perry-runtime/src/url/search_params.rscrates/perry/tests/issue_5989_strict_global_builtin_assignment.rs
✅ Files skipped from review due to trivial changes (1)
- crates/perry-hir/src/lower/lower_expr.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/perry-runtime/src/object/global_this/install_static.rs
- crates/perry-codegen/src/runtime_decls/strings.rs
- crates/perry-hir/src/lower/lower_expr/assignment.rs
- crates/perry-runtime/src/object/global_this.rs
- crates/perry-hir/src/lower/lower_expr/helpers.rs
- crates/perry-hir/src/lower/mod.rs
- crates/perry-runtime/src/object/class_registry/class_meta.rs
- crates/perry-runtime/src/object/global_this/bigint_promise.rs
- crates/perry-runtime/src/error.rs
- crates/perry-hir/src/lower/expr_assign.rs
- crates/perry-runtime/src/url/search_params.rs
- crates/perry/tests/issue_5989_strict_global_builtin_assignment.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@crates/perry-runtime/src/object/class_registry/construct.rs`:
- Around line 912-928: The heap-pointer check in construct.rs is using a raw
address threshold that can still admit small handle values before the unsafe
GcHeader dereference. Update the has_user_proto logic to reuse the canonical
is_plausible_heap_addr(raw) guard, or an equivalent handle-band plus heap-range
check, before reading hdr in the Object/Array prototype detection path. Keep the
existing is_fn fast path and the hdr.obj_type match, but ensure the unsafe deref
only happens for plausible heap addresses.
🪄 Autofix (Beta)
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: 515daca8-3ff1-4f5d-8fa7-da30cf441b57
📒 Files selected for processing (14)
crates/perry-codegen/src/runtime_decls/strings.rscrates/perry-hir/src/lower/expr_assign.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-hir/src/lower/lower_expr/assignment.rscrates/perry-hir/src/lower/lower_expr/helpers.rscrates/perry-hir/src/lower/mod.rscrates/perry-runtime/src/error.rscrates/perry-runtime/src/object/class_registry/class_meta.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this/bigint_promise.rscrates/perry-runtime/src/object/global_this/install_static.rscrates/perry-runtime/src/url/search_params.rscrates/perry/tests/issue_5989_strict_global_builtin_assignment.rs
✅ Files skipped from review due to trivial changes (1)
- crates/perry-hir/src/lower/lower_expr.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/perry-runtime/src/object/global_this/install_static.rs
- crates/perry-codegen/src/runtime_decls/strings.rs
- crates/perry-hir/src/lower/lower_expr/assignment.rs
- crates/perry-runtime/src/object/global_this.rs
- crates/perry-hir/src/lower/lower_expr/helpers.rs
- crates/perry-hir/src/lower/mod.rs
- crates/perry-runtime/src/object/class_registry/class_meta.rs
- crates/perry-runtime/src/object/global_this/bigint_promise.rs
- crates/perry-runtime/src/error.rs
- crates/perry-hir/src/lower/expr_assign.rs
- crates/perry-runtime/src/url/search_params.rs
- crates/perry/tests/issue_5989_strict_global_builtin_assignment.rs
🛑 Comments failed to post (1)
crates/perry-runtime/src/object/class_registry/construct.rs (1)
912-928: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm the handle-band / valid-obj-ptr helpers and their module path used by is_closure_ptr. rg -nP 'pub (fn|const fn) (is_handle_band|is_valid_obj_ptr)\b' crates/perry-runtime/src # Check whether any dynamic "prototype" value can carry a handle-band pointer tag. rg -nP 'closure_(set|get)_dynamic_prop' crates/perry-runtime/src -C2 | rg -n 'prototype'Repository: PerryTS/perry
Length of output: 15629
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- construct.rs around the cited branch ---' sed -n '890,935p' crates/perry-runtime/src/object/class_registry/construct.rs echo echo '--- value/addr_class.rs helpers ---' sed -n '1,180p' crates/perry-runtime/src/value/addr_class.rs echo echo '--- closure/dynamic_props.rs is_closure_ptr context ---' rg -n 'pub fn is_closure_ptr|is_valid_obj_ptr|is_handle_band|SIGSEGV|Headers' crates/perry-runtime/src/closure/dynamic_props.rs -n -C 4 echo echo '--- prototype_objects.rs relevant prototype handling ---' sed -n '1,80p' crates/perry-runtime/src/object/class_registry/prototype_objects.rs echo echo '--- any direct use of dp.is_pointer in construct.rs nearby ---' rg -n 'is_pointer\(\)|has_user_proto|ensure_function_prototype_object|closure_get_dynamic_prop' crates/perry-runtime/src/object/class_registry/construct.rs -n -C 3Repository: PerryTS/perry
Length of output: 24698
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n 'is_plausible_heap_addr\(' crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 1391
Use the canonical heap-pointer guard here.
dp.is_pointer()can still be a small handle value, andraw >= GC_HEADER_SIZE + 0x1000doesn’t exclude the handle band, so a fetch/Headers-style id can fault on theGcHeaderread. Reuseis_plausible_heap_addr(raw)(or the same handle-band + heap-range check) before the unsafe deref.🤖 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-runtime/src/object/class_registry/construct.rs` around lines 912 - 928, The heap-pointer check in construct.rs is using a raw address threshold that can still admit small handle values before the unsafe GcHeader dereference. Update the has_user_proto logic to reuse the canonical is_plausible_heap_addr(raw) guard, or an equivalent handle-band plus heap-range check, before reading hdr in the Object/Array prototype detection path. Keep the existing is_fn fast path and the hdr.obj_type match, but ensure the unsafe deref only happens for plausible heap addresses.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #5988 (async-step deadlock) — hardens the boot chain behind #5989's "dynamic RSC render evaporates silently."
Once #5988 gave the render a clean async graph, the request stopped hanging and started failing forward, which unmasked a chain of pre-existing compiler/runtime bugs that Next.js 16's
cacheComponentspipeline hits before it can render. Each fix here has a node-identical minimal repro; none are async-related.The five fixes
fix(hir,runtime)— strict assignment to an existing global builtin is a property write, not a ReferenceError. Strict-modeX = vwith no lexical binding threw unconditionally; per spec (PutValue resolving to the global environment) an existing global property is a normal write. Next'scacheComponentsextensions install their dynamic-IO clock interceptor via strict CJSDate = createDate(Date)— so every boot printed "Failed to installDateclass extension" (the install'scatch{}swallowed the throw) and clock-based dynamic detection never armed. New runtime helperjs_global_assign_existing_or_throw(presence-probe + write-back, mirroringjs_global_update); both HIR assignment arms emit it in strict mode via a shared helper; absent bindings still throw the named ReferenceError.fix(runtime)— make the Date-extension wrapper shape construct real Dates. Three coupled gaps made every construction through the installed wrapper an unbranded "Invalid Date": (a) the reifiedReflect.constructVALUE was a no-op stub returningundefined— the wrapper calls it through a capturedconst construct = Reflect.constructbinding; (b)Reflect.construct(Date, args, newTarget)fell to the generic construct tail (plain object + thunk); (c)identify_global_builtin_constructorrecovered names by walkingglobalThis, which breaks the instant the global is reassigned (the extension's whole point). Fixed with a realreflect_construct_thunk, a Date arm honoringGetPrototypeFromConstructor, and a direct thunk→name map for dedicated builtin thunks. Verified on the post-revert base: theReflect.construct(Date, arguments, new.target)case the wrapper uses now matches node.fix(runtime)— validatekeys_arraybefore deref inshape_is_url_search_params. The fix(runtime): #5961 — URLSearchParams methods reachable through dynamic access #5964 receiver gate validated the object header but dereferencedkeys_arrayraw; aGC_TYPE_OBJECTreceiver reached mid-transition carries a non-heap word there, and reading(*keys_arr).lengthSIGSEGV'd on the first request (config.js method dispatch probes arbitrary receivers through this shape check). Require the eagerGC_TYPE_ARRAYlayout specifically (an object's own key list is always eager; per CodeRabbit, rejectGC_TYPE_LAZY_ARRAYrather than read its non-matching layout) via the sametry_read_gc_headercheck (Segmentation fault.: console.log(dayjs(1749820051142).format()); #5429/fix(runtime): validate spread sources; full handle band + no type-pun on the dynamic set path #5943 family).Plus the two CodeRabbit review fixes folded in (lazy-array rejection above; shared
strict_global_assign_existing_or_throwhelper to dedupe the two assignment arms).Validation (on current main, post-#6015)
crates/perry/tests/issue_5989_strict_global_builtin_assignment.rs): the exact Next.jsdate.jswrapper shape installs + intercepts (value path), and absent bindings still throw the named ReferenceError.proto-constructorcase byte-identical to node); URLSearchParams byte-identical to node.cargo fmt, GC store-site inventory, address-classification audit, file-size gate all clean.What still blocks 8/8 (tracked in #5989)
The deferred class-forward-capture fixes are what got the compiled zod bundle past config-schema validation at boot on the #5874 base. Without #5874's machinery on current main, that path is different and needs re-assessment. Boot-chain diagnostics, repro recipes (
require("next/dist/server/config-schema")reproduces the boot-blocking layers in ~2 min without a bundle compile), and the full findings are on #5989.Summary by CodeRabbit
New Features
Reflect.construct, including correct handling of the optional new-target behavior.Bug Fixes
Dateconstruction so instances are created with the correct branding and prototype behavior.ReferenceError.