[spec] [[UnderlyingIterator]] -> [[SourceIterators]] so flatMap can proxy return to the inner iterator also

[ljharb]

This closes in the proper order - inner first, then outer - and if both throw, the last exception thrown is the one you see, just like a nested try/finally.

However, this is inconsistent in that flatMap makes ignores the inner iterator's exception and throws the outer one's.

One solution is to make the first exception the one thrown, but that doesn't match try/finally.

Another solution is to throw an AggregateError - either always, or, only when more than one of the return methods throws.

[bakkot]

I don't think this is necessary. [[UnderlyingIterator]], introduced in #267, is only necessary for the specific case that .return is called before .next - i.e., when the generator is in the ~suspendedStart~ case. But in that case you haven't called the mapper function yet and so there's only a single iterator to worry about.

In the non-~suspendedStart~ case closing of both iterators is handled by the injected return completion triggering steps 5.b.ix.4.d of flatMap.

[ljharb]

Without this change, if you call iter.next() and then iter.return() with a flatMap mapper that returns an iterator that yields more than one item, the inner iterator (returned by the mapper) won't get its .return called.

[bakkot]

Yes, it will, per the step named in my comment. This change does not affect anything that happens after the first call to .next: that call will immediately transition the helper out of the ~suspendedStart~ state, and the only place this PR would matter is guarded by If _O_.[[GeneratorState]] is ~suspendedStart~, then.

[ljharb]

@bakkot that covers if next throws. This is if you explicitly call .return on the wrapped iterator, after calling .next (which @michaelficarra does in one of the test262 tests in the PR, which is what prompted this PR)

[bakkot]

5.b.ix.4.d actually doesn't cover .next throwing - that's step 5.b.ix.2.

Rather, it covers the case where the Yield produces any abrupt completion, which includes the Return completion which step 6 of %IteratorHelperPrototype%.return will cause Yield to produce.

That's how it works in syntactic generators, too: a call to .return causes a yield expression to return, which is observable e.g. by the fact that it will trigger a finally but not a catch:

function* gen() {
  try {
    yield;
  } catch {
    console.log('not hit');
  } finally {
    console.log('hit');
  }
}
let it = gen();
it.next();
it.return(); // prints 'hit'

[ljharb]

That applies to a generator, but does it also apply to a manually created iterator object with a .return function?

[bakkot]

It depends on what the .return function does. In this case, the iterators in this proposal are specified the same way as generators and reuse their machinery. Specifically, step 6 of %IteratorHelperPrototype%.return will cause the spec Yield to produce a Return completion, just as would happen for a syntactic iterator.

[ljharb]

OK, so that would mean that in cases like this we don't want the inner non-generator iterator's return method to be called by the wrapper iterator's .return?

[bakkot]

I don't understand your question; can you rephrase or give an example?

[ljharb]

This is the specific test https://github.com/tc39/test262/pull/2818/files#diff-ae62fa2bde6a909c28ea6de018d9e8419cb85a0d813d724448c0eecf995bc954R26 that fails with the current spec, and would pass with this PR.

[bakkot]

That test would pass with the current spec and would not be affected by this PR.

The call to .next on line 34 will advance the spec-internal generator to step 5.b.ix.4.c of flatMap, at which point control returns to the caller and the spec-internal generator remains paused on the Yield. The subsequent call to .return on line 41 is a call to %IteratorHelperPrototype%.return, which in step 6 will inject a Return completion at the Yield and resume execution of flatMap. Then the condition for 5.b.ix.4.d will be met, and subsequently 5.b.ix.4.d.i will call the return method of the inner iterator.

[ljharb]

Thanks, I see what my implementation is missing now (due to me not using syntactic generators). I think this can likely indeed be closed, but since @michaelficarra asked me to open it I'll leave it open until he can confirm.

[michaelficarra]

I'm satisfied by @bakkot's explanation. We also discussed using AggregateError but it isn't consistent with other error handling (an unfortunate consequence of history), nor is it worth disrupting a stage 3 proposal more than we already are.