Bug 153864

Following case should not lead to Syntax Error: const defaultValue = 'default-value'; class A { setDefaultValue() { this.value = defaultValue; } } class B extends A { constructor() { eval("super()"); } setValue () { eval("super.setDefaultValue()"); } } let b = new B(); b.setValue(); b.id === defaultValue;

Created attachment 273142 [details] Patch Patch, more test will be later

Comment on attachment 273142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273142&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:307 > + return !(m_codeBlock->isArrowFunction() || m_codeType == EvalCode) || m_isNewTargetLoadedInArrowFunction > ? m_newTargetRegister : emitLoadNewTargetFromArrowFunctionLexicalEnvironment(); This function is getting hard to read. I vote for an if statement here. Also, I found a bug in the logic of caching here: https://bugs.webkit.org/show_bug.cgi?id=155153 > Source/JavaScriptCore/parser/Parser.cpp:2068 > + ConstructorKind functionConstructorKind = functionBodyType == StandardFunctionBodyBlock && !m_isEvalContext Don't we already know if we're parsing an eval? Why do we need m_isEvalContext? Maybe I'm mistaken. > Source/JavaScriptCore/parser/Parser.cpp:3820 > + semanticFailIfFalse(currentScope()->isFunction() || (m_isEvalContext && closestParentOrdinaryFunctionNonLexicalScope()->expectedSuperBinding() == SuperBinding::Needed), "super is only valid inside functions"); This error message should be updated. > Source/JavaScriptCore/runtime/CodeCache.cpp:104 > + bool isEvalConext = true; This is definitely not true. This should probably be a parameter to this function. > Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:232 > +class K extends A { let's have some eval tests inside a constructor. tests for TDZ, etc.

Created attachment 274093 [details] Patch

Comment on attachment 273142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273142&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:307 >> ? m_newTargetRegister : emitLoadNewTargetFromArrowFunctionLexicalEnvironment(); > > This function is getting hard to read. I vote for an if statement here. > Also, I found a bug in the logic of caching here: > https://bugs.webkit.org/show_bug.cgi?id=155153 OK. It will be overwritten in fix of the 155153 >> Source/JavaScriptCore/parser/Parser.cpp:2068 >> + ConstructorKind functionConstructorKind = functionBodyType == StandardFunctionBodyBlock && !m_isEvalContext > > Don't we already know if we're parsing an eval? > Why do we need m_isEvalContext? > Maybe I'm mistaken. In new patch, I've used isEvalNode template function that is deepest place where I can check EvalNode. >> Source/JavaScriptCore/parser/Parser.cpp:3820 >> + semanticFailIfFalse(currentScope()->isFunction() || (m_isEvalContext && closestParentOrdinaryFunctionNonLexicalScope()->expectedSuperBinding() == SuperBinding::Needed), "super is only valid inside functions"); > > This error message should be updated. Yeah, I've created issue https://bugs.webkit.org/show_bug.cgi?id=155491. There is several tests that rely on this 'text', so I decided to separate this change is another patch. >> Source/JavaScriptCore/runtime/CodeCache.cpp:104 >> + bool isEvalConext = true; > > This is definitely not true. This should probably be a parameter to this function. Removed >> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:232 >> +class K extends A { > > let's have some eval tests inside a constructor. > tests for TDZ, etc. Done

