[esnext] implement frontend changes for async/await proposal

[esnext] implement frontend changes for async/await proposal

BUG=v8:4483
LOG=Y
R=littledan@chromium.org, adamk@chromium.org

Committed: https://crrev.com/0d43421a228b53ecf0e3a176e65618a35b047da6
Cr-Commit-Position: refs/heads/master@{#36261}

[caitp (gmail), 2016-03-29 20:06:27]

Hi,

I thought I'd get the ball rolling on the frontend-side of this proposal.

As it is, this is pretty bad --- it's not clear that everything necessarily
needs the extra FunctionKind bit signifying that a function is async or not.

Additionally, to satisfy the CoverCallExpressionAndAsyncArrowHead cover grammar
production, the expression classifier becomes slightly fatter (and this is
probably a pretty rare error to run into in practice) --- so this would benefit
from the ExpressionClassifier refactoring that has apparently not landed yet.

At the time of this writing, there is probably some duplicated or
no-longer-necessary changes in the patch, and it may not necessarily handle
everything entirely properly (such as differences from / interactions with
ecma262's Contains algorithm, and static errors that might result from that)

[Dan Ehrenberg, 2016-03-29 23:33:05]

Description was changed from

==========
[esnext] implement frontend changes for async/await proposal

BUG=
LOG=N
R=littledan@chromium.org, adamk@chromium.org
==========

to

==========
[esnext] implement frontend changes for async/await proposal

BUG=v8:4483
LOG=Y
R=littledan@chromium.org, adamk@chromium.org
==========

[caitp (gmail), 2016-03-31 21:08:54]

/CC mike

[caitp (gmail), 2016-04-16 02:55:03]

Patchset #7 (id:120001) has been deleted

[Dan Ehrenberg, 2016-04-22 06:32:07]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-22 06:32:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841543003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841543003/160001

[Michael Achenbach, 2016-04-22 07:32:48]

Please upload a new (empty) patchset and queue that one (see
http://crbug.com/605846). Sorry for the inconvenience.

[commit-bot: I haz the power, 2016-04-22 08:36:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-22 08:36:42]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on tryserver.v8 (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/6130)

[caitp (gmail), 2016-04-24 23:58:06]

On 2016/04/22 07:32:48, Michael Achenbach wrote:
> Please upload a new (empty) patchset and queue that one (see
> http://crbug.com/605846). Sorry for the inconvenience.

I don't have access to that bug --- but alright

[Dan Ehrenberg, 2016-04-25 21:18:16]

https://codereview.chromium.org/1841543003/diff/160001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1841543003/diff/160001/src/parsing/parser.cc#...
src/parsing/parser.cc:1281: PeekContextualKeyword(CStrVector("async")) &&
You mentioned that this patch causes a parser slowdown. I wonder how much of
that would be addressed by parsing 'async' similarly to 'let'--patch up a couple
of places to make sure that Token::LET/Token::ASYNC is treated as an identifier
on one hand, but on the other hand, peeking for an 'async' might become cheaper
(and you peek for an async all over the place in this patch!).

[caitp (gmail), 2016-04-26 00:09:38]

https://codereview.chromium.org/1841543003/diff/160001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1841543003/diff/160001/src/parsing/parser.cc#...
src/parsing/parser.cc:1281: PeekContextualKeyword(CStrVector("async")) &&
On 2016/04/25 21:18:16, Dan Ehrenberg wrote:
> You mentioned that this patch causes a parser slowdown. I wonder how much of
> that would be addressed by parsing 'async' similarly to 'let'--patch up a
couple
> of places to make sure that Token::LET/Token::ASYNC is treated as an
identifier
> on one hand, but on the other hand, peeking for an 'async' might become
cheaper
> (and you peek for an async all over the place in this patch!).

I said there was a parser slowdown in JSC. I don't know if this one causes a
significant slowdown, since we have the single token lookahead, which could
mitigate the biggest perf hit in JSC.

Will have to measure the impact in chromium to see how negligible it is.

[Dan Ehrenberg, 2016-05-05 01:14:40]

Just a first pass, not a full review

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/parser-base.h
File src/parsing/parser-base.h (left):

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/parser-bas...
src/parsing/parser-base.h:1309: 
Spurious whitespace change.

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/parser-bas...
src/parsing/parser-base.h:32: enum class ParseFunctionFlags {
Cute overloading idiom. We could do this elsewhere, like PropertyAttributes.
Even define the overloading once in a templated way.

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/parser-bas...
src/parsing/parser-base.h:1258: scanner()->location(),
MessageTemplate::kAwaitBindingIdentifier);
"await is only treated as a FutureReservedWord when Module is the goal symbol of
the syntactic grammar." (from ES2015)
"await is always parsed as an AwaitExpression but is a Syntax Error via static
semantics." (from the async await spec)

I'm not sure what either of these ends up having to do with arrow functions...
I'm a bit confused by the grammars.

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/parser-bas...
src/parsing/parser-base.h:1312: 
Spurious whitespace change.

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/preparser....
src/parsing/preparser.h:937: return PreParserExpression::Default();
When do you accumulate these to a classifier/validate arrow function formal
parameters/ban await?

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/scanner.cc...
src/parsing/scanner.cc:285: has_multiline_comment_before_next_ =
has_multiline_comment_before_next;
From my experience with let, I'd predict that maintaining these will cause a
noticeable regression on CodeLoad. I wonder if we could consolidate them into
one variable. I only see a single usage which separates the two--detecting
-->-style comments inside of SkipWhiteSpace. Do you think there's any way we
might refactor this?

https://codereview.chromium.org/1841543003/diff/260001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1841543003/diff/260001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7464: "var O = { async 0() { await 1; } }",
Maybe some tests which check that
- async is a contextual keyword and can be used as a normal variable, including
inside an async function
- await is completely usable as an identifier outside of an async function,
e.g., 'async function await() {}' is legal (when not enclosed in another async
function).
- A non-async function inside of an async function regains the ability to use
await as an identifier

https://codereview.chromium.org/1841543003/diff/260001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7510: "async function f() { var O = { async
[await](a, a) {} } }",
- async generators and constructors are a syntax error.
- await; is a syntax error inside an async function (unlike yield)
- await as an identifier is disallowed in computed property names in class
declarations inside async functions
- async (x = await y) => z, and async function (x = await y) { } -- await is
banned in formal parameters not based on banning the identifier but based on
parsing only AwaitExpressions and banning those.

https://codereview.chromium.org/1841543003/diff/260001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7523: "var O = { async 0(arguments) {} }",
- async(a, a) => a (For this one in particular, I'm not sure if the code handles
it. Ditto other things that are banned in strict mode parameters of arrow
functions.)

[caitp (gmail), 2016-05-05 01:36:58]

Rest of the comments on the tests are good, I'll work on these tomorrow. Thanks
for the look through the big patch

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/parser-bas...
src/parsing/parser-base.h:1258: scanner()->location(),
MessageTemplate::kAwaitBindingIdentifier);
On 2016/05/05 01:14:40, Dan Ehrenberg wrote:
> "await is only treated as a FutureReservedWord when Module is the goal symbol
of
> the syntactic grammar." (from ES2015)
> "await is always parsed as an AwaitExpression but is a Syntax Error via static
> semantics." (from the async await spec)
> 
> I'm not sure what either of these ends up having to do with arrow functions...
> I'm a bit confused by the grammars.

Ideally we'd have a variety of versions of this message for the different causes
(but that's a bit harder in v8 than in JSC).

Basically, in an async function, you can't use `await` as a parameter name (or
as a BindingIdentifier within the function body) --- so that's what the error is
meant to communicate.

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/parser-bas...
src/parsing/parser-base.h:1312: 
On 2016/05/05 01:14:40, Dan Ehrenberg wrote:
> Spurious whitespace change.

Acknowledged.

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/preparser....
src/parsing/preparser.h:937: return PreParserExpression::Default();
On 2016/05/05 01:14:40, Dan Ehrenberg wrote:
> When do you accumulate these to a classifier/validate arrow function formal
> parameters/ban await?

The main use of this is to be able to translate `async(FormalParameters) => ...`
into arrow formal parameters --- but classifier errors are recorded during
ParseArguments(), AFAIK if we see it is actually an arrow function, we report
the error.

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/scanner.cc...
src/parsing/scanner.cc:285: has_multiline_comment_before_next_ =
has_multiline_comment_before_next;
On 2016/05/05 01:14:40, Dan Ehrenberg wrote:
> From my experience with let, I'd predict that maintaining these will cause a
> noticeable regression on CodeLoad. I wonder if we could consolidate them into
> one variable. I only see a single usage which separates the two--detecting
> -->-style comments inside of SkipWhiteSpace. Do you think there's any way we
> might refactor this?

Well, I've done this in the simplest way I could, so it's very likely it could
be done better. Maybe it's worth a look at the CodeLoad impact before that,
though?

https://codereview.chromium.org/1841543003/diff/260001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1841543003/diff/260001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7464: "var O = { async 0() { await 1; } }",
On 2016/05/05 01:14:40, Dan Ehrenberg wrote:
> Maybe some tests which check that
> - async is a contextual keyword and can be used as a normal variable,
including
> inside an async function
> - await is completely usable as an identifier outside of an async function,
> e.g., 'async function await() {}' is legal (when not enclosed in another async
> function).
> - A non-async function inside of an async function regains the ability to use
> await as an identifier

Acknowledged --- Sounds good.

[caitp (gmail), 2016-05-05 21:49:37]

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/1841543003/diff/260001/src/parsing/scanner.cc...
src/parsing/scanner.cc:285: has_multiline_comment_before_next_ =
has_multiline_comment_before_next;
On 2016/05/05 01:14:40, Dan Ehrenberg wrote:
> From my experience with let, I'd predict that maintaining these will cause a
> noticeable regression on CodeLoad. I wonder if we could consolidate them into
> one variable. I only see a single usage which separates the two--detecting
> -->-style comments inside of SkipWhiteSpace. Do you think there's any way we
> might refactor this?

I've had a go at refactoring this --- I doubt it has any substantial improvement
on CodeLoad, but maybe.

https://codereview.chromium.org/1841543003/diff/260001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1841543003/diff/260001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7510: "async function f() { var O = { async
[await](a, a) {} } }",
On 2016/05/05 01:14:40, Dan Ehrenberg wrote:
> - async generators and constructors are a syntax error.
> - await; is a syntax error inside an async function (unlike yield)
> - await as an identifier is disallowed in computed property names in class
> declarations inside async functions
> - async (x = await y) => z, and async function (x = await y) { } -- await is
> banned in formal parameters not based on banning the identifier but based on
> parsing only AwaitExpressions and banning those.

