[es6] parse destructuring assignment

[es6] parse destructuring assignment

BUG=v8:811
LOG=N
R=dslomov@chromium.org, arv@chromium.org

[caitp (gmail), 2015-06-04 18:35:49]

PTAL, I hacked around on this for a bit and now, make check is passing along
with a bunch of new parsing tests.

It's quite likely that the eager rewriting of AssignmentExpressions could break
binding pattern runtime stuff though, so need to be careful with that.

[arv (Not doing code reviews), 2015-06-04 20:08:04]

Looks solid to me. A few nits and comments.

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode626
src/preparser.h:626: inline void AppendAssignmentTarget(AssignmentTargetType
value) {
s/value/type/

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode...
src/preparser.h:2615: ExpressionClassifier binding_classifier;
rename this?

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode...
src/preparser.h:3072: ParserBase<Traits>::ParseAssignmentExpression(bool
accept_IN, bool is_rhs,
TODO: Maybe add enums for this and accept_IN?

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode...
src/preparser.h:3087: bool maybeAssignmentPattern =
maybe_assignment_pattern

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode...
src/preparser.h:3088: allow_harmony_destructuring() && !is_rhs &&
maybe rename is_rhs to is_lhs?

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode...
src/preparser.h:3144: bool validDestructuringAssignmentTarget =
s/validDestructuringAssignmentTarget/valid_destructuring_assignment_target/

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

https://codereview.chromium.org/1168643005/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6545: {"var x, y, z; (", " = {});"},
Do you want to do for-in and for-of too?

https://codereview.chromium.org/1168643005/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6583: NULL};
Needs more LHS expressions.

this.x
super.x
o['x']
f().prop
(lhsExpr)

Can be done in a follow up if you want.

https://codereview.chromium.org/1168643005/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6616: "{ x: new.target }",
I think this is valid syntax inside a function but it would throw at runtime
since it is a const binding.

[caitp (gmail), 2015-06-04 20:54:15]

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

https://codereview.chromium.org/1168643005/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6616: "{ x: new.target }",
On 2015/06/04 20:08:04, arv wrote:
> I think this is valid syntax inside a function but it would throw at runtime
> since it is a const binding.

IsValidSimpleAssignmentTarget returns false for `new.target`, making it an early
error when used as a DestructuringAssignmentTarget, iirc --- However, I think
`new.target.foo` would be acceptable, if that's legal to parse

[arv (Not doing code reviews), 2015-06-04 20:55:09]

On 2015/06/04 20:54:15, caitp wrote:
>
https://codereview.chromium.org/1168643005/diff/20001/test/cctest/test-parsin...
> File test/cctest/test-parsing.cc (right):
> 
>
https://codereview.chromium.org/1168643005/diff/20001/test/cctest/test-parsin...
> test/cctest/test-parsing.cc:6616: "{ x: new.target }",
> On 2015/06/04 20:08:04, arv wrote:
> > I think this is valid syntax inside a function but it would throw at runtime
> > since it is a const binding.
> 
> IsValidSimpleAssignmentTarget returns false for `new.target`, making it an
early
> error when used as a DestructuringAssignmentTarget, iirc --- However, I think
> `new.target.foo` would be acceptable, if that's legal to parse

OK. Early error > late error.

[caitp (gmail), 2015-06-05 18:36:10]

Patchset #6 (id:100001) has been deleted

[caitp (gmail), 2015-06-05 18:37:14]

I'm a bit worried about this affecting parse performance, due to the increased
shuffling around of classifier data. But maybe it's not that bad.

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode626
src/preparser.h:626: inline void AppendAssignmentTarget(AssignmentTargetType
value) {
On 2015/06/04 20:08:03, arv wrote:
> s/value/type/

Done.

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode...
src/preparser.h:2615: ExpressionClassifier binding_classifier;
On 2015/06/04 20:08:03, arv wrote:
> rename this?

Done.

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode...
src/preparser.h:3072: ParserBase<Traits>::ParseAssignmentExpression(bool
accept_IN, bool is_rhs,
On 2015/06/04 20:08:03, arv wrote:
> TODO: Maybe add enums for this and accept_IN?

Done.

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode...
src/preparser.h:3087: bool maybeAssignmentPattern =
On 2015/06/04 20:08:03, arv wrote:
> maybe_assignment_pattern 

Done.

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode...
src/preparser.h:3088: allow_harmony_destructuring() && !is_rhs &&
On 2015/06/04 20:08:04, arv wrote:
> maybe rename is_rhs to is_lhs?

renamed to `pattern_lhs` in a previous patch

https://codereview.chromium.org/1168643005/diff/20001/src/preparser.h#newcode...
src/preparser.h:3144: bool validDestructuringAssignmentTarget =
On 2015/06/04 20:08:04, arv wrote:
> s/validDestructuringAssignmentTarget/valid_destructuring_assignment_target/

Done.

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

https://codereview.chromium.org/1168643005/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6545: {"var x, y, z; (", " = {});"},
On 2015/06/04 20:08:04, arv wrote:
> Do you want to do for-in and for-of too?

Done.

https://codereview.chromium.org/1168643005/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6583: NULL};
On 2015/06/04 20:08:04, arv wrote:
> Needs more LHS expressions.
> 
> this.x
> super.x
> o['x']
> f().prop
> (lhsExpr)
> 
> Can be done in a follow up if you want.

I've added a bunch of these

[arv (Not doing code reviews), 2015-06-05 18:45:26]

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

https://codereview.chromium.org/1168643005/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:6556: {"var x, y, z; for (x in ", " = {});"},
This was not what I had in mind. Here is an example of what I had in mind

for ({length} in {x: 1, y: 2}) {}

and similarly for for-of

[caitp (gmail), 2015-06-05 19:00:20]

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

https://codereview.chromium.org/1168643005/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:6556: {"var x, y, z; for (x in ", " = {});"},
On 2015/06/05 18:45:26, arv wrote:
> This was not what I had in mind. Here is an example of what I had in mind
> 
> for ({length} in {x: 1, y: 2}) {}
> 
> and similarly for for-of

They're both valid, but we may be reporting an error if the LHS of for-loops
isn't a valid identifier, I think tests for the LHS bindings need to happen in a
followup.

For the RHS, since those are assignment expressions, if destructuring assignment
happens there it should  be allowed

[caitp (gmail), 2015-06-06 12:45:48]

Patchset #9 (id:180001) has been deleted

[caitp (gmail), 2015-06-06 12:56:53]

Patchset #9 (id:200001) has been deleted

[caitp (gmail), 2015-06-06 16:23:05]

Patchset #11 (id:260001) has been deleted

[caitp (gmail), 2015-06-09 15:35:49]

So, do we have any comments about this patch? I'd like to do something with this
one at some point

[Dmitry Lomov (no reviews), 2015-06-09 16:26:36]

On 2015/06/09 15:35:49, caitp wrote:
> So, do we have any comments about this patch? I'd like to do something with
this
> one at some point

Sorry for being slow on this, I am looking at it - will need a bit of time
though.

[Dmitry Lomov (no reviews), 2015-06-10 08:08:33]

On 2015/06/09 16:26:36, Dmitry S. Lomov wrote:
> On 2015/06/09 15:35:49, caitp wrote:
> > So, do we have any comments about this patch? I'd like to do something with
> this
> > one at some point
> 
> Sorry for being slow on this, I am looking at it - will need a bit of time
> though.

Sorry for sabotaging this by moving ExpressionClassfier into a separate header -
since it is me who is too slow on reviews here I'll rebase this patch and send
it to Caitlin.

[Dmitry Lomov (no reviews), 2015-06-10 09:34:47]

On 2015/06/10 08:08:33, Dmitry S. Lomov wrote:
> On 2015/06/09 16:26:36, Dmitry S. Lomov wrote:
> > On 2015/06/09 15:35:49, caitp wrote:
> > > So, do we have any comments about this patch? I'd like to do something
with
> > this
> > > one at some point
> > 
> > Sorry for being slow on this, I am looking at it - will need a bit of time
> > though.
> 
> Sorry for sabotaging this by moving ExpressionClassfier into a separate header
-
> since it is me who is too slow on reviews here I'll rebase this patch and send
> it to Caitlin.

Done in https://codereview.chromium.org/1173003002/

[caitp (gmail), 2015-06-10 11:41:14]

On 2015/06/10 09:34:47, Dmitry S. Lomov wrote:
> On 2015/06/10 08:08:33, Dmitry S. Lomov wrote:
> > On 2015/06/09 16:26:36, Dmitry S. Lomov wrote:
> > > On 2015/06/09 15:35:49, caitp wrote:
> > > > So, do we have any comments about this patch? I'd like to do something
> with
> > > this
> > > > one at some point
> > > 
> > > Sorry for being slow on this, I am looking at it - will need a bit of time
> > > though.
> > 
> > Sorry for sabotaging this by moving ExpressionClassfier into a separate
header
> -
> > since it is me who is too slow on reviews here I'll rebase this patch and
send
> > it to Caitlin.
> 
> Done in https://codereview.chromium.org/1173003002/

Thanks, that is a time saver =)

[noordhuis, 2015-06-15 22:01:08]

info@bnoordhuis.nl changed reviewers:
+ info@bnoordhuis.nl

[noordhuis, 2015-06-15 22:01:09]

Sorry Caitlin, nothing insightful here, just some nits.  The general approach
looks alright to me.

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

https://codereview.chromium.org/1168643005/diff/300001/src/expression-classif...
src/expression-classifier.h:91: TARGET_RESTRICTED,
TARGET_RESTRICTED doesn't seem to be used anywhere.

https://codereview.chromium.org/1168643005/diff/300001/src/expression-classif...
src/expression-classifier.h:126: return lhs_type_ >= TARGET_IDENTIFIER &&
lhs_type_ <= TARGET_PROPERTY;
This code is perhaps a little easier to understand if you use == instead of <=
and >= because then you don't have to look up the enum and check what falls in
that range.

https://codereview.chromium.org/1168643005/diff/300001/src/expression-classif...
src/expression-classifier.h:142: return lhs_type_ == TARGET_PATTERN &&
assigned_;
I think this can be written as `return IsValidPattern() && IsAssigned()`?

https://codereview.chromium.org/1168643005/diff/300001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1168643005/diff/300001/src/preparser.h#newcod...
src/preparser.h:514: bool rewrite =
Maybe make this `const bool`.  The mutable version immediately made me scan the
code below for reassignment that doesn't happen.

https://codereview.chromium.org/1168643005/diff/300001/src/preparser.h#newcod...
src/preparser.h:2924: classifier->set_assigned();
This is a little easier to comprehend if you swap the clauses around (although
it makes the diff noisier.)

[Dmitry Lomov (no reviews), 2015-06-19 08:57:30]

Very sorry for a looong delay :(
Looks good to me modulo other people's comments and a test comment below.

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

https://codereview.chromium.org/1168643005/diff/300001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:6565: "{ x : y }",
Add more tests of the form:
 { x : y.z } = ...
 { x : foo().x ) = ...
and so on (where the binding location is not a valid binding)

You have some of these, but not all. Consider the tests of "pattern context":
   { x : E }
   { x : { y : E }}
   [E]
   [a, b, E]
   ....
where E is various valid assignment targets (x.y, foo(y), x[i] etc.)

[caitp (gmail), 2015-06-19 14:23:21]

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

https://codereview.chromium.org/1168643005/diff/300001/src/expression-classif...
src/expression-classifier.h:91: TARGET_RESTRICTED,
On 2015/06/15 22:01:09, noordhuis wrote:
> TARGET_RESTRICTED doesn't seem to be used anywhere.

Done.

https://codereview.chromium.org/1168643005/diff/300001/src/expression-classif...
src/expression-classifier.h:126: return lhs_type_ >= TARGET_IDENTIFIER &&
lhs_type_ <= TARGET_PROPERTY;
On 2015/06/15 22:01:09, noordhuis wrote:
> This code is perhaps a little easier to understand if you use == instead of <=
> and >= because then you don't have to look up the enum and check what falls in
> that range.

Done.

https://codereview.chromium.org/1168643005/diff/300001/src/expression-classif...
src/expression-classifier.h:142: return lhs_type_ == TARGET_PATTERN &&
assigned_;
On 2015/06/15 22:01:09, noordhuis wrote:
> I think this can be written as `return IsValidPattern() && IsAssigned()`?

Done.

https://codereview.chromium.org/1168643005/diff/300001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1168643005/diff/300001/src/preparser.h#newcod...
src/preparser.h:514: bool rewrite =
On 2015/06/15 22:01:09, noordhuis wrote:
> Maybe make this `const bool`.  The mutable version immediately made me scan
the
> code below for reassignment that doesn't happen.

Done.

https://codereview.chromium.org/1168643005/diff/300001/src/preparser.h#newcod...
src/preparser.h:2924: classifier->set_assigned();
On 2015/06/15 22:01:09, noordhuis wrote:
> This is a little easier to comprehend if you swap the clauses around (although
> it makes the diff noisier.)

Done.

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

https://codereview.chromium.org/1168643005/diff/300001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:6565: "{ x : y }",
On 2015/06/19 08:57:30, Dmitry S. Lomov wrote:
> Add more tests of the form:
>  { x : y.z } = ...
>  { x : foo().x ) = ...
> and so on (where the binding location is not a valid binding)
> 
> You have some of these, but not all. Consider the tests of "pattern context":
>    { x : E }
>    { x : { y : E }}
>    [E]
>    [a, b, E]
>    ....
> where E is various valid assignment targets (x.y, foo(y), x[i] etc.)
>    

I think there are cases for each of those, but I've added a bunch more just to
be extra systematic about it

[Dmitry Lomov (no reviews), 2015-06-19 15:37:28]

lgtm

[caitp (gmail), 2015-06-19 18:09:47]

arv: what do you think? I want to make sure I don't accidentally break "normal"
assignments with this, but I think it would break them with or without the flag
if it was going to break them

[arv (Not doing code reviews), 2015-06-19 18:49:05]

https://codereview.chromium.org/1168643005/diff/320001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1168643005/diff/320001/src/preparser.h#newcod...
src/preparser.h:2936: rhs_flags, needs_destructuring, &rhs_classifier,
CHECK_OK);
Don't we need to pass in a new boolean here? The outer needs_destructuring
should not be updated here.


var x, y, z;
[x] = y = z

y does not need_destructuring

var x, y, z;
x = [y] = z

x does not need_destructuring

[caitp (gmail), 2015-06-19 18:52:38]

On 2015/06/19 18:49:05, arv wrote:
> https://codereview.chromium.org/1168643005/diff/320001/src/preparser.h
> File src/preparser.h (right):
> 
>
https://codereview.chromium.org/1168643005/diff/320001/src/preparser.h#newcod...
> src/preparser.h:2936: rhs_flags, needs_destructuring, &rhs_classifier,
> CHECK_OK);
> Don't we need to pass in a new boolean here? The outer needs_destructuring
> should not be updated here.
> 
> 
> var x, y, z;
> [x] = y = z
> 
> y does not need_destructuring
> 
> var x, y, z;
> x = [y] = z
> 
> x does not need_destructuring

I thought about this --- My thinking was that it made more sense to just rewrite
the entire chain of expressions if destructuring is used anywhere in them,
instead of only each individual part involving destructuring. But I could be
sold either way on it

[caitp (gmail), 2015-06-19 18:52:41]

On 2015/06/19 18:49:05, arv wrote:
> https://codereview.chromium.org/1168643005/diff/320001/src/preparser.h
> File src/preparser.h (right):
> 
>
https://codereview.chromium.org/1168643005/diff/320001/src/preparser.h#newcod...
> src/preparser.h:2936: rhs_flags, needs_destructuring, &rhs_classifier,
> CHECK_OK);
> Don't we need to pass in a new boolean here? The outer needs_destructuring
> should not be updated here.
> 
> 
> var x, y, z;
> [x] = y = z
> 
> y does not need_destructuring
> 
> var x, y, z;
> x = [y] = z
> 
> x does not need_destructuring

I thought about this --- My thinking was that it made more sense to just rewrite
the entire chain of expressions if destructuring is used anywhere in them,
instead of only each individual part involving destructuring. But I could be
sold either way on it

[arv (Not doing code reviews), 2015-06-19 18:54:47]

On 2015/06/19 at 18:52:41, caitpotter88 wrote:
> I thought about this --- My thinking was that it made more sense to just
rewrite the entire chain of expressions if destructuring is used anywhere in
them, instead of only each individual part involving destructuring. But I could
be sold either way on it

I see. That seems fine to me too.

Re normal assignment: Tests will break if that was the case... If you want to
feel more confident you could try to build with destructuring assignent turned
on by default and make sure that no tests fail.

[arv (Not doing code reviews), 2015-06-19 18:54:55]

LGTM

[caitp (gmail), 2015-06-19 21:08:53]

On 2015/06/19 18:54:55, arv wrote:
> LGTM

It was actually failing pretty bad with the flag flipped, but as far as I can
tell I've fixed it up.

[arv (Not doing code reviews), 2015-06-19 21:21:25]

LGTM

[caitp (gmail), 2015-06-19 21:24:23]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-06-19 21:24:24]

The patchset sent to the CQ was uploaded after l-g-t-m from dslomov@chromium.org
Link to the patchset: https://codereview.chromium.org/1168643005/#ps360001
(title: "Make the error message slightly better")

[commit-bot: I haz the power, 2015-06-19 21:24:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1168643005/360001

[commit-bot: I haz the power, 2015-06-19 21:57:27]

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

[commit-bot: I haz the power, 2015-06-19 21:57:30]

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)

[Dmitry Lomov (no reviews), 2015-06-22 12:01:18]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2015-06-22 12:01:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1168643005/360001

[commit-bot: I haz the power, 2015-06-22 12:19:23]

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

[commit-bot: I haz the power, 2015-06-22 12:19:24]

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)