Annex B.3.3 semantics

src/parsing/parser.cc

Index: src/parsing/parser.cc
diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc
index 389068162dd7882d3072ef3d88ca1ef0fb971092..cfadad176d2404da13b64cc8b47ded0646677341 100644
--- a/src/parsing/parser.cc
+++ b/src/parsing/parser.cc
@@ -969,7 +969,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, &ok);
+      InsertSloppyBlockFunctionVarBindings(scope, false, &ok);
     }
     if (ok) {
       CheckConflictingVarDeclarations(scope_, &ok);
@@ -4356,9 +4356,6 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
       CheckDecimalLiteralWithLeadingZero(use_counts_, scope->start_position(),
                                          scope->end_position());
     }
-    if (is_sloppy(language_mode)) {
-      InsertSloppyBlockFunctionVarBindings(scope, CHECK_OK);
-    }
     CheckConflictingVarDeclarations(scope, CHECK_OK);
 
     if (body) {
@@ -4807,6 +4804,10 @@ ZoneList<Statement*>* Parser::ParseEagerFunctionBody(
     SetLanguageMode(scope_, inner_scope->language_mode());
     Block* init_block = BuildParameterInitializationBlock(parameters, CHECK_OK);
 
+    if (is_sloppy(inner_scope->language_mode())) {
+      InsertSloppyBlockFunctionVarBindings(inner_scope, true, CHECK_OK);
+    }
+
     if (IsAsyncFunction(kind)) {
       init_block = BuildRejectPromiseOnException(init_block);
     }
@@ -4822,6 +4823,10 @@ ZoneList<Statement*>* Parser::ParseEagerFunctionBody(
 
     result->Add(init_block, zone());
     result->Add(inner_block, zone());
+  } else {
+    if (is_sloppy(inner_scope->language_mode())) {
+      InsertSloppyBlockFunctionVarBindings(inner_scope, false, CHECK_OK);
+    }
   }
 
   if (function_type == FunctionLiteral::kNamedExpression) {
@@ -5108,37 +5113,76 @@ void Parser::InsertShadowingVarBindingInitializers(Block* inner_block) {
   }
 }
 
-
-void Parser::InsertSloppyBlockFunctionVarBindings(Scope* scope, bool* ok) {
+void Parser::InsertSloppyBlockFunctionVarBindings(Scope* scope,
+                                                  bool outer_is_param_scope,
+                                                  bool* ok) {
   // For each variable which is used as a function declaration in a sloppy
   // block,
   DCHECK(scope->is_declaration_scope());
   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,
-    Variable* var = scope->LookupLocal(name);
-    if (var == nullptr || !IsLexicalVariableMode(var->mode())) {
+
+    bool var_created = false;
+
+    // Write in assignments to var for each block-scoped function declaration
+    auto delegates = static_cast<SloppyBlockFunctionMap::Vector*>(p->value);
+    for (SloppyBlockFunctionStatement* delegate : *delegates) {
+      // If the variable wouldn't conflict with a lexical declaration
+      // or parameter,
+
+      // Check if there's a conflict with a simple parameter
+      if (scope->IsDeclaredParameter(name)) continue;
+
+      Scope* outer_scope = scope->outer_scope();
+
+      /* Check if there's a conflict with a complex parameter.

[adamk, 2016/06/28 20:55:11]

Style nit: please use '//' style comments for consistency.

[bakkot, 2016/06/29 00:25:02]

Done.

+       * This depends on the fact that functions always create a new scope to

[adamk, 2016/06/28 20:55:11]

This comment seems a bit backwards, and it confused me on first reading. We
create a new scope to hold the declarations in the function body, and the
parameters themselves live in the scope that's left outside. I suppose that
amounts to the same thing, but as I said this was confusing as a reader of this
comment.

The use of the name "param_scope" in "outer_scope_is_param_scope" is further
confusing since "param_scope" is the name given to scopes created when a sloppy
eval is used in a complex parameter.

Please consider rewriting in terms that match the usage elsewhere in this file.

[bakkot, 2016/06/29 00:25:02]

Amended the comment slightly, and now we have a complex_params_scope passed in
instead of that flag.

+       * hold complex parameters, and the names local to that scope are
+       * precisely the names of the parameters. IsDeclaredParameter(name) does

[adamk, 2016/06/28 20:55:10]

Does IsDeclaredParameter ever hold in this case? I don't think it does, because
complex parameters are never named.

[bakkot, 2016/06/29 00:25:02]

You're correct; refactored that out. (I thought it might hold for the simple
parameters in a non-simple parameter list, but it does not.)

+       * not necessarily hold for names declared by complex parameters, nor
+       * are those bindings necessarily declared lexically, so we have to
+       * check for them explicitly. outer_is_param_scope holds iff we are in a
+       * function with complex parameters.
+       */
+      if (outer_is_param_scope && outer_scope->LookupLocal(name) != nullptr) {
+        continue;
+      }
+
+      // Check if there's a conflict with a lexical declaration
+      Scope* query_scope = delegate->scope()->outer_scope();

[adamk, 2016/06/28 20:55:10]

Can't everything above here live outside the delegate loop? It doesn't seem to
depend on the delegate at all.

[bakkot, 2016/06/29 00:25:02]

Done.

+      Variable* var = nullptr;
+      bool should_hoist = true;
+      do {

[adamk, 2016/06/28 20:55:10]

Even this loop seems like it should be hoistable with a little help (we
presumably query the same scopes multiple times...). Maybe add a TODO?

[bakkot, 2016/06/29 00:25:02]

Not exactly, but I added a note.

+        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
-      VariableProxy* proxy = scope->NewUnresolved(factory(), name);
-      Declaration* declaration = factory()->NewVariableDeclaration(
-          proxy, VAR, scope, RelocInfo::kNoPosition);
-      Declare(declaration, DeclarationDescriptor::NORMAL, true, ok, scope);
-      DCHECK(ok);  // Based on the preceding check, this should not fail
-      if (!ok) return;
-
-      // Write in assignments to var for each block-scoped function declaration
-      auto delegates = static_cast<SloppyBlockFunctionMap::Vector*>(p->value);
-      for (SloppyBlockFunctionStatement* delegate : *delegates) {
-        // 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, RelocInfo::kNoPosition);
-        Statement* statement = factory()->NewExpressionStatement(
-            assignment, RelocInfo::kNoPosition);
-        delegate->set_statement(statement);
+      if (!var_created) {
+        var_created = true;
+        VariableProxy* proxy = scope->NewUnresolved(factory(), name);
+        Declaration* declaration = factory()->NewVariableDeclaration(
+            proxy, VAR, scope, RelocInfo::kNoPosition);
+        Declare(declaration, DeclarationDescriptor::NORMAL, true, ok, scope);
+        DCHECK(ok);  // Based on the preceding check, this should not fail
+        if (!ok) return;
       }
+
+      // 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,
+                                                        RelocInfo::kNoPosition);
+      Statement* statement =
+          factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition);
+      delegate->set_statement(statement);
     }
   }
 }