Issue 362153002: Transform functions marked as async

Issue 362153002: Transform functions marked as async (Closed)

Created:
6 years, 5 months ago by Michael Lippautz (Google)

Modified:
6 years, 4 months ago

Reviewers:
hausner, Ivan Posva

CC:
reviews_dartlang.org, vm-dev_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Transform functions marked as async In a nutshell: foo(params) async { <body> return result; } transforms to foo(params) async { var c = new Completer(); var async_body = () { <body> completer.complete(result); } new Future(async_body); return c.future; } BUG= R=hausner@google.com Committed: https://code.google.com/p/dart/source/detail?r=38681

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : getters and setters #

Total comments: 22

Patch Set 4 : first round of comments #

Total comments: 4

Patch Set 5 : #

Patch Set 6 : reworked #

Patch Set 7 : #

Total comments: 4

Patch Set 8 : #

Total comments: 4

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+429 lines, -7 lines)			Patch
M	runtime/vm/ast.h	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	runtime/vm/object.h	View	1 2 3 4 5 6	6 chunks	+20 lines, -0 lines	2 comments	Download
M	runtime/vm/object.cc	View	1 2 3 4 5	5 chunks	+13 lines, -1 line	0 comments	Download
M	runtime/vm/parser.h	View	1 2 3 4 5	3 chunks	+8 lines, -0 lines	0 comments	Download
M	runtime/vm/parser.cc	View	1 2 3 4 5 6 7	18 chunks	+246 lines, -3 lines	2 comments	Download
M	runtime/vm/raw_object.h	View	1 2 3 4 5	2 chunks	+6 lines, -1 line	0 comments	Download
M	runtime/vm/raw_object_snapshot.cc	View	1 2 3 4 5	2 chunks	+2 lines, -2 lines	0 comments	Download
M	runtime/vm/symbols.h	View	1 2 3 4 5	1 chunk	+9 lines, -0 lines	0 comments	Download
A	tests/language/async_test.dart	View	1 2 3 4 5 6	1 chunk	+121 lines, -0 lines	0 comments	Download
M	tests/language/language.status	View	1 2 3 4 5	1 chunk	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

Michael Lippautz (Google)

not ready yet. maybe a first round of comments?

6 years, 5 months ago (2014-07-10 23:22:18 UTC) #1

hausner

First set of comments. I like that you were able to keep the meat of ...

6 years, 5 months ago (2014-07-11 20:18:41 UTC) #2

Michael Lippautz (Google)

Addressed comments. Still missing tests. For the record (as discussed offline): For async it is ...

6 years, 5 months ago (2014-07-11 23:40:45 UTC) #3

Addressed comments. Still missing tests.

For the record (as discussed offline): For  async it is enough to re-capture
variables from outer scope(s) and capture parameters. For await we also need to
capture stack variables from the body. This can be done by capturing every
variable from the original scope (except implicit closure parameter).

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

https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:2884:
library_.LookupClass(String::Handle(String::New("Future"))));
On 2014/07/11 20:18:40, hausner wrote:
> As mentioned in another place, we need to make sure we get the right Future
> class. I would factor this out into a function that does all the necessary
> checks. Also, please make the strings symbols.

Done.

https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:2904: closure.set_is_synthetic_container(true);
On 2014/07/11 20:18:40, hausner wrote:
> Thinking about how to name things: isn't the "outer" function, the one that
> contains the synthesized "return new Future(..)" the synthesized container? It
> contains the closure you are constructing here.

The outer function contains the synthesized body, yes. The outer function does
not need a tag though.

As for the closure (which is only partly synthesized), .skip_parameters()?
Suggestions?

https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:2905: // TODO(mlippautz): Result type should be the
originally declared result type.
On 2014/07/11 20:18:40, hausner wrote:
> Not sure what the result type really should be. The "correct" result type of
the
> outer function is Future<T>, and the result type of the closure is T.

Clarified the TODO a bit. Leaving for now as we need to decide on what to do
with malformed (i.e. non-Future<T>) result types for the outer functions.

https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:2939: // Capture all variables that have already been
captured by the enclosing
On 2014/07/11 20:18:40, hausner wrote:
> Captured _by_ the enclosing function, or variables that have been captured in
> the enclosing function (by nested functions and closures)?

We re-capture variables (into the scope of the closure) that have been captured
by parsing the body of the enclosing method (the one we generate the synthesized
Future in).

https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:2953: ReturnNode* rn = new (I) ReturnNode(saved_pos, ccn);
On 2014/07/11 20:18:40, hausner wrote:
> You should use Scanner::kNoSourcePos for synthesized code, so that the
debugger
> won't break on these locations or won't show the synthesized code.

