fix(runtime,codegen): #5907 — class X extends Promise subclass support - #5991
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughThis PR adds compile-time and runtime support for ChangesPromise Subclass Support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/new.rs`:
- Around line 1317-1320: The `promise_parent_runtime` check in `new.rs` only
looks at the direct superclass, so `Leaf extends Mid` where `Mid extends
Promise` is missed and `emit_promise_subclass_init` never runs. Update the logic
around `promise_parent_runtime` to walk the class ancestry the same way the
Map/Set and stream helper paths do, so any Promise-descended class without its
own/imported constructor is recognized. Keep the existing `has_own_ctor` and
`has_imported_ctor` gating, but determine the Promise parent status from the
full inheritance chain before deciding whether to emit the hidden backing
promise init.
In `@crates/perry-runtime/src/value/dynamic_object.rs`:
- Around line 280-291: The Promise-subclass static property lookup path is
constructing `class_ref_obj` from the full tagged `obj_value` bits instead of
the extracted class ID, which can send `js_object_get_field_by_name_f64` down
the wrong path. In `dynamic_object.rs`, update the
`class_ref_id(obj_value).is_some()` branch so the lookup uses the bare
`class_id` returned by `class_ref_id` when calling
`js_object_get_field_by_name_f64`, rather than `obj_value.to_bits()`, ensuring
`Subclass.resolve`/`NewPromiseCapability` reads stay on the intended fast path
and do not dereference a bogus pointer.
🪄 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: 5e98d0d8-72f0-4467-8ff8-b9c8d8d7e9fe
📒 Files selected for processing (16)
crates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_call/new_helpers.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi/streams_events.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/class_registry/class_meta.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/class_registry/parent_static.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/promise/checked_dispatch.rscrates/perry-runtime/src/promise/mod.rscrates/perry-runtime/src/promise/spec_combinators.rscrates/perry-runtime/src/promise/subclass.rscrates/perry-runtime/src/promise/then.rscrates/perry-runtime/src/value/dynamic_object.rs
| // `class X extends Promise {}` with no own ctor — `new X(executor)` runs the | ||
| // Promise constructor against a hidden backing cell (see new_helpers). | ||
| let promise_parent_runtime = | ||
| !has_own_ctor && !has_imported_ctor && class.extends_name.as_deref() == Some("Promise"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== new.rs around promise_parent_runtime =="
sed -n '1290,1345p' crates/perry-codegen/src/lower_call/new.rs
echo
sed -n '1645,1675p' crates/perry-codegen/src/lower_call/new.rs
echo
sed -n '1900,1935p' crates/perry-codegen/src/lower_call/new.rs
echo "== new_helpers.rs relevant helpers =="
sed -n '300,430p' crates/perry-codegen/src/lower_call/new_helpers.rs
echo "== search for promise_parent_in_chain =="
rg -n "promise_parent_in_chain|promise_parent_runtime|map_set_default_super_kind|node_stream_parent_kind" crates/perry-codegen/src -SRepository: PerryTS/perry
Length of output: 11503
Walk Promise ancestors before setting promise_parent_runtime. class Leaf extends Mid with Mid extends Promise and no constructors is treated as non-Promise here, so emit_promise_subclass_init never runs and new Leaf(executor) misses the hidden backing promise. Match the ancestor walk used by the Map/Set and stream helpers.
🤖 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 1317 - 1320, The
`promise_parent_runtime` check in `new.rs` only looks at the direct superclass,
so `Leaf extends Mid` where `Mid extends Promise` is missed and
`emit_promise_subclass_init` never runs. Update the logic around
`promise_parent_runtime` to walk the class ancestry the same way the Map/Set and
stream helper paths do, so any Promise-descended class without its own/imported
constructor is recognized. Keep the existing `has_own_ctor` and
`has_imported_ctor` gating, but determine the Promise parent status from the
full inheritance chain before deciding whether to emit the hidden backing
promise init.
| if crate::object::class_ref_id(obj_value).is_some() { | ||
| let name_slice = if property_name_ptr.is_null() { | ||
| return f64::from_bits(TAG_UNDEFINED); | ||
| } else if property_name_len > 0 { | ||
| std::slice::from_raw_parts(property_name_ptr as *const u8, property_name_len) | ||
| } else { | ||
| std::ffi::CStr::from_ptr(property_name_ptr as *const std::ffi::c_char).to_bytes() | ||
| }; | ||
| let key = crate::string::js_string_from_bytes(name_slice.as_ptr(), name_slice.len() as u32); | ||
| let class_ref_obj = obj_value.to_bits() as *const crate::object::ObjectHeader; | ||
| return crate::object::js_object_get_field_by_name_f64(class_ref_obj, key); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Casts the full tagged bits instead of the extracted class_id, likely causing a crash on Promise-subclass static property reads.
class_ref_id() already extracts the bare class_id (bits & 0xFFFF_FFFF), but class_ref_obj is built from obj_value.to_bits() — the full 0x7FFE_...-tagged value. js_object_get_field_by_name_f64's fast path only recognizes small values (< 0x10000); a large bogus "pointer" here falls through to js_object_get_field_by_name, which will dereference it as a real object pointer. This is exactly the code path exercised by Subclass.resolve / NewPromiseCapability(Subclass)-style reads that this branch was added to support.
🐛 Proposed fix — use the extracted `class_id`
- if crate::object::class_ref_id(obj_value).is_some() {
+ if let Some(class_id) = crate::object::class_ref_id(obj_value) {
let name_slice = if property_name_ptr.is_null() {
return f64::from_bits(TAG_UNDEFINED);
} else if property_name_len > 0 {
std::slice::from_raw_parts(property_name_ptr as *const u8, property_name_len)
} else {
std::ffi::CStr::from_ptr(property_name_ptr as *const std::ffi::c_char).to_bytes()
};
let key = crate::string::js_string_from_bytes(name_slice.as_ptr(), name_slice.len() as u32);
- let class_ref_obj = obj_value.to_bits() as *const crate::object::ObjectHeader;
+ let class_ref_obj = class_id as usize as *const crate::object::ObjectHeader;
return crate::object::js_object_get_field_by_name_f64(class_ref_obj, key);
}📝 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.
| if crate::object::class_ref_id(obj_value).is_some() { | |
| let name_slice = if property_name_ptr.is_null() { | |
| return f64::from_bits(TAG_UNDEFINED); | |
| } else if property_name_len > 0 { | |
| std::slice::from_raw_parts(property_name_ptr as *const u8, property_name_len) | |
| } else { | |
| std::ffi::CStr::from_ptr(property_name_ptr as *const std::ffi::c_char).to_bytes() | |
| }; | |
| let key = crate::string::js_string_from_bytes(name_slice.as_ptr(), name_slice.len() as u32); | |
| let class_ref_obj = obj_value.to_bits() as *const crate::object::ObjectHeader; | |
| return crate::object::js_object_get_field_by_name_f64(class_ref_obj, key); | |
| } | |
| if let Some(class_id) = crate::object::class_ref_id(obj_value) { | |
| let name_slice = if property_name_ptr.is_null() { | |
| return f64::from_bits(TAG_UNDEFINED); | |
| } else if property_name_len > 0 { | |
| std::slice::from_raw_parts(property_name_ptr as *const u8, property_name_len) | |
| } else { | |
| std::ffi::CStr::from_ptr(property_name_ptr as *const std::ffi::c_char).to_bytes() | |
| }; | |
| let key = crate::string::js_string_from_bytes(name_slice.as_ptr(), name_slice.len() as u32); | |
| let class_ref_obj = class_id as usize as *const crate::object::ObjectHeader; | |
| return crate::object::js_object_get_field_by_name_f64(class_ref_obj, key); | |
| } |
🤖 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/value/dynamic_object.rs` around lines 280 - 291, The
Promise-subclass static property lookup path is constructing `class_ref_obj`
from the full tagged `obj_value` bits instead of the extracted class ID, which
can send `js_object_get_field_by_name_f64` down the wrong path. In
`dynamic_object.rs`, update the `class_ref_id(obj_value).is_some()` branch so
the lookup uses the bare `class_id` returned by `class_ref_id` when calling
`js_object_get_field_by_name_f64`, rather than `obj_value.to_bits()`, ensuring
`Subclass.resolve`/`NewPromiseCapability` reads stay on the intended fast path
and do not dereference a bogus pointer.
super(executor) now runs the Promise constructor against a hidden backing cell stashed on the subclass instance; inherited then/catch/finally, NewPromiseCapability(Subclass), and inherited builtin statics (Subclass.resolve/all/race/...) all resolve. Fixes the Promise combinator ctx-ctor + invoke-resolve-every-iteration test262 clusters.
d186798 to
7c5fd81
Compare
…5991 preserved) The #5983 squash unknowingly re-landed PR #5874 (Improve Zod compatibility, 849f297): #5874 had merged to main and been force- rewound, but the pump branch rebased inside that window, retained the commit, and the squash re-introduced all 161 files — including the 'constv'/'direct' binaries and the class-lowering change that regresses temporal_subclass_capture_writeback_inner_class ('class X extends <param>' flips from the static New path to ClassExprFresh/NewDynamic and the capture writeback is lost). This is the inverse patch of that payload applied to CURRENT main (rather than a snapshot restore), so the five fleet merges that landed meanwhile are preserved — notably #5991 (extends Promise), which built on top of #5874's new.rs/new_helpers split: its promise_parent_runtime scaffold, emit_promise_subclass_init wiring, and new_helpers additions are kept (verified: subclass executor resolves and .then sees the value, matching node); #5874's map_set_default_super_kind scaffold goes with the rest. Verified: issue_5587_temporal_subclass 6/6 (was 5/6 on main), issue_806 suite unchanged, cargo test -p perry-stdlib links (the intended #5983 pump-lockstep line is untouched). #5874 remains in history for its author to re-land through review after fixing the extends regression.
…essions A test262 sweep of main at the #6008 tip regressed vs the #5979 tip (parity 96.5% -> 95.8%, Temporal self-validated 99.1% -> 96.7%, 322 tests newly failing). git bisect (good 28948ea .. bad 76cc2b9, witness = built-ins/Boolean + Number + Temporal.Duration parity) lands on 849f297 "Improve Zod compatibility support" (#5874) as the first bad commit: 98.3% witness parity at its parent-2 (4f5bc6a) drops to 93.9% at #5874 (Boolean 89%->67%, Temporal 100%->95%, self-validated 100%->95.1%). #5874 was a 156-file monolithic squash that swept in unreviewed codegen/runtime WIP + two 5.1MB binaries (constv, direct) + ~30 stray tests/*.sh alongside the intended Zod work. Two swept changes dominate: - codegen/lower_call/property_get/number_string.rs gated the universal .toString() interception on is_numeric_expr || is_bigint_expr, so boxed built-in receivers (new String()/Number()/Date()) with a user-assigned built-in method bypassed the brand-check and stopped throwing TypeError (Boolean S15.6.4.2_A2_T1..T3) -> the "method is not a function" + SameValue clusters. - the runtime prototype-identity / class-registry / instanceof rewrite regressed Temporal subclass prototype resolution (Object.getPrototypeOf(result) === construct.prototype) -> the Temporal 159 + "(no output)" crash clusters. This is the inverse patch of the #5874 payload applied to CURRENT main (not a snapshot restore), so the later fleet merges are preserved: - #5991 (class X extends Promise): its promise_parent_runtime scaffold, emit_promise_subclass_init wiring, and new_helpers additions are kept (subclass executor resolves and .then sees the value). - #5999 (js_dynamic_mod fmod sign-of-zero): kept; only #5874s dynamic_number_operand double-coercion was dropped from the modulo path. - #5983 (external-http-client-pump dropped from stdlib full): untouched and links green. new.rs / new_helpers.rs: reverting new.rs to its pre-#5874 form put it at 2000 lines; applying #5991s +9 net lines on top pushed it to 2009, over the 2000-line file-size gate. Moved three self-contained ctor predicate helpers (effective_constructor_param_count, local_constructor_symbol_exists, ctor_chain_uses_new_target) into the new_helpers sibling to restore the split (new.rs -> 1939 lines). Verified on an internal Linux sweep host: witness slice back to 98.3% / self-validated 100% / Temporal.Duration 100%; fmt + file-size + gc-store-site + addr-class audits clean. #5874 remains in history for its author to re-land in reviewable pieces.
…main (preserves 5 later fixes) The #5983 squash unknowingly re-landed force-rewound PR #5874 (Improve Zod compatibility, 849f297, 161 files incl the constv/direct binaries and a class-lowering change that regresses temporal_subclass_capture_writeback_inner_class: 'class X extends <param>' flips from static New to ClassExprFresh/NewDynamic and the capture writeback is lost). Inverse patch of the #5874 payload applied to CURRENT main via 3-way, so the fleet merges that landed on top are preserved: - #5991 (extends Promise): new.rs/new_helpers.rs taken from the prior working revert (unchanged by the 11 intervening merges); its promise_parent_runtime + emit_promise_subclass_init kept, #5874's map_set_default_super_kind scaffold dropped. - #5999 (js_dynamic_mod fmod sign-of-zero): #5874's redundant dynamic_number_operand insertion removed, #5999's 'a % b' kept. - #6004/#6005/#6007 (exotic-receiver dispatch, proxy get, seal on functions): 3-way removed ONLY #5874's hunks from native_call_method.rs/proxy.rs; the later fixes' additions stay. Verified: temporal 6/6 (was 5/6 on main), 806 unchanged, stdlib links, proxy e2e green, and behavioral probes for #5999 (mod -0), #5991 (promise subclass = 42), #6004/#6007 (exotic dispatch + fn seal) all match node. Both stray binaries deleted. #5874 stays in history at 4500b5d for its author to re-land through review after fixing the extends regression.
…essions (#6015) A test262 sweep of main at the #6008 tip regressed vs the #5979 tip (parity 96.5% -> 95.8%, Temporal self-validated 99.1% -> 96.7%, 322 tests newly failing). git bisect (good 28948ea .. bad 76cc2b9, witness = built-ins/Boolean + Number + Temporal.Duration parity) lands on 849f297 "Improve Zod compatibility support" (#5874) as the first bad commit: 98.3% witness parity at its parent-2 (4f5bc6a) drops to 93.9% at #5874 (Boolean 89%->67%, Temporal 100%->95%, self-validated 100%->95.1%). #5874 was a 156-file monolithic squash that swept in unreviewed codegen/runtime WIP + two 5.1MB binaries (constv, direct) + ~30 stray tests/*.sh alongside the intended Zod work. Two swept changes dominate: - codegen/lower_call/property_get/number_string.rs gated the universal .toString() interception on is_numeric_expr || is_bigint_expr, so boxed built-in receivers (new String()/Number()/Date()) with a user-assigned built-in method bypassed the brand-check and stopped throwing TypeError (Boolean S15.6.4.2_A2_T1..T3) -> the "method is not a function" + SameValue clusters. - the runtime prototype-identity / class-registry / instanceof rewrite regressed Temporal subclass prototype resolution (Object.getPrototypeOf(result) === construct.prototype) -> the Temporal 159 + "(no output)" crash clusters. This is the inverse patch of the #5874 payload applied to CURRENT main (not a snapshot restore), so the later fleet merges are preserved: - #5991 (class X extends Promise): its promise_parent_runtime scaffold, emit_promise_subclass_init wiring, and new_helpers additions are kept (subclass executor resolves and .then sees the value). - #5999 (js_dynamic_mod fmod sign-of-zero): kept; only #5874s dynamic_number_operand double-coercion was dropped from the modulo path. - #5983 (external-http-client-pump dropped from stdlib full): untouched and links green. new.rs / new_helpers.rs: reverting new.rs to its pre-#5874 form put it at 2000 lines; applying #5991s +9 net lines on top pushed it to 2009, over the 2000-line file-size gate. Moved three self-contained ctor predicate helpers (effective_constructor_param_count, local_constructor_symbol_exists, ctor_chain_uses_new_target) into the new_helpers sibling to restore the split (new.rs -> 1939 lines). Verified on an internal Linux sweep host: witness slice back to 98.3% / self-validated 100% / Temporal.Duration 100%; fmt + file-size + gc-store-site + addr-class audits clean. #5874 remains in history for its author to re-land in reviewable pieces. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
…main (preserves 5 later fixes) The #5983 squash unknowingly re-landed force-rewound PR #5874 (Improve Zod compatibility, 849f297, 161 files incl the constv/direct binaries and a class-lowering change that regresses temporal_subclass_capture_writeback_inner_class: 'class X extends <param>' flips from static New to ClassExprFresh/NewDynamic and the capture writeback is lost). Inverse patch of the #5874 payload applied to CURRENT main via 3-way, so the fleet merges that landed on top are preserved: - #5991 (extends Promise): new.rs/new_helpers.rs taken from the prior working revert (unchanged by the 11 intervening merges); its promise_parent_runtime + emit_promise_subclass_init kept, #5874's map_set_default_super_kind scaffold dropped. - #5999 (js_dynamic_mod fmod sign-of-zero): #5874's redundant dynamic_number_operand insertion removed, #5999's 'a % b' kept. - #6004/#6005/#6007 (exotic-receiver dispatch, proxy get, seal on functions): 3-way removed ONLY #5874's hunks from native_call_method.rs/proxy.rs; the later fixes' additions stay. Verified: temporal 6/6 (was 5/6 on main), 806 unchanged, stdlib links, proxy e2e green, and behavioral probes for #5999 (mod -0), #5991 (promise subclass = 42), #6004/#6007 (exotic dispatch + fn seal) all match node. Both stray binaries deleted. #5874 stays in history at 4500b5d for its author to re-land through review after fixing the extends regression.
Summary
Adds real
class X extends Promisesubclass support (issue #5907's core work).super(executor)now runs the ECMA-262 27.2.3.1 Promise constructor against a hidden backingPromisecell stashed on the subclass instance, and inheritedthen/catch/finally,NewPromiseCapability(Subclass), SpeciesConstructor, and inherited builtin statics all resolve against it.Before / after (
built-ins/Promise, node v26.3.0)Parity 96.3% → 97.9%, zero regressions in the slice. Broader spot-checks:
language/statements/class95.7% → 95.8% (slightly better),built-ins/Objectunchanged at ~99–100%,built-ins/Array100%.Fixed clusters:
{all,allSettled,race,resolve}/ctx-ctor(5) —Promise.all.call(Subclass, …)now buildsNewPromiseCapability(Subclass), running the subclass constructor soinstance.constructor === Subclass,instance instanceof Subclass,callCount === 1,typeof executor === 'function'all hold.{all,allSettled,race,any}/invoke-resolve-on-…-every-iteration-of-custom(5) — inherited/overriddenSubclass.resolve(incl.Subclass.resolve.bind(Subclass)) now resolves; the per-element loop calls the user override.Root cause / approach
A Perry class instance is a plain
ObjectHeader, not aGC_TYPE_PROMISEcell, sosuper()toPromiseused to be a no-op. Mirroring the existingMap/Setsubclass-backing pattern (object::map_set_subclass), a newpromise::subclassmodule runs the Promise constructor against a real backing cell and stashes it on the instance under a hidden field. Runtime dispatch unwraps that cell wherever a Promise receiver is needed:promise::subclass::{js_promise_subclass_init, subclass_backing_promise}— new module.super()routing: codegenthis_super_call.rs(explicitsuper(exec)) +lower_call/new.rs(no-own-ctornew X(exec)) + runtimeconstruct.rs(NewPromiseCapability(Subclass)/ dynamicnew) all attach the backing.promise::then(then/catch/finallythunks +then_thunkSpeciesConstructor),checked_dispatch(codegen fast-path entries), andnative_call_method(dynamic dispatch).is_object_value/is_promise_species_objectaccept a constructor value (a user-class ClassRef is still an Object per spec).js_dynamic_object_get_property+ class-ref value read inget_field_by_namefall back to the reified Promise static (after checking a user override first);js_class_static_method_calldispatches the spec static withthis = Subclass;class_metarecognizes the Promise ctor value.Remaining (noted on the issue, deeper architectural work)
finally/subclass-{resolve,reject}-count= 7, observable-then-count = 5): Perry's native promise fast path collapses the chain to count 1/3 instead of routing every internal step throughNewPromiseCapability(subclass).then/capability-executor-*,finally/species-constructor,then/deferred-is-resolved-value: deeper then-capability semantics (constructor returning a plain{}).catch/finally this-value-*: primitivethiswhose prototype carries.then.*/does-not-invoke-array-setters: unrelated negative-test miss.Code-only (no version bump / CHANGELOG per contributor workflow).
cargo fmt --all -- --checkandbash scripts/check_file_size.shpass. Refs #5907.Summary by CodeRabbit
New Features
class X extends Promise, including correctsuper(...)-style initialization with promise executors and hidden backing Promise handling.Bug Fixes
then,catch, andfinallycorrectly unwrap backing promises and align withSpeciesConstructor.resolve/all) work with subclasses.