Bug 155672 - Implement Annex B.3.3 function hoisting rules for function code
Summary: Implement Annex B.3.3 function hoisting rules for function code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on: 156163
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-18 16:45 PDT by Saam Barati
Modified: 2016-04-05 09:53 PDT (History)
13 users (show)

See Also:


Attachments
WIP (10.09 KB, patch)
2016-03-18 19:29 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (52.68 KB, patch)
2016-03-21 18:28 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (913.97 KB, application/zip)
2016-03-21 19:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (769.29 KB, application/zip)
2016-03-21 19:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (627.69 KB, application/zip)
2016-03-21 19:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (892.56 KB, application/zip)
2016-03-21 19:48 PDT, Build Bot
no flags Details
patch (61.94 KB, patch)
2016-03-22 16:40 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (61.93 KB, patch)
2016-03-22 17:22 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (61.76 KB, patch)
2016-03-23 16:15 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (64.21 KB, patch)
2016-03-23 16:18 PDT, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff
patch (71.27 KB, patch)
2016-04-01 15:43 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (920.54 KB, application/zip)
2016-04-01 16:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (882.66 KB, application/zip)
2016-04-01 16:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (628.32 KB, application/zip)
2016-04-01 16:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (846.75 KB, application/zip)
2016-04-01 16:50 PDT, Build Bot
no flags Details
patch (81.16 KB, patch)
2016-04-01 16:59 PDT, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff
patch for landing (81.03 KB, patch)
2016-04-03 11:10 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Saam Barati 2016-03-18 19:29:39 PDT
Created attachment 274492 [details]
WIP
Comment 2 Saam Barati 2016-03-21 18:28:37 PDT
Created attachment 274635 [details]
patch
Comment 3 Build Bot 2016-03-21 19:25:11 PDT
Comment on attachment 274635 [details]
patch

Attachment 274635 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1018371

New failing tests:
js/kde/func-decl.html
Comment 4 Build Bot 2016-03-21 19:25:14 PDT
Created attachment 274637 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 5 Build Bot 2016-03-21 19:28:26 PDT
Comment on attachment 274635 [details]
patch

Attachment 274635 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1018376

New failing tests:
js/kde/func-decl.html
Comment 6 Build Bot 2016-03-21 19:28:29 PDT
Created attachment 274638 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-03-21 19:31:38 PDT
Comment on attachment 274635 [details]
patch

Attachment 274635 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1018375

New failing tests:
js/kde/func-decl.html
Comment 8 Build Bot 2016-03-21 19:31:42 PDT
Created attachment 274639 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5
Comment 9 Build Bot 2016-03-21 19:48:14 PDT
Comment on attachment 274635 [details]
patch

Attachment 274635 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1018386

New failing tests:
js/kde/func-decl.html
js/function-declaration-statement.html
Comment 10 Build Bot 2016-03-21 19:48:18 PDT
Created attachment 274640 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Saam Barati 2016-03-22 16:40:22 PDT
Created attachment 274700 [details]
patch
Comment 12 Saam Barati 2016-03-22 17:22:50 PDT
Created attachment 274708 [details]
patch
Comment 13 Saam Barati 2016-03-23 00:32:54 PDT
Comment on attachment 274708 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274708&action=review

> Source/JavaScriptCore/tests/stress/sloppy-mode-function-hoisting.js:575
> +test(function() {
> +    var f = 20;
> +    assert(f === 20);
> +    {
> +        while (false)
> +            while (false)
> +                if (true)
> +                    function f() { return 20; }
> +    }
> +    assert(f() === 20);
> +});

I'm not sure this is right anymore. I'm actually starting to think
anytime we parse a function as a "Statement" and not a "StatementListItem",
we should just always immediately hoist it to the top. I believe that's what B.3.3. says.
I'm going to change it to do that.

Otherwise, things get weird for this:
```
function foo() { 
    for (let i = 0; i < 10; i++)
        function f() { return i; }
}
```
Comment 14 Saam Barati 2016-03-23 00:41:09 PDT
Comment on attachment 274708 [details]
patch

I also need to implement this for programs/eval :/
Comment 15 Saam Barati 2016-03-23 16:15:41 PDT
Created attachment 274790 [details]
patch
Comment 16 Saam Barati 2016-03-23 16:18:36 PDT
Created attachment 274791 [details]
patch
Comment 17 Geoffrey Garen 2016-03-30 15:38:19 PDT
Comment on attachment 274791 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274791&action=review

r=me

> Source/JavaScriptCore/parser/Parser.h:565
> +    void getCapturedVarsAndSloppyHoistedFunctions(IdentifierSet& capturedVariables, UniquedStringImplPtrSet& sloppyHoistedFunctions, bool& modifiedParameter, bool& modifiedArguments)

Let's split this into two functions. They happen to want to run back-to-back, but they have distinct inputs and outputs.
Comment 18 Saam Barati 2016-03-31 16:10:31 PDT
Thanks for the review.

