-
-
Notifications
You must be signed in to change notification settings - Fork 662
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 #2727 Type checker does not detect a signature mismatch #2728
Conversation
cea03b7
to
eb7b4c2
Compare
@MaxGraey All checks passed, can is PR be approved? |
This comment was marked as resolved.
This comment was marked as resolved.
This test case expect a compiler failed error. It can't emit wat file |
d5fc165
to
cbe74d6
Compare
cbe74d6
to
0055da6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes I'm suggesting are about style and don't change functionality.
In most cases, a function type is assignable to another if the target `this` parameter is a subtype of the original `this` parameter. However, this check wasn't performed correctly in `Signature#isAssignableTo`. Also, the reverse is true when checking whether override signatures are compatible, since the `this` parameter refers to the subclass that's overriding the parent class's method. At least, that's how I think it works... Fixes AssemblyScript#2727. Closes AssemblyScript#2728.
In most cases, a function type is assignable to another if the target `this` parameter is a subtype of the original `this` parameter. However, this check wasn't performed correctly in `Signature#isAssignableTo`. Also, the reverse is true when checking whether override signatures are compatible, since the `this` parameter refers to the subclass that's overriding the parent class's method. At least, that's how I think it works... Fixes AssemblyScript#2727. Closes AssemblyScript#2728.
In most cases, a function type is assignable to another if the target `this` parameter is a subtype of the original `this` parameter. However, this check wasn't performed correctly in `Signature#isAssignableTo`. Also, the reverse is true when checking whether override signatures are compatible, since the `this` parameter refers to the subclass that's overriding the parent class's method. At least, that's how I think it works... Fixes AssemblyScript#2727. Closes AssemblyScript#2728.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor style issues. I hope I'm not being too pedantic.
b386305
to
b74c7de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. But maybe add test for B implement A
It might be redundant, since the same code path is used. |
fixes #2727