[JSC] Implement Promise.allKeyed and Promise.allSettledKeyed behind usePromiseAllKeyed - #359
[JSC] Implement Promise.allKeyed and Promise.allSettledKeyed behind usePromiseAllKeyed#359robobun wants to merge 2 commits into
Conversation
…sePromiseAllKeyed
Adds the Stage 3 await-dictionary proposal (tc39/proposal-await-dictionary)
as JS builtins on the Promise constructor, gated by the new
usePromiseAllKeyed option (default off).
Both methods take an object, enumerate its own enumerable keys via
[[OwnPropertyKeys]] (strings then symbols, one trap call), resolve each
value through C.resolve, and resolve the capability with a null-prototype
object keyed in enumeration order. allSettledKeyed wraps each settlement
as {status, value}/{status, reason}.
To get spec-correct [[OwnPropertyKeys]] semantics from a JS builtin, a
new @object.@OwnKeys private slot is added that calls
ownPropertyKeys(StringsAndSymbols, Include), matching Reflect.ownKeys.
test262: maps the await-dictionary feature to usePromiseAllKeyed. One
vendored test (allSettledKeyed/result-property-descriptors.js) is synced
to the upstream fix for tc39/test262#5084. 89/89 tests pass.
JSTests/stress/promise-allKeyed.js covers the basics plus symbol keys,
non-enumerable and inherited skipping, ordering, rejections, and
C.resolve validation.
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a full implementation of a new Stage 3 TC39 proposal touching JSC builtins, the Promise/Object constructors, and a new private intrinsic slot — worth a human look even though it's flag-gated.
What was reviewed:
allKeyed/allSettledKeyedbuiltins: remaining-elements counter, alreadyCalled guards, null-prototype result construction, error paths route topromiseCapability.reject.@Object.@ownKeysprivate slot wiring mirrors the existing@getOwnPropertyNames/@getOwnPropertySymbolspattern and reusesownPropertyKeys(..., StringsAndSymbols, Include).usePromiseAllKeyedoption is generated fromUnifiedWebPreferences.yamlthe same wayusePromiseIsPromiseis; methods are absent when the flag is off.
Extended reasoning...
Overview
This PR implements the Stage 3 await-dictionary proposal (Promise.allKeyed / Promise.allSettledKeyed) as JS builtins in PromiseConstructor.js, registered conditionally in JSPromiseConstructor::finishCreation behind a new usePromiseAllKeyed option (default off). Supporting changes: a new @Object.@ownKeys private slot in ObjectConstructor.cpp + BuiltinNames.h (so the builtin can do a single [[OwnPropertyKeys]] call), a new PromiseAllKeyedEnabled preference in UnifiedWebPreferences.yaml that generates the JSC option, a test262/config.yaml mapping, a test262 test sync, and a new stress test.
Security risks
None apparent. The new @ownKeys is a private-name slot (unreachable from user JS), and objectConstructorOwnKeys is a thin wrapper around the same ownPropertyKeys helper Reflect.ownKeys already uses. The builtins use @-prefixed intrinsics for internal operations and only call user-observable .resolve/.then where the spec requires it. Everything is off by default.
Level of scrutiny
Moderate-to-high. While flag-gated and default-off, this is ~160 lines of new spec-implementing builtin logic in the JSC runtime — the first engine implementation of this proposal per the PR description. Spec conformance, Proxy trap ordering, and the remainingElementsCount bookkeeping are the kind of details a JSC-familiar reviewer should sign off on. It also adds a new private intrinsic surface (@ownKeys) that will presumably be maintained going forward.
Other factors
- The implementation closely follows existing patterns (
Promise.tryfor registration, the old JS-builtinPromise.all/allSettledshape for the counter/closure logic), which lowers risk. - 89/89 test262 tests reportedly pass and the stress test covers ordering, symbols, non-enumerables, inherited props, non-object args, and non-constructor
this. - I confirmed
Options::usePromiseAllKeyed()is generated fromUnifiedWebPreferences.yamlviaJSCWebPreferenceOptions.h.erb, matching the adjacentusePromiseIsPromise. - No prior human or bot review comments to address.
Given the scope (new TC39 feature, core runtime files, first-of-kind implementation), this exceeds the bar for auto-approval even with no bugs found.
Preview Builds
|
…Keyed Asserts that [[OwnPropertyKeys]] is called exactly once on a Proxy input and that per-key [[GetOwnProperty]]/[[Get]] fire in the spec order (all own keys for gOPD, only enumerable keys for Get). A refactor to @getOwnPropertyNames + @getOwnPropertySymbols would fail the ownKeysCalls === 1 check. Mirrors JSTests/stress/object-spread.js.
There was a problem hiding this comment.
I didn't find any bugs, but this is a new Stage 3 feature implementation touching core JSC runtime/builtins, so it warrants a human look.
What was reviewed:
- Builtin structure (remainingElementsCount bookkeeping, try/catch → reject, null-prototype result via
@Object.@create(null)) mirrors the existingpromiseAllSlow/promiseAllSettledSlowshape. @Object.@ownKeysprivate slot wraps the sameownPropertyKeys(..., StringsAndSymbols, Include)path asReflect.ownKeys, and is only reachable via the private name.- Flag gating follows the
usePromiseIsPromisepattern;Options::usePromiseAllKeyed()is generated fromUnifiedWebPreferences.yamlviajscOptionName, default off.
Extended reasoning...
Overview
This PR implements the Stage 3 await-dictionary proposal (Promise.allKeyed / Promise.allSettledKeyed) as JS builtins in PromiseConstructor.js, gated behind a new usePromiseAllKeyed option (default off). Supporting changes: a new @Object.@ownKeys private slot in ObjectConstructor.cpp + BuiltinNames.h (so the builtin can perform a single [[OwnPropertyKeys]] call), conditional registration in JSPromiseConstructor::finishCreation, a new PromiseAllKeyedEnabled preference in UnifiedWebPreferences.yaml, a stress test, and a test262 config/test sync. Roughly 400 lines of diff across 8 files.
Security risks
Low. The new objectConstructorOwnKeys host function is registered only under a private name (not user-reachable) and delegates to the same ownPropertyKeys helper already used by Reflect.ownKeys / Object.getOwnPropertyNames. The builtins use @-prefixed intrinsics (@Object.@create, @putByValDirect, @isObject, etc.) so they aren't affected by prototype pollution. The feature is off by default.
Level of scrutiny
Medium-high. Even though it's flag-gated, this is a fresh spec implementation in the engine runtime — the kind of change where spec-step ordering, observable Proxy trap counts, resolve-element function .name/.length, and subclassing edge cases matter and are worth a human eye. The author reports 89/89 test262 passes and includes a Proxy-trap-count stress test, which is a good signal, but this isn't a mechanical or config-only change.
Other factors
- The implementation approach (JS builtins with closures vs. the C++
JSPromiseCombinatorsContextfast path used forPromise.all/allSettled) is a design choice a maintainer may want to weigh in on. - The
allKeyedresolve element uses a plain arrow (name"newResolveElement"vs."") whileallSettledKeyeduses the(0, arrow)form; the PR description notes test262 passes, so this may be fine, but it's the sort of nit a human reviewer might catch. - No CODEOWNERS file in the repo; no prior human review comments to address.
Given the scope and that it touches core JSC builtins/runtime, deferring to a human reviewer.
2 participants
Implements the Stage 3 await-dictionary proposal (
Promise.allKeyed/Promise.allSettledKeyed) as JS builtins on the Promise constructor, gated by a newusePromiseAllKeyedoption (default off).Semantics
Both methods:
NewPromiseCapability(this)thenGetPromiseResolve(this)(rejecting on failure).TypeErrorif the argument is not an Object.[[OwnPropertyKeys]]call (strings then symbols), skipping non-enumerable and inherited properties.Call(promiseResolve, C, value)thenInvoke(nextPromise, "then", ...).CreateKeyedPromiseCombinatorResultObject).allSettledKeyedwraps each settlement as{status: "fulfilled", value}/{status: "rejected", reason}(Object.prototype) and never rejects for individual entries.Implementation
JS builtins in
PromiseConstructor.js, following thePromise.trypattern. Registered conditionally inJSPromiseConstructor::finishCreationso the methods are absent when the flag is off.To get spec-correct
[[OwnPropertyKeys]]behaviour (one ProxyownKeystrap call rather than two), a new@Object.@ownKeysprivate slot is added that wrapsownPropertyKeys(..., StringsAndSymbols, Include), the same helperReflect.ownKeysuses.Resolve/reject element closures use the
(0, arrow)form so their.nameis""perCreateBuiltinFunction(..., 1, "", ...).Prior art
js/src/builtin/Promise.cpp, bugs 2003970 / 2003971), Nightly-only behind a runtime pref, off by default.test262.statusskips everyallKeyed/allSettledKeyedtest.await-dictionaryfeature skipped; no implementation.esnext.promise.all-keyed/esnext.promise.all-settled-keyedviaproposals/await-dictionary.Testing
JSTests/test262/config.yamlnow maps theawait-dictionaryfeature flag tousePromiseAllKeyedinstead of skipping it.JSTests/test262/test/built-ins/Promise/allSettledKeyed/result-property-descriptors.jsis synced to the current upstream version, which saves the entry objects beforeverifyPropertydeletes them (tc39/test262#5084).Files
Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml: newPromiseAllKeyedEnabledpreference withjscOptionName: usePromiseAllKeyed.Source/JavaScriptCore/builtins/PromiseConstructor.js:allKeyed/allSettledKeyedbuiltins.Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp: conditional registration.Source/JavaScriptCore/builtins/BuiltinNames.h,Source/JavaScriptCore/runtime/ObjectConstructor.cpp:@Object.@ownKeysprivate slot.JSTests/stress/promise-allKeyed.js: stress coverage.JSTests/test262/config.yaml: feature mapping.