Fix arrow function lexical arguments binding, allow rest + arguments

Issue #1547

18 files changed, 137 insertions, 137 deletions

diff --git a/js/src/frontend/BytecodeCompiler.cpp b/js/src/frontend/BytecodeCompiler.cpp
index 501390944..b32438bff 100644
--- a/js/src/frontend/BytecodeCompiler.cpp
+++ b/js/src/frontend/BytecodeCompiler.cpp
@@ -65,13 +65,6 @@ SetSourceMap(ExclusiveContext* cx, TokenStream& tokenStream, ScriptSource* ss)
 static bool
 CheckArgumentsWithinEval(JSContext* cx, Parser<FullParseHandler>& parser, HandleFunction fun)
 {
-    if (fun->hasRest()) {
-        // It's an error to use |arguments| in a function that has a rest
-        // parameter.
-        parser.report(ParseError, false, nullptr, JSMSG_ARGUMENTS_AND_REST);
-        return false;
-    }
-
     RootedScript script(cx, fun->getOrCreateScript(cx));
     if (!script)
         return false;
diff --git a/js/src/frontend/BytecodeEmitter.cpp b/js/src/frontend/BytecodeEmitter.cpp
index f799b8d03..4c5a359da 100644
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -6917,8 +6917,6 @@ frontend::EmitTree(ExclusiveContext* cx, BytecodeEmitter* bce, ParseNode* pn)
             ParseNode* rest = nullptr;
             bool restIsDefn = false;
             if (fun->hasRest()) {
-                MOZ_ASSERT(!bce->sc->asFunctionBox()->argumentsHasLocalBinding());
-
                 // Defaults with a rest parameter need special handling. The
                 // rest parameter needs to be undefined while defaults are being
                 // processed. To do this, we create the rest argument and let it
@@ -6964,7 +6962,6 @@ frontend::EmitTree(ExclusiveContext* cx, BytecodeEmitter* bce, ParseNode* pn)
                 return false;
             if (pn2->pn_next == pnlast && fun->hasRest() && !hasDefaults) {
                 // Fill rest parameter. We handled the case with defaults above.
-                MOZ_ASSERT(!bce->sc->asFunctionBox()->argumentsHasLocalBinding());
                 bce->switchToProlog();
                 if (Emit1(cx, bce, JSOP_REST) < 0)
                     return false;
diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp
index 962bc4d73..d6cdea95e 100644
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -909,10 +909,6 @@ Parser<FullParseHandler>::checkFunctionArguments()
         }
     }
 
-    /*
-     * Report error if both rest parameters and 'arguments' are used. Do this
-     * check before adding artificial 'arguments' below.
-     */
     Definition* maybeArgDef = pc->decls().lookupFirst(arguments);
     bool argumentsHasBinding = !!maybeArgDef;
     // ES6 9.2.13.17 says that a lexical binding of 'arguments' shadows the
@@ -920,18 +916,12 @@ Parser<FullParseHandler>::checkFunctionArguments()
     bool argumentsHasLocalBinding = maybeArgDef && (maybeArgDef->kind() != Definition::ARG &&
                                                     maybeArgDef->kind() != Definition::LET &&
                                                     maybeArgDef->kind() != Definition::CONST);
-    bool hasRest = pc->sc->asFunctionBox()->function()->hasRest();
-    if (hasRest && argumentsHasLocalBinding) {
-        report(ParseError, false, nullptr, JSMSG_ARGUMENTS_AND_REST);
-        return false;
-    }
 
     /*
      * Even if 'arguments' isn't explicitly mentioned, dynamic name lookup
-     * forces an 'arguments' binding. The exception is that functions with rest
-     * parameters are free from 'arguments'.
+     * forces an 'arguments' binding.
      */
-    if (!argumentsHasBinding && pc->sc->bindingsAccessedDynamically() && !hasRest) {
+    if (!argumentsHasBinding && pc->sc->bindingsAccessedDynamically()) {
         ParseNode* pn = newName(arguments);
         if (!pn)
             return false;
@@ -989,21 +979,8 @@ template <>
 bool
 Parser<SyntaxParseHandler>::checkFunctionArguments()
 {
-    bool hasRest = pc->sc->asFunctionBox()->function()->hasRest();
-
-    if (pc->lexdeps->lookup(context->names().arguments)) {
+    if (pc->lexdeps->lookup(context->names().arguments))
         pc->sc->asFunctionBox()->usesArguments = true;
-        if (hasRest) {
-            report(ParseError, false, null(), JSMSG_ARGUMENTS_AND_REST);
-            return false;
-        }
-    } else if (hasRest) {
-        DefinitionNode maybeArgDef = pc->decls().lookupFirst(context->names().arguments);
-        if (maybeArgDef && handler.getDefinitionKind(maybeArgDef) != Definition::ARG) {
-            report(ParseError, false, null(), JSMSG_ARGUMENTS_AND_REST);
-            return false;
-        }
-    }
 
     return true;
 }
@@ -1088,9 +1065,12 @@ Parser<ParseHandler>::functionBody(FunctionSyntaxKind kind, FunctionBodyType typ
             return null();
     }
 
-    /* Define the 'arguments' binding if necessary. */
-    if (!checkFunctionArguments())
-        return null();
+    if (kind != Arrow) {
+        // Define the 'arguments' binding if necessary. Arrow functions
+        // don't have 'arguments'.
+        if (!checkFunctionArguments())
+            return null();
+    }
 
     return pn;
 }
@@ -1982,8 +1962,9 @@ Parser<ParseHandler>::addFreeVariablesFromLazyFunction(JSFunction* fun,
     for (size_t i = 0; i < lazy->numFreeVariables(); i++) {
         JSAtom* atom = freeVariables[i].atom();
 
-        // 'arguments' will be implicitly bound within the inner function.
-        if (atom == context->names().arguments)
+        // 'arguments' will be implicitly bound within the inner function,
+        // except if the inner function is an arrow function.
+        if (atom == context->names().arguments && !fun->isArrow())
             continue;
 
         DefinitionNode dn = pc->decls().lookupFirst(atom);
diff --git a/js/src/jit-test/tests/arguments/bug844048.js b/js/src/jit-test/tests/arguments/bug844048.js
index b98630f82..b7e56a758 100644
--- a/js/src/jit-test/tests/arguments/bug844048.js
+++ b/js/src/jit-test/tests/arguments/bug844048.js
@@ -16,9 +16,4 @@ function bar() {
 bar();
 
 (function(){assertEq(typeof eval("var arguments; arguments"), "object")})();
-try {
-  (function(... rest){assertEq(typeof eval("var arguments; arguments"), "object")})();
-  assertEq(false, true);
-} catch (e) {
-  assertEq(/SyntaxError/.test(e), true);
-}
+(function(... rest){assertEq(typeof eval("var arguments; arguments"), "object")})();
diff --git a/js/src/jit-test/tests/arguments/rest-debugger.js b/js/src/jit-test/tests/arguments/rest-debugger.js
index 627f95848..c559e3a04 100644
--- a/js/src/jit-test/tests/arguments/rest-debugger.js
+++ b/js/src/jit-test/tests/arguments/rest-debugger.js
@@ -9,9 +9,9 @@ var g = newGlobal();
 g.eval("function f(...rest) { debugger; }");
 var dbg = Debugger(g);
 dbg.onDebuggerStatement = function (frame) {
-    var result = frame.eval("arguments");
-    assertEq("throw" in result, true);
-    var result2 = frame.evalWithBindings("exc instanceof SyntaxError", {exc: result.throw});
-    assertEq(result2.return, true);
+    frame.eval("args = arguments");
 };
-g.f();
\ No newline at end of file
+g.f(9, 8, 7);
+
+assertEq(g.args.length, 3);
+assertEq(g.args[2], 7);
\ No newline at end of file
diff --git a/js/src/jit-test/tests/arguments/rest-disallow-arguments.js b/js/src/jit-test/tests/arguments/rest-disallow-arguments.js
deleted file mode 100644
index ee1ff0e1b..000000000
--- a/js/src/jit-test/tests/arguments/rest-disallow-arguments.js
+++ /dev/null
@@ -1,30 +0,0 @@
-load(libdir + "asserts.js");
-var ieval = eval;
-
-// Now for a tour of the various ways you can access arguments.
-assertThrowsInstanceOf(function () {
-    ieval("function x(...rest) { arguments; }");
-}, SyntaxError)
-assertThrowsInstanceOf(function () {
-    Function("...rest", "arguments;");
-}, SyntaxError);
-assertThrowsInstanceOf(function (...rest) {
-    eval("arguments;");
-}, SyntaxError);
-assertThrowsInstanceOf(function (...rest) {
-    eval("arguments = 42;");
-}, SyntaxError);
-
-function g(...rest) {
-    assertThrowsInstanceOf(h, Error);
-}
-function h() {
-    g.arguments;
-}
-g();
-
-// eval() is evil, but you can still use it with rest parameters!
-function still_use_eval(...rest) {
-    eval("x = 4");
-}
-still_use_eval();
\ No newline at end of file
diff --git a/js/src/jit-test/tests/arguments/rest-with-arguments.js b/js/src/jit-test/tests/arguments/rest-with-arguments.js
new file mode 100644
index 000000000..f6e029f12
--- /dev/null
+++ b/js/src/jit-test/tests/arguments/rest-with-arguments.js
@@ -0,0 +1,45 @@
+// 'arguments' is allowed with rest parameters.
+
+// FIXME: We should create an unmapped arguments object in this case,
+// see bug 1175394. This test is not correct until then.
+
+var args;
+
+function restWithArgs(a, b, ...rest) {
+    return arguments;
+}
+
+args = restWithArgs(1, 3, 6, 9);
+assertEq(args.length, 4);
+assertEq(JSON.stringify(args), '{"0":1,"1":3,"2":[6,9],"3":9}',
+	 "Did you just fix bug 1175394?");
+
+args = restWithArgs();
+assertEq(args.length, 0);
+
+args = restWithArgs(4, 5);
+assertEq(args.length, 2);
+assertEq(JSON.stringify(args), '{"0":4,"1":5}');
+
+function restWithArgsEval(a, b, ...rest) {
+    return eval("arguments");
+}
+
+args = restWithArgsEval(1, 3, 6, 9);
+assertEq(args.length, 4);
+assertEq(JSON.stringify(args), '{"0":1,"1":3,"2":[6,9],"3":9}',
+	 "Did you just fix bug 1175394?");
+
+function g(...rest) {
+    h();
+}
+function h() {
+    g.arguments;
+}
+g();
+
+// eval() is evil, but you can still use it with rest parameters!
+function still_use_eval(...rest) {
+    eval("x = 4");
+}
+still_use_eval();
diff --git a/js/src/jit-test/tests/arrow-functions/arguments-1.js b/js/src/jit-test/tests/arrow-functions/arguments-1.js
index a4a4765ee..1bd8bc0cd 100644
--- a/js/src/jit-test/tests/arrow-functions/arguments-1.js
+++ b/js/src/jit-test/tests/arrow-functions/arguments-1.js
@@ -1,13 +1,5 @@
-// arrow functions have an 'arguments' binding, like any other function
+// no 'arguments' binding in arrow functions
 
 var arguments = [];
 var f = () => arguments;
-var args = f();
-assertEq(args.length, 0);
-assertEq(Object.getPrototypeOf(args), Object.prototype);
-
-args = f(true, false);
-assertEq(args.length, 2);
-assertEq(args[0], true);
-assertEq(args[1], false);
-
+assertEq(f(), arguments);
diff --git a/js/src/jit-test/tests/arrow-functions/arguments-2.js b/js/src/jit-test/tests/arrow-functions/arguments-2.js
index 99f1af51a..d6ad23ce8 100644
--- a/js/src/jit-test/tests/arrow-functions/arguments-2.js
+++ b/js/src/jit-test/tests/arrow-functions/arguments-2.js
@@ -1,14 +1,9 @@
-// 'arguments' in arrow functions nested in other functions
+// 'arguments' is lexically scoped in arrow functions
 
-var g;
+var args, g;
 function f() {
     g = () => arguments;
+    args = arguments;
 }
 f();
-var args = g();
-assertEq(args.length, 0);
-
-args = g(1, 2, 3);
-assertEq(args.length, 3);
-assertEq(args[0], 1);
-assertEq(args[2], 3);
+assertEq(g(), args);
diff --git a/js/src/jit-test/tests/arrow-functions/arguments-3.js b/js/src/jit-test/tests/arrow-functions/arguments-3.js
index 6460eecb5..a86cfdbbc 100644
--- a/js/src/jit-test/tests/arrow-functions/arguments-3.js
+++ b/js/src/jit-test/tests/arrow-functions/arguments-3.js
@@ -1,13 +1,17 @@
-// the 'arguments' binding in an arrow function is visible in direct eval code
+// 'arguments' in eval
 
 function f() {
-    return s => eval(s);
+    var g = s => eval(s);
+    assertEq(g("arguments"), arguments);
 }
 
-var g = f();
-var args = g("arguments");
-assertEq(typeof args, "object");
-assertEq(args !== g("arguments"), true);
-assertEq(args.length, 1);
-assertEq(args[0], "arguments");
+f();
+f(0, 1, 2);
+ 
+function h() {
+    return s => eval(s);
+}
+var result = h(1, 2, 3, 4)("arguments");
+assertEq(result.length, 4);
+assertEq(result[3], 4);
 
diff --git a/js/src/jit-test/tests/arrow-functions/arguments-4.js b/js/src/jit-test/tests/arrow-functions/arguments-4.js
index 4e337a6d0..e94acac4c 100644
--- a/js/src/jit-test/tests/arrow-functions/arguments-4.js
+++ b/js/src/jit-test/tests/arrow-functions/arguments-4.js
@@ -1,14 +1,22 @@
-// 'arguments' is banned in a function with a rest param,
-// even nested in an arrow-function parameter default value
-
 load(libdir + "asserts.js");
 
-var mistakes = [
-    "(...rest) => arguments",
-    "(...rest) => (x=arguments) => 0",
-    "function f(...rest) { return (x=arguments) => 0; }",
-    "function f(...rest) { return (x=(y=arguments) => 1) => 0; }",
-];
+// 'arguments' is allowed in a non-arrow-function with a rest param.
+assertEq((function(...rest) { return (x => arguments)(1, 2)})().length, 0);
+
+function restAndArgs(...rest) {
+    return () => eval("arguments");
+}
+
+var args = restAndArgs(1, 2, 3)();
+assertEq(args.length, 3);
+assertDeepEq(args[0], [1, 2, 3], "This is bogus, see bug 1175394");
+assertEq(args[1], 2);
+assertEq(args[2], 3);
 
-for (var s of mistakes)
-    assertThrowsInstanceOf(function () { eval(s); }, SyntaxError);
+(function() {
+    return ((...rest) => {
+        assertDeepEq(rest, [1, 2, 3]);
+        assertEq(arguments.length, 2);
+        assertEq(eval("arguments").length, 2);
+    })(1, 2, 3);
+})(4, 5);
diff --git a/js/src/jit-test/tests/arrow-functions/bug-885067-2.js b/js/src/jit-test/tests/arrow-functions/bug-885067-2.js
index e90ee0301..a45bdccb8 100644
--- a/js/src/jit-test/tests/arrow-functions/bug-885067-2.js
+++ b/js/src/jit-test/tests/arrow-functions/bug-885067-2.js
@@ -1,4 +1,28 @@
+// deoptimize `arguments` in the arrow's closest enclosing non-arrow-function
+
+// non-arrow-function -> arrow function
+a = 0;
 (function() {
-    a = (b => eval("arguments"))();
+    a = (() => eval("arguments"))();
 })(1, 2, 3, 4);
-assertEq(a.length, 0);
+assertEq(a.length, 4);
+
+// non-arrow-function -> arrow function -> arrow function
+a = 0;
+(function() {
+    (() => {
+        a = (() => eval("arguments"))();
+    })();
+})(1, 2, 3, 4);
+assertEq(a.length, 4);
+
+// non-arrow-function -> arrow function -> non-arrow-function -> arrow function
+a = 0;
+(function() {
+    (() => {
+        (function () {
+            a = (() => eval("arguments"))();
+        })(1, 2, 3, 4);
+    })();
+})();
+assertEq(a.length, 4);
diff --git a/js/src/jit-test/tests/basic/spread-call-length.js b/js/src/jit-test/tests/basic/spread-call-length.js
index 937eea780..3356c913a 100644
--- a/js/src/jit-test/tests/basic/spread-call-length.js
+++ b/js/src/jit-test/tests/basic/spread-call-length.js
@@ -46,8 +46,6 @@ function checkLength(f, makeFn) {
 
 checkLength(function(x) arguments.length, makeCall);
 checkLength(function(x) arguments.length, makeFunCall);
-checkLength((x) => arguments.length, makeCall);
-checkLength((x) => arguments.length, makeFunCall);
 function lengthClass(x) {
   this.length = arguments.length;
 }
diff --git a/js/src/jit/IonBuilder.cpp b/js/src/jit/IonBuilder.cpp
index 0897cb95d..a88a2647b 100644
--- a/js/src/jit/IonBuilder.cpp
+++ b/js/src/jit/IonBuilder.cpp
@@ -9129,6 +9129,15 @@ IonBuilder::jsop_arguments()
 bool
 IonBuilder::jsop_rest()
 {
+    if (info().analysisMode() == Analysis_ArgumentsUsage) {
+        // There's no BaselineScript with the template object. Just push a
+        // dummy value, it does not affect the arguments analysis.
+        MUnknownValue* unknown = MUnknownValue::New(alloc());
+        current->add(unknown);
+        current->push(unknown);
+        return true;
+    }
+
     ArrayObject* templateObject = &inspector->getTemplateObject(pc)->as<ArrayObject>();
 
     if (inliningDepth_ == 0) {
diff --git a/js/src/js.msg b/js/src/js.msg
index 4e64c4c0a..9be978fe7 100644
--- a/js/src/js.msg
+++ b/js/src/js.msg
@@ -131,7 +131,6 @@ MSG_DEF(JSMSG_BAD_APPLY_ARGS,          1, JSEXN_TYPEERR, "second argument to Fun
 MSG_DEF(JSMSG_BAD_FORMAL,              0, JSEXN_SYNTAXERR, "malformed formal parameter")
 MSG_DEF(JSMSG_CALLER_IS_STRICT,        0, JSEXN_TYPEERR, "access to strict mode caller function is censored")
 MSG_DEF(JSMSG_DEPRECATED_USAGE,        1, JSEXN_REFERENCEERR, "deprecated {0} usage")
-MSG_DEF(JSMSG_FUNCTION_ARGUMENTS_AND_REST, 0, JSEXN_TYPEERR, "the 'arguments' property of a function with a rest parameter may not be used")
 MSG_DEF(JSMSG_NOT_SCRIPTED_FUNCTION,   1, JSEXN_TYPEERR, "{0} is not a scripted function")
 MSG_DEF(JSMSG_NO_REST_NAME,            0, JSEXN_SYNTAXERR, "no parameter name after ...")
 MSG_DEF(JSMSG_PARAMETER_AFTER_REST,    0, JSEXN_SYNTAXERR, "parameter after rest parameter")
@@ -173,7 +172,6 @@ MSG_DEF(JSMSG_UNKNOWN_FORMAT,          1, JSEXN_INTERNALERR, "unknown bytecode f
 
 // Frontend
 MSG_DEF(JSMSG_ACCESSOR_WRONG_ARGS,     3, JSEXN_SYNTAXERR, "{0} functions must have {1} argument{2}")
-MSG_DEF(JSMSG_ARGUMENTS_AND_REST,      0, JSEXN_SYNTAXERR, "'arguments' object may not be used in conjunction with a rest parameter")
 MSG_DEF(JSMSG_ARRAY_COMP_LEFTSIDE,     0, JSEXN_SYNTAXERR, "invalid array comprehension left-hand side")
 MSG_DEF(JSMSG_ARRAY_INIT_TOO_BIG,      0, JSEXN_INTERNALERR, "array initialiser too large")
 MSG_DEF(JSMSG_AS_AFTER_RESERVED_WORD,  1, JSEXN_SYNTAXERR, "missing keyword 'as' after reserved word '{0}'")
diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
index 14295f6f2..ec12b91e0 100644
--- a/js/src/jsfun.cpp
+++ b/js/src/jsfun.cpp
@@ -128,14 +128,6 @@ ArgumentsRestrictions(JSContext* cx, HandleFunction fun)
         return false;
     }
 
-    // Functions with rest arguments don't include a local |arguments| binding.
-    // Similarly, "arguments" shouldn't work on them.
-    if (fun->hasRest()) {
-        JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr,
-                             JSMSG_FUNCTION_ARGUMENTS_AND_REST);
-        return false;
-    }
-
     // Otherwise emit a strict warning about |f.arguments| to discourage use of
     // this non-standard, performance-harmful feature.
     if (!JS_ReportErrorFlagsAndNumber(cx, JSREPORT_WARNING | JSREPORT_STRICT, js_GetErrorMessage,
diff --git a/js/src/vm/Interpreter.cpp b/js/src/vm/Interpreter.cpp
index bf52d3db7..690b38182 100644
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -2893,7 +2893,6 @@ CASE(JSOP_TABLESWITCH)
 }
 
 CASE(JSOP_ARGUMENTS)
-    MOZ_ASSERT(!REGS.fp()->fun()->hasRest());
     if (!script->ensureHasAnalyzedArgsUsage(cx))
         goto error;
     if (script->needsArgsObj()) {
diff --git a/js/src/vm/Xdr.h b/js/src/vm/Xdr.h
index 13c3431b2..c7a350e50 100644
--- a/js/src/vm/Xdr.h
+++ b/js/src/vm/Xdr.h
@@ -29,11 +29,11 @@ namespace js {
  *
  *  https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode
  */
-static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 250;
+static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 251;
 static const uint32_t XDR_BYTECODE_VERSION =
     uint32_t(0xb973c0de - XDR_BYTECODE_VERSION_SUBTRAHEND);
 
-static_assert(JSErr_Limit == 367,
+static_assert(JSErr_Limit == 365,
               "GREETINGS, POTENTIAL SUBTRAHEND INCREMENTER! If you added or "
               "removed MSG_DEFs from js.msg, you should increment "
               "XDR_BYTECODE_VERSION_SUBTRAHEND and update this assertion's "