Sloppy-mode function declarations in blocks are now hoisted appropriately.

In ES2016, function declarations nested in blocks are formally allowed. This was
never a part of ECMAScript, but was a common extension. Unfortunately
implementations differed in the exact semantics. Annex B.3.3 in the spec tries
to standardize the parts which are common to different implementations, but does
so with some fairly complicated semantics.

This CL addresses three issues related to annex B.3.3:
* When the outer function had a complex parameter list, no hoisting whatsoever was
  being performed.
* Hoisting was not blocked by parameters of the same name.
* Hoisting was not blocked by nested lexical declarations of the same name.

We had tests which checked for the second, but they were incorrectly passing due to
the first. This CL adds more complete tests.

BUG=v8:5151, v8:5111

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

src/parsing/parser.cc

@@ -970,7 +970,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, nullptr, &ok);
     }
     if (ok) {
       CheckConflictingVarDeclarations(scope_, &ok);
@@ -4357,9 +4357,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) {
@@ -4808,6 +4805,11 @@ 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, inner_scope->outer_scope(), CHECK_OK);
+    }
+
     if (IsAsyncFunction(kind)) {
       init_block = BuildRejectPromiseOnException(init_block);
     }
@@ -4823,6 +4825,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, nullptr, CHECK_OK);
+    }
   }
 
   if (function_type == FunctionLiteral::kNamedExpression) {
@@ -5110,37 +5116,83 @@ void Parser::InsertShadowingVarBindingInitializers(Block* inner_block) {
   }
 }
 
-
+void Parser::InsertSloppyBlockFunctionVarBindings(Scope* scope,
-void Parser::InsertSloppyBlockFunctionVarBindings(Scope* scope, bool* ok) {
+                                                  Scope* complex_params_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())) {
-      // 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
+    // If the variable wouldn't conflict with a lexical declaration
-      auto delegates = static_cast<SloppyBlockFunctionMap::Vector*>(p->value);
+    // or parameter,
-      for (SloppyBlockFunctionStatement* delegate : *delegates) {
+
-        // Read from the local lexical scope and write to the function scope
+    // Check if there's a conflict with a parameter.
-        VariableProxy* to = scope->NewUnresolved(factory(), name);
+    // This depends on the fact that functions always have a scope solely to
-        VariableProxy* from = delegate->scope()->NewUnresolved(factory(), name);
+    // hold complex parameters, and the names local to that scope are
-        Expression* assignment = factory()->NewAssignment(
+    // precisely the names of the parameters. IsDeclaredParameter(name) does
-            Token::ASSIGN, to, from, RelocInfo::kNoPosition);
+    // not hold for names declared by complex parameters, nor are those
-        Statement* statement = factory()->NewExpressionStatement(
+    // bindings necessarily declared lexically, so we have to check for them
-            assignment, RelocInfo::kNoPosition);
+    // explicitly. On the other hand, if there are not complex parameters,
-        delegate->set_statement(statement);
+    // 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<SloppyBlockFunctionMap::Vector*>(p->value);
+    for (SloppyBlockFunctionStatement* delegate : *delegates) {
+      // Check if there's a conflict with a lexical declaration
+      Scope* outer_scope = scope->outer_scope();
+      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, 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);
     }
   }
 }

src/parsing/parser.h

@@ -1019,7 +1019,9 @@ class Parser : public ParserBase<ParserTraits> {
   void InsertShadowingVarBindingInitializers(Block* block);
 
   // Implement sloppy block-scoped functions, ES2015 Annex B 3.3
-  void InsertSloppyBlockFunctionVarBindings(Scope* scope, bool* ok);
+  void InsertSloppyBlockFunctionVarBindings(Scope* scope,
+                                            Scope* complex_params_scope,
+                                            bool* ok);
 
   // Parser support
   VariableProxy* NewUnresolved(const AstRawString* name, VariableMode mode);

test/mjsunit/es6/block-sloppy-function.js

@@ -109,6 +109,31 @@
   assertEquals('function', typeof f);
 })();
 
