[Webkit-unassigned] [Bug 163208] [ES6]. Implement Annex B.3.3 function hoisting rules for eval

[bugzilla-daemon at webkit.org]

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

--- Comment #22 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 297742
  --> https://bugs.webkit.org/attachment.cgi?id=297742
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);

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170111/512e2b65/attachment.html>