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]
This sounds like we're special casing being derived or not.
(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.
Created attachment 298851 [details] Patch Load super for all classes
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.
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?
Created attachment 299045 [details] Patch Small performance improvements
(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
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.
Created attachment 299148 [details] Patch Fix review comments
(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.
Created attachment 299152 [details] Patch Small improvements
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
Comment on attachment 299152 [details] Patch Clearing flags on attachment: 299152 Committed r210958: <http://trac.webkit.org/changeset/210958>
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.