fix(codegen): class extending a function base must preserve derived identity through constructor-return-object - #5610
Closed
proggeramlug wants to merge 6 commits into
Closed
fix(codegen): class extending a function base must preserve derived identity through constructor-return-object#5610proggeramlug wants to merge 6 commits into
proggeramlug wants to merge 6 commits into
Conversation
added 6 commits
June 23, 2026 06:25
…spatch
Named/value-form imports of node-core native-module functions
(e.g. `import { realpathSync } from "fs"; realpathSync(p)`) reached
codegen as a receiver-less NativeMethodCall with no static dispatch-table
row and fell through to a TAG_UNDEFINED sentinel, returning `undefined`
even though the member form (`mod.fn(p)`) works. The member form routes
through the runtime by-name dispatcher; bridge the value form onto that
same path by synthesizing the module-namespace receiver and dispatching
via js_native_call_method, scoped to modules that own a runtime dispatch
bucket. Fixes the realpathSync class (fs/os/path/url/... named-import
fns); unknown/perry-internal modules keep the historical undefined.
Adds an IR regression test.
…tream receiver `a?.b?.method(args)` and `a?.b?.method?.(args)` threw `TypeError: Cannot read properties of <nullish> (reading 'method')` when the upstream `a?.b` was null/undefined, instead of short-circuiting to undefined. In the OptChain(Call) lowering, when the optional method member's receiver is itself produced by an upstream optional chain it lowers to a Conditional; the inner-Conditional nesting branch reused the un-short-circuited receiver (`a.b`) as the call object without a per-receiver null guard. For the optional-CALL variant the function-value nullish guard additionally read `(a.b).method` off the nullish receiver and threw during guard evaluation. Re-add a receiver-nullish short-circuit (`a.b == null ? undefined : ...`) for the optional-member case in both the plain-call and optional-call branches, gated on a side-effect-free receiver so it never double-evaluates side effects. Fixes the `_?.allowModels?.some(...)` startup wall. Adds a runnable regression test.
…pump + dispatch Request objects The await busy-pump only drains PENDING_RESOLUTIONS via the registered STDLIB_PUMP_FN, which is installed lazily by spawn(). js_fetch_with_options' early-return path (fetch() handed a non-string first arg, e.g. a Request object) queued an 'Invalid URL' rejection WITHOUT a preceding spawn, so the pump stayed unregistered: js_stdlib_has_active_handles reported the pending entry (awaiter keeps waiting) but nothing ever drained it, leaving the awaited Promise pending forever — a hard deadlock with zero observable pending work. 1) queue_promise_resolution / queue_deferred_resolution now call ensure_pump_registered() so any queued resolution is guaranteed to be processed even with no prior spawn. Fixes the deadlock for all callers. 2) js_fetch_with_options now recognizes the fetch(Request) form: when the url slot is a live Request handle, it recovers url/method/body/headers from the Request registry and actually dispatches the request (init members still override). Previously fetch(Request) could only reject. Repro: `const r = await fetch(new Request(url)); // hung; now resolves 200` Gates: fmt clean; perry-runtime lib tests 1074/0; perry-stdlib lib tests 103/0.
…lper
The fetch thunk serialized init.headers via the generic js_json_stringify,
which dereferenced a Headers handle (a fetch-band registry id) as a heap
pointer -> EXC_BAD_ACCESS in gc_obj_type. Add js_fetch_headers_to_json that
reads a Headers handle from its registry instead, and route the
fetch(url, { headers }) path through it. Same handle-band family as the
string_from_header / inline-.length guards.
(Advances the bundle's -p path past the json-stringify crash; a further
handle-band deref remains in js_object_has_property on a Headers handle.)
`key in <handle>` where the receiver is a Web Fetch Headers/Request/Response handle (a fetch-band registry id, e.g. 0x40007) dereferenced the id as a heap object -> EXC_BAD_ACCESS. Return false for handle-band receivers instead, same family as the string_from_header / inline-.length / json_stringify guards.
…dentity through constructor-return-object
A no-own-ctor derived class whose parent resolves to a plain function
value at runtime (`class D extends <fnValue> {}`, captured by HIR as
`extends_expr`) gets an implicit default derived constructor
`constructor(...args){ super(...args) }`. The inline `new` lowering only
emitted that implicit `super(...)` when the parent could be resolved
statically to a class in `ctx.classes` / `imported_class_ctors`. When the
parent is a runtime function value — e.g. a factory-returned constructor
function that itself returns a `this`-aliasing local — none of those
matched, so no `super(...)` was emitted: the parent function body never
ran on the new instance. Its `this.<field> = …` /
`Object.defineProperty(this, …)` writes were lost, and (when the parent
returns its own `this`) the derived instance was left uninitialized.
Fix: in the no-own-ctor inline-`new` path, when no inherited constructor
was found and the class has a dynamic `extends_expr`, resolve the
decl-time-registered parent value (`js_get_dynamic_parent_value`) and
dispatch it on `this` via `js_fetch_or_value_super`, which binds
IMPLICIT_THIS to the instance for the call. This mirrors the existing
synthesized-default-ctor dynamic-parent super in `codegen/method.rs`
(standalone-symbol path) and the explicit `Expr::SuperCall` dynamic-parent
arm in `expr/this_super_call.rs` — the inline path was the one gap.
Verified against a 12-line repro: a class extending a `$constructor`-style
function base now (a) keeps the derived prototype on the constructed
instance and (b) runs the parent function body first, matching Node.
No codegen/HIR test regressions (perry-codegen 128/128, perry-hir 196/196).
📝 WalkthroughWalkthroughThis PR delivers four independent fixes: a safe Changesfetch(Headers) Serialization and fetch(Request) Support
Named-Import Native Module Call Dispatch
Dynamic-Extends super() Fallback
Double Optional-Chain Member-Call Lowering Fix
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over Codegen,js_headers_fetch_object_json: fetch(url, { headers: Headers }) path
end
participant Codegen as FetchWithOptions lowering
participant js_fetch_headers_to_json
participant headers_init_json_ptr
participant js_headers_fetch_object_json
participant HEADERS_REGISTRY
Codegen->>js_fetch_headers_to_json: call(headers_value: f64)
js_fetch_headers_to_json->>headers_init_json_ptr: delegate(headers)
headers_init_json_ptr->>headers_init_json_ptr: addr_class::is_handle_band?
alt handle-band Headers value
headers_init_json_ptr->>js_headers_fetch_object_json: call via GLOBAL_HEADERS_OBJECT_JSON
js_headers_fetch_object_json->>HEADERS_REGISTRY: lookup handle, iterate entries
HEADERS_REGISTRY-->>js_headers_fetch_object_json: deduplicated header map
js_headers_fetch_object_json-->>headers_init_json_ptr: *mut StringHeader (JSON)
else plain heap value
headers_init_json_ptr->>headers_init_json_ptr: js_json_stringify fallback → "{}"
end
headers_init_json_ptr-->>js_fetch_headers_to_json: *const StringHeader
js_fetch_headers_to_json-->>Codegen: i64 pointer
sequenceDiagram
rect rgba(144, 238, 144, 0.5)
Note over lower_native_method_call,js_native_call_method: Named-import native dispatch
end
participant lower_native_method_call
participant NativeModuleRef as NativeModuleRef (synthesized receiver)
participant js_native_call_method
lower_native_method_call->>lower_native_method_call: nm_install_symbol(module).is_some()?
alt module has dispatch bucket
lower_native_method_call->>NativeModuleRef: synthesize namespace receiver
lower_native_method_call->>lower_native_method_call: lower args → F64 array buffer
lower_native_method_call->>lower_native_method_call: intern method name → FFI symbol
lower_native_method_call->>js_native_call_method: call(receiver, method_symbol, args_ptr, args_len)
js_native_call_method-->>lower_native_method_call: runtime result
else no dispatch bucket
lower_native_method_call->>lower_native_method_call: fall through to TAG_UNDEFINED path
end
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)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-codegen/src/lower_call/new.rs (1)
1185-1260: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat
extends_expras heritage before field initialization.Line 1186 ignores dynamic heritage, so
class D extends <expr> { x = ... }can hit the Line 1257SelfOnlypath before the new dynamicsuper(...)call at Line 1699. That reverses derived field order; parent constructor writes can overwrite derived fields.Proposed fix
- let has_extends = class.extends_name.is_some(); + let has_extends = class.extends_name.is_some() || class.extends_expr.is_some();Also applies to: 1668-1708, 1736-1747
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/lower_call/new.rs` around lines 1185 - 1260, The current logic only considers static heritage through class.extends_name when determining field initialization order, ignoring dynamic heritage via extends_expr. This causes classes with dynamic extends like `class D extends <expr> { x = ... }` to apply self-only field initialization before the parent constructor call, allowing parent constructors to overwrite derived fields. Modify the has_extends variable to check both class.extends_name.is_some() and whether class.extends_expr exists (if that field is available on the class structure), and similarly update the inherited_ctor_class logic to handle dynamic extends expressions so field initialization respects the proper order where parent fields are initialized first.crates/perry-runtime/src/object/global_fetch.rs (1)
163-171: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve absent
init.headerssofetch(Request)keeps Request headers.Returning a real
"{}"string for missing headers makesjs_fetch_with_optionsparse an empty map and skip its Request-header fallback. Return null forTAG_UNDEFINED/absent headers, and only stringify when the headers member is actually supplied.Suggested fix
if matches!( headers.to_bits(), - crate::value::TAG_UNDEFINED | crate::value::TAG_NULL + crate::value::TAG_UNDEFINED ) { - return crate::string::js_string_from_bytes(b"{}".as_ptr(), 2); + return std::ptr::null(); + } + if headers.to_bits() == crate::value::TAG_NULL { + return crate::string::js_string_from_bytes(b"{}".as_ptr(), 2); }fn headers_init_json_ptr(headers: f64) -> *const crate::StringHeader { + if headers.to_bits() == crate::value::TAG_UNDEFINED { + return std::ptr::null(); + } let jsv = crate::value::JSValue::from_bits(headers.to_bits());Also applies to: 303-320
🤖 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/global_fetch.rs` around lines 163 - 171, The function currently returns a stringified empty object "{}" when headers are undefined or null, which causes the empty map to be parsed and prevents the fallback logic from using Request headers. Instead of returning the js_string_from_bytes result for TAG_UNDEFINED and TAG_NULL cases, return null to preserve the absent headers state and allow js_fetch_with_options to fall back to Request headers. Only call headers_init_json_ptr to stringify headers when they are actually supplied (neither TAG_UNDEFINED nor TAG_NULL).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/lower_call/native/mod.rs`:
- Around line 2028-2064: The native module fallback logic for nm_install_symbol
should only run for named imports without class qualifiers, but currently it
executes even when class_name is Some(_), causing class-qualified calls to
incorrectly dispatch the top-level module function. Modify the if condition that
checks nm_install_symbol(module).is_some() to also verify that class_name is
None before allowing the js_native_call_method dispatch to execute, ensuring
class-qualified calls preserve their prior miss/undefined behavior when the
proper lookup fails.
In `@crates/perry-codegen/src/lower_call/new.rs`:
- Around line 1671-1675: After the call to js_get_dynamic_parent_value that
assigns to parent_val, add a check to handle the TAG_UNDEFINED case. When
parent_val equals TAG_UNDEFINED, you need to implement a fallback mechanism that
re-evaluates the extends expression instead of passing undefined directly to
js_fetch_or_value_super. Only proceed with the existing js_fetch_or_value_super
logic when parent_val is not TAG_UNDEFINED. This ensures that super(...args)
does not become a no-op when no decl-time parent was stashed.
- Around line 1699-1708: The call to js_fetch_or_value_super is currently
discarding its returned DOUBLE value with let _, which prevents a dynamic
function base that returns an object from replacing the constructed result.
Capture the returned value from the js_fetch_or_value_super call instead of
discarding it, then use this returned value to update both the active this slot
and the ctor_result_slot, mirroring the pattern used in the explicit dynamic
SuperCall path. Apply the same fix to the related location mentioned at lines
1789-1804 that also calls js_fetch_or_value_super.
In `@crates/perry-codegen/tests/native_proof_regressions.rs`:
- Around line 2225-2233: The assertions in the native_proof_regressions test are
checking for the presence of symbol names like "`@js_native_call_method`",
"`@js_nm_install_fs`", and "`@js_create_native_module_namespace`" using simple
contains() checks, which can match declarations or unrelated contexts rather
than actual call instructions. Tighten both assert! blocks by modifying the
string patterns to specifically match the emitted call instruction syntax for
these functions rather than just their bare symbol names, ensuring the test
catches regressions when the code falls back to TAG_UNDEFINED or uses
alternative code paths.
In `@crates/perry-hir/src/lower/lower_expr.rs`:
- Around line 2107-2112: The receiver guard for optional method calls only
applies when the receiver is repeatable, causing non-repeatable receivers with
side effects to bypass short-circuit semantics. In the code block starting at
line 2107 where receiver_for_member_guard is assigned, modify the logic to
capture inner_else in receiver_for_member_guard for both repeatable AND
non-repeatable receivers from an optional chain. For repeatable receivers, clone
inner_else as currently done; for non-repeatable receivers, bind inner_else to a
temporary variable to avoid re-evaluation while still allowing the outer
receiver-nullish guard (around lines 2183-2188) to apply unconditionally,
ensuring short-circuit behavior is preserved. Additionally, add a regression
test case that demonstrates the fix by testing an optional call chain where the
receiver is a function call that returns a nullish value.
In `@crates/perry-runtime/src/object/field_get_set.rs`:
- Around line 2271-2277: The early return of nanbox_false in the handle-band
pointer check that tests obj_val.is_pointer() and is_handle_band bypasses the
handle dispatcher for string keys, causing properties like 'headers' on
handle-band receivers to incorrectly return false. Keep the deref guard
condition, but before returning nanbox_false, check if the key being accessed is
a string and route it through the handle dispatcher first. Only fall back to
returning nanbox_false after attempting to dispatch the string key through the
handle dispatcher or if the key is not a string.
In `@crates/perry-stdlib/src/fetch/mod.rs`:
- Around line 251-279: The RequestFetchFields struct's body field is defined as
Option<String>, and the request_fields_from_handle function converts raw bytes
to UTF-8 string using from_utf8_lossy, which corrupts non-UTF-8 and binary
request bodies. Change the body field type in the RequestFetchFields struct from
Option<String> to Option<Vec<u8>>, and update the mapping logic in
request_fields_from_handle to preserve the raw bytes directly instead of
decoding them to a string by removing the String::from_utf8_lossy call. Apply
the same changes to any other code sections that handle the request body (as
mentioned in the related lines 704-736).
- Around line 266-271: The current implementation in the headers collection
section (where entries are being iterated and cloned into a HashMap) only
preserves the last value for duplicate header names, but HeadersStore::get
combines repeated header names correctly. Replace the direct entries collection
with an approach that uses HeadersStore::get semantics: iterate through the
unique header names from the entries, and for each name, retrieve its value
using the HeadersStore::get method to ensure duplicate headers are properly
combined (typically as comma-separated values) rather than overwritten.
---
Outside diff comments:
In `@crates/perry-codegen/src/lower_call/new.rs`:
- Around line 1185-1260: The current logic only considers static heritage
through class.extends_name when determining field initialization order, ignoring
dynamic heritage via extends_expr. This causes classes with dynamic extends like
`class D extends <expr> { x = ... }` to apply self-only field initialization
before the parent constructor call, allowing parent constructors to overwrite
derived fields. Modify the has_extends variable to check both
class.extends_name.is_some() and whether class.extends_expr exists (if that
field is available on the class structure), and similarly update the
inherited_ctor_class logic to handle dynamic extends expressions so field
initialization respects the proper order where parent fields are initialized
first.
In `@crates/perry-runtime/src/object/global_fetch.rs`:
- Around line 163-171: The function currently returns a stringified empty object
"{}" when headers are undefined or null, which causes the empty map to be parsed
and prevents the fallback logic from using Request headers. Instead of returning
the js_string_from_bytes result for TAG_UNDEFINED and TAG_NULL cases, return
null to preserve the absent headers state and allow js_fetch_with_options to
fall back to Request headers. Only call headers_init_json_ptr to stringify
headers when they are actually supplied (neither TAG_UNDEFINED nor TAG_NULL).
🪄 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: e772c297-a03c-436d-9fac-42dab2e8830f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
crates/perry-codegen/src/expr/logical_collections.rscrates/perry-codegen/src/lower_call/native/mod.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi.rscrates/perry-codegen/tests/native_proof_regressions.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/global_fetch.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/tests.rscrates/perry-stdlib/src/common/async_bridge.rscrates/perry-stdlib/src/common/dispatch.rscrates/perry-stdlib/src/fetch/headers.rscrates/perry-stdlib/src/fetch/mod.rstests/test_optional_chain_double_member_call.sh
Comment on lines
+2028
to
+2064
| if crate::nm_install::nm_install_symbol(module).is_some() { | ||
| let recv_box = | ||
| crate::expr::lower_expr(ctx, &Expr::NativeModuleRef(module.to_string()))?; | ||
| let mut lowered_args: Vec<String> = Vec::with_capacity(args.len()); | ||
| for arg in args { | ||
| lowered_args.push(lower_expr(ctx, arg)?); | ||
| } | ||
| let (args_ptr, args_len) = if lowered_args.is_empty() { | ||
| ("null".to_string(), "0".to_string()) | ||
| } else { | ||
| let n = lowered_args.len(); | ||
| let buf = ctx.func.alloca_entry_array(DOUBLE, n); | ||
| { | ||
| let blk = ctx.block(); | ||
| for (i, value) in lowered_args.iter().enumerate() { | ||
| let slot = blk.gep(DOUBLE, &buf, &[(I64, &i.to_string())]); | ||
| blk.store(DOUBLE, value, &slot); | ||
| } | ||
| } | ||
| (buf, n.to_string()) | ||
| }; | ||
| let method_idx = ctx.strings.intern(method); | ||
| let entry = ctx.strings.entry(method_idx); | ||
| let bytes_global = format!("@{}", entry.bytes_global); | ||
| let name_len = entry.byte_len.to_string(); | ||
| return Ok(ctx.block().call( | ||
| DOUBLE, | ||
| "js_native_call_method", | ||
| &[ | ||
| (DOUBLE, &recv_box), | ||
| (PTR, &bytes_global), | ||
| (I64, &name_len), | ||
| (PTR, &args_ptr), | ||
| (I64, &args_len), | ||
| ], | ||
| )); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep class-qualified calls out of the named-import fallback.
This branch now also runs when class_name is Some(_), but it synthesizes only the top-level module namespace and ignores that qualifier. If native_module_lookup(module, false, method, class_name) misses, a class/subnamespace call with a colliding method name can dispatch the top-level module function instead of preserving the prior miss/undefined behavior. Scope this bridge to the named-import shape.
🐛 Proposed fix
- if crate::nm_install::nm_install_symbol(module).is_some() {
+ if class_name.is_none() && crate::nm_install::nm_install_symbol(module).is_some() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/lower_call/native/mod.rs` around lines 2028 - 2064,
The native module fallback logic for nm_install_symbol should only run for named
imports without class qualifiers, but currently it executes even when class_name
is Some(_), causing class-qualified calls to incorrectly dispatch the top-level
module function. Modify the if condition that checks
nm_install_symbol(module).is_some() to also verify that class_name is None
before allowing the js_native_call_method dispatch to execute, ensuring
class-qualified calls preserve their prior miss/undefined behavior when the
proper lookup fails.
Comment on lines
+1671
to
+1675
| let parent_val = ctx.block().call( | ||
| DOUBLE, | ||
| "js_get_dynamic_parent_value", | ||
| &[(I32, &cid.to_string())], | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Handle the TAG_UNDEFINED parent fallback.
js_get_dynamic_parent_value can return undefined when no decl-time parent was stashed, and the runtime contract expects callers to fall back to re-evaluating the extends expression. This path passes undefined straight into js_fetch_or_value_super, so default super(...args) becomes a no-op in that case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/lower_call/new.rs` around lines 1671 - 1675, After
the call to js_get_dynamic_parent_value that assigns to parent_val, add a check
to handle the TAG_UNDEFINED case. When parent_val equals TAG_UNDEFINED, you need
to implement a fallback mechanism that re-evaluates the extends expression
instead of passing undefined directly to js_fetch_or_value_super. Only proceed
with the existing js_fetch_or_value_super logic when parent_val is not
TAG_UNDEFINED. This ensures that super(...args) does not become a no-op when no
decl-time parent was stashed.
Comment on lines
+1699
to
+1708
| let _ = ctx.block().call( | ||
| DOUBLE, | ||
| "js_fetch_or_value_super", | ||
| &[ | ||
| (DOUBLE, &parent_val), | ||
| (DOUBLE, &this_box), | ||
| (PTR, &args_ptr), | ||
| (I64, &args_len), | ||
| ], | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the dynamic super() result.
Line 1699 discards the DOUBLE returned by js_fetch_or_value_super, so a dynamic function base that returns an object cannot replace the constructed result. Mirror the explicit dynamic SuperCall path here by using the normalized super(...) result to update the active this slot and ctor_result_slot.
Also applies to: 1789-1804
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/lower_call/new.rs` around lines 1699 - 1708, The
call to js_fetch_or_value_super is currently discarding its returned DOUBLE
value with let _, which prevents a dynamic function base that returns an object
from replacing the constructed result. Capture the returned value from the
js_fetch_or_value_super call instead of discarding it, then use this returned
value to update both the active this slot and the ctor_result_slot, mirroring
the pattern used in the explicit dynamic SuperCall path. Apply the same fix to
the related location mentioned at lines 1789-1804 that also calls
js_fetch_or_value_super.
Comment on lines
+2225
to
+2233
| assert!( | ||
| ir.contains("@js_native_call_method"), | ||
| "named-import native fn must route through js_native_call_method:\n{ir}" | ||
| ); | ||
| // It must also install the fs dispatch bucket via the module-namespace | ||
| // receiver synthesis so the method actually resolves at runtime. | ||
| assert!( | ||
| ir.contains("@js_nm_install_fs") || ir.contains("@js_create_native_module_namespace"), | ||
| "fs module namespace receiver must be synthesized:\n{ir}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert call sites instead of symbol declarations.
These bare contains("@...") checks can pass from declarations or unrelated paths, so the regression may not fail if this branch falls back to TAG_UNDEFINED again. Match the emitted call instructions.
💚 Proposed test tightening
assert!(
- ir.contains("`@js_native_call_method`"),
+ ir.contains("call double `@js_native_call_method`"),
"named-import native fn must route through js_native_call_method:\n{ir}"
);
@@
assert!(
- ir.contains("`@js_nm_install_fs`") || ir.contains("`@js_create_native_module_namespace`"),
+ ir.contains("call void `@js_nm_install_fs`")
+ || ir.contains("call double `@js_create_native_module_namespace`"),
"fs module namespace receiver must be synthesized:\n{ir}"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert!( | |
| ir.contains("@js_native_call_method"), | |
| "named-import native fn must route through js_native_call_method:\n{ir}" | |
| ); | |
| // It must also install the fs dispatch bucket via the module-namespace | |
| // receiver synthesis so the method actually resolves at runtime. | |
| assert!( | |
| ir.contains("@js_nm_install_fs") || ir.contains("@js_create_native_module_namespace"), | |
| "fs module namespace receiver must be synthesized:\n{ir}" | |
| assert!( | |
| ir.contains("call double `@js_native_call_method`"), | |
| "named-import native fn must route through js_native_call_method:\n{ir}" | |
| ); | |
| // It must also install the fs dispatch bucket via the module-namespace | |
| // receiver synthesis so the method actually resolves at runtime. | |
| assert!( | |
| ir.contains("call void `@js_nm_install_fs`") | |
| || ir.contains("call double `@js_create_native_module_namespace`"), | |
| "fs module namespace receiver must be synthesized:\n{ir}" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/tests/native_proof_regressions.rs` around lines 2225 -
2233, The assertions in the native_proof_regressions test are checking for the
presence of symbol names like "`@js_native_call_method`", "`@js_nm_install_fs`", and
"`@js_create_native_module_namespace`" using simple contains() checks, which can
match declarations or unrelated contexts rather than actual call instructions.
Tighten both assert! blocks by modifying the string patterns to specifically
match the emitted call instruction syntax for these functions rather than just
their bare symbol names, ensuring the test catches regressions when the code
falls back to TAG_UNDEFINED or uses alternative code paths.
Comment on lines
+2107
to
+2112
| let receiver_for_member_guard = | ||
| if opt_member_chain && opt_call_receiver_repeatable(&inner_else) { | ||
| Some(inner_else.as_ref().clone()) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cd crates/perry-hir && wc -l src/lower/lower_expr.rsRepository: PerryTS/perry
Length of output: 180
🏁 Script executed:
cd crates/perry-hir && sed -n '2100,2120p' src/lower/lower_expr.rsRepository: PerryTS/perry
Length of output: 1557
🏁 Script executed:
cd crates/perry-hir && sed -n '2165,2205p' src/lower/lower_expr.rsRepository: PerryTS/perry
Length of output: 2707
🏁 Script executed:
cd crates/perry-hir && ast-grep outline src/lower/lower_expr.rs --view expanded | grep -A 5 -B 5 "opt_call_receiver_repeatable"Repository: PerryTS/perry
Length of output: 890
🏁 Script executed:
cd crates/perry-hir && sed -n '241,273p' src/lower/lower_expr.rsRepository: PerryTS/perry
Length of output: 1806
🏁 Script executed:
cd crates/perry-hir && sed -n '2050,2150p' src/lower/lower_expr.rsRepository: PerryTS/perry
Length of output: 6596
🏁 Script executed:
cd crates/perry-hir && sed -n '2140,2210p' src/lower/lower_expr.rsRepository: PerryTS/perry
Length of output: 4696
🏁 Script executed:
cd crates/perry-hir && sed -n '274,295p' src/lower/lower_expr.rsRepository: PerryTS/perry
Length of output: 836
🏁 Script executed:
cd crates/perry-hir && rg -A 10 "opt_call_func_nullish_guard" src/lower/lower_expr.rs | head -30Repository: PerryTS/perry
Length of output: 1766
🏁 Script executed:
cat > /tmp/analyze_flow.py << 'EOF'
# Trace the control flow for the scenario getCfg()?.allowModels?.some(...)
# where getCfg() is not repeatable
print("Scenario: getCfg()?.allowModels?.some(...)")
print("=" * 60)
print("\n1. Processing getCfg()?.allowModels:")
print(" - Creates a Conditional (inner_cond=true if getCfg() is nullish)")
print(" - inner_then = Undefined (short-circuits)")
print(" - inner_else = getCfg().allowModels (not repeatable - contains function call)")
print("\n2. Processing ?.some(...) on the result:")
print(" - check_expr = Conditional from step 1")
print(" - opt_member_chain = true (? on .some)")
print(" - opt_call_receiver_repeatable(inner_else) = false (contains getCfg() call)")
print(" - receiver_for_member_guard = None")
print("\n3. For optional call ?.some?.():")
print(" - Enters callee_from_chain branch (line 2161-2189)")
print(" - Creates guard_cond = opt_call_func_nullish_guard(recv, fixed_callee)")
print(" - fixed_callee = PropertyGet { object: inner_else, property: 'some' }")
print(" - ISSUE: guard_cond evaluates PropertyGet on potentially null inner_else!")
print(" - This throws 'Cannot read properties' BEFORE short-circuiting")
print("\n4. Why receiver_for_member_guard = None prevents the fix:")
print(" - receiver_for_member_guard is None (not repeatable)")
print(" - So the outer guard (line 2183-2188) is skipped")
print(" - No wrapper around guarded_call to null-check receiver first")
print(" - guarded_call's guard itself tries to access the property, which throws")
print("\nConclusion: The review comment is CORRECT.")
print("When receiver is non-repeatable, the function-value guard throws")
print("instead of short-circuiting to undefined.")
EOF
python3 /tmp/analyze_flow.pyRepository: PerryTS/perry
Length of output: 1567
🏁 Script executed:
cd crates/perry-hir && rg -B 5 -A 10 "call-produced" src/ || rg -B 5 -A 10 "getCfg\|non.*repeatable.*receiver" tests/ || echo "Searching for regression tests..."Repository: PerryTS/perry
Length of output: 185
🏁 Script executed:
cd crates/perry-hir && find . -name "*.rs" -type f -exec grep -l "test.*optional.*chain\|regression" {} \; | head -5Repository: PerryTS/perry
Length of output: 455
🏁 Script executed:
cat > /tmp/verify_guard.sh << 'EOF'
cd crates/perry-hir
# Check the exact guard evaluation order in the optional-call case
echo "=== Guard evaluation in optional-call branch (callee_from_chain=true) ==="
sed -n '2163,2189p' src/lower/lower_expr.rs | grep -A 30 "let guard_cond"
echo -e "\n=== Inner PropertyGet evaluation in fixed_callee ==="
sed -n '2120,2135p' src/lower/lower_expr.rs | grep -B 5 -A 5 "fixed_callee"
echo -e "\n=== Issue: When receiver_for_member_guard is None ==="
sed -n '2107,2112p' src/lower/lower_expr.rs
sed -n '2175,2190p' src/lower/lower_expr.rs
EOF
bash /tmp/verify_guard.shRepository: PerryTS/perry
Length of output: 2564
🏁 Script executed:
cd crates/perry-hir && sed -n '2150,2210p' src/lower/lower_expr.rs | cat -nRepository: PerryTS/perry
Length of output: 4513
🏁 Script executed:
cd crates/perry-hir && grep -B 3 -A 15 "callee_from_chain.*false" src/lower/lower_expr.rs | head -40Repository: PerryTS/perry
Length of output: 3042
🏁 Script executed:
cd crates/perry-hir && cat > /tmp/final_verify.py << 'EOF'
# Verify the exact scenario with optional call and non-repeatable receiver
code_flow = """
getCfg()?.allowModels?.some(...)
Inner chain: getCfg()?.allowModels
- Creates: Conditional {
condition: getCfg() == null,
then: undefined,
else: getCfg().allowModels
}
Processing ?.some(...):
- check_expr = above Conditional
- opt_member_chain = true (? on .some)
- opt_call_receiver_repeatable(getCfg().allowModels) = false
- receiver_for_member_guard = None
For optional call ?.some?.():
- callee_from_chain = true
- opt_call_member_receiver = getCfg (the original object before PropertyGet)
- fixed_callee = PropertyGet{getCfg().allowModels, 'some'}
- guard_cond = opt_call_func_nullish_guard(getCfg, fixed_callee)
- Since receiver not repeatable: guard = PropertyGet{getCfg().allowModels, 'some'} == null
THE BUG:
- Evaluating guard_cond requires evaluating PropertyGet{getCfg().allowModels, 'some'}
- But getCfg().allowModels could be null/undefined (inner_else when inner_then taken)
- PropertyGet on null throws BEFORE guard expression completes
- No outer receiver guard (receiver_for_member_guard is None) to catch this
CURRENT CODE (lines 2161-2189):
if callee_from_chain {
let guard_cond = opt_call_func_nullish_guard(recv, fixed_callee);
let guarded_call = Conditional { guard_cond, then: Undefined, else: outer_call };
match receiver_for_member_guard {
Some(recv) => {
// Outer guard that prevents the throw
Conditional { receiver == null, then: Undefined, else: guarded_call }
},
None => guarded_call // <- BUG: skips outer guard for non-repeatable
}
}
"""
print(code_flow)
EOF
python3 /tmp/final_verify.pyRepository: PerryTS/perry
Length of output: 1706
🏁 Script executed:
cd crates/perry-hir && sed -n '2113,2125p' src/lower/lower_expr.rsRepository: PerryTS/perry
Length of output: 850
Add receiver guard for optional method calls with non-repeatable receivers to prevent throwing instead of short-circuiting.
In the optional-call branch (?.method?.(args)), when the receiver comes from an upstream optional chain (inner_else from a Conditional) and the receiver is non-repeatable (contains side effects like function calls), the function-value guard evaluates a PropertyGet on a potentially null/undefined receiver, causing a "Cannot read properties" throw instead of short-circuiting to undefined. This breaks chains like getCfg()?.allowModels?.some(...) where getCfg() returns a nullish value.
The outer receiver-nullish guard (lines 2183-2188) correctly prevents this for repeatable receivers but is skipped when receiver_for_member_guard is None. For non-repeatable receivers, bind inner_else once to a temporary and apply the receiver guard unconditionally in the optional-call branch, avoiding double-evaluation of side effects while ensuring short-circuit semantics are preserved. Add a regression test with a call-produced receiver.
🤖 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/lower_expr.rs` around lines 2107 - 2112, The
receiver guard for optional method calls only applies when the receiver is
repeatable, causing non-repeatable receivers with side effects to bypass
short-circuit semantics. In the code block starting at line 2107 where
receiver_for_member_guard is assigned, modify the logic to capture inner_else in
receiver_for_member_guard for both repeatable AND non-repeatable receivers from
an optional chain. For repeatable receivers, clone inner_else as currently done;
for non-repeatable receivers, bind inner_else to a temporary variable to avoid
re-evaluation while still allowing the outer receiver-nullish guard (around
lines 2183-2188) to apply unconditionally, ensuring short-circuit behavior is
preserved. Additionally, add a regression test case that demonstrates the fix by
testing an optional call chain where the receiver is a function call that
returns a nullish value.
Comment on lines
+2271
to
+2277
| if obj_val.is_pointer() | ||
| && crate::value::addr_class::is_handle_band( | ||
| (obj_val.bits() & crate::value::POINTER_MASK) as usize, | ||
| ) | ||
| { | ||
| return nanbox_false; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Route handle-band string keys through the handle dispatcher before returning false.
This early return makes the existing small-handle property-dispatch path below unreachable for all pointer-tagged handle-band receivers, so checks like "headers" in request or other native-handle properties can incorrectly return false. Keep the deref guard, but dispatch string keys here before falling back.
Suggested direction
- if obj_val.is_pointer()
- && crate::value::addr_class::is_handle_band(
- (obj_val.bits() & crate::value::POINTER_MASK) as usize,
- )
- {
- return nanbox_false;
- }
+ if obj_val.is_pointer() {
+ let addr = (obj_val.bits() & crate::value::POINTER_MASK) as usize;
+ if crate::value::addr_class::is_handle_band(addr) {
+ if key_val.is_any_string() {
+ unsafe {
+ if let Some(dispatch) = super::class_registry::handle_property_dispatch() {
+ let key_ptr = crate::value::js_get_string_pointer_unified(key)
+ as *const crate::StringHeader;
+ let name_ptr =
+ (key_ptr as *const u8).add(std::mem::size_of::<crate::StringHeader>());
+ let name_len = (*key_ptr).byte_len as usize;
+ let result = dispatch(addr as i64, name_ptr, name_len);
+ if result.to_bits() != crate::value::TAG_UNDEFINED {
+ return nanbox_true;
+ }
+ }
+ }
+ }
+ return nanbox_false;
+ }
+ }🤖 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/field_get_set.rs` around lines 2271 - 2277,
The early return of nanbox_false in the handle-band pointer check that tests
obj_val.is_pointer() and is_handle_band bypasses the handle dispatcher for
string keys, causing properties like 'headers' on handle-band receivers to
incorrectly return false. Keep the deref guard condition, but before returning
nanbox_false, check if the key being accessed is a string and route it through
the handle dispatcher first. Only fall back to returning nanbox_false after
attempting to dispatch the string key through the handle dispatcher or if the
key is not a string.
Comment on lines
+251
to
+279
| /// Fields recovered from a `Request` object for the `fetch(Request)` form. | ||
| struct RequestFetchFields { | ||
| url: String, | ||
| method: String, | ||
| body: Option<String>, | ||
| headers: HashMap<String, String>, | ||
| } | ||
|
|
||
| /// When `fetch()` is handed a `Request` object, its handle id lands in the | ||
| /// `url_ptr` slot. Recover the url/method/body/headers from the Request | ||
| /// registry so the request can actually be dispatched. Returns `None` when the | ||
| /// id isn't a live Request handle (e.g. a genuinely bad/undefined first arg). | ||
| fn request_fields_from_handle(maybe_handle: usize) -> Option<RequestFetchFields> { | ||
| let guard = REQUEST_REGISTRY.lock().unwrap(); | ||
| let req = guard.get(&maybe_handle)?; | ||
| let headers = req | ||
| .headers | ||
| .entries | ||
| .iter() | ||
| .map(|(k, v)| (k.clone(), v.clone())) | ||
| .collect(); | ||
| Some(RequestFetchFields { | ||
| url: req.url.clone(), | ||
| method: req.method.clone(), | ||
| body: req | ||
| .body | ||
| .as_ref() | ||
| .map(|b| String::from_utf8_lossy(b).into_owned()), | ||
| headers, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve Request body bytes through fetch(Request).
RequestRecord.body is stored as raw bytes, but this recovery decodes it with from_utf8_lossy and later sends a string, corrupting non-UTF-8/binary request bodies. Keep the recovered body as Vec<u8> and convert only init string bodies to bytes.
Suggested fix
struct RequestFetchFields {
url: String,
method: String,
- body: Option<String>,
+ body: Option<Vec<u8>>,
headers: HashMap<String, String>,
}- body: req
- .body
- .as_ref()
- .map(|b| String::from_utf8_lossy(b).into_owned()),
+ body: req.body.clone(),- let body = string_from_header(body_ptr)
- .or_else(|| request_fields.as_ref().and_then(|rf| rf.body.clone()));
+ let body = string_from_header(body_ptr)
+ .map(String::into_bytes)
+ .or_else(|| request_fields.as_ref().and_then(|rf| rf.body.clone()));Also applies to: 704-736
🤖 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-stdlib/src/fetch/mod.rs` around lines 251 - 279, The
RequestFetchFields struct's body field is defined as Option<String>, and the
request_fields_from_handle function converts raw bytes to UTF-8 string using
from_utf8_lossy, which corrupts non-UTF-8 and binary request bodies. Change the
body field type in the RequestFetchFields struct from Option<String> to
Option<Vec<u8>>, and update the mapping logic in request_fields_from_handle to
preserve the raw bytes directly instead of decoding them to a string by removing
the String::from_utf8_lossy call. Apply the same changes to any other code
sections that handle the request body (as mentioned in the related lines
704-736).
Comment on lines
+266
to
+271
| let headers = req | ||
| .headers | ||
| .entries | ||
| .iter() | ||
| .map(|(k, v)| (k.clone(), v.clone())) | ||
| .collect(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use HeadersStore::get semantics when recovering Request headers.
Collecting entries directly into a HashMap keeps only the last duplicate header value, while the Headers JSON path combines repeated names via HeadersStore::get. This can send different headers for fetch(request) than for fetch(url, { headers: request.headers }).
Suggested fix
- let headers = req
- .headers
- .entries
- .iter()
- .map(|(k, v)| (k.clone(), v.clone()))
- .collect();
+ let mut headers = HashMap::new();
+ for (k, _) in &req.headers.entries {
+ if headers.contains_key(k) {
+ continue;
+ }
+ if let Some(v) = req.headers.get(k) {
+ headers.insert(k.clone(), v);
+ }
+ }🤖 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-stdlib/src/fetch/mod.rs` around lines 266 - 271, The current
implementation in the headers collection section (where entries are being
iterated and cloned into a HashMap) only preserves the last value for duplicate
header names, but HeadersStore::get combines repeated header names correctly.
Replace the direct entries collection with an approach that uses
HeadersStore::get semantics: iterate through the unique header names from the
entries, and for each name, retrieve its value using the HeadersStore::get
method to ensure duplicate headers are properly combined (typically as
comma-separated values) rather than overwritten.
Contributor
Author
|
Superseded by a clean single-commit re-cut (this PR's head was build/latest, carrying the whole stack + showing CONFLICTING). Re-opened cleanly: see the new PR for fix/class-extends-fn-base. |
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.
Problem
When a class with no own constructor extends a parent that resolves to a plain function value at runtime (
class D extends <fnValue> {}, captured by HIR asextends_expr),new D()did not run the parent function body on the new instance.JS spec: such a class gets an implicit default derived ctor
constructor(...args){ super(...args) }. The inlinenewlowering only emitted that implicitsuper(...)when the parent could be resolved statically to a class inctx.classes/imported_class_ctors. When the parent is a runtime function value — e.g. a factory-returned constructor function that itself returns athis-aliasing local — none of those matched, so nosuper(...)was emitted. The parent function body never ran on the derived instance: itsthis.<field> = …/Object.defineProperty(this, …)writes were lost, and (when the parent returns its ownthis) the derived instance was left uninitialized.This is the exact shape of a popular schema library's
$constructorpattern (a class extends another$constructor-returned function used as the heritage), which produced instances missing their inherited setup.Fix
In the no-own-ctor inline-
newpath (crates/perry-codegen/src/lower_call/new.rs), when no inherited constructor was found and the class has a dynamicextends_expr, resolve the decl-time-registered parent value (js_get_dynamic_parent_value) and dispatch it onthisviajs_fetch_or_value_super, which binds IMPLICIT_THIS to the instance for the duration of the call.This mirrors the existing dynamic-parent super handling that already lives in:
codegen/method.rs— the synthesized-default-ctor / standalone-symbol pathexpr/this_super_call.rs— the explicitExpr::SuperCalldynamic-parent armThe inline
newpath was the one place missing it.Verification (12-line repro, perry vs node)
A class extending a
$constructor-style function base now:new Child()is a Child, not a Parent), andBoth signals now match Node where before they diverged. The simple non-nested cases (
class D extends FnThatReturnsThis {}, directnew D()) continue to pass — no regression.Gates
cargo checkcleancargo fmt --checkcleancargo test -p perry-codegen -p perry-hir --lib: 128/128 and 196/196 passperry-runtime --lib: 3 pre-existing GC-mark-phase / dynamic-prop test failures are baseline (reproduce on a clean tree without this change; this PR touches codegen only)Summary by CodeRabbit
New Features
fetch()with Headers object initializationBug Fixes
Tests