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 #2727 Type checker does not detect a signature mismatch #2728

Merged
merged 3 commits into from
Oct 1, 2023

Conversation

Changqing-JING
Copy link
Contributor

fixes #2727

@Changqing-JING Changqing-JING changed the title fix #2727: fix #2727 Type checker does not detect a signature mismatch Jul 29, 2023
src/types.ts Outdated Show resolved Hide resolved
@Changqing-JING
Copy link
Contributor Author

@MaxGraey All checks passed, can is PR be approved?

@MaxGraey

This comment was marked as resolved.

@Changqing-JING
Copy link
Contributor Author

This test case expect a compiler failed error. It can't emit wat file

@Changqing-JING Changqing-JING force-pushed the bugfix/2727 branch 4 times, most recently from d5fc165 to cbe74d6 Compare July 31, 2023 06:10
Copy link
Member

@CountBleck CountBleck left a 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.

src/compiler.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
tests/compiler/class-member-function-as-parameter.ts Outdated Show resolved Hide resolved
tests/compiler/class-member-function-as-parameter.json Outdated Show resolved Hide resolved
CountBleck added a commit to CountBleck/assemblyscript that referenced this pull request Jul 31, 2023
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.
CountBleck added a commit to CountBleck/assemblyscript that referenced this pull request Jul 31, 2023
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.
CountBleck added a commit to CountBleck/assemblyscript that referenced this pull request Jul 31, 2023
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.
Copy link
Member

@CountBleck CountBleck left a 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.

src/compiler.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
tests/compiler/class-member-function-as-parameter.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@CountBleck CountBleck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine.

Copy link
Member

@HerrCai0907 HerrCai0907 left a 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

@CountBleck
Copy link
Member

LGTM. But maybe add test for B implement A

It might be redundant, since the same code path is used.

@HerrCai0907 HerrCai0907 merged commit 927b7cd into AssemblyScript:main Oct 1, 2023
12 checks passed
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.

Type checker does not detect a signature mismatch
5 participants