+(function complexParams(a = 0) {
+  {
+    let y = 3;
+    function f(b = 0) {
+      y = 2;
+    }
+    f();
+    assertEquals(2, y);
+  }
+  assertEquals('function', typeof f);
+})();
+
+(function complexVarParams(a = 0) {
+  var f;
+  {
+    let y = 3;
+    function f(b = 0) {
+      y = 2;
+    }
+    f();
+    assertEquals(2, y);
+  }
+  assertEquals('function', typeof f);
+})();
+
 (function conditional() {
   if (true) {
     function f() { return 1; }
@@ -142,6 +167,46 @@
   assertEquals(2, f());
 })();
 
+(function executionOrder() {
+  function getOuter() {
+    return f;
+  }
+  assertEquals('undefined', typeof getOuter());
+
+  {
+    assertEquals('function', typeof f);
+    assertEquals('undefined', typeof getOuter());
+    function f () {}
+    assertEquals('function', typeof f);
+    assertEquals('function', typeof getOuter());
+  }
+
+  assertEquals('function', typeof getOuter());
+})();
+
+(function reassignBindings() {
+  function getOuter() {
+    return f;
+  }
+  assertEquals('undefined', typeof getOuter());
+
+  {
+    assertEquals('function', typeof f);
+    assertEquals('undefined', typeof getOuter());
+    f = 1;
+    assertEquals('number', typeof f);
+    assertEquals('undefined', typeof getOuter());
+    function f () {}
+    assertEquals('number', typeof f);
+    assertEquals('number', typeof getOuter());
+    f = '';
+    assertEquals('string', typeof f);
+    assertEquals('number', typeof getOuter());
+  }
+
+  assertEquals('number', typeof getOuter());
+})();
+
 // Test that shadowing arguments is fine
 (function shadowArguments(x) {
   assertArrayEquals([1], arguments);
@@ -153,43 +218,242 @@
   assertEquals('function', typeof arguments);
 })(1);
 
-// Shadow function parameter
+
-(function shadowParameter(x) {
+// Don't shadow simple parameter
+(function shadowingParameterDoesntBind(x) {
   assertEquals(1, x);
   {
     function x() {}
   }
+  assertEquals(1, x);
+})(1);
+
+// Don't shadow complex parameter
+(function shadowingDefaultParameterDoesntBind(x = 0) {
+  assertEquals(1, x);
+  {
+    function x() {}
+  }
+  assertEquals(1, x);
+})(1);
+
+// Don't shadow nested complex parameter
+(function shadowingNestedParameterDoesntBind([[x]]) {
+  assertEquals(1, x);
+  {
+    function x() {}
+  }
+  assertEquals(1, x);
+})([[1]]);
+
+// Don't shadow rest parameter
+(function shadowingRestParameterDoesntBind(...x) {
+  assertArrayEquals([1], x);
+  {
+    function x() {}
+  }
+  assertArrayEquals([1], x);
+})(1);
+
+// Don't shadow complex rest parameter
+(function shadowingComplexRestParameterDoesntBind(...[x]) {
+  assertArrayEquals(1, x);
+  {
+    function x() {}
+  }
+  assertArrayEquals(1, x);
+})(1);
+
+// Previous tests with a var declaration thrown in.
+// Don't shadow simple parameter
+(function shadowingVarParameterDoesntBind(x) {
+  var x;
+  assertEquals(1, x);
+  {
+    function x() {}
+  }
+  assertEquals(1, x);
+})(1);
+
+// Don't shadow complex parameter
+(function shadowingVarDefaultParameterDoesntBind(x = 0) {
+  var x;
+  assertEquals(1, x);
+  {
+    function x() {}
+  }
+  assertEquals(1, x);
+})(1);
+
+// Don't shadow nested complex parameter
+(function shadowingVarNestedParameterDoesntBind([[x]]) {
+  var x;
+  assertEquals(1, x);
+  {
+    function x() {}
+  }
+  assertEquals(1, x);
+})([[1]]);
+
+// Don't shadow rest parameter
+(function shadowingVarRestParameterDoesntBind(...x) {
+  var x;
+  assertArrayEquals([1], x);
+  {
+    function x() {}
+  }
+  assertArrayEquals([1], x);
+})(1);
+
+// Don't shadow complex rest parameter
+(function shadowingVarComplexRestParameterDoesntBind(...[x]) {
+  var x;
+  assertArrayEquals(1, x);
+  {
+    function x() {}
+  }
+  assertArrayEquals(1, x);
+})(1);
+
+
+// Hoisting is not affected by other simple parameters
+(function irrelevantParameterBinds(y, z) {
+  assertEquals(undefined, x);
+  {
+    function x() {}
+  }
   assertEquals('function', typeof x);
 })(1);
 
-// Shadow function parameter
+// Hoisting is not affected by other complex parameters
-(function shadowDefaultParameter(x = 0) {
+(function irrelevantComplexParameterBinds([y] = [], z) {
-  assertEquals(1, x);
+  assertEquals(undefined, x);
   {
     function x() {}
   }
-  // TODO(littledan): Once destructured parameters are no longer
+  assertEquals('function', typeof x);
-  // let-bound, enable this assertion. This is the core of the test.
+})();
-  // assertEquals('function', typeof x);
-})(1);
 
-(function shadowRestParameter(...x) {
+// Hoisting is not affected by rest parameters
-  assertArrayEquals([1], x);
+(function irrelevantRestParameterBinds(y, ...z) {
+  assertEquals(undefined, x);
   {
     function x() {}
   }
-  // TODO(littledan): Once destructured parameters are no longer
+  assertEquals('function', typeof x);
-  // let-bound, enable this assertion. This is the core of the test.
+})();
-  // assertEquals('function', typeof x);
-})(1);
 
-assertThrows(function notInDefaultScope(x = y) {
+// Hoisting is not affected by complex rest parameters
+(function irrelevantRestParameterBinds(y, ...[z]) {
+  assertEquals(undefined, x);
   {
-    function y() {}
+    function x() {}
   }
-  assertEquals('function', typeof y);
+  assertEquals('function', typeof x);
-  assertEquals(x, undefined);
+})();
-}, ReferenceError);
+
+
+// Test that shadowing function name is fine
+{
+  let called = false;
+  (function shadowFunctionName() {
+    if (called) assertUnreachable();
+    called = true;
+    {
+      function shadowFunctionName() {
+        return 0;
+      }
+      assertEquals(0, shadowFunctionName());
+    }
+    assertEquals(0, shadowFunctionName());
+  })();
+}
+
+{
+  let called = false;
+  (function shadowFunctionNameWithComplexParameter(...r) {
+    if (called) assertUnreachable();
+    called = true;
+    {
+      function shadowFunctionNameWithComplexParameter() {
+        return 0;
+      }
+      assertEquals(0, shadowFunctionNameWithComplexParameter());
+    }
+    assertEquals(0, shadowFunctionNameWithComplexParameter());
+  })();
+}
+
+(function shadowOuterVariable() {
+  {
+    let f = 0;
+    (function () {
+      assertEquals(undefined, f);
+      {
+        assertEquals(1, f());
+        function f() { return 1; }
+        assertEquals(1, f());
+      }
+      assertEquals(1, f());
+    })();
+    assertEquals(0, f);
+  }
+})();
+
+(function notInDefaultScope() {
+  var y = 1;
+  (function innerNotInDefaultScope(x = y) {
+    assertEquals('undefined', typeof y);
+    {
+      function y() {}
+    }
+    assertEquals('function', typeof y);
+    assertEquals(1, x);
+  })();
+})();
+
+(function noHoistingThroughNestedLexical() {
+  {
+    let f = 2;
+    {
+      let y = 3;
+      function f() {
+        y = 2;
+      }
+      f();
+      assertEquals(2, y);
+    }
+    assertEquals(2, f);
+  }
+  assertThrows(()=>f, ReferenceError);
+})();
+
+// Only the first function is hoisted; the second is blocked by the first.
+// Contrast overridingLocalFunction, in which the outer function declaration
+// is not lexical and so the inner declaration is hoisted.
+(function noHoistingThroughNestedFunctions() {
+  assertEquals(undefined, f); // Also checks that the var-binding exists
+
+  {
+    assertEquals(4, f());
+
+    function f() {
+      return 4;
+    }
+
+    {
+      assertEquals(5, f());
+      function f() {
+        return 5;
+      }
+      assertEquals(5, f());
+    }
+
+    assertEquals(4, f());
+  }
+
+  assertEquals(4, f());
+})();
 
 // Test that hoisting from blocks does happen in global scope
 function globalHoisted() { return 0; }

test/test262/test262.status

@@ -323,16 +323,6 @@
   # https://bugs.chromium.org/p/v8/issues/detail?id=1569
   'language/module-code/*': [SKIP],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=5111
-  'annexB/language/function-code/block-decl-func-skip-param': [FAIL],
-  'annexB/language/function-code/if-decl-else-decl-a-func-skip-param': [FAIL],
-  'annexB/language/function-code/if-decl-else-decl-b-func-skip-param': [FAIL],
-  'annexB/language/function-code/if-decl-else-stmt-func-skip-param': [FAIL],
-  'annexB/language/function-code/if-decl-no-else-func-skip-param': [FAIL],
-  'annexB/language/function-code/if-stmt-else-decl-func-skip-param': [FAIL],
-  'annexB/language/function-code/switch-case-func-skip-param': [FAIL],
-  'annexB/language/function-code/switch-dflt-func-skip-param': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=5112
   'language/statements/try/scope-catch-block-lex-open': [FAIL],
   'language/statements/try/scope-catch-param-lex-open': [FAIL],