Added a bunch of these

https://codereview.chromium.org/1841543003/diff/260001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7523: "var O = { async 0(arguments) {} }",
On 2016/05/05 01:14:40, Dan Ehrenberg wrote:
> - async(a, a) => a (For this one in particular, I'm not sure if the code
handles
> it. Ditto other things that are banned in strict mode parameters of arrow
> functions.)

Duplicate parameters in async arrow functions behave like regular arrow
functions --- they throw in Parser, but not in the PreParser (this is a
long-standing TODO of Andy's, I believe). We probably can't fix this in a
trivial patch without killing performance, which nobody wants to do.

I've added tests for these, but they're temporarily commented out with a TODO
(though I'm fairly sure async methods will behave correctly).

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/expression...
File src/parsing/expression-classifier.h (right):

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/expression...
src/parsing/expression-classifier.h:260: void
RecordAsyncBindingPatternError(const Scanner::Location& loc,
Sorry about this, but it was the simplest way to make `var [await];` an error
while allowing `async function x() { var f = ([await]) => 1; return f([]); }`.

Due to the new classifier errors (which should be rare, in practice), I would
really like to see some variation of https://codereview.chromium.org/1708193003/
land.

[Dan Ehrenberg, 2016-05-06 00:08:56]

It would be nice to have 'messages' tests for the new syntax errors as well.

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/expression...
File src/parsing/expression-classifier.h (right):

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/expression...
src/parsing/expression-classifier.h:260: void
RecordAsyncBindingPatternError(const Scanner::Location& loc,
On 2016/05/05 at 21:49:36, caitp wrote:
> Sorry about this, but it was the simplest way to make `var [await];` an error
while allowing `async function x() { var f = ([await]) => 1; return f([]); }`.
> 
> Due to the new classifier errors (which should be rare, in practice), I would
really like to see some variation of https://codereview.chromium.org/1708193003/
land.

Seems appropriate to me.

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/parser-base.h
File src/parsing/parser-base.h (left):

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/parser-bas...
src/parsing/parser-base.h:2778: 
Add back irrelevant blank line removals.

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/preparser....
src/parsing/preparser.cc:72: }
My hunch is that the parser and preparser would work out better if async were
lexed like await--in the scanner, rather than having special behavior in the
parser, with the parser then going back and treating it as an identifier when
appropriate. But either way is functional apparently.

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/preparser....
src/parsing/preparser.cc:75: }
This looks like dead code. I think this case is handled by 43-44.

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/preparser....
src/parsing/preparser.h:77: bool IsFutureReserved() const {
Why do you add this? I don't see it used anywhere.

https://codereview.chromium.org/1841543003/diff/280001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1841543003/diff/280001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7604: // "var O = { async method(dupe, dupe) {} }",
If these work, then probably good to uncomment them.

https://codereview.chromium.org/1841543003/diff/280001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7605: // "var f = async(dupe, dupe) => {}",
I'm fine with the arrow functions not enforcing these restrictions in the
preparser yet, but it would be good to have parser-only tests that get at this.

[caitp (gmail), 2016-05-06 00:31:03]

thanks, will do a bit more cleanup shortly

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/preparser....
src/parsing/preparser.cc:72: }
On 2016/05/06 00:08:56, Dan Ehrenberg wrote:
> My hunch is that the parser and preparser would work out better if async were
> lexed like await--in the scanner, rather than having special behavior in the
> parser, with the parser then going back and treating it as an identifier when
> appropriate. But either way is functional apparently.

Im not entirely sure this change is really needed anymore, actually. I think
PeekContextualKeyword makes this redundant now, maybe.

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/preparser....
src/parsing/preparser.cc:75: }
On 2016/05/06 00:08:56, Dan Ehrenberg wrote:
> This looks like dead code. I think this case is handled by 43-44.

yes, its no longer needed since Mikes CL, will delete

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/1841543003/diff/280001/src/parsing/preparser....
src/parsing/preparser.h:77: bool IsFutureReserved() const {
On 2016/05/06 00:08:56, Dan Ehrenberg wrote:
> Why do you add this? I don't see it used anywhere.

was originally useful, became redundant but wasnt noticed, i guess

[caitp (gmail), 2016-05-10 14:08:25]

I've added some Octane measurements in
https://gist.github.com/anonymous/27980d70f09d24c64c27d3ee29e217cd

There's about a 2.5% regression in CodeLoad (on a 3ghz Xeon E5) --- I will see
if this does any better with the tokenization approach, but I expect this is not
related to the keyword vs not keyword issue

[Dan Ehrenberg, 2016-05-11 21:04:17]

Looking forward to the keyword version

https://codereview.chromium.org/1841543003/diff/420001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/1841543003/diff/420001/src/globals.h#newcode1079
src/globals.h:1079: };
This seems to overlap a lot with ObjectLiteralProperty::Kind and FunctionKind,
and the information doesn't seem to be used all that much. Can we make do with
fewer concepts, maybe merge some of these together?

https://codereview.chromium.org/1841543003/diff/420001/src/globals.h#newcode1103
src/globals.h:1103: inline bool IsMethod(MethodKind kind) { return kind !=
MethodKind::None; }
Huh? What about an ordinary non-static non-generator non-async concise method?

[caitp (gmail), 2016-05-11 21:14:08]

https://codereview.chromium.org/1841543003/diff/420001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/1841543003/diff/420001/src/globals.h#newcode1079
src/globals.h:1079: };
On 2016/05/11 21:04:17, Dan Ehrenberg wrote:
> This seems to overlap a lot with ObjectLiteralProperty::Kind and FunctionKind,
> and the information doesn't seem to be used all that much. Can we make do with
> fewer concepts, maybe merge some of these together?

goal was to get rid of a boolean trap in CheckProperty, rather than adding a new
one. its really parser-specific and can be moved out of globals.h, but having it
here simplifies creating the ast node a bit. I am a bit iffy about merging it
into PropertyKind, as backends may rely on it, but if you have ideas to do it in
an uninvasive way, i can try it

https://codereview.chromium.org/1841543003/diff/420001/src/globals.h#newcode1103
src/globals.h:1103: inline bool IsMethod(MethodKind kind) { return kind !=
MethodKind::None; }
On 2016/05/11 21:04:17, Dan Ehrenberg wrote:
> Huh? What about an ordinary non-static non-generator non-async concise method?

