Bug 215966 – [JSC] super property with new should be accepted

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] super property with new should be accepted

[Yusuke Suzuki]

Created attachment 407522 [details]
Patch

[Yusuke Suzuki]

Interestingly, there is no test262 tests for that.

[Alexey Shvayka]

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``;
?.
```

[Alexey Shvayka]

(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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 407528 [details]
Patch

[Ross Kirsling]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Mac-debug-wk1 failure is WebAudio ones we already know.

[Yusuke Suzuki]

Committed r266322: <https://trac.webkit.org/changeset/266322>

[Radar WebKit Bug Importer]

<rdar://problem/67983175>