Comment on attachment 274093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274093&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:643 > + if (needsToUpdateArrowFunctionContext() && !codeBlock->isArrowFunctionContext() && !isDerivedConstructorContext()) { Why the "!isDerivedConstructorContext()" check? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4145 > + return m_scopeNode->doAnyInnerArrowFunctionsUseNewTarget() || m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval(); I believe m_codeBlock->usesEval() should always be true if m_scopeNode->doAnyInnerAroowFunctionsUseEval(). You should double check though. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4150 > + return m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseSuperProperty() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval(); ditto > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4155 > + return m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval(); ditto > Source/JavaScriptCore/parser/Parser.cpp:3848 > + // TODO: Change error message for more suitable. https://bugs.webkit.org/show_bug.cgi?id=155491 Style: FIXME not TODO

Comment on attachment 274093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274093&action=review > Source/JavaScriptCore/ChangeLog:3 > + Invoking super()/super inside of the eval should not lead to SyntaxError Do we have a bug open for new.target inside eval? >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:643 >> + if (needsToUpdateArrowFunctionContext() && !codeBlock->isArrowFunctionContext() && !isDerivedConstructorContext()) { > > Why the "!isDerivedConstructorContext()" check? Ah, I guess it's that way so we don't recreate a scope. I think I understand.

Comment on attachment 274093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274093&action=review >> Source/JavaScriptCore/ChangeLog:3 >> + Invoking super()/super inside of the eval should not lead to SyntaxError > > Do we have a bug open for new.target inside eval? Yes, I did this recently https://bugs.webkit.org/show_bug.cgi?id=155545 >>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:643 >>> + if (needsToUpdateArrowFunctionContext() && !codeBlock->isArrowFunctionContext() && !isDerivedConstructorContext()) { >> >> Why the "!isDerivedConstructorContext()" check? > > Ah, I guess it's that way so we don't recreate a scope. I think I understand. Yes, that is correct. It is for this case: class C {}; class D extends C { constructor() { eval("(()=>super())()");//Error } } new D(); Otherwise we will have two context scope for 'this'. I've spend last three evenings to find out why mention simple test case did not work >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4145 >> + return m_scopeNode->doAnyInnerArrowFunctionsUseNewTarget() || m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval(); > > I believe m_codeBlock->usesEval() should always be true if m_scopeNode->doAnyInnerAroowFunctionsUseEval(). > You should double check though. It is true, but unfortunately this patch cover cases when we do not have arrow function, for instance from previous comments, so I need to check m_codeBlock->usesEval(). Possible we need to change name of the function isNewTargetUsedInInnerArrowFunction -> isNewTargetUsedInInnerArrowFunctionOrEval >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4150 >> + return m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseSuperProperty() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval(); > > ditto The same >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4155 >> + return m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval(); > > ditto The same >> Source/JavaScriptCore/parser/Parser.cpp:3848 >> + // TODO: Change error message for more suitable. https://bugs.webkit.org/show_bug.cgi?id=155491 > > Style: FIXME not TODO Will be updated in next patch.

(In reply to comment #8) > Comment on attachment 274093 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=274093&action=review > > >> Source/JavaScriptCore/ChangeLog:3 > >> + Invoking super()/super inside of the eval should not lead to SyntaxError > > > > Do we have a bug open for new.target inside eval? > > Yes, I did this recently > https://bugs.webkit.org/show_bug.cgi?id=155545 > > >>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:643 > >>> + if (needsToUpdateArrowFunctionContext() && !codeBlock->isArrowFunctionContext() && !isDerivedConstructorContext()) { > >> > >> Why the "!isDerivedConstructorContext()" check? > > > > Ah, I guess it's that way so we don't recreate a scope. I think I understand. > > Yes, that is correct. It is for this case: > class C {}; > class D extends C { > constructor() { > eval("(()=>super())()");//Error > } > } > new D(); > Otherwise we will have two context scope for 'this'. I've spend last three > evenings to find out why mention simple test case did not work > > >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4145 > >> + return m_scopeNode->doAnyInnerArrowFunctionsUseNewTarget() || m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval(); > > > > I believe m_codeBlock->usesEval() should always be true if m_scopeNode->doAnyInnerAroowFunctionsUseEval(). > > You should double check though. > > It is true, but unfortunately this patch cover cases when we do not have > arrow function, for instance from previous comments, so I need to check > m_codeBlock->usesEval(). Possible we need to change name of the function > isNewTargetUsedInInnerArrowFunction -> > isNewTargetUsedInInnerArrowFunctionOrEval What I mean is shouldn't it be sufficient to just check m_codeBlock->usesEval()? It should never be the case that m_scopeNode->innerArrowEval() is true but m_codeBlock->usesEval() is false. Therefore, m_codeBlock->usesEval() should cover everything.

Created attachment 274212 [details] Patch

Comment on attachment 274093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274093&action=review >>>> Source/JavaScriptCore/ChangeLog:3 >>>> + Invoking super()/super inside of the eval should not lead to SyntaxError >>> >>> Do we have a bug open for new.target inside eval? >> >> Yes, I did this recently >> https://bugs.webkit.org/show_bug.cgi?id=155545 > > What I mean is shouldn't it be sufficient to just check m_codeBlock->usesEval()? > It should never be the case that m_scopeNode->innerArrowEval() is true but m_codeBlock->usesEval() is false. > Therefore, m_codeBlock->usesEval() should cover everything. Hmm, before it was not, innerArrow... is set only for arrow functions and usesEval only if in current function/block we are using eval. So as for now: m_codeBlock->usesEval() is true but m_scopeNode-innerArrowEval() is false for this case -> var f = function () { eval('debug("hello world")'); }; f(); m_codeBlock->usesEval() is false but m_scopeNode->innerArrowEval() true for case -> var f = function () { var arr = () => () => eval('debug("hello world")'); }; f();

Committed r198324: <http://trac.webkit.org/changeset/198324>

All reviewed patches have been landed. Closing bug.