Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JSC] Implement lexical scope chain walk to correctly determine Annex B hoisted functions #17778

Conversation

shvaikalesh
Copy link
Member

@shvaikalesh shvaikalesh commented Sep 14, 2023

18dd8af

[JSC] Implement lexical scope chain walk to correctly determine Annex B hoisted functions
https://bugs.webkit.org/show_bug.cgi?id=261564
<rdar://problem/115504046>

Reviewed by Yusuke Suzuki.

Please consider the following code:

```js
{
    function foo() { return 1; }
}

{
    let foo = 1;

    {
        function foo() { return 2; }
    }
}

foo(); // should be 1
```

While both functions share the same identifier, only the first one should be hoisted per spec [1];
hoisting the second function would result in an early error due to name clash with the lexical declaration.
Given it's impossible to determine if a function is hoistable by its identifier only, this change replaces
maintaining identifier set of hoistable functions on a ScopeNode with a flag on FunctionMetadataNode.

Also, this patch introduces bubbleSloppyModeFunctionHoistingCandidates() that effectively walks up the
scope chain to ensure that for every hoisting candidate there are no clashes with lexical bindings.
Before this change, only top-level lexical variables and parameters were considered.

Considering the following case:

```js
{ // scope A
    { // scope B
        function foo() {}
    }

    const foo = 1;
}

foo(); // should throw ReferenceError
```

The approach of this patch is to bubble all hoisting candidates from scope B to A, during popScope() of B,
and then reject duplicates during popScope() of A. We can't check for clashes during popScope() of B
because lexical declarations may be added to scope A later on.

To make this work, we must skip duplicate checks against same-scope (not bubbled) function declarations,
which is implemented via NeedsDuplicateDeclarationCheck. declareLexicalVariable() / declareFunction()
already check for duplicates, both in strict and sloppy mode, making this approach viable.

When bubbling hoisting candidates, NeedsDuplicateDeclarationCheck::No can get overriden by inner scope,
ensuring correctness of the following case [3]:

```js
{
    function foo() { return 1; }

    {
        function foo() { return 2; }
    }
}

foo(); // should be 1
```

Since per another Annex B section [2], `catch (foo) { var foo; }` doesn't result in a SyntaxError, simple
parameter catch scopes are skipped when determining if a function should be hoisted.

Performance-wise this change adds only a single HashMap::isEmpty() check per scope, and was proven to be
neutral on JetStream3.

Aligns JSC with V8 and SpiderMonkey, expect for the one test [3].

[1]: https://tc39.es/ecma262/#sec-web-compat-functiondeclarationinstantiation (29.a.ii)
[2]: https://tc39.es/ecma262/#sec-variablestatements-in-catch-blocks
[3]: https://github.com/tc39/test262/blob/main/test/annexB/language/function-code/block-decl-nested-blocks-with-fun-decl.js

* JSTests/stress/sloppy-mode-function-hoisting-2.js: Added.
* JSTests/test262/expectations.yaml: Mark 193 tests as passing.
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::BytecodeGenerator):
(JSC::BytecodeGenerator::hoistSloppyModeFunctionIfNecessary):
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h: Removes unused m_functionOffsets.
* Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:
(JSC::FuncDeclNode::emitBytecode):
* Source/JavaScriptCore/parser/Nodes.cpp:
(JSC::ScopeNode::ScopeNode):
(JSC::ProgramNode::ProgramNode):
(JSC::ModuleProgramNode::ModuleProgramNode):
(JSC::EvalNode::EvalNode):
(JSC::FunctionMetadataNode::operator== const):
(JSC::FunctionMetadataNode::dump const):
(JSC::FunctionNode::FunctionNode):
* Source/JavaScriptCore/parser/Nodes.h:
(JSC::ScopeNode::captures):
(JSC::ScopeNode::hasSloppyModeHoistedFunction const): Deleted.
* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::parseInner):
(JSC::Parser<LexerType>::parseFunctionDeclaration):
* Source/JavaScriptCore/parser/Parser.h:
(JSC::Scope::declareFunction):
(JSC::Scope::addSloppyModeFunctionHoistingCandidate):
(JSC::Scope::finalizeSloppyModeFunctionHoisting):
(JSC::Scope::bubbleSloppyModeFunctionHoistingCandidates):
(JSC::Scope::hasSloppyModeFunctionHoistingCandidates const):
(JSC::Parser::popScopeInternal):
(JSC::Parser::declareFunction):
(JSC::Parser<LexerType>::parse):
(JSC::Scope::addSloppyModeHoistableFunctionCandidate): Deleted.
(JSC::Scope::getSloppyModeHoistedFunctions): Deleted.
* Source/JavaScriptCore/parser/VariableEnvironment.h:
Harmonize naming to include "candidate" only when hoisting isn't guaranteed
due to unchecked clashes with lexical bindings.

Canonical link: https://commits.webkit.org/268302@main

bf501fb

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ›  jsc-armv7
βœ… πŸ›  tv-sim   πŸ§ͺ jsc-armv7-tests
βœ… πŸ›  watch βœ… πŸ›  jsc-mips
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim βœ… πŸ§ͺ jsc-mips-tests

@shvaikalesh shvaikalesh requested a review from a team as a code owner September 14, 2023 17:57
@shvaikalesh shvaikalesh self-assigned this Sep 14, 2023
@shvaikalesh shvaikalesh added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Sep 14, 2023
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with comment.

