Editorial: Simplify algorithms by making (Async)IteratorClose idempotent

[gibson042]

This removes a lot of encapsulation-piercing iteratorRecord.[[Done]] checks.

[bakkot]

Hmm. This does simplify the algorithms, but I'm not sure it's actually an improvement, especially given that many uses e.g. deterministically know that the iterator is not closed at the time they call IteratorClose.

What if instead we add a new AO like IteratorCloseIfNotClosed to be used in these places?

[gibson042]

I considered something like that, and decided that it wasn't very helpful but to be honest am still on the fence. However, rather than introducing a new operation, I'd prefer to precede selected steps with Assert: _iteratorRecord_.[[Done]] is *false*. AFAICT, the full collection is:

• GroupBy and Array.from and AddEntriesFromIterable, early in the body of an IteratorStepValue loop to abort with error (like IfAbruptCloseIterator but with an explicitly created abrupt completion)
  • Also IfAbruptCloseIterator without direct IteratorClose in constructors for Set and WeakSet, effectively a simplified AddEntriesFromIterable
• ForIn/OfBodyEvaluation steps 6.i and 6.l.ii and evaluation of YieldExpression : yield * AssignmentExpression step 7.b.iii, later in the body of a similar loop [that manually recapitulates IteratorStepValue to handle awaiting an async iteration result as needed] just before exiting the algorithm containing the loop.
• DestructuringAssignmentEvaluation of ArrayAssignmentPattern : [ ], to trivially close an Iterator Record immediately after creating it

[bakkot]

There's another 7 coming in iterator helpers, another 2 in Set methods, and another 8 in temporal, all of which only call IteratorClose on iterators known to be open at the time of the call.

These will increasingly be the common case - the ones currently in the spec are the unusual ones, which I don't want to optimize for.

[gibson042]

I think we could also make things more clear by refactoring IfAbruptCloseIterator to return the unwrapped value rather than using it for alias reassignment, such that rather than being a shorthand it is a local operation more similar to IteratorClose:

-          1. IfAbruptCloseIterator(_key_, _iteratorRecord_).
+          1. Set _key_ to ? IfAbruptCloseIterator(_key_, _iteratorRecord_).

[bakkot]

Unfortunately we can't make that change for IfAbruptRejectPromise, because in that case the macro is returning a normal completion (containing a rejected promise) from the containing algorithm.

So the value of doing it for IfAbruptCloseIterator is somewhat diminished (because it would be out of sync with IfAbruptRejectPromise).

Still maybe worth it on net; I'm not sure.

[gibson042]

Good point, and de-macroizing IfAbruptRejectPromise would require new steps.

-          1. IfAbruptRejectPromise(_return_, _promiseCapability_).
+          1. Let _rejected_ be IfAbruptRejectPromise(_return_, _promiseCapability_).
+          1. If _rejected_ is not ~empty~, return _rejected_; else set _return_ to ! _return_.

I'll leave this open for a few days of consideration.

[bakkot]

In editor call @michaelficarra was also not in favor of this PR as-is, and didn't think that IteratorCloseIfNotClosed was necessary either.