good point, although it doesnt seem to actually matter in this CL

[Dan Ehrenberg, 2016-05-12 19:24:32]

Things are really looking good in this patch; close to being landable IMO.

https://codereview.chromium.org/1841543003/diff/480001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1841543003/diff/480001/src/ast/ast.h#newcode2742
src/ast/ast.h:2742: class FunctionTypeBits : public BitField16<FunctionType, 0,
2> {};
I like this new way that you found the extra bit!

https://codereview.chromium.org/1841543003/diff/480001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1841543003/diff/480001/src/parsing/parser-bas...
src/parsing/parser-base.h:679: next == Token::ENUM || next == Token::ASYNC ||
Nit: put async next to await

https://codereview.chromium.org/1841543003/diff/480001/src/parsing/parser-bas...
src/parsing/parser-base.h:700: }
Dead code

https://codereview.chromium.org/1841543003/diff/480001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/1841543003/diff/480001/src/parsing/scanner.cc...
src/parsing/scanner.cc:280: has_preceding_line_terminator_ ||
has_preceding_multiline_comment_;
What does has_preceding_line_terminator actually mean after this change? It
would be nice to document how it all works currently, in case something else
comes along that wants to refer to it.

[caitp (gmail), 2016-05-12 19:56:05]

https://codereview.chromium.org/1841543003/diff/480001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/1841543003/diff/480001/src/parsing/scanner.cc...
src/parsing/scanner.cc:280: has_preceding_line_terminator_ ||
has_preceding_multiline_comment_;
On 2016/05/12 19:24:32, Dan Ehrenberg wrote:
> What does has_preceding_line_terminator actually mean after this change? It
> would be nice to document how it all works currently, in case something else
> comes along that wants to refer to it.

There's no real difference in how it currently works, it's just renamed to
something that makes more sense to me. I can change it back (and restore the old
comments that were modified).

[caitp (gmail), 2016-05-12 20:39:17]

On 2016/05/12 19:56:05, caitp wrote:
> https://codereview.chromium.org/1841543003/diff/480001/src/parsing/scanner.cc
> File src/parsing/scanner.cc (right):
> 
>
https://codereview.chromium.org/1841543003/diff/480001/src/parsing/scanner.cc...
> src/parsing/scanner.cc:280: has_preceding_line_terminator_ ||
> has_preceding_multiline_comment_;
> On 2016/05/12 19:24:32, Dan Ehrenberg wrote:
> > What does has_preceding_line_terminator actually mean after this change? It
> > would be nice to document how it all works currently, in case something else
> > comes along that wants to refer to it.
> 
> There's no real difference in how it currently works, it's just renamed to
> something that makes more sense to me. I can change it back (and restore the
old
> comments that were modified).

Okay, I've cleaned up as much of the canner as I couldm

[Dan Ehrenberg, 2016-05-12 23:58:29]

https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
src/parsing/parser-base.h:380: }
How could this strategy detect an error in an arrow function formal parameter
inside an async function? Maybe switch back to the classifier approach.

[caitp (gmail), 2016-05-13 00:03:07]

https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
src/parsing/parser-base.h:380: }
On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> How could this strategy detect an error in an arrow function formal parameter
> inside an async function? Maybe switch back to the classifier approach.

Well, it should be recorded in the classifier too, ill make that change and add
a few more parser tests later

[Dan Ehrenberg, 2016-05-13 00:14:09]

https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
src/parsing/parser-base.h:380: }
On 2016/05/13 at 00:03:07, caitp wrote:
> On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > How could this strategy detect an error in an arrow function formal
parameter
> > inside an async function? Maybe switch back to the classifier approach.
> 
> Well, it should be recorded in the classifier too, ill make that change and
add a few more parser tests later

I don't really see how this state can be maintained properly in a world with
arrow functions (though I wish we could!). Would the classifier be sufficient?

[caitp (gmail), 2016-05-13 01:28:40]

https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
src/parsing/parser-base.h:380: }
On 2016/05/13 00:14:08, Dan Ehrenberg wrote:
> On 2016/05/13 at 00:03:07, caitp wrote:
> > On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > > How could this strategy detect an error in an arrow function formal
> parameter
> > > inside an async function? Maybe switch back to the classifier approach.
> > 
> > Well, it should be recorded in the classifier too, ill make that change and
> add a few more parser tests later
> 
> I don't really see how this state can be maintained properly in a world with
> arrow functions (though I wish we could!). Would the classifier be sufficient?

