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

src/parser.cc

Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index 608a6bdaf0435aebb24edeac1bd9a21160a120f7..4412b8a98cdca5618ec0b7caf1a77caf246718b3 100644
--- a/src/parser.cc
+++ b/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;

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

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.

       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) {