Re: [heycam/webidl] Add async_iterable support (#720)

bzbarsky commented on this pull request.



> +being iterated over asynchronously to obtain a sequence of values.
+
+Note: In the ECMAScript language binding, an interface that is asynchronously iterable will have
+<code class="idl">entries</code>, <code class="idl">keys</code>, <code class="idl">values</code>,
+and {{@@asyncIterator}} properties on its [=interface prototype object=].
+
+Prose accompanying an [=interface=] with an [=asynchronously iterable declaration=] must define a
+<dfn id="dfn-get-the-next-iteration-result">get the next iteration result</dfn> algorithm.
+This algorithm receives a <b>[=this=]</b> value, which is an instance of the [=interface=] that it
+is defined for, and the <dfn export>current state</dfn>.
+It must return a {{Promise}} that either resolves with undefined – to signal the end of the
+iteration – or a tuple with three elements:
+
+1.  a value of the first type given in the declaration;
+1.  a value of the second type given in the declaration;
+1.  an opaque value that is passed back to the next invocation of the algorithm,

If we do stick with this setup, item 3 here should say that the value is passed back as the `<dfn>current state</dfn>`

> +
+        To [=get the next iteration result=] for <code class="idl">SessionManager</code>, run the
+        following steps:
+
+        1.  Let |promise| be a new promise.
+        1.  Let |key| be as follows:
+            <dl class="switch">
+                  :  If <b>current state</b> is not given
+                  :: the username in <b>this</b>'s open sessions that is first in lexicographical
+                     order
+
+                  :  If <b>current state</b> is given
+                  :: the username in <b>this</b>'s open sessions that is next in lexicographical
+                     order, after <b>current state</b>
+            </dl>
+        1.  If there is no such |key|, then:

It's a little weird to say that we set key to something, but then sort of offhand acknowledge that setting it that way may not have been possible.  If someone were actually writing a spec like this, I would want them to be a bit more precise about what's going on.

Also, I would want them to be a _lot_ more precise about what happens if the "current state" is given but is no longer in the list of open sessions.  It's really not clear whether the "next in lexicographical order" bit means "find the current state in the list, then take the next item" or whether it means "find the first item in the list that comes after current state".  The two are equivalent if the list is not getting mutated during the iteration, of course, but I worry about every consumer of this needing to carefully handle iterator invalidation due to mutations of the iterable.  Unfortunately I don't have a great one-size-fits-all solution here.  But it might be worth having some guidance for how specs should handle this.  Do we have any concrete examples to think about that we could base that guidance on?

> +    interface interface_identifier {
+      async_iterable&lt;key_type, value_type&gt;;
+    };
+</pre>
+
+Objects that [=implement=] an [=interface=] that is declared to be asynchronously iterable support
+being iterated over asynchronously to obtain a sequence of values.
+
+Note: In the ECMAScript language binding, an interface that is asynchronously iterable will have
+<code class="idl">entries</code>, <code class="idl">keys</code>, <code class="idl">values</code>,
+and {{@@asyncIterator}} properties on its [=interface prototype object=].
+
+Prose accompanying an [=interface=] with an [=asynchronously iterable declaration=] must define a
+<dfn id="dfn-get-the-next-iteration-result">get the next iteration result</dfn> algorithm.
+This algorithm receives a <b>[=this=]</b> value, which is an instance of the [=interface=] that it
+is defined for, and the <dfn export>current state</dfn>.

The example below talks about the current state being "given" or "not given", so presumably this arg is optional?  That should be made clear here, probably, along with the fact that the "not given" case corresponds to the start of the iteration.

> +Note: In the ECMAScript language binding, an interface that is asynchronously iterable will have
+<code class="idl">entries</code>, <code class="idl">keys</code>, <code class="idl">values</code>,
+and {{@@asyncIterator}} properties on its [=interface prototype object=].
+
+Prose accompanying an [=interface=] with an [=asynchronously iterable declaration=] must define a
+<dfn id="dfn-get-the-next-iteration-result">get the next iteration result</dfn> algorithm.
+This algorithm receives a <b>[=this=]</b> value, which is an instance of the [=interface=] that it
+is defined for, and the <dfn export>current state</dfn>.
+It must return a {{Promise}} that either resolves with undefined – to signal the end of the
+iteration – or a tuple with three elements:
+
+1.  a value of the first type given in the declaration;
+1.  a value of the second type given in the declaration;
+1.  an opaque value that is passed back to the next invocation of the algorithm,
+
+The prose may also define <dfn>asynchronous iterator initialization steps</dfn> for the

What are these steps allowed to do?  This seems pretty open-ended...  Maybe that's ok.

> +An [=interface=] must not have more than one [=asynchronously iterable declaration=].
+The [=inherited interfaces=] of an [=interface=] with an [=asynchronously iterable declaration=]
+must not also have an [=asynchronously iterable declaration=].
+An [=interface=] with an [=asynchronously iterable declaration=] and its [=inherited interfaces=]
+must not have a [=maplike declaration=], [=setlike declaration=], or [=iterable declaration=].
+
+The following extended attributes are applicable to [=asynchronously iterable declarations=]:
+[{{Exposed}}],
+[{{SecureContext}}].
+
+Issue: these [=extended attributes=] are not currently taken into account.
+When they are, the effect will be as you would expect.
+
+<pre class="grammar" id="prod-AsyncIterable">
+    AsyncIterable :
+        "async_iterable" "&lt;" TypeWithExtendedAttributes "," TypeWithExtendedAttributes "&gt;" ";"

Is there a reason for `async_iterable` rather than `async iterable`?  The latter seems a bit nicer to read/write to me and the grammar bit should be fine, I expect.

> +        for await (let username of sm.keys()) {
+          console.log(username);
+        }
+
+        // Yet another way of accomplishing the same.
+        for await (let [username, session] of sm) {
+          console.log(username);
+        }
+    </pre>
+</div>
+
+An [=interface=] must not have more than one [=asynchronously iterable declaration=].
+The [=inherited interfaces=] of an [=interface=] with an [=asynchronously iterable declaration=]
+must not also have an [=asynchronously iterable declaration=].
+An [=interface=] with an [=asynchronously iterable declaration=] and its [=inherited interfaces=]
+must not have a [=maplike declaration=], [=setlike declaration=], or [=iterable declaration=].

The corresponding prose for "iterable declaration" needs to have "asynchronously iterable declaration" added.

> @@ -11742,6 +11981,33 @@ for the interface.
 The \[[Prototype]] [=internal slot=] of an [=iterator prototype object=]
 must be {{%IteratorPrototype%}}.
 
+<div algorithm>
+  The <dfn>iterator result</dfn> for a pair of IDL values |pair| and a kind |kind| is given by the
+  following steps:
+
+    1.  Let |result| be a value determined by the value of |kind|:
+        <dl class="switch">
+             :  "<code>key</code>"
+             :: 1.  Let |idlKey| be |pair|’s key.

I'm not sure what this means.  We have a pair, whatever that is.  It's not really defined anywhere.  Sync iterables define a concept of "value pair" which is explicitly a pair of things one of which is the key and one of which is the value.  If that's the concept being used here, then we should call it "value pair" to make that clear.  The algorithm you factored this out from got _pair_ out of `value pairs to iterate over` which is a list of "value pairs", so that wasn't an issue there.

> +<h5 id="es-default-asynchronous-iterator-object">Default asynchronous iterator objects</h5>
+
+A <dfn id="dfn-default-asynchronous-iterator-object" export>default asynchronous iterator
+object</dfn> for a given [=interface=], target and iteration kind is an object whose \[[Prototype]]
+[=internal slot=] is the [=asynchronous iterator prototype object=] for the [=interface=].
+
+A [=default asynchronous iterator object=] has internal values:
+
+*   its <dfn for="default asynchronous iterator object">target</dfn>, which is an object whose
+    values are to be iterated,
+*   its <dfn for="default asynchronous iterator object">kind</dfn>, which is the iteration kind,
+*   its <dfn for="default asynchronous iterator object">ongoing promise</dfn>, which is a
+    {{Promise}} or undefined,
+*   its <dfn for="default asynchronous iterator object">state</dfn>, which is an opaque value used
+    to store the position of the iterator by the algorithm to [=get the next iteration result=], or
+    null.

Why null rather than undefined?

> +        If this threw an exception |e|, then:
+        1.  Perform [=!=] [$Call$](|thisValidationPromiseCapability|.\[[Reject]],
+            <emu-val>undefined</emu-val>, « |e| »).
+        1.  Return |thisValidationPromiseCapability|.\[[Promise]].
+
+    1.  If |object| is not a [=default asynchronous iterator object=] for |interface|, then:
+        1.  Issue: [=Realm=] check?
+        1.  Let |error| be a new {{ECMAScript/TypeError}}.
+        1.  Perform [=!=] [$Call$](|thisValidationPromiseCapability|.\[[Reject]],
+            <emu-val>undefined</emu-val>, « |error| »).
+        1.  Return |thisValidationPromiseCapability|.\[[Promise]].
+
+    1.  Let |nextSteps| be the following steps:
+        1.  Let |nextPromiseCapability| be [=!=] [$NewPromiseCapability$]({{%Promise%}}).
+        1.  Let |oldState| be |object|'s [=default asynchronous iterator object/state=].
+        1.  If |oldState| is null, then:

So the state management here is a bit wonky.  Nothing before this point said anything about the state getting _set_ to null.  How would it get there?  And the initial state bit is not really set anywhere, and explicitly labeled as "not given" at the start.  I guess the intent is that "not given" is not the same thing as "null", but it's not very clear.  It would be good to be very clear about the value space of the state (IDL values, ES values, some other set of values) and when it's set to which of those.  It sounds like the intent is that it's in some other value space, which contains all possible opaque values, and two magical values "start of iteration" ("not given") and "stop iteration" ("null")?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/heycam/webidl/pull/720#pullrequestreview-254190242