Skip to content

Implement Promise.all #797

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from
Draft

Implement Promise.all #797

wants to merge 20 commits into from

Conversation

komyg
Copy link
Contributor

@komyg komyg commented Jul 22, 2025

No description provided.

@komyg komyg force-pushed the feature/promise.all branch from 5013ece to d6226b1 Compare July 28, 2025 23:30
@komyg komyg force-pushed the feature/promise.all branch 2 times, most recently from 828ac3c to 11a9777 Compare August 8, 2025 13:22
@komyg komyg force-pushed the feature/promise.all branch from 297b697 to 493bcde Compare August 12, 2025 10:09
Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job so far! The heap addition looks really good, though some parts are missing.

#[derive(Debug, Clone, Copy)]
pub struct PromiseAllRecordHeapData<'a> {
pub remaining_unresolved_promise_count: u32,
pub result_array: Array<'a>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: Here it might be more appropriate to use the ElementsVector type, though that is not quite so easy with its API as the Array is.

Hmmm, ElementsVector should maybe be renamed to List...

}
}

impl Index<PromiseAllRecord<'_>> for Agent {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I've actually started to move us away from using the Index traits, as they leak the Output type to the public scope when both the "key" (PromiseAllRecord) and the "target" (Agent) are public.

A set of get and get_mut methods on PromiseAllRecord (taking either Agent or AsRef/AsMut<[PromiseAllRecordHeapData]>, depending on your taste and needs) is what I've been doing recently, although that should really be turned into a custom trait.

@@ -159,6 +159,7 @@ pub fn heap_gc(agent: &mut Agent, root_realms: &mut [Option<Realm<'static>>], gc
promise_reaction_records,
promise_resolving_functions,
promises,
promise_all_records,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Here you seem to be missing the loop over the marked PromiseAllRecords; see below all the drain things.

Also, there's an is_empty method that should check for && promise_all_records.is_empty().

use crate::ecmascript::builtins::promise_objects::promise_abstract_operations::promise_all_record::PromiseAllRecordHeapData;
use crate::ecmascript::builtins::promise_objects::promise_abstract_operations::promise_reaction_records::PromiseReactionHandler;
use crate::ecmascript::builtins::promise_objects::promise_prototype::inner_promise_then;
use crate::ecmascript::builtins::Array;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Combine imports.


impl<'a> PromiseAllRecordHeapData<'a> {
pub(crate) fn on_promise_fufilled(
&mut self,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Aight, let's move this method to PromiseAllRecord; it doesn't really need to change all that much. Assuming we set up the get and get_mut method on PromiseAllRecord then something like this ought to do:

pub(crate) fn on_promise_fufilled(
        self,
        agent: &mut Agent,
        index: u32,
        value: Value<'a>,
        mut gc: GcScope<'a, '_>,
    ) {
        let promise_all = self.bind(gc.nogc());
        let value = value.bind(gc.nogc());
        let data = promise_all.get_mut(&mut agent.heap.promise_all_records); // splitting heap borrow
        let result_array = data.result_array.bind(gc.nogc()); // note: this copies the handle from the heap to stack; on stack we must always bind to make sure we don't use the handle after GC.
        let elements = result_array.as_mut_slice(...arrayheap...); // we want to split the heap borrow so we can get the elements slice as mutable at the same time as we get data as mutable. ArrayHeap is the thing you want to pass here here, IIRC.
        elements[index as usize] = Some(value.unbind());

        data.remaining_unresolved_promise_count -= 1;
        if data.remaining_unresolved_promise_count == 0 {
          let capability = ...;
          capability.resolve(agent, result_array.into_value().unbind(), gc);
        }
}

Now everything should be up-and-up from the GC perspective.

capability.resolve(
agent,
self.result_array.unbind().into_value(),
gc.reborrow(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: No need to reborrow "on the last line" so to say; though here it doesn't have any effect really.

) -> JsResult<'gc, Value<'gc>> {
Err(agent.todo("Promise.all", gc.into_nogc()))
// 1. Let C be the this value.
if this_value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I'm skipping reviewing the all method; it's missing nearly all spec text and I assume it's still mostly WIP and testing code for you.

Copy link
Contributor Author

@komyg komyg Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is mostly WIP and debug code at this point. I will mark it more clearly next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants