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

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: jandem)

Tracking

(Blocks: 2 bugs, {dev-doc-complete, site-compat})

unspecified
mozilla43
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 852762
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?  :-(
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.
(Reporter)

Comment 3

6 years ago
Sigh.
(Assignee)

Updated

5 years ago
Depends on: 989204
(Reporter)

Comment 4

5 years ago
Morphing, since jandem is fixing the issue this bug was originally about.
Blocks: 694100
Summary: arguments.callee is incorrect for arrow functions → Calling an arrow function shouldn't create an 'arguments' binding
Tentatively taking, my bugmail happened to mention this and I got nerd-sniped.  Patch almost in hand, in theory.
Assignee: general → jwalden+bmo
Posted patch Partial patch (obsolete) — Splinter Review
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.
Duplicate of this bug: 914695
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]

Comment 8

4 years ago
I will try to implement this today.
Assignee: jwalden+bmo → 446240525

Comment 9

4 years ago
Posted patch bug-889158-v1.patch (obsolete) — Splinter Review
Attachment #8401013 - Attachment is obsolete: true
Attachment #8534172 - Flags: review?(jwalden+bmo)
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
(Reporter)

Comment 11

4 years ago
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.
Attachment #8534172 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 12

4 years ago
ziyunfei, can we get this landed? Let us know if you need any help.
Blocks: 1177735
Flags: needinfo?(446240525)
(Assignee)

Comment 13

4 years ago
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.
(Assignee)

Comment 14

4 years ago
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...
(Assignee)

Comment 15

4 years ago
(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...
Flags: needinfo?(luke)
(Assignee)

Comment 16

4 years ago
(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.
(Assignee)

Updated

4 years ago
Depends on: 1187021
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);
(Assignee)

Comment 18

4 years ago
Posted patch PatchSplinter Review
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.
Assignee: 446240525 → jdemooij
Attachment #8534172 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8638131 - Flags: review?(jorendorff)
(Assignee)

Comment 19

4 years ago
(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.
Jan: if there isn't any big benchmark hit, removing the complexity seems fine to me.
Flags: needinfo?(luke)
(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 ?
(Assignee)

Comment 22

4 years ago
(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..
Blocks: 1133298
(Reporter)

Comment 23

4 years ago
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.
Attachment #8638131 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 24

4 years ago
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

4 years ago
> 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?
(Assignee)

Comment 26

4 years ago
(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

4 years ago
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

4 years ago
of *such* programs.
(Assignee)

Comment 29

4 years ago
(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...
Flags: needinfo?(446240525)
(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;`.
(Assignee)

Comment 31

4 years ago
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.
Attachment #8649806 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Depends on: 1196202
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.
Attachment #8649806 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Updated

4 years ago
Keywords: leave-open
(Assignee)

Updated

4 years ago
Depends on: 1197787
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
(Assignee)

Comment 36

4 years ago
(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...
(Assignee)

Updated

4 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/7d70643818b5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Attachment #8652816 - Flags: review?(jdarcangelo)
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.
Attachment #8652816 - Flags: review?(jdarcangelo) → review+

Comment 41

4 years ago
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
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.
You need to log in before you can comment on or make changes to this bug.