[destructuring] Implement basic binding destructuring infrastructure

[destructuring] Implement basic binding destructuring infrastructure

This patch:
  - Refactors Parser::ParseVariableDeclarations
  - Introduces Parser::PatternMatcher class
  - Implements matching a single variable pattern
  - Implements rudimentary matching against object literal pattern
    as a proof of concept

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

Committed: https://crrev.com/5bbe7992db92710e6615c4d2aab6a6ef45a3620f
Cr-Commit-Position: refs/heads/master@{#28345}

[Dmitry Lomov (no reviews), 2015-05-08 18:04:55]

Please take initial look at your leisure. 
I will do a round of cleanups later, but let if you have any high-level
comments.

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

Looks like a good approach.

Some high level comments

- Use macro for visitor
- Can you use a non inner class and provide an .h file for PatternMatcher?
Parser.h is already huge.

https://codereview.chromium.org/1130623004/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1130623004/diff/1/src/parser.cc#newcode2496
src/parser.cc:2496: // statements).
I'm not sure what this means?

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc
File src/pattern-matcher.cc (right):

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:9: 
Can I has .h file?

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:146: //
stest-parsing/InitializedDeclarationsInStrictForOfError tart context for
typo

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:151: DCHECK(proxy != NULL);
DCHECK_NOT_NULL

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:190: for (auto i = properties->begin(); i !=
properties->end(); i++) {
Can we do?

for (auto property : properties) {

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:196: property->key(), RelocInfo::kNoPosition));
TODO(dslomov): Computed property names

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:207: void Parser::PatternMatcher::VisitProperty(Property*
node) { UNREACHABLE(); }
This is only unreachable in binding patterns:

var o = {x: 1};
({y: o.x} = {y: 2});
assert(o.x === 2);

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:320: void
Parser::PatternMatcher::VisitDoWhileStatement(DoWhileStatement* node) {
Maybe a macro for all of these?

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:341: void
Parser::PatternMatcher::VisitAssignment(Assignment* node) {
TODO(dslomov): Implement

var {x: y = 42} = {};
assert(y === 42);

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:356: void Parser::PatternMatcher::VisitSpread(Spread*
node) { UNREACHABLE(); }
TODO(dslomov): Implement

var [...xs] = []

https://codereview.chromium.org/1130623004/diff/1/test/mjsunit/harmony/destru...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1130623004/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:8: var { x : x, y : y } = { x : 1, y : 2
};
Also:

var {z} = {z: 3};

Should work for free since the parser desugars it before it gets to the
destructuring.

[Dmitry Lomov (no reviews), 2015-05-08 19:17:27]

On 2015/05/08 19:09:49, arv wrote:
> Looks like a good approach.
> 
> Some high level comments
> 
> - Use macro for visitor
> - Can you use a non inner class and provide an .h file for PatternMatcher?
> Parser.h is already huge.
This means that PatternMatcher has to be a friend class of Parser. Do you think
it is preferable to inner class?

[arv (Not doing code reviews), 2015-05-08 19:35:51]

On 2015/05/08 19:17:27, Dmitry Lomov (chromium) wrote:
> On 2015/05/08 19:09:49, arv wrote:
> > Looks like a good approach.
> > 
> > Some high level comments
> > 
> > - Use macro for visitor
> > - Can you use a non inner class and provide an .h file for PatternMatcher?
> > Parser.h is already huge.
> This means that PatternMatcher has to be a friend class of Parser. Do you
think
> it is preferable to inner class?

I'll let you make the call since you have more C++ experience than me.

[Dmitry Lomov (no reviews), 2015-05-11 11:07:28]

PTAL

https://codereview.chromium.org/1130623004/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1130623004/diff/1/src/parser.cc#newcode2496
src/parser.cc:2496: // statements).
On 2015/05/08 19:09:48, arv wrote:
> I'm not sure what this means?

This comment is stale, the correct place for it is in
PatternMatcher::VisitVariableProxy. Deleted this one and clarified the other one
a bit, let me know if it helps.

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc
File src/pattern-matcher.cc (right):

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:146: //
stest-parsing/InitializedDeclarationsInStrictForOfError tart context for
On 2015/05/08 19:09:49, arv wrote:
> typo

Done.

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:151: DCHECK(proxy != NULL);
On 2015/05/08 19:09:49, arv wrote:
> DCHECK_NOT_NULL

Done.

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:190: for (auto i = properties->begin(); i !=
properties->end(); i++) {
On 2015/05/08 19:09:49, arv wrote:
> Can we do?
> 
> for (auto property : properties) {

Done.

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:196: property->key(), RelocInfo::kNoPosition));
On 2015/05/08 19:09:49, arv wrote:
> TODO(dslomov): Computed property names

Done.

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:207: void Parser::PatternMatcher::VisitProperty(Property*
node) { UNREACHABLE(); }
On 2015/05/08 19:09:49, arv wrote:
> This is only unreachable in binding patterns:
> 
> var o = {x: 1};
> ({y: o.x} = {y: 2});
> assert(o.x === 2);

I am not dealing with assignment patterns yet. As the code is now, this is
unreachable.

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:320: void
Parser::PatternMatcher::VisitDoWhileStatement(DoWhileStatement* node) {
On 2015/05/08 19:09:49, arv wrote:
> Maybe a macro for all of these?

Done.

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:341: void
Parser::PatternMatcher::VisitAssignment(Assignment* node) {
On 2015/05/08 19:09:49, arv wrote:
> TODO(dslomov): Implement
> 
> var {x: y = 42} = {};
> assert(y === 42);

Done.

https://codereview.chromium.org/1130623004/diff/1/src/pattern-matcher.cc#newc...
src/pattern-matcher.cc:356: void Parser::PatternMatcher::VisitSpread(Spread*
node) { UNREACHABLE(); }
On 2015/05/08 19:09:48, arv wrote:
> TODO(dslomov): Implement
> 
> var [...xs] = []

Done.

https://codereview.chromium.org/1130623004/diff/1/test/mjsunit/harmony/destru...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1130623004/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:8: var { x : x, y : y } = { x : 1, y : 2
};
On 2015/05/08 19:09:49, arv wrote:
> Also:
> 
> var {z} = {z: 3};
> 
> Should work for free since the parser desugars it before it gets to the
> destructuring.

Done.

[Dmitry Lomov (no reviews), 2015-05-11 11:19:23]

PTAL, this is ready for proper review.

[rossberg, 2015-05-11 11:49:33]

https://codereview.chromium.org/1130623004/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1130623004/diff/20001/src/parser.cc#newcode2373
src/parser.cc:2373: // Create new block with one expected declaration.
Nit: Is the "one" comment still accurate?

https://codereview.chromium.org/1130623004/diff/20001/src/parser.cc#newcode2376
src/parser.cc:2376: decl.nvars = &nvars;
Why not move nvars itself to decl, instead of a pointer?

https://codereview.chromium.org/1130623004/diff/20001/src/parser.cc#newcode2417
src/parser.cc:2417: // Parse initialization expression if present and/or needed.
A
The remainder of this comment mainly explains the definition of
initialization_scope that followed previously and is now gone. You probably want
to move it into PatternMatcher::VisitVariableProxy. Also, abstracting
initialization_scope into a generic helper function seems inappropriate to me,
since it's only meaningful for the specific hack described by this comment
(i.e., it's nonsensical for lexical declarations' inits, for example).

https://codereview.chromium.org/1130623004/diff/20001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1130623004/diff/20001/src/parser.h#newcode942
src/parser.h:942: class PatternMatcher : private AstVisitor {
Can we call this PatternRewriter? It isn't a "matcher" by itself...  And would
it be possible to move the actual declaration to a separate .h file and just
forward-declare it here?

https://codereview.chromium.org/1130623004/diff/20001/src/parser.h#newcode959
src/parser.h:959: return is_const ? declaration_scope : scope;
I think this function is not very meaningful as an abstraction, see other
comment.

[Dmitry Lomov (no reviews), 2015-05-11 12:52:58]

PTAL - comments addressed

https://codereview.chromium.org/1130623004/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1130623004/diff/20001/src/parser.cc#newcode2373
src/parser.cc:2373: // Create new block with one expected declaration.
On 2015/05/11 11:49:33, rossberg wrote:
> Nit: Is the "one" comment still accurate?

Yes - we still expect 1 declaration in the majority of cases.

https://codereview.chromium.org/1130623004/diff/20001/src/parser.cc#newcode2376
src/parser.cc:2376: decl.nvars = &nvars;
On 2015/05/11 11:49:33, rossberg wrote:
> Why not move nvars itself to decl, instead of a pointer?

I want to keep decl_ const in PatternRewriter.

https://codereview.chromium.org/1130623004/diff/20001/src/parser.cc#newcode2417
src/parser.cc:2417: // Parse initialization expression if present and/or needed.
A
On 2015/05/11 11:49:33, rossberg wrote:
> The remainder of this comment mainly explains the definition of
> initialization_scope that followed previously and is now gone. You probably
want
> to move it into PatternMatcher::VisitVariableProxy. Also, abstracting
> initialization_scope into a generic helper function seems inappropriate to me,
> since it's only meaningful for the specific hack described by this comment
> (i.e., it's nonsensical for lexical declarations' inits, for example).

Done.

https://codereview.chromium.org/1130623004/diff/20001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1130623004/diff/20001/src/parser.h#newcode942
src/parser.h:942: class PatternMatcher : private AstVisitor {
On 2015/05/11 11:49:33, rossberg wrote:
> Can we call this PatternRewriter? It isn't a "matcher" by itself...  And would
> it be possible to move the actual declaration to a separate .h file and just
> forward-declare it here?

Oh, good trick - I didn't realize it is allowed in C++!
Done

https://codereview.chromium.org/1130623004/diff/20001/src/parser.h#newcode959
src/parser.h:959: return is_const ? declaration_scope : scope;
On 2015/05/11 11:49:33, rossberg wrote:
> I think this function is not very meaningful as an abstraction, see other
> comment.

Done.

[rossberg, 2015-05-11 13:18:29]

https://codereview.chromium.org/1130623004/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1130623004/diff/20001/src/parser.cc#newcode2376
src/parser.cc:2376: decl.nvars = &nvars;
On 2015/05/11 12:52:57, Dmitry Lomov (chromium) wrote:
> On 2015/05/11 11:49:33, rossberg wrote:
> > Why not move nvars itself to decl, instead of a pointer?
> 
> I want to keep decl_ const in PatternRewriter.

Hm, I see. In that case I'd rather pass the pointer as a separate argument to
either the PatternRewriter constructor or to DeclareAndInitializeVariables --
it's an out-argument, whereas decl is an in-argument; having it with the
function would make clearer when it actually gets modified.

https://codereview.chromium.org/1130623004/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1130623004/diff/60001/src/parser.cc#newcode2418
src/parser.cc:2418: decl.initialization_scope =
Is there a reason why this can't be local to the rewriter? E.g. where you moved
the comment? (In either case, the comment should be in the same place where this
code is.)

[Dmitry Lomov (no reviews), 2015-05-11 13:57:19]

Great suggestions, all addressed, PTAL

https://codereview.chromium.org/1130623004/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1130623004/diff/60001/src/parser.cc#newcode2418
src/parser.cc:2418: decl.initialization_scope =
On 2015/05/11 13:18:29, rossberg wrote:
> Is there a reason why this can't be local to the rewriter? E.g. where you
moved
> the comment? (In either case, the comment should be in the same place where
this
> code is.)

Done.

[arv (Not doing code reviews), 2015-05-11 14:25:17]

LGTM with nits/improvements that can be done in the future.

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

https://codereview.chromium.org/1130623004/diff/80001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:222: // TODO(dslomov): computed property names.
Also:

TODO(dslomov): Add support for initializer

let {x: y = e1} = e2;

=>

t1 = e2;
let y = (t2 = t1.x) === undefined ? e1 : t2;

https://codereview.chromium.org/1130623004/diff/80001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:255: NOT_A_PATTERN(Property)
I think this one still belongs with the implement ones above to make sure we
remember to handle it.

https://codereview.chromium.org/1130623004/diff/80001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:260: NOT_A_PATTERN(BreakStatement)
Sort these...

https://codereview.chromium.org/1130623004/diff/80001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:295: }
#undef NOT_A_PATTERN

https://codereview.chromium.org/1130623004/diff/80001/test/mjsunit/harmony/de...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1130623004/diff/80001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:12: var {z} = { z : 3 };
Maybe add let and const tests too and add TDZ check.


let {x, y} = {x: 1, y};
                    ^
                   TDZ

[Dmitry Lomov (no reviews), 2015-05-11 14:53:26]

Comments addressed

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

https://codereview.chromium.org/1130623004/diff/80001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:222: // TODO(dslomov): computed property names.
On 2015/05/11 14:25:16, arv wrote:
> Also:
> 
> TODO(dslomov): Add support for initializer
> 
> let {x: y = e1} = e2;
> 
> =>
> 
> t1 = e2;
> let y = (t2 = t1.x) === undefined ? e1 : t2;

I think this is covered by VisitAssignment

https://codereview.chromium.org/1130623004/diff/80001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:255: NOT_A_PATTERN(Property)
On 2015/05/11 14:25:16, arv wrote:
> I think this one still belongs with the implement ones above to make sure we
> remember to handle it.

I'd rather not (it really does not matter at this point)

https://codereview.chromium.org/1130623004/diff/80001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:260: NOT_A_PATTERN(BreakStatement)
On 2015/05/11 14:25:16, arv wrote:
> Sort these...

Done.

https://codereview.chromium.org/1130623004/diff/80001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:295: }
On 2015/05/11 14:25:16, arv wrote:
> #undef NOT_A_PATTERN

Done.

https://codereview.chromium.org/1130623004/diff/80001/test/mjsunit/harmony/de...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1130623004/diff/80001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:12: var {z} = { z : 3 };
On 2015/05/11 14:25:16, arv wrote:
> Maybe add let and const tests too and add TDZ check.
> 
> 
> let {x, y} = {x: 1, y};
>                     ^
>                    TDZ

This CL is just a rudimentary implementation of destructuring, for
proof-of-concept. I'll expand on implemenattion and tests in subsequent CLs

[arv (Not doing code reviews), 2015-05-11 15:09:23]

Still LGTM

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

https://codereview.chromium.org/1130623004/diff/80001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:222: // TODO(dslomov): computed property names.
On 2015/05/11 14:53:26, Dmitry Lomov (chromium) wrote:
> On 2015/05/11 14:25:16, arv wrote:
> > Also:
> > 
> > TODO(dslomov): Add support for initializer
> > 
> > let {x: y = e1} = e2;
> > 
> > =>
> > 
> > t1 = e2;
> > let y = (t2 = t1.x) === undefined ? e1 : t2;
> 
> I think this is covered by VisitAssignment

You are right.

[rossberg, 2015-05-11 15:36:24]

lgtm

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

https://codereview.chromium.org/1130623004/diff/100001/src/pattern-rewriter.c...
src/pattern-rewriter.cc:84: 
Nit: remove empty line

[Dmitry Lomov (no reviews), 2015-05-11 15:44:15]

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

https://codereview.chromium.org/1130623004/diff/100001/src/pattern-rewriter.c...
src/pattern-rewriter.cc:84: 
On 2015/05/11 15:36:24, rossberg wrote:
> Nit: remove empty line

Done.

[Dmitry Lomov (no reviews), 2015-05-11 15:44:20]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-05-11 15:44:21]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1130623004/#ps120001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-05-11 15:44:32]

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

[commit-bot: I haz the power, 2015-05-11 16:28:34]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-11 16:28:46]

Patchset 7 (id:??) landed as
https://crrev.com/5bbe7992db92710e6615c4d2aab6a6ef45a3620f
Cr-Commit-Position: refs/heads/master@{#28345}