Well, until we have better support for backtracking (and I know there's a lot of
dislike for backtracking, but the existing backtracking implementations are sort
of clobbering V8 in CodeLoad very badly right now, whilst also being simpler and
easier to follow --- so it's probably not as bad as we make it out to be), it
won't get it right for eagerly parsed arrow functions.

The classifier is probably good enough on its own for now, so I'll drop the
parse phase stuff

[Dan Ehrenberg, 2016-05-13 01:37:13]

There are more places where 'await' has to be banned as an identifier:

ParserBase<Traits>::ParseIdentifierOrStrictReservedWord

(e.g., for async function foo() { function async() {} } -- thanks to bterlson
for the test in test262)

Possibly also in Token::IsIdentifier though I don't have a test case yet.

[caitp (gmail), 2016-05-13 01:44:52]

On 2016/05/13 01:28:40, caitp wrote:
>
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
> File src/parsing/parser-base.h (right):
> 
>
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
> src/parsing/parser-base.h:380: }
> On 2016/05/13 00:14:08, Dan Ehrenberg wrote:
> > On 2016/05/13 at 00:03:07, caitp wrote:
> > > On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > > > How could this strategy detect an error in an arrow function formal
> > parameter
> > > > inside an async function? Maybe switch back to the classifier approach.
> > > 
> > > Well, it should be recorded in the classifier too, ill make that change
and
> > add a few more parser tests later
> > 
> > I don't really see how this state can be maintained properly in a world with
> > arrow functions (though I wish we could!). Would the classifier be
sufficient?
> 
> Well, until we have better support for backtracking (and I know there's a lot
of
> dislike for backtracking, but the existing backtracking implementations are
sort
> of clobbering V8 in CodeLoad very badly right now, whilst also being simpler
and
> easier to follow --- so it's probably not as bad as we make it out to be), it
> won't get it right for eagerly parsed arrow functions.
> 
> The classifier is probably good enough on its own for now, so I'll drop the
> parse phase stuff

Actually, the runtime CL depends on this to prevent trying to use the generator
object variable when it's not going to be present. So, the parse phase stuff
helps a lot there for non-error uses

[caitp (gmail), 2016-05-13 01:45:03]

Patchset #28 (id:550001) has been deleted

[Dan Ehrenberg, 2016-05-13 19:28:11]

On 2016/05/13 at 01:44:52, caitpotter88 wrote:
> On 2016/05/13 01:28:40, caitp wrote:
> >
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
> > File src/parsing/parser-base.h (right):
> > 
> >
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
> > src/parsing/parser-base.h:380: }
> > On 2016/05/13 00:14:08, Dan Ehrenberg wrote:
> > > On 2016/05/13 at 00:03:07, caitp wrote:
> > > > On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > > > > How could this strategy detect an error in an arrow function formal
> > > parameter
> > > > > inside an async function? Maybe switch back to the classifier
approach.
> > > > 
> > > > Well, it should be recorded in the classifier too, ill make that change
and
> > > add a few more parser tests later
> > > 
> > > I don't really see how this state can be maintained properly in a world
with
> > > arrow functions (though I wish we could!). Would the classifier be
sufficient?
> > 
> > Well, until we have better support for backtracking (and I know there's a
lot of
> > dislike for backtracking, but the existing backtracking implementations are
sort
> > of clobbering V8 in CodeLoad very badly right now, whilst also being simpler
and
> > easier to follow --- so it's probably not as bad as we make it out to be),
it
> > won't get it right for eagerly parsed arrow functions.
> > 
> > The classifier is probably good enough on its own for now, so I'll drop the
> > parse phase stuff
> 
> Actually, the runtime CL depends on this to prevent trying to use the
generator object variable when it's not going to be present. So, the parse phase
stuff helps a lot there for non-error uses

Where is that usage? I can't find it. It looks like this stuff is controlled by
checking is_async_function(), which works like is_generator() by looking at the
function_state_ stack.

[caitp (gmail), 2016-05-13 19:43:02]

On 2016/05/13 19:28:11, Dan Ehrenberg wrote:
> On 2016/05/13 at 01:44:52, caitpotter88 wrote:
> > On 2016/05/13 01:28:40, caitp wrote:
> > >
>
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
> > > File src/parsing/parser-base.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
> > > src/parsing/parser-base.h:380: }
> > > On 2016/05/13 00:14:08, Dan Ehrenberg wrote:
> > > > On 2016/05/13 at 00:03:07, caitp wrote:
> > > > > On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > > > > > How could this strategy detect an error in an arrow function formal
> > > > parameter
> > > > > > inside an async function? Maybe switch back to the classifier
> approach.
> > > > > 
> > > > > Well, it should be recorded in the classifier too, ill make that
change
> and
> > > > add a few more parser tests later
> > > > 
> > > > I don't really see how this state can be maintained properly in a world
> with
> > > > arrow functions (though I wish we could!). Would the classifier be
> sufficient?
> > > 
> > > Well, until we have better support for backtracking (and I know there's a
> lot of
> > > dislike for backtracking, but the existing backtracking implementations
are
> sort
> > > of clobbering V8 in CodeLoad very badly right now, whilst also being
simpler
> and
> > > easier to follow --- so it's probably not as bad as we make it out to be),
> it
> > > won't get it right for eagerly parsed arrow functions.
> > > 
> > > The classifier is probably good enough on its own for now, so I'll drop
the
> > > parse phase stuff
> > 
> > Actually, the runtime CL depends on this to prevent trying to use the
> generator object variable when it's not going to be present. So, the parse
phase
> stuff helps a lot there for non-error uses
> 
> Where is that usage? I can't find it. It looks like this stuff is controlled
by
> checking is_async_function(), which works like is_generator() by looking at
the
> function_state_ stack.

RewriteAwaitExpression relies on the presence of `generator_object_variable`.
Removing the early exit makes it possible to reach RewriteAwaitExpression
without a valid generator_object_variable.

We could work around this by allocating generator objects more liberally, but I
had trouble getting that working the last time, and it seems more expensive

[Dan Ehrenberg, 2016-05-13 22:31:29]

On 2016/05/13 at 19:43:02, caitpotter88 wrote:
> On 2016/05/13 19:28:11, Dan Ehrenberg wrote:
> > On 2016/05/13 at 01:44:52, caitpotter88 wrote:
> > > On 2016/05/13 01:28:40, caitp wrote:
> > > >
> >
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
> > > > File src/parsing/parser-base.h (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
> > > > src/parsing/parser-base.h:380: }
> > > > On 2016/05/13 00:14:08, Dan Ehrenberg wrote:
> > > > > On 2016/05/13 at 00:03:07, caitp wrote:
> > > > > > On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > > > > > > How could this strategy detect an error in an arrow function
formal
> > > > > parameter
> > > > > > > inside an async function? Maybe switch back to the classifier
> > approach.
> > > > > > 
> > > > > > Well, it should be recorded in the classifier too, ill make that
change
> > and
> > > > > add a few more parser tests later
> > > > > 
> > > > > I don't really see how this state can be maintained properly in a
world
> > with
> > > > > arrow functions (though I wish we could!). Would the classifier be
> > sufficient?
> > > > 
> > > > Well, until we have better support for backtracking (and I know there's
a
> > lot of
> > > > dislike for backtracking, but the existing backtracking implementations
are
> > sort
> > > > of clobbering V8 in CodeLoad very badly right now, whilst also being
simpler
> > and
> > > > easier to follow --- so it's probably not as bad as we make it out to
be),
> > it
> > > > won't get it right for eagerly parsed arrow functions.
> > > > 
> > > > The classifier is probably good enough on its own for now, so I'll drop
the
> > > > parse phase stuff
> > > 
> > > Actually, the runtime CL depends on this to prevent trying to use the
> > generator object variable when it's not going to be present. So, the parse
phase
> > stuff helps a lot there for non-error uses
> > 
> > Where is that usage? I can't find it. It looks like this stuff is controlled
by
> > checking is_async_function(), which works like is_generator() by looking at
the
> > function_state_ stack.
> 
> RewriteAwaitExpression relies on the presence of `generator_object_variable`.
Removing the early exit makes it possible to reach RewriteAwaitExpression
without a valid generator_object_variable.
> 
> We could work around this by allocating generator objects more liberally, but
I had trouble getting that working the last time, and it seems more expensive

We definitely can't start adding generator objects when they are not needed.

Can we just check is_async_function() rather than is_async_function_body(),
analogous to how Generators only call ParseYieldExpression if is_generator (yet
we still manage to ban yield in generator parameters)?

[caitp (gmail), 2016-05-14 02:17:55]

On 2016/05/13 22:31:29, Dan Ehrenberg wrote:
> On 2016/05/13 at 19:43:02, caitpotter88 wrote:
> > On 2016/05/13 19:28:11, Dan Ehrenberg wrote:
> > > On 2016/05/13 at 01:44:52, caitpotter88 wrote:
> > > > On 2016/05/13 01:28:40, caitp wrote:
> > > > >
> > >
>
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
> > > > > File src/parsing/parser-base.h (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
> > > > > src/parsing/parser-base.h:380: }
> > > > > On 2016/05/13 00:14:08, Dan Ehrenberg wrote:
> > > > > > On 2016/05/13 at 00:03:07, caitp wrote:
> > > > > > > On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > > > > > > > How could this strategy detect an error in an arrow function
> formal
> > > > > > parameter
> > > > > > > > inside an async function? Maybe switch back to the classifier
> > > approach.
> > > > > > > 
> > > > > > > Well, it should be recorded in the classifier too, ill make that
> change
> > > and
> > > > > > add a few more parser tests later
> > > > > > 
> > > > > > I don't really see how this state can be maintained properly in a
> world
> > > with
> > > > > > arrow functions (though I wish we could!). Would the classifier be
> > > sufficient?
> > > > > 
> > > > > Well, until we have better support for backtracking (and I know
there's
> a
> > > lot of
> > > > > dislike for backtracking, but the existing backtracking
implementations
> are
> > > sort
> > > > > of clobbering V8 in CodeLoad very badly right now, whilst also being
> simpler
> > > and
> > > > > easier to follow --- so it's probably not as bad as we make it out to
> be),
> > > it
> > > > > won't get it right for eagerly parsed arrow functions.
> > > > > 
> > > > > The classifier is probably good enough on its own for now, so I'll
drop
> the
> > > > > parse phase stuff
> > > > 
> > > > Actually, the runtime CL depends on this to prevent trying to use the
> > > generator object variable when it's not going to be present. So, the parse
> phase
> > > stuff helps a lot there for non-error uses
> > > 
> > > Where is that usage? I can't find it. It looks like this stuff is
controlled
> by
> > > checking is_async_function(), which works like is_generator() by looking
at
> the
> > > function_state_ stack.
> > 
> > RewriteAwaitExpression relies on the presence of
`generator_object_variable`.
> Removing the early exit makes it possible to reach RewriteAwaitExpression
> without a valid generator_object_variable.
> > 
> > We could work around this by allocating generator objects more liberally,
but
> I had trouble getting that working the last time, and it seems more expensive
> 
> We definitely can't start adding generator objects when they are not needed.
> 
> Can we just check is_async_function() rather than is_async_function_body(),
> analogous to how Generators only call ParseYieldExpression if is_generator
(yet
> we still manage to ban yield in generator parameters)?

There are 2 ways we can make that work. 1) by deferring the rewriting, or 2)
giving AwaitExpression its own AST node (and treating it like a Yield in the
backends).

N'either of these seem especially attractive, but if you insist, it's doable

[Dan Ehrenberg, 2016-05-16 18:16:25]

