Last Comment Bug 852762 - Arrow functions are not automatically strict after all
: Arrow functions are not automatically strict after all
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla24
Assigned To: Jason Orendorff [:jorendorff]
: general
Mentors:
Depends on: 889158
Blocks: 846406
  Show dependency treegraph
 
Reported: 2013-03-19 16:10 PDT by Jason Orendorff [:jorendorff]
Modified: 2013-07-08 19:01 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (6.52 KB, patch)
2013-03-22 01:11 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
v2 - add another test or two (7.81 KB, patch)
2013-03-27 12:12 PDT, Jason Orendorff [:jorendorff]
ejpbruel: review+
Details | Diff | Review

Description Jason Orendorff [:jorendorff] 2013-03-19 16:10:19 PDT
There is also the question of whether they have arguments, which I'll have to figure out from the meeting notes. This bug can address both.

<brendan> rs=me on arguments :-P
Comment 1 Rick Waldron [:rwaldron] 2013-03-19 17:52:38 PDT
In case any future readers are interested, the resolution of record for this decision is here:

https://mail.mozilla.org/pipermail/es-discuss/2013-February/028632.html

(Formatted: https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-01/jan-30.md)
Comment 2 Jason Orendorff [:jorendorff] 2013-03-22 01:11:33 PDT
Created attachment 728097 [details] [diff] [review]
v1
Comment 3 Jason Orendorff [:jorendorff] 2013-03-27 12:12:59 PDT
Created attachment 730273 [details] [diff] [review]
v2 - add another test or two
Comment 4 Jason Orendorff [:jorendorff] 2013-04-15 08:36:27 PDT
Review ping! :)
Comment 5 Eddy Bruel [:ejpbruel] 2013-04-22 06:07:48 PDT
Comment on attachment 730273 [details] [diff] [review]
v2 - add another test or two

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

Looks mostly good to me. r+ with comments addressed.

::: js/src/jit-test/tests/arrow-functions/arguments-2.js
@@ +1,1 @@
> +// 'arguments' in arrow functions nested in other functions

Based on what this test does, this means that arrow functions get their own instance of arguments, right? That wasn't clear to me from this comment.

::: js/src/jit-test/tests/arrow-functions/arguments-4.js
@@ +10,3 @@
>  ];
>  
> +

Spurious newline here

::: js/src/jit-test/tests/arrow-functions/strict-1.js
@@ +10,3 @@
>  
> +f = (a = {x: 1, x: 2}) => b => { "use strict"; return a.x; };
> +assertEq(f()(0), 2);

I'm not sure what this tests for. Should the inner => be strict here? What about the outer =>? And how does this assertion confirm that?

::: js/src/jit-test/tests/arrow-functions/strict-2.js
@@ +2,5 @@
>  
>  load(libdir + "asserts.js");
>  
>  assertThrowsInstanceOf(
> +    () => Function("(a = function (obj) { with (obj) f(); }) => { 'use strict'; }"),

Why do we need the outer () => Function(...) here? I would assume this test throws even without that.

::: js/src/jit-test/tests/arrow-functions/syntax-errors.js
@@ +14,5 @@
>      "{p} => p",
>      "(...x => expr)",
>      "1 || a => a",
> +    "'use strict' => {}",
> +    "package => {'use strict';}",    // tricky: FutureReservedWord in strict mode code only

How do we make sure that this only throws a syntax error in strict mode? And if we don't, could you file a followup bug for this?
Comment 6 David Bruant 2013-06-06 13:45:17 PDT
I added a note on MDN about this bug [1] Feel free to remove it when this bug is fixed.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/arrow_functions#Upcoming_change
Comment 7 Jason Orendorff [:jorendorff] 2013-06-20 21:40:22 PDT
(In reply to Eddy Bruel [:ejpbruel] from comment #5)
> > +// 'arguments' in arrow functions nested in other functions
> 
> Based on what this test does, this means that arrow functions get their own
> instance of arguments, right? That wasn't clear to me from this comment.

Yes, they do. I improved the comments in several of the tests.

> ::: js/src/jit-test/tests/arrow-functions/strict-1.js
> @@ +10,3 @@
> >  
> > +f = (a = {x: 1, x: 2}) => b => { "use strict"; return a.x; };
> > +assertEq(f()(0), 2);
> 
> I'm not sure what this tests for. Should the inner => be strict here? What
> about the outer =>? And how does this assertion confirm that?

Yes; no; the fact that it compiles and runs confirms that the outer arrow is non-strict. I only check the return value because why not.

> ::: js/src/jit-test/tests/arrow-functions/strict-2.js
> @@ +2,5 @@
> >  
> >  load(libdir + "asserts.js");
> >  
> >  assertThrowsInstanceOf(
> > +    () => Function("(a = function (obj) { with (obj) f(); }) => { 'use strict'; }"),
> 
> Why do we need the outer () => Function(...) here? I would assume this test
> throws even without that.

Another way to write this test would be:

    // |jit-test| error: SyntaxError
    (a = function (obj) { with (obj) f(); }) => { "use strict"; };

Using assertThrowsInstanceOf requires putting the test case in quotes and using ()=>Function(...) to parse it on demand. Otherwise the error would be thrown at parse time, before assertThrowsInstanceOf is ever called, so the test would fail.

> ::: js/src/jit-test/tests/arrow-functions/syntax-errors.js
> > +    "package => {'use strict';}",    // tricky: FutureReservedWord in strict mode code only
> 
> How do we make sure that this only throws a syntax error in strict mode? And
> if we don't, could you file a followup bug for this?

Added a test for this (in the same file).
Comment 8 Jason Orendorff [:jorendorff] 2013-06-21 08:48:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d580712ec0
Comment 9 Phil Ringnalda (:philor) 2013-06-21 19:40:03 PDT
https://hg.mozilla.org/mozilla-central/rev/60d580712ec0

Note You need to log in before you can comment on or make changes to this bug.