Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1010433002:
Assorted fixes to async: yield may suspend in async* methods; yield* skips emptysequences; definiti… (Closed)
|
Created:
5 years, 9 months ago by gbracha Modified:
5 years, 9 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAssorted fixes to async: yield may suspend in async* methods; yield* skips emptysequences; definition of flatten.
R=hausner@google.com, paulberry@google.com
Committed: https://code.google.com/p/dart/source/detail?r=44569
Patch Set 1 #
Total comments: 17
Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Total comments: 1
Messages
Total messages: 18 (1 generated)
gbracha@google.com changed reviewers: + hausner@google.com, lrn@google.com, paulberry@google.com
A bunch of clarifications and corrections to the async specs in light of discussions these past few weeks. These should go to TC52 next week.
https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3095: is $(T_1 \ldots, T_n, [T_{n+1}$ $x_{n+1}, \ldots, T_{n+k}$ $x_{n+k}]) \rightarrow Future<flatten(T_0)>$, where $T_0$ is the static type of $e$ and $flatten(T) = flatten(S)$ if $T = Future<S>$ and $T \ne S$, and $T$ otherwise. I'm confused as to why $T \ne S$ is necessary. Doesn't $T = Future<S>$ imply $T \ne S$, since infinite types are impossible to create? https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3100: The sole exception to that would be a type $X$ that extended or implemented \code{Future$<X>$}. In that case, the result type is $X$ and no further unwrapping takes place. This is in a \rationale block, so I would expect to see some normative text to back up this statement. But the normative definition of flatten above doesn't seem to account for this case. Did you perhaps mean to change the definition of flatten to something like this? "$flatten(T) = flatten(S)$ if $T = Future<S>$, $S$ if $T \ne Future<S>$ but extends or implements $Future<S>$, and $T$ otherwise."
Changes to yield in async* LGTM.
https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3095: is $(T_1 \ldots, T_n, [T_{n+1}$ $x_{n+1}, \ldots, T_{n+k}$ $x_{n+k}]) \rightarrow Future<flatten(T_0)>$, where $T_0$ is the static type of $e$ and $flatten(T) = flatten(S)$ if $T = Future<S>$ and $T \ne S$, and $T$ otherwise. I think we should flatten if T is a subtype of Future<S>, not just if it is Future<S>. It is possible to have subtypes of Future in the language, and they should also be flattened. Since you can't be a subtype of Future<S> and Future<R> at the same time (S != R), there should be at most one Future<?> in the type hierarchy of T (right?). https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3100: The sole exception to that would be a type $X$ that extended or implemented \code{Future$<X>$}. In that case, the result type is $X$ and no further unwrapping takes place. So, as stated above, I disagree with that choice. It also makes the runtime-semantics different from the static semantics, because at runtime, we don't distinguish between being exactly Future<T> or extending/implementing Future<T> (all runtime classes are subclasses of the abstract Future class). https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:5918: Otherwise, if the enclosing function $m$ is marked \ASYNC* (\ref{functions}) then the enclosing function may suspend. I still think it should be "must suspend". If that's too expensive, then it's probably because suspending should be cheaper (maybe not having to pause incoming streams from nesting await-for statements). When you put more than one value into a stream in the same microtask, they will be buffered. There is no need for this buffering - you know that there is an unpaused listener (otherwise the yield would block or exit), so you might as well yield to it immediately after adding the value. This is also consistent with the use of the word "yield" which is traditionally used to yield control. https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:5968: \item It is a dynamic error if the class of $o$ does not implement \code{Iterable}. Otherwise Still think this line should be dropped. If you can get "iterator" on the object and call "moveNext" and "current" on the result, it should work, just as for-in does. You don't *need* the "Iterable" check for anything, because you need to check every the result of every call anyway. Having this check, even in production mode, is pure overhead. https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:5969: \item The method \cd{iterator} is invoked upon $o$ returning an object $i$. Or it may throw - implementing an interface doesn't force you to do it correctly. Do we need to say that if it throws, the error is propagated (execution of s completes and throws the same error)? https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:5970: \item \label{moveNext} The \cd{moveNext} method of $i$ is invoked. If \cd{moveNext} returns \FALSE{} execution of $s$ is complete. Otherwise The result of moveNext should be subjected to boolean conversion, and if the result of that isn't true, execution is complete (and boolean conversion may throw in checked mode). https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:5970: \item \label{moveNext} The \cd{moveNext} method of $i$ is invoked. If \cd{moveNext} returns \FALSE{} execution of $s$ is complete. Otherwise Should we say the the moveNext method of i is invoked with zero arguments? https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:5971: \item An element $x$ is extracted from $i$ and $x$ is added to the iterable or stream associated with $m$. "extracted from" doesn't say how. What you do is to try to call the current" getter on i. If that throws, execution of $s$ is complete and throws the same error. Otherwise x is the result of calling the getter.
https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3100: The sole exception to that would be a type $X$ that extended or implemented \code{Future$<X>$}. In that case, the result type is $X$ and no further unwrapping takes place. On 2015/03/15 11:53:15, Lasse Reichstein Nielsen wrote: > So, as stated above, I disagree with that choice. > It also makes the runtime-semantics different from the static semantics, because > at runtime, we don't distinguish between being exactly Future<T> or > extending/implementing Future<T> (all runtime classes are subclasses of the > abstract Future class). Understood. What I wish we could say is: flatten(T) = flatten(S) if T is Future<S> or a subtype of Future<S>, and T otherwise. However there's a problem with that definition: it's possible to construct a legal class hierarchy for which it is undefined: class C implements Future<C> { ... } With this definition, flatten(C) = flatten(C) = flatten(C) = ..., which never terminates but does reach a fixed point. Here's an even more pathological example: class C<T> implements Future<C<C<T>>> { ... } With this definition, flatten(C) = flatten(C<C>) = flatten(C<C<C>>) = ..., which doesn't terminate and doesn't even reach a fixed point. So we need some way to ensure that the computation of flatten() terminates. I think Gilad's approach of unwrapping just once in the case where the type extends or implements Future is a reasonable compromise: it will do the right thing in just about every conceivable non-pathological case, and it's well-defined even in the pathological cases. (Perhaps we should consider adding a paragraph to the rationale section with one of these two pathological examples, to clarify why the more obvious definition of flatten() wasn't chosen).
https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:3095: is $(T_1 \ldots, T_n, [T_{n+1}$ $x_{n+1}, \ldots, T_{n+k}$ $x_{n+k}]) \rightarrow Future<flatten(T_0)>$, where $T_0$ is the static type of $e$ and $flatten(T) = flatten(S)$ if $T = Future<S>$ and $T \ne S$, and $T$ otherwise. On 2015/03/15 11:53:15, Lasse Reichstein Nielsen wrote: > I think we should flatten if T is a subtype of Future<S>, not just if it is > Future<S>. It is possible to have subtypes of Future in the language, and they > should also be flattened. Since you can't be a subtype of Future<S> and > Future<R> at the same time (S != R), there should be at most one Future<?> in > the type hierarchy of T (right?). The intent was to use subtyping, not equality, but somehow that got missed in the end. Howevr, I thnk there can be multiple Futures in the hierarchy. class A implements Future<A> class B extends A implements Future<B> No B <: Future<B> and B <: Future<A> (not to mention Future<Object> and Future<dynamic>). So strictly speaking, flatten would be ill-defined- flatten(B) could yield B, A, Object or dynamic. But we really want B. I'll try and reword to address all this. https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:5918: Otherwise, if the enclosing function $m$ is marked \ASYNC* (\ref{functions}) then the enclosing function may suspend. On 2015/03/15 11:53:15, Lasse Reichstein Nielsen wrote: > I still think it should be "must suspend". If that's too expensive, then it's > probably because suspending should be cheaper (maybe not having to pause > incoming streams from nesting await-for statements). > > When you put more than one value into a stream in the same microtask, they will > be buffered. There is no need for this buffering - you know that there is an > unpaused listener (otherwise the yield would block or exit), so you might as > well yield to it immediately after adding the value. > This is also consistent with the use of the word "yield" which is traditionally > used to yield control. Per discussion with Lars, we are not doing that. There are other ways to skin this cat and we won't overspecify them here. https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:5968: \item It is a dynamic error if the class of $o$ does not implement \code{Iterable}. Otherwise On 2015/03/15 11:53:15, Lasse Reichstein Nielsen wrote: > Still think this line should be dropped. If you can get "iterator" on the object > and call "moveNext" and "current" on the result, it should work, just as for-in > does. > You don't *need* the "Iterable" check for anything, because you need to check > every the result of every call anyway. Having this check, even in production > mode, is pure overhead. My goal in this CL is to deal with oversights and errors in the existing spec, not to change it. That is still being discussed with TC52, and may show up in another CL.
PTAL. https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:5970: \item \label{moveNext} The \cd{moveNext} method of $i$ is invoked. If \cd{moveNext} returns \FALSE{} execution of $s$ is complete. Otherwise On 2015/03/15 11:53:16, Lasse Reichstein Nielsen wrote: > Should we say the the moveNext method of i is invoked with zero arguments? Done. https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:5971: \item An element $x$ is extracted from $i$ and $x$ is added to the iterable or stream associated with $m$. On 2015/03/15 11:53:16, Lasse Reichstein Nielsen wrote: > "extracted from" doesn't say how. > > What you do is to try to call the current" getter on i. > If that throws, execution of $s$ is complete and throws the same error. > Otherwise x is the result of calling the getter. But for streams it's a bit different. So I abstracted form that.
https://codereview.chromium.org/1010433002/diff/40001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1010433002/diff/40001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3105: In any other circumstance, $flatten(T) = T$. I don't think this addresses the infinite regress problem in the example I gave yesterday where class C<T> implements Future<C<C<T>>>. AFAICT, the evaluation of flatten(C<dynamic>) goes as follows: C<dynamic> <: Future, so we must find a type S such that C<dynamic> << Future<S> and for all R, if C<dynamic> << Future<R> then S << R. The only S which satisfies this is C<C<dynamic>>. Since C<C<dynamic>> != C<dynamic> flatten(C<dynamic>) = flatten(C<C<dynamic>>). Now we must evaluate flatten(C<C<dynamic>>). By parallel logic to the above, this works out to flatten(C<C<dynamic>>) = flatten(C<C<C<dynamic>>>). And so on and so forth. https://codereview.chromium.org/1010433002/diff/40001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3114: \cd{\CLASS{} C$<$T$>$ \IMPLEMENTS{} C$<$C$<$T$>>$ \ldots } This is an illegal definition (a class cannot implement itself). I'm guessing you meant to quote the case I mentioned yesterday, which was: \cd{\CLASS{} C$<$T$>$ \IMPLEMENTS{} Future$<$C$<$C$<$T$>>>$ \ldots } As I mentioned above, I don't think we've successfully avoided an infinite regress in this case.
https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... docs/language/dartLangSpec.tex:5971: \item An element $x$ is extracted from $i$ and $x$ is added to the iterable or stream associated with $m$. It's very different for Stream, but this text only applies to sync*, so there is no need for abstraction. Maybe all this can be reduced to: yield* e is equivalent to for (var tmp in e as Iterable) { // "as Iterable" only so it throws if not. yield tmp; } or directly: var itl = e as Iterable; var itr = itl.iterator; while (itr.moveNext()) { yield itr.current; } just as it's done for the synchronous "for-in".
On 2015/03/17 07:11:21, Lasse Reichstein Nielsen wrote: > https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.tex > File docs/language/dartLangSpec.tex (right): > > https://codereview.chromium.org/1010433002/diff/1/docs/language/dartLangSpec.... > docs/language/dartLangSpec.tex:5971: \item An element $x$ is extracted from $i$ > and $x$ is added to the iterable or stream associated with $m$. > It's very different for Stream, but this text only applies to sync*, so there is > no need for abstraction. Ah yes, I split those apart. The leftover phrase 'iterator or stream' confused me. > > Maybe all this can be reduced to: > yield* e > is equivalent to > for (var tmp in e as Iterable) { // "as Iterable" only so it throws if not. > yield tmp; > } > or directly: > var itl = e as Iterable; > var itr = itl.iterator; > while (itr.moveNext()) { > yield itr.current; > } > just as it's done for the synchronous "for-in". That would be nice. The only difference I see is that using this formulation, we throw a CastError if we don't have an iterator. That is a bit of a tightening of the spec. What do we actually throw right now?
On 2015/03/17 01:51:52, Paul Berry wrote: > https://codereview.chromium.org/1010433002/diff/40001/docs/language/dartLangS... > File docs/language/dartLangSpec.tex (right): > > https://codereview.chromium.org/1010433002/diff/40001/docs/language/dartLangS... > docs/language/dartLangSpec.tex:3105: In any other circumstance, $flatten(T) = > T$. > I don't think this addresses the infinite regress problem in the example I gave > yesterday where class C<T> implements Future<C<C<T>>>. AFAICT, the evaluation > of flatten(C<dynamic>) goes as follows: > > C<dynamic> <: Future, so we must find a type S such that C<dynamic> << Future<S> > and for all R, if C<dynamic> << Future<R> then S << R. The only S which > satisfies this is C<C<dynamic>>. Since C<C<dynamic>> != C<dynamic> > flatten(C<dynamic>) = flatten(C<C<dynamic>>). > > Now we must evaluate flatten(C<C<dynamic>>). By parallel logic to the above, > this works out to flatten(C<C<dynamic>>) = flatten(C<C<C<dynamic>>>). And so on > and so forth. > > https://codereview.chromium.org/1010433002/diff/40001/docs/language/dartLangS... > docs/language/dartLangSpec.tex:3114: \cd{\CLASS{} C$<$T$>$ \IMPLEMENTS{} > C$<$C$<$T$>>$ \ldots } > This is an illegal definition (a class cannot implement itself). > > I'm guessing you meant to quote the case I mentioned yesterday, which was: > > \cd{\CLASS{} C$<$T$>$ \IMPLEMENTS{} Future$<$C$<$C$<$T$>>>$ \ldots } > > As I mentioned above, I don't think we've successfully avoided an infinite > regress in this case. Argh, yes. So one crude hack is to unwrap Future recursively, but to unwrap proper subtypes of future only once.
On 2015/03/18 00:16:52, gbracha wrote: > On 2015/03/17 01:51:52, Paul Berry wrote: > > > https://codereview.chromium.org/1010433002/diff/40001/docs/language/dartLangS... > > File docs/language/dartLangSpec.tex (right): > > > > > https://codereview.chromium.org/1010433002/diff/40001/docs/language/dartLangS... > > docs/language/dartLangSpec.tex:3105: In any other circumstance, $flatten(T) = > > T$. > > I don't think this addresses the infinite regress problem in the example I > gave > > yesterday where class C<T> implements Future<C<C<T>>>. AFAICT, the evaluation > > of flatten(C<dynamic>) goes as follows: > > > > C<dynamic> <: Future, so we must find a type S such that C<dynamic> << > Future<S> > > and for all R, if C<dynamic> << Future<R> then S << R. The only S which > > satisfies this is C<C<dynamic>>. Since C<C<dynamic>> != C<dynamic> > > flatten(C<dynamic>) = flatten(C<C<dynamic>>). > > > > Now we must evaluate flatten(C<C<dynamic>>). By parallel logic to the above, > > this works out to flatten(C<C<dynamic>>) = flatten(C<C<C<dynamic>>>). And so > on > > and so forth. > > > > > https://codereview.chromium.org/1010433002/diff/40001/docs/language/dartLangS... > > docs/language/dartLangSpec.tex:3114: \cd{\CLASS{} C$<$T$>$ \IMPLEMENTS{} > > C$<$C$<$T$>>$ \ldots } > > This is an illegal definition (a class cannot implement itself). > > > > I'm guessing you meant to quote the case I mentioned yesterday, which was: > > > > \cd{\CLASS{} C$<$T$>$ \IMPLEMENTS{} Future$<$C$<$C$<$T$>>>$ \ldots } > > > > As I mentioned above, I don't think we've successfully avoided an infinite > > regress in this case. > > Argh, yes. So one crude hack is to unwrap Future recursively, but to unwrap > proper subtypes of future only once. Personally I would be ok with that hack--I think it would cover the majority of reasonable use cases.
Ok, I've tried to addressed all comments. Tomorrow is the TC52 meeting; after that I have to finalize the draft within a few days. I will integrate proposals for tear-offs and null-aware operators into a different CL.
Revised definition of flatten() LGTM. Thanks! https://codereview.chromium.org/1010433002/diff/60001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1010433002/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3114: The exception to that would be a type $X$ that extended or implemented \code{Future}. In that case, the result type only one unwrapping takes place. As an example of why this is done, consider Nit: it looks like "the result type" was inserted into this paragraph by accident.
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as r44569 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1010433002/diff/80001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/1010433002/diff/80001/docs/language/dartLangS... docs/language/dartLangSpec.tex:5876: If $s_E$ is an asynchronous for loop (\ref{asynchronousFor-in}), its associated stream subscription is canceled. Furthermore, let $a_k$ be the set of asynchronous for loops and yield-each statements (\ref{yieldEach}) enclosing $s_b$ that are enclosed in $s_E , 1 \le k \le m$. The stream subscriptions associated with $a_j$ are canceled, $1 \le j \le m$. Just noticed: When the stream subscription is canceled, the call to subscription.cancel may return a future. If so, the implementation should wait for that future to complete before continuing with the code outside the loop. If the stream is created using async*, cancel always returns a future containing the result of the stream cleanup (execution of the finally blocks that the yield transferred control to when it noticed that it was canceled). Manually created streams may also return a future that tells you when clean-up is done. It is important to wait for that future before continuing. Also, when canceling multiple subscriptions, they should be canceled from the inside out, and each one should be waited on before the next one is canceled. Same goes for continue and the handlers hit by return/throw. |
