[parser] early error when declaration Pattern missing Initializer

[parser] early error when declaration Pattern missing Initializer

Emit an early error when BindingPatterns are used in a VariableDeclaration
or LexicalBinding without an Initializer.

BUG=v8:4532
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, wingo@igalia.com

Committed: https://crrev.com/55e1cfebfd72ba9ba418091dab98ff7724222b02
Cr-Commit-Position: refs/heads/master@{#31802}

[caitp (gmail), 2015-11-03 19:04:59]

PTAL

It's more code than it should be to do this consistently in both parsers, but
that's how it is

[adamk, 2015-11-03 20:13:53]

https://codereview.chromium.org/1416753009/diff/1/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1416753009/diff/1/src/messages.h#newcode130
src/messages.h:130: T(InvalidDestructuringDeclaration,                          
                \
Looks like we're currently giving bad error messages for this for the const
case, so it seems like this could just be "MissingInitializer" and the message
could be broadened to cover that case too.

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

https://codereview.chromium.org/1416753009/diff/1/src/parser.cc#newcode2587
src/parser.cc:2587: if (peek() == Token::ASSIGN ||
(parsing_result->descriptor.mode == CONST &&
This if statement is already set up for handling the required initializer case,
I'd add your code here instead.

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

https://codereview.chromium.org/1416753009/diff/1/src/preparser.cc#newcode534
src/preparser.cc:534: bool require_initializer = false;
Similarly (though sadly differently), this is already tracking the "initializer
required" information.

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

https://codereview.chromium.org/1416753009/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:1107: (function
TestDeclDestructuringArrayWithoutInitializer() {
Could you make this and the below into message tests instead? Then you'll also
be covering the position information with the test case.

[caitp (gmail), 2015-11-03 23:55:31]

- I've added a bunch of message tests and gotten rid of the syntax error tests
in mjsunit/harmony/destructuring.js

- I've also refactored to combine stuff into the existing if-statement that
already handles initializers. This was a bit of a pain to get working properly
for the PreParser version of the code, and I think that stuff definitely could
use a second look.

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

https://codereview.chromium.org/1416753009/diff/1/src/parser.cc#newcode2587
src/parser.cc:2587: if (peek() == Token::ASSIGN ||
(parsing_result->descriptor.mode == CONST &&
On 2015/11/03 20:13:53, adamk wrote:
> This if statement is already set up for handling the required initializer
case,
> I'd add your code here instead.

Done.

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

https://codereview.chromium.org/1416753009/diff/1/src/preparser.cc#newcode534
src/preparser.cc:534: bool require_initializer = false;
On 2015/11/03 20:13:53, adamk wrote:
> Similarly (though sadly differently), this is already tracking the
"initializer
> required" information.

The preparser doesn't do an amazing job of tracking this info, because it
doesn't care about `for-in/of`, so I don't think adding the `Token::IN ||
CheckContextualKeyword("of")` stuff can really disappear completely. The
preparser is actually doing some weird stuff here, and I don't 100% understand
it :\ What is the "is_strict_const" stuff about?

[adamk, 2015-11-04 00:37:54]

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

https://codereview.chromium.org/1416753009/diff/20001/src/parser.cc#newcode2578
src/parser.cc:2578: if (peek() != Token::ASSIGN) {
You can also consume here to avoid mentioning ASSIGN once more below:

if (!Check(Token::ASSIGN)) {
  ...
}

https://codereview.chromium.org/1416753009/diff/20001/src/parser.cc#newcode2580
src/parser.cc:2580: Scanner::Location(decl_pos, scanner()->location().end_pos),
I'm not sure this location makes any more sense than just pointing at where the
missing ASSIGN token should have been (as the old code did). Seems weird to
point at the syntactically-valid pattern or variable name when it's what doesn't
come after it that's the problem.

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc#newcod...
src/preparser.cc:560: is_strict_const = true;
This means "is not legacy const".

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc#newcod...
src/preparser.cc:561: require_initializer = true;
Seems like "is_const" would be a better name for this variable now.

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc#newcod...
src/preparser.cc:589: is_pattern = pattern.IsObjectLiteral();  // (or
pattern.IsArrayLiteral());
I'd rather you just add the bit for ArrayLiteral to PreParserExpression and make
this an actual || in the code.

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc#newcod...
src/preparser.cc:610: (!is_for_iteration_variable && (require_initializer ||
is_pattern)) ||
Maybe put this in the same order as in the parser to make it easier to read?

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc#newcod...
src/preparser.cc:612: (is_strict_const && peek() == Token::COMMA)) {
I think this bit is now redundant with your is_for_iteration_variable thing. See
https://codereview.chromium.org/671913002 for its origin, I think it was just a
mistake.

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc#newcod...
src/preparser.cc:613: if (peek() != Token::ASSIGN) {
Same note here as in the parser, you can use if (!Check(...))

[caitp (gmail), 2015-11-04 00:50:02]

I'll have an updated patchset in the morning

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

https://codereview.chromium.org/1416753009/diff/20001/src/parser.cc#newcode2578
src/parser.cc:2578: if (peek() != Token::ASSIGN) {
On 2015/11/04 00:37:54, adamk wrote:
> You can also consume here to avoid mentioning ASSIGN once more below:
> 
> if (!Check(Token::ASSIGN)) {
>   ...
> }

Yeah, it messes up the positioning a bit if i do that. I doubt anyone other than
d8/node users would notice, but still

https://codereview.chromium.org/1416753009/diff/20001/src/parser.cc#newcode2580
src/parser.cc:2580: Scanner::Location(decl_pos, scanner()->location().end_pos),
On 2015/11/04 00:37:54, adamk wrote:
> I'm not sure this location makes any more sense than just pointing at where
the
> missing ASSIGN token should have been (as the old code did). Seems weird to
> point at the syntactically-valid pattern or variable name when it's what
doesn't
> come after it that's the problem.

If the devtools actually showed the positions, I'd personally get more context
from seeing the object that needs the initializer. But since devtools don't do
this, it doesn't matter a whole lot. I'll change it to be simpler i guess

[adamk, 2015-11-04 00:55:10]

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

https://codereview.chromium.org/1416753009/diff/20001/src/parser.cc#newcode2578
src/parser.cc:2578: if (peek() != Token::ASSIGN) {
On 2015/11/04 00:50:02, caitp wrote:
> On 2015/11/04 00:37:54, adamk wrote:
> > You can also consume here to avoid mentioning ASSIGN once more below:
> > 
> > if (!Check(Token::ASSIGN)) {
> >   ...
> > }
> 
> Yeah, it messes up the positioning a bit if i do that. I doubt anyone other
than
> d8/node users would notice, but still

Ah, right, feel free to leave as-is.

https://codereview.chromium.org/1416753009/diff/20001/src/parser.cc#newcode2580
src/parser.cc:2580: Scanner::Location(decl_pos, scanner()->location().end_pos),
On 2015/11/04 00:50:02, caitp wrote:
> On 2015/11/04 00:37:54, adamk wrote:
> > I'm not sure this location makes any more sense than just pointing at where
> the
> > missing ASSIGN token should have been (as the old code did). Seems weird to
> > point at the syntactically-valid pattern or variable name when it's what
> doesn't
> > come after it that's the problem.
> 
> If the devtools actually showed the positions, I'd personally get more context
> from seeing the object that needs the initializer. But since devtools don't do
> this, it doesn't matter a whole lot. I'll change it to be simpler i guess

I too have been wondering why devtools doesn't show the positions.

[rossberg, 2015-11-04 10:52:36]

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

https://codereview.chromium.org/1416753009/diff/20001/src/parser.cc#newcode2534
src/parser.cc:2534: bool is_pattern = false;
Nit: declare this after the block and initialise directly.

https://codereview.chromium.org/1416753009/diff/20001/src/parser.cc#newcode2576
src/parser.cc:2576: ((parsing_result->descriptor.mode == CONST || is_pattern) &&
Why not test these in the else branch and move the error generation there?

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc#newcod...
src/preparser.cc:610: (!is_for_iteration_variable && (require_initializer ||
is_pattern)) ||
Same comment as in the parser: move this to else branch.

[caitp (gmail), 2015-11-04 15:07:28]

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

https://codereview.chromium.org/1416753009/diff/20001/src/parser.cc#newcode2576
src/parser.cc:2576: ((parsing_result->descriptor.mode == CONST || is_pattern) &&
On 2015/11/04 10:52:35, rossberg wrote:
> Why not test these in the else branch and move the error generation there?

I think I've done what you're asking for here --- it looks a bit cleaner and
passes tests, at any rate

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc#newcod...
src/preparser.cc:589: is_pattern = pattern.IsObjectLiteral();  // (or
pattern.IsArrayLiteral());
On 2015/11/04 00:37:54, adamk wrote:
> I'd rather you just add the bit for ArrayLiteral to PreParserExpression and
make
> this an actual || in the code.

Done, although I think it will mean adding an extra bit to the TypeField for the
destructuring assignment parsing CL

https://codereview.chromium.org/1416753009/diff/20001/src/preparser.cc#newcod...
src/preparser.cc:612: (is_strict_const && peek() == Token::COMMA)) {
On 2015/11/04 00:37:54, adamk wrote:
> I think this bit is now redundant with your is_for_iteration_variable thing.
See
> https://codereview.chromium.org/671913002 for its origin, I think it was just
a
> mistake.

Well, this was the only place the variable was used --- I've gotten rid of it
and tests still pass, so it's probably okay

[rossberg, 2015-11-04 15:10:32]

LGTM, but let Adam take another pass, too.

[adamk, 2015-11-04 15:59:25]

lgtm

[caitp (gmail), 2015-11-04 19:02:08]

Description was changed from

==========
[parser] early error when declaration Pattern missing Initializer

Emit an early error when BindingPatterns are used in a VariableDeclaration
or LexicalBinding without an Initializer.

BUG=v8:4532
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, wingo@chromium.org
==========

to

==========
[parser] early error when declaration Pattern missing Initializer

Emit an early error when BindingPatterns are used in a VariableDeclaration
or LexicalBinding without an Initializer.

BUG=v8:4532
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, wingo@igalia.com
==========

[caitp (gmail), 2015-11-04 19:02:08]

caitpotter88@gmail.com changed reviewers:
+ wingo@igalia.com
- wingo@chromium.org

[caitp (gmail), 2015-11-04 19:02:42]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-11-04 19:02:53]

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

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

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-04 19:26:46]

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