Bug 163208 – [ES6]. Implement Annex B.3.3 function hoisting rules for eval

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[GSkachkov]

Implement Annex B.3.3 function hoisting rules for eval.
Link to the spec https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation

Also keep in mind that there are some case that should be covered by implementations, for instance:
'''
function foo() {
    {
        let f = 20;
        eval(" { function f() { }; } ");
        print(type f);// number 
    }
    print(f);// undefined 
}
foo();
'''
See discussion in this threadhttps://github.com/tc39/ecma262/issues/162#issuecomment-252051508

//////////////
Because:
'f' in the 2nd console.log(f) should be undefined.

Annex B.3.3.3 for EvalDeclarationInstantiation says in 1.d.2.ii: "If replacing the FunctionDeclaration f with a VariableStatement that has F as a BindingIdentifier would not produce any Early Errors for body [...]"

You can try to run this program where function f() { } is replaced with var f:

function foo() {
    {
        let f = 20;
        eval(" { var f; } ");
        console.log(f);
    }
    console.log(f);
}
foo();
This throws a redeclaration error, so Annex B.3.3.3 does not synthesize a var binding for the function.
////////////////

[GSkachkov]

Created attachment 291304 [details]
Patch

Draft version of the patch. Later will be added more tests

[GSkachkov]

Created attachment 291309 [details]
Patch

Rebase to latest version

[GSkachkov]

Created attachment 294188 [details]
Patch

Patch for review, later I will provide more tests

[GSkachkov]

Created attachment 294284 [details]
Patch

Try to fix 32bit builds

[Build Bot]

Comment on attachment 294284 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/transaction-create_in_versionchange.htm
imported/w3c/web-platform-tests/IndexedDB/transaction-abort-index-metadata-revert.html

[Build Bot]

Created attachment 294311 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 294413 [details]
Patch

One more, try to fix 32bit builds

[GSkachkov]

Created attachment 294533 [details]
Patch

One more, try to fix 32bit builds

[Saam Barati]

Comment on attachment 294533 [details]
Patch

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

I think the main approach is OK, but there are a lot of little things I think might be wrong or could be better/simpler. Comments below.

> Source/JavaScriptCore/ChangeLog:13
> +        Current patch implement Annex B.3.3 that is related to hoisting of function
> +        declaration in eval. https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation
> +        Function declaration in eval should create variable with function name in function scope 
> +        where eval is invoked or bind to variable if it declared outside of the eval. If variable is created 
> +        it can be removed by delete a; command. If eval is invoke in block scope that 
> +        contains let/const variable with the same name as function declaration  we do not binding

It's worth also talking about your implementation in the changelog.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2851
> +        // Fixme: we do not have llint optimization for those op_codes

Please file a bug for this or implement the optimization.
Also, should be FIXME not Fixme.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:754
> +    auto varKind = [&] (UniquedStringImpl* uid) -> VarKind {
> +        return evalNode->captures(uid) ? VarKind::Scope : VarKind::Stack;
> +    };
> +
> +    for (FunctionMetadataNode* function : evalNode->functionStack()) {
> +        VarKind kind = varKind(function->ident().impl());
> +        if (kind == VarKind::Scope)
> +            m_codeBlock->addFunctionDecl(makeFunction(function));
> +        else
> +            m_functionsToInitialize.append(std::make_pair(function, GlobalFunctionVariable));
> +    }

Why is this change needed?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2193
> +RegisterID* BytecodeGenerator::emitResolveOuterScopeType(RegisterID* dst, RegisterID* scope)

This function name is incorrect.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2196
> +    m_codeBlock->addPropertyAccessInstruction(instructions().size());

This looks wrong.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2201
> +    dst = tempDestination(dst);
> +    emitOpcode(op_is_scope_var_type);
> +    instructions().append(kill(dst));

Why not just make dst the result w/out all the tempDestination stuff? This seems like it could be wrong.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2209
> +    m_codeBlock->addPropertyAccessInstruction(instructions().size());

This looks wrong.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2215
> +    

remove newline.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2217
> +    instructions().append(localScopeDepth());

Why not just make the access off the base-most scope of the eval instead of having a local scope depth? We should already have the scope on the symbol table stack, right?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1100
> +                        JSScope* scope = jsCast<JSScope*>(child.value());

Doesn't LexicalEnvironmentType guarantee that it's a JSSymbolTableObject?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103
> +                        if (scope->symbolTable() != nullptr)
> +                            result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope;

How we normally do this is:
if (SymbolTable* symbolTable = scope->symbolTable())
     ....

> Source/JavaScriptCore/dfg/DFGNode.h:983
> +    int32_t getOpInfo2()

Please give this a name for your usage of it.

> Source/JavaScriptCore/dfg/DFGOperations.h:193
> +JSCell* JIT_OPERATION operationResolveClosestVarScope(ExecState*, JSScope*, UniquedStringImpl*, int32_t);

should be uint32_t

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8777
> +    callOperation(operationResolveClosestVarScope, resultGPR, scopeGPR, identifierUID(node->identifierNumber()), node->getOpInfo2());

We usually give names to the opInfo methods so we know what it's for.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:821
> +    linkSlowCase(iter);
> +    JITSlowPathCall slowPathCall(this, currentInstruction, slow_path_is_scope_var_type);
> +    slowPathCall.call();

This should not have a slow path. The fast path should do the function call.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:828
> +    linkSlowCase(iter);
> +    JITSlowPathCall slowPathCall(this, currentInstruction, slow_path_resolve_closest_var_scope);
> +    slowPathCall.call();

ditto

> Source/JavaScriptCore/parser/Parser.h:1182
> +            // We allways try to hoist function to top scope inside of eval

I don't think this comment adds anything.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:826
> +    CHECK_EXCEPTION();

An exception could not be thrown by the previous statement.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:832
> +    else if (scope->symbolTable() != nullptr)
> +        result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope;

We usually do:
else if (SymbolTable* symbolTable = scope->symbolTable())

> Source/JavaScriptCore/runtime/JSScope.cpp:218
> +JSObject* JSScope::resolveClosestVar(ExecState* exec, JSScope* scope, const Identifier& ident, int skipScope)

should be unsigned instead of int

> Source/JavaScriptCore/runtime/JSScope.cpp:224
> +    int skipedScopes = 0;

typo, should be: "skippedScopes"
also, should be unsigned.

> Source/JavaScriptCore/runtime/JSScope.cpp:245
> +        if (skipedScopes < skipScope) {
> +            ++skipedScopes;
> +            continue;
> +        }

Why not just have the bytecode emit starting from the base-most scope of the function and you can remove the skipScope parameter entirely?

[Saam Barati]

<rdar://problem/29145081>

[GSkachkov]

Created attachment 296083 [details]
Patch

Fix comments

[GSkachkov]

Created attachment 296084 [details]
Patch

Fix build & comments

[GSkachkov]

Comment on attachment 294533 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2851
>> +        // Fixme: we do not have llint optimization for those op_codes
> 
> Please file a bug for this or implement the optimization.
> Also, should be FIXME not Fixme.

Just a question, do we need optimization there? I'm not familiar with this part.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:754
>> +    }
> 
> Why is this change needed?

It is allow to fix following code:
eval("'use strict'; class C { constructor() { } }; function foo() { return new C; }; foo()");

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2193
>> +RegisterID* BytecodeGenerator::emitResolveOuterScopeType(RegisterID* dst, RegisterID* scope)
> 
> This function name is incorrect.

