Closed Bug 1272784 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- affected
firefox52 --- fixed

People

(Reporter: arai, Assigned: anba)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete, site-compat)

Attachments

(2 files, 2 obsolete files)

(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"; }
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.
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;
> }
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.
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).
Duplicate of this bug: 1245413
Attached patch bug1272784.part1.patch (obsolete) — Splinter Review
Part 1: Implement the new checks, but functions with "use strict" directives are still rewound.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8796263 - Flags: review?(arai.unmht)
Attached patch bug1272784.part2.patch (obsolete) — Splinter Review
Part 2: Don't rewind functions with "use strict" directives (except for error reporting).
Attachment #8796266 - Flags: review?(arai.unmht)
(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 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.
Attachment #8796263 - Flags: review?(arai.unmht) → review+
adding some keywords.
site-compat might be inappropriate, feel free to clear it.
Attachment #8796266 - Flags: review?(arai.unmht) → review+
Have this passed try run?
wasn't there anything hit this in-tree?
(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
(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.
Please do a try run now :) If it passes I am for landing this, addon can be deal with a little breakage.
Flags: needinfo?(andrebargull)
Blocks: es6
Blocks: es7
No longer blocks: es6
Flags: needinfo?(andrebargull)
(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?
(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
(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.
(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!
Will post a site compat doc for this, just in case.
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.
Updated patch to apply cleanly on inbound. Carrying r+ from arai.
Attachment #8796263 - Attachment is obsolete: true
Attachment #8803989 - Flags: review+
Updated patch to apply cleanly on inbound. Carrying r+ from arai.
Attachment #8796266 - Attachment is obsolete: true
Attachment #8803990 - Flags: review+
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.)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
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
Flags: needinfo?(andrebargull)
I'm writing a documentation.
will post a URL here once it's ready.
then it can be linked from devtools.
https://hg.mozilla.org/mozilla-central/rev/f066bb4bc6f0
https://hg.mozilla.org/mozilla-central/rev/a42b25de7402
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(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.
Flags: needinfo?(andrebargull)
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.
You need to log in before you can comment on or make changes to this bug.