[webkit-changes] [WebKit/WebKit] 18dd8a: [JSC] Implement lexical scope chain walk to correc...

[Commit Queue]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 18dd8af86932de0640de2a4586f6f3c0e72eb3c2
      https://github.com/WebKit/WebKit/commit/18dd8af86932de0640de2a4586f6f3c0e72eb3c2
  Author: Alexey Shvayka <ashvayka at apple.com>
  Date:   2023-09-22 (Fri, 22 Sep 2023)

  Changed paths:
    A JSTests/stress/sloppy-mode-function-hoisting-2.js
    M JSTests/test262/expectations.yaml
    M Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
    M Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h
    M Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp
    M Source/JavaScriptCore/parser/Nodes.cpp
    M Source/JavaScriptCore/parser/Nodes.h
    M Source/JavaScriptCore/parser/Parser.cpp
    M Source/JavaScriptCore/parser/Parser.h
    M Source/JavaScriptCore/parser/VariableEnvironment.h

  Log Message:
  -----------
  [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