Ecma262: Promise.{all,allSettled,race} should check resolve is a function before opening their iteratable

[kmiller68]

Currently, the spec for Promise.{all,allSettled,race} requires calling the first arguments iterator before checking if this.resolve is a function. Since this.resolve is hoisted out of the loop it makes writing the function in JS much more complicated.

If I were going to write a polyfill for Promise.{all,allSettled,race} I would expect it to look something like:

function all(iterable) {
    ...
    var promiseResolve = this.resolve;
    if (typeof promiseResolve !== "function")
        #throwTypeError("Promise resolve is not a function");
    for (let i of iterable)
        ...

Instead you need something like:

function all(iterable) {
    ...
    var iterator = iterable.#SymbolIterator();

    try {
        var promiseResolve = this.resolve;
        if (typeof promiseResolve !== "function")
            #throwTypeError("Promise.resolve is not a function");
    } catch (error) {
        try {
            iterator.return();
        } catch {
            // Inner error is ignored: see steps 6-7 of https://tc39.es/ecma262/#sec-iteratorclose
        }
        throw error;
    }
    // Since for-of loop once more looks up the @@iterator property of a given iterable,
    // it could be observable if the user defines a getter for @@iterator.
    // To avoid this situation, we define a wrapper object that @@iterator just returns a given iterator.
    var wrapper = {};
    wrapper.@@iterator = function() { return iterator; };

    for (var value of wrapper) {
          ...

Alternatively, you could iterate the iterator by hand but getting that right is actually also tricky...

Given that at one point every engine implemented this incorrectly, (https://test262.report/browse/built-ins/Promise/all/invoke-resolve-get-error-close.js?date=2019-07-06) it seems like it might be reasonable to change the spec to match the first implementation. Thoughts?

[kmiller68]

I also just remembered that we changed this behavior in: https://github.com/tc39/ecma262/issues/1505. At the time I had assumed that we would be pulling the get of resolve all the way out of the loop. This just seems like a "fix" for that change.

[bakkot]

For comparison, the Map constructor also takes an iterable and looks up a property (on some object) which is expected to be a function, and it does the four relevant steps in the order

1. Get the property (in this case "set", for Promise.all "resolve").
2. Check that the result is callable.
3. Get the iterator from the iterable.
4. Iterate.

It looks like Promise.all does them in

1. Get the iterator from the iterable.
2. Get the property.
3. Check that the result is callable.
4. Iterate.

That's a weird inconsistency just on its own. I would be happy to switch to the Map constructor's order, which I agree seems better.

@kmiller68 want to make a needs-consensus PR we can discuss at the upcoming meeting?