On 2016/05/14 at 02:17:55, caitpotter88 wrote:
> On 2016/05/13 22:31:29, Dan Ehrenberg wrote:
> > On 2016/05/13 at 19:43:02, caitpotter88 wrote:
> > > On 2016/05/13 19:28:11, Dan Ehrenberg wrote:
> > > > On 2016/05/13 at 01:44:52, caitpotter88 wrote:
> > > > > On 2016/05/13 01:28:40, caitp wrote:
> > > > > >
> > > >
> >
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
> > > > > > File src/parsing/parser-base.h (right):
> > > > > > 
> > > > > >
> > > >
> >
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
> > > > > > src/parsing/parser-base.h:380: }
> > > > > > On 2016/05/13 00:14:08, Dan Ehrenberg wrote:
> > > > > > > On 2016/05/13 at 00:03:07, caitp wrote:
> > > > > > > > On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > > > > > > > > How could this strategy detect an error in an arrow function
> > formal
> > > > > > > parameter
> > > > > > > > > inside an async function? Maybe switch back to the classifier
> > > > approach.
> > > > > > > > 
> > > > > > > > Well, it should be recorded in the classifier too, ill make that
> > change
> > > > and
> > > > > > > add a few more parser tests later
> > > > > > > 
> > > > > > > I don't really see how this state can be maintained properly in a
> > world
> > > > with
> > > > > > > arrow functions (though I wish we could!). Would the classifier be
> > > > sufficient?
> > > > > > 
> > > > > > Well, until we have better support for backtracking (and I know
there's
> > a
> > > > lot of
> > > > > > dislike for backtracking, but the existing backtracking
implementations
> > are
> > > > sort
> > > > > > of clobbering V8 in CodeLoad very badly right now, whilst also being
> > simpler
> > > > and
> > > > > > easier to follow --- so it's probably not as bad as we make it out
to
> > be),
> > > > it
> > > > > > won't get it right for eagerly parsed arrow functions.
> > > > > > 
> > > > > > The classifier is probably good enough on its own for now, so I'll
drop
> > the
> > > > > > parse phase stuff
> > > > > 
> > > > > Actually, the runtime CL depends on this to prevent trying to use the
> > > > generator object variable when it's not going to be present. So, the
parse
> > phase
> > > > stuff helps a lot there for non-error uses
> > > > 
> > > > Where is that usage? I can't find it. It looks like this stuff is
controlled
> > by
> > > > checking is_async_function(), which works like is_generator() by looking
at
> > the
> > > > function_state_ stack.
> > > 
> > > RewriteAwaitExpression relies on the presence of
`generator_object_variable`.
> > Removing the early exit makes it possible to reach RewriteAwaitExpression
> > without a valid generator_object_variable.
> > > 
> > > We could work around this by allocating generator objects more liberally,
but
> > I had trouble getting that working the last time, and it seems more
expensive
> > 
> > We definitely can't start adding generator objects when they are not needed.
> > 
> > Can we just check is_async_function() rather than is_async_function_body(),
> > analogous to how Generators only call ParseYieldExpression if is_generator
(yet
> > we still manage to ban yield in generator parameters)?
> 
> There are 2 ways we can make that work. 1) by deferring the rewriting, or 2)
giving AwaitExpression its own AST node (and treating it like a Yield in the
backends).
> 
> N'either of these seem especially attractive, but if you insist, it's doable

What purpose would the division serve? Aren't all yield/await expressions banned
in all parameters?

If you do need to distinguish them, then you could put a bit in the Yield node,
which backends would ignore but the parser could use internally.

I'm not totally opposed to maintaining a new form of state like this, but I just
don't think it's being done correctly here. At the very least, wouldn't you need
a stack, like function_state_ has? And there's still the problem of determining
whether you're within arrow function parameters (and once you've solved that,
still seems to me like you're almost done).

[caitp (gmail), 2016-05-16 18:40:21]

On 2016/05/16 18:16:25, Dan Ehrenberg wrote:
> On 2016/05/14 at 02:17:55, caitpotter88 wrote:
> > On 2016/05/13 22:31:29, Dan Ehrenberg wrote:
> > > On 2016/05/13 at 19:43:02, caitpotter88 wrote:
> > > > On 2016/05/13 19:28:11, Dan Ehrenberg wrote:
> > > > > On 2016/05/13 at 01:44:52, caitpotter88 wrote:
> > > > > > On 2016/05/13 01:28:40, caitp wrote:
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
> > > > > > > File src/parsing/parser-base.h (right):
> > > > > > > 
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
> > > > > > > src/parsing/parser-base.h:380: }
> > > > > > > On 2016/05/13 00:14:08, Dan Ehrenberg wrote:
> > > > > > > > On 2016/05/13 at 00:03:07, caitp wrote:
> > > > > > > > > On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > > > > > > > > > How could this strategy detect an error in an arrow function
> > > formal
> > > > > > > > parameter
> > > > > > > > > > inside an async function? Maybe switch back to the
classifier
> > > > > approach.
> > > > > > > > > 
> > > > > > > > > Well, it should be recorded in the classifier too, ill make
that
> > > change
> > > > > and
> > > > > > > > add a few more parser tests later
> > > > > > > > 
> > > > > > > > I don't really see how this state can be maintained properly in
a
> > > world
> > > > > with
> > > > > > > > arrow functions (though I wish we could!). Would the classifier
be
> > > > > sufficient?
> > > > > > > 
> > > > > > > Well, until we have better support for backtracking (and I know
> there's
> > > a
> > > > > lot of
> > > > > > > dislike for backtracking, but the existing backtracking
> implementations
> > > are
> > > > > sort
> > > > > > > of clobbering V8 in CodeLoad very badly right now, whilst also
being
> > > simpler
> > > > > and
> > > > > > > easier to follow --- so it's probably not as bad as we make it out
> to
> > > be),
> > > > > it
> > > > > > > won't get it right for eagerly parsed arrow functions.
> > > > > > > 
> > > > > > > The classifier is probably good enough on its own for now, so I'll
> drop
> > > the
> > > > > > > parse phase stuff
> > > > > > 
> > > > > > Actually, the runtime CL depends on this to prevent trying to use
the
> > > > > generator object variable when it's not going to be present. So, the
> parse
> > > phase
> > > > > stuff helps a lot there for non-error uses
> > > > > 
> > > > > Where is that usage? I can't find it. It looks like this stuff is
> controlled
> > > by
> > > > > checking is_async_function(), which works like is_generator() by
looking
> at
> > > the
> > > > > function_state_ stack.
> > > > 
> > > > RewriteAwaitExpression relies on the presence of
> `generator_object_variable`.
> > > Removing the early exit makes it possible to reach RewriteAwaitExpression
> > > without a valid generator_object_variable.
> > > > 
> > > > We could work around this by allocating generator objects more
liberally,
> but
> > > I had trouble getting that working the last time, and it seems more
> expensive
> > > 
> > > We definitely can't start adding generator objects when they are not
needed.
> > > 
> > > Can we just check is_async_function() rather than
is_async_function_body(),
> > > analogous to how Generators only call ParseYieldExpression if is_generator
> (yet
> > > we still manage to ban yield in generator parameters)?
> > 
> > There are 2 ways we can make that work. 1) by deferring the rewriting, or 2)
> giving AwaitExpression its own AST node (and treating it like a Yield in the
> backends).
> > 
> > N'either of these seem especially attractive, but if you insist, it's doable
> 
> What purpose would the division serve? Aren't all yield/await expressions
banned
> in all parameters?
> 
> If you do need to distinguish them, then you could put a bit in the Yield
node,
> which backends would ignore but the parser could use internally.
> 
> I'm not totally opposed to maintaining a new form of state like this, but I
just
> don't think it's being done correctly here. At the very least, wouldn't you
need
> a stack, like function_state_ has? And there's still the problem of
determining
> whether you're within arrow function parameters (and once you've solved that,
> still seems to me like you're almost done).

well, it's part of FunctionState, so it's a stack already.

As I said, I agree it doesn't "really" work for arrow functions, (hence the need
for the additional classifier error) --- but being there means we can exit early
when generator_object_variable() is NULL (which can happen while
is_async_function() is true --- during FormalParameter parsing, or at any point
during PreParser runs).

It's relatively lightweight state that doesn't need to change very often (only
at key points during function parsing), and it lets us avoid having to allocate
the generator object in the preparser (by signalling that it's present in some
other way).

Of course, it's possible to remove the assertion that
generator_object_variable() is not null, which should make it work in the
preparser, and let us get away with just the classifier error. But this isn't
really that great either, since it helps (and has helped in the past) catch
errors early during refactorings.

So, take your pick:

- A little extra state that is cheap to manage, but not 100% reliable for Arrow
functions (yet) --- hopefully this can be fixed in a later refactoring with a
backtracking parser
- Always allocate the generator object variable (even in the preparser), and get
rid of the GeneratorVariable* abstraction (using a Variable* all the time)
- Get rid of the CHECK(), and likely trip during future refactorings and waste
time with harder to identify crashes

