[es6] implement destructuring assignment

[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting strategy involves inserting a placeholder RewritableAssignmentExpression into the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes which form part of a binding pattern, thus breaking the meaning of the pattern --- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=Y
R=adamk@chromium.org, bmeurer@chromium.org, rossberg@chromium.org

Committed: https://crrev.com/b634a61d84b8793ee2939690685d4b9f7fb32b95
Cr-Commit-Position: refs/heads/master@{#32623}

[caitp (gmail), 2015-08-26 20:58:54]

PTAL when time permits

This is the third try at this (following https://crrev.com/1168643005 and
https://crrev.com/1301523004. There is a lot of cleanup needed. This only deals
with parsing, the desugaring is only a placeholder at the moment.

[caitp (gmail), 2015-08-26 21:05:57]

On 2015/08/26 20:58:54, caitp wrote:
> PTAL when time permits
> 
> This is the third try at this (following https://crrev.com/1168643005 and
> https://crrev.com/1301523004. There is a lot of cleanup needed. This only
deals
> with parsing, the desugaring is only a placeholder at the moment.

oh, it also handles CoverInitializedName, that thing I was saying was really
hard to deal with. I guess it's not as bad as I thought.

[conradw, 2015-08-27 09:40:26]

https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h
File src/expression-classifier.h (right):

https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h...
src/expression-classifier.h:158: void RecordPatternError(const
Scanner::Location& loc,
BindingPatternErrors and AssignmentPatternErrors being separate seems odd to me,
from scanning the parser code it looks like the latter implies the former. This
might be an opportunity to unify them somewhat.

https://codereview.chromium.org/1309813007/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/1/src/preparser.h#newcode1134
src/preparser.h:1134: kArrayLiteralExpression
Do Object and Array literals need to be distinguished in the preparser? It seems
like the added checks are satisfied by either.

https://codereview.chromium.org/1309813007/diff/1/src/preparser.h#newcode3008
src/preparser.h:3008: if (is_pattern_element) {
It seems to me that the classifier should always record the error, and it should
be the responsibility of the caller to only convert this to a "real" parser
error if it's appropriate. I appreciate this might not be possible to arrange,
however.

https://codereview.chromium.org/1309813007/diff/1/src/preparser.h#newcode3048
src/preparser.h:3048: } else if (assignment_pattern) {
unifying the two errors would clean up this code

[caitp (gmail), 2015-08-27 15:16:01]

Thanks for the look

https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h
File src/expression-classifier.h (right):

https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h...
src/expression-classifier.h:158: void RecordPatternError(const
Scanner::Location& loc,
On 2015/08/27 09:40:26, conradw wrote:
> BindingPatternErrors and AssignmentPatternErrors being separate seems odd to
me,
> from scanning the parser code it looks like the latter implies the former.
This
> might be an opportunity to unify them somewhat.

There are cases where they do need to be separate. EG, `[ a.b.c ]` is a valid
AssignmentPattern, but not a valid BindingPattern. I think it's likely that in
the majority of cases, they are the same though.

https://codereview.chromium.org/1309813007/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/1/src/preparser.h#newcode1134
src/preparser.h:1134: kArrayLiteralExpression
On 2015/08/27 09:40:26, conradw wrote:
> Do Object and Array literals need to be distinguished in the preparser? It
seems
> like the added checks are satisfied by either.

They would be satisfied by either. If folks prefer, they can be unified into a
single flag.

https://codereview.chromium.org/1309813007/diff/1/src/preparser.h#newcode3008
src/preparser.h:3008: if (is_pattern_element) {
On 2015/08/27 09:40:26, conradw wrote:
> It seems to me that the classifier should always record the error, and it
should
> be the responsibility of the caller to only convert this to a "real" parser
> error if it's appropriate. I appreciate this might not be possible to arrange,
> however.

The context this flag provides is important. It allows the parser to treat
references like `(a => a).b` as a valid destructuring assignment target (which
is, per spec, correct --- despite not being very useful)

https://codereview.chromium.org/1309813007/diff/1/src/preparser.h#newcode3048
src/preparser.h:3048: } else if (assignment_pattern) {
On 2015/08/27 09:40:26, conradw wrote:
> unifying the two errors would clean up this code

The BindingPattern error doesn't really need to be here, thought I had deleted
it. It's marked for cleanup and will be removed in the next iteration

[conradw, 2015-08-27 15:32:24]

https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h
File src/expression-classifier.h (right):

https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h...
src/expression-classifier.h:158: void RecordPatternError(const
Scanner::Location& loc,
On 2015/08/27 15:16:01, caitp wrote:
> On 2015/08/27 09:40:26, conradw wrote:
> > BindingPatternErrors and AssignmentPatternErrors being separate seems odd to
> me,
> > from scanning the parser code it looks like the latter implies the former.
> This
> > might be an opportunity to unify them somewhat.
> 
> There are cases where they do need to be separate. EG, `[ a.b.c ]` is a valid
> AssignmentPattern, but not a valid BindingPattern. I think it's likely that in
> the majority of cases, they are the same though.

I was thinking it might be possible to eliminate/combine the
assignment_pattern_error_ object, since every time assignment_pattern_error_ is
set, binding_pattern_error_ is set identically. From what you're saying, there
would still need to be two separate bits in invalid_productions_ however.

[wingo, 2015-08-28 09:54:13]

https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h
File src/expression-classifier.h (right):

https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h...
src/expression-classifier.h:158: void RecordPatternError(const
Scanner::Location& loc,
On 2015/08/27 15:32:24, conradw wrote:
> I was thinking it might be possible to eliminate/combine the
> assignment_pattern_error_ object, since every time assignment_pattern_error_
is
> set, binding_pattern_error_ is set identically. From what you're saying, there
> would still need to be two separate bits in invalid_productions_ however.

Not sure that this works.  Consider:

  ({x.y, 2}) => 45

We're going to want to raise the binding pattern error, not the assignment
pattern error.

[conradw, 2015-08-31 11:22:53]

On 2015/08/28 09:54:13, wingo wrote:
> https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h
> File src/expression-classifier.h (right):
> 
>
https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h...
> src/expression-classifier.h:158: void RecordPatternError(const
> Scanner::Location& loc,
> On 2015/08/27 15:32:24, conradw wrote:
> > I was thinking it might be possible to eliminate/combine the
> > assignment_pattern_error_ object, since every time assignment_pattern_error_
> is
> > set, binding_pattern_error_ is set identically. From what you're saying,
there
> > would still need to be two separate bits in invalid_productions_ however.
> 
> Not sure that this works.  Consider:
> 
>   ({x.y, 2}) => 45
> 
> We're going to want to raise the binding pattern error, not the assignment
> pattern error.

My impression was that this is the reason that {assignment, binding}_pattern
errors are both recorded in the case of a potential assignment pattern error.
Since the message is identical, I feel like it's possible to eliminate a copy of
the error object in this case (as long as the distinction of validating
specifically for an assignment pattern error, for the case caitp mentioned
above, still exists in the invalid_productions_ pseudo-bitfield).

[wingo, 2015-08-31 13:13:30]

On 2015/08/31 11:22:53, conradw wrote:
> On 2015/08/28 09:54:13, wingo wrote:
> >
https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h
> > File src/expression-classifier.h (right):
> > 
> >
>
https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h...
> > src/expression-classifier.h:158: void RecordPatternError(const
> > Scanner::Location& loc,
> > On 2015/08/27 15:32:24, conradw wrote:
> > > I was thinking it might be possible to eliminate/combine the
> > > assignment_pattern_error_ object, since every time
assignment_pattern_error_
> > is
> > > set, binding_pattern_error_ is set identically. From what you're saying,
> there
> > > would still need to be two separate bits in invalid_productions_ however.
> > 
> > Not sure that this works.  Consider:
> > 
> >   ({x.y, 2}) => 45
> > 
> > We're going to want to raise the binding pattern error, not the assignment
> > pattern error.
> 
> My impression was that this is the reason that {assignment, binding}_pattern
> errors are both recorded in the case of a potential assignment pattern error.
> Since the message is identical, I feel like it's possible to eliminate a copy
of
> the error object in this case (as long as the distinction of validating
> specifically for an assignment pattern error, for the case caitp mentioned
> above, still exists in the invalid_productions_ pseudo-bitfield).

Do you expect the same error for these productions?

  ({x.y, 2});
  ({x.y, 2}) => 45;

I would expect the first to give:

  ({x.y, 2});
            ^ Unexpected token: 2

and the second:

  ({x.y, 2}) => 45;
     ^ Unexpected token: .

[conradw, 2015-08-31 13:43:34]

On 2015/08/31 13:13:30, wingo wrote:
> On 2015/08/31 11:22:53, conradw wrote:
> > On 2015/08/28 09:54:13, wingo wrote:
> > >
> https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h
> > > File src/expression-classifier.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h...
> > > src/expression-classifier.h:158: void RecordPatternError(const
> > > Scanner::Location& loc,
> > > On 2015/08/27 15:32:24, conradw wrote:
> > > > I was thinking it might be possible to eliminate/combine the
> > > > assignment_pattern_error_ object, since every time
> assignment_pattern_error_
> > > is
> > > > set, binding_pattern_error_ is set identically. From what you're saying,
> > there
> > > > would still need to be two separate bits in invalid_productions_
however.
> > > 
> > > Not sure that this works.  Consider:
> > > 
> > >   ({x.y, 2}) => 45
> > > 
> > > We're going to want to raise the binding pattern error, not the assignment
> > > pattern error.
> > 
> > My impression was that this is the reason that {assignment, binding}_pattern
> > errors are both recorded in the case of a potential assignment pattern
error.
> > Since the message is identical, I feel like it's possible to eliminate a
copy
> of
> > the error object in this case (as long as the distinction of validating
> > specifically for an assignment pattern error, for the case caitp mentioned
> > above, still exists in the invalid_productions_ pseudo-bitfield).
> 
> Do you expect the same error for these productions?
> 
>   ({x.y, 2});
>   ({x.y, 2}) => 45;
> 
> I would expect the first to give:
> 
>   ({x.y, 2});
>             ^ Unexpected token: 2
> 
> and the second:
> 
>   ({x.y, 2}) => 45;
>      ^ Unexpected token: .

Ah, I see, I completely missed the point you were trying to make first time
around, sorry :P

I suppose it could still be made to work by reporting the shared error
preferentially but I can see how that could make things unintuitive. Seems like
a bad idea then!

[caitp (gmail), 2015-09-03 19:20:13]

On 2015/08/31 13:43:34, conradw wrote:
> On 2015/08/31 13:13:30, wingo wrote:
> > On 2015/08/31 11:22:53, conradw wrote:
> > > On 2015/08/28 09:54:13, wingo wrote:
> > > >
> >
https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h
> > > > File src/expression-classifier.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h...
> > > > src/expression-classifier.h:158: void RecordPatternError(const
> > > > Scanner::Location& loc,
> > > > On 2015/08/27 15:32:24, conradw wrote:
> > > > > I was thinking it might be possible to eliminate/combine the
> > > > > assignment_pattern_error_ object, since every time
> > assignment_pattern_error_
> > > > is
> > > > > set, binding_pattern_error_ is set identically. From what you're
saying,
> > > there
> > > > > would still need to be two separate bits in invalid_productions_
> however.
> > > > 
> > > > Not sure that this works.  Consider:
> > > > 
> > > >   ({x.y, 2}) => 45
> > > > 
> > > > We're going to want to raise the binding pattern error, not the
assignment
> > > > pattern error.
> > > 
> > > My impression was that this is the reason that {assignment,
binding}_pattern
> > > errors are both recorded in the case of a potential assignment pattern
> error.
> > > Since the message is identical, I feel like it's possible to eliminate a
> copy
> > of
> > > the error object in this case (as long as the distinction of validating
> > > specifically for an assignment pattern error, for the case caitp mentioned
> > > above, still exists in the invalid_productions_ pseudo-bitfield).
> > 
> > Do you expect the same error for these productions?
> > 
> >   ({x.y, 2});
> >   ({x.y, 2}) => 45;
> > 
> > I would expect the first to give:
> > 
> >   ({x.y, 2});
> >             ^ Unexpected token: 2
> > 
> > and the second:
> > 
> >   ({x.y, 2}) => 45;
> >      ^ Unexpected token: .
> 
> Ah, I see, I completely missed the point you were trying to make first time
> around, sorry :P
> 
> I suppose it could still be made to work by reporting the shared error
> preferentially but I can see how that could make things unintuitive. Seems
like
> a bad idea then!

I haven't changed the error reporting just yet (need some thought for that), but
I've made some changes:

- destructuring assignment is behind --harmony-destructuring-assignment flag
rather than --harmony-destructuring
- AST node stuff is simplified a bit for preparser's benefit
(IsPatternOrLiteral() is added to full AST and PreParserExpression to support
this for the preparser)

The arrow function test that relies on destructuring assignment isn't fixed
here, I'd like to see if that can be fixed without depending on destructuring
assignment

[wingo, 2015-09-04 12:39:30]

Looking good.  Some open questions on my side.  I think the runtime bit is going
to be gnarly because e.g. we never have a loop inside an expression, currently,
and that is going to throw off all kinds of things in the compiler, seems to me.

https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h#newcode708
src/preparser.h:708: }
I don't see how you are going to implement the desugaring without implementing
an internal version of do expressions, and if you have done that then it seems
to me that the rewrite can be pushed down into the main body of
ParseAssignmentExpression.  WDYT?

Or are you doing this here because it could be that the expression is a valid
assignmentexpression but actually is an arrow function formal parameter?  I
guess that's possible too.  In any case I think a comment is warranted :)

https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h#newcode811
src/preparser.h:811: void SetCoverInitializedNameLocation(int begin, int end)
override {
Some comments here would be great :)

https://codereview.chromium.org/1309813007/diff/10001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1309813007/diff/10001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:7078: kError, NULL, 0, always_flags,
arraysize(always_flags));
Change to kSuccess?  Not sure if the to-do is done tho.

[caitp (gmail), 2015-09-04 13:08:16]

https://codereview.chromium.org/1309813007/diff/10001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1309813007/diff/10001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:7078: kError, NULL, 0, always_flags,
arraysize(always_flags));
On 2015/09/04 12:39:30, wingo wrote:
> Change to kSuccess?  Not sure if the to-do is done tho.

Oops, forgot I deleted the comment.

I don't think we should fix this with destructuring assignment, because it's (as
noted above) not destructuring assignment. Maybe if in the middle of
ParseExpression(), the error should just be delegated until after the
expression, if a `=>` is found or not.

I'll see if I can send a patch to fix that

[caitp (gmail), 2015-09-04 14:26:57]

On 2015/09/04 13:08:16, caitp wrote:
>
https://codereview.chromium.org/1309813007/diff/10001/test/cctest/test-parsin...
> File test/cctest/test-parsing.cc (right):
> 
>
https://codereview.chromium.org/1309813007/diff/10001/test/cctest/test-parsin...
> test/cctest/test-parsing.cc:7078: kError, NULL, 0, always_flags,
> arraysize(always_flags));
> On 2015/09/04 12:39:30, wingo wrote:
> > Change to kSuccess?  Not sure if the to-do is done tho.
> 
> Oops, forgot I deleted the comment.
> 
> I don't think we should fix this with destructuring assignment, because it's
(as
> noted above) not destructuring assignment. Maybe if in the middle of
> ParseExpression(), the error should just be delegated until after the
> expression, if a `=>` is found or not.
> 
> I'll see if I can send a patch to fix that

Hey Andy, PTAL --- It's sort of ugly, but I've added a mechanism to support
this. It looks like it can be mostly deleted once more features are shipping,
too

[caitp (gmail), 2015-09-04 14:42:21]

https://codereview.chromium.org/1309813007/diff/10001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1309813007/diff/10001/src/flag-definitions.h#...
src/flag-definitions.h:416: 
from rebasing, pay no mind

https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h#newcode708
src/preparser.h:708: }
On 2015/09/04 12:39:30, wingo wrote:
> I don't see how you are going to implement the desugaring without implementing
> an internal version of do expressions, and if you have done that then it seems
> to me that the rewrite can be pushed down into the main body of
> ParseAssignmentExpression.  WDYT?
> 
> Or are you doing this here because it could be that the expression is a valid
> assignmentexpression but actually is an arrow function formal parameter?  I
> guess that's possible too.  In any case I think a comment is warranted :)

The main reason for doing the rewriting here, is that it's known that an entire
assignment chain has been parsed by the time this happens.

I'm not sure if it's a better approach or not, I need to test it. I recall some
discussions about the order of operations being slightly different from ordinary
assignments, so I figured it would be safest to just do it all at once in the
desugaring

https://codereview.chromium.org/1309813007/diff/30001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/30001/src/preparser.h#newcode166
src/preparser.h:166: kIsPatternElement = 1 << 1
boolean parameter moved into flags since I needed an extra one (RHS of a binding
pattern is never a binding pattern, so the distinction is needed)

https://codereview.chromium.org/1309813007/diff/30001/src/preparser.h#newcode...
src/preparser.h:3031: PushArrowFormalsState(false);
This never gets popped, but it doesn't really matter. Maybe this doesn't need to
be treated as a stack at all

[caitp (gmail), 2015-09-04 14:55:53]

https://codereview.chromium.org/1309813007/diff/30001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/30001/src/preparser.h#newcode...
src/preparser.h:3031: PushArrowFormalsState(false);
On 2015/09/04 14:42:21, caitp wrote:
> This never gets popped, but it doesn't really matter. Maybe this doesn't need
to
> be treated as a stack at all

I stand corrected, it would do the wrong thing in cases like `"use strict"; (a,
{ x } = yield) => x` without treating it as a proper stack. Fixed this

[caitp (gmail), 2015-09-17 17:05:21]

https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h#newcode811
src/preparser.h:811: void SetCoverInitializedNameLocation(int begin, int end)
override {
On 2015/09/04 12:39:30, wingo wrote:
> Some comments here would be great :)

Done. Actually, unfortunately it's not good enough to do this in the
ObjectLiteralChecker, this has led to the creation of a new test case and an
alternate fix (unfortunately slightly more expensive ._.)

I will leave some comments for it though.

[caitp (gmail), 2015-09-24 09:09:34]

So the latest change involves recording in ExpressionClassifier whether a
CoverInitializedName occurs, which makes the ExpressionClassifier at least 8
bytes bigger, and likely slows down parsing a tiny bit.

The reason for this, compared to the strategy in ObjectLiteralChecker, is so
that the parser can handle expressions like `({ bar: this.y, z: { foo } } =
someObject)` --- previously it was only treated as a pattern if followed by
Token::ASSIGN, which would break nested Object* patterns. So, it's now possible
to use the classifier to store the CoverInitializedName state from nested
properties.

It might be an expensive approach for the corner case, but I don't have a better
idea right now.

---

There are also some slightly messy things tracking whether or not you are
potentially parsing arrow function formal parameters (which is used to defer
reporting of expression errors if Token::ARROW shows up), and tracking if
parsing a property or element or not, so that it's possible to distinguish
whether parsing the top-level literal / pattern or not. This enables deferring
reporting the CoverInitializedName error for expressions until it's known for
sure that an expression is being parsed or not.

These flags are small bits of parser state, but the RAII-style trackers for them
add a bit of code (and, these things could be refactored to share as much as
possible via macros or templates, so that the diff is a bit tinier, not sure if
it's preferred or if it would be better to just manually track these, or if
people think tracking these is pointless)

---

Finally, figuring out the error story seems pretty important, I'm not sure it's
possible to merge both pattern errors, but it would be nice to make the
classifier smaller.

If anyone has some ideas on how to clean this up, so that less stack space is
used, that would be awesome.

[caitp (gmail), 2015-09-24 10:09:11]

https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h#newcode708
src/preparser.h:708: }
On 2015/09/04 12:39:30, wingo wrote:
> I don't see how you are going to implement the desugaring without implementing
> an internal version of do expressions, and if you have done that then it seems
> to me that the rewrite can be pushed down into the main body of
> ParseAssignmentExpression.  WDYT?
> 
> Or are you doing this here because it could be that the expression is a valid
> assignmentexpression but actually is an arrow function formal parameter?  I
> guess that's possible too.  In any case I think a comment is warranted :)

IteratorDestructuringAssignmentEvaluation may require some kind of do-expression
for desugaring, I guess. It might be doable with crazy ternary expression
chains, but that seems horrible, and would produce a scary amount of code.

So I guess the destructuring version is blocked on something like that.

[caitp (gmail), 2015-10-21 12:19:32]

so, the big problem with eagerly rewriting destructuring assignment, is the
ambiguity with arrow functions:


`({ x } = { x: 1 }) => x` -> rewriting the destructuring assignment early breaks
things, because binding patterns need to be rewritten differently


So there are a few ways to solve this:

1. Track whether we might be parsing something that is actually a binding
pattern (this is half-done in the current implementation). This is probably
going to slow down code load a fair bit, and it's a more complicated solution
with different parts all over the place.

2. Keep a list of assignments with an AssignmentPattern LHS, the scope they
occur in, and the expression they belong to (full-parser only), and lazily
rewrite these when parsing a FunctionLiteral, or once parsing is finished.

3. Have a mix of both, and only track pattern-like assignments which occur in
ambiguous situations (OTOH, the only ambiguity is assignments where the LHS is
an ObjectLiteral or ArrayLiteral, which at the top level of a comma-separated
list of expressions to rewrite lazily.

No matter what is picked, it likely has a negative impact on code load, but it
would be nice to minimize it

[caitp (gmail), 2015-10-22 17:45:42]

I've added an alternative implementation, but I'm not positive it's really
adding much over the previous version.

Also, I'm not 100% sure that the ambiguity issue is solved, since this is only
testing parsing. The old rewriting implementation was removed, and there's some
other stuff that I was planning to use but now think is probably not necessary.

I guess I'll send a quick update for this to fix all that

[adamk, 2015-10-27 19:16:58]

adamk@chromium.org changed reviewers:
- aperez@igalia.com, conradw@chromium.org, dehrenberg@chromium.org

[adamk, 2015-10-27 19:16:58]

Back in the USA, and now I've lost state on this. Is this the change I ought to
be reviewing? Or was there more cleanup you wanted to do on it before
continuing?

[caitp (gmail), 2015-10-27 19:18:09]

On 2015/10/27 19:16:58, adamk wrote:
> Back in the USA, and now I've lost state on this. Is this the change I ought
to
> be reviewing? Or was there more cleanup you wanted to do on it before
> continuing?

I'd look at the bits before patch set 10.

Patch set 10 and beyond is mostly trying to get rid of the extra parser state,
but it needs more work

[adamk, 2015-10-27 23:04:18]

It's taking me longer than I'd like to boot up on this (hard to keep the whole
thing in my head), but here's some feedback in the meantime (while I read and
re-read).

https://codereview.chromium.org/1309813007/diff/150001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1309813007/diff/150001/src/ast.h#newcode227
src/ast.h:227: inline bool IsPatternOrLiteral() const {
Nit: "inline" is redundant here

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h
File src/preparser.h (left):

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#oldcod...
src/preparser.h:2684: this->ExpressionUnexpectedToken(classifier);
Sorry, what happened here? Is this supposed to be dependent on the
allow_destructuring_assignment flag?

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcod...
src/preparser.h:777: #define DECLARE_BOOLEAN_STATE_ON_STACK(type, state_var)    
               \
This whole thing looks pretty big for what it's doing.

I'm not convinced you need to add Push and Pop, for one, since it seems like you
should always be able to use RAII. And the Is() accessor seems like unnecessary
boilerplate given that the state is directly available from the parser.

Overall I think this stuff should be as minimal as possible (if you could get
away without the macro that'd be even better).

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcod...
src/preparser.h:800: friend class type##State
No need to friend an inner class.

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcod...
src/preparser.h:2477: bool old_state_ = PushArrowFormalsState(true);
Can't you wrap the next few lines in a block and use an RAII object here instead
of calling these directly?

[caitp (gmail), 2015-10-27 23:20:05]

Thanks --- I'll spend some time addressing these comments, but I'm still leaning
more towards the alt. implementation

https://codereview.chromium.org/1309813007/diff/150001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1309813007/diff/150001/src/ast.h#newcode227
src/ast.h:227: inline bool IsPatternOrLiteral() const {
On 2015/10/27 23:04:18, adamk wrote:
> Nit: "inline" is redundant here

Acknowledged.

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h
File src/preparser.h (left):

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#oldcod...
src/preparser.h:2684: this->ExpressionUnexpectedToken(classifier);
On 2015/10/27 23:04:18, adamk wrote:
> Sorry, what happened here? Is this supposed to be dependent on the
> allow_destructuring_assignment flag?

We can't record this as an ExpressionError here, because it breaks
CoverInitializedNames in arrow formals.

The workaround is the ArrowFormalParametersState thing (in this patch set), or
the `kMaybeBindingPattern` flag in the 2nd version, which I like better. It's
just used to defer reporting of the error.

I haven't tested if it still reports the `=` as an error if the flag isn't used,
but I expect it does.

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcod...
src/preparser.h:777: #define DECLARE_BOOLEAN_STATE_ON_STACK(type, state_var)    
               \
On 2015/10/27 23:04:18, adamk wrote:
> This whole thing looks pretty big for what it's doing.
> 
> I'm not convinced you need to add Push and Pop, for one, since it seems like
you
> should always be able to use RAII. And the Is() accessor seems like
unnecessary
> boilerplate given that the state is directly available from the parser.
> 
> Overall I think this stuff should be as minimal as possible (if you could get
> away without the macro that'd be even better).

Yeah, true

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcod...
src/preparser.h:800: friend class type##State
On 2015/10/27 23:04:18, adamk wrote:
> No need to friend an inner class.

Really? I could have sworn I've seen errors from this kind of thing before

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcod...
src/preparser.h:2477: bool old_state_ = PushArrowFormalsState(true);
On 2015/10/27 23:04:18, adamk wrote:
> Can't you wrap the next few lines in a block and use an RAII object here
instead
> of calling these directly?

As mentioned, the second version gets rid of all this extra parser state, it may
be a simpler. Which one do you think would be preferred?

[adamk, 2015-10-27 23:23:58]

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcod...
src/preparser.h:2477: bool old_state_ = PushArrowFormalsState(true);
On 2015/10/27 23:20:05, caitp wrote:
> On 2015/10/27 23:04:18, adamk wrote:
> > Can't you wrap the next few lines in a block and use an RAII object here
> instead
> > of calling these directly?
> 
> As mentioned, the second version gets rid of all this extra parser state, it
may
> be a simpler. Which one do you think would be preferred?

That comment makes more sense now that I've read the first approach :)

I prefer passing state via params, as that's consistent with what we have
already (e.g., accept_IN). From a quick perusal of your second approach it looks
like that's what it does, but brain too fried for the day (from
eval-parsing-in-arrow-formal-parameters) to give a detailed review of patchsets
10 or 11.

[caitp (gmail), 2015-10-28 05:03:10]

On 2015/10/27 23:23:58, adamk wrote:
> https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h
> File src/preparser.h (right):
> 
>
https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcod...
> src/preparser.h:2477: bool old_state_ = PushArrowFormalsState(true);
> On 2015/10/27 23:20:05, caitp wrote:
> > On 2015/10/27 23:04:18, adamk wrote:
> > > Can't you wrap the next few lines in a block and use an RAII object here
> > instead
> > > of calling these directly?
> > 
> > As mentioned, the second version gets rid of all this extra parser state, it
> may
> > be a simpler. Which one do you think would be preferred?
> 
> That comment makes more sense now that I've read the first approach :)
> 
> I prefer passing state via params, as that's consistent with what we have
> already (e.g., accept_IN). From a quick perusal of your second approach it
looks
> like that's what it does, but brain too fried for the day (from
> eval-parsing-in-arrow-formal-parameters) to give a detailed review of
patchsets
> 10 or 11.

The last change isn't a clean diff because of the rebase, but it turns out I
wasn't on the right issue because had to do the 3-way merge, so there's a clean
version on https://codereview.chromium.org/1411203002

[caitp (gmail), 2015-10-28 05:04:03]

On 2015/10/28 05:03:10, caitp wrote:
> On 2015/10/27 23:23:58, adamk wrote:
> > https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h
> > File src/preparser.h (right):
> > 
> >
>
https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcod...
> > src/preparser.h:2477: bool old_state_ = PushArrowFormalsState(true);
> > On 2015/10/27 23:20:05, caitp wrote:
> > > On 2015/10/27 23:04:18, adamk wrote:
> > > > Can't you wrap the next few lines in a block and use an RAII object here
> > > instead
> > > > of calling these directly?
> > > 
> > > As mentioned, the second version gets rid of all this extra parser state,
it
> > may
> > > be a simpler. Which one do you think would be preferred?
> > 
> > That comment makes more sense now that I've read the first approach :)
> > 
> > I prefer passing state via params, as that's consistent with what we have
> > already (e.g., accept_IN). From a quick perusal of your second approach it
> looks
> > like that's what it does, but brain too fried for the day (from
> > eval-parsing-in-arrow-formal-parameters) to give a detailed review of
> patchsets
> > 10 or 11.
> 
> The last change isn't a clean diff because of the rebase, but it turns out I
> wasn't on the right issue because had to do the 3-way merge, so there's a
clean
> version on https://codereview.chromium.org/1411203002

wait, no, that's not a clean diff either, I guess that was the other part of
this. So no luck then =]

[adamk, 2015-10-28 19:11:41]

Understanding more and more (or so I think)...

https://codereview.chromium.org/1309813007/diff/210001/src/expression-classif...
File src/expression-classifier.h (right):

https://codereview.chromium.org/1309813007/diff/210001/src/expression-classif...
src/expression-classifier.h:319: static const Error& FirdRecorded(const Error&
a, const Error& b) {
Did you mean "FirstRecorded"? Given that there's only a single caller, not sure
it's worth having a whole method here, could just be in ValidateExpression.

https://codereview.chromium.org/1309813007/diff/210001/src/expression-classif...
src/expression-classifier.h:323: if (a.location.beg_pos < 0) return b;
Isn't this covered by the DCHECK above?

https://codereview.chromium.org/1309813007/diff/210001/src/expression-classif...
src/expression-classifier.h:324: if (b.location.beg_pos < 0 ||
b.location.beg_pos >= a.location.beg_pos) {
Same here, I don't imagine pos < 0 is valid.

https://codereview.chromium.org/1309813007/diff/210001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1309813007/diff/210001/src/parser.cc#newcode3363
src/parser.cc:3363: return expression;
Probably keep the TODO here you had before.

https://codereview.chromium.org/1309813007/diff/210001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1309813007/diff/210001/src/parser.h#newcode563
src/parser.h:563: struct MarkedAssignment {
Looks like this snuck in from elsewhere? I don't see it used in this patch. Same
with other marking infrastructure in this file.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:711: enum AssignmentFlags {
This enum deserves a comment, along with a comment for each value. In particular
I find the usage of kIsPatternElement somewhat counter-intuitive.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2449: if (peek() != Token::ARROW) {
Why does this code have to be here instead of handling it like other classifier
errors, in ParseMemberExpressionContinuation and ParseAssignmentExpression?

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2532: ExpressionT result = ParseExpression(accept_IN, 0,
classifier, CHECK_OK);
Seems like 0 should have a name in this enum, too.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2612: if (!allow_harmony_spread_arrays() &&
!allow_harmony_destructuring() &&
FYI spread arrays flag is gone so this if statement is too.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2622: classifier->RecordPatternError(
Why is this not directly calling RecordAssignmentPatternError? Were we getting
this wrong before, or does this have to do with the ambiguity in arrow params?

Same question for the below calls to RecordPatternError.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2631: if (argument->IsAssignment()) {
Seems like this could be combined with the above check into a
ValidateDestructuringRestElement() helper function.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2638: MessageTemplate::kElementAfterRest);
This error is already handled for binding patterns down below, see "if
(seen_spread)". Is the idea that you're giving a better error message here?

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:3068: bool maybe_binding_pattern = flags & kMaybeBindingPattern;
I think you don't need this at all, since I don't see it in a DCHECK below.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:3071: flags & ~(kIsPatternElement | kMaybeBindingPattern) |
kIsRightHandSide;
Please split across two lines (&= and then |=), my brain can do bitwise ops much
more easily that way.

I think a comment would also be worthwhile above those two lines to say what
you're doing.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:4339: // TODO(caitp): do this in a less redundant way :(
I take it this TODO is about the fact that this is redundant with
CheckAndRewriteReferenceExpression?

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:4340: if (expression->IsValidReferenceExpression()) {
I'd do an early return here to reduce indentation:

if (!expression->IsValidReferenceExpression) return false;

[caitp (gmail), 2015-10-28 19:57:34]

https://codereview.chromium.org/1309813007/diff/210001/src/expression-classif...
File src/expression-classifier.h (right):

https://codereview.chromium.org/1309813007/diff/210001/src/expression-classif...
src/expression-classifier.h:324: if (b.location.beg_pos < 0 ||
b.location.beg_pos >= a.location.beg_pos) {
On 2015/10/28 19:11:41, adamk wrote:
> Same here, I don't imagine pos < 0 is valid.

You'd be surprised, I ran into it a few times and added the DCHECK to catch any
more

https://codereview.chromium.org/1309813007/diff/210001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1309813007/diff/210001/src/parser.h#newcode563
src/parser.h:563: struct MarkedAssignment {
On 2015/10/28 19:11:41, adamk wrote:
> Looks like this snuck in from elsewhere? I don't see it used in this patch.
Same
> with other marking infrastructure in this file.

Most of the changes in here aren't needed now, forgot about this one

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:711: enum AssignmentFlags {
On 2015/10/28 19:11:41, adamk wrote:
> This enum deserves a comment, along with a comment for each value. In
particular
> I find the usage of kIsPatternElement somewhat counter-intuitive.

I'll add comments, but the general reasoning is this:

- Tracking whether eagerly desugaring AssigmentPatters might make it harder or
impossible to rewrite BindingPatterns.
- Tracking whether it's possibly ambiguous at all.

Even with the tracking, I'm fairly sure there are still corner cases"

[caitp (gmail), 2015-10-29 16:38:41]

https://codereview.chromium.org/1309813007/diff/210001/src/expression-classif...
File src/expression-classifier.h (right):

https://codereview.chromium.org/1309813007/diff/210001/src/expression-classif...
src/expression-classifier.h:324: if (b.location.beg_pos < 0 ||
b.location.beg_pos >= a.location.beg_pos) {
On 2015/10/28 19:11:41, adamk wrote:
> Same here, I don't imagine pos < 0 is valid.

The above test doesn't guarantee that both errors are valid, only at least one
of them. So if one is invalid (eg beg_pos < 0), the other must be valid.

They're tracked in separate errors because they're propagated differently

https://codereview.chromium.org/1309813007/diff/210001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1309813007/diff/210001/src/parser.h#newcode563
src/parser.h:563: struct MarkedAssignment {
On 2015/10/28 19:57:34, caitp wrote:
> On 2015/10/28 19:11:41, adamk wrote:
> > Looks like this snuck in from elsewhere? I don't see it used in this patch.
> Same
> > with other marking infrastructure in this file.
> 
> Most of the changes in here aren't needed now, forgot about this one

Well, it's likely some variation of this will be needed for te actual
desugaring, but for now its gone

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2449: if (peek() != Token::ARROW) {
On 2015/10/28 19:11:41, adamk wrote:
> Why does this code have to be here instead of handling it like other
classifier
> errors, in ParseMemberExpressionContinuation and ParseAssignmentExpression?

`( expression ) [lookahead ∉ =>]` -> plain expression / possibly
DestructuringAssignment. There isn't enough context to figure this out in
ParseAssignmentExpression, because ParseExpression is not always part of
ParseAssignmentExpression.

in particular, this code used to use a version which always called
ValidateExpression(), but now it does this conditionally depending on flags
(which is a temporary measure). However, I suspect that it will probably eagerly
rewrite destructuring assignments if and only if any are found here, so there
will probably still be some extra code like this in the future.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2532: ExpressionT result = ParseExpression(accept_IN, 0,
classifier, CHECK_OK);
On 2015/10/28 19:11:41, adamk wrote:
> Seems like 0 should have a name in this enum, too.

Acknowledged.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2622: classifier->RecordPatternError(
On 2015/10/28 19:11:41, adamk wrote:
> Why is this not directly calling RecordAssignmentPatternError? Were we getting
> this wrong before, or does this have to do with the ambiguity in arrow params?
> 
> Same question for the below calls to RecordPatternError.

It's an invalid binding pattern as well as invalid assignment pattern, but
there's no information about which one we think we're parsing, and even if we
think we're expecting an assignment, the `( expression ) [maybe =>]` case makes
it ambiguous. So this one records the error for both.

The only case, afaik, where it's an error for binding patterns but not
assignment patterns, is when the BindingElement is a property reference. But
adding a 3rd error class just for that seems like the wrong approach.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2631: if (argument->IsAssignment()) {
On 2015/10/28 19:11:41, adamk wrote:
> Seems like this could be combined with the above check into a
> ValidateDestructuringRestElement() helper function.

Isn't this going to be the only caller of that function, unless
https://github.com/sebmarkbage/ecmascript-rest-spread gets ratified? (I guess it
is at stage 2...)

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2638: MessageTemplate::kElementAfterRest);
On 2015/10/28 19:11:41, adamk wrote:
> This error is already handled for binding patterns down below, see "if
> (seen_spread)". Is the idea that you're giving a better error message here?

Yeah, I think the message is better than "unexpected token...". I guess the
redundant code should be removed

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:3068: bool maybe_binding_pattern = flags & kMaybeBindingPattern;
On 2015/10/28 19:11:41, adamk wrote:
> I think you don't need this at all, since I don't see it in a DCHECK below.

It's used to determine whether destructuring assignment should be eagerly
rewritten. We can't eagerly rewrite or eagerly validate as an AssignmentPattern
if it could be a BindingPattern (as in left-most ObjectLiteral / ArrayLiterals
in parenthesized Expressions)

The USE() isn't really needed, though.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:3071: flags & ~(kIsPatternElement | kMaybeBindingPattern) |
kIsRightHandSide;
On 2015/10/28 19:11:41, adamk wrote:
> Please split across two lines (&= and then |=), my brain can do bitwise ops
much
> more easily that way.
> 
> I think a comment would also be worthwhile above those two lines to say what
> you're doing.

Acknowledged.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:4339: // TODO(caitp): do this in a less redundant way :(
On 2015/10/28 19:11:41, adamk wrote:
> I take it this TODO is about the fact that this is redundant with
> CheckAndRewriteReferenceExpression?

Yes, will clarify that

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:4340: if (expression->IsValidReferenceExpression()) {
On 2015/10/28 19:11:41, adamk wrote:
> I'd do an early return here to reduce indentation:
> 
> if (!expression->IsValidReferenceExpression) return false;

Acknowledged.

[adamk, 2015-10-29 23:43:21]

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2622: classifier->RecordPatternError(
On 2015/10/29 16:38:41, caitp wrote:
> On 2015/10/28 19:11:41, adamk wrote:
> > Why is this not directly calling RecordAssignmentPatternError? Were we
getting
> > this wrong before, or does this have to do with the ambiguity in arrow
params?
> > 
> > Same question for the below calls to RecordPatternError.
> 
> It's an invalid binding pattern as well as invalid assignment pattern, but
> there's no information about which one we think we're parsing, and even if we
> think we're expecting an assignment, the `( expression ) [maybe =>]` case
makes
> it ambiguous. So this one records the error for both.

Sorry, I didn't highlight the most important question: was this already broken
for binding patterns? What confuses me about these checks is that I would have
assumed that our destructuring binding tests would already exercise these paths.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2638: MessageTemplate::kElementAfterRest);
On 2015/10/29 16:38:41, caitp wrote:
> On 2015/10/28 19:11:41, adamk wrote:
> > This error is already handled for binding patterns down below, see "if
> > (seen_spread)". Is the idea that you're giving a better error message here?
> 
> Yeah, I think the message is better than "unexpected token...". I guess the
> redundant code should be removed

There are plenty of existing places where we use BindingPatternUnexpectedToken,
it's not clear to me why this case should be special.

[caitp (gmail), 2015-10-30 00:57:19]

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2622: classifier->RecordPatternError(
On 2015/10/29 23:43:21, adamk wrote:
> On 2015/10/29 16:38:41, caitp wrote:
> > On 2015/10/28 19:11:41, adamk wrote:
> > > Why is this not directly calling RecordAssignmentPatternError? Were we
> getting
> > > this wrong before, or does this have to do with the ambiguity in arrow
> params?
> > > 
> > > Same question for the below calls to RecordPatternError.
> > 
> > It's an invalid binding pattern as well as invalid assignment pattern, but
> > there's no information about which one we think we're parsing, and even if
we
> > think we're expecting an assignment, the `( expression ) [maybe =>]` case
> makes
> > it ambiguous. So this one records the error for both.
> 
> Sorry, I didn't highlight the most important question: was this already broken
> for binding patterns? What confuses me about these checks is that I would have
> assumed that our destructuring binding tests would already exercise these
paths.

It's old code now, presumably something failed at some point, maybe still does
if it's removed. OTOH I don't know, though.

https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcod...
src/preparser.h:2638: MessageTemplate::kElementAfterRest);
On 2015/10/29 23:43:21, adamk wrote:
> On 2015/10/29 16:38:41, caitp wrote:
> > On 2015/10/28 19:11:41, adamk wrote:
> > > This error is already handled for binding patterns down below, see "if
> > > (seen_spread)". Is the idea that you're giving a better error message
here?
> > 
> > Yeah, I think the message is better than "unexpected token...". I guess the
> > redundant code should be removed
> 
> There are plenty of existing places where we use
BindingPatternUnexpectedToken,
> it's not clear to me why this case should be special.

For one thing, the diagnostic is inaccurate. It's not an "unexpected token", the
parser is explicitly allowing this token to occur, and reporting an error
depending on context. So, given that, why not make the error actionable, by
relating it to the context that it depends on? This is what makes diagnostic
messages useful.

This doesn't apply to every use of BindingPatternUnexpectedToken(), but it does
apply to a few of them

[adamk, 2015-11-02 20:46:30]

It seems to me that supporting parsing of destructuring assignment and improving
error messages for destructuring parse failures are separable tasks. I'd find
patches for the former easier to read if they didn't also make changes to the
latter at the same time.

[caitp (gmail), 2015-11-03 17:43:46]

just making a few notes (stealing a small portion of this CL, noticed some
things that aren't needed now)

https://codereview.chromium.org/1309813007/diff/330001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/330001/src/preparser.h#newcod...
src/preparser.h:1023:
HasCoverInitializedNameField::encode(has_cover_initialized_name));
note: delete HasCoverInitializedNameField when time permits

https://codereview.chromium.org/1309813007/diff/330001/src/preparser.h#newcod...
src/preparser.h:1089: return *this;
note: delete method when time permits

https://codereview.chromium.org/1309813007/diff/330001/src/preparser.h#newcod...
src/preparser.h:1216: typedef BitField<bool, TypeField::kNext, 1>
HasCoverInitializedNameField;
note: delete this when time permits

[caitp (gmail), 2015-11-17 02:51:33]

Okay, updated with an implementation.

It's pretty messy, but it seems to work locally. Not sure how much it can be
cleaned up without a new parser

[caitp (gmail), 2015-11-17 03:03:20]

On 2015/11/17 02:51:33, caitp wrote:
> Okay, updated with an implementation.
> 
> It's pretty messy, but it seems to work locally. Not sure how much it can be
> cleaned up without a new parser

but even if it does work, and ends up passing test262 and whatever else, I'd
still feel really bad checking this in :/

[caitp (gmail), 2015-11-17 04:13:17]

On 2015/11/17 03:03:20, caitp wrote:
> On 2015/11/17 02:51:33, caitp wrote:
> > Okay, updated with an implementation.
> > 
> > It's pretty messy, but it seems to work locally. Not sure how much it can be
> > cleaned up without a new parser
> 
> but even if it does work, and ends up passing test262 and whatever else, I'd
> still feel really bad checking this in :/

Seems to work ok. two issues, one is the 5th stress-opt run for the
testArrayLiteralProperty thing in mjsunit/harmony/destructuring-assignment
(seems to fail to deal with the AssignmentPattern node correctly or something
when processing the spread) --- the other one is just a bug that caused
object/array-literal LHS to not be a syntax error, and thus caused some DCHECKS
to be hit. The second one is easily fixable, I'm not sure about the first one.

[adamk, 2015-11-20 19:49:48]

Having read through a few times, I have some high-level questions:

- You mentioned on IRC that the keeping-track-of-assigments-to-rewrite thing
didn't interact well with something else. Was that resolved? Overall, do you
know of any cases where this doesn't work correctly?

- It seems a bit strange that the AssignmentPattern AST node subsumes the entire
Assignment. This makes the code in full-codegen look odd, since only the target
is visited and not the value. Was there a reason you didn't put more of this
logic in Assignment?

[caitp (gmail), 2015-11-20 19:56:45]

On 2015/11/20 19:49:48, adamk wrote:
> Having read through a few times, I have some high-level questions:
> 
> - You mentioned on IRC that the keeping-track-of-assigments-to-rewrite thing
> didn't interact well with something else. Was that resolved? Overall, do you
> know of any cases where this doesn't work correctly?
> 

I believe that was resolved --- the last issues are the ones mentioned in my
previous comment.

> - It seems a bit strange that the AssignmentPattern AST node subsumes the
entire
> Assignment. This makes the code in full-codegen look odd, since only the
target
> is visited and not the value. Was there a reason you didn't put more of this
> logic in Assignment?

The AssignmentPattern node is basically a stand-in for the rewritten expression.
It's unfortunate that it's done this way, but since the Parser is doing the
desugaring, I'm not sure of a better way to do it.

[adamk, 2015-11-20 20:02:24]

On 2015/11/20 19:56:45, caitp wrote:
> On 2015/11/20 19:49:48, adamk wrote:
> > Having read through a few times, I have some high-level questions:
> > 
> > - You mentioned on IRC that the keeping-track-of-assigments-to-rewrite thing
> > didn't interact well with something else. Was that resolved? Overall, do you
> > know of any cases where this doesn't work correctly?
> > 
> 
> I believe that was resolved --- the last issues are the ones mentioned in my
> previous comment.

Ah, I didn't see that comment (didn't get emailed to me for some reason).

The stress-opt problem sounds possibly bad, I'd be interested in more
investigation there. Note that there has been some recent churn in TurboFan
support for spread.

> > - It seems a bit strange that the AssignmentPattern AST node subsumes the
> entire
> > Assignment. This makes the code in full-codegen look odd, since only the
> target
> > is visited and not the value. Was there a reason you didn't put more of this
> > logic in Assignment?
> 
> The AssignmentPattern node is basically a stand-in for the rewritten
expression.
> It's unfortunate that it's done this way, but since the Parser is doing the
> desugaring, I'm not sure of a better way to do it.

But why isn't this handled by the Assignment node, rather than being buried down
in the target? It's really the whole Assignment that gets rewritten, not just
the target.

[caitp (gmail), 2015-11-20 20:05:41]

On 2015/11/20 20:02:24, adamk wrote:
> On 2015/11/20 19:56:45, caitp wrote:
> > On 2015/11/20 19:49:48, adamk wrote:
> > > Having read through a few times, I have some high-level questions:
> > > 
> > > - You mentioned on IRC that the keeping-track-of-assigments-to-rewrite
thing
> > > didn't interact well with something else. Was that resolved? Overall, do
you
> > > know of any cases where this doesn't work correctly?
> > > 
> > 
> > I believe that was resolved --- the last issues are the ones mentioned in my
> > previous comment.
> 
> Ah, I didn't see that comment (didn't get emailed to me for some reason).
> 
> The stress-opt problem sounds possibly bad, I'd be interested in more
> investigation there. Note that there has been some recent churn in TurboFan
> support for spread.
> 
> > > - It seems a bit strange that the AssignmentPattern AST node subsumes the
> > entire
> > > Assignment. This makes the code in full-codegen look odd, since only the
> > target
> > > is visited and not the value. Was there a reason you didn't put more of
this
> > > logic in Assignment?
> > 
> > The AssignmentPattern node is basically a stand-in for the rewritten
> expression.
> > It's unfortunate that it's done this way, but since the Parser is doing the
> > desugaring, I'm not sure of a better way to do it.
> 
> But why isn't this handled by the Assignment node, rather than being buried
down
> in the target? It's really the whole Assignment that gets rewritten, not just
> the target.

Because, there isn't really a way to go in and completely replace the Assignment
with the rewritten version, since there's no way to access the parent node and
insert the replacement into the right place --- so, instead, the LHS is a
stand-in for the rewritten Assignment. It's a hack, but I'm not sure there's a
better way here, given that the parser is doing the desugaring

[caitp (gmail), 2015-11-20 21:42:38]

On 2015/11/20 20:05:41, caitp wrote:
> On 2015/11/20 20:02:24, adamk wrote:
> > On 2015/11/20 19:56:45, caitp wrote:
> > > On 2015/11/20 19:49:48, adamk wrote:
> > > > Having read through a few times, I have some high-level questions:
> > > > 
> > > > - You mentioned on IRC that the keeping-track-of-assigments-to-rewrite
> thing
> > > > didn't interact well with something else. Was that resolved? Overall, do
> you
> > > > know of any cases where this doesn't work correctly?
> > > > 
> > > 
> > > I believe that was resolved --- the last issues are the ones mentioned in
my
> > > previous comment.
> > 
> > Ah, I didn't see that comment (didn't get emailed to me for some reason).
> > 
> > The stress-opt problem sounds possibly bad, I'd be interested in more
> > investigation there. Note that there has been some recent churn in TurboFan
> > support for spread.
> > 
> > > > - It seems a bit strange that the AssignmentPattern AST node subsumes
the
> > > entire
> > > > Assignment. This makes the code in full-codegen look odd, since only the
> > > target
> > > > is visited and not the value. Was there a reason you didn't put more of
> this
> > > > logic in Assignment?
> > > 
> > > The AssignmentPattern node is basically a stand-in for the rewritten
> > expression.
> > > It's unfortunate that it's done this way, but since the Parser is doing
the
> > > desugaring, I'm not sure of a better way to do it.
> > 
> > But why isn't this handled by the Assignment node, rather than being buried
> down
> > in the target? It's really the whole Assignment that gets rewritten, not
just
> > the target.
> 
> Because, there isn't really a way to go in and completely replace the
Assignment
> with the rewritten version, since there's no way to access the parent node and
> insert the replacement into the right place --- so, instead, the LHS is a
> stand-in for the rewritten Assignment. It's a hack, but I'm not sure there's a
> better way here, given that the parser is doing the desugaring

Per discussion on IRC, you seem to be saying that you'd prefer storing the
entire Assignment in the list, and storing the rewritten expression in all
Assignment nodes --- is that right? But you'd still have to, in the compiler,
avoid doing the normal Assignment node work.

To me, either way it seems like a horrible hack, and either way it's going to be
fixed later once it's possible. I can make the change if that's preferred, but
it's not a significant difference, and still needs little hacks added to each
full-codegen implementation

[adamk, 2015-11-20 22:42:59]

Another source of tests for this would be the test262 tests in
language/expressions/assignment/destructuring (currently all skipped).

Here are some comments on the current patch. This looks like a good direction to
me; I'd really like to see stuff moved up into Assignment, though (I've noted
another problem with the current approach: the strange treatment of
AssignmentPattern when it's visited by various AST visitors).

https://codereview.chromium.org/1309813007/diff/350001/src/ast-expression-vis...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1309813007/diff/350001/src/ast-expression-vis...
src/ast-expression-visitor.cc:235:
RECURSE_EXPRESSION(Visit(expr->expression()));
For each visitor, how did you decide whether to visit the expression vs the
pattern? This is another reason why it would make more sense to handle this in
VisitAssignment, it's sort of meaningless to visit an assignment pattern without
more context.

https://codereview.chromium.org/1309813007/diff/350001/src/crankshaft/hydroge...
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1309813007/diff/350001/src/crankshaft/hydroge...
src/crankshaft/hydrogen.cc:5864: void
HOptimizedGraphBuilder::VisitAssignmentPattern(AssignmentPattern* expr) {
I don't think this is sufficient to bail out of Crankshaft. I suspect you need
to modify VisitAssignment, otherwise you'll get DCHECK failures there.

https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h...
src/flag-definitions.h:678: DEFINE_BOOL(parallel_compaction, true, "use parallel
compaction")
Looks like a stray change here

https://codereview.chromium.org/1309813007/diff/350001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1309813007/diff/350001/src/parser.h#newcode1430
src/parser.h:1430: for (int i = assignments.length() - 1; i >= 0; --i) {
Any particular reason for iterating backwards here?

https://codereview.chromium.org/1309813007/diff/350001/src/parser.h#newcode1451
src/parser.h:1451: void
ParserTraits::ShouldRewriteDestructuringAssignment(Expression* expr) {
The name of this function doesn't make sense to me. Perhaps 
"QueueDestructuringAssignmentForRewriting"? "Should" prefix suggests a const
method that answers a question.

https://codereview.chromium.org/1309813007/diff/350001/src/parser.h#newcode1452
src/parser.h:1452: DCHECK(expr->IsAssignment());
Why can't this method just take an Assignment* to avoid this DCHECK?

https://codereview.chromium.org/1309813007/diff/350001/src/parser.h#newcode1460
src/parser.h:1460: inline void
Parser::PatternRewriter::VisitObjectLiteral(ObjectLiteral* node) {
Can these methods move to the .cc file? Seems like this header's already plenty
big.

https://codereview.chromium.org/1309813007/diff/350001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1309813007/diff/350001/src/pattern-rewriter.c...
src/pattern-rewriter.cc:432: DoExpression* expr =
factory()->NewDoExpression(block_, temp, pos);
Looks like this creates a DoExpression even for destructured binding? Seems
weird to do that only to wrap it in an ExpressionStatement below...

https://codereview.chromium.org/1309813007/diff/350001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/350001/src/preparser.h#newcod...
src/preparser.h:279: List<DestructuringAssignment>
destructuring_assignments_to_rewrite_;
Why does ParserBase need this List? Couldn't it just be in Parser?

[caitp (gmail), 2015-11-20 23:10:25]

I'll update the patch set Monday

https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h...
src/flag-definitions.h:678: DEFINE_BOOL(parallel_compaction, true, "use parallel
compaction")
On 2015/11/20 22:42:58, adamk wrote:
> Looks like a stray change here

It's from a rebase, so not everything in the inter diff is mine

[adamk, 2015-11-20 23:16:55]

https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h...
src/flag-definitions.h:678: DEFINE_BOOL(parallel_compaction, true, "use parallel
compaction")
On 2015/11/20 23:10:25, caitp wrote:
> On 2015/11/20 22:42:58, adamk wrote:
> > Looks like a stray change here
> 
> It's from a rebase, so not everything in the inter diff is mine

This shows up in the whole diff, not the interdiff.

[caitp (gmail), 2015-11-23 16:42:50]

On 2015/11/20 23:16:55, adamk wrote:
> https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h
> File src/flag-definitions.h (right):
> 
>
https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h...
> src/flag-definitions.h:678: DEFINE_BOOL(parallel_compaction, true, "use
parallel
> compaction")
> On 2015/11/20 23:10:25, caitp wrote:
> > On 2015/11/20 22:42:58, adamk wrote:
> > > Looks like a stray change here
> > 
> > It's from a rebase, so not everything in the inter diff is mine
> 
> This shows up in the whole diff, not the interdiff.

I think that's just the way Rietveld is, it's still a diff against the base
revision of the patch, so weird stuff shows up after a rebase. This stuff was
added in CLs by other people from last week, and it just shows up here because
it wasn't in the original base revision. In any case, I'll do another rebase and
see if I can make the impl look cleaner to you (I'm fairly sure it's still going
to look like a hack though)

[caitp (gmail), 2015-11-24 17:42:23]

Got the test262 run passing most of the destructuring assignment tests
(remaining ones that are still bad are all related to SetFunctionName, which
isn't done yet in V8)

[caitp (gmail), 2015-11-24 17:42:26]

Got the test262 run passing most of the destructuring assignment tests
(remaining ones that are still bad are all related to SetFunctionName, which
isn't done yet in V8)

[caitp (gmail), 2015-11-24 18:06:39]

So, there are still some mixups in the AST visitors, and it would probably be a
good idea to get feedback on the design just to make sure I actually understood
what you were asking for.

https://codereview.chromium.org/1309813007/diff/410001/src/ast-expression-vis...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1309813007/diff/410001/src/ast-expression-vis...
src/ast-expression-visitor.cc:268:
RETURN_IF_VISIT_NODE(expr->destructuring_assignment());
One of these isn't needed, and I'm not sure which makes more sense

https://codereview.chromium.org/1309813007/diff/410001/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/1309813007/diff/410001/src/ast-numbering.cc#n...
src/ast-numbering.cc:352:
RETURN_IF_VISIT_NODE(node->destructuring_assignment());
Should this happen before IncrementNodeCount(), or after?

https://codereview.chromium.org/1309813007/diff/410001/src/full-codegen/x64/f...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1309813007/diff/410001/src/full-codegen/x64/f...
src/full-codegen/x64/full-codegen-x64.cc:1769:
RETURN_IF_VISIT_NODE(expr->destructuring_assignment());
Unfortunately, even with the removed AST node, this still means adding little
bits like this to every VisitAssignment() in the tree

[adamk, 2015-11-24 19:52:10]

Overall I really do prefer putting this stuff in Assignment. A few questions
about other additions, but I think this is on the right track.

https://codereview.chromium.org/1309813007/diff/410001/src/expression-classif...
File src/expression-classifier.h (right):

https://codereview.chromium.org/1309813007/diff/410001/src/expression-classif...
src/expression-classifier.h:165: void RecordExpressionError(const
Scanner::Location& loc,
Was this added as a cleanup or to fix something that was broken before?

https://codereview.chromium.org/1309813007/diff/410001/src/full-codegen/x64/f...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1309813007/diff/410001/src/full-codegen/x64/f...
src/full-codegen/x64/full-codegen-x64.cc:1769:
RETURN_IF_VISIT_NODE(expr->destructuring_assignment());
On 2015/11/24 18:06:39, caitp wrote:
> Unfortunately, even with the removed AST node, this still means adding little
> bits like this to every VisitAssignment() in the tree

To my eyes that's a definite improvement: you're effectively replacing the
assignment with a different expression, so any visit to the assignment's
target/value should be replaced by a visit to the destructured expression.

https://codereview.chromium.org/1309813007/diff/410001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1309813007/diff/410001/src/parser.h#newcode1065
src/parser.h:1065: ASSIGNMENT_INITIALIZER
Was this added to fix the mentioned test262 tests? If so can you give an example
of what it fixes?

[caitp (gmail), 2015-11-24 20:26:27]

Thanks for the comments --- I think the various AST visitor questions I asked
are probably responsible for the failing test case, so I'm going to see if I can
get that fixed.

https://codereview.chromium.org/1309813007/diff/410001/src/expression-classif...
File src/expression-classifier.h (right):

https://codereview.chromium.org/1309813007/diff/410001/src/expression-classif...
src/expression-classifier.h:165: void RecordExpressionError(const
Scanner::Location& loc,
On 2015/11/24 19:52:09, adamk wrote:
> Was this added as a cleanup or to fix something that was broken before?

Previously, CheckAndRewriteReferenceExpression() would immediately report an
error, which may have been a ReferenceError or SyntaxError

This change makes it possible to delay reporting the error in the event an
ObjectLiteral/ArrayLiteral LHS is used in a maybe-expression-maybe-arrow-formals
context. So, to do this, the error type has to be recordable (otherwise we
violate the spec by throwing SyntaxErrors that need to be ReferenceErrors, etc).

https://codereview.chromium.org/1309813007/diff/410001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1309813007/diff/410001/src/parser.h#newcode1065
src/parser.h:1065: ASSIGNMENT_INITIALIZER
On 2015/11/24 19:52:10, adamk wrote:
> Was this added to fix the mentioned test262 tests? If so can you give an
example
> of what it fixes?

This was added to distinguish between AssignmentPattern initializers and
BindingPattern initializers. Since we rewrite variable proxies in certain
function parameter initializers, and this should not happen (AFAICT) for
initializers in assignment patterns.

So it makes it possible to keep the other initializer behaviours (`value =
IS_UNDEFINED(current_value_) ? initializer : current_value_`) while still
distinguishing between the binding context and assignment contexts, in order to
keep that rewriting behaviour working

[caitp (gmail), 2015-11-24 21:37:29]

caitpotter88@gmail.com changed reviewers:
+ yangguo@chromium.org

[caitp (gmail), 2015-11-24 21:37:29]

+yang, do you have an idea what would cause the DCHECK failure in
VerifyRecompiledCode()? It doesn't seem to break the code when DCHECKS are
disabled.

[caitp (gmail), 2015-11-25 00:04:51]

On 2015/11/24 21:37:29, caitp wrote:
> +yang, do you have an idea what would cause the DCHECK failure in
> VerifyRecompiledCode()? It doesn't seem to break the code when DCHECKS are
> disabled.

More context, old_target->kind() == BUILTIN, while new_target->kind() == STUB

[Yang, 2015-11-25 01:41:13]

On 2015/11/25 00:04:51, caitp wrote:
> On 2015/11/24 21:37:29, caitp wrote:
> > +yang, do you have an idea what would cause the DCHECK failure in
> > VerifyRecompiledCode()? It doesn't seem to break the code when DCHECKS are
> > disabled.
> 
> More context, old_target->kind() == BUILTIN, while new_target->kind() == STUB

Caitlin,

this check makes sure that when we recompile code for debugging (to get code
with debug break slots), the new code is in sync with the old one so that we can
find the matching pc when replacing old code while it is on the stack. We match
up positions in both code objects by counting calls up to the current pc.
Therefore we expect each call to correspond to each other.

If you are not doing weird tricks like patching call targets after compile, the
corresponding call (i.e. same number of calls preceding it) should not target
different code kinds.

Let me know if you found anything. I can try to figure this one out in the
morning if you havent yet.

[Yang, 2015-11-25 01:44:58]

On 2015/11/25 01:41:13, Yang wrote:
> On 2015/11/25 00:04:51, caitp wrote:
> > On 2015/11/24 21:37:29, caitp wrote:
> > > +yang, do you have an idea what would cause the DCHECK failure in
> > > VerifyRecompiledCode()? It doesn't seem to break the code when DCHECKS are
> > > disabled.
> > 
> > More context, old_target->kind() == BUILTIN, while new_target->kind() ==
STUB
> 
> Caitlin,
> 
> this check makes sure that when we recompile code for debugging (to get code
> with debug break slots), the new code is in sync with the old one so that we
can
> find the matching pc when replacing old code while it is on the stack. We
match
> up positions in both code objects by counting calls up to the current pc.
> Therefore we expect each call to correspond to each other.
> 
> If you are not doing weird tricks like patching call targets after compile,
the
> corresponding call (i.e. same number of calls preceding it) should not target
> different code kinds.
> 
> Let me know if you found anything. I can try to figure this one out in the
> morning if you havent yet.

Btw this likely happens because the recompile took a different code path. That
could be because of lazy vs eager parsing, because of debugger being active, or
maybe because of subtle inderminism in the parser/compiler.

[caitp (gmail), 2015-11-25 17:28:14]

On 2015/11/25 01:44:58, Yang wrote:
> On 2015/11/25 01:41:13, Yang wrote:
> > On 2015/11/25 00:04:51, caitp wrote:
> > > On 2015/11/24 21:37:29, caitp wrote:
> > > > +yang, do you have an idea what would cause the DCHECK failure in
> > > > VerifyRecompiledCode()? It doesn't seem to break the code when DCHECKS
are
> > > > disabled.
> > > 
> > > More context, old_target->kind() == BUILTIN, while new_target->kind() ==
> STUB
> > 
> > Caitlin,
> > 
> > this check makes sure that when we recompile code for debugging (to get code
> > with debug break slots), the new code is in sync with the old one so that we
> can
> > find the matching pc when replacing old code while it is on the stack. We
> match
> > up positions in both code objects by counting calls up to the current pc.
> > Therefore we expect each call to correspond to each other.
> > 
> > If you are not doing weird tricks like patching call targets after compile,
> the
> > corresponding call (i.e. same number of calls preceding it) should not
target
> > different code kinds.
> > 
> > Let me know if you found anything. I can try to figure this one out in the
> > morning if you havent yet.
> 
> Btw this likely happens because the recompile took a different code path. That
> could be because of lazy vs eager parsing, because of debugger being active,
or
> maybe because of subtle inderminism in the parser/compiler.

DCHECK failure resolved, although it still doesn't make a ton of sense to me why
(related to parameter initializer scopes not being rewritten, consistently)

[caitp (gmail), 2015-11-25 20:05:53]

Patchset #24 (id:450001) has been deleted

[caitp (gmail), 2015-11-25 20:06:00]

Patchset #23 (id:430001) has been deleted

[caitp (gmail), 2015-11-25 20:06:08]

Patchset #22 (id:410001) has been deleted

[caitp (gmail), 2015-11-25 20:06:32]

Patchset #17 (id:310001) has been deleted

[caitp (gmail), 2015-11-25 20:06:57]

Patchset #12 (id:210001) has been deleted

[caitp (gmail), 2015-11-25 20:07:13]

Patchset #6 (id:90001) has been deleted

[caitp (gmail), 2015-11-25 20:07:20]

Patchset #5 (id:70001) has been deleted

[caitp (gmail), 2015-11-25 20:07:29]

Patchset #4 (id:50001) has been deleted

[caitp (gmail), 2015-11-25 20:07:35]

Patchset #3 (id:30001) has been deleted

[caitp (gmail), 2015-11-25 20:07:42]

Patchset #2 (id:10001) has been deleted

[caitp (gmail), 2015-11-25 20:07:58]

Patchset #1 (id:1) has been deleted

[caitp (gmail), 2015-11-25 20:08:09]

Patchset #1 (id:110001) has been deleted

[caitp (gmail), 2015-11-25 20:08:22]

Patchset #1 (id:130001) has been deleted

[caitp (gmail), 2015-11-25 20:08:32]

Patchset #1 (id:150001) has been deleted

[caitp (gmail), 2015-11-25 20:08:45]

Patchset #5 (id:270001) has been deleted

[caitp (gmail), 2015-11-25 20:09:00]

Patchset #3 (id:230001) has been deleted

[caitp (gmail), 2015-11-25 20:09:11]

Patchset #1 (id:170001) has been deleted

[caitp (gmail), 2015-11-25 20:09:21]

Patchset #1 (id:190001) has been deleted

[caitp (gmail), 2015-11-25 20:09:34]

Patchset #1 (id:250001) has been deleted

[caitp (gmail), 2015-11-25 20:09:43]

Patchset #1 (id:290001) has been deleted

[caitp (gmail), 2015-11-25 20:09:51]

Patchset #1 (id:330001) has been deleted

[adamk, 2015-11-25 21:05:28]

I understand more and more each time, quite exciting.

https://codereview.chromium.org/1309813007/diff/470001/src/ast-expression-vis...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1309813007/diff/470001/src/ast-expression-vis...
src/ast-expression-visitor.cc:270:
RECURSE_EXPRESSION_RETURN_IF_VISIT_NODE(expr->destructuring_assignment());
Is this needed because VisitExpression could mutate |expr|?

https://codereview.chromium.org/1309813007/diff/470001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1309813007/diff/470001/src/ast.h#newcode2379
src/ast.h:2379: Expression* destructuring_assignment() { return
destructuring_assignment_; }
Looks like this is a non-const dup of the above accessor, I don't think you need
it.

https://codereview.chromium.org/1309813007/diff/470001/src/ast.h#newcode2402
src/ast.h:2402: : public BitField16<bool, TokenField::kNext, 1> {};
Can you use kNext for the above lines while you're at it?

https://codereview.chromium.org/1309813007/diff/470001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1309813007/diff/470001/src/parser.h#newcode1095
src/parser.h:1095: PatternContext SetInitializerContextIfNeeded(Expression*
node) {
Any reason all of these functions need to be in the .h file? If it's for
inlining, that's not needed; these are private methods and all their callers are
on this class, so they could just as well be inline and defined in the .cc file.

https://codereview.chromium.org/1309813007/diff/470001/src/parser.h#newcode1473
src/parser.h:1473: void Parser::RewriteDestructuringAssignments() {
Another "why is this in the header?" question.

https://codereview.chromium.org/1309813007/diff/470001/src/parser.h#newcode1496
src/parser.h:1496: void
ParserTraits::ShouldRewriteDestructuringAssignment(Expression* expr) {
And here...

Also, as mentioned before, this name is confusing. How about
"QueueDestructuringAssignmentForRewriting" (also long, but clearer what it's
doing).

https://codereview.chromium.org/1309813007/diff/470001/src/parser.h#newcode1506
src/parser.h:1506: inline void
Parser::PatternRewriter::VisitObjectLiteral(ObjectLiteral* node) {
And here and the below. parser.h is long enough as it is :)

https://codereview.chromium.org/1309813007/diff/470001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1309813007/diff/470001/src/pattern-rewriter.c...
src/pattern-rewriter.cc:276: set_context(context);
Can you use an RAII object for this instead of relying on set_context at the end
of the block to handle this?

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcod...
src/preparser.h:279: List<DestructuringAssignment>
destructuring_assignments_to_rewrite_;
Please move this to Parser unless there's some reason it's needed in the
preparser.

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcod...
src/preparser.h:281: void RewriteDestructuringAssignments();
I don't see this defined anywhere (and I don't think it needs to be defined).

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcod...
src/preparser.h:2493: if (peek() != Token::ARROW) {
Sorry, I've forgotten if you've answered this already: why are doing this
peeking here now (what does it have to do with parsing destructuring assignment,
that is)?

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcod...
src/preparser.h:2632: bool seen_spread = false;
This bool is now unused.

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcod...
src/preparser.h:3108: USE(is_arrow_formals);
This USE is not needed.

[caitp (gmail), 2015-11-25 21:41:52]

I'll get to these in a bit, adding some more tests first

https://codereview.chromium.org/1309813007/diff/470001/src/ast-expression-vis...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1309813007/diff/470001/src/ast-expression-vis...
src/ast-expression-visitor.cc:270:
RECURSE_EXPRESSION_RETURN_IF_VISIT_NODE(expr->destructuring_assignment());
On 2015/11/25 21:05:28, adamk wrote:
> Is this needed because VisitExpression could mutate |expr|?

I suppose it can, the InitializerRewriter  visitor does that, I think. AFAIK the
top-most RETURN_IF_VISIT_NODE probably should be removed, but I'm not positive

https://codereview.chromium.org/1309813007/diff/470001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1309813007/diff/470001/src/ast.h#newcode2379
src/ast.h:2379: Expression* destructuring_assignment() { return
destructuring_assignment_; }
On 2015/11/25 21:05:28, adamk wrote:
> Looks like this is a non-const dup of the above accessor, I don't think you
need
> it.

Acknowledged.

https://codereview.chromium.org/1309813007/diff/470001/src/ast.h#newcode2402
src/ast.h:2402: : public BitField16<bool, TokenField::kNext, 1> {};
On 2015/11/25 21:05:28, adamk wrote:
> Can you use kNext for the above lines while you're at it?

Acknowledged.

https://codereview.chromium.org/1309813007/diff/470001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1309813007/diff/470001/src/parser.h#newcode1095
src/parser.h:1095: PatternContext SetInitializerContextIfNeeded(Expression*
node) {
On 2015/11/25 21:05:28, adamk wrote:
> Any reason all of these functions need to be in the .h file? If it's for
> inlining, that's not needed; these are private methods and all their callers
are
> on this class, so they could just as well be inline and defined in the .cc
file.

No real reason, it was just convenient while writing them. I'll move them

https://codereview.chromium.org/1309813007/diff/470001/src/parser.h#newcode1496
src/parser.h:1496: void
ParserTraits::ShouldRewriteDestructuringAssignment(Expression* expr) {
On 2015/11/25 21:05:28, adamk wrote:
> And here...
> 
> Also, as mentioned before, this name is confusing. How about
> "QueueDestructuringAssignmentForRewriting" (also long, but clearer what it's
> doing).

Acknowledged.

https://codereview.chromium.org/1309813007/diff/470001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1309813007/diff/470001/src/pattern-rewriter.c...
src/pattern-rewriter.cc:276: set_context(context);
On 2015/11/25 21:05:28, adamk wrote:
> Can you use an RAII object for this instead of relying on set_context at the
end
> of the block to handle this?

Doable, but it is (relatively) a lot of code and I don't think it needs to be
used a whole lot. I'll put that in the next patch

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcod...
src/preparser.h:279: List<DestructuringAssignment>
destructuring_assignments_to_rewrite_;
On 2015/11/25 21:05:28, adamk wrote:
> Please move this to Parser unless there's some reason it's needed in the
> preparser.

The list lives in FunctionState, so each FunctionState has its own collection of
assignments to rewrite, and does once the function is ready to be made into an
AST node.

The List is always empty in the PreParser (eg, manipulating the list is
Parser-specific). It might be possible to declare it as like, a void type or
something in the PreParser, but I'm not positive that would realy work.
Thoughts?

I originally stored the list in the Parser and tried to do the rewriting once
ParseProgram had finished (if it had been called), but had issues with that.

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcod...
src/preparser.h:281: void RewriteDestructuringAssignments();
On 2015/11/25 21:05:28, adamk wrote:
> I don't see this defined anywhere (and I don't think it needs to be defined).

Acknowledged.

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcod...
src/preparser.h:2493: if (peek() != Token::ARROW) {
On 2015/11/25 21:05:28, adamk wrote:
> Sorry, I've forgotten if you've answered this already: why are doing this
> peeking here now (what does it have to do with parsing destructuring
assignment,
> that is)?

```

// Valid if: --harmony-destructuring-assignment
var o = ({ a: b } = someObj);

// Valid if: --harmony-default-parameters + --harmony-destructuring-bind, only
if followed by `=>`
var f = ({ a: b } = someObj) => b;
```

Can't eagerly break the initializer in ParseAssignmentExpression unless we know
if the ARROW is there or not, so it defers to the caller and does it here

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcod...
src/preparser.h:2632: bool seen_spread = false;
On 2015/11/25 21:05:28, adamk wrote:
> This bool is now unused.

Acknowledged.

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcod...
src/preparser.h:3108: USE(is_arrow_formals);
On 2015/11/25 21:05:28, adamk wrote:
> This USE is not needed.

Acknowledged.

[adamk, 2015-11-25 21:54:21]

https://codereview.chromium.org/1309813007/diff/470001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1309813007/diff/470001/src/pattern-rewriter.c...
src/pattern-rewriter.cc:276: set_context(context);
On 2015/11/25 21:41:51, caitp wrote:
> On 2015/11/25 21:05:28, adamk wrote:
> > Can you use an RAII object for this instead of relying on set_context at the
> end
> > of the block to handle this?
> 
> Doable, but it is (relatively) a lot of code and I don't think it needs to be
> used a whole lot. I'll put that in the next patch

I'm ok with this for now if you feel like there just aren't enough callers to be
worth it.

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcod...
src/preparser.h:2493: if (peek() != Token::ARROW) {
On 2015/11/25 21:41:52, caitp wrote:
> On 2015/11/25 21:05:28, adamk wrote:
> > Sorry, I've forgotten if you've answered this already: why are doing this
> > peeking here now (what does it have to do with parsing destructuring
> assignment,
> > that is)?
> 
> ```
> 
> // Valid if: --harmony-destructuring-assignment
> var o = ({ a: b } = someObj);
> 
> // Valid if: --harmony-default-parameters + --harmony-destructuring-bind, only
> if followed by `=>`
> var f = ({ a: b } = someObj) => b;
> ```
> 
> Can't eagerly break the initializer in ParseAssignmentExpression unless we
know
> if the ARROW is there or not, so it defers to the caller and does it here

Thanks!

[caitp (gmail), 2015-11-30 15:47:37]

So, I'm still not really happy with this approach, but it is what it is.

Maybe there's a better way to do this, like just introducing a new AST node like
"RewritableExpression" or something, and wrapping the original Assignment in
that if it will be rewritten (and VisitRewritableExpression will always just
directly visit whatever the expression is).

Doing it that way is still messy, but might avoid having to add weird code to
VisitAssignment all over the place

[adamk, 2015-11-30 19:20:28]

On 2015/11/30 15:47:37, caitp wrote:
> So, I'm still not really happy with this approach, but it is what it is.
> 
> Maybe there's a better way to do this, like just introducing a new AST node
like
> "RewritableExpression" or something, and wrapping the original Assignment in
> that if it will be rewritten (and VisitRewritableExpression will always just
> directly visit whatever the expression is).
> 
> Doing it that way is still messy, but might avoid having to add weird code to
> VisitAssignment all over the place

That approach sounds good to me, and is even closer to what Dan did for sloppy
block functions. It would allow you to get rid of the RETURN_IF_VISIT_NODE()
stuff, as well.

[adamk, 2015-11-30 19:22:05]

On 2015/11/30 19:20:28, adamk wrote:
> On 2015/11/30 15:47:37, caitp wrote:
> > So, I'm still not really happy with this approach, but it is what it is.
> > 
> > Maybe there's a better way to do this, like just introducing a new AST node
> like
> > "RewritableExpression" or something, and wrapping the original Assignment in
> > that if it will be rewritten (and VisitRewritableExpression will always just
> > directly visit whatever the expression is).
> > 
> > Doing it that way is still messy, but might avoid having to add weird code
to
> > VisitAssignment all over the place
> 
> That approach sounds good to me, and is even closer to what Dan did for sloppy
> block functions. It would allow you to get rid of the RETURN_IF_VISIT_NODE()
> stuff, as well.

I guess the only question is whether wrapping the original Assignment is
possible, structurally (the approaches used so far have been about mutating the
Assignment or its children, but haven't had to deal with its parent in the
tree).

[caitp (gmail), 2015-12-01 22:21:37]

I've added the RewritableExpression node, which I think makes this a little bit
clearer.

It still looks extremely fragile to me, but more understandable maybe?

[adamk, 2015-12-01 22:42:57]

I actually think this is looking pretty good. Why do you think this is fragile
(I mean, it at least doesn't seem any more fragile than the last two
iterations)?

My one big request is to de-generalize RewritableExpression (see comments in
ast.h and elsewhere).

https://codereview.chromium.org/1309813007/diff/590001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1309813007/diff/590001/src/ast/ast.h#newcode442
src/ast/ast.h:442: Expression* expr_;
Nit: just call this "expression_"

https://codereview.chromium.org/1309813007/diff/590001/src/ast/ast.h#newcode443
src/ast/ast.h:443: uint32_t bit_field_;
If you move this before expr_ and make it a uint8_t, it'll get packed in with
the 16 bit field in Expression (that seems to be the suggested pattern elsewhere
in ast.h)

Alternatively, this node is currently way more generic than necessary. You could
make it more specific to the current use case and simplify much of the logic
above, using a bool to track state instead of the bitfield.

I think I prefer the latter; otherwise there are lots of places in the code
where we check for a RewriteHint that's always true. Probably rename to
RewritableAssignmentExpression in that case.

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

https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#...
src/parsing/parser.cc:4547: RewritableExpression* rw =
expr->AsRewritableExpression();
Naming nit: I like "to_rewrite" better than "rw".

https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#...
src/parsing/parser.cc:4548: if (!rw ||
to_rewrite == nullptr

https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#...
src/parsing/parser.cc:4549:
!rw->IsRewritableAs(RewritableExpression::kDestructuringAssignment)) {
It's checks like this that I don't see as terribly useful, since they're always
true.

https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#...
src/parsing/parser.cc:6525: RewritableExpression* rw =
pair.assignment->AsRewritableExpression();
Like the old name better here too

https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#...
src/parsing/parser.cc:6539:
DCHECK(expr->AsRewritableExpression()->expression()->IsAssignment());
This, too, will go away if you just use the C++ type Assignment in
RewritableExpression.

https://codereview.chromium.org/1309813007/diff/590001/src/parsing/pattern-re...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/1309813007/diff/590001/src/parsing/pattern-re...
src/parsing/pattern-rewriter.cc:356: return Visit(node->expression());
I'd move this to the top and early return, letting you un-indent the rest of
this function.

[caitp (gmail), 2015-12-01 22:51:00]

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

https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#...
src/parsing/parser.cc:4549:
!rw->IsRewritableAs(RewritableExpression::kDestructuringAssignment)) {
On 2015/12/01 22:42:56, adamk wrote:
> It's checks like this that I don't see as terribly useful, since they're
always
> true.

the check is checking 2 things: 1, that it's "possibly" a destructuring
assignment (eg, valid syntax), and 2, that it hasn't already been rewritten as a
destructuring assignment.

While the first check could probably go away until more things need to use it
(which hopefully never happens, admittedly), the second one is pretty important
to make sure there's no endless looping (or hitting UNREACHABLEs and crashing)

[adamk, 2015-12-01 22:54:23]

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

https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#...
src/parsing/parser.cc:4549:
!rw->IsRewritableAs(RewritableExpression::kDestructuringAssignment)) {
On 2015/12/01 22:51:00, caitp wrote:
> On 2015/12/01 22:42:56, adamk wrote:
> > It's checks like this that I don't see as terribly useful, since they're
> always
> > true.
> 
> the check is checking 2 things: 1, that it's "possibly" a destructuring
> assignment (eg, valid syntax), and 2, that it hasn't already been rewritten as
a
> destructuring assignment.
> 
> While the first check could probably go away until more things need to use it
> (which hopefully never happens, admittedly), the second one is pretty
important
> to make sure there's no endless looping (or hitting UNREACHABLEs and crashing)

Right, sorry, there is that second case. Even so, I think it's more
understandable if the only reason something would fail this check was if it's
already been rewritten.

[caitp (gmail), 2015-12-02 00:20:51]

Alright, re-fixed

[adamk, 2015-12-02 00:44:57]

On 2015/12/02 00:20:51, caitp wrote:
> Alright, re-fixed

Fixed is good, ping for re-review when you've de-generalized
RewritableExpression (or when you make an argument for keeping it as-is).

[caitp (gmail), 2015-12-02 01:05:45]

On 2015/12/02 00:44:57, adamk wrote:
> On 2015/12/02 00:20:51, caitp wrote:
> > Alright, re-fixed
> 
> Fixed is good, ping for re-review when you've de-generalized
> RewritableExpression (or when you make an argument for keeping it as-is).

I think general is better, because there definitely are other cases for using
it. But right now it's essentially single use and is treated as such anyways,
what sort of degeneralizing did you have in mind?

[adamk, 2015-12-02 01:13:13]

I've noted what I mean by de-generalizing.

I'm all for reusable code, but I tend to lean towards doing that once there are
multiple cases. This ensures that the general code actually serves multiple
use-cases well. As-is, this code looks general but in fact only handles a single
case.

Maybe I'm missing something: is there another thing this would be used for in,
say, the next week or two?

https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h#newcode416
src/ast/ast.h:416: int hints() const { return
RewriteHintsField::decode(bit_field_); }
This returns an int, but will only ever be kDestructuringAssignment

https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h#newcode418
src/ast/ast.h:418: bool IsRewritableAs(RewriteHint hint) const {
There is only a single argument that could be passed here

https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h#newcode423
src/ast/ast.h:423: bool IsRewrittenAs(RewriteHint reason) const {
Ditto

https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h#newcode427
src/ast/ast.h:427: void RewriteAs(Expression* new_expression, RewriteHint
reason) {
And again, the second argument here is can only ever be one thing

https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h#newcode447
src/ast/ast.h:447: class RewriteHintsField : public BitField<uint16_t, 0, 4> {};
This bit field wouldn't be necessary, you could have one bool thats
'is_rewritten_'

[caitp (gmail), 2015-12-02 01:40:31]

On 2015/12/02 01:13:13, adamk wrote:
> I've noted what I mean by de-generalizing.
> 
> I'm all for reusable code, but I tend to lean towards doing that once there
are
> multiple cases. This ensures that the general code actually serves multiple
> use-cases well. As-is, this code looks general but in fact only handles a
single
> case.
> 
> Maybe I'm missing something: is there another thing this would be used for in,
> say, the next week or two?
> 
> https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h
> File src/ast/ast.h (right):
> 
>
https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h#newcode416
> src/ast/ast.h:416: int hints() const { return
> RewriteHintsField::decode(bit_field_); }
> This returns an int, but will only ever be kDestructuringAssignment
> 
>
https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h#newcode418
> src/ast/ast.h:418: bool IsRewritableAs(RewriteHint hint) const {
> There is only a single argument that could be passed here
> 
>
https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h#newcode423
> src/ast/ast.h:423: bool IsRewrittenAs(RewriteHint reason) const {
> Ditto
> 
>
https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h#newcode427
> src/ast/ast.h:427: void RewriteAs(Expression* new_expression, RewriteHint
> reason) {
> And again, the second argument here is can only ever be one thing
> 
>
https://codereview.chromium.org/1309813007/diff/690001/src/ast/ast.h#newcode447
> src/ast/ast.h:447: class RewriteHintsField : public BitField<uint16_t, 0, 4>
{};
> This bit field wouldn't be necessary, you could have one bool thats
> 'is_rewritten_'

alright, done

[adamk, 2015-12-02 01:51:35]

adamk@chromium.org changed reviewers:
+ bmeurer@chromium.org
- yangguo@chromium.org

[adamk, 2015-12-02 01:51:36]

lgtm once nits are taken care of and the CL description is trimmed
down/rewritten to match reality (for one, this is much more than a parsing patch
now).

Will also need some OWNERS in Munich, adding bmeurer who's in all the relevant
files.

A few nits below.

https://codereview.chromium.org/1309813007/diff/710001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1309813007/diff/710001/src/ast/ast.h#newcode406
src/ast/ast.h:406: class RewritableExpression : public Expression {
This got lost in the back-and-forth, but now that this is less-generic, I think
this should be called "RewritableAssignmentExpression"

https://codereview.chromium.org/1309813007/diff/710001/src/ast/ast.h#newcode3585
src/ast/ast.h:3585: RewritableExpression* NewRewritableExpression(Expression*
expression) {
And this should take an Assignment*

https://codereview.chromium.org/1309813007/diff/710001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1309813007/diff/710001/src/flag-definitions.h...
src/flag-definitions.h:211: V(harmony_destructuring_assignment, "harmony
destructuring assignment")
Does this need to imply harmony_destructuring_bind?

[caitp (gmail), 2015-12-02 02:25:26]

https://codereview.chromium.org/1309813007/diff/710001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1309813007/diff/710001/src/ast/ast.h#newcode3585
src/ast/ast.h:3585: RewritableExpression* NewRewritableExpression(Expression*
expression) {
On 2015/12/02 01:51:36, adamk wrote:
> And this should take an Assignment*

complicates things a bit too much --- causes issues with the return value of
ParseAssignmentExpression in the full parser, and some other unfortunate things.
Type-safety would be nice, but I think it's stuck with just a DCHECK for now (it
gets cast to an Expression because the rewritten expression is most likely not
an Assignment)

https://codereview.chromium.org/1309813007/diff/710001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1309813007/diff/710001/src/flag-definitions.h...
src/flag-definitions.h:211: V(harmony_destructuring_assignment, "harmony
destructuring assignment")
On 2015/12/02 01:51:36, adamk wrote:
> Does this need to imply harmony_destructuring_bind?

it can function without it, so I don't think it really needs to. since
destructuring bind is shipping, it might not matter much?

I don't have a strong feeling one way or the other

[caitp (gmail), 2015-12-02 02:29:55]

Description was changed from

==========
[es6] parse destructuring assignment

Attempt #3

This will cause (pretty bad maybe) code load perf regressions. This may be
mitigated a bit with some cleanup and refactoring (not all of the changes
turned out to be necessary).

It is also quite likely that the test coverage does not cover every possible
combination of inputs, and may be missing some corner cases.

Some slightly unrelated changes include:

- Make Array element following rest element, or assignment to a rest element a
syntax error
- Only report `...` as an unexpected token when parsing ArrayLiteral if both
`harmony_spread_arrays` and `harmony-destructuring` are disabled.

TODO:
- Delete unused changes / cleanup
- New flag for destructuring assignment, which implies destructuring
- Find cheaper ways of performing the various checks specific to reporting
pattern
errors.

BUG=v8:811
LOG=N
R=adamk, wingo, aperez, conradw, dehrenberg
==========

to

==========
[es6] implement destructuring assignment

Attempt #<really big number>

This will cause (pretty bad maybe) code load perf regressions. This may be
mitigated a bit with some cleanup and refactoring (not all of the changes
turned out to be necessary).

It is also quite likely that the test coverage does not cover every possible
combination of inputs, and may be missing some corner cases.

Some slightly unrelated changes include:

- Make Array element following rest element, or assignment to a rest element a
syntax error
- Only report `...` as an unexpected token when parsing ArrayLiteral if both
`harmony_spread_arrays` and `harmony-destructuring` are disabled.

TODO:
- Delete unused changes / cleanup
- New flag for destructuring assignment, which implies destructuring
- Find cheaper ways of performing the various checks specific to reporting
pattern
errors.

BUG=v8:811
LOG=N
R=adamk, wingo, aperez, conradw, dehrenberg
==========

[caitp (gmail), 2015-12-02 02:33:55]

Description was changed from

==========
[es6] implement destructuring assignment

Attempt #<really big number>

This will cause (pretty bad maybe) code load perf regressions. This may be
mitigated a bit with some cleanup and refactoring (not all of the changes
turned out to be necessary).

It is also quite likely that the test coverage does not cover every possible
combination of inputs, and may be missing some corner cases.

Some slightly unrelated changes include:

- Make Array element following rest element, or assignment to a rest element a
syntax error
- Only report `...` as an unexpected token when parsing ArrayLiteral if both
`harmony_spread_arrays` and `harmony-destructuring` are disabled.

TODO:
- Delete unused changes / cleanup
- New flag for destructuring assignment, which implies destructuring
- Find cheaper ways of performing the various checks specific to reporting
pattern
errors.

BUG=v8:811
LOG=N
R=adamk, wingo, aperez, conradw, dehrenberg
==========

to

==========
[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting
strategy involves inserting a placeholder RewritableAssignmentExpression into
the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes
which form part of a binding pattern, thus breaking the meaning of the pattern
--- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=N
R=adamk, wingo, aperez, conradw, dehrenberg
==========

[caitp (gmail), 2015-12-02 17:21:49]

Description was changed from

==========
[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting
strategy involves inserting a placeholder RewritableAssignmentExpression into
the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes
which form part of a binding pattern, thus breaking the meaning of the pattern
--- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=N
R=adamk, wingo, aperez, conradw, dehrenberg
==========

to

==========
[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting
strategy involves inserting a placeholder RewritableAssignmentExpression into
the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes
which form part of a binding pattern, thus breaking the meaning of the pattern
--- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=N
R=adamk, wingo, aperez, dehrenberg
==========

[adamk, 2015-12-02 17:24:59]

Description was changed from

==========
[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting
strategy involves inserting a placeholder RewritableAssignmentExpression into
the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes
which form part of a binding pattern, thus breaking the meaning of the pattern
--- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=N
R=adamk, wingo, aperez, dehrenberg
==========

to

==========
[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting
strategy involves inserting a placeholder RewritableAssignmentExpression into
the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes
which form part of a binding pattern, thus breaking the meaning of the pattern
--- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=Y
R=adamk, wingo, aperez, dehrenberg
==========

[Benedikt Meurer, 2015-12-03 18:55:24]

LGTM on compiler, crankshaft, full-codegen and interpreter.

[caitp (gmail), 2015-12-03 18:57:16]

caitpotter88@gmail.com changed reviewers:
+ rossberg@chromium.org

[caitp (gmail), 2015-12-03 18:59:49]

On 2015/12/03 18:55:24, Benedikt Meurer wrote:
> LGTM on compiler, crankshaft, full-codegen and interpreter.

Thanks --- this makes a bit of a mess of the codebase, and I'm not sure there
are too many ways around that. It would be nice to get one last round before
checkin. +rossberg and maybe dan?

[rossberg, 2015-12-04 14:24:21]

The main thing I'm confused about is why we need the rewriting queue. What
prevents you from invoking the rewriting right at the time where you currently
just queue it up?

[caitp (gmail), 2015-12-04 14:26:09]

On 2015/12/04 14:24:21, rossberg wrote:
> The main thing I'm confused about is why we need the rewriting queue. What
> prevents you from invoking the rewriting right at the time where you currently
> just queue it up?

The main thing is the ambiguity `({ x } = { x: 1 }) => x` << where the meaning
changes depending on whether `=>` is found or not. Binding patterns are
rewritten differently, so by necessity the rewriting is deferred until it's
known for sure what is being rewritten

[rossberg, 2015-12-04 14:32:33]

On 2015/12/04 14:26:09, caitp wrote:
> On 2015/12/04 14:24:21, rossberg wrote:
> > The main thing I'm confused about is why we need the rewriting queue. What
> > prevents you from invoking the rewriting right at the time where you
currently
> > just queue it up?
> 
> The main thing is the ambiguity `({ x } = { x: 1 }) => x` << where the meaning
> changes depending on whether `=>` is found or not. Binding patterns are
> rewritten differently, so by necessity the rewriting is deferred until it's
> known for sure what is being rewritten

But is anything ever removed from this queue without being rewritten? I can't
find that happening anywhere, so isn't it always performed unconditionally
anyway, just delayed?

[caitp (gmail), 2015-12-04 14:34:43]

On 2015/12/04 14:32:33, rossberg wrote:
> On 2015/12/04 14:26:09, caitp wrote:
> > On 2015/12/04 14:24:21, rossberg wrote:
> > > The main thing I'm confused about is why we need the rewriting queue. What
> > > prevents you from invoking the rewriting right at the time where you
> currently
> > > just queue it up?
> > 
> > The main thing is the ambiguity `({ x } = { x: 1 }) => x` << where the
meaning
> > changes depending on whether `=>` is found or not. Binding patterns are
> > rewritten differently, so by necessity the rewriting is deferred until it's
> > known for sure what is being rewritten
> 
> But is anything ever removed from this queue without being rewritten? I can't
> find that happening anywhere, so isn't it always performed unconditionally
> anyway, just delayed?

Hmm, that's a good point, right now there might be some redundant rewriting
happening. I'm not sure it should be removed from the queue because it can't be
easily, but it can be marked as rewritten to prevent redundant rewriting

[caitp (gmail), 2015-12-04 14:35:32]

On 2015/12/04 14:34:43, caitp wrote:
> On 2015/12/04 14:32:33, rossberg wrote:
> > On 2015/12/04 14:26:09, caitp wrote:
> > > On 2015/12/04 14:24:21, rossberg wrote:
> > > > The main thing I'm confused about is why we need the rewriting queue.
What
> > > > prevents you from invoking the rewriting right at the time where you
> > currently
> > > > just queue it up?
> > > 
> > > The main thing is the ambiguity `({ x } = { x: 1 }) => x` << where the
> meaning
> > > changes depending on whether `=>` is found or not. Binding patterns are
> > > rewritten differently, so by necessity the rewriting is deferred until
it's
> > > known for sure what is being rewritten
> > 
> > But is anything ever removed from this queue without being rewritten? I
can't
> > find that happening anywhere, so isn't it always performed unconditionally
> > anyway, just delayed?
> 
> Hmm, that's a good point, right now there might be some redundant rewriting
> happening. I'm not sure it should be removed from the queue because it can't
be
> easily, but it can be marked as rewritten to prevent redundant rewriting

--- but, just to be clear, that redundant rewriting doesn't mean that it's able
to eagerly rewrite, it just means that the redundant rewriting is sort of
discarded, because the RewritableAssignmentExpression itself is discarded

[rossberg, 2015-12-04 15:16:33]

On 2015/12/04 14:35:32, caitp wrote:
> On 2015/12/04 14:34:43, caitp wrote:
> > On 2015/12/04 14:32:33, rossberg wrote:
> > > On 2015/12/04 14:26:09, caitp wrote:
> > > > On 2015/12/04 14:24:21, rossberg wrote:
> > > > > The main thing I'm confused about is why we need the rewriting queue.
> What
> > > > > prevents you from invoking the rewriting right at the time where you
> > > currently
> > > > > just queue it up?
> > > > 
> > > > The main thing is the ambiguity `({ x } = { x: 1 }) => x` << where the
> > meaning
> > > > changes depending on whether `=>` is found or not. Binding patterns are
> > > > rewritten differently, so by necessity the rewriting is deferred until
> it's
> > > > known for sure what is being rewritten
> > > 
> > > But is anything ever removed from this queue without being rewritten? I
> can't
> > > find that happening anywhere, so isn't it always performed unconditionally
> > > anyway, just delayed?
> > 
> > Hmm, that's a good point, right now there might be some redundant rewriting
> > happening. I'm not sure it should be removed from the queue because it can't
> be
> > easily, but it can be marked as rewritten to prevent redundant rewriting
> 
> --- but, just to be clear, that redundant rewriting doesn't mean that it's
able
> to eagerly rewrite, it just means that the redundant rewriting is sort of
> discarded, because the RewritableAssignmentExpression itself is discarded

I see. The deeper reason I'm asking is because Niko is now implementing the
rewriting for array literals with spread, which has a similar problem. The way I
thought this would work is that we introduce something dual to the pattern
rewriter, i.e., an expression visitor that is invoked when an ambiguous
cover-expression/pattern is resolved to be an expression. That would then walk
and rewrite the necessary expressions inside. (In most cases that would be a
no-op, so it's probably worth introducing a bit in the expression classifier to
indicate the necessity for rewriting.)

You already have that kind of visitor, it seems, so I wonder why it isn't
possible to invoke it directly at the resolution points.

[caitp (gmail), 2015-12-04 15:31:49]

On 2015/12/04 15:16:33, rossberg wrote:
> On 2015/12/04 14:35:32, caitp wrote:
> > On 2015/12/04 14:34:43, caitp wrote:
> > > On 2015/12/04 14:32:33, rossberg wrote:
> > > > On 2015/12/04 14:26:09, caitp wrote:
> > > > > On 2015/12/04 14:24:21, rossberg wrote:
> > > > > > The main thing I'm confused about is why we need the rewriting
queue.
> > What
> > > > > > prevents you from invoking the rewriting right at the time where you
> > > > currently
> > > > > > just queue it up?
> > > > > 
> > > > > The main thing is the ambiguity `({ x } = { x: 1 }) => x` << where the
> > > meaning
> > > > > changes depending on whether `=>` is found or not. Binding patterns
are
> > > > > rewritten differently, so by necessity the rewriting is deferred until
> > it's
> > > > > known for sure what is being rewritten
> > > > 
> > > > But is anything ever removed from this queue without being rewritten? I
> > can't
> > > > find that happening anywhere, so isn't it always performed
unconditionally
> > > > anyway, just delayed?
> > > 
> > > Hmm, that's a good point, right now there might be some redundant
rewriting
> > > happening. I'm not sure it should be removed from the queue because it
can't
> > be
> > > easily, but it can be marked as rewritten to prevent redundant rewriting
> > 
> > --- but, just to be clear, that redundant rewriting doesn't mean that it's
> able
> > to eagerly rewrite, it just means that the redundant rewriting is sort of
> > discarded, because the RewritableAssignmentExpression itself is discarded
> 
> I see. The deeper reason I'm asking is because Niko is now implementing the
> rewriting for array literals with spread, which has a similar problem. The way
I
> thought this would work is that we introduce something dual to the pattern
> rewriter, i.e., an expression visitor that is invoked when an ambiguous
> cover-expression/pattern is resolved to be an expression. That would then walk
> and rewrite the necessary expressions inside. (In most cases that would be a
> no-op, so it's probably worth introducing a bit in the expression classifier
to
> indicate the necessity for rewriting.)
> 
> You already have that kind of visitor, it seems, so I wonder why it isn't
> possible to invoke it directly at the resolution points.

It may be possible, I've been looking at this CL for too long now, almost 4
months. Should it wait to see if I can find a better way to deal with the cover
grammar?

[rossberg, 2015-12-04 16:37:08]

On 2015/12/04 15:31:49, caitp wrote:
> On 2015/12/04 15:16:33, rossberg wrote:
> > On 2015/12/04 14:35:32, caitp wrote:
> > > On 2015/12/04 14:34:43, caitp wrote:
> > > > On 2015/12/04 14:32:33, rossberg wrote:
> > > > > On 2015/12/04 14:26:09, caitp wrote:
> > > > > > On 2015/12/04 14:24:21, rossberg wrote:
> > > > > > > The main thing I'm confused about is why we need the rewriting
> queue.
> > > What
> > > > > > > prevents you from invoking the rewriting right at the time where
you
> > > > > currently
> > > > > > > just queue it up?
> > > > > > 
> > > > > > The main thing is the ambiguity `({ x } = { x: 1 }) => x` << where
the
> > > > meaning
> > > > > > changes depending on whether `=>` is found or not. Binding patterns
> are
> > > > > > rewritten differently, so by necessity the rewriting is deferred
until
> > > it's
> > > > > > known for sure what is being rewritten
> > > > > 
> > > > > But is anything ever removed from this queue without being rewritten?
I
> > > can't
> > > > > find that happening anywhere, so isn't it always performed
> unconditionally
> > > > > anyway, just delayed?
> > > > 
> > > > Hmm, that's a good point, right now there might be some redundant
> rewriting
> > > > happening. I'm not sure it should be removed from the queue because it
> can't
> > > be
> > > > easily, but it can be marked as rewritten to prevent redundant rewriting
> > > 
> > > --- but, just to be clear, that redundant rewriting doesn't mean that it's
> > able
> > > to eagerly rewrite, it just means that the redundant rewriting is sort of
> > > discarded, because the RewritableAssignmentExpression itself is discarded
> > 
> > I see. The deeper reason I'm asking is because Niko is now implementing the
> > rewriting for array literals with spread, which has a similar problem. The
way
> I
> > thought this would work is that we introduce something dual to the pattern
> > rewriter, i.e., an expression visitor that is invoked when an ambiguous
> > cover-expression/pattern is resolved to be an expression. That would then
walk
> > and rewrite the necessary expressions inside. (In most cases that would be a
> > no-op, so it's probably worth introducing a bit in the expression classifier
> to
> > indicate the necessity for rewriting.)
> > 
> > You already have that kind of visitor, it seems, so I wonder why it isn't
> > possible to invoke it directly at the resolution points.
> 
> It may be possible, I've been looking at this CL for too long now, almost 4
> months. Should it wait to see if I can find a better way to deal with the
cover
> grammar?

I don't want to block this. So perhaps you land it and then look if it can be
simplified in a follow up?

[caitp (gmail), 2015-12-04 16:38:09]

On 2015/12/04 16:37:08, rossberg wrote:
> On 2015/12/04 15:31:49, caitp wrote:
> > On 2015/12/04 15:16:33, rossberg wrote:
> > > On 2015/12/04 14:35:32, caitp wrote:
> > > > On 2015/12/04 14:34:43, caitp wrote:
> > > > > On 2015/12/04 14:32:33, rossberg wrote:
> > > > > > On 2015/12/04 14:26:09, caitp wrote:
> > > > > > > On 2015/12/04 14:24:21, rossberg wrote:
> > > > > > > > The main thing I'm confused about is why we need the rewriting
> > queue.
> > > > What
> > > > > > > > prevents you from invoking the rewriting right at the time where
> you
> > > > > > currently
> > > > > > > > just queue it up?
> > > > > > > 
> > > > > > > The main thing is the ambiguity `({ x } = { x: 1 }) => x` << where
> the
> > > > > meaning
> > > > > > > changes depending on whether `=>` is found or not. Binding
patterns
> > are
> > > > > > > rewritten differently, so by necessity the rewriting is deferred
> until
> > > > it's
> > > > > > > known for sure what is being rewritten
> > > > > > 
> > > > > > But is anything ever removed from this queue without being
rewritten?
> I
> > > > can't
> > > > > > find that happening anywhere, so isn't it always performed
> > unconditionally
> > > > > > anyway, just delayed?
> > > > > 
> > > > > Hmm, that's a good point, right now there might be some redundant
> > rewriting
> > > > > happening. I'm not sure it should be removed from the queue because it
> > can't
> > > > be
> > > > > easily, but it can be marked as rewritten to prevent redundant
rewriting
> > > > 
> > > > --- but, just to be clear, that redundant rewriting doesn't mean that
it's
> > > able
> > > > to eagerly rewrite, it just means that the redundant rewriting is sort
of
> > > > discarded, because the RewritableAssignmentExpression itself is
discarded
> > > 
> > > I see. The deeper reason I'm asking is because Niko is now implementing
the
> > > rewriting for array literals with spread, which has a similar problem. The
> way
> > I
> > > thought this would work is that we introduce something dual to the pattern
> > > rewriter, i.e., an expression visitor that is invoked when an ambiguous
> > > cover-expression/pattern is resolved to be an expression. That would then
> walk
> > > and rewrite the necessary expressions inside. (In most cases that would be
a
> > > no-op, so it's probably worth introducing a bit in the expression
classifier
> > to
> > > indicate the necessity for rewriting.)
> > > 
> > > You already have that kind of visitor, it seems, so I wonder why it isn't
> > > possible to invoke it directly at the resolution points.
> > 
> > It may be possible, I've been looking at this CL for too long now, almost 4
> > months. Should it wait to see if I can find a better way to deal with the
> cover
> > grammar?
> 
> I don't want to block this. So perhaps you land it and then look if it can be
> simplified in a follow up?

Sounds good to me. flipping the switch

[caitp (gmail), 2015-12-04 16:39:15]

Description was changed from

==========
[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting
strategy involves inserting a placeholder RewritableAssignmentExpression into
the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes
which form part of a binding pattern, thus breaking the meaning of the pattern
--- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=Y
R=adamk, wingo, aperez, dehrenberg
==========

to

==========
[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting
strategy involves inserting a placeholder RewritableAssignmentExpression into
the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes
which form part of a binding pattern, thus breaking the meaning of the pattern
--- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=Y
R=adamk@chromium.org, bmeurer@chromium.org, rossberg@chromium.org
==========

[caitp (gmail), 2015-12-04 16:39:27]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-12-04 16:39:28]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1309813007/#ps770001
(title: "Remove redundant rewriting")

[commit-bot: I haz the power, 2015-12-04 16:39:31]

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

[caitp (gmail), 2015-12-04 16:39:34]

The CQ bit was unchecked by caitpotter88@gmail.com

[caitp (gmail), 2015-12-04 16:40:20]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-12-04 16:40:31]

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

[commit-bot: I haz the power, 2015-12-04 17:20:15]

Description was changed from

==========
[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting
strategy involves inserting a placeholder RewritableAssignmentExpression into
the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes
which form part of a binding pattern, thus breaking the meaning of the pattern
--- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=Y
R=adamk@chromium.org, bmeurer@chromium.org, rossberg@chromium.org
==========

to

==========
[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting
strategy involves inserting a placeholder RewritableAssignmentExpression into
the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes
which form part of a binding pattern, thus breaking the meaning of the pattern
--- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=Y
R=adamk@chromium.org, bmeurer@chromium.org, rossberg@chromium.org
==========

[commit-bot: I haz the power, 2015-12-04 17:20:16]

Committed patchset #19 (id:770001)

[commit-bot: I haz the power, 2015-12-04 17:20:31]

Description was changed from

==========
[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting
strategy involves inserting a placeholder RewritableAssignmentExpression into
the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes
which form part of a binding pattern, thus breaking the meaning of the pattern
--- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=Y
R=adamk@chromium.org, bmeurer@chromium.org, rossberg@chromium.org
==========

to

==========
[es6] implement destructuring assignment

Attempt #<really big number>

Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting
strategy involves inserting a placeholder RewritableAssignmentExpression into
the AST, whose content expression can be completely rewritten at a later time.

Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes
which form part of a binding pattern, thus breaking the meaning of the pattern
--- or by eagerly rewriting ambiguous constructs that are not immediately known

BUG=v8:811
LOG=Y
R=adamk@chromium.org, bmeurer@chromium.org, rossberg@chromium.org

Committed: https://crrev.com/b634a61d84b8793ee2939690685d4b9f7fb32b95
Cr-Commit-Position: refs/heads/master@{#32623}
==========

[commit-bot: I haz the power, 2015-12-04 17:20:32]

Patchset 19 (id:??) landed as
https://crrev.com/b634a61d84b8793ee2939690685d4b9f7fb32b95
Cr-Commit-Position: refs/heads/master@{#32623}

[rossberg, 2015-12-09 13:52:08]

FWIW, Niko is looking into simplifying the rewriting mechanism as part of his
patch now.

[adamk, 2015-12-09 19:25:51]

On 2015/12/09 13:52:08, rossberg wrote:
> FWIW, Niko is looking into simplifying the rewriting mechanism as part of his
> patch now.

Sounds good. Can you add me to the patch (when it gets sent out, that is)?