Done.

https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:2973: intptr_t saved_pos = TokenPos();
On 2014/07/11 20:18:40, hausner wrote:
> Nit: please name the variable after what it points to, e.g. formal_params_pos.

Done.

https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:4884: intptr_t Parser::ParseAsyncSpecifier() {
On 2014/07/11 20:18:41, hausner wrote:
> What does the integer return value mean. Should probably use bool or an
> enumeration type eventually.

Return value is now RawFunction::Modifier. This can be easily extended to
support async* and sync*.

https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:4890: ReportError("async modifier requires dart:async to be
imported");
On 2014/07/11 20:18:41, hausner wrote:
> I think we need to have a discussion with Gilad whether the user must
explicitly
> import dart:async.
> 
> Also, is it really necessarily an error if the program does not know the type
> "Future"?
> 
> Third, what does it mean if "Future" is found but it's not the one from
> dart:async? If we do require that dart:async is imported (explicitly or
> implicitly) we need to make sure that we find the right Future class.
> 
>  

Factored out. We still bomb until we know what to actually do...

https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6339: (FLAG_enable_async && IsLiteral("async"))) {
On 2014/07/11 20:18:40, hausner wrote:
> You should create a new Symbol for "async" (in symbols.h) and do a raw pointer
> comparison, which is much faster than IsLiteral().

Done.

https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:10865: void Parser::SkipFunctionLiteral() {
On 2014/07/11 20:18:40, hausner wrote:
> You probably need to handle the async specifiers here too.

Done.

hausner

Looking forward to seeing the tests. https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/362153002/diff/60001/runtime/vm/parser.cc#newcode2904 runtime/vm/parser.cc:2904: closure.set_is_synthetic_container(true); I can't ...

6 years, 5 months ago (2014-07-12 00:05:24 UTC) #4

Michael Lippautz (Google)

Added tests. Async getters/operators are a thing. For setters the situation is currently being discussed. ...

6 years, 5 months ago (2014-07-14 20:22:48 UTC) #5

Michael Lippautz (Google)

PTAL All parsing/processing is hidden behind the flag --enable_async. The generated AST now follows what ...

6 years, 4 months ago (2014-07-28 21:23:19 UTC) #6

hausner

Looks pretty solid to me now. Ship it! LGTM. https://codereview.chromium.org/362153002/diff/220001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/362153002/diff/220001/runtime/vm/parser.cc#newcode5568 runtime/vm/parser.cc:5568: ...

6 years, 4 months ago (2014-07-29 00:35:13 UTC) #7

Michael Lippautz (Google)

https://codereview.chromium.org/362153002/diff/220001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/362153002/diff/220001/runtime/vm/parser.cc#newcode5568 runtime/vm/parser.cc:5568: // Replace the current function's body with the async ...

6 years, 4 months ago (2014-07-29 17:15:33 UTC) #8

Michael Lippautz (Google)

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

6 years, 4 months ago (2014-07-29 17:16:10 UTC) #9

Ivan Posva

DBC -ip https://codereview.chromium.org/362153002/diff/240001/runtime/vm/object.h File runtime/vm/object.h (right): https://codereview.chromium.org/362153002/diff/240001/runtime/vm/object.h#newcode2070 runtime/vm/object.h:2070: public BitField<RawFunction::AsyncModifier, kModifierPos, 2> {}; // NOLINT ...

6 years, 4 months ago (2014-07-29 21:10:33 UTC) #10

Michael Lippautz (Google)

6 years, 4 months ago (2014-07-29 21:43:50 UTC) #11

Message was sent while issue was closed.

https://codereview.chromium.org/362153002/diff/240001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/362153002/diff/240001/runtime/vm/object.h#new...
runtime/vm/object.h:2070: public BitField<RawFunction::AsyncModifier,
kModifierPos, 2> {};  // NOLINT
On 2014/07/29 21:10:33, Ivan Posva wrote:
> AsyncModifier is only two states, why do we encode it in two bits here?

In the end it will be: none,async,sync*,async*. I suspect that we need to
distinguish them.

(We already need 32 bits for the tag.)

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

https://codereview.chromium.org/362153002/diff/240001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:2907: ReportError("async modifier requires dart:async to be
imported");
On 2014/07/29 21:10:33, Ivan Posva wrote:
> dart:async needs to be imported without prefix, correct?

Yes, I will clarify the error when I reland this.

Expand Messages | Collapse Messages