[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

Review URL: https://codereview.chromium.org/1062263002

Cr-Commit-Position: refs/heads/master@{#27677}

src/messages.js

@@ -199,7 +199,8 @@ var kMessages = {
   constructor_noncallable:       ["Class constructors cannot be invoked without 'new'"],
   array_not_subclassable:        ["Subclassing Arrays is not currently supported."],
   for_in_loop_initializer:       ["for-in loop variable declaration may not have an initializer."],
-  for_of_loop_initializer:       ["for-of loop variable declaration may not have an initializer."]
+  for_of_loop_initializer:       ["for-of loop variable declaration may not have an initializer."],
+  for_inof_loop_multi_bindings:  ["Invalid left-hand side in ", "%0", " loop: Must have a single binding."]
 };
 
 

src/parser.cc

@@ -2217,8 +2217,8 @@ Block* Parser::ParseVariableStatement(VariableDeclarationContext var_context,
   //   VariableDeclarations ';'
 
   const AstRawString* ignore;
-  Block* result =
-      ParseVariableDeclarations(var_context, names, &ignore, nullptr, CHECK_OK);
+  Block* result = ParseVariableDeclarations(
+      var_context, nullptr, names, &ignore, nullptr, nullptr, CHECK_OK);
   ExpectSemicolon(CHECK_OK);
   return result;
 }
@@ -2230,9 +2230,10 @@ Block* Parser::ParseVariableStatement(VariableDeclarationContext var_context,
 // to initialize it properly. This mechanism is used for the parsing
 // of 'for-in' loops.
 Block* Parser::ParseVariableDeclarations(
-    VariableDeclarationContext var_context,
+    VariableDeclarationContext var_context, int* num_decl,
     ZoneList<const AstRawString*>* names, const AstRawString** out,
-    Scanner::Location* first_initializer_loc, bool* ok) {
+    Scanner::Location* first_initializer_loc, Scanner::Location* bindings_loc,
+    bool* ok) {
   // VariableDeclarations ::
   //   ('var' | 'const' | 'let') (Identifier ('=' AssignmentExpression)?)+[',']
   //
@@ -2304,7 +2305,9 @@ Block* Parser::ParseVariableDeclarations(
   // Create new block with one expected declaration.
   Block* block = factory()->NewBlock(NULL, 1, true, pos);
   int nvars = 0;  // the number of variables declared
+  int bindings_start = peek_position();
   const AstRawString* name = NULL;
+  const AstRawString* first_name = NULL;
   bool is_for_iteration_variable;
   do {
     if (fni_ != NULL) fni_->Enter();
@@ -2312,6 +2315,7 @@ Block* Parser::ParseVariableDeclarations(
     // Parse variable name.
     if (nvars > 0) Consume(Token::COMMA);
     name = ParseIdentifier(kDontAllowEvalOrArguments, CHECK_OK);
+    if (!first_name) first_name = name;
     Scanner::Location variable_loc = scanner()->location();
     if (fni_ != NULL) fni_->PushVariableName(name);
 
@@ -2519,12 +2523,14 @@ Block* Parser::ParseVariableDeclarations(
     if (fni_ != NULL) fni_->Leave();
   } while (peek() == Token::COMMA);
 
-  // If there was a single non-const declaration, return it in the output
-  // parameter for possible use by for/in.
-  if (nvars == 1 && (!is_const || is_for_iteration_variable)) {
-    *out = name;
+  if (bindings_loc) {
+    *bindings_loc =
+        Scanner::Location(bindings_start, scanner()->location().end_pos);
   }
 
+  if (num_decl) *num_decl = nvars;
+  *out = first_name;
+
   return block;
 }
 
@@ -3368,15 +3374,27 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
         (peek() == Token::CONST && is_sloppy(language_mode()))) {
       const AstRawString* name = NULL;
       Scanner::Location first_initializer_loc = Scanner::Location::invalid();
+      Scanner::Location bindings_loc = Scanner::Location::invalid();
+      int num_decl;
       Block* variable_statement = ParseVariableDeclarations(
-          kForStatement, nullptr, &name, &first_initializer_loc, CHECK_OK);
+          kForStatement, &num_decl, nullptr, &name, &first_initializer_loc,
+          &bindings_loc, CHECK_OK);
+      bool accept_IN = num_decl >= 1;
       bool accept_OF = true;
       ForEachStatement::VisitMode mode;
       int each_beg_pos = scanner()->location().beg_pos;
       int each_end_pos = scanner()->location().end_pos;
 
-      if (name != NULL && CheckInOrOf(accept_OF, &mode, ok)) {
+      if (accept_IN && CheckInOrOf(accept_OF, &mode, ok)) {
         if (!*ok) return nullptr;
+        if (num_decl != 1) {
+          const char* loop_type =
+              mode == ForEachStatement::ITERATE ? "for-of" : "for-in";
+          ParserTraits::ReportMessageAt(
+              bindings_loc, "for_inof_loop_multi_bindings", loop_type);
+          *ok = false;
+          return nullptr;
+        }
         if (first_initializer_loc.IsValid() &&
             (is_strict(language_mode()) || mode == ForEachStatement::ITERATE)) {
           if (mode == ForEachStatement::ITERATE) {
@@ -3417,10 +3435,12 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
       is_const = peek() == Token::CONST;
       const AstRawString* name = NULL;
       Scanner::Location first_initializer_loc = Scanner::Location::invalid();
-      Block* variable_statement =
-          ParseVariableDeclarations(kForStatement, &lexical_bindings, &name,
-                                    &first_initializer_loc, CHECK_OK);
-      bool accept_IN = name != NULL;
+      Scanner::Location bindings_loc = Scanner::Location::invalid();
+      int num_decl;
+      Block* variable_statement = ParseVariableDeclarations(
+          kForStatement, &num_decl, &lexical_bindings, &name,
+          &first_initializer_loc, &bindings_loc, CHECK_OK);
+      bool accept_IN = num_decl >= 1;
       bool accept_OF = true;
       ForEachStatement::VisitMode mode;
       int each_beg_pos = scanner()->location().beg_pos;
@@ -3428,6 +3448,14 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
 
       if (accept_IN && CheckInOrOf(accept_OF, &mode, ok)) {
         if (!*ok) return nullptr;
+        if (num_decl != 1) {
+          const char* loop_type =
+              mode == ForEachStatement::ITERATE ? "for-of" : "for-in";
+          ParserTraits::ReportMessageAt(
+              bindings_loc, "for_inof_loop_multi_bindings", loop_type);
+          *ok = false;
+          return nullptr;
+        }
         if (first_initializer_loc.IsValid() &&
             (is_strict(language_mode()) || mode == ForEachStatement::ITERATE)) {
           if (mode == ForEachStatement::ITERATE) {

src/parser.h

@@ -909,10 +909,11 @@ class Parser : public ParserBase<ParserTraits> {
                                 ZoneList<const AstRawString*>* names,
                                 bool* ok);
   Block* ParseVariableDeclarations(VariableDeclarationContext var_context,
+                                   int* num_decl,
                                    ZoneList<const AstRawString*>* names,
                                    const AstRawString** out,
                                    Scanner::Location* first_initializer_loc,
-                                   bool* ok);
+                                   Scanner::Location* bindings_loc, bool* ok);
   Statement* ParseExpressionOrLabelledStatement(
       ZoneList<const AstRawString*>* labels, bool* ok);
   IfStatement* ParseIfStatement(ZoneList<const AstRawString*>* labels,

src/preparser.cc

@@ -410,8 +410,8 @@ PreParser::Statement PreParser::ParseVariableStatement(
   // VariableStatement ::
   //   VariableDeclarations ';'
 
-  Statement result =
-      ParseVariableDeclarations(var_context, nullptr, nullptr, CHECK_OK);
+  Statement result = ParseVariableDeclarations(var_context, nullptr, nullptr,
+                                               nullptr, CHECK_OK);
   ExpectSemicolon(CHECK_OK);
   return result;
 }
@@ -424,7 +424,8 @@ PreParser::Statement PreParser::ParseVariableStatement(
 // of 'for-in' loops.
 PreParser::Statement PreParser::ParseVariableDeclarations(
     VariableDeclarationContext var_context, int* num_decl,
-    Scanner::Location* first_initializer_loc, bool* ok) {
+    Scanner::Location* first_initializer_loc, Scanner::Location* bindings_loc,
+    bool* ok) {
   // VariableDeclarations ::
   //   ('var' | 'const') (Identifier ('=' AssignmentExpression)?)+[',']
   //
@@ -478,6 +479,7 @@ PreParser::Statement PreParser::ParseVariableDeclarations(
   // of a let declared variable is the scope of the immediately enclosing
   // block.
   int nvars = 0;  // the number of variables declared
+  int bindings_start = peek_position();
   do {
     // Parse variable name.
     if (nvars > 0) Consume(Token::COMMA);
@@ -497,6 +499,11 @@ PreParser::Statement PreParser::ParseVariableDeclarations(
     }
   } while (peek() == Token::COMMA);
 
+  if (bindings_loc) {
+    *bindings_loc =
+        Scanner::Location(bindings_start, scanner()->location().end_pos);
+  }
+
   if (num_decl != NULL) *num_decl = nvars;
   return Statement::Default();
 }
@@ -732,17 +739,24 @@ PreParser::Statement PreParser::ParseForStatement(bool* ok) {
     ForEachStatement::VisitMode mode;
     if (peek() == Token::VAR || peek() == Token::CONST ||
         (peek() == Token::LET && is_strict(language_mode()))) {
-      bool is_lexical = peek() == Token::LET ||
-                        (peek() == Token::CONST && is_strict(language_mode()));
       int decl_count;
       Scanner::Location first_initializer_loc = Scanner::Location::invalid();
+      Scanner::Location bindings_loc = Scanner::Location::invalid();
       ParseVariableDeclarations(kForStatement, &decl_count,
-                                &first_initializer_loc, CHECK_OK);
-      bool has_initializers = first_initializer_loc.IsValid();
-      bool accept_IN = decl_count == 1 && !(is_lexical && has_initializers);
+                                &first_initializer_loc, &bindings_loc,
+                                CHECK_OK);
+      bool accept_IN = decl_count >= 1;
       bool accept_OF = true;
       if (accept_IN && CheckInOrOf(accept_OF, &mode, ok)) {
         if (!*ok) return Statement::Default();
+        if (decl_count != 1) {
+          const char* loop_type =
+              mode == ForEachStatement::ITERATE ? "for-of" : "for-in";
+          PreParserTraits::ReportMessageAt(
+              bindings_loc, "for_inof_loop_multi_bindings", loop_type);
+          *ok = false;
+          return Statement::Default();
+        }
         if (first_initializer_loc.IsValid() &&
             (is_strict(language_mode()) || mode == ForEachStatement::ITERATE)) {
           if (mode == ForEachStatement::ITERATE) {

src/preparser.h

@@ -1587,6 +1587,7 @@ class PreParser : public ParserBase<PreParserTraits> {
   Statement ParseVariableDeclarations(VariableDeclarationContext var_context,
                                       int* num_decl,
                                       Scanner::Location* first_initializer_loc,
+                                      Scanner::Location* bindings_loc,
                                       bool* ok);
   Statement ParseExpressionOrLabelledStatement(bool* ok);
   Statement ParseIfStatement(bool* ok);

test/cctest/test-parsing.cc

@@ -4628,6 +4628,159 @@ TEST(ConstParsingInForInError) {
 }
 
 
+TEST(InitializedDeclarationsInStrictForInError) {
+  const char* context_data[][2] = {{"'use strict';", ""},
+                                   {"function foo(){ 'use strict';", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {
+      "for (var i = 1 in {}) {}",
+      "for (var i = void 0 in [1, 2, 3]) {}",
+      "for (let i = 1 in {}) {}",
+      "for (let i = void 0 in [1, 2, 3]) {}",
+      "for (const i = 1 in {}) {}",
+      "for (const i = void 0 in [1, 2, 3]) {}",
+      NULL};
+  RunParserSyncTest(context_data, data, kError);
+}
+
+
+TEST(InitializedDeclarationsInStrictForOfError) {
+  const char* context_data[][2] = {{"'use strict';", ""},
+                                   {"function foo(){ 'use strict';", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {
+      "for (var i = 1 of {}) {}",
+      "for (var i = void 0 of [1, 2, 3]) {}",
+      "for (let i = 1 of {}) {}",
+      "for (let i = void 0 of [1, 2, 3]) {}",
+      "for (const i = 1 of {}) {}",
+      "for (const i = void 0 of [1, 2, 3]) {}",
+      NULL};
+  RunParserSyncTest(context_data, data, kError);
+}
+
+
+TEST(InitializedDeclarationsInSloppyForInError) {
+  const char* context_data[][2] = {{"", ""},
+                                   {"function foo(){", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {
+      "for (var i = 1 in {}) {}",
+      "for (var i = void 0 in [1, 2, 3]) {}",
+      NULL};
+  // TODO(caitp): This should be an error in sloppy mode.
+  RunParserSyncTest(context_data, data, kSuccess);
+}
+
+
+TEST(InitializedDeclarationsInSloppyForOfError) {
+  const char* context_data[][2] = {{"", ""},
+                                   {"function foo(){", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {
+      "for (var i = 1 of {}) {}",
+      "for (var i = void 0 of [1, 2, 3]) {}",
+      NULL};
+  RunParserSyncTest(context_data, data, kError);
+}
+
+
+TEST(ForInMultipleDeclarationsError) {
+  const char* context_data[][2] = {{"", ""},
+                                   {"function foo(){", "}"},
+                                   {"'use strict';", ""},
+                                   {"function foo(){ 'use strict';", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {
+      "for (var i, j in {}) {}",
+      "for (var i, j in [1, 2, 3]) {}",
+      "for (var i, j = 1 in {}) {}",
+      "for (var i, j = void 0 in [1, 2, 3]) {}",
+
+      "for (let i, j in {}) {}",
+      "for (let i, j in [1, 2, 3]) {}",
+      "for (let i, j = 1 in {}) {}",
+      "for (let i, j = void 0 in [1, 2, 3]) {}",
+
+      "for (const i, j in {}) {}",
+      "for (const i, j in [1, 2, 3]) {}",
+      "for (const i, j = 1 in {}) {}",
+      "for (const i, j = void 0 in [1, 2, 3]) {}",
+      NULL};
+  static const ParserFlag always_flags[] = {kAllowHarmonySloppy};
+  RunParserSyncTest(context_data, data, kError, nullptr, 0, always_flags,
+                    arraysize(always_flags));
+}
+
+
+TEST(ForOfMultipleDeclarationsError) {
+  const char* context_data[][2] = {{"", ""},
+                                   {"function foo(){", "}"},
+                                   {"'use strict';", ""},
+                                   {"function foo(){ 'use strict';", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {
+      "for (var i, j of {}) {}",
+      "for (var i, j of [1, 2, 3]) {}",
+      "for (var i, j = 1 of {}) {}",
+      "for (var i, j = void 0 of [1, 2, 3]) {}",
+
+      "for (let i, j of {}) {}",
+      "for (let i, j of [1, 2, 3]) {}",
+      "for (let i, j = 1 of {}) {}",
+      "for (let i, j = void 0 of [1, 2, 3]) {}",
+
+      "for (const i, j of {}) {}",
+      "for (const i, j of [1, 2, 3]) {}",
+      "for (const i, j = 1 of {}) {}",
+      "for (const i, j = void 0 of [1, 2, 3]) {}",
+      NULL};
+  static const ParserFlag always_flags[] = {kAllowHarmonySloppy};
+  RunParserSyncTest(context_data, data, kError, nullptr, 0, always_flags,
+                    arraysize(always_flags));
+}
+
+
+TEST(ForInNoDeclarationsError) {
+  const char* context_data[][2] = {{"", ""},
+                                   {"function foo(){", "}"},
+                                   {"'use strict';", ""},
+                                   {"function foo(){ 'use strict';", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {
+      "for (var in {}) {}",
+      "for (const in {}) {}",
+      NULL};
+  static const ParserFlag always_flags[] = {kAllowHarmonySloppy};
+  RunParserSyncTest(context_data, data, kError, nullptr, 0, always_flags,
+                    arraysize(always_flags));
+}
+
+
+TEST(ForOfNoDeclarationsError) {
+  const char* context_data[][2] = {{"", ""},
+                                   {"function foo(){", "}"},
+                                   {"'use strict';", ""},
+                                   {"function foo(){ 'use strict';", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {
+      "for (var of [1, 2, 3]) {}",
+      "for (const of [1, 2, 3]) {}",
+      NULL};
+  static const ParserFlag always_flags[] = {kAllowHarmonySloppy};
+  RunParserSyncTest(context_data, data, kError, nullptr, 0, always_flags,
+                    arraysize(always_flags));
+}
+
+
 TEST(InvalidUnicodeEscapes) {
   const char* context_data[][2] = {{"", ""},
                                    {"'use strict';", ""},