Summary: | Super property access in base class constructor doesn't work | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Klein <adamk> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, oliver, rniwa, saam, ticaiolima, ysuzuki | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Local Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Adam Klein
2017-01-03 14:44:59 PST
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. |