Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix super access related problems #363

Merged
merged 2 commits into from
Dec 30, 2022
Merged

Conversation

lahma
Copy link
Collaborator

@lahma lahma commented Dec 30, 2022

  • Introduced AllowSuperCall and AllowSuperProperty
  • Removes 114 lines from allow list, basically all having -super- as part of their name

@lahma lahma marked this pull request as ready for review December 30, 2022 13:38
@lahma lahma requested a review from adams85 December 30, 2022 13:38
@adams85
Copy link
Collaborator

adams85 commented Dec 30, 2022

Sorry for meddling with your work but it was faster to me to make the proposed changes than explain them... 😄

Besides some cleanup, my commit fixes some nasty edge cases like

class X extends Y {
    constructor() {
         var Z = class { get x() { super(); } } // should be rejected
    }
}
class X extends Y {
    constructor() {
        class Z { static { super() } } // should be rejected
    }
}
class X extends Y {
    constructor() {
        class Z extends ZZ { constructor(x = super()) { } } // should be accepted!!!
    }
}
class X extends Y {
    constructor() {
        class Z { x = super() } // should be rejected
    }
}

etc.

Also renamed AllowSuperProperty to AllowSuperAccess (as suggested by #359) because that sounds a bit more expressive to me. Hope you don't mind.

Please take a look at my changes and if you're ok with them, we can consider this done.

@lahma
Copy link
Collaborator Author

lahma commented Dec 30, 2022

Thanks for the review and improvements! Generally on esprima side I feel even more lost than on Jint side.

I'll gladly swallow my pride and take corrections/insights improving my attempt from peers more educated in the subject, so all good!

I'll try to push some fixes later too and appreciate this kind of help!

@lahma lahma merged commit a711ff2 into sebastienros:main Dec 30, 2022
@lahma lahma deleted the allow-access-call branch December 30, 2022 18:45
@adams85
Copy link
Collaborator

adams85 commented Dec 30, 2022

I bet you're much more educated in this subject than me... 😄 You and others did the actual heavy-lifting, apart from the JS generator stuff, all I'm doing is just polishing off some rough edges here and there.

I'll try to push some fixes later too and appreciate this kind of help!

Sure thing! Daily job's gonna restart soon, so probably my response time won't be that fast but, of course, I'll gladly help with further refinements as time permits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants