Use ExpressionClassifier to identify valid arrow function formals

Use ExpressionClassifier to identify valid arrow function formals

R=dslomov@chromium.org
LOG=N
BUG=

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

[wingo, 2015-05-12 12:12:50]

Early WIP to use the expression classifier to identify valid arrow functions. 
Next revision will use the classifier result instead of the ValidArrowFunction
bits.  WDYT, is it on the right track?

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

Yes, this makes sense to me

[wingo, 2015-05-12 13:51:25]

Updated patch is pretty much complete modulo a couple of tests.  Three things
aren't working:

 1) {} is a valid BindingPattern but not a valid BindingElement.  So the tests
that e.g. ({})=>x is not a valid arrow function fail.

 2) Likewise for [].

 3) The test that (a=b)=>foo is invalid is now failing, I think because a=b is a
valid component of a BindingPattern but I'm not sure.

Any thoughts on these, Dmitry?  I guess I can add special cases to {} and [] to
make them call ArrowFormalParametersUnexpectedToken.

[caitp (gmail), 2015-05-12 14:01:50]

On 2015/05/12 13:51:25, wingo wrote:
> Updated patch is pretty much complete modulo a couple of tests.  Three things
> aren't working:
> 
>  1) {} is a valid BindingPattern but not a valid BindingElement.  So the tests
> that e.g. ({})=>x is not a valid arrow function fail.
> 

How is it not a valid BindingElement? See 13.2.3.6 for BindingElement :
BindingPattern Initializer[opt]

>  2) Likewise for [].
> 
>  3) The test that (a=b)=>foo is invalid is now failing, I think because a=b is
a
> valid component of a BindingPattern but I'm not sure.
> 
> Any thoughts on these, Dmitry?  I guess I can add special cases to {} and []
to
> make them call ArrowFormalParametersUnexpectedToken.

[wingo, 2015-05-12 14:02:43]

On 2015/05/12 13:51:25, wingo wrote:
> Updated patch is pretty much complete modulo a couple of tests.  Three things
> aren't working:
> 
>  1) {} is a valid BindingPattern but not a valid BindingElement.  So the tests
> that e.g. ({})=>x is not a valid arrow function fail.
> 
>  2) Likewise for [].
> 
>  3) The test that (a=b)=>foo is invalid is now failing, I think because a=b is
a
> valid component of a BindingPattern but I'm not sure.
> 
> Any thoughts on these, Dmitry?  I guess I can add special cases to {} and []
to
> make them call ArrowFormalParametersUnexpectedToken.

Actually it seems that [] isn't a valid BindingPattern either.  I guess we
should be classifying BindingElement and BindingPattern separately.

[wingo, 2015-05-12 14:03:43]

On 2015/05/12 14:01:50, caitp wrote:
> On 2015/05/12 13:51:25, wingo wrote:
> > Updated patch is pretty much complete modulo a couple of tests.  Three
things
> > aren't working:
> > 
> >  1) {} is a valid BindingPattern but not a valid BindingElement.  So the
tests
> > that e.g. ({})=>x is not a valid arrow function fail.
> > 
> 
> How is it not a valid BindingElement? See 13.2.3.6 for BindingElement :
> BindingPattern Initializer[opt]

Good catch caitp, thank you!

[wingo, 2015-05-12 14:13:02]

On 2015/05/12 14:03:43, wingo wrote:
> On 2015/05/12 14:01:50, caitp wrote:
> > On 2015/05/12 13:51:25, wingo wrote:
> > > Updated patch is pretty much complete modulo a couple of tests.  Three
> things
> > > aren't working:
> > > 
> > >  1) {} is a valid BindingPattern but not a valid BindingElement.  So the
> tests
> > > that e.g. ({})=>x is not a valid arrow function fail.
> > > 
> > 
> > How is it not a valid BindingElement? See 13.2.3.6 for BindingElement :
> > BindingPattern Initializer[opt]
> 
> Good catch caitp, thank you!

So yeah then, I think this patch works fine.  The only question is how to turn
off destructuring to be able to ship these things separately.

[Dmitry Lomov (no reviews), 2015-05-12 14:19:55]

On 2015/05/12 14:13:02, wingo wrote:
> On 2015/05/12 14:03:43, wingo wrote:
> > On 2015/05/12 14:01:50, caitp wrote:
> > > On 2015/05/12 13:51:25, wingo wrote:
> > > > Updated patch is pretty much complete modulo a couple of tests.  Three
> > things
> > > > aren't working:
> > > > 
> > > >  1) {} is a valid BindingPattern but not a valid BindingElement.  So the
> > tests
> > > > that e.g. ({})=>x is not a valid arrow function fail.
> > > > 
> > > 
> > > How is it not a valid BindingElement? See 13.2.3.6 for BindingElement :
> > > BindingPattern Initializer[opt]
> > 
> > Good catch caitp, thank you!
> 
> So yeah then, I think this patch works fine.  The only question is how to turn
> off destructuring to be able to ship these things separately.

ExpressionClassifier does not depend on destructuring flag

[wingo, 2015-05-12 14:28:38]

