Allow lexically declared "arguments" in function scope in sloppy mode.

Lexically declared "arguments" in sloppy mode will throw redeclaration error
currently, this patch fixes it by delaying the declaration of arguments until we
fully parse parameter list and function body.

BUG=v8:4577
LOG=N

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

src/ast/scopes.cc

@@ -486,15 +486,32 @@ void DeclarationScope::DeclareThis(AstValueFactory* ast_value_factory) {
   receiver_ = var;
 }
 
-void DeclarationScope::DeclareDefaultFunctionVariables(
-    AstValueFactory* ast_value_factory) {
+void DeclarationScope::DeclareArguments(AstValueFactory* ast_value_factory) {
   DCHECK(is_function_scope());
   DCHECK(!is_arrow_scope());
+
+  // Check if there's lexically declared variable named arguments to avoid
+  // redeclaration. See ES#sec-functiondeclarationinstantiation, step 20.
+  Variable* arg_variable = LookupLocal(ast_value_factory->arguments_string());
+  if (arg_variable != nullptr && IsLexicalVariableMode(arg_variable->mode())) {
+    return;
+  }
+
   // Declare 'arguments' variable which exists in all non arrow functions.
   // Note that it might never be accessed, in which case it won't be
   // allocated during variable allocation.
-  arguments_ = Declare(zone(), this, ast_value_factory->arguments_string(), VAR,
-                       Variable::ARGUMENTS, kCreatedInitialized);
+  if (arg_variable == nullptr) {
+    arguments_ = Declare(zone(), this, ast_value_factory->arguments_string(),
+                         VAR, Variable::ARGUMENTS, kCreatedInitialized);
+  } else {
+    arguments_ = arg_variable;
+  }
+}
+
+void DeclarationScope::DeclareDefaultFunctionVariables(
+    AstValueFactory* ast_value_factory) {
+  DCHECK(is_function_scope());
+  DCHECK(!is_arrow_scope());
 
   new_target_ = Declare(zone(), this, ast_value_factory->new_target_string(),
                         CONST, Variable::NORMAL, kCreatedInitialized);
@@ -1499,8 +1516,8 @@ void DeclarationScope::AllocateParameterLocals() {
 
   bool uses_sloppy_arguments = false;
 
-  // Functions have 'arguments' declared implicitly in all non arrow functions.
   if (arguments_ != nullptr) {
+    DCHECK(!is_arrow_scope());
     // 'arguments' is used. Unless there is also a parameter called
     // 'arguments', we must be conservative and allocate all parameters to
     // the context assuming they will be captured by the arguments object.
@@ -1521,9 +1538,6 @@ void DeclarationScope::AllocateParameterLocals() {
       // allocate the arguments object by nulling out arguments_.
       arguments_ = nullptr;
     }
-
-  } else {
-    DCHECK(is_arrow_scope());
   }
 
   // The same parameter may occur multiple times in the parameters_ list.

src/ast/scopes.h

@@ -626,6 +626,7 @@ class DeclarationScope : public Scope {
   void set_asm_function() { asm_module_ = true; }
 
   void DeclareThis(AstValueFactory* ast_value_factory);
+  void DeclareArguments(AstValueFactory* ast_value_factory);
   void DeclareDefaultFunctionVariables(AstValueFactory* ast_value_factory);
 
   // This lookup corresponds to a lookup in the "intermediate" scope sitting

src/parsing/parser.cc

@@ -3861,6 +3861,10 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
 
     // Parsing the body may change the language mode in our scope.
     language_mode = scope->language_mode();
+    scope->DeclareArguments(ast_value_factory());
+    if (main_scope != scope) {
+      main_scope->DeclareArguments(ast_value_factory());
+    }
 
     // Validate name and parameter names. We can do this only after parsing the
     // function, since the function can declare itself strict.

test/cctest/interpreter/bytecode_expectations/CallLookupSlot.golden

@@ -20,9 +20,9 @@ bytecodes: [
                 B(Ldar), R(this),
                 B(StaContextSlot), R(context), U8(4),
                 B(CreateMappedArguments),
-                B(StaContextSlot), R(context), U8(5),
-                B(Ldar), R(new_target),
                 B(StaContextSlot), R(context), U8(6),
+                B(Ldar), R(new_target),
+                B(StaContextSlot), R(context), U8(5),
   /*   30 E> */ B(StackCheck),
   /*   34 S> */ B(CreateClosure), U8(0), U8(2),
   /*   36 E> */ B(StaLookupSlotSloppy), U8(1),

test/cctest/interpreter/bytecode_expectations/CreateRestParameter.golden

@@ -85,17 +85,17 @@ parameter count: 2
 bytecode array length: 26
 bytecodes: [
                 B(CreateUnmappedArguments),
-                B(Star), R(0),
+                B(Star), R(2),
                 B(CreateRestParameter),
-                B(Star), R(1),
+                B(Star), R(0),
                 B(LdaTheHole),
-                B(Star), R(2),
+                B(Star), R(1),
   /*   10 E> */ B(StackCheck),
-                B(Mov), R(arg0), R(2),
+                B(Mov), R(arg0), R(1),
   /*   29 S> */ B(LdaZero),
-  /*   44 E> */ B(LdrKeyedProperty), R(1), U8(1), R(4),
+  /*   44 E> */ B(LdrKeyedProperty), R(0), U8(1), R(4),
                 B(LdaZero),
-  /*   59 E> */ B(LdaKeyedProperty), R(0), U8(3),
+  /*   59 E> */ B(LdaKeyedProperty), R(2), U8(3),
                 B(Add), R(4), U8(5),
   /*   64 S> */ B(Return),
 ]

test/cctest/interpreter/bytecode_expectations/Eval.golden

@@ -20,9 +20,9 @@ bytecodes: [
                 B(Ldar), R(this),
                 B(StaContextSlot), R(context), U8(4),
                 B(CreateMappedArguments),
-                B(StaContextSlot), R(context), U8(5),
-                B(Ldar), R(new_target),
                 B(StaContextSlot), R(context), U8(6),
+                B(Ldar), R(new_target),
+                B(StaContextSlot), R(context), U8(5),
   /*   30 E> */ B(StackCheck),
   /*   34 S> */ B(LdaConstant), U8(0),
                 B(Star), R(3),

test/cctest/interpreter/bytecode_expectations/LookupSlot.golden

@@ -20,9 +20,9 @@ bytecodes: [
                 B(Ldar), R(this),
                 B(StaContextSlot), R(context), U8(4),
                 B(CreateMappedArguments),
-                B(StaContextSlot), R(context), U8(5),
-                B(Ldar), R(new_target),
                 B(StaContextSlot), R(context), U8(6),
+                B(Ldar), R(new_target),
+                B(StaContextSlot), R(context), U8(5),
   /*   30 E> */ B(StackCheck),
   /*   34 S> */ B(LdaConstant), U8(0),
                 B(Star), R(3),
@@ -65,9 +65,9 @@ bytecodes: [
                 B(Ldar), R(this),
                 B(StaContextSlot), R(context), U8(4),
                 B(CreateMappedArguments),
-                B(StaContextSlot), R(context), U8(5),
-                B(Ldar), R(new_target),
                 B(StaContextSlot), R(context), U8(6),
+                B(Ldar), R(new_target),
+                B(StaContextSlot), R(context), U8(5),
   /*   30 E> */ B(StackCheck),
   /*   34 S> */ B(LdaConstant), U8(0),
                 B(Star), R(3),
@@ -111,9 +111,9 @@ bytecodes: [
                 B(Ldar), R(this),
                 B(StaContextSlot), R(context), U8(4),
                 B(CreateMappedArguments),
-                B(StaContextSlot), R(context), U8(5),
-                B(Ldar), R(new_target),
                 B(StaContextSlot), R(context), U8(6),
+                B(Ldar), R(new_target),
+                B(StaContextSlot), R(context), U8(5),
   /*   30 E> */ B(StackCheck),
   /*   34 S> */ B(LdaSmi), U8(20),
   /*   36 E> */ B(StaLookupSlotSloppy), U8(0),

test/cctest/interpreter/test-bytecode-generator.cc

@@ -56,12 +56,13 @@ namespace interpreter {
 #define REPEAT_64_UNIQUE_VARS() REPEAT_32_UNIQUE_VARS() REPEAT_32_UNIQUE_VARS()
 #define REPEAT_128_UNIQUE_VARS() REPEAT_64_UNIQUE_VARS() REPEAT_64_UNIQUE_VARS()
 
-#define REPEAT_249_UNIQUE_VARS() \
+#define REPEAT_250_UNIQUE_VARS() \
   REPEAT_128_UNIQUE_VARS()       \
   REPEAT_64_UNIQUE_VARS()        \
   REPEAT_32_UNIQUE_VARS()        \
   REPEAT_16_UNIQUE_VARS()        \
   REPEAT_8_UNIQUE_VARS()         \
+  UNIQUE_VAR()                   \
   UNIQUE_VAR()
 
 static const char* kGoldenFileDirectory =
@@ -1353,7 +1354,7 @@ TEST(ContextVariables) {
     "{ let b = 2; return function() { a + b; }; }\n",
 
     "'use strict';\n"
-    REPEAT_249_UNIQUE_VARS()
+    REPEAT_250_UNIQUE_VARS()
     "eval();\n"
     "var b = 100;\n"
     "return b\n",

test/cctest/test-parsing.cc

@@ -6444,10 +6444,9 @@ TEST(DestructuringPositiveTests) {
   RunParserSyncTest(context_data, data, kSuccess);
 
   // v8:5201
-  // TODO(lpy): The two test sets below should be merged once
-  // we fix https://bugs.chromium.org/p/v8/issues/detail?id=4577
   {
-    const char* sloppy_context_data1[][2] = {
+    // clang-format off
+    const char* sloppy_context_data[][2] = {
       {"var ", " = {};"},
       {"function f(", ") {}"},
       {"function f(argument1, ", ") {}"},
@@ -6456,26 +6455,17 @@ TEST(DestructuringPositiveTests) {
       {"try {} catch(", ") {}"},
       {NULL, NULL}
     };
-    const char* data1[] = {
+    const char* data[] = {
+      "{arguments}",
       "{eval}",
+      "{x: arguments}",
       "{x: eval}",
+      "{arguments = false}",
       "{eval = false}",
       NULL
     };
-    RunParserSyncTest(sloppy_context_data1, data1, kSuccess);
-
-    const char* sloppy_context_data2[][2] = {
-      {"var ", " = {};"},
-      {"try {} catch(", ") {}"},
-      {NULL, NULL}
-    };
-    const char* data2[] = {
-      "{arguments}",
-      "{x: arguments}",
-      "{arguments = false}",
-      NULL,
-    };
-    RunParserSyncTest(sloppy_context_data2, data2, kSuccess);
+    // clang-format on
+    RunParserSyncTest(sloppy_context_data, data, kSuccess);
   }
 }
 
@@ -8330,3 +8320,38 @@ TEST(TrailingCommasInParametersErrors) {
   RunParserSyncTest(context_data, data, kError, NULL, 0, always_flags,
                     arraysize(always_flags));
 }
+
+TEST(ArgumentsRedeclaration) {
+  {
+    // clang-format off
+    const char* context_data[][2] = {
+      { "function f(", ") {}" },
+      { NULL, NULL }
+    };
+    const char* success_data[] = {
+      "{arguments}",
+      "{arguments = false}",
+      "arg1, arguments",
+      "arg1, ...arguments",
+      NULL
+    };
+    // clang-format on
+    RunParserSyncTest(context_data, success_data, kSuccess);
+  }
+
+  {
+    // clang-format off
+    const char* context_data[][2] = {
+      { "function f() {", "}" },
+      { NULL, NULL }
+    };
+    const char* data[] = {
+      "const arguments = 1",
+      "let arguments",
+      "var arguments",
+      NULL
+    };
+    // clang-format on
+    RunParserSyncTest(context_data, data, kSuccess);
+  }
+}

test/mjsunit/bugs/bug-4577.js → test/mjsunit/regress/regress-4577.js

@@ -11,3 +11,11 @@ function g({arguments}) {
   return arguments === 42;
 }
 assertTrue(g({arguments: 42}));
+
+function foo() {
+  let arguments = 2;
+  return arguments;
+}
+assertEquals(2, foo());
+
+assertThrows(function(x = arguments, arguments) {}, ReferenceError);