promise: spec-compliant then/finally + SpeciesConstructor + error propagation#5767
Conversation
…pagation Fixes a batch of built-ins/Promise test262 failures rooted in three gaps: 1. **SpeciesConstructor in `Promise.prototype.then`** — previously the native fast-path was always taken, ignoring `this.constructor[@@species]`. Now `promise_prototype_then_thunk` reads the species constructor once; if it is the default %Promise% constructor the fast path fires, otherwise a `NewPromiseCapability(C)` record is built and `PerformPromiseThen` routes fulfillment/rejection through that capability's resolve/reject functions via per-reaction wrapper closures with their own setjmp frames. 2. **Spec-compliant `Promise.prototype.finally`** — rewritten to always call `Invoke(this, "then", [thenFinally, catchFinally])` so poisoned `.then` getters and non-callable `.then` values are properly observable. When `onFinally` is callable, spec-compliant `thenFinally`/`catchFinally` closures are constructed: each calls `onFinally()` with zero arguments, then does the extra `.then(valueThunk/thrower)` hop via `C.resolve(result)` as required by §27.2.5.3. 3. **Error propagation in `js_promise_resolve_spec`/`js_promise_reject_spec`** — previously the `call_with_this(cap.resolve/reject, …)` result was discarded with `let _ = …`, silently eating exceptions thrown by custom capability executors. Both now call `crate::exception::js_throw(thrown)` on error. Also adds `pub(super)` visibility on `Capability`, `new_promise_capability`, `is_default_promise_constructor`, and a thin `is_callable_value` wrapper so `then.rs` can use these without re-exporting the full struct. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016dCA4sTJ7GDCvNHMgM3P4K
|
Warning Review limit reached
More reviews will be available in 33 minutes and 14 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesPromise spec compliance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 5
🤖 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/promise/spec_combinators.rs`:
- Around line 210-214: The default Promise check in
is_default_promise_constructor is using the mutable globalThis.Promise lookup,
which can be changed by user code and break the fast path. Update this helper to
compare against an immutable intrinsic Promise reference instead of
js_get_global_this_builtin_value, and reuse that same intrinsic reference in the
SpeciesConstructor default-constructor path so both checks stay consistent.
In `@crates/perry-runtime/src/promise/then.rs`:
- Around line 1196-1198: The get_intrinsic_promise helper is incorrectly reading
the mutable global Promise binding instead of the intrinsic %Promise%
constructor. Update this path so SpeciesConstructor’s default constructor comes
from the engine’s intrinsic object for Promise, not globalThis.Promise; locate
the fix in get_intrinsic_promise and any call sites that rely on it so they use
the intrinsic lookup API instead of the global builtin value accessor.
- Around line 1525-1528: The Promise.prototype.finally receiver check is too
broad because `JSValue::is_pointer()` allows non-object pointer-like values such
as registered symbols and native handles to slip through. Update the validation
in `then.rs` near the `JSValue::from_bits(receiver.to_bits())` and
`throw_promise_finally_non_object()` path to use a real JS object check from the
relevant Promise/finally handling logic, so only actual JS objects can proceed
to `.constructor` lookup and all other cases throw the required TypeError.
- Around line 1271-1292: The custom-species promise handler currently calls
`on_fulfilled` via `arg_to_closure`, which can misclassify pointer-tagged
non-callables and skip or misinvoke callable proxies/classes; update the `then`
callback path to invoke handlers through the generic callable flow used by
`call_with_this`-style helpers. Also make the capability `resolve`/`reject`
invocation pass `this = undefined` and preserve the existing throw-to-reject
handling, and apply the same fix in the other promise continuation blocks
referenced by the related `then` logic.
- Around line 1183-1194: The `is_promise_species_object` predicate is too strict
for `SpeciesConstructor` because it rejects handle-band values before
`@@species` is accessed, which breaks proxy constructors. Update this check to
use the runtime’s proxy-aware `Type(Object)` logic so proxies are treated as
valid objects/constructors and can route the species lookup through their traps.
Keep the function name `is_promise_species_object` as the place to adjust the
object test and remove the handle-band rejection that incorrectly excludes
proxies.
🪄 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: 2e899d9b-9cb4-46ab-98ae-30499a551d61
📒 Files selected for processing (2)
crates/perry-runtime/src/promise/spec_combinators.rscrates/perry-runtime/src/promise/then.rs
| pub(super) fn is_default_promise_constructor(c: f64) -> bool { | ||
| let promise_ctor = | ||
| crate::object::js_get_global_this_builtin_value(b"Promise".as_ptr(), b"Promise".len()); | ||
| !is_undef(promise_ctor) && promise_ctor.to_bits() == c.to_bits() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not identify %Promise% through mutable globalThis.Promise.
This helper reads the current global Promise field, so replacing globalThis.Promise can make the real intrinsic miss the fast path or make a user constructor look like the default. Use an immutable intrinsic Promise reference for this check, and share it with the SpeciesConstructor default 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/promise/spec_combinators.rs` around lines 210 - 214,
The default Promise check in is_default_promise_constructor is using the mutable
globalThis.Promise lookup, which can be changed by user code and break the fast
path. Update this helper to compare against an immutable intrinsic Promise
reference instead of js_get_global_this_builtin_value, and reuse that same
intrinsic reference in the SpeciesConstructor default-constructor path so both
checks stay consistent.
| fn get_intrinsic_promise() -> f64 { | ||
| crate::object::js_get_global_this_builtin_value(b"Promise".as_ptr(), b"Promise".len()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Return the intrinsic %Promise%, not the mutable global binding.
SpeciesConstructor’s default constructor is the intrinsic %Promise%. Reading globalThis.Promise makes p.constructor === undefined depend on user reassignment of the global binding.
🤖 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/promise/then.rs` around lines 1196 - 1198, The
get_intrinsic_promise helper is incorrectly reading the mutable global Promise
binding instead of the intrinsic %Promise% constructor. Update this path so
SpeciesConstructor’s default constructor comes from the engine’s intrinsic
object for Promise, not globalThis.Promise; locate the fix in
get_intrinsic_promise and any call sites that rely on it so they use the
intrinsic lookup API instead of the global builtin value accessor.
…nally receiver check Two fixes addressing CodeRabbit review comments on PR #5767: 1. arg_to_closure misclassification (then_cap_fulfill_fn, then_cap_reject_fn, spec_then_finally_fn, spec_catch_finally_fn): arg_to_closure treats any POINTER_TAG value as a callable closure, so a plain non-callable object passed as onFulfilled/onRejected/onFinally would produce a non-null but invalid closure pointer and corrupt the continuation. Guard each call site with is_callable_value so non-callable objects correctly take the pass-through (identity/rethrow) branch. 2. Promise.prototype.finally receiver check: the old JSValue::is_pointer() guard allowed registered symbols and native UI handles (both POINTER_TAG) through to the constructor lookup. Replace with is_promise_species_object which additionally rejects the handle band and registered symbols, matching the spec requirement that Type(O) === Object. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016dCA4sTJ7GDCvNHMgM3P4K
2 participants
Closes part of #5590 (built-ins/Promise test262 failures).
Root cause
Three independent gaps caused the bulk of the
built-ins/Promise/test262 failures:Promise.prototype.thenignoredthis.constructor[@@species]— the native fast-path was always taken, so tests asserting thatthenusesSpeciesConstructor(this, %Promise%)to pick the result constructor all failed. The fix reads the species constructor once; if it equals the default%Promise%the existing fast path fires unchanged, otherwise aNewPromiseCapability(C)record is built andPerformPromiseThenroutes fulfillment/rejection through that capability's resolve/reject functions via per-reaction wrapper closures with their ownsetjmpframes.Promise.prototype.finallydid not callInvoke(this, "then", …)— the old implementation bypassed the observable.thencall, so poisoned.thengetters and non-callable.thenvalues were invisible. Rewritten to always callInvoke(this, "then", [thenFinally, catchFinally]). WhenonFinallyis callable, spec-compliantthenFinally/catchFinallyclosures are constructed: each callsonFinally()with zero arguments and then does the extra.then(valueThunk/thrower)hop viaC.resolve(result)as required by §27.2.5.3.js_promise_resolve_spec/js_promise_reject_specsilently swallowed errors — thecall_with_this(cap.resolve/reject, …)result was discarded withlet _ = …, eating exceptions thrown by custom capability executors. Both functions now propagate viacrate::exception::js_throw(thrown).Changes
crates/perry-runtime/src/promise/spec_combinators.rs: error propagation injs_promise_resolve_specandjs_promise_reject_spec;pub(super)visibility onCapability,new_promise_capability,is_default_promise_constructor; newis_callable_valuewrapper.crates/perry-runtime/src/promise/then.rs:SpeciesConstructorhelper (promise_species_constructor), rewrittenpromise_prototype_then_thunk(fast + slow paths), rewrittenpromise_prototype_finally_thunk, newspec_then_finally_fn/spec_catch_finally_fn/spec_value_return_fn/spec_reason_thrower_fnextern-C closures,perform_promise_then_with_capwith per-reactionsetjmpframes.Test impact
The test262 corpus is not vendored in this environment (the pinned SHA
4249661388e5d3f92a85186213da140a6481490fis referenced intest-compat/test262/pinned-sha.txt). Expected improvements based on direct spec mapping:Promise/resolve/capability-invocation-errorPromise/reject/capability-invocation-errorPromise/prototype/then/ctor-*(access-count, null, poisoned, species)Promise/prototype/finally/this-value-then-*(poisoned, throws, not-callable)Promise/prototype/finally/*-observable-then-callsPromise/prototype/finally/this-value-proxyCI parity run will produce the authoritative before/after count.
🤖 Generated with Claude Code
https://claude.ai/code/session_016dCA4sTJ7GDCvNHMgM3P4K
Generated by Claude Code
Summary by CodeRabbit
Promise.thenandPromise.finallybehavior to better match standard JavaScript semantics, including correct handling of custom promise constructors and@@species.Promise.rejectandPromise.resolvenow surface errors from their internal handlers instead of silently ignoring them.Promise.finallynow handles non-callable callbacks and non-Promise object receivers more consistently.