[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 #294533|review?                     |review-
              Flags|                            |

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

-- 
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/20161128/27767b1e/attachment-0001.html>