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

src/parser.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/v8.h"
 
 #include "src/api.h"
 #include "src/ast.h"
 #include "src/bailout-reason.h"
 #include "src/base/platform/platform.h"
@@ skipping 2199 matching lines @@
 }
 
 
 Block* Parser::ParseVariableStatement(VariableDeclarationContext var_context,
                                       ZoneList<const AstRawString*>* names,
                                       bool* ok) {
   // VariableStatement ::
   //   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;
 }
 
 
 // If the variable declaration declares exactly one non-const
 // variable, then *out is set to that variable. In all other cases,
 // *out is untouched; in particular, it is the caller's responsibility
 // 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)?)+[',']
   //
   // The ES6 Draft Rev3 specifies the following grammar for const declarations
   //
   // ConstDeclaration ::
   //   const ConstBinding (',' ConstBinding)* ';'
   // ConstBinding ::
   //   Identifier '=' AssignmentExpression
   //
@@ skipping 51 matching lines @@
   //
   // We mark the block as initializer block because we don't want the
   // rewriter to add a '.result' assignment to such a block (to get compliant
   // behavior for code such as print(eval('var x = 7')), and for cosmetic
   // reasons when pretty-printing. Also, unless an assignment (initialization)
   // is inside an initializer block, it is ignored.
   //
   // 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();
 
     // 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);
 
     // Declare variable.
     // Note that we *always* must treat the initial value via a separate init
     // assignment for variables and constants because the value must be assigned
     // when the variable is encountered in the source. But the variable/constant
     // is declared (and set to 'undefined') upon entering the function within
     // which the variable or constant is declared. Only function variables have
     // an initial value in the declaration (because they are initialized upon
@@ skipping 187 matching lines @@
       Assignment* assignment =
           factory()->NewAssignment(init_op, proxy, value, pos);
       block->AddStatement(
           factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition),
           zone());
     }
 
     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;
 }
 
 
 static bool ContainsLabel(ZoneList<const AstRawString*>* labels,
                           const AstRawString* label) {
   DCHECK(label != NULL);
   if (labels != NULL) {
     for (int i = labels->length(); i-- > 0; ) {
       if (labels->at(i) == label) {
@@ skipping 823 matching lines @@
 
   Expect(Token::FOR, CHECK_OK);
   Expect(Token::LPAREN, CHECK_OK);
   for_scope->set_start_position(scanner()->location().beg_pos);
   bool is_let_identifier_expression = false;
   if (peek() != Token::SEMICOLON) {
     if (peek() == Token::VAR ||
         (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) {
             ReportMessageAt(first_initializer_loc, "for_of_loop_initializer");
           } else {
             // TODO(caitp): This should be an error in sloppy mode too.
             ReportMessageAt(first_initializer_loc, "for_in_loop_initializer");
           }
           *ok = false;
           return nullptr;
@@ skipping 20 matching lines @@
         // Parsed for-in loop w/ variable/const declaration.
         return result;
       } else {
         init = variable_statement;
       }
     } else if ((peek() == Token::LET || peek() == Token::CONST) &&
                is_strict(language_mode())) {
       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;
       int each_end_pos = scanner()->location().end_pos;
 
       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) {
             ReportMessageAt(first_initializer_loc, "for_of_loop_initializer");
           } else {
             ReportMessageAt(first_initializer_loc, "for_in_loop_initializer");
           }
           *ok = false;
           return nullptr;
         }
@@ skipping 2139 matching lines @@
     } else {
       const uc16* data = reinterpret_cast<const uc16*>(raw_string->raw_data());
       running_hash = StringHasher::ComputeRunningHash(running_hash, data,
                                                       raw_string->length());
     }
   }
 
   return running_hash;
 }
 } }  // namespace v8::internal