Renamed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2196
>> +    m_codeBlock->addPropertyAccessInstruction(instructions().size());
> 
> This looks wrong.

Removed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2201
>> +    instructions().append(kill(dst));
> 
> Why not just make dst the result w/out all the tempDestination stuff? This seems like it could be wrong.

Hmm, without    dst = tempDestination(dst), I received segment 11.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2209
>> +    m_codeBlock->addPropertyAccessInstruction(instructions().size());
> 
> This looks wrong.

Removed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2215
>> +    
> 
> remove newline.

Done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2217
>> +    instructions().append(localScopeDepth());
> 
> Why not just make the access off the base-most scope of the eval instead of having a local scope depth? We should already have the scope on the symbol table stack, right?

Ohh, not sure if I got how I can do this.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1100
>> +                        JSScope* scope = jsCast<JSScope*>(child.value());
> 
> Doesn't LexicalEnvironmentType guarantee that it's a JSSymbolTableObject?

Hmm, it is rewritten function: 
bool JSScope::isVarScope()
{
    if (type() != LexicalEnvironmentType)
        return false;
    return jsCast<JSLexicalEnvironment*>(this)->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope;
}
So I expect that it should work

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103
>> +                            result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope;
> 
> How we normally do this is:
> if (SymbolTable* symbolTable = scope->symbolTable())
>      ....

Fixed

>> Source/JavaScriptCore/dfg/DFGNode.h:983
>> +    int32_t getOpInfo2()
> 
> Please give this a name for your usage of it.

Renamed to skipScope()

>> Source/JavaScriptCore/dfg/DFGOperations.h:193
>> +JSCell* JIT_OPERATION operationResolveClosestVarScope(ExecState*, JSScope*, UniquedStringImpl*, int32_t);
> 
> should be uint32_t

Done

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8777
>> +    callOperation(operationResolveClosestVarScope, resultGPR, scopeGPR, identifierUID(node->identifierNumber()), node->getOpInfo2());
> 
> We usually give names to the opInfo methods so we know what it's for.

Fixed

>> Source/JavaScriptCore/parser/Parser.h:1182
>> +            // We allways try to hoist function to top scope inside of eval
> 
> I don't think this comment adds anything.

Removed

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:826
>> +    CHECK_EXCEPTION();
> 
> An exception could not be thrown by the previous statement.

Removed

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:832
>> +        result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope;
> 
> We usually do:
> else if (SymbolTable* symbolTable = scope->symbolTable())

Fixed

>> Source/JavaScriptCore/runtime/JSScope.cpp:218
>> +JSObject* JSScope::resolveClosestVar(ExecState* exec, JSScope* scope, const Identifier& ident, int skipScope)
> 
> should be unsigned instead of int

Fixed

>> Source/JavaScriptCore/runtime/JSScope.cpp:224
>> +    int skipedScopes = 0;
> 
> typo, should be: "skippedScopes"
> also, should be unsigned.

Removed

[Build Bot]

Comment on attachment 296084 [details]
Patch

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

New failing tests:
media/modern-media-controls/media-controller/media-controller-resize.html

[Build Bot]

Created attachment 296087 [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

[Saam Barati]

Comment on attachment 294533 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2199
> +    dst = tempDestination(dst);

Yeah, you're right. dst could be nullptr. However, you don't want to use tempDestination here, you want finalDestination.

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2217
>>> +    instructions().append(localScopeDepth());
>> 
>> Why not just make the access off the base-most scope of the eval instead of having a local scope depth? We should already have the scope on the symbol table stack, right?
> 
> Ohh, not sure if I got how I can do this.

The first thing that the bytecode generator does is emit a get_scope.
You'll just need to make sure to record what RegisterID* is that scope inside BytecodeGenerator(Eval) constructor, and then your operation can just work off that scope, instead of having to record the localDepth().
This localDepth() just adds more complexity to what you're trying to do for no reason IMO.

[Saam Barati]

Comment on attachment 296084 [details]
Patch

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2876
> +        // Fixme: we do not have llint optimization for those op_codes

Please file a bug for this if you think we should do it. That said, this is way more than a LLInt optimization, knowing which scope you'll resolve to helps in all tiers.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:769
> +    for (FunctionMetadataNode* function : evalNode->functionStack()) {
> +        VarKind kind = varKind(function->ident().impl());
> +        if (kind == VarKind::Scope)
> +            m_codeBlock->addFunctionDecl(makeFunction(function));
> +        else
> +            m_functionsToInitialize.append(std::make_pair(function, GlobalFunctionVariable));
> +    }

Do you mind elaborating on your previous example for why you said this is needed, I don't quite get it.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2193
> +    // resolve_scope dst, id, ResolveType, skipLocalScope

This comment is wrong. It has the wrong bytecode name.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2208
> +    // resolve_scope dst, scope id

Ditto.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1109
> +                ASSERT_NOT_REACHED();
> +                break;

This feels wrong, AI should be able to handle a constant you don't expect. That said, I think this only allows for scope arguments, however, I think this would break if a `with` scope or some other scope was the argument to this. It seems totally possible that other scopes can be your constant argument here.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2848
> +    case ResolveClosestVarScope:

You also need a rule for IsVarScopeType here.
Did this not cause any issues for you? If seems like it should lead to a crash, so please add a test that stresses this.

> Source/JavaScriptCore/dfg/DFGNode.h:994
> +    int32_t skipScope()
> +    {
> +        return m_opInfo2.as<int32_t>();
> +    }

should be uint32_t

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2118
> +    loadisFromInstruction(4, t0)

I don't think this is needed.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2124
> +    loadisFromInstruction(4, t0)

Ditto

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2069
> +    loadisFromInstruction(4, t0)

ditto

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2075
> +    loadisFromInstruction(4, t0)

ditto

[GSkachkov]

Created attachment 297708 [details]
Patch

Fix comments

[GSkachkov]

Created attachment 297718 [details]
Patch

Small fixes

[GSkachkov]

Comment on attachment 296084 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2876
>> +        // Fixme: we do not have llint optimization for those op_codes
> 
> Please file a bug for this if you think we should do it. That said, this is way more than a LLInt optimization, knowing which scope you'll resolve to helps in all tiers.

Done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:769
>> +    }
> 
> Do you mind elaborating on your previous example for why you said this is needed, I don't quite get it.

I've removed this change. It seems that not ready yet.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2193
>> +    // resolve_scope dst, id, ResolveType, skipLocalScope
> 
> This comment is wrong. It has the wrong bytecode name.

Removed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2208
>> +    // resolve_scope dst, scope id
> 
> Ditto.

Removed

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1109
>> +                break;
> 
> This feels wrong, AI should be able to handle a constant you don't expect. That said, I think this only allows for scope arguments, however, I think this would break if a `with` scope or some other scope was the argument to this. It seems totally possible that other scopes can be your constant argument here.

Yeah. I've just removed this part. I did not managed to find case when child.value().isObject() is valid. So Only forNode(node).setType(m_graph, SpecBoolean) is left.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2848
>> +    case ResolveClosestVarScope:
> 
> You also need a rule for IsVarScopeType here.
> Did this not cause any issues for you? If seems like it should lead to a crash, so please add a test that stresses this.

Done

>> Source/JavaScriptCore/dfg/DFGNode.h:994
>> +    }
> 
> should be uint32_t

