Convert more pub code to use async/await.

Convert more pub code to use async/await.

R=nweiz@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39933

[Bob Nystrom, 2014-08-29 22:01:20]

rnystrom@google.com changed reviewers:
+ nweiz@google.com

[Bob Nystrom, 2014-08-29 22:01:21]

Moved some more pub code over to using async/await.

This is pretty much the last batch I can do before I get support for using
async/await in code reached by tests.

Some of the changes here are, I think, quite a bit better than the previous
form. :)

[nweiz, 2014-08-29 22:27:40]

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
File sdk/lib/_internal/pub/bin/pub.dart (right):

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:132: Future invokeCommand(String cacheDir,
ArgResults mainOptions) async {
I think we should omit the return type annotation for async methods that
conceptually return "Future<void>" for the same reason we omit the return type
annotation for setters: the return type is already implicit in the signature,
and writing it down provides no additional value.

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:178: return command.run(cacheDir,
mainOptions, options);
I also think we should prefer "await" to "return" if we aren't actually
returning a value. Not only does that better communicate that the return value
of e.g. command.run is unused, it also makes it easier to add additional logic
later in the function.

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/executable.dart (right):

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:154: // TODO(rnystrom): Use
cascade here when async_await compiler supports it.
Link this to an issue.

[Bob Nystrom, 2014-08-29 23:03:20]

Thanks!

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
File sdk/lib/_internal/pub/bin/pub.dart (right):

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:132: Future invokeCommand(String cacheDir,
ArgResults mainOptions) async {
On 2014/08/29 22:27:39, nweiz wrote:
> I think we should omit the return type annotation for async methods that
> conceptually return "Future<void>" for the same reason we omit the return type
> annotation for setters: the return type is already implicit in the signature,
> and writing it down provides no additional value.

Actually, that's not why we omit it for setters. The return type of a setter is
always *void*, not inferred from the parameter type. There are only two allowed
return types for a setter: "void" or nothing. Anything else produces a warning.

A set expression invokes the setter, but does not return the result of the
setter. It returns the RHS. The return value of the setter method is discarded.

In this case, I think we should keep the return type as Future. The fact that
the body uses async is an implementation detail. It won't be (and shouldn't be)
processed by stuff like dartdoc. If we get a warning if you call a function that
returns a Future in statement position (which I think is very important), that
warning will be based on the annotated type, and won't take the async modifier
into account. (I realize that the warning *could* take that into account, but
good luck selling the language team on that.)

I think it's best to think of "async" as an implementation detail of the body. A
method like { return 123; } also has a trivially inferrable type, but we don't
mind annotating that.

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:178: return command.run(cacheDir,
mainOptions, options);
On 2014/08/29 22:27:39, nweiz wrote:
> I also think we should prefer "await" to "return" if we aren't actually
> returning a value. Not only does that better communicate that the return value
> of e.g. command.run is unused, it also makes it easier to add additional logic
> later in the function.

Done and agreed. When converting the vanilla code over, it isn't always easy to
see which calls were producing values, so I got some places wrong.

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/executable.dart (right):

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:154: // TODO(rnystrom): Use
cascade here when async_await compiler supports it.
On 2014/08/29 22:27:39, nweiz wrote:
> Link this to an issue.

Done.

[nweiz, 2014-08-29 23:30:41]

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
File sdk/lib/_internal/pub/bin/pub.dart (right):

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:132: Future invokeCommand(String cacheDir,
ArgResults mainOptions) async {
On 2014/08/29 23:03:20, Bob Nystrom wrote:
> On 2014/08/29 22:27:39, nweiz wrote:
> > I think we should omit the return type annotation for async methods that
> > conceptually return "Future<void>" for the same reason we omit the return
type
> > annotation for setters: the return type is already implicit in the
signature,
> > and writing it down provides no additional value.
> 
> Actually, that's not why we omit it for setters. The return type of a setter
is
> always *void*, not inferred from the parameter type. There are only two
allowed
> return types for a setter: "void" or nothing. Anything else produces a
warning.
> 
> A set expression invokes the setter, but does not return the result of the
> setter. It returns the RHS. The return value of the setter method is
discarded.
> 
> In this case, I think we should keep the return type as Future. The fact that
> the body uses async is an implementation detail. It won't be (and shouldn't
be)
> processed by stuff like dartdoc. If we get a warning if you call a function
that
> returns a Future in statement position (which I think is very important), that
> warning will be based on the annotated type, and won't take the async modifier
> into account. (I realize that the warning *could* take that into account, but
> good luck selling the language team on that.)
> 
> I think it's best to think of "async" as an implementation detail of the body.
A
> method like { return 123; } also has a trivially inferrable type, but we don't
> mind annotating that.

Despite the fact that the parser considers "async" to be attached to the body,
it looks like it's part of the signature and a human reader is going to read it
as such. We should optimize for humans first and let the tools follow second,
and to any human it's clear from nothing more than the declaration line of the
function that it's going to return a future whether it's annotated or not.

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/bi...
File sdk/lib/_internal/pub/bin/pub.dart (right):

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/bi...
sdk/lib/_internal/pub/bin/pub.dart:194: return flushThenExit(exit_codes.USAGE);
"return" -> "await"

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/command/cache_repair.dart (right):

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_repair.dart:49: if (failures > 0)
return flushThenExit(exit_codes.UNAVAILABLE);
"return" -> "await"

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/serve.dart:131: return _completer.future;
"return" -> "await"

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/executable.dart (right):

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/executable.dart:141: Future
_runCachedExecutable(Entrypoint entrypoint, String snapshotPath,
This should be typed as "Future<int>" (my bad).

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/global_packages.dart (right):

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:72: return _installInCache(
"return" -> "await"

[Bob Nystrom, 2014-09-03 23:44:17]

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
File sdk/lib/_internal/pub/bin/pub.dart (right):

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:132: Future invokeCommand(String cacheDir,
ArgResults mainOptions) async {
On 2014/08/29 23:30:41, nweiz wrote:
> On 2014/08/29 23:03:20, Bob Nystrom wrote:
> > On 2014/08/29 22:27:39, nweiz wrote:
> > > I think we should omit the return type annotation for async methods that
> > > conceptually return "Future<void>" for the same reason we omit the return
> type
> > > annotation for setters: the return type is already implicit in the
> signature,
> > > and writing it down provides no additional value.
> > 
> > Actually, that's not why we omit it for setters. The return type of a setter
> is
> > always *void*, not inferred from the parameter type. There are only two
> allowed
> > return types for a setter: "void" or nothing. Anything else produces a
> warning.
> > 
> > A set expression invokes the setter, but does not return the result of the
> > setter. It returns the RHS. The return value of the setter method is
> discarded.
> > 
> > In this case, I think we should keep the return type as Future. The fact
that
> > the body uses async is an implementation detail. It won't be (and shouldn't
> be)
> > processed by stuff like dartdoc. If we get a warning if you call a function
> that
> > returns a Future in statement position (which I think is very important),
that
> > warning will be based on the annotated type, and won't take the async
modifier
> > into account. (I realize that the warning *could* take that into account,
but
> > good luck selling the language team on that.)
> > 
> > I think it's best to think of "async" as an implementation detail of the
body.
> A
> > method like { return 123; } also has a trivially inferrable type, but we
don't
> > mind annotating that.
> 
> Despite the fact that the parser considers "async" to be attached to the body,
> it looks like it's part of the signature and a human reader is going to read
it
> as such. We should optimize for humans first and let the tools follow second,
> and to any human it's clear from nothing more than the declaration line of the
> function that it's going to return a future whether it's annotated or not.

I don't think it's part of the signature and I'm quite certain our tools won't
be interpreting it as such. Gilad has been very clear in multiple places that
"async" is an implementation detail and is unrelated to the static return type.

What I can do is change this to Future<Null>, which makes it clear it doesn't
complete to a value. Thoughts?

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/bi...
File sdk/lib/_internal/pub/bin/pub.dart (right):

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/bi...
sdk/lib/_internal/pub/bin/pub.dart:194: return flushThenExit(exit_codes.USAGE);
On 2014/08/29 23:30:41, nweiz wrote:
> "return" -> "await"

Done.

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/command/cache_repair.dart (right):

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_repair.dart:49: if (failures > 0)
return flushThenExit(exit_codes.UNAVAILABLE);
On 2014/08/29 23:30:41, nweiz wrote:
> "return" -> "await"

Done.

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/serve.dart:131: return _completer.future;
On 2014/08/29 23:30:41, nweiz wrote:
> "return" -> "await"

Done.

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/executable.dart (right):

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/executable.dart:141: Future
_runCachedExecutable(Entrypoint entrypoint, String snapshotPath,
On 2014/08/29 23:30:41, nweiz wrote:
> This should be typed as "Future<int>" (my bad).

Done.

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/global_packages.dart (right):

https://codereview.chromium.org/521643005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:72: return _installInCache(
On 2014/08/29 23:30:41, nweiz wrote:
> "return" -> "await"

Done.

[Bob Nystrom, 2014-09-05 18:27:27]

Friendly ping!

[nweiz, 2014-09-05 18:57:38]

lgtm

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
File sdk/lib/_internal/pub/bin/pub.dart (right):

https://codereview.chromium.org/521643005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:132: Future invokeCommand(String cacheDir,
ArgResults mainOptions) async {
On 2014/09/03 23:44:16, Bob Nystrom wrote:
> On 2014/08/29 23:30:41, nweiz wrote:
> > On 2014/08/29 23:03:20, Bob Nystrom wrote:
> > > On 2014/08/29 22:27:39, nweiz wrote:
> > > > I think we should omit the return type annotation for async methods that
> > > > conceptually return "Future<void>" for the same reason we omit the
return
> > type
> > > > annotation for setters: the return type is already implicit in the
> > signature,
> > > > and writing it down provides no additional value.
> > > 
> > > Actually, that's not why we omit it for setters. The return type of a
setter
> > is
> > > always *void*, not inferred from the parameter type. There are only two
> > allowed
> > > return types for a setter: "void" or nothing. Anything else produces a
> > warning.
> > > 
> > > A set expression invokes the setter, but does not return the result of the
> > > setter. It returns the RHS. The return value of the setter method is
> > discarded.
> > > 
> > > In this case, I think we should keep the return type as Future. The fact
> that
> > > the body uses async is an implementation detail. It won't be (and
shouldn't
> > be)
> > > processed by stuff like dartdoc. If we get a warning if you call a
function
> > that
> > > returns a Future in statement position (which I think is very important),
> that
> > > warning will be based on the annotated type, and won't take the async
> modifier
> > > into account. (I realize that the warning *could* take that into account,
> but
> > > good luck selling the language team on that.)
> > > 
> > > I think it's best to think of "async" as an implementation detail of the
> body.
> > A
> > > method like { return 123; } also has a trivially inferrable type, but we
> don't
> > > mind annotating that.
> > 
> > Despite the fact that the parser considers "async" to be attached to the
body,
> > it looks like it's part of the signature and a human reader is going to read
> it
> > as such. We should optimize for humans first and let the tools follow
second,
> > and to any human it's clear from nothing more than the declaration line of
the
> > function that it's going to return a future whether it's annotated or not.
> 
> I don't think it's part of the signature and I'm quite certain our tools won't
> be interpreting it as such. Gilad has been very clear in multiple places that
> "async" is an implementation detail and is unrelated to the static return
type.

I'm not saying it's literally part of the signature, I'm saying it looks like it
is and that's what matters for reading the code. Gilad can say whatever he wants
but ultimately readers of the code read the syntax and not his emails.

That said, I don't want to block the patch on this debate. If you feel strongly
that we should include both declarations, go for it.

> What I can do is change this to Future<Null>, which makes it clear it doesn't
> complete to a value. Thoughts?

I think just "Future" is redundant; I don't want to add *more* text.

[Bob Nystrom, 2014-09-05 21:25:11]

Committed patchset #3 (id:40001) manually as 39933 (presubmit successful).