[Webkit-unassigned] [Bug 149615] [ES6] Arrow function syntax. Arrow function specific features. Lexical bind "super" property

[bugzilla-daemon at webkit.org]

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

--- Comment #13 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 267799
  --> https://bugs.webkit.org/attachment.cgi?id=267799
Patch

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

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:69
> +    // this executable is part of the class

comment isn't needed

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:575
> +    if (SourceParseMode::ArrowFunctionMode == parseMode && (functionNode->usesThis() || isDerivedClassContext() || isDerivedConstructorContext()))

Why are these new conditions needed?
Why do we need to load this if we're "SourceParseMode::ArrowFunctionMode == parseMode && (isDerivedClassContext() || isDerivedConstructorContext())"?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4050
> +    if ((isConstructor() && constructorKind() == ConstructorKind::Derived) || (m_codeBlock->isClassContext())) {

style: the parens around "m_codeBlock->isClassContext()" aren't needed.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:813
> +                bool newisDerivedConstructorContext = constructorKind() == ConstructorKind::Derived || (derivedContextType() == DerivedContextType::DerivedConstructorContext);
> +            
> +                if (newisDerivedConstructorContext)
> +                    newDerivedContextType = DerivedContextType::DerivedConstructorContext;
> +                else  {
> +                    bool isArrowFunctionInClassContext = m_codeBlock->isClassContext() || (m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext);
> +                    newDerivedContextType = isArrowFunctionInClassContext ? DerivedContextType::DerivedMethodContext : DerivedContextType::None;
> +                }

Nit: I think this whole section of code would be easier to read like this:
```
if (constructorKind() == ConstructorKind::Derived || isDerivedConstructorContext())
    newDerivedContextType = DerivedContextType::DerivedConstructorContext;
else if (m_codeBlock->isClassContext() || (m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext))
    newDerivedContextType =  DerivedContextType::DerivedMethodContext
```

Also, why do we need to check "m_codeBlock->isArrowFunction()" when checking "m_derivedContextType == DerivedContextType::DerivedMethodContext"?
When would we have "!m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext"?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:182
> +        RegisterID* scope = generator.emitLoadDerivedConstructorFromArrowFunctionLexicalEnvironment();    

This isn't a JSScope. Maybe a better variable name is "derivedConstructor" or something similar.

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:195
> +    EvalExecutable* eval = EvalExecutable::create(callFrame, makeSource(script), codeBlock.isStrictMode(), thisTDZMode, codeBlock.unlinkedCodeBlock()->derivedContextType(), codeBlock.unlinkedCodeBlock()->isArrowFunction(), &variablesUnderTDZ);

have you tried this out inside the inspector to make sure it works?
I.e, pausing inside an arrow function and typing in "super" into the console?

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:160
> +      erorr = true;

typo: "erorr" => "error"

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:162
> +    testCase(erorr, true, 'Error: using "super" should lead to error');

ditto

> LayoutTests/js/script-tests/arrowfunction-superproperty.js:78
> +// FIXME: Problem with access to the super before super in constructor

nit: "super before super" => "super before super()"

-- 
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/20151229/580c3e74/attachment-0001.html>