[parser] Refactor of Parse*Statement*, part 8

src/parsing/parser-base.h

Index: src/parsing/parser-base.h
diff --git a/src/parsing/parser-base.h b/src/parsing/parser-base.h
index 3171e57144e09562b294fd26ff934481c981848d..b5801662a5d62253fc2144b0356297c9fe19f623 100644
--- a/src/parsing/parser-base.h
+++ b/src/parsing/parser-base.h
@@ -656,6 +656,21 @@ class ParserBase {
     TailCallExpressionList tail_call_expressions;
   };
 
+  struct ForInfo {
+   public:
+    explicit ForInfo(ParserBase* parser)
+        : bound_names(1, parser->zone()),
+          bound_names_are_lexical(false),
+          mode(ForEachStatement::ENUMERATE),
+          each_loc(),
+          parsing_result() {}
+    ZoneList<const AstRawString*> bound_names;
+    bool bound_names_are_lexical;
+    ForEachStatement::VisitMode mode;
+    Scanner::Location each_loc;
+    DeclarationParsingResult parsing_result;
+  };
+
   DeclarationScope* NewScriptScope() const {
     return new (zone()) DeclarationScope(zone(), ast_value_factory());
   }
@@ -1250,7 +1265,7 @@ class ParserBase {
   StatementT ParseSwitchStatement(ZoneList<const AstRawString*>* labels,
                                   bool* ok);
   StatementT ParseTryStatement(bool* ok);
-  Statement* ParseForStatement(ZoneList<const AstRawString*>* labels, bool* ok);
+  StatementT ParseForStatement(ZoneList<const AstRawString*>* labels, bool* ok);
 
   bool IsNextLetKeyword();
   bool IsTrivialExpression();
@@ -4350,7 +4365,7 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseStatement(
     case Token::WHILE:
       return ParseWhileStatement(labels, ok);
     case Token::FOR:
-      return impl()->ParseForStatement(labels, ok);
+      return ParseForStatement(labels, ok);
     case Token::CONTINUE:
     case Token::BREAK:
     case Token::RETURN:
@@ -4977,12 +4992,11 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseTryStatement(
                                      catch_info, pos);
 }
 
-Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
-                                     bool* ok) {
+template <typename Impl>
+typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseForStatement(
+    ZoneList<const AstRawString*>* labels, bool* ok) {
   int stmt_pos = peek_position();
-  Statement* init = NULL;
-  ZoneList<const AstRawString*> bound_names(1, zone());
-  bool bound_names_are_lexical = false;
+  ForInfo for_info(this);
 
   // Create an in-between scope for let-bound iteration variables.
   BlockState for_state(&scope_state_);
@@ -4990,190 +5004,96 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
   Expect(Token::LPAREN, CHECK_OK);
   for_state.set_start_position(scanner()->location().beg_pos);
   for_state.set_is_hidden();
-  DeclarationParsingResult parsing_result;
+
+  StatementT init = impl()->NullStatement();
   if (peek() != Token::SEMICOLON) {
+    // An initializer is present.
     if (peek() == Token::VAR || peek() == Token::CONST ||
         (peek() == Token::LET && IsNextLetKeyword())) {
-      ParseVariableDeclarations(kForStatement, &parsing_result, nullptr,
-                                CHECK_OK);
-
-      ForEachStatement::VisitMode mode = ForEachStatement::ENUMERATE;
-      int each_beg_pos = scanner()->location().beg_pos;
-      int each_end_pos = scanner()->location().end_pos;
-
-      if (CheckInOrOf(&mode)) {
-        if (parsing_result.declarations.length() != 1) {
-          ReportMessageAt(parsing_result.bindings_loc,
-                          MessageTemplate::kForInOfLoopMultiBindings,
-                          ForEachStatement::VisitModeString(mode));
+      // The initializer contains declarations.
+      ParseVariableDeclarations(kForStatement, &for_info.parsing_result,
+                                nullptr, CHECK_OK);
+      for_info.bound_names_are_lexical =

[marja, 2016/09/20 19:36:30]

bound_names_are_lexical is now redundant, right? Maybe we shouldn't have it (as
the information is available in for_info.parsing_result), so that there's no
fear that for_info.bound_names_are_lexical and
for_info.parsing_result.descriptor.mode disagree.

If it makes the code clearer, there can be a bound_names_are_lexical helper
variable wherever it's needed (this is quite far away from the use..)

[nickie, 2016/09/21 09:20:02]

Yes.  I'm removing it from ForInfo but I'm keeping it as a local variable here,
to avoid unnecessary calls to IsLexicalVariableMode.

+          IsLexicalVariableMode(for_info.parsing_result.descriptor.mode);
+      for_info.each_loc = scanner()->location();
+
+      if (CheckInOrOf(&for_info.mode)) {
+        // Just one declaration followed by in/of.
+        if (for_info.parsing_result.declarations.length() != 1) {
+          impl()->ReportMessageAt(
+              for_info.parsing_result.bindings_loc,
+              MessageTemplate::kForInOfLoopMultiBindings,
+              ForEachStatement::VisitModeString(for_info.mode));
           *ok = false;
-          return nullptr;
+          return impl()->NullStatement();
         }
-        DeclarationParsingResult::Declaration& decl =
-            parsing_result.declarations[0];
-        if (parsing_result.first_initializer_loc.IsValid() &&
-            (is_strict(language_mode()) || mode == ForEachStatement::ITERATE ||
-             IsLexicalVariableMode(parsing_result.descriptor.mode) ||
-             !decl.pattern->IsVariableProxy() || allow_harmony_for_in())) {
+        if (for_info.parsing_result.first_initializer_loc.IsValid() &&
+            (is_strict(language_mode()) ||
+             for_info.mode == ForEachStatement::ITERATE ||

[marja, 2016/09/21 08:23:21]

Random whining unrelated to this CL: Why do we need to have ITERATE and
ENUMERATE, so that everybody reading the code needs to remember that ITERATE is
"for of" and ENUMERATE is "for in". I have hard time remembering which is which.
IMO it would be clearer if we used the terms "ForIn" and "ForOf". In addition,
bugs where somebody accidentally uses the wrong one would look more obvious (now
they're quite hard to spot).

For example, this code would read: for_info.mode == ForEachStatement.FOR_IN ||
... || allow_harmony_for_in(), so it's clear that they belong together.


If you agree, maybe a follow-up CL?

[nickie, 2016/09/21 09:20:02]

I agree.  Also, it seems that this is used only locally in the parser.  There
are 4+9 occurrences of ENUMERATE+ITERATE.

+             IsLexicalVariableMode(for_info.parsing_result.descriptor.mode) ||
+             !impl()->IsIdentifier(
+                 for_info.parsing_result.declarations[0].pattern) ||
+             allow_harmony_for_in())) {
           // Only increment the use count if we would have let this through
           // without the flag.
           if (allow_harmony_for_in()) {
-            ++use_counts_[v8::Isolate::kForInInitializer];
+            impl()->CountUsage(v8::Isolate::kForInInitializer);
           }
-          ReportMessageAt(parsing_result.first_initializer_loc,
-                          MessageTemplate::kForInOfLoopInitializer,
-                          ForEachStatement::VisitModeString(mode));
+          impl()->ReportMessageAt(
+              for_info.parsing_result.first_initializer_loc,
+              MessageTemplate::kForInOfLoopInitializer,
+              ForEachStatement::VisitModeString(for_info.mode));
           *ok = false;
-          return nullptr;
+          return impl()->NullStatement();
         }
 
-        Block* init_block = nullptr;
-        bound_names_are_lexical =
-            IsLexicalVariableMode(parsing_result.descriptor.mode);
-
-        // special case for legacy for (var ... = ... in ...)
-        if (!bound_names_are_lexical && decl.pattern->IsVariableProxy() &&
-            decl.initializer != nullptr) {
-          DCHECK(!allow_harmony_for_in());
-          ++use_counts_[v8::Isolate::kForInInitializer];
-          const AstRawString* name =
-              decl.pattern->AsVariableProxy()->raw_name();
-          VariableProxy* single_var = NewUnresolved(name);
-          init_block = factory()->NewBlock(
-              nullptr, 2, true, parsing_result.descriptor.declaration_pos);
-          init_block->statements()->Add(
-              factory()->NewExpressionStatement(
-                  factory()->NewAssignment(Token::ASSIGN, single_var,
-                                           decl.initializer, kNoSourcePosition),
-                  kNoSourcePosition),
-              zone());
-        }
+        BlockT init_block = impl()->RewriteForVarInLegacy(&for_info);
+
+        auto loop =

[marja, 2016/09/20 19:36:30]

I was thinking about the use of "auto" here (and below)... it's not clear from
the context what type it is (some factory funcs, especially in the PreParser,
return generic types of Statements, some of them return specialized
Statements... so it's not possible to say just after reading this line what type
auto suppresses.

However, does the use of auto here allow us to get away with fewer typedefs in
the traits? (A stupid question.) Any other benefits I'm not seeing?

[nickie, 2016/09/21 09:20:02]

Yes, that's precisely what it does.  (BTW, it's used in the same way in other
places too, e.g., while and do-while loops.)  In the preparser, loop is a
PreParserStatement; in the parser, it's a ForEachStatement*.  We would need a
typedef such as Types::ForEachStatementT if we did not to use "auto" here.

[marja, 2016/09/21 10:31:42]

Ok, then it's fine.

+            factory()->NewForEachStatement(for_info.mode, labels, stmt_pos);
+        typename Types::Target target(this, loop);
+
+        int each_keyword_pos = scanner()->location().beg_pos;
 
-        // Rewrite a for-in/of statement of the form
-        //
-        //   for (let/const/var x in/of e) b
-        //
-        // into
-        //
-        //   {
-        //     <let x' be a temporary variable>
-        //     for (x' in/of e) {
-        //       let/const/var x;
-        //       x = x';
-        //       b;
-        //     }
-        //     let x;  // for TDZ
-        //   }
-
-        Variable* temp = NewTemporary(ast_value_factory()->dot_for_string());
-        ForEachStatement* loop =
-            factory()->NewForEachStatement(mode, labels, stmt_pos);
-        ParserTarget target(this, loop);
-
-        int each_keyword_position = scanner()->location().beg_pos;
-
-        Expression* enumerable;
-        if (mode == ForEachStatement::ITERATE) {
+        ExpressionT enumerable = impl()->EmptyExpression();
+        if (for_info.mode == ForEachStatement::ITERATE) {
           ExpressionClassifier classifier(this);
           enumerable = ParseAssignmentExpression(true, CHECK_OK);
-          RewriteNonPattern(CHECK_OK);
+          impl()->RewriteNonPattern(CHECK_OK);
         } else {
           enumerable = ParseExpression(true, CHECK_OK);
         }
 
         Expect(Token::RPAREN, CHECK_OK);
 
-
-        Block* body_block =
-            factory()->NewBlock(NULL, 3, false, kNoSourcePosition);
-
-        Statement* final_loop;
+        StatementT final_loop = impl()->NullStatement();
         {
           ReturnExprScope no_tail_calls(function_state_,
                                         ReturnExprContext::kInsideForInOfBody);
           BlockState block_state(&scope_state_);
           block_state.set_start_position(scanner()->location().beg_pos);
 
-          Statement* body = ParseScopedStatement(NULL, true, CHECK_OK);
-
-          auto each_initialization_block =
-              factory()->NewBlock(nullptr, 1, true, kNoSourcePosition);
-          {
-            auto descriptor = parsing_result.descriptor;
-            descriptor.declaration_pos = kNoSourcePosition;
-            descriptor.initialization_pos = kNoSourcePosition;
-            decl.initializer = factory()->NewVariableProxy(temp);
-
-            bool is_for_var_of =
-                mode == ForEachStatement::ITERATE &&
-                parsing_result.descriptor.mode == VariableMode::VAR;
-
-            PatternRewriter::DeclareAndInitializeVariables(
-                this, each_initialization_block, &descriptor, &decl,
-                bound_names_are_lexical || is_for_var_of ? &bound_names
-                                                         : nullptr,
-                CHECK_OK);
-
-            // Annex B.3.5 prohibits the form
-            // `try {} catch(e) { for (var e of {}); }`
-            // So if we are parsing a statement like `for (var ... of ...)`
-            // we need to walk up the scope chain and look for catch scopes
-            // which have a simple binding, then compare their binding against
-            // all of the names declared in the init of the for-of we're
-            // parsing.
-            if (is_for_var_of) {
-              Scope* catch_scope = scope();
-              while (catch_scope != nullptr &&
-                     !catch_scope->is_declaration_scope()) {
-                if (catch_scope->is_catch_scope()) {
-                  auto name = catch_scope->catch_variable_name();
-                  if (name !=
-                      ast_value_factory()
-                          ->dot_catch_string()) {  // i.e. is a simple binding
-                    if (bound_names.Contains(name)) {
-                      ReportMessageAt(parsing_result.bindings_loc,
-                                      MessageTemplate::kVarRedeclaration, name);
-                      *ok = false;
-                      return nullptr;
-                    }
-                  }
-                }
-                catch_scope = catch_scope->outer_scope();
-              }
-            }
-          }
+          StatementT body = ParseScopedStatement(nullptr, true, CHECK_OK);
+
+          std::pair<BlockT, ExpressionT> body_block_and_each_variable =
+              impl()->DesugarBindingInForEachStatement(&for_info, CHECK_OK);
+          body_block_and_each_variable.first->statements()->Add(body, zone());

[adamk, 2016/09/20 16:34:31]

Please create a variable for body_block, it'll make the rest of this code a
little more readable.

[marja, 2016/09/21 08:23:21]

Why do we need the pair anyway, it looks like we're always using either .first
or .second?

[nickie, 2016/09/21 09:20:02]

After offline discussion, I'm removing the pair and introducing two out
parameters to DesugarBindingInForEachStatement.

[nickie, 2016/09/21 09:20:02]

See comment below.

+          final_loop = impl()->InitializeForEachStatement(
+              loop, body_block_and_each_variable.second, enumerable,
+              body_block_and_each_variable.first, each_keyword_pos);
 
-          body_block->statements()->Add(each_initialization_block, zone());
-          body_block->statements()->Add(body, zone());
-          VariableProxy* temp_proxy =
-              factory()->NewVariableProxy(temp, each_beg_pos, each_end_pos);
-          final_loop = InitializeForEachStatement(
-              loop, temp_proxy, enumerable, body_block, each_keyword_position);
           block_state.set_end_position(scanner()->location().end_pos);
-          body_block->set_scope(block_state.FinalizedBlockScope());
+          body_block_and_each_variable.first->set_scope(
+              block_state.FinalizedBlockScope());
         }
 
-        // Create a TDZ for any lexically-bound names.
-        if (bound_names_are_lexical) {
-          DCHECK_NULL(init_block);
-
-          init_block =
-              factory()->NewBlock(nullptr, 1, false, kNoSourcePosition);
-
-          for (int i = 0; i < bound_names.length(); ++i) {
-            // TODO(adamk): This needs to be some sort of special
-            // INTERNAL variable that's invisible to the debugger
-            // but visible to everything else.
-            Declaration* tdz_decl = DeclareVariable(
-                bound_names[i], LET, kNoSourcePosition, CHECK_OK);
-            tdz_decl->proxy()->var()->set_initializer_position(position());
-          }
-        }
+        init_block =
+            impl()->CreateForEachStatementTDZ(init_block, &for_info, ok);
 
         for_state.set_end_position(scanner()->location().end_pos);
         Scope* for_scope = for_state.FinalizedBlockScope();
         // Parsed for-in loop w/ variable declarations.
-        if (init_block != nullptr) {
+        if (!impl()->IsNullStatement(init_block)) {
           init_block->statements()->Add(final_loop, zone());
           init_block->set_scope(for_scope);
           return init_block;
@@ -5182,47 +5102,48 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
           return final_loop;
         }
       } else {
-        bound_names_are_lexical =
-            IsLexicalVariableMode(parsing_result.descriptor.mode);
-        init = BuildInitializationBlock(
-            &parsing_result, bound_names_are_lexical ? &bound_names : nullptr,
+        // One or more declaration not followed by in/of.
+        init = impl()->BuildInitializationBlock(
+            &for_info.parsing_result,
+            for_info.bound_names_are_lexical ? &for_info.bound_names : nullptr,
             CHECK_OK);
       }
     } else {
+      // The initializer does not contain declarations.
       int lhs_beg_pos = peek_position();
       ExpressionClassifier classifier(this);
-      Expression* expression = ParseExpressionCoverGrammar(false, CHECK_OK);
+      ExpressionT expression = ParseExpressionCoverGrammar(false, CHECK_OK);
       int lhs_end_pos = scanner()->location().end_pos;
-      ForEachStatement::VisitMode mode = ForEachStatement::ENUMERATE;
 
-      bool is_for_each = CheckInOrOf(&mode);
+      bool is_for_each = CheckInOrOf(&for_info.mode);
       bool is_destructuring = is_for_each && (expression->IsArrayLiteral() ||
                                               expression->IsObjectLiteral());
 
       if (is_destructuring) {
         ValidateAssignmentPattern(CHECK_OK);
       } else {
-        RewriteNonPattern(CHECK_OK);
+        impl()->RewriteNonPattern(CHECK_OK);
       }
 
       if (is_for_each) {
+        // Initializer is reference followed by in/of.
         if (!is_destructuring) {
-          expression = CheckAndRewriteReferenceExpression(
+          expression = impl()->CheckAndRewriteReferenceExpression(
               expression, lhs_beg_pos, lhs_end_pos,
               MessageTemplate::kInvalidLhsInFor, kSyntaxError, CHECK_OK);
         }
 
-        ForEachStatement* loop =
-            factory()->NewForEachStatement(mode, labels, stmt_pos);
-        ParserTarget target(this, loop);
+        auto loop =
+            factory()->NewForEachStatement(for_info.mode, labels, stmt_pos);
+        typename Types::Target target(this, loop);
 
-        int each_keyword_position = scanner()->location().beg_pos;
+        int each_keyword_pos = scanner()->location().beg_pos;
 
-        Expression* enumerable;
-        if (mode == ForEachStatement::ITERATE) {
+        ExpressionT enumerable = impl()->EmptyExpression();
+        if (for_info.mode == ForEachStatement::ITERATE) {
           ExpressionClassifier classifier(this);
           enumerable = ParseAssignmentExpression(true, CHECK_OK);
-          RewriteNonPattern(CHECK_OK);
+          impl()->RewriteNonPattern(CHECK_OK);
         } else {
           enumerable = ParseExpression(true, CHECK_OK);
         }
@@ -5237,10 +5158,10 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
 
           // For legacy compat reasons, give for loops similar treatment to
           // if statements in allowing a function declaration for a body
-          Statement* body = ParseScopedStatement(NULL, true, CHECK_OK);
+          StatementT body = ParseScopedStatement(nullptr, true, CHECK_OK);
           block_state.set_end_position(scanner()->location().end_pos);
-          Statement* final_loop = InitializeForEachStatement(
-              loop, expression, enumerable, body, each_keyword_position);
+          StatementT final_loop = impl()->InitializeForEachStatement(
+              loop, expression, enumerable, body, each_keyword_pos);
 
           Scope* for_scope = for_state.FinalizedBlockScope();
           DCHECK_NULL(for_scope);
@@ -5251,27 +5172,27 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
           return final_loop;
         }
       } else {
+        // Initializer is just an expression.
         init = factory()->NewExpressionStatement(expression, lhs_beg_pos);
       }
     }
   }
 
-  // Standard 'for' loop
-  ForStatement* loop = factory()->NewForStatement(labels, stmt_pos);
-  ParserTarget target(this, loop);
+  // Standard 'for' loop, we have parsed the initializer at this point.
+  auto loop = factory()->NewForStatement(labels, stmt_pos);
+  typename Types::Target target(this, loop);
 
-  // Parsed initializer at this point.
   Expect(Token::SEMICOLON, CHECK_OK);
 
-  Expression* cond = NULL;
-  Statement* next = NULL;
-  Statement* body = NULL;
+  ExpressionT cond = impl()->EmptyExpression();
+  StatementT next = impl()->NullStatement();
+  StatementT body = impl()->NullStatement();
 
   // If there are let bindings, then condition and the next statement of the
   // for loop must be parsed in a new scope.
   Scope* inner_scope = scope();
   // TODO(verwaest): Allocate this through a ScopeState as well.
-  if (bound_names_are_lexical && bound_names.length() > 0) {
+  if (for_info.bound_names_are_lexical && for_info.bound_names.length() > 0) {
     inner_scope = NewScopeWithParent(inner_scope, BLOCK_SCOPE);
     inner_scope->set_start_position(scanner()->location().beg_pos);
   }
@@ -5284,24 +5205,23 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
     Expect(Token::SEMICOLON, CHECK_OK);
 
     if (peek() != Token::RPAREN) {
-      Expression* exp = ParseExpression(true, CHECK_OK);
+      ExpressionT exp = ParseExpression(true, CHECK_OK);
       next = factory()->NewExpressionStatement(exp, exp->position());
     }
     Expect(Token::RPAREN, CHECK_OK);
 
-    body = ParseScopedStatement(NULL, true, CHECK_OK);
+    body = ParseScopedStatement(nullptr, true, CHECK_OK);
   }
 
-  Statement* result = NULL;
-  if (bound_names_are_lexical && bound_names.length() > 0) {
-    result = DesugarLexicalBindingsInForStatement(
-        inner_scope, parsing_result.descriptor.mode, &bound_names, loop, init,
-        cond, next, body, CHECK_OK);
+  if (for_info.bound_names_are_lexical && for_info.bound_names.length() > 0) {
+    auto result = impl()->DesugarLexicalBindingsInForStatement(
+        loop, init, cond, next, body, inner_scope, &for_info, CHECK_OK);
     for_state.set_end_position(scanner()->location().end_pos);
+    return result;
   } else {
     for_state.set_end_position(scanner()->location().end_pos);
     Scope* for_scope = for_state.FinalizedBlockScope();
-    if (for_scope) {
+    if (for_scope != nullptr) {
       // Rewrite a for statement of the form
       //   for (const x = i; c; n) b
       //
@@ -5320,20 +5240,19 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
       //   }
       // just in case b introduces a lexical binding some other way, e.g., if b
       // is a FunctionDeclaration.
-      Block* block = factory()->NewBlock(NULL, 2, false, kNoSourcePosition);
-      if (init != nullptr) {
+      BlockT block = factory()->NewBlock(NULL, 2, false, kNoSourcePosition);
+      if (!impl()->IsNullStatement(init)) {
         block->statements()->Add(init, zone());
       }
       block->statements()->Add(loop, zone());
       block->set_scope(for_scope);
-      loop->Initialize(NULL, cond, next, body);
-      result = block;
+      loop->Initialize(init, cond, next, body);
+      return block;
     } else {
       loop->Initialize(init, cond, next, body);
-      result = loop;
+      return loop;
     }
   }
-  return result;
 }
 
 #undef CHECK_OK