I'm going to change the semantics in this patch to match what's in the
spec for the degenerate case of a function declaration being in an if/while/etc 
without a block.

i.e
`if (c) function foo() {..}`
will implicitly be converted to:
`if (c) { function foo() {..} }`

The spec specifically allows for this in an "if" statement in sloppy mode.
Note that "if" is the only place this exception is allowed.
The following code, as an example, is a syntax error.
`while (c) function foo() {..}`
Comment 19 Saam Barati 2016-04-01 15:43:34 PDT
Created attachment 275438 [details]
patch

back up for review because of the new semantics for
"if (cond) function bar() { ... }"
Comment 20 Saam Barati 2016-04-01 15:45:15 PDT
Comment on attachment 275438 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275438&action=review

> Source/JavaScriptCore/parser/Parser.cpp:1654
> +        if (!strictMode()) {
> +
> +            failIfFalse(isParentIfStatement, "Function declarations are only allowed inside block statements or at the top level of a program");
> +            if (currentScope()->isFunction()) {
> +                // Any function declaration that isn't in a block is a syntax error unless it's
> +                // in an if/else statement. If it's in an if/else statement, we will magically
> +                // treat it as if the if/else statement is inside a block statement.
> +                // to the very top like a "var". For example:
> +                // function a() {
> +                //     if (cond) function foo() { }
> +                // }
> +                // will be rewritten as:
> +                // function a() {
> +                //     if (cond) { function foo() { } }
> +                // }
> +                AutoPopScopeRef blockScope(this, pushScope());
> +                blockScope->setIsLexicalScope();
> +                blockScope->preventVarDeclarations();
> +                JSTokenLocation location(tokenLocation());
> +                int start = tokenLine();
> +
> +                TreeStatement function = parseFunctionDeclaration(context);
> +                propagateError();
> +                failIfFalse(function, "Expected valid function statement after 'function' keyword");
> +                TreeSourceElements sourceElements = context.createSourceElements();
> +                context.appendStatement(sourceElements, function);
> +                result = context.createBlockStatement(location, sourceElements, start, m_lastTokenEndPosition.line, currentScope()->finalizeLexicalEnvironment(), currentScope()->takeFunctionDeclarations());
> +                popScope(blockScope, TreeBuilder::NeedsFreeVariableInfo);
> +            } else {
> +                // We only implement annex B.3.3 if we're in function mode. Otherwise, we fall back
> +                // to hoisting behavior.
> +                // FIXME: https://bugs.webkit.org/show_bug.cgi?id=155813
> +                DepthManager statementDepth(&m_statementDepth);
> +                m_statementDepth = 1;
> +                result = parseFunctionDeclaration(context);

This is new since the last patch
Comment 21 Saam Barati 2016-04-01 16:01:52 PDT
looks like I missed a case:
```
function foo() {
label: function baz() { }
}
```
should still parse. I'll make the necessary changes.
Comment 22 Build Bot 2016-04-01 16:40:01 PDT
Comment on attachment 275438 [details]
patch

Attachment 275438 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1083441

New failing tests:
js/function-declaration-statement.html
Comment 23 Build Bot 2016-04-01 16:40:05 PDT
Created attachment 275440 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 24 Build Bot 2016-04-01 16:43:18 PDT
Comment on attachment 275438 [details]
patch

Attachment 275438 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1083445

New failing tests:
js/function-declaration-statement.html
Comment 25 Build Bot 2016-04-01 16:43:21 PDT
Created attachment 275441 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 26 Build Bot 2016-04-01 16:46:41 PDT
Comment on attachment 275438 [details]
patch

Attachment 275438 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1083446

New failing tests:
js/function-declaration-statement.html
Comment 27 Build Bot 2016-04-01 16:46:44 PDT
Created attachment 275442 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5
Comment 28 Build Bot 2016-04-01 16:50:55 PDT
Comment on attachment 275438 [details]
patch

Attachment 275438 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1083448

New failing tests:
js/function-declaration-statement.html
Comment 29 Build Bot 2016-04-01 16:50:59 PDT
Created attachment 275443 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 30 Saam Barati 2016-04-01 16:59:57 PDT
Created attachment 275447 [details]
patch

ok. I think this is the patch.
Comment 31 Geoffrey Garen 2016-04-01 17:12:48 PDT
Comment on attachment 275447 [details]
patch

r=me
Comment 32 Saam Barati 2016-04-03 11:10:39 PDT
Created attachment 275506 [details]
patch for landing
Comment 34 WebKit Commit Bot 2016-04-03 12:45:01 PDT
Comment on attachment 275506 [details]
patch for landing

Clearing flags on attachment: 275506

Committed r198989: <http://trac.webkit.org/changeset/198989>
Comment 35 WebKit Commit Bot 2016-04-03 12:45:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Alexey Proskuryakov 2016-04-05 09:53:21 PDT
This made a JSC test time out, see bug 156239.