Implement Annex B.3.3 function hoisting rules for function code

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

https://tc39.github.io/ecma262/#sec-block-level-function-declarations-web-legacy-compatibility-semantics

[Saam Barati]

Created attachment 274492 [details]
WIP

[Saam Barati]

Created attachment 274635 [details]
patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Saam Barati]

Created attachment 274700 [details]
patch

[Saam Barati]

Created attachment 274708 [details]
patch

[Saam Barati]

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; }
}
```

[Saam Barati]

Comment on attachment 274708 [details]
patch

I also need to implement this for programs/eval :/

[Saam Barati]

Created attachment 274790 [details]
patch

[Saam Barati]

Created attachment 274791 [details]
patch

[Geoffrey Garen]

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.

[Saam Barati]

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() {..}`

[Saam Barati]

Created attachment 275438 [details]
patch

back up for review because of the new semantics for
"if (cond) function bar() { ... }"

[Saam Barati]

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

[Saam Barati]

looks like I missed a case:
```
function foo() {
label: function baz() { }
}
```
should still parse. I'll make the necessary changes.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Saam Barati]

Created attachment 275447 [details]
patch

ok. I think this is the patch.

[Geoffrey Garen]

Comment on attachment 275447 [details]
patch

r=me

[Saam Barati]

Created attachment 275506 [details]
patch for landing

[Saam Barati]

a couple follow up bugs:

https://bugs.webkit.org/show_bug.cgi?id=156146
https://bugs.webkit.org/show_bug.cgi?id=155813

[WebKit Commit Bot]

Comment on attachment 275506 [details]
patch for landing

Clearing flags on attachment: 275506

Committed r198989: <http://trac.webkit.org/changeset/198989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This made a JSC test time out, see bug 156239.