fix(runtime): resolve the subclass in Promise SpeciesConstructor (#5907)#6702
Conversation
…ryTS#5907) `Promise.prototype.{then,catch,finally}` run SpeciesConstructor(receiver, %Promise%), which reads `receiver.constructor[Symbol.species]`. Perry installs no `Promise[Symbol.species]` accessor, so that read was always `undefined` and SpeciesConstructor fell back to the intrinsic %Promise% — collapsing every `class X extends Promise` chain onto the native fast path and never routing through NewPromiseCapability(X). Emulate the standard getter (which returns `this`) directly in `promise_species_constructor`: an absent `@@species` on a Promise-branded constructor resolves to `C` itself, so subclass then/catch/finally chain a subclass promise via NewPromiseCapability. Restricted to Promise/subclass constructors — a reassigned non-Promise `.constructor` keeps the %Promise% default — and an explicit `null` species still yields the spec default. Fixes built-ins/Promise/prototype/finally/subclass-{resolve,reject}-count (test262). built-ins/Promise: 563 -> 565 pass, zero regressions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPromise species resolution now detects Promise-branded constructors, including Promise subclasses. Undefined species returns the receiver constructor only for branded constructors, while null and non-Promise constructors fall back to the intrinsic Promise. ChangesPromise species resolution
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/promise/then.rs`:
- Around line 1094-1097: Root the NaN-boxed object value c with
RuntimeHandleScope before the Step 4 @@species lookup, create a handle via
create_handle_f64, and reload c with get_nanbox_f64 after
js_object_get_symbol_property completes. Use the reloaded c for
is_promise_brand_constructor and the return path, preserving existing species
handling.
🪄 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: 881b4a70-5e4b-45b0-aff2-9e533b579f74
📒 Files selected for processing (1)
crates/perry-runtime/src/promise/then.rs
Address review: a user `Symbol.species` accessor runs arbitrary code that can allocate and evacuate the (movable, heap-pointer) constructor, so holding the NaN-boxed `C` across `js_object_get_symbol_property` and reusing it afterward (the new `undefined`-species emulation returns `C`) risked a stale pointer. Root `C` with a `RuntimeHandleScope` before the read and reload it from the rewritten handle. No-op for class-ref constructors (non-pointer ids); built-ins/ Promise stays at 565 pass, zero regressions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Promise.prototype.{then,catch,finally}run SpeciesConstructor(receiver, %Promise%), which readsreceiver.constructor[Symbol.species]. Perry installs noPromise[Symbol.species]accessor, so that read always returnedundefinedand SpeciesConstructor fell back to the intrinsic%Promise%. The effect: everyclass X extends Promisechain collapsed onto the native fast path and never routed throughNewPromiseCapability(X), so subclassthen/catch/finallyproduced the wrong number of chained promises.Root cause
In
promise_species_constructor(crates/perry-runtime/src/promise/then.rs), step 5 mapped anundefinedspecies to%Promise%. Since the standardPromise[Symbol.species]getter (which returnsthis) is never installed, the read is alwaysundefined, so the subclass was lost and the fast path was always taken.Fix
Emulate the standard getter directly in
promise_species_constructor: an absent@@specieson a Promise-branded constructor resolves toCitself — the value real engines observe from the inherited getter. Now:Promisestill resolves to%Promise%(fast path unchanged for plain promises);class X extends Promiseresolves toX, soX.prototype.{then,catch,finally}chain a subclass promise viaNewPromiseCapability(X)(ECMA-262 27.2.5.4.1);.constructorcarries no such inherited getter and keeps the%Promise%default;nullspecies still yields the spec default.Validation
scripts/test262_subset.py --dir built-ins/Promise(serial):Newly passing (0 regressions, diffed by test path):
built-ins/Promise/prototype/finally/subclass-resolve-count.jsbuilt-ins/Promise/prototype/finally/subclass-reject-count.jsAlso verified: 14 promise/async gap tests still match the Node oracle byte-for-byte; the promise-scoped
perry-runtimeunit tests pass;rustfmt --checkclean on the changed file.Code-only PR (no version bump / CHANGELOG / CLAUDE.md — maintainer folds metadata). Refs #5907.
The deeper remaining
built-ins/Promisefailures the issue tracks (subclass constructors that overridethisby returning a plain object —then/capability-executor-*,then/deferred-is-resolved-value— and the per-step observablePromiseResolvecount infinally/{resolved,rejected}-observable-then-calls-PromiseResolve) are out of scope here; they need derived-constructor return-value semantics and the full finally routing, respectively.Summary by CodeRabbit
Promise.prototype.thenhandling of@@speciesby correctly distinguishingnullvsundefinedreads.null@@speciescontinues to fall back to the intrinsic%Promise%.undefined@@speciesnow resolves to the receiver constructor only when it’s Promise-branded; otherwise it falls back to%Promise%, improving compatibility with Promise subclasses.