[parser] report better errors for multiple ForBindings in ForIn/Of loops

[parser] report better errors for multiple ForBindings in ForIn/Of loops

Instead of "unexpected token" errors, report something meatier and more actionable.

BUG=
R=marja@chromium.org, arv@chromium.org
LOG=N

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

[caitp (gmail), 2015-04-07 19:39:57]

Followup to crrev.com/1033823002

Unfortunately, it doesn't seem to clean things up as much as I wanted, but it
does report better errors, and make the error reporting consistent between
preparser and parser

[arv (Not doing code reviews), 2015-04-07 21:14:28]

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

https://codereview.chromium.org/1062263002/diff/1/src/parser.cc#newcode3394
src/parser.cc:3394: bindings_loc, "for_inof_loop_multi_bindings", loop_type);
Another option is to have two different message keys.

https://codereview.chromium.org/1062263002/diff/1/src/parser.cc#newcode3443
src/parser.cc:3443: bool accept_IN = num_decl >= 1;
Not sure I understand? for-in loop cannot have more than one declaration.

https://codereview.chromium.org/1062263002/diff/1/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1062263002/diff/1/src/preparser.cc#newcode502
src/preparser.cc:502: if (bindings_loc) {
Is this right?

[caitp (gmail), 2015-04-07 21:22:46]

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

https://codereview.chromium.org/1062263002/diff/1/src/parser.cc#newcode3394
src/parser.cc:3394: bindings_loc, "for_inof_loop_multi_bindings", loop_type);
On 2015/04/07 21:14:28, arv wrote:
> Another option is to have two different message keys.

yeah, I don't have a strong preference here. I had an idea that there could just
be a "for_loop_bad_lhs" message with a parameterized loop-type and a
parameterized reason for the invalidity, but adding support for two arguments
looked like more work. If two message keys are preferred, that's what I'll do

https://codereview.chromium.org/1062263002/diff/1/src/parser.cc#newcode3443
src/parser.cc:3443: bool accept_IN = num_decl >= 1;
On 2015/04/07 21:14:28, arv wrote:
> Not sure I understand? for-in loop cannot have more than one declaration.

Right, but the error reported below is much clearer than the one that is
currently reported (unexpected token "in"/"of")

https://codereview.chromium.org/1062263002/diff/1/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1062263002/diff/1/src/preparser.cc#newcode502
src/preparser.cc:502: if (bindings_loc) {
On 2015/04/07 21:14:28, arv wrote:
> Is this right? 

The logic is, if it's not a nullptr, set its dereferenced value to this new
location comprising of all of the declared variables.

now, it's not clear that the careful location picking is worthwhile, because the
chromium devtools don't seem to use them (as far as I can tell). Could always
get rid of these extra location pointers if it's not worth it

[marja, 2015-04-08 10:05:47]

Cool that you're unifying variable declaration parsing in Parser & PreParser,
it's one of my pet peeves that they are inconsistent!

https://codereview.chromium.org/1062263002/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1062263002/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:4644: RunParserSyncTest(context_data, data, kError,
nullptr, 0, nullptr, 0);
You don't need the params after kError.

https://codereview.chromium.org/1062263002/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:4691: 
Hmm, none of the added tests exercises the code path for the ndecl != 1 error?
Could you add some?

[caitp (gmail), 2015-04-08 12:14:47]

Patchset #2 (id:20001) has been deleted

[caitp (gmail), 2015-04-08 12:44:36]

https://codereview.chromium.org/1062263002/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1062263002/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:4691: 
On 2015/04/08 10:05:47, marja wrote:
> Hmm, none of the added tests exercises the code path for the ndecl != 1 error?
> Could you add some?

done

[marja, 2015-04-08 15:11:05]

lgtm whenever arv is happy

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

https://codereview.chromium.org/1062263002/diff/80001/src/parser.cc#newcode3382
src/parser.cc:3382: bool accept_IN = num_decl >= 1;
I was reading this code for a while and it's slightly messy that we first check
for >= 1 and almost right after, check for != 1 - but I guess this is necessary
to produce the right kinds of errors. E.g., if there's no "in" or "of" keyword,
we want to whine about that, not about multiple declarations, I guess. So I
don't have any concrete ideas on how to make this better.

[arv (Not doing code reviews), 2015-04-08 15:23:00]

I'm happy!

LGTM

[caitp (gmail), 2015-04-08 15:41:29]

On 2015/04/08 15:23:00, arv wrote:
> I'm happy!
> 
> LGTM

Cool. I think the message text could be a bit better, but I guess can make it
better later

[caitp (gmail), 2015-04-08 18:23:28]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-04-08 18:23:47]

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

[commit-bot: I haz the power, 2015-04-08 18:47:42]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-04-08 18:48:08]

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