Source/JavaScriptCore/parser/Parser.h Outdated Show resolved Hide resolved
@shvaikalesh shvaikalesh changed the title [JSC] Implement proper scope chain lookup for Annex B hoisting candidates [JSC] Implement lexical scope chain walk to correctly determine Annex B hoisted functions Sep 22, 2023
@shvaikalesh shvaikalesh force-pushed the eng/JSC-Implement-proper-scope-chain-lookup-for-Annex-B-hoisting-candidates branch from bf656db to bf501fb Compare September 22, 2023 04:48
@shvaikalesh shvaikalesh added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Sep 22, 2023
… B hoisted functions

https://bugs.webkit.org/show_bug.cgi?id=261564
<rdar://problem/115504046>

Reviewed by Yusuke Suzuki.

Please consider the following code:

```js
{
    function foo() { return 1; }
}

{
    let foo = 1;

    {
        function foo() { return 2; }
    }
}

foo(); // should be 1
```

While both functions share the same identifier, only the first one should be hoisted per spec [1];
hoisting the second function would result in an early error due to name clash with the lexical declaration.
Given it's impossible to determine if a function is hoistable by its identifier only, this change replaces
maintaining identifier set of hoistable functions on a ScopeNode with a flag on FunctionMetadataNode.

Also, this patch introduces bubbleSloppyModeFunctionHoistingCandidates() that effectively walks up the
scope chain to ensure that for every hoisting candidate there are no clashes with lexical bindings.
Before this change, only top-level lexical variables and parameters were considered.

Considering the following case:

```js
{ // scope A
    { // scope B
        function foo() {}
    }

    const foo = 1;
}

foo(); // should throw ReferenceError
```

The approach of this patch is to bubble all hoisting candidates from scope B to A, during popScope() of B,
and then reject duplicates during popScope() of A. We can't check for clashes during popScope() of B
because lexical declarations may be added to scope A later on.

To make this work, we must skip duplicate checks against same-scope (not bubbled) function declarations,
which is implemented via NeedsDuplicateDeclarationCheck. declareLexicalVariable() / declareFunction()
already check for duplicates, both in strict and sloppy mode, making this approach viable.

When bubbling hoisting candidates, NeedsDuplicateDeclarationCheck::No can get overriden by inner scope,
ensuring correctness of the following case [3]:

```js
{
    function foo() { return 1; }

    {
        function foo() { return 2; }
    }
}

foo(); // should be 1
```

Since per another Annex B section [2], `catch (foo) { var foo; }` doesn't result in a SyntaxError, simple
parameter catch scopes are skipped when determining if a function should be hoisted.

Performance-wise this change adds only a single HashMap::isEmpty() check per scope, and was proven to be
neutral on JetStream3.

Aligns JSC with V8 and SpiderMonkey, expect for the one test [3].

[1]: https://tc39.es/ecma262/#sec-web-compat-functiondeclarationinstantiation (29.a.ii)
[2]: https://tc39.es/ecma262/#sec-variablestatements-in-catch-blocks
[3]: https://github.com/tc39/test262/blob/main/test/annexB/language/function-code/block-decl-nested-blocks-with-fun-decl.js

* JSTests/stress/sloppy-mode-function-hoisting-2.js: Added.
* JSTests/test262/expectations.yaml: Mark 193 tests as passing.
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::BytecodeGenerator):
(JSC::BytecodeGenerator::hoistSloppyModeFunctionIfNecessary):
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h: Removes unused m_functionOffsets.
* Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:
(JSC::FuncDeclNode::emitBytecode):
* Source/JavaScriptCore/parser/Nodes.cpp:
(JSC::ScopeNode::ScopeNode):
(JSC::ProgramNode::ProgramNode):
(JSC::ModuleProgramNode::ModuleProgramNode):
(JSC::EvalNode::EvalNode):
(JSC::FunctionMetadataNode::operator== const):
(JSC::FunctionMetadataNode::dump const):
(JSC::FunctionNode::FunctionNode):
* Source/JavaScriptCore/parser/Nodes.h:
(JSC::ScopeNode::captures):
(JSC::ScopeNode::hasSloppyModeHoistedFunction const): Deleted.
* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::parseInner):
(JSC::Parser<LexerType>::parseFunctionDeclaration):
* Source/JavaScriptCore/parser/Parser.h:
(JSC::Scope::declareFunction):
(JSC::Scope::addSloppyModeFunctionHoistingCandidate):
(JSC::Scope::finalizeSloppyModeFunctionHoisting):
(JSC::Scope::bubbleSloppyModeFunctionHoistingCandidates):
(JSC::Scope::hasSloppyModeFunctionHoistingCandidates const):
(JSC::Parser::popScopeInternal):
(JSC::Parser::declareFunction):
(JSC::Parser<LexerType>::parse):
(JSC::Scope::addSloppyModeHoistableFunctionCandidate): Deleted.
(JSC::Scope::getSloppyModeHoistedFunctions): Deleted.
* Source/JavaScriptCore/parser/VariableEnvironment.h:
Harmonize naming to include "candidate" only when hoisting isn't guaranteed
due to unchecked clashes with lexical bindings.

Canonical link: https://commits.webkit.org/268302@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/JSC-Implement-proper-scope-chain-lookup-for-Annex-B-hoisting-candidates branch from bf501fb to 18dd8af Compare September 22, 2023 07:20
@webkit-commit-queue webkit-commit-queue merged commit 18dd8af into WebKit:main Sep 22, 2023
@webkit-commit-queue
Copy link
Collaborator

Committed 268302@main (18dd8af): https://commits.webkit.org/268302@main

Reviewed commits have been landed. Closing PR #17778 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
4 participants