889158 - Calling an arrow function shouldn't create an 'arguments' binding

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Discovered by :arai in bug 762363.

var f = x => arguments.callee;
assertEq(f(), f); // FAILS

This exposes the unbound arrow function object to script, which is incorrect (i.e. contrary to the spec) but harmless from a security standpoint--it's just a normal function.

Comment 1

[Jeff Walden [:Waldo]]

This is contrary to how Function.prototype.bind'd functions work.

  var f = function f() { return arguments.callee; }; var bf = f.bind(); bf() == f

Why oh why is there this insistence on making arrow functions more than syntactic sugar, and unlike anything else in the language?  :-(

Comment 2

[Jeff Walden [:Waldo]]

The claim on es-discuss today in https://mail.mozilla.org/pipermail/es-discuss/2013-July/031885.html is that arrow functions don't have an |arguments| binding at all.  Joy.

Comment 3

[Jason Orendorff [:jorendorff]]

Sigh.

Comment 4

[Jason Orendorff [:jorendorff]]

Morphing, since jandem is fixing the issue this bug was originally about.

Comment 5

[Jeff Walden [:Waldo]]

Tentatively taking, my bugmail happened to mention this and I got nerd-sniped.  Patch almost in hand, in theory.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Partial patch

I think this solves things *except* for two cases: where a JSOP_NAME is used to access an outer |arguments| binding that's still JS_OPTIMIZED_ARGUMENTS, and (I think, just going by mental inspection) where an arrow function nested in an arrow function accesses an outer function's JS_OPTIMIZED_ARGUMENTS.  Still working through the first one.  Only barely thought about the second.  But enough of a start for now, and might be worth poking people now-absent about techniques here tomorrow.

Comment 8

[ziyunfei]

I will try to implement this today.

Comment 9

[ziyunfei]

Attachment: bug-889158-v1.patch

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 8534172 [details] [diff] [review]
bug-889158-v1.patch

>+    // Arrow functions do not have an `arguments` binding, so we construct
>+    // the `arguments` of it's closest enclosing non-arrow-function.
>+    if (fun->isArrow()) {
>+        RootedObject scope(cx, script->enclosingStaticScope());
>+        if (scope && scope->is<JSFunction>()) {
>+            RootedFunction enclosingFunction(cx, scope.as<JSFunction>());
>+            if (!enclosingFunction)
>+                return false;
>+            if (!enclosingFunction->isArrow()) {
>+                if (enclosingFunction->hasRest()) {
>+                    parser.report(ParseError, false, nullptr, JSMSG_ARGUMENTS_AND_REST);
>+                    return false;
>+                }
>+                RootedScript outerScript(cx, enclosingFunction->getOrCreateScript(cx));
>+                if (!outerScript)
>+                    return false;
>+                if (!JSScript::argumentsOptimizationFailed(cx, outerScript))
>+                    return false;
>+                return true;
>+            } else {

No else after return non-sequiturs, please.

>+                // Recursively find the first non-arrow-function
>+                return CheckArgumentsWithinEval(cx, parser, enclosingFunction);
>+            }
>+        }

The question of `arguments` (and `this`) in top-level arrows came up today:

https://esdiscuss.org/topic/clarification-regarding-top-level-arrow-functions-and-this-arguments

prompting my drive-by. Hope this patch can get into landing shape soon. Thanks for working on it!

/be

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 8534172 [details] [diff] [review]
bug-889158-v1.patch

Review of attachment 8534172 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review. If this patch still applies, let's get it landed.

r=me with these comments addressed. But if you end up having to change the patch because of the static scope chain stuff, please post a new patch, r?me.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +71,3 @@
>          parser.report(ParseError, false, nullptr, JSMSG_ARGUMENTS_AND_REST);
>          return false;
>      }

As far as I can tell, the spec no longer forbids `arguments` in functions that have rest parameters. So this block can just be deleted.

If you'd like to keep it and just land what you've got so far, that's fine. But please delete the extra space at the end of the comment line ending with "that".

@@ +78,5 @@
>      if (!script)
>          return false;
> +
> +    // Arrow functions do not have an `arguments` binding, so we construct
> +    // the `arguments` of it's closest enclosing non-arrow-function.

Spelling: The possessive is "its", not "it's", but it would be better to change "it's" to "the" here.

@@ +81,5 @@
> +    // Arrow functions do not have an `arguments` binding, so we construct
> +    // the `arguments` of it's closest enclosing non-arrow-function.
> +    if (fun->isArrow()) {
> +        RootedObject scope(cx, script->enclosingStaticScope());
> +        if (scope && scope->is<JSFunction>()) {

What happens if the enclosing static scope is a block scope or named-lambda-binding scope?

    var f = function f(s) { // scope containing only 'f'
        { // block scope containing 'x'
            let x;
            return () => eval(s);
        }
    }
    f("arguments");

Please add a test!

::: js/src/frontend/Parser.cpp
@@ +1097,5 @@
>      }
>  
> +    if (kind != Arrow) {
> +        /* Define the 'arguments' binding if necessary. Arrow functions don't have 'arguments'. */
> +        if (!checkFunctionArguments())

Arrow functions can't be parsed lazily yet, but I agree it's good to add this now anyway.

::: js/src/jit-test/tests/arrow-functions/arguments-3.js
@@ +2,4 @@
>  
>  function f() {
> +    var g = s => eval(s);
> +    assertEq(g("arguments"), arguments);

Please add a test that does the same thing, but without mentioning `arguments` inside the function:

    function f() {
        return s => eval(s);
    }
    var result = f()("arguments", 2, 3, 4);
    assertEq(result.length, 4);
    assertEq(result[3], 4);

::: js/src/jit-test/tests/arrow-functions/arguments-4.js
@@ +1,4 @@
>  load(libdir + "asserts.js");
>  
> +// 'arguments' is banned in a non-arrow-function with a rest param, even nested in an arrow-function
> +assertThrowsInstanceOf(function(){eval('(function(...rest) {(x => arguments)()})()')}, SyntaxError);

Please simplify the syntax to help convince the reader that the SyntaxError isn't just a typo. :)

    assertThrowsInstanceOf(() => Function('(function(...rest) {(x => arguments)()})'), SyntaxError);

    function f(...rest) { return x => eval("arguments"); }
    assertThrowsInstanceOf(f, SyntaxError);

::: js/src/jit-test/tests/arrow-functions/bug889158.js
@@ +2,2 @@
>  var f = x => arguments.callee;
> +f();

It's fine to just delete this test.

Comment 12

[Jan de Mooij [:jandem]]

ziyunfei, can we get this landed? Let us know if you need any help.

Comment 13

[Jan de Mooij [:jandem]]

ziyunfei, do you think you'll be able to finish this bug, soonish?

Else I can probably steal this, we need to fix this ASAP before a lot of code depends on our non-standard behavior here. Especially a risk for add-ons and chrome code...

There's also the perf issue in bug 1177518, but that's less urgent.

Comment 14

[Jan de Mooij [:jandem]]

Hm I'm rebasing this patch atm. There's at least one annoying problem:

  function f() {
    return () => eval("arguments");
  }
  var g = f();
  g();

When we do the eval, f's frame is no longer on the stack, so JSScript::argumentsOptimizationFailed won't create an arguments object for the arrow function.

Not sure what the right fix is. Fixing argumentsOptimizationFailed to handle this case is a bit annoying. The alternative is to always create an arguments object for functions that have an arrow function that uses eval...

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #14)
> Not sure what the right fix is. Fixing argumentsOptimizationFailed to handle
> this case is a bit annoying. The alternative is to always create an
> arguments object for functions that have an arrow function that uses eval...

I wonder if we should revert bug 842522, "Don't force construction of arguments objects on dynamic name accesses". As that bug mentions, at the time Ion was unable to compile functions that required non-lazy arguments. This is no longer true. Furthermore, with GGC and some other optimizations I did last month, creating arguments objects is a lot faster.

Luke, since Brian is on PTO (you too, but I think you check bugmail..), does this seem reasonable? I think it'd allow us to remove some complicated code and it should help here.

I'll check what this does to date-format-tofte...

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #15)
> I'll check what this does to date-format-tofte...

It seems to be at most a 0.5 ms slowdown there (6.5 -> 7.0 ms) and we can remove 200+ lines of code from the frontend and JIT.

So the question is if there are other cases where it'd hurt to create an arguments object when direct eval is used. Considering direct eval already kills performance in a lot of ways, and that it's pretty well-known, I think it's fair to go ahead with this.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 8534172 [details] [diff] [review]
bug-889158-v1.patch

Review of attachment 8534172 [details] [diff] [review]:
-----------------------------------------------------------------

Blah, I didn't even realize someone had swiped the review here.  :-\

Back a long while ago (like, last December or something) I started looking into this and quickly stumbled across the issue in comment 14.  My conclusion was that the only real way to fix this was to eagerly create the arguments object: inside any non-arrow function, before evaluating an arrow function that contained (at any level of arrow-function-nesting, until an |arguments| binding was observed in an intervening scope) a direct eval.  I *think* that covers all the bases, but I could have missed something.  Whatever patch is produced here, probably should be dwarfed by the number of tests accompanying it.

This obviously required deep bytecode and analysis hackery.  And then I got distracted by other things and never got back to it.  The patch here doesn't do anything like this, sadly -- it's assuming everything is live and visible to it when it operates, which isn't the case.  But that seems like the only principled way to do this, to me, that doesn't involve a long trail of tears of regressions and missed places needing changes.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +81,5 @@
> +    // Arrow functions do not have an `arguments` binding, so we construct
> +    // the `arguments` of it's closest enclosing non-arrow-function.
> +    if (fun->isArrow()) {
> +        RootedObject scope(cx, script->enclosingStaticScope());
> +        if (scope && scope->is<JSFunction>()) {

Also, what if the |arguments| is a block-scoped variable?

var f = function f(s) { // scope for "f"
  {
    let arguments = 42;
    return () => eval(s);
  }
};
assertEq(f("arguments"), 42);

And what if the enclosing static scope *isn't* the one containing the binding?

var f = function f(s) { // scope for "f"
  {
    let arguments = 17;
    return x => y => eval(s);
  }
};
assertEq(f("arguments"), 17);

And and and...

@@ +98,5 @@
> +                    return false;
> +                return true;
> +            } else {
> +                // Recursively find the first non-arrow-function
> +                return CheckArgumentsWithinEval(cx, parser, enclosingFunction);

Surely, I think, we can make this into a scope-walking loop and avoid actual recursion, right?  This is clearly tail position and all.

@@ +107,4 @@
>      if (script->argumentsHasVarBinding()) {
>          if (!JSScript::argumentsOptimizationFailed(cx, script))
>              return false;
>      }

assertEq((code => { var arguments = 42; return eval(code); })("arguments"), 42);

var f = s => { // scope for "s"
  let arguments = 17;
  {
    let createScope = true;
    return () => eval(s);
  }
};
assertEq(f("arguments")(), 42);

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Patch

Rebased patch. This patch also allows functions with a rest parameter to have an arguments binding. With that + bug 1187021 I was able to avoid the static scope walk in CheckArgumentsWithinEval :)

There's on wrinkle: the arguments object should be unmapped (strict) in this case, that's bug 1175394. I can fix that bug as well, but one step at a time.

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> Also, what if the |arguments| is a block-scoped variable?

I *think* my patch(es) address all your comments by eliminating this static scope walk.

Also feel free to steal this review, I didn't see your comment before uploading the patch.

Comment 20

[Luke Wagner [:luke]]

Jan: if there isn't any big benchmark hit, removing the complexity seems fine to me.

Comment 21

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #18)
> This patch also allows functions with a rest parameter to have an arguments binding.

I haven't read the patch, but I assume this will resolve bug 1133298 ?

Comment 22

[Jan de Mooij [:jandem]]

(In reply to André Bargull from comment #21)
> I haven't read the patch, but I assume this will resolve bug 1133298 ?

Yes indeed. We'll still have to fix bug 1175394 though..

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 8638131 [details] [diff] [review]
Patch

Review of attachment 8638131 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/arguments/rest-debugger.js
@@ +8,5 @@
>  var g = newGlobal();
>  g.eval("function f(...rest) { debugger; }");
>  var dbg = Debugger(g);
>  dbg.onDebuggerStatement = function (frame) {
> +    args = frame.eval("args = arguments");

`args =` is useless here, right?

::: js/src/jit-test/tests/arrow-functions/arguments-4.js
@@ +16,5 @@
> +(function() {
> +    return ((...rest) => {
> +        assertDeepEq(rest, [1, 2, 3]);
> +        assertDeepEq(arguments.length, 2);
> +        assertDeepEq(eval("arguments").length, 2);

Just assertEq would be fine for the last two.

Comment 24

[Jan de Mooij [:jandem]]

So according to Try there's a fair number of places in our codebase and tests where we rely on the current behavior :(

Will post a separate patch to fix those up. Hopefully it won't break too many addons, we really need to land this ASAP.

Comment 25

[Mark S. Miller]

> So according to Try there's a fair number of places in our codebase and tests where 
> we rely on the current behavior :(

Just curious: What are these and why did they occur? Is there a typical example?

Comment 26

[Jan de Mooij [:jandem]]

(In reply to Mark S. Miller from comment #25)
> > So according to Try there's a fair number of places in our codebase and tests where 
> > we rely on the current behavior :(
> 
> Just curious: What are these and why did they occur? Is there a typical
> example?

Here's a patch to fix a number of them:

https://hg.mozilla.org/try/rev/4f7c8a931e3a

I'll see how much this helps. Tomorrow I also want to add some logging for arrow functions with arguments, that should hopefully tell us the remaining ones in the browser and tests.

Comment 27

[Mark S. Miller]

At https://hg.mozilla.org/try/rev/4f7c8a931e3a#l5.12 the function in question mentions "this". Merely changing from an arrow function to a "function" function will generally change the meaning of programs.

Comment 28

[Mark S. Miller]

of *such* programs.

Comment 29

[Jan de Mooij [:jandem]]

(In reply to Mark S. Miller from comment #27)
> At https://hg.mozilla.org/try/rev/4f7c8a931e3a#l5.12 the function in
> question mentions "this". Merely changing from an arrow function to a
> "function" function will generally change the meaning of programs.

Ugh, thanks. Time to bring back |var self = this| I guess...

Comment 30

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jan de Mooij [:jandem] from comment #29)
> (In reply to Mark S. Miller from comment #27)
> > At https://hg.mozilla.org/try/rev/4f7c8a931e3a#l5.12 the function in
> > question mentions "this". Merely changing from an arrow function to a
> > "function" function will generally change the meaning of programs.
> 
> Ugh, thanks. Time to bring back |var self = this| I guess...

Most of those look like they coud be fixed by s/arguments/args/ in the bodies and `(...args) => { ... }` which may be easier than `var self = this;`.

Comment 31

[Jan de Mooij [:jandem]]

Attachment: Fix chrome and test JS

I added some logging to the parser for arrows-with-arguments and this fixes all the places reported on a Try run.

Unfortunately also one hit in Gaia code that I'll have to fix separately.

Comment 32

[Jeff Walden [:Waldo]]

Comment on attachment 8649806 [details] [diff] [review]
Fix chrome and test JS

Review of attachment 8649806 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is an excellent example of why adding more function syntaxes to ES6 was a grave error.  Even once all browsers work the same, these subtle differences among the syntaxes are going to produce endless subtle errors going into the future.  It's like we took the worst idea from Perl and ignored the best sentiment from Python, because chattering classes were unhappy about one problem, so we added more syntax to solve it and gratuitously changed more things just to make everyone's debugging lives a living horror.

</rant>

Tryserver this for sure, this finicky detail sort of thing is easy to get wrong EVEN IF YOU KNOW THE SPECIFICATION.  *stab*  Not sure I have much confidence that I didn't miss something here, if there's something to miss.

</rant>

::: addon-sdk/source/test/context-menu/test-helper.js
@@ +29,5 @@
>  // items the same label.
>  function TestHelper(assert, done) {
>    // Methods on the wrapped test can be called on this object.
>    for (var prop in assert)
> +    this[prop] = (...args) => assert[prop].apply(assert, args);

Addon SDK changes are sort of supposed to happen upstream in a Github repo.  Sort of.  Last time I did that, they took my change, then said to just land on inbound anyway.  Who knows if that's policy with a capital P.

::: browser/base/content/test/general/head.js
@@ +722,2 @@
>              if (e.target == win.document) {
>                win.removeEventListener("unload", arguments.callee);

Make that |function listener| and s/arguments.callee/listener/.  This is a particularly dumb arguments use.

::: browser/components/places/tests/browser/browser_library_commands.js
@@ +59,5 @@
>       "Delete command is enabled");
>  
>    // Execute the delete command and check visit has been removed.
>    let promiseURIRemoved = promiseHistoryNotification("onDeleteURI",
> +                                                     (...args) => TEST_URI.equals(args[0]));

wut

Surely |v => TEST_URI.equals(v)|?

::: toolkit/devtools/event-emitter.js
@@ -91,2 @@
>        if (aListener) {
>          aListener.apply(null, arguments);

I'd add |, ...aRest| to the end of the arguments list, then s/arguments/[aEvent, aFirstArg, ...aRest]/, myself.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7b04b9e4f17

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a7b04b9e4f17

Comment 35

[Benjamin Bouvier [:bbouvier] (inactive)]

If I understand correctly this bug, this sub test of six-speed is wrong and needs fixing as well?

https://github.com/kpdecker/six-speed/blob/master/tests/arrow-args/arrow-args.es6

Comment 36

[Jan de Mooij [:jandem]]

(In reply to Benjamin Bouvier (PTO until August 24th) [:bbouvier] from comment #35)
> If I understand correctly this bug, this sub test of six-speed is wrong and
> needs fixing as well?
> 
> https://github.com/kpdecker/six-speed/blob/master/tests/arrow-args/arrow-
> args.es6

No the test is fine and SpiderMonkey is wrong. Right now we give arrow functions an arguments object (like other functions), but we should use the 'outer' arguments-object.

We used to do really bad on this test (that's why I took this bug) because we kept allocating new arguments objects. Now they also test correctness and just mark it as Incorrect.

Hopefully I can finally land this after the next b2g-inbound -> mozilla-inbound merge, assuming no new issues show up...

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d70643818b5

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d70643818b5

Comment 39

[GitHub Autolander Bot]

Attachment: [gaia] nbp:bugzil.la/889158 > mozilla-b2g:master

Comment 40

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8652816 [details] [review]
[gaia] nbp:bugzil.la/889158 > mozilla-b2g:master

Thanks for the patch! You will unfortunately have to rebase before landing this though. Since its a small patch, it shouldn't be too difficult though.

Comment 41

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/61c4f19be987bbb181e45bfa6597ba1a18acf68b
Bug 889158 part 1 - Fix chrome and tests to not use arrow functions with arguments. r=Waldo

Comment 42

[Florian Scholz (Open Web Docs)]

Review appreciated:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions#Use_of_arguments_object_or_yield_keyword
https://developer.mozilla.org/en-US/Firefox/Releases/43#JavaScript

Comment 43

[Jeff Walden [:Waldo]]

Tweaked the arrow function docs some -- seems best to describe arguments as just an inherited binding, not as something forbidden/prohibited/etc. and then walk that back semi-contradictorily.

Comment 44

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/ecmascript-arguments-implementation-has-been-updated/