[parser] early error when declaration Pattern missing Initializer

src/preparser.cc

Index: src/preparser.cc
diff --git a/src/preparser.cc b/src/preparser.cc
index 74e11b1d454b3b33a3f6bcadb18f91476a246f37..bf1d6a766667744cafa2acc399396ed18421232d 100644
--- a/src/preparser.cc
+++ b/src/preparser.cc
@@ -558,7 +558,7 @@ PreParser::Statement PreParser::ParseVariableDeclarations(
         (allow_harmony_sloppy() && !allow_legacy_const())) {
       DCHECK(var_context != kStatement);
       is_strict_const = true;

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

This means "is not legacy const".

-      require_initializer = var_context != kForStatement;
+      require_initializer = true;

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

Seems like "is_const" would be a better name for this variable now.

       lexical = true;
     }
   } else if (peek() == Token::LET && allow_let()) {
@@ -579,11 +579,15 @@ PreParser::Statement PreParser::ParseVariableDeclarations(
   do {
     // Parse binding pattern.
     if (nvars > 0) Consume(Token::COMMA);
+    int decl_pos = peek_position();
+    bool is_pattern = false;
     {
       ExpressionClassifier pattern_classifier;
       Token::Value next = peek();
       PreParserExpression pattern =
           ParsePrimaryExpression(&pattern_classifier, CHECK_OK);
+      is_pattern = pattern.IsObjectLiteral();  // (or pattern.IsArrayLiteral());

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

I'd rather you just add the bit for ArrayLiteral to PreParserExpression and make
this an actual || in the code.

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

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

+
       ValidateBindingPattern(&pattern_classifier, CHECK_OK);
       if (lexical) {
         ValidateLetPattern(&pattern_classifier, CHECK_OK);
@@ -596,12 +600,25 @@ PreParser::Statement PreParser::ParseVariableDeclarations(
       }
     }
 
+    bool is_for_iteration_variable =
+        var_context == kForStatement &&
+        (peek() == Token::IN || PeekContextualKeyword(CStrVector("of")));
+
     Scanner::Location variable_loc = scanner()->location();
     nvars++;
-    if (peek() == Token::ASSIGN || require_initializer ||
+    if (peek() == Token::ASSIGN ||
+        (!is_for_iteration_variable && (require_initializer || is_pattern)) ||

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

Maybe put this in the same order as in the parser to make it easier to read?

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

Same comment as in the parser: move this to else branch.

         // require initializers for multiple consts.
         (is_strict_const && peek() == Token::COMMA)) {

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

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.

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

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

-      Expect(Token::ASSIGN, CHECK_OK);
+      if (peek() != Token::ASSIGN) {

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

Same note here as in the parser, you can use if (!Check(...))

+        PreParserTraits::ReportMessageAt(
+            Scanner::Location(decl_pos, scanner()->location().end_pos),
+            MessageTemplate::kDeclarationMissingInitializer,
+            is_pattern ? "destructuring" : "const");
+        *ok = false;
+        return Statement::Default();
+      }
+      Consume(Token::ASSIGN);
       ExpressionClassifier classifier;
       ParseAssignmentExpression(var_context != kForStatement, &classifier,
                                 CHECK_OK);