Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify yield* and await for in more detail. #1527

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Specify yield* and await for in more detail. #1527

wants to merge 1 commit into from

Conversation

lrhn
Copy link
Member

@lrhn lrhn commented Mar 16, 2021

No description provided.

@lrhn lrhn requested a review from eernstg March 16, 2021 13:56
@google-cla google-cla bot added the cla: yes label Mar 16, 2021
@@ -16880,13 +16880,13 @@ \subsection{If}

\rationale{%
Under reasonable scope rules such code is problematic.
If we assume that \code{v} is declared
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems my "remove trailing spaces on save" has caught some dangling spaces.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR might then need to be rebased: The current version of dartLangSpec.tex (34c4e1a) does not contain any trailing spaces.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing spaces are bad, m'kay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly!

I'm saying that there are no trailing spaces in dartLangSpec.tex today. In general, there shouldn't be many commits where there are any trailing spaces, because I usually remember to remove them, but I can see that this PR starts from a commit where there are quite a few trailing spaces.

But this means that removing trailing spaces in this PR will not remove any trailing spaces from the result of landing this PR (they're gone already), it will just make the merging operation a bit more complex.

I was worried that this kind of "fix something that has been fixed" change could in some situations give rise to a reversal: A line here and there could suddenly be edited back to an earlier version, because it has been changed in this PR, and it contains the old text in this PR (minus that trailing space). So git thinks that the old text is what we really want, and replaces the current text by the old text from this PR.

That's the reason why I suggested doing a rebase on this PR.

@lrhn
Copy link
Member Author

lrhn commented May 1, 2021

PTAL

@eernstg
Copy link
Member

eernstg commented May 3, 2021

Oops, I didn't see this one. Looking at it now.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, surely this is a considerable improvement!

However, we do now mention several times that a specific behavior is undefined. I suspect that said cases are rare anomalies (generally associated with silly/wrong implementations of Stream or similar classes), and they will not occur when using a platform provided stream, stream subscription, etc.

If this is true then I think it would be helpful to mention this each time it comes up. I added comments about this along the way.

@@ -104,7 +104,7 @@
% - Add requirement that the iterator of a for-in statement must have
% type `Iterator`.
% - Clarify which constructors are covered by the section 'Constant
% Constructors' and removed confusing redundancy in definiton of
% Constructors' and removed confusing redundancy in definition of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well fix and removed --> , and remove.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted.

@@ -16880,13 +16880,13 @@ \subsection{If}

\rationale{%
Under reasonable scope rules such code is problematic.
If we assume that \code{v} is declared
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR might then need to be rebased: The current version of dartLangSpec.tex (34c4e1a) does not contain any trailing spaces.

@@ -18083,7 +18083,8 @@ \subsection{Yield-Each}
if the class of $o$ is not a subtype of \code{Iterable<$T_f$>}.
Otherwise
\item
The method \code{iterator} is invoked upon $o$ returning an object $i$.
The getter \code{iterator} is invoked upon $o$ returning an object $i$.
Otherwise
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the word Otherwise actually be added here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the phrase is invoked upon is quite unusual in this spec, and it seems likely that I'm not the only reader who would need to look at it twice in order to understand what it means.

Wouldn't it be more like other parts of the specification if we use Evaluate \code{$o$.iterator} to an object $i$?

(We do say it like Evaluate \code{$v$.iterator}, where $v$ is a fresh variable bound to $o$. in many locations, but we do also have the slight abuse of notation where a symbol is at first denoting an object, and then considered to be a variable. Search for, e.g., 'evaluation of $ee$ is equivalent to the method invocation'.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should follow the norm here.
I was probably trying to avoid introducing new syntax (which I think is a bad choice in general, semantics should be explained at the semantic level without dipping back into syntax). But that's a much larger rewrite to avoid that.

@@ -18083,7 +18083,8 @@ \subsection{Yield-Each}
if the class of $o$ is not a subtype of \code{Iterable<$T_f$>}.
Otherwise
\item
The method \code{iterator} is invoked upon $o$ returning an object $i$.
The getter \code{iterator} is invoked upon $o$ returning an object $i$.
Otherwise
\item
\label{moveNext} The \code{moveNext} method of $i$ is invoked on it
with no arguments.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And Evaluate \code{$u$.moveNext()}, where $u$ is a fresh variable bound to $i$., or Evaluate \code{$i$.moveNext()}.

then execution of $m$ is suspended until $u$ is resumed or canceled.
If the stream associated with $m$ becomes paused,
then $u$ is paused by executing \code{$v$.pause();}.
It is unspecified what happens if this execution throws.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be a friendly gesture to the practical developer to mention in commentary that a Stream whose pause throws is an unexpected anomaly, and a platform provided Stream will never do so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

\code{$p$.listen} and let $v$ be a fresh variable bound to $u$.
\item
If the stream associated with $m$ is paused, then $u$ is immediately
paused as if by invoking \code{$v$.pause} with no arguments.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it throws?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default semantic fallback is that if some sub-part of the semantics of a construct throws, so does the construct itself unless otherwise stated. In this case, nothing otherwise is stated, so if $v$.pause throws, so does the await for body evaluation. (Which then does say what happens in that case - it cancels the subscription and rethrows).

Now, should it work like that? Probably yes.
If pause throws ... the stream probably isn't paused. Something is deeply wrong with that stream, so it's better to exit that loop immediately, and stop handling events from the stream entirely. (We should ignore any events after calling cancel).

It's possible to be obnoxious about the exiting. Say:

label: try {
  yield whatnot;
} finally {
  break label;
}

This will swallow any control flow trying to leave the try block and continue unabated.
We can't entirely control what happens after that, if the stream subscription acts maliciously.
(And that's why C# disallowed control flow in finally blocks!)

paused as if by invoking \code{$v$.pause} with no arguments.
\item{}
If the stream associated with $m$ has been cancelled, then
$u$ is cancelled by executing \code{await $v$.cancel();}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it more straightforward to say evaluating \code{await $v$.cancel()}. It doesn't bring much value to have that (visually clunky) semicolon. If we wish to make the point that if it evaluates to an object then that object is ignored then we can say that explicitly, but it seems sufficient to actually ignore it by not mentioning it. ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Evaluating an expression is cleaner.

If the stream associated with $m$ has been cancelled, then
$u$ is cancelled by executing \code{await $v$.cancel();}.
If this execution does not throw, the execution of $s$
completes by returning without a value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like we could have commentary saying that it is an unexpected anomaly if await $v$.cancel() throws, it would only occur if listen or pause has side-effects where the stream is cancelled, and this will not occur with a platform provided stream.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, cancel is one of the functions which can throw. It's not great when it does, but it's allowed and should be handled gracefully (which means propagating the error).

If the stream $u$ associated with $m$ has been paused,
then execution of $m$ is suspended until $u$ is resumed or canceled.
If the stream associated with $m$ becomes paused,
then $u$ is paused by executing \code{$v$.pause();}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to line 18138, this could be evaluating \code{$v$.pause()}.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

the stream execution of $s$ returns without an object
(\ref{statementCompletion}).
If the stream associated with $m$ is resumed after being paused,
then $u$ is resumed as by executing \code{$v$.resume();}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could again be evaluating \code{$v$.resume()}.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

Copy link
Member Author

@lrhn lrhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good points.

\item
The $o$ stream is listened to by invoking \code{$v_o$.listen}
with system provided arguments,
where $v_o$ is a fresh variable referencing the stream $o$.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Much better, yes.

\code{$p$.listen} and let $v$ be a fresh variable bound to $u$.
\item
If the stream associated with $m$ is paused, then $u$ is immediately
paused as if by invoking \code{$v$.pause} with no arguments.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default semantic fallback is that if some sub-part of the semantics of a construct throws, so does the construct itself unless otherwise stated. In this case, nothing otherwise is stated, so if $v$.pause throws, so does the await for body evaluation. (Which then does say what happens in that case - it cancels the subscription and rethrows).

Now, should it work like that? Probably yes.
If pause throws ... the stream probably isn't paused. Something is deeply wrong with that stream, so it's better to exit that loop immediately, and stop handling events from the stream entirely. (We should ignore any events after calling cancel).

It's possible to be obnoxious about the exiting. Say:

label: try {
  yield whatnot;
} finally {
  break label;
}

This will swallow any control flow trying to leave the try block and continue unabated.
We can't entirely control what happens after that, if the stream subscription acts maliciously.
(And that's why C# disallowed control flow in finally blocks!)

paused as if by invoking \code{$v$.pause} with no arguments.
\item{}
If the stream associated with $m$ has been cancelled, then
$u$ is cancelled by executing \code{await $v$.cancel();}.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Evaluating an expression is cleaner.

If the stream associated with $m$ has been cancelled, then
$u$ is cancelled by executing \code{await $v$.cancel();}.
If this execution does not throw, the execution of $s$
completes by returning without a value.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, cancel is one of the functions which can throw. It's not great when it does, but it's allowed and should be handled gracefully (which means propagating the error).

If the stream $u$ associated with $m$ has been paused,
then execution of $m$ is suspended until $u$ is resumed or canceled.
If the stream associated with $m$ becomes paused,
then $u$ is paused by executing \code{$v$.pause();}.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

% The error can be passed to \code{Zone.current.handleUncaughtError}.
\item
If the stream associated with $m$ is cancelled,
then $u$ is also cancelled as by executing \code{await $v$.cancel();}.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preferably, the code calling stream.cancel will get the future. That assumes that the cancel comes from cancelling a StreamSubscription, which ... it does, but we haven't been explicit about that.

Maybe I should go back to the drawing board and be explicit about everything.

Calling listen on the stream returned by the async* function returns a stream subscription.
The stream subscription doesn't "become paused", rather someone calls pause on it. That means we have a receiver for any synchronous errors happening during that call. Same for resume and cancel.

@@ -104,7 +104,7 @@
% - Add requirement that the iterator of a for-in statement must have
% type `Iterator`.
% - Clarify which constructors are covered by the section 'Constant
% Constructors' and removed confusing redundancy in definiton of
% Constructors' and removed confusing redundancy in definition of
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted.

@@ -16880,13 +16880,13 @@ \subsection{If}

\rationale{%
Under reasonable scope rules such code is problematic.
If we assume that \code{v} is declared
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing spaces are bad, m'kay?

@@ -18083,7 +18083,8 @@ \subsection{Yield-Each}
if the class of $o$ is not a subtype of \code{Iterable<$T_f$>}.
Otherwise
\item
The method \code{iterator} is invoked upon $o$ returning an object $i$.
The getter \code{iterator} is invoked upon $o$ returning an object $i$.
Otherwise
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should follow the norm here.
I was probably trying to avoid introducing new syntax (which I think is a bad choice in general, semantics should be explained at the semantic level without dipping back into syntax). But that's a much larger rewrite to avoid that.

not begin to listen to the stream $o$.}
Otherwise
\item
The $o$ stream is listened to by invoking \code{$v_o$.listen}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a couple of extra comments, especially about the trailing spaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants