Make void-arrow-functions statically accept any expression type.

Make void-arrow-functions statically accept any expression type.

R=eernst@google.com

Committed: https://github.com/dart-lang/sdk/commit/26d6e3596d034024605ecf9dcfa489b544fa6245

[Lasse Reichstein Nielsen, 2017-05-11 13:05:57]

lrn@google.com changed reviewers:
+ eernst@google.com

[Lasse Reichstein Nielsen, 2017-05-11 13:05:58]

WDYT? Am I missing anything?

[Lasse Reichstein Nielsen, 2017-05-18 14:57:40]

PTAL

[eernst, 2017-05-19 09:48:14]

LGTM, with a couple of comments.

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:617: \item A block statement (\ref{blocks})
containing the statements  (\ref{statements}) executed by the function,
optionally marked with one of the modifiers: \ASYNC, \ASYNC* or \SYNC*.
Might as well fix all the stray double spaces (there's one at
`(\ref{statements})`).

I was surprised that the last sentence was moved from normative text to
commentary, but I can see that it is the right thing to do. Checking for
evidence elsewhere that a function body execution that falls through at the end
will return \NULL{}: 

Found \section{Return} saying `Executing .. return; .. returns with no value.`
and 

`Otherwise, if the execution completes normally or returns with no value, then
$e$ evaluates to \NULL.`

in \subsubsection{New} (about factory constructor execution), and

`If the execution completes normally or it returns without a value, the
invocation evaluates to \NULL (\ref{null}).`

in \subsection{Function Invocation}. Voila!

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:619: \commentary{
We haven't been re-categorizing rationale/commentary very much, but this looks
to me like a rationale: _Why_ must every function return a value? Then the
normative part should explain _that_ it does so, but we know that now. So why
not just keep it at `\rationale{}`?

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:624: \item of the form \code{async => $e$} or the
form \code{\ASYNC{} => $e$}, which both return the value of the expression $e$
as if by a \code{return $e$}. \rationale{The other modifiers do not apply here,
because they apply only to generators, discussed below, and generators do not
allow returning a value, values are added to the generated stream or iterable
using \YIELD{} instead.}
I believe it's a typo to have `async` in the first code snippet.

In this particular case, however, I actually think that the non-normative text
should be a `\commentary{}`: It explains why the normative text doesn't mention
\ASYNC* and \SYNC*, based on the rules of the language, but it does not give a
rationale for a language design decision.

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:631: or the function is asynchronous and
\code{Future<flatten{$R$}>}
`\code{Future<$flatten$($R$)>}`, for consistency with other occurrences of
"flatten".

There is one more occurrence of "flatten" in non-math-mode which can be found
with `It is a static type warning if the body of \$f\$ is marked \\ASYNC\{\}`,
in \subsection{Return}. We may again choose to fix this now that we noticed it.

It's a nice coincidence that we've eliminated the syntactic sugar approach and
then (1) we need to give the typing rules here because we don't get them by
desugaring, and (2) we needed special casing anyway. ;-)

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:3720: Execution a body of the form \code{async =>
$e$} is equivalent to executing a body of the form \code{async \{ return $e$;
\}}.
I think we should use `\ASYNC{}` to get the right font in code.

There is one occurrence of `async` in text. You can find it by a search for `and
the result of an async function will never have type`. If you don't think it's
too messy, we could fix that also in this CL, and then always use `\ASYNC{}` and
`\ASYNC*` whenever we refer to that concept.

[Lasse Reichstein Nielsen, 2017-05-22 14:18:50]

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:617: \item A block statement (\ref{blocks})
containing the statements  (\ref{statements}) executed by the function,
optionally marked with one of the modifiers: \ASYNC, \ASYNC* or \SYNC*.
Indeed, I wrote that into the specification of method termination because then
this line wouldn't be necessary.

