Introduce await

Introduce await.

This CL adds basic infrastructure needed for awaitable expressions. Implementation of continuations is not part of this CL.

Expressions containing ``await'' are transformed into a series of operations on intermediates, effectively getting rid
of the temporary expression stack. Currently only expressions evaluating to an actual value (read: non-future) are supported.

Also, not all kinds of statements support awaitable expressions yet.

Missing:
 * Capturing all (needed) variables
 * Continuations (connecting a preamble with await statements; re-adding the closure to the run queue)

BUG=
R=hausner@google.com

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

[Michael Lippautz (Google), 2014-08-07 00:18:56]

Please take a first look.

As stated above not all statements are awaitable yet. (Actually, only variable
declarations are.)

https://codereview.chromium.org/447003003/diff/40001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/447003003/diff/40001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:506: // TODO(mlippautz): Check initializers and
their temps.
#initializers and #temps should 0 here?

https://codereview.chromium.org/447003003/diff/40001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:516: void
AwaitTransformer::VisitThrowNode(ThrowNode* node) {
``throws'' are not actual expressions, but the parser can insert throw nodes,
right?

https://codereview.chromium.org/447003003/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/447003003/diff/40001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:10855: } else if (IsLiteral("await")) {
Let's see whether this will get a keyword

[hausner, 2014-08-07 21:48:44]

First set of comments. I like how how you keep the additional complexity out of
the parser as much as possible.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_printer.cc
File runtime/vm/ast_printer.cc (right):

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_printer.c...
runtime/vm/ast_printer.cc:155: void AstPrinter::VisitAwaitNode(AwaitNode* node)
{
You can call VisitGenericAstNode(node) here instead of duplicating this pattern.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:10: 
It would be nice to write up a few lines of comment to describe how the
AwaitTransformer generally works.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:11: #define I Isolate::Current()
It might be good to cache the current isolate in the AwaitTransformer and make
this macro access the cached value. We do that in the parser as well.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:51: "%s%" Pd "",
Symbols::AwaitTempVarPrefix().ToCString(), temp_cnt_));
I'm not sure it makes sense to create a symbol for the prefix. You have to
create a new symbol for each variable anyway. Also, converting the symbol back
to a C string allocates a buffer in the current zone and copies the characters
each time.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:76: result_ = new (I)
LiteralNode(node->token_pos(), node->literal());
Could you not just return the node given as a parameter, instead of allocating a
new one?

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:96: LocalVariable* async_op =
preamble_->scope()->LookupVariable(
LookupVariable() will search outer scopes, which is probably not what you want.
Even though the preamble scope may not have an outer scope (not sure), you may
want to use scope()->LocalLookupVariable(), which only searches that particular
scope.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:116: is_future_branch->Add(new (I)
ReturnNode(Scanner::kNoSourcePos));
Instead of silently return null, maybe throw an exception for now?

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:118: String::Handle(String::New("Future"))));
We should probably have a predefined symbol for "Future".

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:142: AstNode*
AwaitTransformer::LazyTransform(const Token::Kind kind,
How about renaming this parameter to logical_op or something more descriptive?

Also, can you add a comment what this function does?

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:173: AstNode* new_right = new (I) LiteralNode(
Why do you allocate a node here when new_right gets overwritten in both
if-branches below?

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:196: AstNode* new_right = new (I) LiteralNode(
See note above.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:242: // ::= (<condition>) ? <true-branch> :
<false-branch>
Nit: add empty line.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.h (right):

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.h:45: intptr_t temp_cnt_;
Can we make temp_cnt_ an int instead of intptr_t? The latter is overkill and
slightly more ugly when having to use the value in a printf format string.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:2981: params.num_optional_parameters++;
Why is this parameter optional? The caller will always pass a parameter, even if
it's null.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:8453: AstNode* Parser::ParseAwaitableExpr(bool
require_compiletime_const,
As discussed offline, I believe all expressions are "awaitable". So this code
may belong in ParseExpr.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:8478: AstNode* Parser::ParseAwaitExpr() {
Could maybe inline this code in ParsePrimary.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.h
File runtime/vm/parser.h (right):

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.h#newc...
runtime/vm/parser.h:144: bool seen_await() const { return seen_await_expr_; }
I'd prefer have_seen_await() and have_seen_await_expr_

https://codereview.chromium.org/447003003/diff/60001/tests/language/await_tes...
File tests/language/await_test.dart (right):

https://codereview.chromium.org/447003003/diff/60001/tests/language/await_tes...
tests/language/await_test.dart:108: var c = (a || b) || await dummy();
dummy() returns 1, so I don't expect this to pass in checked mode.

[Michael Lippautz (Google), 2014-08-08 18:12:18]

Thanks a lot!

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_printer.cc
File runtime/vm/ast_printer.cc (right):

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_printer.c...
runtime/vm/ast_printer.cc:155: void AstPrinter::VisitAwaitNode(AwaitNode* node)
{
On 2014/08/07 21:48:41, hausner wrote:
> You can call VisitGenericAstNode(node) here instead of duplicating this
pattern.

Done.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:10: 
On 2014/08/07 21:48:43, hausner wrote:
> It would be nice to write up a few lines of comment to describe how the
> AwaitTransformer generally works.

Done.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:11: #define I Isolate::Current()
On 2014/08/07 21:48:42, hausner wrote:
> It might be good to cache the current isolate in the AwaitTransformer and make
> this macro access the cached value. We do that in the parser as well.

Done.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:51: "%s%" Pd "",
Symbols::AwaitTempVarPrefix().ToCString(), temp_cnt_));
On 2014/08/07 21:48:41, hausner wrote:
> I'm not sure it makes sense to create a symbol for the prefix. You have to
> create a new symbol for each variable anyway. Also, converting the symbol back
> to a C string allocates a buffer in the current zone and copies the characters
> each time.

Done.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:76: result_ = new (I)
LiteralNode(node->token_pos(), node->literal());
On 2014/08/07 21:48:43, hausner wrote:
> Could you not just return the node given as a parameter, instead of allocating
a
> new one?

Done.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:96: LocalVariable* async_op =
preamble_->scope()->LookupVariable(
On 2014/08/07 21:48:42, hausner wrote:
> LookupVariable() will search outer scopes, which is probably not what you
want.
> Even though the preamble scope may not have an outer scope (not sure), you may
> want to use scope()->LocalLookupVariable(), which only searches that
particular
> scope.

I think LookupVariable() is right here.

The scope setup looks roughly like that:

  foo() async {
    var :async_op = (:result_param=null) {
      <preamle>;
    }
  }

So we search up from <preamble> to the outer (async) function and find
:async_op.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:116: is_future_branch->Add(new (I)
ReturnNode(Scanner::kNoSourcePos));
On 2014/08/07 21:48:42, hausner wrote:
> Instead of silently return null, maybe throw an exception for now?

Done. I am not sure why the literal needs to be a symbol though.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:118: String::Handle(String::New("Future"))));
On 2014/08/07 21:48:42, hausner wrote:
> We should probably have a predefined symbol for "Future".

Done. We already had a symbol.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:142: AstNode*
AwaitTransformer::LazyTransform(const Token::Kind kind,
On 2014/08/07 21:48:42, hausner wrote:
> How about renaming this parameter to logical_op or something more descriptive?
> 
> Also, can you add a comment what this function does?

Done.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:173: AstNode* new_right = new (I) LiteralNode(
On 2014/08/07 21:48:42, hausner wrote:
> Why do you allocate a node here when new_right gets overwritten in both
> if-branches below?

Not needed. Done.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:196: AstNode* new_right = new (I) LiteralNode(
On 2014/08/07 21:48:42, hausner wrote:
> See note above.

Done.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:242: // ::= (<condition>) ? <true-branch> :
<false-branch>
On 2014/08/07 21:48:42, hausner wrote:
> Nit: add empty line.

Done.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.h (right):

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.h:45: intptr_t temp_cnt_;
On 2014/08/07 21:48:43, hausner wrote:
> Can we make temp_cnt_ an int instead of intptr_t? The latter is overkill and
> slightly more ugly when having to use the value in a printf format string.

Done.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:2981: params.num_optional_parameters++;
On 2014/08/07 21:48:43, hausner wrote:
> Why is this parameter optional? The caller will always pass a parameter, even
if
> it's null.

Maybe I am missing something, but the first time we invoke :async_op we do not
have a parameter.

  foo() async {
    ...
    var c = new Completer();
    var :async_op = (:result_param = null) {
      ...
    }
    new Future(:async_op); // HERE
    return c.future;
  }

The future constructor expects a computation without parameter.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:8453: AstNode* Parser::ParseAwaitableExpr(bool
require_compiletime_const,
On 2014/08/07 21:48:43, hausner wrote:
> As discussed offline, I believe all expressions are "awaitable". So this code
> may belong in ParseExpr.

Again discussed offline :)

Example:

  var x = ((await foo()) + (await bar()))

ParseExpr is called 3 (?) times for the right hand side, i.e., for (await
foo()), (await bar()), and the binary op. We need to transform this expression
as a whole though.

If we would transform the subexpressions, we would end up with something alike:

  // preamble for await foo()
  t_1 = foo();
  <continuation code>
  // preamble for await bar()
  t_1 = bar();
  <continuation code>
  // We did overwrite t_1 already, so we cannot continue?
  t2 = ?
  
The idea is to keep ParseAwaitableExpr() and insert it at the appropriate
positions on statement level.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:8478: AstNode* Parser::ParseAwaitExpr() {
On 2014/08/07 21:48:43, hausner wrote:
> Could maybe inline this code in ParsePrimary.

Done.

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.h
File runtime/vm/parser.h (right):

https://codereview.chromium.org/447003003/diff/60001/runtime/vm/parser.h#newc...
runtime/vm/parser.h:144: bool seen_await() const { return seen_await_expr_; }
On 2014/08/07 21:48:43, hausner wrote:
> I'd prefer have_seen_await() and have_seen_await_expr_

Done.

https://codereview.chromium.org/447003003/diff/60001/tests/language/await_tes...
File tests/language/await_test.dart (right):

https://codereview.chromium.org/447003003/diff/60001/tests/language/await_tes...
tests/language/await_test.dart:108: var c = (a || b) || await dummy();
On 2014/08/07 21:48:43, hausner wrote:
> dummy() returns 1, so I don't expect this to pass in checked mode.

b is true, so await dummy() will never get evaluated (because of lazy
evaluation). So this also works with --checked.

[Michael Lippautz (Google), 2014-08-13 18:11:07]

PTAL

I updated the CL and got rid of stuff we will not need anymore.

Current plan is to create the preamble on the fly (while processing awaits) in
the flow graph builder. 

So there will be two kinds of await nodes:
Kind (1) will reach the transformer, be translated into a future check, and
create a node of kind (2)
Kind (2) will reach the flow graph builder and make sure that a branch in the
preamble connects to this fragment. (GotoIntr/JoinInstr)

+srdjan

[srdjan, 2014-08-13 18:41:07]

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:154: expr_(expr) {
all in one line ?

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:51: const String& cnt_str = String::Handle(
I,

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:128: const Class& cls =
Class::Handle(library_.LookupClass(Symbols::Future()));
I,

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:174: SequenceNode* eval = new SequenceNode(
new (I)

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:200: if (node->kind() == Token::kAND ||
node->kind() == Token::kOR) {
Use more parentheses.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:221: if (node->kind() == Token::kAND ||
node->kind() == Token::kOR) {
Use more parentheses

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:261: 
two empty lines

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:265: // TODO(mlippautz): Optimize variable
assingments and branching.
s/assingments/assignments/

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.h (right):

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:15: // Tranlate an AstNode containing an expression
(that contains one or more
s/Tranlate/Translate/

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:16: // awaits) into a sequential representation
that evaluation one subexpression
fix sentence.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:20: // expression. The evaluation is split among a
so called preamble and the
s/so called/so-called/

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:23: // Example (minimalisitc):
s/minimalisitc/minimalistic/

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:41: explicit AwaitTransformer(SequenceNode*
preamble,
No explicit

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8449: Symbols::AsyncOperation(), true)) {
indent +4 spaces

https://codereview.chromium.org/447003003/diff/180001/tests/language/await_te...
File tests/language/await_test.dart (right):

https://codereview.chromium.org/447003003/diff/180001/tests/language/await_te...
tests/language/await_test.dart:5: // VMOptions=--enable_async
May want to check it for optimized code as well (add loop, add VM option)

[Michael Lippautz (Google), 2014-08-13 20:32:24]

Thanks! PTAL

I would like to use this as baseline for the final step (actual continuation).

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:154: expr_(expr) {
On 2014/08/13 18:41:05, srdjan wrote:
> all in one line ?

Done.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:51: const String& cnt_str = String::Handle(
On 2014/08/13 18:41:05, srdjan wrote:
> I, 

Done.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:128: const Class& cls =
Class::Handle(library_.LookupClass(Symbols::Future()));
On 2014/08/13 18:41:06, srdjan wrote:
> I, 

Done.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:174: SequenceNode* eval = new SequenceNode(
On 2014/08/13 18:41:06, srdjan wrote:
> new (I)

Done.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:200: if (node->kind() == Token::kAND ||
node->kind() == Token::kOR) {
On 2014/08/13 18:41:05, srdjan wrote:
> Use more parentheses.

Done.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:221: if (node->kind() == Token::kAND ||
node->kind() == Token::kOR) {
On 2014/08/13 18:41:05, srdjan wrote:
> Use more parentheses

Done.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:261: 
On 2014/08/13 18:41:05, srdjan wrote:
> two empty lines

Done.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:265: // TODO(mlippautz): Optimize variable
assingments and branching.
On 2014/08/13 18:41:06, srdjan wrote:
> s/assingments/assignments/

Got rid of the comment as it doesn't make too much sense anyways.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.h (right):

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:15: // Tranlate an AstNode containing an expression
(that contains one or more
On 2014/08/13 18:41:06, srdjan wrote:
> s/Tranlate/Translate/

Done.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:16: // awaits) into a sequential representation
that evaluation one subexpression
On 2014/08/13 18:41:06, srdjan wrote:
> fix sentence.

Holy crap. It should now be an actual english sentence :)

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:20: // expression. The evaluation is split among a
so called preamble and the
On 2014/08/13 18:41:06, srdjan wrote:
> s/so called/so-called/

Done.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:23: // Example (minimalisitc):
On 2014/08/13 18:41:06, srdjan wrote:
> s/minimalisitc/minimalistic/

Done.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:41: explicit AwaitTransformer(SequenceNode*
preamble,
On 2014/08/13 18:41:06, srdjan wrote:
> No explicit

Done.

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/447003003/diff/180001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8449: Symbols::AsyncOperation(), true)) {
On 2014/08/13 18:41:06, srdjan wrote:
> indent +4 spaces

Done.

https://codereview.chromium.org/447003003/diff/180001/tests/language/await_te...
File tests/language/await_test.dart (right):

https://codereview.chromium.org/447003003/diff/180001/tests/language/await_te...
tests/language/await_test.dart:5: // VMOptions=--enable_async
On 2014/08/13 18:41:06, srdjan wrote:
> May want to check it for optimized code as well (add loop, add VM option)

Done.

[hausner, 2014-08-15 18:09:31]

LGTMwC.

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:44: void AwaitTransformer::Transform(AstNode*
expr) {
Why not return the result_ field as a function result? See comment below.

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:158: //   (a || b) only evaluates b if a is true
only evaluates b if a is *false*

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:163: //   if (t_1) {
!t_1

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:205: new_right = result_;
Would it not make sense to group these two statements into a convenience
function?

AwaitTransformer::Transform(AstNode* n) {
  n->Visit(this);
  return result_;
}

Then the two statements above become new_right = Transform(node->right());

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.h (right):

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:20: // This allows a function to be suspended and
continue from within evaluating an
'continued', or maybe 'resumed'

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:2992: // Async closures have one optional parameter for
continuation results.
Why is the parameter optional? When would the continuation get invoked with no
parameter? await e by definition always evaluates to a result (if it completes
without error).

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:10835: } else if (IsLiteral("await")) {
I think Gilad mentioned that await is only recognized as a 'keyword' inside a
function that is marked as async. In a synchronous function, await would be
treated as an identifier that gets resolved like any other identifier.

[Michael Lippautz (Google), 2014-08-18 18:39:31]

Addressed commments and changed code to use the proposed pattern:

  AstNode* new_node = Transform(old_node);

Also changed how await is recognized (only in async functions) and added a test
that calls a function called await from within a non-async function.

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:44: void AwaitTransformer::Transform(AstNode*
expr) {
On 2014/08/15 18:09:31, hausner wrote:
> Why not return the result_ field as a function result? See comment below.

Done.

I also adjusted the parser to take this return value and got rid of the Result()
getter.

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:158: //   (a || b) only evaluates b if a is true
On 2014/08/15 18:09:31, hausner wrote:
> only evaluates b if a is *false*

Done.

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:163: //   if (t_1) {
On 2014/08/15 18:09:31, hausner wrote:
> !t_1

Done.

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:205: new_right = result_;
On 2014/08/15 18:09:31, hausner wrote:
> Would it not make sense to group these two statements into a convenience
> function?
> 
> AwaitTransformer::Transform(AstNode* n) {
>   n->Visit(this);
>   return result_;
> }
> 
> Then the two statements above become new_right = Transform(node->right());

Done.

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.h (right):

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:20: // This allows a function to be suspended and
continue from within evaluating an
On 2014/08/15 18:09:31, hausner wrote:
> 'continued', or maybe 'resumed'

Done.

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:2992: // Async closures have one optional parameter for
continuation results.
On 2014/08/15 18:09:31, hausner wrote:
> Why is the parameter optional? When would the continuation get invoked with no
> parameter? await e by definition always evaluates to a result (if it completes
> without error).

As discussed offline: The first invocation of this async closure happens from
within the Future constructor which expects a computation without any
parameters.

  Future(Function computation())

To avoid wrapping this in yet another closure we just make the parameter
optional.

https://codereview.chromium.org/447003003/diff/200001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:10835: } else if (IsLiteral("await")) {
On 2014/08/15 18:09:31, hausner wrote:
> I think Gilad mentioned that await is only recognized as a 'keyword' inside a
> function that is marked as async. In a synchronous function, await would be
> treated as an identifier that gets resolved like any other identifier.

Done. Also added a test.

[Michael Lippautz (Google), 2014-08-18 18:41:21]

Committed patchset #8 manually as 39345 (presubmit successful).