On 2015/05/12 14:19:55, Dmitry Lomov (chromium) wrote:
> On 2015/05/12 14:13:02, wingo wrote:
> > So yeah then, I think this patch works fine.  The only question is how to
turn
> > off destructuring to be able to ship these things separately.
> 
> ExpressionClassifier does not depend on destructuring flag

Understood, but if we are to use the classifier to detect valid arrow parameters
in the preparser, we need to not allow ({},{})=>({}) until destructuring
arguments is enabled.  I added a couple BindingPatternUnexpectedToken calls to
that effect; wdyt, horrible idea or no?

[Dmitry Lomov (no reviews), 2015-05-12 14:38:59]

On 2015/05/12 14:28:38, wingo wrote:
> On 2015/05/12 14:19:55, Dmitry Lomov (chromium) wrote:
> > On 2015/05/12 14:13:02, wingo wrote:
> > > So yeah then, I think this patch works fine.  The only question is how to
> turn
> > > off destructuring to be able to ship these things separately.
> > 
> > ExpressionClassifier does not depend on destructuring flag
> 
> Understood, but if we are to use the classifier to detect valid arrow
parameters
> in the preparser, we need to not allow ({},{})=>({}) until destructuring
> arguments is enabled.  I added a couple BindingPatternUnexpectedToken calls to
> that effect; wdyt, horrible idea or no?
I see; yes that is the right approach.

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

https://codereview.chromium.org/1138153003/diff/40001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1138153003/diff/40001/src/preparser.h#newcode677
src/preparser.h:677: void RecordArrowFormalParametersError(const
Scanner::Location& loc,
Instead of having all these methods, I suggest merging
FormalParameterErrorLocations into ExpressionClassifier and just use one common
mechanism to classify all expression-like constructs.

[wingo, 2015-05-13 09:27:47]

Updated patch merges FormalParameterErrorLocations into the ExpressionClassifier
as discussed offline.  I tried to make a separate patch but it makes sense to
solve the arrow function stuff at the same time, and was gnarly to separate. 
WDYT Dmitry?

There's one pending TODO in the patch to remove some of the checks when
re-parsing arrow function parameters in the full parser.  The classifier should
have ensured that they will parse, and so we should be able to DCHECK that the
expressions are of the right type.

[Dmitry Lomov (no reviews), 2015-05-13 09:43:17]

lgtm

[wingo, 2015-05-13 09:48:04]

The CQ bit was checked by wingo@igalia.com

[commit-bot: I haz the power, 2015-05-13 09:56:45]

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

[commit-bot: I haz the power, 2015-05-13 10:25:07]

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

[commit-bot: I haz the power, 2015-05-13 10:25:08]

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/...)

[wingo, 2015-05-13 10:39:44]

On 2015/05/13 10:25:08, I haz the power (commit-bot) wrote:
> 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/...)

The test that failed with asan, regress/regress-1132.js, is short.  It is below.
 The failure was that stack overflow was signalled but not caught.

// Test the case when exception is thrown from the parser when lazy
// compiling a function.

// Flags: --stack_size=32
// NOTE: stack size constant above has been empirically chosen.
// If the test starts to fail in Genesis, consider increasing this constant.

function test() {
  try {
    test(1, test(1));
  } catch(e) {
    assertFalse(delete e, "deleting catch variable");
    assertEquals(42, e);
  }
}

var exception = false;
try {
  test();
} catch (e) {
  exception = true;
}
assertTrue(exception);

I am thinking that the increased stack usage due to the few extra on-stack
ExpressionClassifier fields is making us brush against this 32 kB limit.  I will
bump the limit and re-run the builder.

[Dmitry Lomov (no reviews), 2015-05-13 11:21:02]

lgtm with bumped stack limit

[wingo, 2015-05-13 11:23:26]

The CQ bit was checked by wingo@igalia.com

[commit-bot: I haz the power, 2015-05-13 11:23:36]

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

[commit-bot: I haz the power, 2015-05-13 11:45:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-13 11:45:27]

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

[arv (Not doing code reviews), 2015-05-13 14:13:04]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2015-05-13 14:13:05]

LGTM

https://codereview.chromium.org/1138153003/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1138153003/diff/80001/src/parser.cc#newcode4029
src/parser.cc:4029: const bool allow_duplicate_parameters =
Does this handle patterns?

// sloppy
function f(x  // ok
function f(x, x  // ok
function f(x, x, {  // no longer ok
function f(x, x =  // no longer ok

[wingo, 2015-05-15 12:30:31]

https://codereview.chromium.org/1138153003/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1138153003/diff/80001/src/parser.cc#newcode4029
src/parser.cc:4029: const bool allow_duplicate_parameters =
On 2015/05/13 14:13:05, arv wrote:
> Does this handle patterns?
> 
> // sloppy
> function f(x  // ok
> function f(x, x  // ok
> function f(x, x, {  // no longer ok
> function f(x, x =  // no longer ok

I guess it should but it doesn't.  In a similar way, duplicate bindings in
patterns are not yet detected, at least not in the preparser.  That will be
tricky to do given that a previous attempt to use the DuplicateFinder for both
the parser and the preparser regressed performance.  Perhaps changing the
DuplicateFinder to allocate in the zone instead of via malloc would improve
things, though.  In any case it's a problem we will need to solve for arrow
functions too:

  (x, x) => 10 // error, but currently not caught by the preparser