Bug 166665 – Super property access in base class constructor doesn't work

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Adam Klein]

Reproduction in jsc:

>>> class C { constructor() { print(typeof super.hasOwnProperty) } } new C()
Exception: TypeError: undefined is not an object (evaluating 'print')

This seems to work OK for derived classes:

>>> class Base { method() { } } class Derived extends Base { constructor() { super(); print(typeof super.method) } } new Derived()
function
[object Object]

[Saam Barati]

This sounds like we're special casing being derived or not.

[GSkachkov]

(In reply to comment #1)
> This sounds like we're special casing being derived or not.

Yeah, I remember there was "SyntaxError: super can't be used in such context", will try to check later.

[GSkachkov]

Created attachment 298851 [details]
Patch

Load super for all classes

[Yusuke Suzuki]

Comment on attachment 298851 [details]
Patch

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

r=me with comment.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3777
> +    emitPutHomeObject(generator, constructor.get(), prototype.get());

Can we avoid always putting this home object here?
I think we can avoid it if the constructor does not have super reference (and of course, arrow func using super etc...).
If we can do that, please add FIXME with bug URL here.

[Yusuke Suzuki]

Comment on attachment 298851 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3777
>> +    emitPutHomeObject(generator, constructor.get(), prototype.get());
> 
> Can we avoid always putting this home object here?
> I think we can avoid it if the constructor does not have super reference (and of course, arrow func using super etc...).
> If we can do that, please add FIXME with bug URL here.

Could you ensure that this patch does not pose perf regression in ES6SampleBench before landing it?

[GSkachkov]

Created attachment 299045 [details]
Patch

Small performance improvements

[GSkachkov]

(In reply to comment #4)
> Comment on attachment 298851 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298851&action=review
> 
> r=me with comment.
> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3777
> > +    emitPutHomeObject(generator, constructor.get(), prototype.get());
> 
> Can we avoid always putting this home object here?
> I think we can avoid it if the constructor does not have super reference
> (and of course, arrow func using super etc...).
> If we can do that, please add FIXME with bug URL here.

I did small refactoring and implement checking if super used inside of the constructor

[Ryosuke Niwa]

Comment on attachment 299045 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Current patch fixed allow to use super inside of the constructor for classes 

Nit: "Current patch fixed allow" -> "Allow".

You should also explain how you're fixing the problem.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3696
> +        } else if (metadata->superBinding() == SuperBinding::Needed)
> +            superIsUsedInConstructor = true;

Why don't we just create a new boolean like "needsHomeObject" and set it to true whenever this is true or m_classHeritage is set?

> Source/JavaScriptCore/parser/Parser.h:1605
> +    ALWAYS_INLINE SuperBinding getSuperBindingForConstructor(ConstructorKind constructorKind, SuperBinding superBinding, bool needsSuperBinding, bool currentScopeUsesEval, InnerArrowFunctionCodeFeatures innerArrowFunctionFeatures)

Nit: we don't prefix a getter function with "get" unless there is an out argument.
See https://webkit.org/code-style-guidelines/#names-setter-getter

> Source/JavaScriptCore/parser/Parser.h:1612
> +            methodSuperBinding = (needsSuperBinding || isSuperUsedInInnerArrowFunction || isEvalUsedInInnerArrowFunctions || currentScopeUsesEval) ? SuperBinding::Needed : SuperBinding::NotNeeded;

Why do we need super binding when eval is used inside an arrow function?
I don't see any test case of that either.

[GSkachkov]

Created attachment 299148 [details]
Patch

Fix review comments

[GSkachkov]

(In reply to comment #8)
> Comment on attachment 299045 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299045&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Current patch fixed allow to use super inside of the constructor for classes 
> 
> Nit: "Current patch fixed allow" -> "Allow".
> 
> You should also explain how you're fixing the problem.
> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3696
> > +        } else if (metadata->superBinding() == SuperBinding::Needed)
> > +            superIsUsedInConstructor = true;
> 
> Why don't we just create a new boolean like "needsHomeObject" and set it to
> true whenever this is true or m_classHeritage is set?
> 
> > Source/JavaScriptCore/parser/Parser.h:1605
> > +    ALWAYS_INLINE SuperBinding getSuperBindingForConstructor(ConstructorKind constructorKind, SuperBinding superBinding, bool needsSuperBinding, bool currentScopeUsesEval, InnerArrowFunctionCodeFeatures innerArrowFunctionFeatures)
> 
> Nit: we don't prefix a getter function with "get" unless there is an out
> argument.
> See https://webkit.org/code-style-guidelines/#names-setter-getter
> 
> > Source/JavaScriptCore/parser/Parser.h:1612
> > +            methodSuperBinding = (needsSuperBinding || isSuperUsedInInnerArrowFunction || isEvalUsedInInnerArrowFunctions || currentScopeUsesEval) ? SuperBinding::Needed : SuperBinding::NotNeeded;
> 
> Why do we need super binding when eval is used inside an arrow function?
> I don't see any test case of that either.
I've tried to explicitly cover case when eval in arrow function so there can be used super, but currentScopeUsesEval should cover this case.

[GSkachkov]

Created attachment 299152 [details]
Patch

Small improvements

[Ryosuke Niwa]

Comment on attachment 299152 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:1866
> +            SuperBinding functionSuperBinding = superBindingForConstructor(constructorKind, superBinding, currentScope()->needsSuperBinding(), currentScope()->usesEval(), currentScope()->innerArrowFunctionFeatures());

I think it's confusing to call superBindingForConstructor to compute functionSuperBinding given a function is not always a constructor.
Why don't we call this function something like adjustSuperBindingForBaseConstructor?

> Source/JavaScriptCore/parser/Parser.cpp:1885
> +    SuperBinding functionSuperBinding = superBindingForConstructor(constructorKind, superBinding, currentScope()->needsSuperBinding(), currentScope()->usesEval(), currentScope()->innerArrowFunctionFeatures());

Why don't we add a function overload with takes the scope object instead of passing in three arguments here?

> Source/JavaScriptCore/parser/Parser.h:1605
> +    ALWAYS_INLINE SuperBinding superBindingForConstructor(ConstructorKind constructorKind, SuperBinding superBinding, bool needsSuperBinding, bool currentScopeUsesEval, InnerArrowFunctionCodeFeatures innerArrowFunctionFeatures)

It's very confusing to have two variables named superBinding and needsSuperBinding.
I think the latter should be called scopeNeedsSuperBinding

[GSkachkov]

Comment on attachment 299152 [details]
Patch

Clearing flags on attachment: 299152

Committed r210958: <http://trac.webkit.org/changeset/210958>

[GSkachkov]

Yusuke Suzuki and Ryosuke Niwa Thanks for review!
Adam Klein Thanks for reporting the bug, could you please close the issue if patch fix your case? I din't have permission to do this.