I feel like you're going with the third option, so I'll get that ready, but let
me know if you change your mind

[Dan Ehrenberg, 2016-05-16 18:46:10]

On 2016/05/16 at 18:40:21, caitpotter88 wrote:
> On 2016/05/16 18:16:25, Dan Ehrenberg wrote:
> > On 2016/05/14 at 02:17:55, caitpotter88 wrote:
> > > On 2016/05/13 22:31:29, Dan Ehrenberg wrote:
> > > > On 2016/05/13 at 19:43:02, caitpotter88 wrote:
> > > > > On 2016/05/13 19:28:11, Dan Ehrenberg wrote:
> > > > > > On 2016/05/13 at 01:44:52, caitpotter88 wrote:
> > > > > > > On 2016/05/13 01:28:40, caitp wrote:
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
> > > > > > > > File src/parsing/parser-base.h (right):
> > > > > > > > 
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
> > > > > > > > src/parsing/parser-base.h:380: }
> > > > > > > > On 2016/05/13 00:14:08, Dan Ehrenberg wrote:
> > > > > > > > > On 2016/05/13 at 00:03:07, caitp wrote:
> > > > > > > > > > On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > > > > > > > > > > How could this strategy detect an error in an arrow
function
> > > > formal
> > > > > > > > > parameter
> > > > > > > > > > > inside an async function? Maybe switch back to the
classifier
> > > > > > approach.
> > > > > > > > > > 
> > > > > > > > > > Well, it should be recorded in the classifier too, ill make
that
> > > > change
> > > > > > and
> > > > > > > > > add a few more parser tests later
> > > > > > > > > 
> > > > > > > > > I don't really see how this state can be maintained properly
in a
> > > > world
> > > > > > with
> > > > > > > > > arrow functions (though I wish we could!). Would the
classifier be
> > > > > > sufficient?
> > > > > > > > 
> > > > > > > > Well, until we have better support for backtracking (and I know
> > there's
> > > > a
> > > > > > lot of
> > > > > > > > dislike for backtracking, but the existing backtracking
> > implementations
> > > > are
> > > > > > sort
> > > > > > > > of clobbering V8 in CodeLoad very badly right now, whilst also
being
> > > > simpler
> > > > > > and
> > > > > > > > easier to follow --- so it's probably not as bad as we make it
out
> > to
> > > > be),
> > > > > > it
> > > > > > > > won't get it right for eagerly parsed arrow functions.
> > > > > > > > 
> > > > > > > > The classifier is probably good enough on its own for now, so
I'll
> > drop
> > > > the
> > > > > > > > parse phase stuff
> > > > > > > 
> > > > > > > Actually, the runtime CL depends on this to prevent trying to use
the
> > > > > > generator object variable when it's not going to be present. So, the
> > parse
> > > > phase
> > > > > > stuff helps a lot there for non-error uses
> > > > > > 
> > > > > > Where is that usage? I can't find it. It looks like this stuff is
> > controlled
> > > > by
> > > > > > checking is_async_function(), which works like is_generator() by
looking
> > at
> > > > the
> > > > > > function_state_ stack.
> > > > > 
> > > > > RewriteAwaitExpression relies on the presence of
> > `generator_object_variable`.
> > > > Removing the early exit makes it possible to reach
RewriteAwaitExpression
> > > > without a valid generator_object_variable.
> > > > > 
> > > > > We could work around this by allocating generator objects more
liberally,
> > but
> > > > I had trouble getting that working the last time, and it seems more
> > expensive
> > > > 
> > > > We definitely can't start adding generator objects when they are not
needed.
> > > > 
> > > > Can we just check is_async_function() rather than
is_async_function_body(),
> > > > analogous to how Generators only call ParseYieldExpression if
is_generator
> > (yet
> > > > we still manage to ban yield in generator parameters)?
> > > 
> > > There are 2 ways we can make that work. 1) by deferring the rewriting, or
2)
> > giving AwaitExpression its own AST node (and treating it like a Yield in the
> > backends).
> > > 
> > > N'either of these seem especially attractive, but if you insist, it's
doable
> > 
> > What purpose would the division serve? Aren't all yield/await expressions
banned
> > in all parameters?
> > 
> > If you do need to distinguish them, then you could put a bit in the Yield
node,
> > which backends would ignore but the parser could use internally.
> > 
> > I'm not totally opposed to maintaining a new form of state like this, but I
just
> > don't think it's being done correctly here. At the very least, wouldn't you
need
> > a stack, like function_state_ has? And there's still the problem of
determining
> > whether you're within arrow function parameters (and once you've solved
that,
> > still seems to me like you're almost done).
> 
> well, it's part of FunctionState, so it's a stack already.
> 
> As I said, I agree it doesn't "really" work for arrow functions, (hence the
need for the additional classifier error) --- but being there means we can exit
early when generator_object_variable() is NULL (which can happen while
is_async_function() is true --- during FormalParameter parsing, or at any point
during PreParser runs).
> 
> It's relatively lightweight state that doesn't need to change very often (only
at key points during function parsing), and it lets us avoid having to allocate
the generator object in the preparser (by signalling that it's present in some
other way).
> 
> Of course, it's possible to remove the assertion that
generator_object_variable() is not null, which should make it work in the
preparser, and let us get away with just the classifier error. But this isn't
really that great either, since it helps (and has helped in the past) catch
errors early during refactorings.
> 
> So, take your pick:
> 
> - A little extra state that is cheap to manage, but not 100% reliable for
Arrow functions (yet) --- hopefully this can be fixed in a later refactoring
with a backtracking parser
> - Always allocate the generator object variable (even in the preparser), and
get rid of the GeneratorVariable* abstraction (using a Variable* all the time)
> - Get rid of the CHECK(), and likely trip during future refactorings and waste
time with harder to identify crashes
> 
> I feel like you're going with the third option, so I'll get that ready, but
let me know if you change your mind

Are you aware of cases where the current code is inaccurate with respect to
arrow functions? If so, it'd be great to have tests for those. I don't think
it's acceptable to have those sorts of bugs in shipped code, or allocate extra
generator object variables when they're not needed (wouldn't that change what
code gets emitted?). I'd also like to avoid depending on a backtracking parser
in this design, and I'm OK with this code introducing yet another thing that
needs to be redone when we make that change (it already does that through the
use of ExpressionClassifiers).

Could you explain more about what you mean about future refactorings and harder
to identify crashes?

[caitp (gmail), 2016-05-16 18:54:31]

