Summary: | [ES6] Arrow function syntax. Using 'super' in arrow function that declared out of the class should lead to Syntax error | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | GSkachkov <gskachkov> | ||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, ggaren, keith_miller, mark.lam, msaboff, saam, ysuzuki | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 149338, 149615 | ||||||||||||||||
Bug Blocks: | 140855, 153977 | ||||||||||||||||
Attachments: |
|
Description
GSkachkov
2015-11-04 09:51:09 PST
Created attachment 269925 [details]
Patch
Patch
Comment on attachment 269925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269925&action=review r=me with comments > Source/JavaScriptCore/parser/Parser.h:880 > + ScopeRef currentNotArrowfunctionFunctionScope() I propose calling this: "closestParentNonArrowFunctionNonLexicalScope" We really don't need it to return a function scope (and it doesn't in some cases). It just needs to return the closest non-arrow-function function or the global scope. > Source/JavaScriptCore/parser/Parser.h:883 > + ASSERT(i < m_scopeStack.size()); If you're making this assertion you should also assert that m_scopeStack.size() is non zero. > Source/JavaScriptCore/parser/Parser.h:886 > + ASSERT(i < m_scopeStack.size()); Style: I don't think this assert adds much here Created attachment 270010 [details]
Patch
Fix comments
Should this be reviewed? I'm confused b/c the r- (In reply to comment #4) > Should this be reviewed? > I'm confused b/c the r- Previous patch does not cover case with eval("super()"). I'm finishing the new patch that cover this case. I hope that I'll manage to upload it today. Created attachment 270653 [details]
Patch
New patch that cover using super in eval
Created attachment 270654 [details]
Patch
Fix build
Comment on attachment 270654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270654&action=review > Source/JavaScriptCore/ChangeLog:20 > + Invoking super()/super in two or more evals will be part of the issue https://bugs.webkit.org/show_bug.cgi?id=153864 what kind of error should this be if it's not a SyntaxError? > Source/JavaScriptCore/bytecode/EvalCodeCache.h:70 > + wrappedCodeBlockDerivedContextType = superBinding == SuperBinding::Needed > + ? DerivedContextType::DerivedMethodContext > + : derivedContextType; style: make this one line or add "{}" to the else statement. > Source/JavaScriptCore/bytecode/ExecutableInfo.h:67 > + DerivedContextType wrappedCodeBlocContextType() const { return static_cast<DerivedContextType>(m_wrappedCodeBlocContextType); } typo: wrappedCodeBlocContextType => wrappedCodeBlockContextType > Source/JavaScriptCore/parser/Parser.cpp:2070 > + if (!m_lexer->isReparsingFunction()) { Is this needed or just an optimization? > Source/JavaScriptCore/parser/Parser.h:881 > + // Find the closest not 'arrow function' function scope. comment not needed. > Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:-131 > - getParentValueWithError() { > - var f = function () { > - return () => super.getValue(); > - }; > - return f(); > - } you could keep this if you turned this into an "return eval(blah)" Created attachment 270826 [details]
Patch
Fix comments
Comment on attachment 270654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270654&action=review >> Source/JavaScriptCore/ChangeLog:20 >> + Invoking super()/super in two or more evals will be part of the issue https://bugs.webkit.org/show_bug.cgi?id=153864 > > what kind of error should this be if it's not a SyntaxError? I've made small change in this sentence in the last patch. It should be SyntaxError, but my patch only partly resolve this issue. I had to fix case with 'eval("super")' because some of the arrow function tests invoke eval('super'). During the additional testing I found out that my current patch does not cover case when we call eval('eval('super()')'), but I decided that it should not be part of this patch and created the issue >> Source/JavaScriptCore/bytecode/EvalCodeCache.h:70 >> + : derivedContextType; > > style: make this one line or add "{}" to the else statement. Done >> Source/JavaScriptCore/bytecode/ExecutableInfo.h:67 >> + DerivedContextType wrappedCodeBlocContextType() const { return static_cast<DerivedContextType>(m_wrappedCodeBlocContextType); } > > typo: > wrappedCodeBlocContextType => wrappedCodeBlockContextType Done >> Source/JavaScriptCore/parser/Parser.cpp:2070 >> + if (!m_lexer->isReparsingFunction()) { > > Is this needed or just an optimization? I added comment in the last patch, I hope that now it is more clear: // It unncecessary to check of using super during reparsing one more time. Also it can lead to syntax error // in case of arrow function becuase during reparsing we don't know that parse arrow function // inside of the constructor or method >> Source/JavaScriptCore/parser/Parser.h:881 >> + // Find the closest not 'arrow function' function scope. > > comment not needed. Removed >> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:-131 >> - } > > you could keep this if you turned this into an "return eval(blah)" I've moved this test case to the LayoutTests/js/script-tests/arrowfunction-syntax-errors.js where I'm trying to collect all case that are related to the syntax errors. See last case in 66 line in this file. Created attachment 270851 [details]
Patch
Remove all changes that are related to the using eval in arrow function. This will be implemented in separate patch
Comment on attachment 270851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270851&action=review > Source/JavaScriptCore/parser/Parser.h:886 > + // When reaching the top level scope (it can be non function scope), we return it. In a later patch, I'd remove this comment, or just say somethin like: Note: it's okay to return the top most scope because it will be the global scope Comment on attachment 270851 [details] Patch Clearing flags on attachment: 270851 Committed r196261: <http://trac.webkit.org/changeset/196261> All reviewed patches have been landed. Closing bug. *** Bug 153977 has been marked as a duplicate of this bug. *** |