I had a problem with this line: In a void function, it should give a warning to
have a `return null;` statement. It doesn't, arguably because the "added" return
statement is synthetic, but it's not really specified anywhere, and it's easier
if it just isn't there.

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:619: \commentary{
My reason for changing this is that this is really the rationale for adding
"return null;" - which the previous paragraph doesn't do any more. So, it's
"rationalizing" something that we haven't said, which is just confusing. That is
We didn't say *that* every function must return a value, so why is there
rationale saying *why*?

so I made it a comment. It is just reiterating rules from elsewhere to give the
reader a helpful reminder about how things work, which is comment-ish.

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:624: \item of the form \code{async => $e$} or the
form \code{\ASYNC{} => $e$}, which both return the value of the expression $e$
as if by a \code{return $e$}. \rationale{The other modifiers do not apply here,
because they apply only to generators, discussed below, and generators do not
allow returning a value, values are added to the generated stream or iterable
using \YIELD{} instead.}
Yes on typo. Fixed.

Changed to commentary.

I'm not sure what the original distinction between commentary and rationale was,
but this rational is explaining a "why" (why only those two forms), which might
have been the reasoning.

Changed "not allowed returning" to "not allowed to return" because I can
actually read that.

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:631: or the function is asynchronous and
\code{Future<flatten{$R$}>}
Flatten fixed, both three places. The other ones could also be found by
searching for "flatten(" :)

We should have a macro for flatten. I think I have one in another CL.

It's not a surprising coincidence - I made this change *because* I needed a
place to write the special-case for void, so it couldn't use the original
rewrite any more - since there was no way to distinguish an arrow-void-function
from a block-void-function at the return statement.

[Lasse Reichstein Nielsen, 2017-05-22 14:20:26]

Description was changed from

==========
Make void-arrow-functions statically accept any expression type.
==========

to

==========
Make void-arrow-functions statically accept any expression type.

R=eernst@google.com

Committed:
https://github.com/dart-lang/sdk/commit/26d6e3596d034024605ecf9dcfa489b544fa6245
==========

[Lasse Reichstein Nielsen, 2017-05-22 14:20:28]

Committed patchset #2 (id:20001) manually as
26d6e3596d034024605ecf9dcfa489b544fa6245 (presubmit successful).

[eernst, 2017-05-22 16:06:40]

As a reminder for a new CL, there are still some typos: 
'{..}' --> '(..)' near flatten, and a few occurrences of 
`async` --> `\ASYNC{}`.

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:617: \item A block statement (\ref{blocks})
containing the statements  (\ref{statements}) executed by the function,
optionally marked with one of the modifiers: \ASYNC, \ASYNC* or \SYNC*.
On 2017/05/22 14:18:49, Lasse Reichstein Nielsen wrote:
> Indeed, I wrote that into the specification of method termination because then
> this line wouldn't be necessary.
> 
> I had a problem with this line: In a void function, it should give a warning
to
> have a `return null;` statement. It doesn't, arguably because the "added"
return
> statement is synthetic, but it's not really specified anywhere, and it's
easier
> if it just isn't there.

Acknowledged.

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:619: \commentary{
On 2017/05/22 14:18:49, Lasse Reichstein Nielsen wrote:
> My reason for changing this is that this is really the rationale for adding
> "return null;" - which the previous paragraph doesn't do any more. So, it's
> "rationalizing" something that we haven't said, which is just confusing. That
is
> We didn't say *that* every function must return a value, so why is there
> rationale saying *why*?
> 
> so I made it a comment. It is just reiterating rules from elsewhere to give
the
> reader a helpful reminder about how things work, which is comment-ish.

Acknowledged.

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:624: \item of the form \code{async => $e$} or the
form \code{\ASYNC{} => $e$}, which both return the value of the expression $e$
as if by a \code{return $e$}. \rationale{The other modifiers do not apply here,
because they apply only to generators, discussed below, and generators do not
allow returning a value, values are added to the generated stream or iterable
using \YIELD{} instead.}
On 2017/05/22 14:18:49, Lasse Reichstein Nielsen wrote:
> Yes on typo. Fixed.
> 
> Changed to commentary.
> 
> I'm not sure what the original distinction between commentary and rationale
was,
> but this rational is explaining a "why" (why only those two forms), which
might
> have been the reasoning.
> 
> Changed "not allowed returning" to "not allowed to return" because I can
> actually read that.

Acknowledged.

https://codereview.chromium.org/2873313003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:631: or the function is asynchronous and
\code{Future<flatten{$R$}>}
On 2017/05/22 14:18:49, Lasse Reichstein Nielsen wrote:
> Flatten fixed, both three places. The other ones could also be found by
> searching for "flatten(" :)
> 
> We should have a macro for flatten. I think I have one in another CL.
> 
> It's not a surprising coincidence - I made this change *because* I needed a
> place to write the special-case for void, so it couldn't use the original
> rewrite any more - since there was no way to distinguish an
arrow-void-function
> from a block-void-function at the return statement.

Oops, there's still one typo: '{..}' should be changed to '(..)'.