Always finalize blocks after parsing, also for do-expressions

Rather than finalizing after rewriting do-expressions, we rewrite in the
outer scope if the block scope was finalized. Rewriting do expressions
cannot introduce any new nodes that requires the block to stay around,
so finalizing before and after is equivalent. (Only a temporary is
introduced which always ends up in a ClosureScope)

BUG=v8:5209
R=rossberg@chromium.org, caitpotter88@gmail.com, adamk@chromium.org

Review-Url: https://codereview.chromium.org/2167713004
Cr-Commit-Position: refs/heads/master@{#38193}

src/parsing/parser.cc

@@ -2291,9 +2291,7 @@ Statement* Parser::ParseClassDeclaration(ZoneList<const AstRawString*>* names,
   return assignment_statement;
 }
 
-
-Block* Parser::ParseBlock(ZoneList<const AstRawString*>* labels,
-                          bool finalize_block_scope, bool* ok) {
+Block* Parser::ParseBlock(ZoneList<const AstRawString*>* labels, bool* ok) {
   // The harmony mode uses block elements instead of statements.
   //
   // Block ::
@@ -2319,19 +2317,12 @@ Block* Parser::ParseBlock(ZoneList<const AstRawString*>* labels,
   }
   Expect(Token::RBRACE, CHECK_OK);
   block_scope->set_end_position(scanner()->location().end_pos);
-  if (finalize_block_scope) {
-    block_scope = block_scope->FinalizeBlockScope();
-  }
+  block_scope = block_scope->FinalizeBlockScope();
   body->set_scope(block_scope);
   return body;
 }
 
 
-Block* Parser::ParseBlock(ZoneList<const AstRawString*>* labels, bool* ok) {
-  return ParseBlock(labels, true, ok);
-}
-
-
 Block* Parser::DeclarationParsingResult::BuildInitializationBlock(
     ZoneList<const AstRawString*>* names, bool* ok) {
   Block* result = descriptor.parser->factory()->NewBlock(
@@ -4221,13 +4212,13 @@ DoExpression* Parser::ParseDoExpression(bool* ok) {
   Expect(Token::DO, CHECK_OK);
   Variable* result =
       scope()->NewTemporary(ast_value_factory()->dot_result_string());
-  Block* block = ParseBlock(nullptr, false, CHECK_OK);
+  Block* block = ParseBlock(nullptr, CHECK_OK);
   DoExpression* expr = factory()->NewDoExpression(block, result, pos);
-  if (!Rewriter::Rewrite(this, expr, ast_value_factory())) {
+  if (!Rewriter::Rewrite(this, scope()->ClosureScope(), expr,
+                         ast_value_factory())) {
     *ok = false;
     return nullptr;
   }
-  block->set_scope(block->scope()->FinalizeBlockScope());
   return expr;
 }
 
@@ -5105,7 +5096,7 @@ Expression* Parser::ParseClassLiteral(ExpressionClassifier* classifier,
   int end_pos = scanner()->location().end_pos;
 
   if (constructor == NULL) {
-    DCHECK_EQ(this->scope(), block_scope);
+    DCHECK_EQ(scope(), block_scope);
     constructor = DefaultConstructor(name, has_extends, pos, end_pos,
                                      block_scope->language_mode());
   }
@@ -5131,7 +5122,8 @@ Expression* Parser::ParseClassLiteral(ExpressionClassifier* classifier,
   do_block->statements()->Add(
       factory()->NewExpressionStatement(class_literal, pos), zone());
   do_expr->set_represented_function(constructor);
-  Rewriter::Rewrite(this, do_expr, ast_value_factory());
+  Rewriter::Rewrite(this, scope()->ClosureScope(), do_expr,
+                    ast_value_factory());
 
   return do_expr;
 }
@@ -6640,7 +6632,7 @@ Expression* ParserTraits::RewriteYieldStar(
 
     Variable* dot_result = scope->NewTemporary(avfactory->dot_result_string());
     yield_star = factory->NewDoExpression(do_block, dot_result, nopos);
-    Rewriter::Rewrite(parser_, yield_star, avfactory);
+    Rewriter::Rewrite(parser_, scope->ClosureScope(), yield_star, avfactory);
   }
 
   return yield_star;

src/parsing/parser.h

@@ -813,8 +813,6 @@ class Parser : public ParserBase<ParserTraits> {
                                    bool default_export, bool* ok);
   Statement* ParseNativeDeclaration(bool* ok);
   Block* ParseBlock(ZoneList<const AstRawString*>* labels, bool* ok);
-  Block* ParseBlock(ZoneList<const AstRawString*>* labels,
-                    bool finalize_block_scope, bool* ok);
   Block* ParseVariableStatement(VariableDeclarationContext var_context,
                                 ZoneList<const AstRawString*>* names,
                                 bool* ok);

src/parsing/rewriter.cc

@@ -13,27 +13,29 @@ namespace internal {
 
 class Processor final : public AstVisitor<Processor> {
  public:
-  Processor(Isolate* isolate, Scope* scope, Variable* result,
+  Processor(Isolate* isolate, Scope* closure_scope, Variable* result,
             AstValueFactory* ast_value_factory)
       : result_(result),
         result_assigned_(false),
         replacement_(nullptr),
         is_set_(false),
         zone_(ast_value_factory->zone()),
-        scope_(scope),
+        closure_scope_(closure_scope),
         factory_(ast_value_factory) {
+    DCHECK_EQ(closure_scope, closure_scope->ClosureScope());
     InitializeAstVisitor(isolate);
   }
 
-  Processor(Parser* parser, Scope* scope, Variable* result,
+  Processor(Parser* parser, Scope* closure_scope, Variable* result,
             AstValueFactory* ast_value_factory)
       : result_(result),
         result_assigned_(false),
         replacement_(nullptr),
         is_set_(false),
         zone_(ast_value_factory->zone()),
-        scope_(scope),
+        closure_scope_(closure_scope),
         factory_(ast_value_factory) {
+    DCHECK_EQ(closure_scope, closure_scope->ClosureScope());
     InitializeAstVisitor(parser->stack_limit());
   }
 
@@ -41,7 +43,7 @@ class Processor final : public AstVisitor<Processor> {
   bool result_assigned() const { return result_assigned_; }
 
   Zone* zone() { return zone_; }
-  Scope* scope() { return scope_; }
+  Scope* closure_scope() { return closure_scope_; }
   AstNodeFactory* factory() { return &factory_; }
 
   // Returns ".result = value"
@@ -75,7 +77,7 @@ class Processor final : public AstVisitor<Processor> {
   bool is_set_;
 
   Zone* zone_;
-  Scope* scope_;
+  Scope* closure_scope_;
   AstNodeFactory factory_;
 
   // Node visitors.
@@ -223,19 +225,19 @@ void Processor::VisitTryFinallyStatement(TryFinallyStatement* node) {
      // at the end again: ".backup = .result; ...; .result = .backup"
      // This is necessary because the finally block does not normally contribute
      // to the completion value.
-    CHECK(scope() != nullptr);
-    Variable* backup = scope()->NewTemporary(
-        factory()->ast_value_factory()->dot_result_string());
-    Expression* backup_proxy = factory()->NewVariableProxy(backup);
-    Expression* result_proxy = factory()->NewVariableProxy(result_);
-    Expression* save = factory()->NewAssignment(
-        Token::ASSIGN, backup_proxy, result_proxy, kNoSourcePosition);
-    Expression* restore = factory()->NewAssignment(
-        Token::ASSIGN, result_proxy, backup_proxy, kNoSourcePosition);
-    node->finally_block()->statements()->InsertAt(
-        0, factory()->NewExpressionStatement(save, kNoSourcePosition), zone());
-    node->finally_block()->statements()->Add(
-        factory()->NewExpressionStatement(restore, kNoSourcePosition), zone());
+     CHECK_NOT_NULL(closure_scope());
+     Variable* backup = closure_scope()->NewTemporary(
+         factory()->ast_value_factory()->dot_result_string());
+     Expression* backup_proxy = factory()->NewVariableProxy(backup);
+     Expression* result_proxy = factory()->NewVariableProxy(result_);
+     Expression* save = factory()->NewAssignment(
+         Token::ASSIGN, backup_proxy, result_proxy, kNoSourcePosition);
+     Expression* restore = factory()->NewAssignment(
+         Token::ASSIGN, result_proxy, backup_proxy, kNoSourcePosition);
+     node->finally_block()->statements()->InsertAt(
+         0, factory()->NewExpressionStatement(save, kNoSourcePosition), zone());
+     node->finally_block()->statements()->Add(
+         factory()->NewExpressionStatement(restore, kNoSourcePosition), zone());
   }
   is_set_ = set_after;
   Visit(node->try_block());
@@ -333,18 +335,19 @@ DECLARATION_NODE_LIST(DEF_VISIT)
 // continue to be used in the case of failure.
 bool Rewriter::Rewrite(ParseInfo* info) {
   FunctionLiteral* function = info->literal();
-  DCHECK(function != NULL);
+  DCHECK_NOT_NULL(function);
   Scope* scope = function->scope();
-  DCHECK(scope != NULL);
+  DCHECK_NOT_NULL(scope);
   if (!scope->is_script_scope() && !scope->is_eval_scope()) return true;
+  Scope* closure_scope = scope->ClosureScope();
 
   ZoneList<Statement*>* body = function->body();
   if (!body->is_empty()) {
-    Variable* result =
-        scope->NewTemporary(info->ast_value_factory()->dot_result_string());
+    Variable* result = closure_scope->NewTemporary(
+        info->ast_value_factory()->dot_result_string());
     // The name string must be internalized at this point.
     DCHECK(!result->name().is_null());
-    Processor processor(info->isolate(), scope, result,
+    Processor processor(info->isolate(), closure_scope, result,
                         info->ast_value_factory());
     processor.Process(body);
     if (processor.HasStackOverflow()) return false;
@@ -362,17 +365,18 @@ bool Rewriter::Rewrite(ParseInfo* info) {
   return true;
 }
 
-
-bool Rewriter::Rewrite(Parser* parser, DoExpression* expr,
+bool Rewriter::Rewrite(Parser* parser, Scope* closure_scope, DoExpression* expr,
                        AstValueFactory* factory) {
   Block* block = expr->block();
-  Scope* scope = block->scope();
+  DCHECK_EQ(closure_scope, closure_scope->ClosureScope());
+  DCHECK(block->scope() == nullptr ||
+         block->scope()->ClosureScope() == closure_scope);
   ZoneList<Statement*>* body = block->statements();
   VariableProxy* result = expr->result();
   Variable* result_var = result->var();
 
   if (!body->is_empty()) {
-    Processor processor(parser, scope, result_var, factory);
+    Processor processor(parser, closure_scope, result_var, factory);
     processor.Process(body);
     if (processor.HasStackOverflow()) return false;
 

src/parsing/rewriter.h

@@ -12,6 +12,7 @@ class AstValueFactory;
 class DoExpression;
 class ParseInfo;
 class Parser;
+class Scope;
 
 class Rewriter {
  public:
@@ -24,8 +25,12 @@ class Rewriter {
   static bool Rewrite(ParseInfo* info);
 
   // Rewrite a list of statements, using the same rules as a top-level program,
-  // to  ensure identical behaviour of completion result.
-  static bool Rewrite(Parser* parser, DoExpression* expr,
+  // to ensure identical behaviour of completion result.  The temporary is added
+  // to the closure scope of the do-expression, which matches the closure scope
+  // of the outer scope (the do-expression itself runs in a block scope, not a
+  // closure scope). This closure scope needs to be passed in since the
+  // do-expression could have dropped its own block scope.
+  static bool Rewrite(Parser* parser, Scope* closure_scope, DoExpression* expr,
                       AstValueFactory* factory);
 };