Removed

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2118
>> +    loadisFromInstruction(4, t0)
> 
> I don't think this is needed.

Removed

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2124
>> +    loadisFromInstruction(4, t0)
> 
> Ditto

Removed

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2069
>> +    loadisFromInstruction(4, t0)
> 
> ditto

Removed

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2075
>> +    loadisFromInstruction(4, t0)
> 
> ditto

Removed

[GSkachkov]

Created attachment 297742 [details]
Patch

Additional fixes

[Saam Barati]

Comment on attachment 297742 [details]
Patch

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

This code mostly LGTM, some comments below.

Also, it's worth opening a bugzilla for a bug that has existed since I implemented the first version of sloppy hoisting:
function foo() {
    {
         let bar;
         {
              function bar() { }
          }
    }
    return bar;
}
in jsc: foo() === function bar() { }
expected behavior: foo(); throws a ReferenceError
I believe your eval code exhibits the same bad behavior for local lexical variables in the eval.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2822
> +        // Fixme: https://bugs.webkit.org/show_bug.cgi?id=166418

Style: "Fixme" => "FIXME"

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2186
> +RegisterID* BytecodeGenerator::emitResolveOuterScope(RegisterID* dst, const Identifier& property)

Nit: Lets call this: "emitResolveOuterVarScope"

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2198
> +RegisterID* BytecodeGenerator::emitCheckIfScopeVarType(RegisterID* dst, RegisterID* scope)

Nit: Lets call this: "emitCheckIfScopeIsVarType"

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:829
> +void JIT::emitSlow_op_is_scope_var_type(Instruction* currentInstruction, Vector<SlowCaseEntry>::iterator& iter)
> +{
> +    linkSlowCase(iter);
> +    JITSlowPathCall slowPathCall(this, currentInstruction, slow_path_is_scope_var_type);
> +    slowPathCall.call();
> +}
> +
> +void JIT::emitSlow_op_resolve_closest_var_scope(Instruction* currentInstruction, Vector<SlowCaseEntry>::iterator& iter)
> +{
> +    linkSlowCase(iter);
> +    JITSlowPathCall slowPathCall(this, currentInstruction, slow_path_resolve_closest_var_scope);
> +    slowPathCall.call();
> +}

These shouldn't be slow paths. The opcodes should just do the call on the fast patch, and should not have slow paths.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2124
> +_llint_op_resolve_closest_var_scope:
> +    traceExecution()
> +    callOpcodeSlowPath(_slow_path_resolve_closest_var_scope)
> +    dispatch(4)
> +
> +_llint_op_is_scope_var_type:
> +    traceExecution()
> +    callOpcodeSlowPath(_slow_path_is_scope_var_type)
> +    dispatch(3)

The 32 and 64 implementations are the same, so they can just go inside LowLevelInterpreter.asm

> Source/JavaScriptCore/parser/Parser.h:639
> +                    addResult.iterator->value.setIsFunction();

Why is this change needed?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:834
> +    bool result = false;
> +    if (scope->isGlobalObject())
> +        result = true;
> +    else if (SymbolTable* scopeSymbolTable = scope->symbolTable())
> +        result = scopeSymbolTable->scopeType() == SymbolTable::ScopeType::VarScope;

This doesn't match the code inside DFGOperations. Which code is correct?

> Source/JavaScriptCore/runtime/JSScope.cpp:234
> +                if (object->hasProperty(exec, ident))
> +                    return object;

I think you need to check an exception here.

