<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6]. Implement Annex B.3.3 function hoisting rules for eval"
href="https://bugs.webkit.org/show_bug.cgi?id=163208#c22">Comment # 22</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6]. Implement Annex B.3.3 function hoisting rules for eval"
href="https://bugs.webkit.org/show_bug.cgi?id=163208">bug 163208</a>
from <span class="vcard"><a class="email" href="mailto:sbarati@apple.com" title="Saam Barati <sbarati@apple.com>"> <span class="fn">Saam Barati</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=297742&action=diff" name="attach_297742" title="Patch">attachment 297742</a> <a href="attachment.cgi?id=297742&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=297742&action=review">https://bugs.webkit.org/attachment.cgi?id=297742&action=review</a>
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.
<span class="quote">> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2822
> + // Fixme: <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add llint optimization for op_resolve_closest_var_scope and op_is_scope_var_type op codes"
href="show_bug.cgi?id=166418">https://bugs.webkit.org/show_bug.cgi?id=166418</a></span >
Style: "Fixme" => "FIXME"
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2186
> +RegisterID* BytecodeGenerator::emitResolveOuterScope(RegisterID* dst, const Identifier& property)</span >
Nit: Lets call this: "emitResolveOuterVarScope"
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2198
> +RegisterID* BytecodeGenerator::emitCheckIfScopeVarType(RegisterID* dst, RegisterID* scope)</span >
Nit: Lets call this: "emitCheckIfScopeIsVarType"
<span class="quote">> 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();
> +}</span >
These shouldn't be slow paths. The opcodes should just do the call on the fast patch, and should not have slow paths.
<span class="quote">> 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)</span >
The 32 and 64 implementations are the same, so they can just go inside LowLevelInterpreter.asm
<span class="quote">> Source/JavaScriptCore/parser/Parser.h:639
> + addResult.iterator->value.setIsFunction();</span >
Why is this change needed?
<span class="quote">> 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;</span >
This doesn't match the code inside DFGOperations. Which code is correct?
<span class="quote">> Source/JavaScriptCore/runtime/JSScope.cpp:234
> + if (object->hasProperty(exec, ident))
> + return object;</span >
I think you need to check an exception here.
<span class="quote">> Source/JavaScriptCore/runtime/JSScope.cpp:242
> + if (object->hasProperty(exec, ident)) {</span >
ditto
<span class="quote">> 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");
> +}</span >
Another good test to have:
function foo() {
eval("if (false) { function bar() { } }");
assert(bar === undefined);
}
<span class="quote">> 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");
> +}
> +*/</span >
What's happening with this?
<span class="quote">> 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");
> +}</span >
Another good test:
function foo() { let h = 20; eval("var h; if (false) { function h() { } }"); return h; }
assert(foo() === 20);</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>