fix(runtime,codegen,hir): Next.js standalone walls 31-35#5112
Merged
Conversation
…ative Stream proto, depd new Function, console[m], default params in CJS)
- type_analysis: `.stack` is no longer typed String unconditionally — it can
be an array (Error.prepareStackTrace) and a plain object may carry any value;
typing it String corrupted array stores (wall 31).
- error.rs + global_this: CallSite shim + honor a user-set
Error.prepareStackTrace; ERROR_CONSTRUCTOR_PTR + default thunk address (wall 31).
- util_inherits: synthesize a .prototype for callable bound-native constructors
(require('stream')) so util.inherits(X, Stream) chains (wall 32).
- new Function with a runtime-built body: recognize depd's wrapper template and
return a working native fn (no JS interpreter); js_function_ctor_from_strings
+ depd_wrapfunction_outer_thunk (wall 33).
- console[dynamicKey]: route to js_console_method_by_value (same bound closure as
console.<key>) — the computed form collapsed the receiver to GlobalGet(0)=0,
so console[m](...) threw '(number).m is not a function' (wall 34).
- nested_fn_decl: emit the default-param prologue for block-nested function decls
— every top-level fn in a CJS module becomes block-nested under cjs_wrap, so
'function f(a, opts={})' dropped its default (wall 35).
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds four runtime compatibility features: ChangesRuntime JS Compatibility Features
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
🤖 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/type_analysis.rs`:
- Around line 399-401: In the `refine_type_from_init` function at the "message"
or "name" property check, you are returning `HirType::String` immediately
without first checking if the object has a declared field type for that
property. Reorder the logic to first check if the object parameter has a
declared field definition for this property name, and only fall back to
returning `HirType::String` if no declared field type exists. This ensures that
user-defined classes with their own `name` or `message` fields preserve their
actual declared types instead of being incorrectly overridden to `String`.
In `@crates/perry-runtime/src/builtins/console.rs`:
- Around line 170-174: The js_console_method_by_value function currently returns
undefined for non-string keys without attempting coercion. Apply JavaScript key
coercion to the input key before performing the console method lookup, similar
to how computed member access should work. Replace the direct
jsvalue_string_content check with a function that coerces the key value to a
string first (following standard JS member access coercion rules), so that
numeric and other coercible keys are properly resolved instead of immediately
returning TAG_UNDEFINED.
In `@crates/perry-runtime/src/error.rs`:
- Around line 1089-1093: The PropertyAttrs being passed to set_property_attrs
for the "stack" property descriptor currently uses true as the first argument
(writable flag), but accessor descriptors in ECMAScript should not include a
writable attribute since that applies only to data descriptors. Change the
PropertyAttrs::new call to use false as the first argument instead of true, so
it reads PropertyAttrs::new(false, false, true), to accurately reflect that this
is an accessor descriptor with only configurable and enumerable attributes.
In `@crates/perry-runtime/src/object/global_this.rs`:
- Around line 1259-1287: The depd wrapper detection in
js_function_ctor_from_strings is too loose—it only checks three body substrings,
which could match unrelated Function bodies. Before returning the native thunk
closure, add stricter validation: check that args_len equals 6 (the depd wrapper
expects 5 parameters plus the body) and validate that the parameter names match
the expected depd parameter names ("fn", "log", "deprecate", "message", "site").
Only take the fast path (returning the depd_wrapfunction_outer_thunk closure)
when both the body substring patterns AND the argument count and parameter names
match.
- Around line 5010-5027: The `ERROR_CONSTRUCTOR_PTR` static atomic stores a
process-global raw pointer to the Error constructor, but the file already treats
globalThis as thread-local to handle arena pointer validity across threads. This
creates a thread-safety hazard where one thread's stored constructor pointer can
be read by another thread, potentially causing access to invalid or freed
memory. Replace the process-global `ERROR_CONSTRUCTOR_PTR` with a thread-local
storage mechanism, or refactor the code to resolve the Error constructor from
the current thread's realm instead of storing it in a process-global atomic.
This change will affect how `install_error_static_methods` stores the
constructor and how `default_prepare_stack_trace_func_ptr()` and
`error::error_prepare_stack_trace_override` retrieve it.
In `@crates/perry-runtime/src/util_inherits.rs`:
- Around line 86-109: The prototype synthesis code (starting with
js_object_alloc and extending through the closure_set_dynamic_prop and
js_object_set_field_by_name calls) currently applies to any value type, but
should only apply to callable values. Add a check at the beginning of this
section to verify that the value is callable (checking if it's a bound-native or
function-like value); if the value is not callable, return current directly
instead of proceeding with the prototype synthesis. This preserves the existing
validation failure behavior for non-callable objects while allowing the
synthesis to proceed only for actual callable values like native functions.
🪄 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: af283824-fee2-4538-a37e-0f8765708e05
📒 Files selected for processing (12)
crates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/runtime_decls/mod.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-codegen/src/type_analysis.rscrates/perry-hir/src/lower/expr_member.rscrates/perry-hir/src/lower_decl/body_stmt/nested_fn_decl.rscrates/perry-runtime/src/builtins/console.rscrates/perry-runtime/src/builtins/mod.rscrates/perry-runtime/src/error.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/util_inherits.rs
Comment on lines
+1089
to
+1093
| crate::object::set_property_attrs( | ||
| target_ptr as usize, | ||
| "stack".to_string(), | ||
| crate::object::PropertyAttrs::new(true, false, true), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how PropertyAttrs is used elsewhere for accessor properties
rg -n "set_property_attrs.*accessor" --type rust
rg -n "PropertyAttrs::new.*getter" --type rust -B2 -A2
# Check PropertyAttrs definition to understand if writable is ignored for accessors
ast-grep --pattern 'struct PropertyAttrs { $$$ }'Repository: PerryTS/perry
Length of output: 10942
🏁 Script executed:
# Find PropertyAttrs struct definition
find . -name "*.rs" -type f | xargs grep -l "struct PropertyAttrs" | head -5Repository: PerryTS/perry
Length of output: 98
🏁 Script executed:
# Check PropertyAttrs struct and set_property_attrs implementation
rg -B2 -A10 "^[[:space:]]*(pub )?struct PropertyAttrs" --type rustRepository: PerryTS/perry
Length of output: 39
🏁 Script executed:
# Find and examine set_property_attrs function
rg -B2 -A20 "fn set_property_attrs" --type rust | head -100Repository: PerryTS/perry
Length of output: 1875
🏁 Script executed:
# Read the specific lines from error.rs to confirm context
sed -n '1080,1100p' crates/perry-runtime/src/error.rsRepository: PerryTS/perry
Length of output: 783
🏁 Script executed:
# Find PropertyAttrs struct definition and implementation
rg -B5 -A15 "struct PropertyAttrs" crates/perry-runtime/src/object/mod.rsRepository: PerryTS/perry
Length of output: 788
🏁 Script executed:
# Check how PropertyAttrs is used when reading descriptors
rg -B3 -A3 "PropertyAttrs\|get_property_attrs" crates/perry-runtime/src/object/mod.rs | head -80Repository: PerryTS/perry
Length of output: 39
🏁 Script executed:
# Search for how Object.getOwnPropertyDescriptor is implemented
rg -B5 -A20 "getOwnPropertyDescriptor\|get_own_property_descriptor" --type rustRepository: PerryTS/perry
Length of output: 39
🏁 Script executed:
# Check how PropertyAttrs is interpreted when there's also an AccessorDescriptor
rg -B10 -A10 "get_accessor_descriptor\|AccessorDescriptor" crates/perry-runtime/src/object/mod.rs | head -100Repository: PerryTS/perry
Length of output: 39
🏁 Script executed:
# Check where PropertyAttrs is used when returning property descriptors
rg -B5 -A15 "Object\.getOwnPropertyDescriptor\|get_property_attrs\|PropertyAttrs" crates/perry-runtime/src/object/mod.rs | grep -A15 -B5 "get.*property\|descriptor"Repository: PerryTS/perry
Length of output: 39
🏁 Script executed:
# Look for how builtin functions construct Object.getOwnPropertyDescriptor responses
rg -B5 -A20 '"getOwnPropertyDescriptor"' --type rustRepository: PerryTS/perry
Length of output: 31445
🏁 Script executed:
# Find js_object_get_own_property_descriptor implementation to see how it combines accessor + attrs
rg -B5 -A30 "pub.*fn js_object_get_own_property_descriptor" --type rustRepository: PerryTS/perry
Length of output: 7395
🏁 Script executed:
# Check if there's branching based on whether an accessor exists
rg -B3 -A15 "get_accessor_descriptor.*obj.*key" crates/perry-runtime/src/object/mod.rsRepository: PerryTS/perry
Length of output: 636
🏁 Script executed:
# Look for how PropertyAttrs is read and used when constructing descriptor results
rg -B2 -A10 "get_property_attrs\|PROPERTY_DESCRIPTORS" crates/perry-runtime/src/object/mod.rs | head -60Repository: PerryTS/perry
Length of output: 39
🏁 Script executed:
# Get the full implementation of js_object_get_own_property_descriptor
sed -n '1,200p' crates/perry-runtime/src/object/descriptors.rs | tail -150Repository: PerryTS/perry
Length of output: 6938
🏁 Script executed:
# Search more of the descriptors.rs file to find how PropertyAttrs is applied
sed -n '1,300p' crates/perry-runtime/src/object/descriptors.rs | grep -A50 "js_object_get_own_property_descriptor"Repository: PerryTS/perry
Length of output: 2805
🏁 Script executed:
# Get lines 100-300 of descriptors.rs to see the full descriptor logic
sed -n '100,300p' crates/perry-runtime/src/object/descriptors.rsRepository: PerryTS/perry
Length of output: 10428
🏁 Script executed:
# Look for build_accessor_descriptor and build_data_descriptor to see how they use PropertyAttrs
rg -B5 -A15 "fn build_(accessor|data)_descriptor" crates/perry-runtime/src/object/descriptors.rsRepository: PerryTS/perry
Length of output: 1858
PropertyAttrs for accessor descriptors should omit the writable flag.
ECMAScript accessor descriptors only have configurable and enumerable attributes; writable applies to data descriptors only. While the runtime correctly ignores the writable bit when building the descriptor object via build_accessor_descriptor(), storing it in PropertyAttrs is semantically misleading. Consider using PropertyAttrs::new(false, false, true) instead, or introduce an accessor-specific variant to clarify intent.
🤖 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/error.rs` around lines 1089 - 1093, The
PropertyAttrs being passed to set_property_attrs for the "stack" property
descriptor currently uses true as the first argument (writable flag), but
accessor descriptors in ECMAScript should not include a writable attribute since
that applies only to data descriptors. Change the PropertyAttrs::new call to use
false as the first argument instead of true, so it reads
PropertyAttrs::new(false, false, true), to accurately reflect that this is an
accessor descriptor with only configurable and enumerable attributes.
- type_analysis: a user class's DECLARED .message/.name field type wins over
the Error-instance String assumption.
- console.rs: apply JS property-key coercion in js_console_method_by_value
(console[0] → "0"; Symbol key → miss).
- global_this: tighten the depd new Function template match — require exactly
6 args and the five verbatim parameter names before returning the native
thunk, so an unrelated dynamic Function body can't be misclassified.
- global_this: ERROR_CONSTRUCTOR_PTR is now thread-local (each agent's realm)
instead of a process-global atomic — a cross-thread Error ctor /
prepareStackTrace pointer could be foreign/freed (same reason globalThis is
per-thread).
- util_inherits: only synthesize .prototype for callable native-module exports
(require('stream')); a plain non-callable object now fails util.inherits as
Node does instead of silently passing.
- rustfmt pass.
proggeramlug
pushed a commit
that referenced
this pull request
Jun 14, 2026
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.
Continues the Next.js standalone-compile validation effort (follow-up to #5050, which landed lazy-require + walls 24-29). Five more walls, each with a minimal repro; verified the standalone
next-serverboots further with each.Walls
Error.prepareStackTrace/.stacktyping.type_analysisno longer types every.stackPropertyGet asString:Error.prepareStackTracecan make.stackan array of CallSites (depd / source-map-support) and a plain object may carry any value — typing itStringcorrupted the array on store. Adds a CallSite shim + honors a user-setError.prepareStackTrace(error.rs,global_this.rs:ERROR_CONSTRUCTOR_PTR+ default-thunk address).util.inherits(X, require('stream')).require('stream')is a callable bound-native with no.prototype; synthesize one soutil.inheritscan chain (util_inherits.rs).new Function(body)(depd). depd builds its deprecation wrapper via string-concatnew Function(...). No JS interpreter: recognize depd's fixed template and return a working native fn (js_function_ctor_from_strings+depd_wrapfunction_outer_thunk); unknown templates still get the placeholder.console[method](...). The computed form collapsed theconsolereceiver to theGlobalGet(0)sentinel (numeric 0 at runtime), soconsole[m](...)threw(number).m is not a function(Next'sprefixedLog). Routeconsole[dynamicKey]tojs_console_method_by_value, the same bound closure a staticconsole.logread produces.nested_fn_declrecordedparam.defaultbut never emitted theif (p === undefined) p = <default>prologue. Since cjs_wrap wraps every module body in an IIFE, every top-levelfunction f(a, opts = {})becomes block-nested and silently dropped its default — brokerecursiveReadDir(dir)→ the wholesetupFsCheck. Mirrorsfn_decl.rs.Validation
cargo build --releaseclean;perry-hirtests pass; targetedperry-runtimetests pass (the GC/stream stress tests are the known parallel-interaction flakes — green in isolation).✓ Ready in 0msandsetupFsCheckwith these applied (next blocker tracked separately as wall 36).Summary by CodeRabbit
new Function(...)using runtime-provided argument strings (limited dynamic body handling).console[key]to resolve and call the correct native method.Error.prepareStackTrace, including lazy.stackgeneration.message/nameare treated as strings (notstack)..prototypesynthesis for missing or nonstandard cases (e.g.,util.inherits).