[JSC] super property with new should be accepted
Created attachment 407522 [details] Patch
Interestingly, there is no test262 tests for that.
Comment on attachment 407522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407522&action=review This is a great find! I've vetted ~130 LOC between the previous and current check location, everything looks correct. > Source/JavaScriptCore/ChangeLog:8 > + While we should reject `new super` / `new super()`, we should accept `new super.property`. Should we explain why by linking the grammar? https://tc39.es/ecma262/#prod-SuperProperty is a child production of https://tc39.es/ecma262/#prod-MemberExpression, unlike https://tc39.es/ecma262/#prod-SuperCall. > Source/JavaScriptCore/parser/Parser.cpp:4977 > + semanticFailIfTrue(baseIsSuper, "Cannot use new with super"); There are 2 error messages that refer invalid super() as "super call" rather than just "super". It would be nice to follow that. > JSTests/stress/super-and-new.js:42 > + shouldBe(typeof (new super["hey2"]()), "object"); A few more ideas on test coverage: ``` new async.super(); new new.target.super(); new super.super``; ?. ```
(In reply to Yusuke Suzuki from comment #2) > Interestingly, there is no test262 tests for that. No open issue either. We should definitely add one since V8 8.4 experiences the same bug. SpiderMonkey is correct.
Comment on attachment 407522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407522&action=review Thanks! >> Source/JavaScriptCore/ChangeLog:8 >> + While we should reject `new super` / `new super()`, we should accept `new super.property`. > > Should we explain why by linking the grammar? > https://tc39.es/ecma262/#prod-SuperProperty is a child production of https://tc39.es/ecma262/#prod-MemberExpression, unlike https://tc39.es/ecma262/#prod-SuperCall. Sounds good, added. >> Source/JavaScriptCore/parser/Parser.cpp:4977 >> + semanticFailIfTrue(baseIsSuper, "Cannot use new with super"); > > There are 2 error messages that refer invalid super() as "super call" rather than just "super". > It would be nice to follow that. Sounds good. Changed. >> JSTests/stress/super-and-new.js:42 >> + shouldBe(typeof (new super["hey2"]()), "object"); > > A few more ideas on test coverage: > > ``` > new async.super(); > new new.target.super(); > new super.super``; > ?. > ``` Added.
(In reply to Alexey Shvayka from comment #4) > (In reply to Yusuke Suzuki from comment #2) > > Interestingly, there is no test262 tests for that. > > No open issue either. We should definitely add one since V8 8.4 experiences > the same bug. SpiderMonkey is correct. Interesting! We should contribute it to test262.
Created attachment 407528 [details] Patch
Comment on attachment 407528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407528&action=review r=me > LayoutTests/ChangeLog:9 > + * js/class-syntax-super-expected.txt: > + * js/script-tests/class-syntax-super.js: Hmm, should we move this file out of LayoutTests while we're at it?
Comment on attachment 407528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407528&action=review >> LayoutTests/ChangeLog:9 >> + * js/script-tests/class-syntax-super.js: > > Hmm, should we move this file out of LayoutTests while we're at it? Yeah, I think we should do that in a separate patch.
Mac-debug-wk1 failure is WebAudio ones we already know.
Committed r266322: <https://trac.webkit.org/changeset/266322>
<rdar://problem/67983175>