fix(runtime): dynamic new P(executor) for the Promise constructor value#6211
fix(runtime): dynamic new P(executor) for the Promise constructor value#6211proggeramlug wants to merge 1 commit into
new P(executor) for the Promise constructor value#6211Conversation
…alue
`new P(executor)` where P holds the global Promise constructor VALUE (a
polyfill or bundler alias) threw "Constructor Promise requires 'new'": the
dynamic-construct builtin match had no "Promise" arm, so the callee fell
through to the closure fallback, which called promise_constructor_call_thunk
as a plain function.
Add the missing arm, routing to js_promise_new_with_executor exactly like the
static literal lowering (non-callable executors still throw the spec
TypeError).
Adds test_gap_dynamic_builtin_construct_dispatch.ts, byte-for-byte against
node: Promise-value construction (resolve + reject paths), dynamic
TextEncoder/TextDecoder method dispatch through require("util") constructor
values (regression coverage for #6162), and Map/Set constructor-value guards.
|
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 (2)
📝 WalkthroughWalkthroughAdds a ChangesDynamic Promise Construction
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Test as Test Script
participant Dispatch as js_new_function_construct
participant Promise as js_promise_new_with_executor
Test->>Dispatch: new Promise(executor)
Dispatch->>Dispatch: extract executor argument
Dispatch->>Dispatch: check pointer-tagged closure/function
Dispatch->>Promise: js_promise_new_with_executor(executorPtr)
Promise-->>Dispatch: constructed promise
Dispatch-->>Test: NaN-boxed promise pointer
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Problem
new P(executor)wherePholds the globalPromiseconstructor value —const P = Promise;or a polyfill/bundler alias — threwConstructor Promise requires 'new'.The dynamic-construct path (
js_new_function_construct) identifies globalbuiltin constructor values and routes them to their native factories, but the
match had no
"Promise"arm, so the callee fell through to the closurefallback, which called
promise_constructor_call_thunkas a plain function —and that thunk (correctly) rejects being invoked without construct semantics.
Fix
Add the missing
"Promise"arm to the identified-builtin construct match inclass_registry/construct.rs, routing tojs_promise_new_with_executorexactlylike the static
new Promise(executor)lowering. A non-callable executor stillthrows the spec TypeError (validation lives in the factory).
Validation
New gap test
test_gap_dynamic_builtin_construct_dispatch.ts, byte-for-byteagainst
node --experimental-strip-types:new P(executor)resolve and reject paths via a Promise constructor valueTextEncoder.encode/encodeInto+TextDecoder.decodethroughrequire("util")constructor values (regression coverage for fix(runtime): TextDecoder/TextEncoder handle methods on type-erased receivers #6162'stype-erased dispatch, which this test also exercises)
Found while unblocking Next.js App Router SSR (#5989): react-server-dom
constructs builtins through module-namespace values rather than literals, so
every missing dynamic-construct arm surfaces there.
Summary by CodeRabbit
New Features
Bug Fixes
new Promise(...)when the constructor is passed around as a value before instantiation.