Should errors in parameter initialization throw, or return an errored async generator?

[domenic]

@caitp pointed out these interesting cases, from https://chromium-review.googlesource.com/c/446961/2/test/mjsunit/harmony/async-generators-basic.js#1191. A representative sample is:

async function* f([...a]) {
}

f(null);

That is, the call tries to destructure null, and fails.

Per spec this currently throws an exception, matching generators. Should it instead convert the exception into an errored async generator object, somewhat matching async functions? That is, should we get

// WITH A SPEC CHANGE:

const g = f(null);

g.next(); // returns a rejected promise

This change would make sense from the perspective of the "naive desugaring":

async function* f(...args) {
  const [[...a]] = args;
}

const g = f(null);
g.next(); // throws, with the current spec

On the other hand that "naive desugaring" is already false for generators:

function* f([...a]) {
}

f(null); // throws; does not return a generator whose .next() throws

I am not sure whether the "generator nature" (two error channels) or "async function nature" (one error channel) should necessarily win out here. I'd be interested in opinions.

I guess the ideal solution here is to say that the generator design was a mistake and fix that to have one error channel :). I doubt anyone's up for that, though.

[caitp]

as mentioned on IRC, I prefer the current behaviour --- I just think it would be ideal if async non-generators had a similar behaviour.

If it's not too late to change the behaviour of async functions WRT this, they would both be congruent, and both make sense.

If congruence doesn't matter to people, then I would leave it as it is rather than change it, I don't think we need an "errored async generator" state

---

In other words, if there were going to be a spec change, my vote would be for this:

async function f([...args]) { ... }

f(null); // throws rather than returning a Promise, to match async generators

(but since async functions are ratified and in use, it may be web incompatible to change :()

[arai-a]

from my understanding, parameter destructuring should be done at the same time as the invocation,
before the function returns (even for generator case). since the caller can change the value after that.
so the "naive desugaring" there doesn't reflect what the original code says.

[ljharb]

It's a very nice property of async functions that they can never synchronously throw; it'd be nice if async generators had the same property.

[caitp]

Well, in order to make them "throw later", you'd have to add an internal field to the generator to hold onto that initial exception (or some other way to hold the exception), and reject any future-enqueued generator requests with it (or just the first one, with subsequent generator requests behaving the way AsyncGeneratorResumeNext specifies, E.g. AsyncGeneratorResolve(generator, undefined, true) when resumed again with generator.next()).

This seems not particularly worthwhile, and would add overhead to either AsyncGeneratorEnqueue, or AsyncGeneratorResumeNext().

I would prefer to keep the synchronous throwing. I don't think it would make a significant difference in practice, and it's simpler

[Jamesernator]

One thing to keep in mind is that the body of async functions isn't lazy, e.g. its immediately evaluated after the arguments so it seems natural that an error in the arguments rejects the Promise instead.

The body of a generator on the other hand may or may not be evaluated on the same tick as the arguments, as such I think it would be more consistent to stick with how it works in generators.

async function foo(x=3) {
    console.log("Hello")
    return x
}

foo()
// The arguments have been evaluated and the body has been fully evaluated, yet the return value can't
// be accessed till the next tick anyway

const err = _ => { throw new Error() }
function* foo(x=err()) {
    yield 1
}

foo()
// The arguments have been evaluated so we synchronously throw the error that was thrown in
// the arguments, the body might not even ever be evaluated

async function* foo(x=err()) {
    await fetch('delay.txt')
    yield 1
}


foo()
// The arguments have been evaluated so we synchronously throw as we didn't complete the tick
// without causing an error
// the body might not be evaluted

[Jamesernator]

Another concern I just realized after writing the previous comment is that a lot of environments are throwing warnings on uncaught promises, in the scenario of deferring the error to the call to next that'd mean we'd wind up swallowing errors (that have already been raised)!

function* naiveFunction(db=open('default.db')) {
    yield db.get('foo')
    yield db.get('bar')
}

let x = naiveFunction()
x = null // Any errors are gone and we might not have even known there was one

[RangerMauve]

we'd wind up swallowing errors (that have already been raised)

Weren't warnings on uncaught errors only visible when the promise is garbage collected?

[domenic]

Thanks all. I think the discussion here does indeed confirm that it's best to stick with the "generator nature". There's maybe a separate useful discussion to be had about whether we can change generator behavior, and if that happens, async generators will certainly follow.

Let's close this with no spec change.

[Jamesernator]

@RangerMauve The behaviour currently in V8 from what I can tell is that if there's a turn of the event loop and a Promise which rejected doesn't have a .catch handler attached it raises a warning.

Here's the tests I used in node, (chrome and firefox had similar behaviour):

const p = Promise.reject('Error')

const silly = message => {
    console.log(message)
}

silly('One')

setTimeout(silly, 1000, 'Two')

p.catch(err => console.log(err))

Works fine, whereas this:

const p = Promise.reject('Error')

const silly = message => {
    console.log(message)
}

silly('One')

setTimeout(silly, 1000, 'Two')

will display

One
(node:12369) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error
(node:12369) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
Two

and even when a catch handler is added later:

const p = Promise.reject('Error')

const silly = message => {
    console.log(message)
    if (message === 'Two') {
        p.catch(err => console.log(err))
    }
}

silly('One')

setTimeout(silly, 1000, 'Two')

it'll produce this output:

One
(node:12403) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error
(node:12403) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
Two
(node:12403) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)
Error