On 2016/05/16 18:46:10, Dan Ehrenberg wrote:
> On 2016/05/16 at 18:40:21, caitpotter88 wrote:
> > On 2016/05/16 18:16:25, Dan Ehrenberg wrote:
> > > On 2016/05/14 at 02:17:55, caitpotter88 wrote:
> > > > On 2016/05/13 22:31:29, Dan Ehrenberg wrote:
> > > > > On 2016/05/13 at 19:43:02, caitpotter88 wrote:
> > > > > > On 2016/05/13 19:28:11, Dan Ehrenberg wrote:
> > > > > > > On 2016/05/13 at 01:44:52, caitpotter88 wrote:
> > > > > > > > On 2016/05/13 01:28:40, caitp wrote:
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
> > > > > > > > > File src/parsing/parser-base.h (right):
> > > > > > > > > 
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
> > > > > > > > > src/parsing/parser-base.h:380: }
> > > > > > > > > On 2016/05/13 00:14:08, Dan Ehrenberg wrote:
> > > > > > > > > > On 2016/05/13 at 00:03:07, caitp wrote:
> > > > > > > > > > > On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > > > > > > > > > > > How could this strategy detect an error in an arrow
> function
> > > > > formal
> > > > > > > > > > parameter
> > > > > > > > > > > > inside an async function? Maybe switch back to the
> classifier
> > > > > > > approach.
> > > > > > > > > > > 
> > > > > > > > > > > Well, it should be recorded in the classifier too, ill
make
> that
> > > > > change
> > > > > > > and
> > > > > > > > > > add a few more parser tests later
> > > > > > > > > > 
> > > > > > > > > > I don't really see how this state can be maintained properly
> in a
> > > > > world
> > > > > > > with
> > > > > > > > > > arrow functions (though I wish we could!). Would the
> classifier be
> > > > > > > sufficient?
> > > > > > > > > 
> > > > > > > > > Well, until we have better support for backtracking (and I
know
> > > there's
> > > > > a
> > > > > > > lot of
> > > > > > > > > dislike for backtracking, but the existing backtracking
> > > implementations
> > > > > are
> > > > > > > sort
> > > > > > > > > of clobbering V8 in CodeLoad very badly right now, whilst also
> being
> > > > > simpler
> > > > > > > and
> > > > > > > > > easier to follow --- so it's probably not as bad as we make it
> out
> > > to
> > > > > be),
> > > > > > > it
> > > > > > > > > won't get it right for eagerly parsed arrow functions.
> > > > > > > > > 
> > > > > > > > > The classifier is probably good enough on its own for now, so
> I'll
> > > drop
> > > > > the
> > > > > > > > > parse phase stuff
> > > > > > > > 
> > > > > > > > Actually, the runtime CL depends on this to prevent trying to
use
> the
> > > > > > > generator object variable when it's not going to be present. So,
the
> > > parse
> > > > > phase
> > > > > > > stuff helps a lot there for non-error uses
> > > > > > > 
> > > > > > > Where is that usage? I can't find it. It looks like this stuff is
> > > controlled
> > > > > by
> > > > > > > checking is_async_function(), which works like is_generator() by
> looking
> > > at
> > > > > the
> > > > > > > function_state_ stack.
> > > > > > 
> > > > > > RewriteAwaitExpression relies on the presence of
> > > `generator_object_variable`.
> > > > > Removing the early exit makes it possible to reach
> RewriteAwaitExpression
> > > > > without a valid generator_object_variable.
> > > > > > 
> > > > > > We could work around this by allocating generator objects more
> liberally,
> > > but
> > > > > I had trouble getting that working the last time, and it seems more
> > > expensive
> > > > > 
> > > > > We definitely can't start adding generator objects when they are not
> needed.
> > > > > 
> > > > > Can we just check is_async_function() rather than
> is_async_function_body(),
> > > > > analogous to how Generators only call ParseYieldExpression if
> is_generator
> > > (yet
> > > > > we still manage to ban yield in generator parameters)?
> > > > 
> > > > There are 2 ways we can make that work. 1) by deferring the rewriting,
or
> 2)
> > > giving AwaitExpression its own AST node (and treating it like a Yield in
the
> > > backends).
> > > > 
> > > > N'either of these seem especially attractive, but if you insist, it's
> doable
> > > 
> > > What purpose would the division serve? Aren't all yield/await expressions
> banned
> > > in all parameters?
> > > 
> > > If you do need to distinguish them, then you could put a bit in the Yield
> node,
> > > which backends would ignore but the parser could use internally.
> > > 
> > > I'm not totally opposed to maintaining a new form of state like this, but
I
> just
> > > don't think it's being done correctly here. At the very least, wouldn't
you
> need
> > > a stack, like function_state_ has? And there's still the problem of
> determining
> > > whether you're within arrow function parameters (and once you've solved
> that,
> > > still seems to me like you're almost done).
> > 
> > well, it's part of FunctionState, so it's a stack already.
> > 
> > As I said, I agree it doesn't "really" work for arrow functions, (hence the
> need for the additional classifier error) --- but being there means we can
exit
> early when generator_object_variable() is NULL (which can happen while
> is_async_function() is true --- during FormalParameter parsing, or at any
point
> during PreParser runs).
> > 
> > It's relatively lightweight state that doesn't need to change very often
(only
> at key points during function parsing), and it lets us avoid having to
allocate
> the generator object in the preparser (by signalling that it's present in some
> other way).
> > 
> > Of course, it's possible to remove the assertion that
> generator_object_variable() is not null, which should make it work in the
> preparser, and let us get away with just the classifier error. But this isn't
> really that great either, since it helps (and has helped in the past) catch
> errors early during refactorings.
> > 
> > So, take your pick:
> > 
> > - A little extra state that is cheap to manage, but not 100% reliable for
> Arrow functions (yet) --- hopefully this can be fixed in a later refactoring
> with a backtracking parser
> > - Always allocate the generator object variable (even in the preparser), and
> get rid of the GeneratorVariable* abstraction (using a Variable* all the time)
> > - Get rid of the CHECK(), and likely trip during future refactorings and
waste
> time with harder to identify crashes
> > 
> > I feel like you're going with the third option, so I'll get that ready, but
> let me know if you change your mind
> 
> Are you aware of cases where the current code is inaccurate with respect to
> arrow functions? If so, it'd be great to have tests for those. I don't think
> it's acceptable to have those sorts of bugs in shipped code, or allocate extra
> generator object variables when they're not needed (wouldn't that change what
> code gets emitted?). I'd also like to avoid depending on a backtracking parser
> in this design, and I'm OK with this code introducing yet another thing that
> needs to be redone when we make that change (it already does that through the
> use of ExpressionClassifiers).
> 
> Could you explain more about what you mean about future refactorings and
harder
> to identify crashes?

Well, if a Yield node with a null generator_object_variable makes it into the
real AST, it will crash, and take up time investigating the reason why --- this
is why assertions are useful, they help identify bugs earlier, and identify them
more clearly. Anything like this would be human error, but because the parser is
doing so many different jobs right now, I think in the current architecture,
human error is incredibly likely, and these assertions help mitigate that human
error.

I'm not saying "this is buggy and will crash on weird inputs", I'm saying "if
you poke it the wrong way, it will break --- and the assertions are there to
help you fix it when it does".

[Dan Ehrenberg, 2016-05-16 19:06:55]