> Source/JavaScriptCore/runtime/JSScope.cpp:242
> +        if (object->hasProperty(exec, ident)) {

ditto

> JSTests/stress/eval-func-decl-within-eval-with-reassign-to-var.js:14
> +function foo() {
> +    {
> +        var f = 20;
> +        eval('var f = 15; eval(" { function f() { }; } ")');
> +        assert(typeof f, "function");
> +    }
> +    assert(typeof f, "function", "#1");
> +}

Another good test to have:
function foo() {
    eval("if (false) { function bar() { } }");
    assert(bar === undefined);
}

> JSTests/stress/eval-func-decl-within-eval-with-reassign-to-var.js:56
> +/* //TODO: Set error 
> +// Fixme: Current test does not work because it should raise exception that f could 
> +// not be redeclared
> +function goo() {
> +    {   
> +        var error = false;
> +        try {
> +            let f = 20;
> +            eval('var f = 15; eval(" { function f() { }; } ")');
> +        } catch (e) {
> +            error = e instanceof SyntaxError;
> +        }
> +        assert(error, true, "syntax error should be raisen");
> +    }
> +    assert(typeof f, "undefined", "#6");
> +}
> +
> +for (var i = 0; i < 10000; i++) {
> +    goo();
> +    assert(typeof f, "undefined", "#7");
> +}
> +*/

What's happening with this?

> JSTests/stress/eval-func-decl-within-eval-with-reassign-to-var.js:65
> +function hoo() {
> +    {
> +        let h = 20;
> +        eval('var h = 15; eval(" if (false){ function h() { }; } ");');
> +        assert(h, 15);
> +    }
> +    assert(typeof h, "undefined", "#8");
> +}

Another good test:
function foo() { let h = 20; eval("var h; if (false) { function h() { } }");  return h; }
assert(foo() === 20);

[GSkachkov]

Created attachment 298664 [details]
Patch

Fix comments

[Saam Barati]

Comment on attachment 298664 [details]
Patch

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

This LGTM, I think it's just about ready to land,  just a few remaining comments.

> Source/JavaScriptCore/bytecode/BytecodeList.json:157
> +            { "name" : "op_resolve_closest_var_scope", "length" : 4 }

I have a naming suggestion for this bytecode (and the various functions & DFG node for it)
Instead of calling it "resolve_closest_var_scope", we should maybe call it:
"resolve_closest_or_var_scope" since the intention is to do a normal scope walk, but stop if we see a var scope. The objective is not to *always* stop at the closest var scope.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:-2115
> -

revert

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:-2068
> -

revert

> Source/JavaScriptCore/parser/Parser.h:639
> +                    addResult.iterator->value.setIsFunction();

I don't think this is strictly true here.
Is this needed?

> Source/JavaScriptCore/runtime/JSScope.cpp:219
> +JSObject* JSScope::resolveClosestVar(ExecState* exec, JSScope* scope, const Identifier& ident)

This function is almost identical to the below resolve. Maybe you can combine them by defining a common function that takes some kind of shouldStopEarly lambda? And just make that function ALWAYS_INLINE.

> Source/JavaScriptCore/runtime/JSScope.cpp:231
> +            JSScope* globalScopeExtension = scope->globalObject(vm)->globalScopeExtension();

Do you know what this is? We should have a test for this if it's easily doable.

[Saam Barati]

*** Bug 167328 has been marked as a duplicate of this bug. ***

[GSkachkov]

Created attachment 299743 [details]
Patch

Fix review comments

[GSkachkov]

Created attachment 299744 [details]
Patch

Fix review comments #1

[Geoffrey Garen]

Comment on attachment 299744 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:19
> +        op_is_scope_var_type - check if scope with variable is function scope           

Let's call this op_is_var_scope. 

I don't understand your explanation "check if scope with variable is function scope", but since op_resolve_closest_or_var_scope names something a "var scope", I'm inferring that this check applies to the same thing.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:697
> +        RegisterID* emitResolveOuterVarScope(RegisterID* dst, const Identifier&);

It is bad that this function name does not match the name of the bytecode it emits.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:698
> +        RegisterID* emitCheckIfScopeIsVarType(RegisterID* dst, RegisterID* scope);

Same comment here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:699
> +        RegisterID* emitPutToOuterScope(RegisterID* scope, const Identifier&, RegisterID* value, ResolveMode, InitializationMode);

How is emitPutToOuterScope different from emitPutToScope?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:814
> +SLOW_PATH_DECL(slow_path_resolve_closest_var_scope)

You forgot the word "or" in this name, which changes its meaning significantly.

> Source/JavaScriptCore/runtime/JSScope.cpp:220
> +ALWAYS_INLINE JSObject* JSScope::baseResolve(ExecState* exec, JSScope* scope, const Identifier& ident, FinishResolveEarlierFunctor finishResolveEarlier)

There's no need to use a different function name here. You can call this resolve.

> Source/JavaScriptCore/runtime/JSScope.cpp:256
> +        if (finishResolveEarlier(scope))

I would call this 'accept' or 'predicate'.

[GSkachkov]

Created attachment 299955 [details]
Patch

Fix latest comments

[GSkachkov]

Comment on attachment 299744 [details]
Patch

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

Thanks for review!!!

>> Source/JavaScriptCore/ChangeLog:19
>> +        op_is_scope_var_type - check if scope with variable is function scope           
> 
> Let's call this op_is_var_scope. 
> 
> I don't understand your explanation "check if scope with variable is function scope", but since op_resolve_closest_or_var_scope names something a "var scope", I'm inferring that this check applies to the same thing.

I've tried to make it more clear in new patch

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:697
>> +        RegisterID* emitResolveOuterVarScope(RegisterID* dst, const Identifier&);
> 
> It is bad that this function name does not match the name of the bytecode it emits.

Fixed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:698
>> +        RegisterID* emitCheckIfScopeIsVarType(RegisterID* dst, RegisterID* scope);
> 
> Same comment here.

Fixed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:699
>> +        RegisterID* emitPutToOuterScope(RegisterID* scope, const Identifier&, RegisterID* value, ResolveMode, InitializationMode);
> 
> How is emitPutToOuterScope different from emitPutToScope?

Yeah, now it is the same. Removed

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:814
>> +SLOW_PATH_DECL(slow_path_resolve_closest_var_scope)
> 
> You forgot the word "or" in this name, which changes its meaning significantly.

Fixed

>> Source/JavaScriptCore/runtime/JSScope.cpp:220
>> +ALWAYS_INLINE JSObject* JSScope::baseResolve(ExecState* exec, JSScope* scope, const Identifier& ident, FinishResolveEarlierFunctor finishResolveEarlier)
> 
> There's no need to use a different function name here. You can call this resolve.

Fixed

>> Source/JavaScriptCore/runtime/JSScope.cpp:256
>> +        if (finishResolveEarlier(scope))
> 
> I would call this 'accept' or 'predicate'.

Renamed

[GSkachkov]

Created attachment 300628 [details]
Patch

Fix build

[GSkachkov]

Created attachment 300630 [details]
Patch

Fix build

[GSkachkov]

Created attachment 300632 [details]
Patch

Add fix me comment

[Saam Barati]

Comment on attachment 300632 [details]
Patch

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

r=me

I have a few minor comments, and I have one comment for a bug that looks real.
Otherwise, this patch looks good to go to me.

> Source/JavaScriptCore/ChangeLog:15
> +        If eval is invoke ind block scope that contains let/const 

typo: "invoke ind" => "invoked in"

> Source/JavaScriptCore/ChangeLog:20
> +        'hoist function in slommy mode' feature, with small changes.

typo: "slommy" => "sloppy"

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2825
> +        // we need add optimization for op_resolve_closest_or_var_scope and op_is_var_scope if it possible

Nit: Lets write this as:
"We need to add optimizations for op_resolve_closest_or_var_scope and op_is_var_scope to do link time scope resolution."

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1843
> +        case IsVarScope:

This should be KnownCellUse on its child1().

> Source/JavaScriptCore/dfg/DFGOperations.h:197
> +size_t JIT_OPERATION operationIsVarScope(ExecState*, JSScope*);

This looks wrong to me. operationResolveClosestOrVarScope may return to you any arbitrary JSObject because of `with` scopes.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9159
> +    JSValueOperand scope(this, node->child1());

And then this will just be SpeculateCellOperand

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9168
> +#if USE(JSVALUE64)
> +    GPRReg scopeGPR = scope.gpr();
> +    flushRegisters();
> +    callOperation(operationIsVarScope, resultGPR, scopeGPR);
> +#else
> +    JSValueRegs scopeRegs = scope.jsValueRegs();
> +    flushRegisters();
> +    callOperation(operationIsVarScope, resultGPR, scopeRegs.payloadGPR());
> +#endif

And you won't need different code for 32 bit and 64 bit, since you'll just be getting a pointer-width value.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1660
> +    JITCompiler::Call callOperation(S_JITOperation_EJsc operation, GPRReg result, GPRReg arg1)
> +    {
> +        m_jit.setupArgumentsWithExecState(arg1);
> +        return appendCallSetResult(operation, result);
> +    }

Nit:
You don't need both a 32-bit and 64-bit version of this function since they're identical. You can just move it outside the compile time conditional.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9868
> +        setBoolean(vmCall(pointerType(), m_out.operation(operationIsVarScope), m_callFrame, lowCell(m_node->child1())));

You use lowCell, which is why you need to make sure that this is KnownCellUse.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:829
> +    JSScope* scope = exec->uncheckedR(pc[2].u.operand).Register::scope();

Same comment as in DFG code:
This looks wrong to me. op_resolve_closest_or_var_scope may return to you any arbitrary JSObject because of `with` scopes.
Can you please make sure to have a test for this? It should probably be an insta-crash.

[GSkachkov]

Created attachment 301063 [details]
Patch

Fix comments

[Saam Barati]

Comment on attachment 301063 [details]
Patch

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

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:833
> +    if (JSScope* scope = jsDynamicCast<JSScope*>(exec->vm(), object)) {

You already have a VM available here, no need to exec->vm().

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:837
> +            result = ((JSCell*)object)->isObject();

This should always be true.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:839
> +        result = ((JSCell*)object)->isObject();

This should always be true. If it resolved to a with scope, with scopes can only be over objects, I think. It's worth verifying I'm correct. I suspect `with(foo) { ... }` will always toObject the `foo`.

[Build Bot]

Comment on attachment 301063 [details]
Patch

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

New failing tests:
fast/scrolling/ios/scroll-events-back-forward.html

[Build Bot]

Created attachment 301103 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[GSkachkov]

Created attachment 301136 [details]
Patch

Fix last comments

[GSkachkov]

Created attachment 301145 [details]
Patch

Improve code readability

[GSkachkov]

Created attachment 301171 [details]
Patch

Remove unnecessary variable declaration

[GSkachkov]

Created attachment 301177 [details]
Patch

Rebase

[Saam Barati]

Comment on attachment 301177 [details]
Patch

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

I think your code may be wrong for `with` scope.
Consider this example:

```
function foo() {
    let o = {f: 20};
    with (o) {
        eval("{ function f(){} }");
    }
    assert(o.f === 20);
}
foo()
```

I think your code will fail the assertion. Please let me know if I'm wrong and misreading your code.
I'm pretty sure the spec says that we should not bind to `f` unless `f` is a var, and in this case, it's
not a var.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:832
> +    bool result = true;

I think this should start as false.

[GSkachkov]

Created attachment 301300 [details]
Patch

Fix with case

[Saam Barati]

Comment on attachment 301300 [details]
Patch

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

I'm leaving as r? given my question about with scopes.

> Source/JavaScriptCore/ChangeLog:24
> +        In short it works in following way: during hoisting of function 
> +        with name S in eval, we are looking for first scope that 
> +        contains space for variable with name S and if this scope 
> +        has var type we bind function there

This code I believe is slightly wrong w.r.t the specification.
The behavior we want is not to always create the binding and then to conditionally bind, but rather, to conditionally create the binding, and to bind to it unconditionally if we created it.

For example, I checked out your code, and this function won't throw an exception. It'll return undefined. I believe the desired behavior is to throw a ReferenceError:

```
function foo() {
    {
        let f;
        eval("{ function f() { return 20; } }");
    }
    return f;
}
foo();
```

That said, I think this patch still does the majority of the work to implementing what I said above. To get the behavior I said above, I think all you'll need to do is change code inside Interpreter.cpp when running eval code.
I think it's best at this point to do this fix as a follow up.

> Source/JavaScriptCore/runtime/JSScope.cpp:248
> +        if (UNLIKELY(doNotCountWithScope) && scope->isWithScope())
> +            continue;

What does the spec say here? Does it say to skip over `with`? Or does it say to iterate the scope chain during `eval` instantiation.
This is important because the hasProperty below is observable for `with` scopes.

Also, thinking about this patch more, I think the resolve_closest_or_var and is_var_scope_type could be fully answerable during bytecode linking if the spec behavior is to skip over `with` scopes.
So we need to find out if we indeed skip `with` scopes, or we somehow interact with them when creating the eval.

[Saam Barati]

If you run this program in Chrome Canary, you can see V8's behavior here w.r.t the specification. It looks like the eval does indeed perform `has` on the thing inside the with scope.

```
function foo() {
    let p2 = new Proxy({}, {
        has(t, p) {
            console.log(p)
            return Reflect.has(t, p);
        }
    });
    with (p2) {
        eval("Reflect.has(p2, 'bar'); { function baz() { };  }");
    }
}
foo();
```

I'm not sure if V8 is correct here, but we should verify the correct behavior and try to get that landed. V8 will have the following output:

```
eval
Reflect
p2
bar
baz
```

[Saam Barati]

Firefox nightly outputs:

```
eval
eval
Reflect
p2
bar
```

Which looks pretty obviously wrong w.r.t looking up `eval` twice. But they don't look up `baz` at all. So perhaps the spec says to skip `with` scopes here.

[GSkachkov]

Comment on attachment 301300 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:24
>> +        has var type we bind function there
> 
> This code I believe is slightly wrong w.r.t the specification.
> The behavior we want is not to always create the binding and then to conditionally bind, but rather, to conditionally create the binding, and to bind to it unconditionally if we created it.
> 
> For example, I checked out your code, and this function won't throw an exception. It'll return undefined. I believe the desired behavior is to throw a ReferenceError:
> 
> ```
> function foo() {
>     {
>         let f;
>         eval("{ function f() { return 20; } }");
>     }
>     return f;
> }
> foo();
> ```
> 
> That said, I think this patch still does the majority of the work to implementing what I said above. To get the behavior I said above, I think all you'll need to do is change code inside Interpreter.cpp when running eval code.
> I think it's best at this point to do this fix as a follow up.

Yeah, my bad, I see my patch is only focus to conditionally bind.

>> Source/JavaScriptCore/runtime/JSScope.cpp:248
>> +            continue;
> 
> What does the spec say here? Does it say to skip over `with`? Or does it say to iterate the scope chain during `eval` instantiation.
> This is important because the hasProperty below is observable for `with` scopes.
> 
> Also, thinking about this patch more, I think the resolve_closest_or_var and is_var_scope_type could be fully answerable during bytecode linking if the spec behavior is to skip over `with` scopes.
> So we need to find out if we indeed skip `with` scopes, or we somehow interact with them when creating the eval.

According to spec B.3.3.3.1.d.ii.4 we do loop over EnvironmentRecord, and  only B.3.3.3.1.d.ii.4.b we do check 'If thisEnvRec is not an _object_ Environment Record'
and then in B.3.3.3.1.d.ii.4.b.i we do check hasBinding for found object. So how I can understand we need to skip 'with' scope

[GSkachkov]

Created attachment 301902 [details]
Patch

New approach

[GSkachkov]

Created attachment 301950 [details]
Patch

Fix build

[GSkachkov]

Created attachment 301960 [details]
Patch

Fix 32bit

[GSkachkov]

Created attachment 302075 [details]
Patch

Add conditional creation of binding

[GSkachkov]

Created attachment 302079 [details]
Patch

Add conditional creation of binding and add new test

[Saam Barati]

Comment on attachment 302079 [details]
Patch

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

Some comments below. Mostly looks good, but I think there might be a few bugs.

> Source/JavaScriptCore/ChangeLog:18
> +        we do not bind. In simple patch should lead to following 
> +        behavior:

"In simple patch should lead to following behavior" => "This patch leads to the following behavior"

> Source/JavaScriptCore/ChangeLog:29
> +        boo();

foo

> Source/JavaScriptCore/ChangeLog:51
> +        

you need a call here to match the other ones. Or you can just remove the calls from your example, I think it's clear what you mean without them.

> Source/JavaScriptCore/ChangeLog:59
> +        function bas() {
> +            {
> +                 let boo = 10;
> +                 eval(' { function boo() {} } ');
> +                 print(boo); // 10
> +            }
> +            print(boo); //Reference Error
> +        }

Can you open a bug to fix this issue:
(This exists both for eval and normal function hoisting):
eval(" { let foo; {function foo(){} } }");

> Source/JavaScriptCore/ChangeLog:72
> +        or return undefined if variable can be binded there.

can => can't

> Source/JavaScriptCore/ChangeLog:75
> +        that is not covered by this path, and will be fixed in 

Typo: path => patch

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:767
> +    for (FunctionMetadataNode* function : evalNode->functionStack())
> +        m_codeBlock->addFunctionDecl(makeFunction(function));

Doesn't this still cause us to unconditionally create the binding?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:770
> +    // unsigned numVariables = varDeclarations.size();

Please remove.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:779
> +        if (!entry.value.isFunction())
> +            variables.append(Identifier::fromUid(m_vm, entry.key.get()));
> +        else
> +            hoistedFunctions.append(Identifier::fromUid(m_vm, entry.key.get()));

This looks wrong to me and looks like it'd cause weird behavior for strict mode. I think you want to check if it's a hoisting candidate instead.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1076
> +    if ((numVariables || numFunctions || numHoistedFunctions) && eval->isStrictMode()) {

I don't think you want this number to be > 0 in strict mode.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:827
> +SLOW_PATH_DECL(slow_path_is_var_scope)

Please remove.

> Source/JavaScriptCore/runtime/JSScope.cpp:220
> +ALWAYS_INLINE JSObject* JSScope::resolve(ExecState* exec, JSScope* scope, const Identifier& ident, FinishResolveEarlierFunctor predicate, bool doNotCountWithScope)

Nit: I'd just make doNoutCountWithScope  a templatized variable.

[GSkachkov]

Created attachment 302546 [details]
Patch

Fixed comments

[Build Bot]

Comment on attachment 302546 [details]
Patch

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

New failing tests:
fast/dom/timer-throttling-hidden-page-non-nested.html

[Build Bot]

Created attachment 302561 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Saam Barati]

Comment on attachment 302546 [details]
Patch

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

Mostly LGTM. Just a couple of questions and a recommendation.

> Source/JavaScriptCore/bytecode/BytecodeList.json:156
> +            { "name" : "op_resolve_scope_for_binding_func_decl_in_eval", "length" : 4 }

Can we call this:
"op_resolve_scope_for_hoisting_func_decl_in_eval"
I think having the word "hoisting" in here will help everyone better understand what this opcode is doing.
(Also, can you change the various C++ functions that have "binding" in the name to "hoisting" or "hoist")

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1063
> +        case ResolveScopeForBindingFuncDeclInEval:
> +            compileResolveScopeForBindingFuncDeclInEval();
> +            break;

You need to say that the FTL supports this opcode inside FTLCapabilities.cpp.
Please verify that this gets compiled in some of your tests, or add new tests that make it to the FTL.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1148
> +            for (int i = 0; i < numFunctions; ++i) {

Why are there both "numFunctions" and "numHoistedFunctions". Isn't every function declaration in an eval a hoisting candidate?

[GSkachkov]

Created attachment 304941 [details]
Patch

Fix comments

[GSkachkov]

Comment on attachment 302546 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/BytecodeList.json:156
>> +            { "name" : "op_resolve_scope_for_binding_func_decl_in_eval", "length" : 4 }
> 
> Can we call this:
> "op_resolve_scope_for_hoisting_func_decl_in_eval"
> I think having the word "hoisting" in here will help everyone better understand what this opcode is doing.
> (Also, can you change the various C++ functions that have "binding" in the name to "hoisting" or "hoist")

Fixed

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1063
>> +            break;
> 
> You need to say that the FTL supports this opcode inside FTLCapabilities.cpp.
> Please verify that this gets compiled in some of your tests, or add new tests that make it to the FTL.

Fixed
I added RELEASE_ASSERT_.. and receive following tests failed:	stress/eval-func-decl-block-with-remove.js.ftl-eager
	stress/eval-func-decl-block-with-remove.js.ftl-eager-no-cjit
	stress/eval-func-decl-block-with-var-and-remove.js.ftl-eager
	stress/eval-func-decl-block-with-var-and-remove.js.ftl-eager-no-cjit
	stress/eval-func-decl-block-with-var-sinthesize.js.ftl-eager
	stress/eval-func-decl-block-with-var-sinthesize.js.ftl-eager-no-cjit
	stress/eval-func-decl-in-eval-within-block-with-let.js.ftl-eager
	stress/eval-func-decl-in-eval-within-block-with-let.js.ftl-eager-no-cjit
	stress/eval-func-decl-in-eval-within-with-scope.js.default
	stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager
	stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit
	stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-no-inline-validate
	stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-no-put-stack-validate
	stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-validate-sampling-profiler
	stress/eval-func-decl-within-eval-with-reassign-to-var.js.ftl-eager
	stress/eval-func-decl-within-eval-with-reassign-to-var.js.ftl-eager-no-cjit
	stress/eval-func-decl-within-eval-without-reassign-to-let.js.ftl-eager
	stress/eval-func-decl-within-eval-without-reassign-to-let.js.ftl-eager-no-cjit
So I think it covered by Tests

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:1148
>> +            for (int i = 0; i < numFunctions; ++i) {
> 
> Why are there both "numFunctions" and "numHoistedFunctions". Isn't every function declaration in an eval a hoisting candidate?

We have topLevel function that is list of the declared functions, and list of the function names from codeBlockes, that later will be bounded to function. 
I've renamed to correspond this difference. Could you please look if it more clear?

[Saam Barati]

Comment on attachment 304941 [details]
Patch

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

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1095
> +    int numTopLevelFunctionDecls = eval->numTopLevelFunctionDecls();
> +    unsigned numCodeBlockFunctionDecls = eval->numCodeBlockFunctionDecls();

These names are not better IMO.

I still don't understand what the difference here is. Can you explain this to me with an example?

[GSkachkov]

(In reply to Saam Barati from comment #61)
> Comment on attachment 304941 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304941&action=review
> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:1095
> > +    int numTopLevelFunctionDecls = eval->numTopLevelFunctionDecls();
> > +    unsigned numCodeBlockFunctionDecls = eval->numCodeBlockFunctionDecls();
> 
> These names are not better IMO.
> 
> I still don't understand what the difference here is. Can you explain this
> to me with an example?

As I remember spec, top level function in eval might lead to error of redeclaration. 
```
function bar() {
    {
        let f = 20;
        let value = 10; 
        eval("function f() { value = 20; }; f();");
        print(value);
    }
}
bar(); // SyntaxError: Can't create duplicate variable in eval: 'f'
``` 

but function declared in code block not lead to error
```
function baz() {
    {
        let f = 20;
        let value = 10; 
        eval("{ function f() { value = 20; }; f();}");
        print(value);
    }
}
baz(); // 20
``` 
So we need to differentiate them.

[Caitlin Potter (:caitp)]

(In reply to Saam Barati from comment #46)
> If you run this program in Chrome Canary, you can see V8's behavior here
> w.r.t the specification. It looks like the eval does indeed perform `has` on
> the thing inside the with scope.
> 
> ```
> function foo() {
>     let p2 = new Proxy({}, {
>         has(t, p) {
>             console.log(p)
>             return Reflect.has(t, p);
>         }
>     });
>     with (p2) {
>         eval("Reflect.has(p2, 'bar'); { function baz() { };  }");
>     }
> }
> foo();
> ```
> 
> I'm not sure if V8 is correct here, but we should verify the correct
> behavior and try to get that landed. V8 will have the following output:
> 
> ```
> eval
> Reflect
> p2
> bar
> baz
> ```

I believe this is correct, per https://tc39.github.io/ecma262/#sec-object-environment-records-hasbinding-n and https://tc39.github.io/ecma262/#sec-object-environment-records-getbindingvalue-n-s

[Saam Barati]

Comment on attachment 304941 [details]
Patch

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

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1094
> +    int numTopLevelFunctionDecls = eval->numTopLevelFunctionDecls();

I know this is how the code was before, but let's make this function return an unsigned integer. It's dumb that this is a signed int.

>>> Source/JavaScriptCore/interpreter/Interpreter.cpp:1095
>>> +    unsigned numCodeBlockFunctionDecls = eval->numCodeBlockFunctionDecls();
>> 
>> These names are not better IMO.
>> 
>> I still don't understand what the difference here is. Can you explain this to me with an example?
> 
> As I remember spec, top level function in eval might lead to error of redeclaration. 
> ```
> function bar() {
>     {
>         let f = 20;
>         let value = 10; 
>         eval("function f() { value = 20; }; f();");
>         print(value);
>     }
> }
> bar(); // SyntaxError: Can't create duplicate variable in eval: 'f'
> ``` 
> 
> but function declared in code block not lead to error
> ```
> function baz() {
>     {
>         let f = 20;
>         let value = 10; 
>         eval("{ function f() { value = 20; }; f();}");
>         print(value);
>     }
> }
> baz(); // 20
> ``` 
> So we need to differentiate them.

Ok. I understand what's going on now.
I like the name top level function. For the "CodeBlockFunctions", I'll propose a different name: "functionHoistingCandidates" or something similar.
This is more in line with our terminology elsewhere.

[GSkachkov]

(In reply to Caitlin Potter (:caitp) from comment #63)
> (In reply to Saam Barati from comment #46)
> > If you run this program in Chrome Canary, you can see V8's behavior here
> > w.r.t the specification. It looks like the eval does indeed perform `has` on
> > the thing inside the with scope.
> > 
> > ```
> > function foo() {
> >     let p2 = new Proxy({}, {
> >         has(t, p) {
> >             console.log(p)
> >             return Reflect.has(t, p);
> >         }
> >     });
> >     with (p2) {
> >         eval("Reflect.has(p2, 'bar'); { function baz() { };  }");
> >     }
> > }
> > foo();
> > ```
> > 
> > I'm not sure if V8 is correct here, but we should verify the correct
> > behavior and try to get that landed. V8 will have the following output:
> > 
> > ```
> > eval
> > Reflect
> > p2
> > bar
> > baz
> > ```
> 
> I believe this is correct, per
> https://tc39.github.io/ecma262/#sec-object-environment-records-hasbinding-n
> and
> https://tc39.github.io/ecma262/#sec-object-environment-records-
> getbindingvalue-n-s

Hmm, I stil think that it should not be printed 'baz', because according to the p.B.3.3.3.1.d.ii.4.b('If thisEnvRec is not an object Environment Record, then') we don't invoke hasBinding, and just go to outer env recode
https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation

[Caitlin Potter (:caitp)]

(In reply to GSkachkov from comment #65)
> (In reply to Caitlin Potter (:caitp) from comment #63)
> > (In reply to Saam Barati from comment #46)
> > > If you run this program in Chrome Canary, you can see V8's behavior here
> > > w.r.t the specification. It looks like the eval does indeed perform `has` on
> > > the thing inside the with scope.
> > > 
> > > ```
> > > function foo() {
> > >     let p2 = new Proxy({}, {
> > >         has(t, p) {
> > >             console.log(p)
> > >             return Reflect.has(t, p);
> > >         }
> > >     });
> > >     with (p2) {
> > >         eval("Reflect.has(p2, 'bar'); { function baz() { };  }");
> > >     }
> > > }
> > > foo();
> > > ```
> > > 
> > > I'm not sure if V8 is correct here, but we should verify the correct
> > > behavior and try to get that landed. V8 will have the following output:
> > > 
> > > ```
> > > eval
> > > Reflect
> > > p2
> > > bar
> > > baz
> > > ```
> > 
> > I believe this is correct, per
> > https://tc39.github.io/ecma262/#sec-object-environment-records-hasbinding-n
> > and
> > https://tc39.github.io/ecma262/#sec-object-environment-records-
> > getbindingvalue-n-s
> 
> Hmm, I stil think that it should not be printed 'baz', because according to
> the p.B.3.3.3.1.d.ii.4.b('If thisEnvRec is not an object Environment Record,
> then') we don't invoke hasBinding, and just go to outer env recode
> https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation

As I said on IRC, I wasn't commenting on the hoisting of baz, because the behaviour of block scoping of functions and annex B changes to that are somewhat hard to follow.

It looks like you're right about that HasProperty() not occurring from step 5, but it may occur further down in the algorithm, from `Let fobj be ! benvRec.GetBindingValue(F, false).`, as I believe in this instance, benv still refers to the with block.

I could be wrong about this, Adam Klein is much more knowledgable about the block-scoping of functions than I am.

[GSkachkov]

Created attachment 306399 [details]
Patch

Patch with fixes

[GSkachkov]

(In reply to Caitlin Potter (:caitp) from comment #66)
> (In reply to GSkachkov from comment #65)
> > (In reply to Caitlin Potter (:caitp) from comment #63)
> > > (In reply to Saam Barati from comment #46)
> > > > If you run this program in Chrome Canary, you can see V8's behavior here
> > > > w.r.t the specification. It looks like the eval does indeed perform `has` on
> > > > the thing inside the with scope.
> > > > 
> > > > ```
> > > > function foo() {
> > > >     let p2 = new Proxy({}, {
> > > >         has(t, p) {
> > > >             console.log(p)
> > > >             return Reflect.has(t, p);
> > > >         }
> > > >     });
> > > >     with (p2) {
> > > >         eval("Reflect.has(p2, 'bar'); { function baz() { };  }");
> > > >     }
> > > > }
> > > > foo();
> > > > ```
> > > > 
> > > > I'm not sure if V8 is correct here, but we should verify the correct
> > > > behavior and try to get that landed. V8 will have the following output:
> > > > 
> > > > ```
> > > > eval
> > > > Reflect
> > > > p2
> > > > bar
> > > > baz
> > > > ```
> > > 
> > > I believe this is correct, per
> > > https://tc39.github.io/ecma262/#sec-object-environment-records-hasbinding-n
> > > and
> > > https://tc39.github.io/ecma262/#sec-object-environment-records-
> > > getbindingvalue-n-s
> > 
> > Hmm, I stil think that it should not be printed 'baz', because according to
> > the p.B.3.3.3.1.d.ii.4.b('If thisEnvRec is not an object Environment Record,
> > then') we don't invoke hasBinding, and just go to outer env recode
> > https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation
> 
> As I said on IRC, I wasn't commenting on the hoisting of baz, because the
> behaviour of block scoping of functions and annex B changes to that are
> somewhat hard to follow.
> 
> It looks like you're right about that HasProperty() not occurring from step
> 5, but it may occur further down in the algorithm, from `Let fobj be !
> benvRec.GetBindingValue(F, false).`, as I believe in this instance, benv
> still refers to the with block.
> 
> I could be wrong about this, Adam Klein is much more knowledgable about the
> block-scoping of functions than I am.

Oh sorry, I misunderstood your message in IRC. I'll try to chat with Adam Klein.

[Saam Barati]

Comment on attachment 306399 [details]
Patch

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

Almost everything LGTM, just 1 fundamental question below:

> Source/JavaScriptCore/bytecode/UnlinkedEvalCodeBlock.h:60
> +    void adoptFunctionHoistingCandidates(Vector<Identifier, 0, UnsafeVectorOverflow>& functionHoistingCandidates)
> +    {
> +        ASSERT(m_functionHoistingCandidates.isEmpty());
> +        m_functionHoistingCandidates.swap(functionHoistingCandidates);
> +    }

Style nit: You can use C++ move semantics here and do something like:
adopt(Vector<T>&& v) { ASSERT(m_v.isEmpty()); m_v = WTFMove(v); }

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:771
> +        if (!entry.value.isSloppyModeHoistingCandidate())
> +            variables.append(Identifier::fromUid(m_vm, entry.key.get()));
> +        else
> +            hoistedFunctions.append(Identifier::fromUid(m_vm, entry.key.get()));

Style nit:
I think this is easier to read if you flip the if and else and remove the "!" on the if's condition.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2225
> +            ASSERT_UNUSED(checkResult.get(), checkResult.get() != nullptr);

no need for ASSERT_UNUSED here, can just be a regular ASSERT.
ASSERT_UNUSED is only for variables, not temp expressions

> Source/JavaScriptCore/parser/Parser.h:1201
> +            bool isSloppyModeHoistingCandidate = !strictMode() && closestParentOrdinaryFunctionNonLexicalScope()->isEvalContext();

This can only be true if m_statementDepth == 1, however, isn't this just a top level function if m_statementDepth == 1? Why is this needed here then?

[GSkachkov]

Created attachment 307127 [details]
Patch

Fix latest comments

[GSkachkov]

Comment on attachment 306399 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/UnlinkedEvalCodeBlock.h:60
>> +    }
> 
> Style nit: You can use C++ move semantics here and do something like:
> adopt(Vector<T>&& v) { ASSERT(m_v.isEmpty()); m_v = WTFMove(v); }

Done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:771
>> +            hoistedFunctions.append(Identifier::fromUid(m_vm, entry.key.get()));
> 
> Style nit:
> I think this is easier to read if you flip the if and else and remove the "!" on the if's condition.

Done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2225
>> +            ASSERT_UNUSED(checkResult.get(), checkResult.get() != nullptr);
> 
> no need for ASSERT_UNUSED here, can just be a regular ASSERT.
> ASSERT_UNUSED is only for variables, not temp expressions

I've just remove ASSERT, not sure if it necessary at this place

>> Source/JavaScriptCore/parser/Parser.h:1201
>> +            bool isSloppyModeHoistingCandidate = !strictMode() && closestParentOrdinaryFunctionNonLexicalScope()->isEvalContext();
> 
> This can only be true if m_statementDepth == 1, however, isn't this just a top level function if m_statementDepth == 1? Why is this needed here then?

Ohhh, you are right.I think it should be just false

[Build Bot]

Comment on attachment 307127 [details]
Patch

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

New failing tests:
webrtc/multi-video.html

[Build Bot]

Created attachment 307138 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Saam Barati]

Comment on attachment 307127 [details]
Patch

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

Nice! r=me

> Source/JavaScriptCore/parser/Parser.cpp:1784
> +    if (!currentScope()->isFunction() && !closestParentOrdinaryFunctionNonLexicalScope()->isEvalContext()) {

Please update the comment below, since you're now adding this for eval.

[GSkachkov]

Committed r215476: <http://trac.webkit.org/changeset/215476>

[GSkachkov]

Patch landed

[GSkachkov]

Comment on attachment 307127 [details]
Patch

Patch landed, clear flags

[Joseph Pecoraro]

This appears to have caused 1 new Test262 failure:
https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20Test262%20%28Tests%29/builds/211

> test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: Exception: SyntaxError: Can't create duplicate variable in eval: 'err'
> test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: at var-env-lower-lex-catch-non-strict.js:28
> test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: eval@[native code]
> test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: global [e-mail]:28:7
> test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: ERROR: Unexpected exit code: 3
> FAIL: test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default

Note we are going to make the bot correctly turn red on failures with:
https://bugs.webkit.org/show_bug.cgi?id=171044

[GSkachkov]

(In reply to Joseph Pecoraro from comment #78)
> This appears to have caused 1 new Test262 failure:
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20Test262%20%28Tests%29/builds/211
> 
> > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: Exception: SyntaxError: Can't create duplicate variable in eval: 'err'
> > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: at var-env-lower-lex-catch-non-strict.js:28
> > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: eval@[native code]
> > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: global [e-mail]:28:7
> > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: ERROR: Unexpected exit code: 3
> > FAIL: test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default
> 
> Note we are going to make the bot correctly turn red on failures with:
> https://bugs.webkit.org/show_bug.cgi?id=171044

Ohh, I'll take look to it in 171055. Before I created separated issue 168184 for case that is covered by this test.

[Saam Barati]

Was there a plan for this to fix this test:

```
eval("'use strict'; const x = 10; function f() {console.log(x);} f()") 
```
This still throws a ReferenceError.

[GSkachkov]

(In reply to Saam Barati from comment #80)
> Was there a plan for this to fix this test:
> 
> ```
> eval("'use strict'; const x = 10; function f() {console.log(x);} f()") 
> ```
> This still throws a ReferenceError.

Yeah, we started from this issue, but later I removed code that fix exact case, because it doesn't fix more complex cases. I think I'll switch to this issue 161099

[WebKit Commit Bot]

Re-opened since this is blocked by bug 171304

[Saam Barati]

This patch broke JSBench.
You can run JSBench locally by going to:
http://jsbench.cs.purdue.edu/suite/jsbench-2013.1/harness.html

With this change applied, the benchmark never gets past stage 1.

It seems like other browsers implement this behavior, but also work in JSBench. So perhaps it's a bug in our implementation? I haven't looked into it besides bisecting the change that broke JSBench. I guess it's possible that it's also some kind of JIT bug.

[GSkachkov]

(In reply to Saam Barati from comment #83)
> This patch broke JSBench.
> You can run JSBench locally by going to:
> http://jsbench.cs.purdue.edu/suite/jsbench-2013.1/harness.html
> 
> With this change applied, the benchmark never gets past stage 1.
> 
> It seems like other browsers implement this behavior, but also work in
> JSBench. So perhaps it's a bug in our implementation? I haven't looked into
> it besides bisecting the change that broke JSBench. I guess it's possible
> that it's also some kind of JIT bug.

I'm looking to it. 
I see error in console "ReferenceError: Can't find variable: adx_sh_291332"
I'll check why it is happens.

[GSkachkov]

(In reply to Saam Barati from comment #83)
> This patch broke JSBench.
> You can run JSBench locally by going to:
> http://jsbench.cs.purdue.edu/suite/jsbench-2013.1/harness.html
> 
> With this change applied, the benchmark never gets past stage 1.
> 
> It seems like other browsers implement this behavior, but also work in
> JSBench. So perhaps it's a bug in our implementation? I haven't looked into
> it besides bisecting the change that broke JSBench. I guess it's possible
> that it's also some kind of JIT bug.

I managed to find root of the issue, and it was already existed issue I created separate bug 171456. Current patch just make this bug visible in JSBench. Will prepare rebased patch after fixing 171456

[GSkachkov]

Created attachment 308677 [details]
Patch

Rebased patch with suppresed error in var-env-lower-lex-catch-non-stric. This test will be fixed in separate issue

[Saam Barati]

(In reply to GSkachkov from comment #86)
> Created attachment 308677 [details]
> Patch
> 
> Rebased patch with suppresed error in var-env-lower-lex-catch-non-stric.
> This test will be fixed in separate issue

No need to put this back up for review since you didn't change the patch and you landed the fix to the preexisting bug separately. You can just reland this change.

[GSkachkov]

(In reply to Saam Barati from comment #87)
> (In reply to GSkachkov from comment #86)
> > Created attachment 308677 [details]
> > Patch
> > 
> > Rebased patch with suppresed error in var-env-lower-lex-catch-non-stric.
> > This test will be fixed in separate issue
> 
> No need to put this back up for review since you didn't change the patch and
> you landed the fix to the preexisting bug separately. You can just reland
> this change.

👌  There is result of the JSBench on my Mac:
JSBench Harness
Final results:
  13.72ms ± 9.39% (lower is better)
  Standard deviation = 20.09% of mean
  Standard error = 4.49% of mean
  23 runs

[GSkachkov]

Landed https://trac.webkit.org/r215984.

[GSkachkov]

Comment on attachment 308677 [details]
Patch

Clear flags

[Maciej Stachowiak]

Comment on attachment 304941 [details]
Patch

Unflagging this old onbsoleted patch on a resolved bug.