[es6] Implement destructuring binding in try/catch

[es6] Implement destructuring binding in try/catch

The approach is to desugar

  try { ... }
  catch ({x, y}) { ... }

into

  try { ... }
  catch (.catch) {
    let x = .catch.x;
    let y = .catch.y;
    ...
  }

using the PatternRewriter's normal facilities. This has the side benefit
of throwing the appropriate variable conflict errors for declarations
made inside the catch block.

No change is made to non-destructured cases, which will hopefully save
us some work if https://github.com/tc39/ecma262/issues/150 is adopted
in the spec.

There's one big problem with this patch, which is a lack of PreParser
support for the redeclaration errors. But it seems we're already lacking
good PreParser support for such errors, so I'm not sure that should
block this moving forward.

BUG=v8:811
LOG=y

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

[adamk, 2015-11-03 00:27:56]

adamk@chromium.org changed reviewers:
+ rossberg@chromium.org

[adamk, 2015-11-03 00:27:57]

This is ready for a look, though it needs some polish before landing.

[rossberg, 2015-11-03 12:00:50]

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

https://codereview.chromium.org/1417483014/diff/1/src/parser.cc#newcode3162
src/parser.cc:3162: bool is_simple = false;
Nit: why not `is_simple = pattern->IsVariableProxy()` here, and then just `if
(is_simple)` below?

https://codereview.chromium.org/1417483014/diff/1/src/parser.cc#newcode3195
src/parser.cc:3195: scope_->DeclarationScope();  // is this right?
Judging from how it's set in the other cases, this should rather be scope_
itself here, because the variables are let-like. However, the only real use
seems to be for finding the initialisation scope for const and to check for too
many variables -- the former does cannot occur for catch, whereas the latter is
hard to trigger.

I wonder why we even need this field, since it should always equate

IsLexicalVariableMode(desc.mode) ? desc.scope : desc.scope->DeclarationScope()

Could we just refactor the DeclarationScope method of the Parser, i.e. move it
to Scope, and use that instead?

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

https://codereview.chromium.org/1417483014/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:1131: assertEquals("undefined", typeof
foo);
This is not a sufficient test, because it will be true even if the variable _is_
scoped here but uninitialised. Perhaps add a test that shadows a variable in
this scope and check that its value isn't clobbered.

[adamk, 2015-11-03 20:35:20]

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

https://codereview.chromium.org/1417483014/diff/1/src/parser.cc#newcode3162
src/parser.cc:3162: bool is_simple = false;
On 2015/11/03 12:00:50, rossberg wrote:
> Nit: why not `is_simple = pattern->IsVariableProxy()` here, and then just `if
> (is_simple)` below?

Done.

https://codereview.chromium.org/1417483014/diff/1/src/parser.cc#newcode3195
src/parser.cc:3195: scope_->DeclarationScope();  // is this right?
On 2015/11/03 12:00:50, rossberg wrote:
> Judging from how it's set in the other cases, this should rather be scope_
> itself here, because the variables are let-like. However, the only real use
> seems to be for finding the initialisation scope for const and to check for
too
> many variables -- the former does cannot occur for catch, whereas the latter
is
> hard to trigger.
> 
> I wonder why we even need this field, since it should always equate
> 
> IsLexicalVariableMode(desc.mode) ? desc.scope : desc.scope->DeclarationScope()
> 
> Could we just refactor the DeclarationScope method of the Parser, i.e. move it
> to Scope, and use that instead?

There's definitely some refactoring to be done here, I'll leave that for another
patch. Switched to scope_ for now.

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

https://codereview.chromium.org/1417483014/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:1131: assertEquals("undefined", typeof
foo);
On 2015/11/03 12:00:50, rossberg wrote:
> This is not a sufficient test, because it will be true even if the variable
_is_
> scoped here but uninitialised. Perhaps add a test that shadows a variable in
> this scope and check that its value isn't clobbered.

Fixed.

Not sure how much more to test here, I'm not doing anything special to support
catch.

[rossberg, 2015-11-04 10:26:56]

lgtm

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

https://codereview.chromium.org/1417483014/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:1131: assertEquals("undefined", typeof
foo);
On 2015/11/03 20:35:20, adamk wrote:
> On 2015/11/03 12:00:50, rossberg wrote:
> > This is not a sufficient test, because it will be true even if the variable
> _is_
> > scoped here but uninitialised. Perhaps add a test that shadows a variable in
> > this scope and check that its value isn't clobbered.
> 
> Fixed.
> 
> Not sure how much more to test here, I'm not doing anything special to support
> catch.

Yeah, I think it's fine. Setting up the descriptor properly (esp the scopes) is
the main thing that could go wrong.

[adamk, 2015-11-04 16:04:20]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2015-11-04 16:04:35]

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

[commit-bot: I haz the power, 2015-11-04 16:06:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-04 16:06:51]

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

[adamk, 2015-11-04 16:39:22]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1408063013/ by adamk@chromium.org.

The reason for reverting is: MSAN errors on arm64:
http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20s....