Move sloppy block function hoisting logic from Parser to Scope.

src/parsing/parser.cc

Index: src/parsing/parser.cc
diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc
index ea9282bf70995f172253943ca86ec2fcc2b3d22b..d53123fd270981089e705912af680aa9c2b629fe 100644
--- a/src/parsing/parser.cc
+++ b/src/parsing/parser.cc
@@ -719,7 +719,7 @@ FunctionLiteral* Parser::DoParseProgram(ParseInfo* info) {
       // pre-existing bindings should be made writable, enumerable and
       // nonconfigurable if possible, whereas this code will leave attributes
       // unchanged if the property already exists.
-      InsertSloppyBlockFunctionVarBindings(scope, nullptr, &ok);
+      InsertSloppyBlockFunctionVarBindings(scope, &ok);
     }
     if (ok) {
       CheckConflictingVarDeclarations(scope, &ok);
@@ -1629,14 +1629,10 @@ VariableProxy* Parser::NewUnresolved(const AstRawString* name) {
                                 scanner()->location().end_pos);
 }
 
-InitializationFlag Parser::DefaultInitializationFlag(VariableMode mode) {
-  DCHECK(IsDeclaredVariableMode(mode));
-  return mode == VAR ? kCreatedInitialized : kNeedsInitialization;
-}
-
 Declaration* Parser::DeclareVariable(const AstRawString* name,
                                      VariableMode mode, int pos, bool* ok) {
-  return DeclareVariable(name, mode, DefaultInitializationFlag(mode), pos, ok);
+  return DeclareVariable(name, mode, Variable::DefaultInitializationFlag(mode),
+                         pos, ok);
 }
 
 Declaration* Parser::DeclareVariable(const AstRawString* name,
@@ -1799,7 +1795,6 @@ Statement* Parser::ParseHoistableDeclaration(
   Declare(declaration, DeclarationDescriptor::NORMAL, mode, kCreatedInitialized,
           CHECK_OK);
   if (names) names->Add(variable_name, zone());
-  EmptyStatement* empty = factory()->NewEmptyStatement(kNoSourcePosition);
   // Async functions don't undergo sloppy mode block scoped hoisting, and don't
   // allow duplicates in a block. Both are represented by the
   // sloppy_block_function_map. Don't add them to the map for async functions.
@@ -1808,12 +1803,12 @@ Statement* Parser::ParseHoistableDeclaration(
   if (is_sloppy(language_mode()) && !scope()->is_declaration_scope() &&
       !is_async && !(allow_harmony_restrictive_generators() && is_generator)) {
     SloppyBlockFunctionStatement* delegate =
-        factory()->NewSloppyBlockFunctionStatement(empty, scope());
+        factory()->NewSloppyBlockFunctionStatement(scope());
     DeclarationScope* target_scope = GetDeclarationScope();
     target_scope->DeclareSloppyBlockFunction(variable_name, delegate);
     return delegate;
   }
-  return empty;
+  return factory()->NewEmptyStatement(kNoSourcePosition);
 }
 
 Statement* Parser::ParseClassDeclaration(ZoneList<const AstRawString*>* names,
@@ -4373,8 +4368,7 @@ ZoneList<Statement*>* Parser::ParseEagerFunctionBody(
     Block* init_block = BuildParameterInitializationBlock(parameters, CHECK_OK);
 
     if (is_sloppy(inner_scope->language_mode())) {
-      InsertSloppyBlockFunctionVarBindings(inner_scope, function_scope,
-                                           CHECK_OK);
+      InsertSloppyBlockFunctionVarBindings(inner_scope, CHECK_OK);
     }
 
     // TODO(littledan): Merge the two rejection blocks into one
@@ -4396,7 +4390,7 @@ ZoneList<Statement*>* Parser::ParseEagerFunctionBody(
   } else {
     DCHECK_EQ(inner_scope, function_scope);
     if (is_sloppy(function_scope->language_mode())) {
-      InsertSloppyBlockFunctionVarBindings(function_scope, nullptr, CHECK_OK);
+      InsertSloppyBlockFunctionVarBindings(function_scope, CHECK_OK);
     }
   }
 
@@ -4483,7 +4477,7 @@ Expression* Parser::ParseClassLiteral(const AstRawString* name,
     Declaration* declaration =
         factory()->NewVariableDeclaration(proxy, block_state.scope(), pos);
     Declare(declaration, DeclarationDescriptor::NORMAL, CONST,
-            DefaultInitializationFlag(CONST), CHECK_OK);
+            Variable::DefaultInitializationFlag(CONST), CHECK_OK);
   }
 
   Expression* extends = nullptr;
@@ -4686,86 +4680,22 @@ void Parser::InsertShadowingVarBindingInitializers(Block* inner_block) {
 }
 
 void Parser::InsertSloppyBlockFunctionVarBindings(DeclarationScope* scope,
-                                                  Scope* complex_params_scope,
                                                   bool* ok) {
-  // For each variable which is used as a function declaration in a sloppy
-  // block,
+  scope->HoistSloppyBlockFunctions(factory(), ok);

[nickie, 2016/09/05 11:53:21]

If I understand it right, this separation of logic results in two traversals of
scope->sloppy_block_function_map(), instead of one.  (One hidden in
HoistSloppyBlockFunctions, and one a few lines below.)  I don't understand the
context enough to assess if that's a problem.

[marja, 2016/09/06 12:16:10]

Yes, this is correct, and this is also why this CL is somewhat "meh". :/ But
otoh, I don't see another way how to make this work (passing AstNodeFactory to
Scope and letting it create the Statements was a no-go). We'd need somehow to
move the responsibility of deciding what to hoist to Scope (so that it's
accessible to PreParser variants too so that they can also start understanding
more about Scopes).

+  DCHECK(*ok);
+  if (!*ok) return;

[nickie, 2016/09/05 11:53:21]

Because of the DCHECK in HoistSloppyBlockFunctions, this DCHECK is not necessary
(there's another DCHECK on *ok before that).  I suggest a CHECK_OK_VOID here.

[marja, 2016/09/06 12:16:10]

Will do (I'll do all review comments at one go, assuming I get some from
littledan@ too).

[marja, 2016/09/07 08:21:22]

Done.

+
   SloppyBlockFunctionMap* map = scope->sloppy_block_function_map();
   for (ZoneHashMap::Entry* p = map->Start(); p != nullptr; p = map->Next(p)) {
-    AstRawString* name = static_cast<AstRawString*>(p->key);
-
-    // If the variable wouldn't conflict with a lexical declaration
-    // or parameter,
-
-    // Check if there's a conflict with a parameter.
-    // This depends on the fact that functions always have a scope solely to
-    // hold complex parameters, and the names local to that scope are
-    // precisely the names of the parameters. IsDeclaredParameter(name) does
-    // not hold for names declared by complex parameters, nor are those
-    // bindings necessarily declared lexically, so we have to check for them
-    // explicitly. On the other hand, if there are not complex parameters,
-    // it is sufficient to just check IsDeclaredParameter.
-    if (complex_params_scope != nullptr) {
-      if (complex_params_scope->LookupLocal(name) != nullptr) {
-        continue;
-      }
-    } else {
-      if (scope->IsDeclaredParameter(name)) {
-        continue;
-      }
-    }
-
-    bool var_created = false;
-
     // Write in assignments to var for each block-scoped function declaration
     auto delegates = static_cast<SloppyBlockFunctionStatement*>(p->value);
-
-    DeclarationScope* decl_scope = scope;
-    while (decl_scope->is_eval_scope()) {
-      decl_scope = decl_scope->outer_scope()->GetDeclarationScope();
-    }
-    Scope* outer_scope = decl_scope->outer_scope();
-
     for (SloppyBlockFunctionStatement* delegate = delegates;
          delegate != nullptr; delegate = delegate->next()) {
-      // Check if there's a conflict with a lexical declaration
-      Scope* query_scope = delegate->scope()->outer_scope();
-      Variable* var = nullptr;
-      bool should_hoist = true;
-
-      // Note that we perform this loop for each delegate named 'name',
-      // which may duplicate work if those delegates share scopes.
-      // It is not sufficient to just do a Lookup on query_scope: for
-      // example, that does not prevent hoisting of the function in
-      // `{ let e; try {} catch (e) { function e(){} } }`
-      do {
-        var = query_scope->LookupLocal(name);
-        if (var != nullptr && IsLexicalVariableMode(var->mode())) {
-          should_hoist = false;
-          break;
-        }
-        query_scope = query_scope->outer_scope();
-      } while (query_scope != outer_scope);
-
-      if (!should_hoist) continue;
-
-      // Declare a var-style binding for the function in the outer scope
-      if (!var_created) {
-        var_created = true;
-        VariableProxy* proxy = scope->NewUnresolved(factory(), name);
-        Declaration* declaration =
-            factory()->NewVariableDeclaration(proxy, scope, kNoSourcePosition);
-        Declare(declaration, DeclarationDescriptor::NORMAL, VAR,
-                DefaultInitializationFlag(VAR), ok, scope);
-        DCHECK(*ok);  // Based on the preceding check, this should not fail
-        if (!*ok) return;
+      if (delegate->to() == nullptr) {
+        continue;
       }
-
-      // Read from the local lexical scope and write to the function scope
-      VariableProxy* to = scope->NewUnresolved(factory(), name);
-      VariableProxy* from = delegate->scope()->NewUnresolved(factory(), name);
-      Expression* assignment =
-          factory()->NewAssignment(Token::ASSIGN, to, from, kNoSourcePosition);
+      Expression* assignment = factory()->NewAssignment(
+          Token::ASSIGN, delegate->to(), delegate->from(), kNoSourcePosition);
       Statement* statement =
           factory()->NewExpressionStatement(assignment, kNoSourcePosition);
       delegate->set_statement(statement);