On 2016/05/16 at 18:54:31, caitpotter88 wrote:
> On 2016/05/16 18:46:10, Dan Ehrenberg wrote:
> > On 2016/05/16 at 18:40:21, caitpotter88 wrote:
> > > On 2016/05/16 18:16:25, Dan Ehrenberg wrote:
> > > > On 2016/05/14 at 02:17:55, caitpotter88 wrote:
> > > > > On 2016/05/13 22:31:29, Dan Ehrenberg wrote:
> > > > > > On 2016/05/13 at 19:43:02, caitpotter88 wrote:
> > > > > > > On 2016/05/13 19:28:11, Dan Ehrenberg wrote:
> > > > > > > > On 2016/05/13 at 01:44:52, caitpotter88 wrote:
> > > > > > > > > On 2016/05/13 01:28:40, caitp wrote:
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-base.h
> > > > > > > > > > File src/parsing/parser-base.h (right):
> > > > > > > > > > 
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/1841543003/diff/530001/src/parsing/parser-bas...
> > > > > > > > > > src/parsing/parser-base.h:380: }
> > > > > > > > > > On 2016/05/13 00:14:08, Dan Ehrenberg wrote:
> > > > > > > > > > > On 2016/05/13 at 00:03:07, caitp wrote:
> > > > > > > > > > > > On 2016/05/12 23:58:29, Dan Ehrenberg wrote:
> > > > > > > > > > > > > How could this strategy detect an error in an arrow
> > function
> > > > > > formal
> > > > > > > > > > > parameter
> > > > > > > > > > > > > inside an async function? Maybe switch back to the
> > classifier
> > > > > > > > approach.
> > > > > > > > > > > > 
> > > > > > > > > > > > Well, it should be recorded in the classifier too, ill
make
> > that
> > > > > > change
> > > > > > > > and
> > > > > > > > > > > add a few more parser tests later
> > > > > > > > > > > 
> > > > > > > > > > > I don't really see how this state can be maintained
properly
> > in a
> > > > > > world
> > > > > > > > with
> > > > > > > > > > > arrow functions (though I wish we could!). Would the
> > classifier be
> > > > > > > > sufficient?
> > > > > > > > > > 
> > > > > > > > > > Well, until we have better support for backtracking (and I
know
> > > > there's
> > > > > > a
> > > > > > > > lot of
> > > > > > > > > > dislike for backtracking, but the existing backtracking
> > > > implementations
> > > > > > are
> > > > > > > > sort
> > > > > > > > > > of clobbering V8 in CodeLoad very badly right now, whilst
also
> > being
> > > > > > simpler
> > > > > > > > and
> > > > > > > > > > easier to follow --- so it's probably not as bad as we make
it
> > out
> > > > to
> > > > > > be),
> > > > > > > > it
> > > > > > > > > > won't get it right for eagerly parsed arrow functions.
> > > > > > > > > > 
> > > > > > > > > > The classifier is probably good enough on its own for now,
so
> > I'll
> > > > drop
> > > > > > the
> > > > > > > > > > parse phase stuff
> > > > > > > > > 
> > > > > > > > > Actually, the runtime CL depends on this to prevent trying to
use
> > the
> > > > > > > > generator object variable when it's not going to be present. So,
the
> > > > parse
> > > > > > phase
> > > > > > > > stuff helps a lot there for non-error uses
> > > > > > > > 
> > > > > > > > Where is that usage? I can't find it. It looks like this stuff
is
> > > > controlled
> > > > > > by
> > > > > > > > checking is_async_function(), which works like is_generator() by
> > looking
> > > > at
> > > > > > the
> > > > > > > > function_state_ stack.
> > > > > > > 
> > > > > > > RewriteAwaitExpression relies on the presence of
> > > > `generator_object_variable`.
> > > > > > Removing the early exit makes it possible to reach
> > RewriteAwaitExpression
> > > > > > without a valid generator_object_variable.
> > > > > > > 
> > > > > > > We could work around this by allocating generator objects more
> > liberally,
> > > > but
> > > > > > I had trouble getting that working the last time, and it seems more
> > > > expensive
> > > > > > 
> > > > > > We definitely can't start adding generator objects when they are not
> > needed.
> > > > > > 
> > > > > > Can we just check is_async_function() rather than
> > is_async_function_body(),
> > > > > > analogous to how Generators only call ParseYieldExpression if
> > is_generator
> > > > (yet
> > > > > > we still manage to ban yield in generator parameters)?
> > > > > 
> > > > > There are 2 ways we can make that work. 1) by deferring the rewriting,
or
> > 2)
> > > > giving AwaitExpression its own AST node (and treating it like a Yield in
the
> > > > backends).
> > > > > 
> > > > > N'either of these seem especially attractive, but if you insist, it's
> > doable
> > > > 
> > > > What purpose would the division serve? Aren't all yield/await
expressions
> > banned
> > > > in all parameters?
> > > > 
> > > > If you do need to distinguish them, then you could put a bit in the
Yield
> > node,
> > > > which backends would ignore but the parser could use internally.
> > > > 
> > > > I'm not totally opposed to maintaining a new form of state like this,
but I
> > just
> > > > don't think it's being done correctly here. At the very least, wouldn't
you
> > need
> > > > a stack, like function_state_ has? And there's still the problem of
> > determining
> > > > whether you're within arrow function parameters (and once you've solved
> > that,
> > > > still seems to me like you're almost done).
> > > 
> > > well, it's part of FunctionState, so it's a stack already.
> > > 
> > > As I said, I agree it doesn't "really" work for arrow functions, (hence
the
> > need for the additional classifier error) --- but being there means we can
exit
> > early when generator_object_variable() is NULL (which can happen while
> > is_async_function() is true --- during FormalParameter parsing, or at any
point
> > during PreParser runs).
> > > 
> > > It's relatively lightweight state that doesn't need to change very often
(only
> > at key points during function parsing), and it lets us avoid having to
allocate
> > the generator object in the preparser (by signalling that it's present in
some
> > other way).
> > > 
> > > Of course, it's possible to remove the assertion that
> > generator_object_variable() is not null, which should make it work in the
> > preparser, and let us get away with just the classifier error. But this
isn't
> > really that great either, since it helps (and has helped in the past) catch
> > errors early during refactorings.
> > > 
> > > So, take your pick:
> > > 
> > > - A little extra state that is cheap to manage, but not 100% reliable for
> > Arrow functions (yet) --- hopefully this can be fixed in a later refactoring
> > with a backtracking parser
> > > - Always allocate the generator object variable (even in the preparser),
and
> > get rid of the GeneratorVariable* abstraction (using a Variable* all the
time)
> > > - Get rid of the CHECK(), and likely trip during future refactorings and
waste
> > time with harder to identify crashes
> > > 
> > > I feel like you're going with the third option, so I'll get that ready,
but
> > let me know if you change your mind
> > 
> > Are you aware of cases where the current code is inaccurate with respect to
> > arrow functions? If so, it'd be great to have tests for those. I don't think
> > it's acceptable to have those sorts of bugs in shipped code, or allocate
extra
> > generator object variables when they're not needed (wouldn't that change
what
> > code gets emitted?). I'd also like to avoid depending on a backtracking
parser
> > in this design, and I'm OK with this code introducing yet another thing that
> > needs to be redone when we make that change (it already does that through
the
> > use of ExpressionClassifiers).
> > 
> > Could you explain more about what you mean about future refactorings and
harder
> > to identify crashes?
> 
> Well, if a Yield node with a null generator_object_variable makes it into the
real AST, it will crash, and take up time investigating the reason why --- this
is why assertions are useful, they help identify bugs earlier, and identify them
more clearly. Anything like this would be human error, but because the parser is
doing so many different jobs right now, I think in the current architecture,
human error is incredibly likely, and these assertions help mitigate that human
error.
> 
> I'm not saying "this is buggy and will crash on weird inputs", I'm saying "if
you poke it the wrong way, it will break --- and the assertions are there to
help you fix it when it does".

Sorry, I am just not sure which assertion you are talking about. My
understanding is that Yield needs a generator_object either way (to save the
context into), and the async/await desugaring does provide the generator_object
by setting it in the function_state in DesugarAsyncFunctionBody.

[caitp (gmail), 2016-05-16 19:12:53]

In the runtime patchset, there's a CHECK_NOT_NULL(generator_object_variable) ---
to ensure that this is set up correctly. There have been a number of cases where
this has helped a lot to solve problems during implementation (eg, when
allocating the generator object used to be done in the same place it is done for
Generators, causing some failures).

It seems obvious, but because the parser is doing so many jobs here, the
slightest changes (which might make sense in their own right) can cause these
kinds of problems, which become harder to diagnose when it gets all the way to
codegen. So in my opinion, it's useful to have these checks (more of them,
even), especially during weird secondary responsibilities of the Parser
(desugaring, scope resolution, etc).

Anyways, I don't care that much about all of this, so like I said, I'm going
with just removing the FunctionParsePhase stuff and removing the assertion,
which makes it more like the YieldExpression stuff in practice.

[Dan Ehrenberg, 2016-05-16 20:38:37]

As we discussed off-line, omitting the CHECK as you've already done seems to me
like a good option. And, as we discussed, there is another minor bug that
test262 exposed, which is that the following is supposed to be allowed in
statement context:

async function await() {}

https://codereview.chromium.org/1841543003/diff/610001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1841543003/diff/610001/src/parsing/parser.cc#...
src/parsing/parser.cc:1604: default: {
What about export default async function foo() {} ? Seems like this will be
parsed as an AssignmentExpression, rather than as a HoistableDeclaration, which
would lead to things like "export default function await() {}" being banned
(even once you fix the bug where it's not banned normally), as well as other
subtle differences between function expressions and declarations (like the
binding of the function inside of itself).

[Dan Ehrenberg, 2016-05-16 21:35:07]

https://codereview.chromium.org/1841543003/diff/650001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1841543003/diff/650001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7624: // "var O = { async method(dupe, dupe) {} }",
This does successfully produce an exception; could you uncomment it?

[caitp (gmail), 2016-05-16 21:36:19]

https://codereview.chromium.org/1841543003/diff/650001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1841543003/diff/650001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7624: // "var O = { async method(dupe, dupe) {} }",
On 2016/05/16 21:35:07, Dan Ehrenberg wrote:
> This does successfully produce an exception; could you uncomment it?

Yep, and will fix the typo too

[Dan Ehrenberg, 2016-05-16 21:51:53]

lgtm

[caitp (gmail), 2016-05-16 22:34:48]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2016-05-16 22:34:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841543003/670001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841543003/670001

[commit-bot: I haz the power, 2016-05-16 23:17:25]

Committed patchset #33 (id:670001)

[commit-bot: I haz the power, 2016-05-16 23:19:14]

Description was changed from

==========
[esnext] implement frontend changes for async/await proposal

BUG=v8:4483
LOG=Y
R=littledan@chromium.org, adamk@chromium.org
==========

to

==========
[esnext] implement frontend changes for async/await proposal

BUG=v8:4483
LOG=Y
R=littledan@chromium.org, adamk@chromium.org

Committed: https://crrev.com/0d43421a228b53ecf0e3a176e65618a35b047da6
Cr-Commit-Position: refs/heads/master@{#36261}
==========

[commit-bot: I haz the power, 2016-05-16 23:19:15]

Patchset 33 (id:??) landed as
https://crrev.com/0d43421a228b53ecf0e3a176e65618a35b047da6
Cr-Commit-Position: refs/heads/master@{#36261}