[parser] early error when declaration Pattern missing Initializer

src/parser.cc

Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index b54172f72c21cc6ccf320ed04877227804a5d134..e8bb657ffd1cab3fa4c12c52ece503830f91d6af 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -2531,6 +2531,8 @@ void Parser::ParseVariableDeclarations(VariableDeclarationContext var_context,
     if (!first_declaration) Consume(Token::COMMA);
 
     Expression* pattern;
+    bool is_pattern = false;

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

Nit: declare this after the block and initialise directly.

+    int decl_pos = peek_position();
     {
       ExpressionClassifier pattern_classifier;
       Token::Value next = peek();
@@ -2547,6 +2549,7 @@ void Parser::ParseVariableDeclarations(VariableDeclarationContext var_context,
         *ok = false;
         return;
       }
+      is_pattern = pattern->IsObjectLiteral() || pattern->IsArrayLiteral();
     }
 
     Scanner::Location variable_loc = scanner()->location();
@@ -2569,10 +2572,18 @@ void Parser::ParseVariableDeclarations(VariableDeclarationContext var_context,
     Expression* value = NULL;
     // Harmony consts have non-optional initializers.
     int initializer_position = RelocInfo::kNoPosition;
-    if (peek() == Token::ASSIGN || (parsing_result->descriptor.mode == CONST &&
-                                    !is_for_iteration_variable)) {
-      Expect(Token::ASSIGN, ok);
-      if (!*ok) return;
+    if (peek() == Token::ASSIGN ||
+        ((parsing_result->descriptor.mode == CONST || is_pattern) &&

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

Why not test these in the else branch and move the error generation there?

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

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

+         !is_for_iteration_variable)) {
+      if (peek() != Token::ASSIGN) {

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

You can also consume here to avoid mentioning ASSIGN once more below:

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

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

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

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

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

+        ParserTraits::ReportMessageAt(
+            Scanner::Location(decl_pos, scanner()->location().end_pos),

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

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.

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

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]

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

+            MessageTemplate::kDeclarationMissingInitializer,
+            is_pattern ? "destructuring" : "const");
+        *ok = false;
+        return;
+      }
+      Consume(Token::ASSIGN);
       ExpressionClassifier classifier;
       value = ParseAssignmentExpression(var_context != kForStatement,
                                         &classifier, ok);