[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

Saam Barati <sbarati at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #302079|review?                     |review-
              Flags|                            |

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

-- 
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/20170220/2e3ebdf5/attachment-0001.html>