Skip to content

fix: override keyword shouldn't throw error in multilevel inherit #2449

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

Merged
merged 5 commits into from
Sep 25, 2022

Conversation

HerrCai0907
Copy link
Member

@HerrCai0907 HerrCai0907 commented Aug 19, 2022

fix override keyword in following case

class A {
  foo(): void {}
}
class B extends A {
  bar(): void {}
}
class C extends B {
  override foo(): void {}. // should not throw error
}
  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

Sorry, something went wrong.

@dcodeIO
Copy link
Member

dcodeIO commented Aug 20, 2022

Wondering why this didn't work before. Can you provide some context of what was missing exactly? Would assume that B inhereits foo from A, and then C upon extending sees foo, overriding it.

@HerrCai0907
Copy link
Member Author

Wondering why this didn't work before. Can you provide some context of what was missing exactly? Would assume that B inhereits foo from A, and then C upon extending sees foo, overriding it.

But B's baseInstanceMembers don't have A's member, so need to search all the node in inherit chain and throw the error at the end.

@dcodeIO
Copy link
Member

dcodeIO commented Aug 20, 2022

Oh, I see. Makes me think this is either an oversight, or perhaps we should look up from instanceMembers if these have all of them? I wonder what I was thinking when I wrote that code :)

@dcodeIO
Copy link
Member

dcodeIO commented Aug 26, 2022

Looking more closely, I think markVirtuals isn't the right place to perform the check / emit the diagnostic. That's a rather specific method, and it was likely an oversight that the prior check was added there. Looks like the check should rather be performed when resolving a class.

@HerrCai0907 HerrCai0907 requested a review from dcodeIO September 4, 2022 14:23
@dcodeIO dcodeIO merged commit cfb4616 into AssemblyScript:main Sep 25, 2022
@HerrCai0907 HerrCai0907 deleted the fix/override branch September 26, 2022 01:34
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.

None yet

2 participants