Bug 155672

Summary:

Implement Annex B.3.3 function hoisting rules for function code

Product:

WebKit

Reporter:

Saam Barati <saam>

Component:

JavaScriptCore

Assignee:

Saam Barati <saam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, buildbot, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, rniwa, sukolsak, ysuzuki

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

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

Bug Depends on:

156163

Bug Blocks:

Attachments:

Description	Flags
WIP	none
patch	buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-yosemite	none
Archive of layout-test-results from ews105 for mac-yosemite-wk2	none
Archive of layout-test-results from ews123 for ios-simulator-wk2	none
Archive of layout-test-results from ews113 for mac-yosemite	none
patch	none
patch	none
patch	none
patch	ggaren: review+
patch	buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-yosemite	none
Archive of layout-test-results from ews104 for mac-yosemite-wk2	none
Archive of layout-test-results from ews125 for ios-simulator-wk2	none
Archive of layout-test-results from ews114 for mac-yosemite	none
patch	ggaren: review+
patch for landing	none

Saam Barati

Reported 2016-03-18 16:45:03 PDT

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

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
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Saam Barati

Comment 1 2016-03-18 19:29:39 PDT

Created attachment 274492 [details] WIP

Saam Barati

Comment 2 2016-03-21 18:28:37 PDT

Created attachment 274635 [details] patch

Build Bot

Comment 3 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

Build Bot

Comment 4 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

Build Bot

Comment 5 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

Build Bot

Comment 6 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

Build Bot

Comment 7 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

Build Bot

Comment 8 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

Build Bot

Comment 9 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

Build Bot

Comment 10 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

Saam Barati

Comment 11 2016-03-22 16:40:22 PDT

Created attachment 274700 [details] patch

Saam Barati

Comment 12 2016-03-22 17:22:50 PDT

Created attachment 274708 [details] patch

Saam Barati

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

Saam Barati

Comment 14 2016-03-23 00:41:09 PDT

Comment on attachment 274708 [details] patch I also need to implement this for programs/eval :/

Saam Barati

Comment 15 2016-03-23 16:15:41 PDT

Created attachment 274790 [details] patch

Saam Barati

Comment 16 2016-03-23 16:18:36 PDT

Created attachment 274791 [details] patch

Geoffrey Garen

Comment 17 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.

Saam Barati

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

Saam Barati

Comment 19 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() { ... }"

Saam Barati

Comment 20 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

Saam Barati

Comment 21 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.

Build Bot

Comment 22 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

Build Bot

Comment 23 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

Build Bot

Comment 24 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

Build Bot

Comment 25 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

Build Bot

Comment 26 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

Build Bot

Comment 27 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

Build Bot

Comment 28 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

Build Bot

Comment 29 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

Saam Barati

Comment 30 2016-04-01 16:59:57 PDT

Created attachment 275447 [details] patch ok. I think this is the patch.

Geoffrey Garen

Comment 31 2016-04-01 17:12:48 PDT

Comment on attachment 275447 [details] patch r=me

Saam Barati

Comment 32 2016-04-03 11:10:39 PDT

Created attachment 275506 [details] patch for landing

Saam Barati

Comment 33 2016-04-03 11:21:04 PDT

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 34 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>

WebKit Commit Bot

Comment 35 2016-04-03 12:45:08 PDT

All reviewed patches have been landed. Closing bug.

Alexey Proskuryakov

Comment 36 2016-04-05 09:53:21 PDT

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

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