1272784 - |function f(a = 1) { "use strict"; }| should throw Early Error.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

(separated from bug 1185106 comment #90)

https://tc39.github.io/ecma262/#sec-function-definitions-static-semantics-early-errors
> FunctionDeclaration:functionBindingIdentifier(FormalParameters){FunctionBody}
> FunctionDeclaration:function(FormalParameters){FunctionBody}
> FunctionExpression:functionBindingIdentifier(FormalParameters){FunctionBody}
> 
>   * It is a Syntax Error if ContainsUseStrict of FunctionBody is true and IsSimpleParameterList of FormalParameters is false.

The above rule means the following code should throw SyntaxError.

  function f(a = 1) { "use strict"; }

Comment 1

[Jason Orendorff [:jorendorff]]

I think the intent of the rule is implementation convenience: we can avoid rewinding when we encounter a "use strict" declaration.

ES6 allows this function. I think V8 never implemented it, which is why TC39 can drop the requirement. It'll be a relief to be rid of the rewinding code. This needs a declaration of "intent to un-ship" on dev.platform though.

Comment 2

[Tooru Fujisawa [:arai]]

ah, so, we can remove the following loop, right?

https://dxr.mozilla.org/mozilla-central/rev/a884b96685aa13b65601feddb24e5f85ba861561/js/src/frontend/Parser.cpp#2846
> template <typename ParseHandler>
> typename ParseHandler::Node
> Parser<ParseHandler>::functionDef(InHandling inHandling,
> ...
> {
> ...
>     while (true) {
>         if (functionArgsAndBody(inHandling, pn, fun, kind, generatorKind, directives,
>                                 &newDirectives))
>         {
>             break;
>         }
>         if (tokenStream.hadError() || directives == newDirectives)
>             return null();
> 
>         // Assignment must be monotonic to prevent reparsing iloops
>         MOZ_ASSERT_IF(directives.strict(), newDirectives.strict());
>         MOZ_ASSERT_IF(directives.asmJS(), newDirectives.asmJS());
>         directives = newDirectives;
> 
>         tokenStream.seek(start);
> 
>         // functionArgsAndBody may have already set pn->pn_body before failing.
>         handler.setFunctionBody(pn, null());
>     }
> 
>     return pn;
> }

Comment 3

[Jason Orendorff [:jorendorff]]

Bah, I guess not, because of "use asm".

But we can do less rewinding. Search in Parser.cpp for "Request that this function be reparsed as strict." I think we can change that to a simple check for the few possible strict errors that can occur in argument lists.

Comment 4

[Jason Orendorff [:jorendorff]]

This kind of rewinding causes parsing to take O(n^2) time:

    var x = "0";
    for (let i = 0; i < 300; i++)
        x = "(function (a = " + x + ") { 'use strict'; return a; })";
    var t0 = Date.now();
    eval(x);
    print(Date.now() - t0);

This takes half a second on my machine, which is rather silly.

But arrow function rewinding is even worse: it's O(2^n).

Comment 6

[André Bargull [:anba]]

Attachment: bug1272784.part1.patch

Part 1: Implement the new checks, but functions with "use strict" directives are still rewound.

Comment 7

[André Bargull [:anba]]

Attachment: bug1272784.part2.patch

Part 2: Don't rewind functions with "use strict" directives (except for error reporting).

Comment 8

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #1)
> This needs a declaration of "intent to un-ship" on dev.platform though.

Did this actually happen? FWIW Chrome, JSC (*) and Edge already implement the new semantics.

(*) I don't know if stable Safari contains the changes.

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8796263 [details] [diff] [review]
bug1272784.part1.patch

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

The code change itself looks good, but the change in the existing tests sounds scary, as it implies this can be hit also in add-ons or firefox-specific code on the web.
This at least needs add-on compatibility note.

Comment 10

[Tooru Fujisawa [:arai]]

adding some keywords.
site-compat might be inappropriate, feel free to clear it.

Comment 11

[Tooru Fujisawa [:arai]]

Have this passed try run?
wasn't there anything hit this in-tree?

Comment 12

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #11)
> Have this passed try run?
> wasn't there anything hit this in-tree?

I've only tried jstests + jit-tests. Still need to request access to Try (bug 1306713). :-D

Comment 13

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #9)
> The code change itself looks good, but the change in the existing tests
> sounds scary, as it implies this can be hit also in add-ons or
> firefox-specific code on the web.
> This at least needs add-on compatibility note.

We could also just warn for now and remove support at a later point.

Comment 14

[Tom S]

Please do a try run now :) If it passes I am for landing this, addon can be deal with a little breakage.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to André Bargull from comment #8)
> (In reply to Jason Orendorff [:jorendorff] from comment #1)
> > This needs a declaration of "intent to un-ship" on dev.platform though.
> 
> Did this actually happen?

I don't see an answer to this question in the bug?

Comment 16

[André Bargull [:anba]]

(In reply to Tom Schuster [:evilpie] from comment #14)
> Please do a try run now :) If it passes I am for landing this, addon can be
> deal with a little breakage.

Try finished:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72197f72300b

Comment 17

[Tom S]

(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #15)
> (In reply to André Bargull from comment #8)
> > (In reply to Jason Orendorff [:jorendorff] from comment #1)
> > > This needs a declaration of "intent to un-ship" on dev.platform though.
> > 
> > Did this actually happen?
> 
> I don't see an answer to this question in the bug?

Sorry, I missed this. We can't land before. André do you feel comfortable doing this? Otherwise Jason or me can probably do it.

Comment 18

[André Bargull [:anba]]

(In reply to Tom Schuster [:evilpie] from comment #17)
> Sorry, I missed this. We can't land before. André do you feel comfortable
> doing this? Otherwise Jason or me can probably do it.

I'd prefer if you (or Jason) do this. Thank you!

Comment 19

[Kohei Yoshino]

Will post a site compat doc for this, just in case.

Comment 20

[Tom S]

I did post to dev-platform a while back. There were some concerns about telemetry and breakage, but as shu suggested we need to land this to find real world issues.

Comment 21

[André Bargull [:anba]]

Attachment: bug1272784.part1.patch

Updated patch to apply cleanly on inbound. Carrying r+ from arai.

Comment 22

[André Bargull [:anba]]

Attachment: bug1272784.part2.patch

Updated patch to apply cleanly on inbound. Carrying r+ from arai.

Comment 23

[André Bargull [:anba]]

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91d7485902e6a5ccd2a0b2a86059a56eb58b0516

(Test failure in ecma_6/Reflect/argumentsList.js already fixed in updated part 1 patch.)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f066bb4bc6f0
Part 1: Disallow 'use strict' directive in function with non-simple parameters list. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/a42b25de7402
Part 2: Don't reparse functions with 'use strict' directives. r=arai

Comment 25

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/use-strict-can-no-longer-be-used-in-function-with-non-simple-parameters/

Comment 26

[Nicolas B. Pierron [:nbp]]

The developer documentation should follow the same layout as other error messages[1]. The name of the newly created MDN page should be referenced in the devtools[2], which maps JS error key with the page name.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors
[2] http://searchfox.org/mozilla-central/source/devtools/server/actors/errordocs.js

Comment 27

[Tooru Fujisawa [:arai]]

I'm writing a documentation.
will post a URL here once it's ready.
then it can be linked from devtools.

Comment 28

[Tooru Fujisawa [:arai]]

Added document.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Strict_Non_Simple_Params

Feel free to fix/update :)

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f066bb4bc6f0
https://hg.mozilla.org/mozilla-central/rev/a42b25de7402

Comment 30

[André Bargull [:anba]]

(In reply to Nicolas B. Pierron [:nbp] from comment #26)
> The developer documentation should follow the same layout as other error
> messages[1].

Filed bug 1315772.

Comment 31

[Florian Scholz (Open Web Docs)]

Thanks for the doc arai! It has been reviewed by nbp and me. 
André also wrote and landed the DevTools patch (one version later, Fx53, bug 1315772), so that it will be linked.