Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[SR-12723][Sema] Validate function type param representations thick-to-thin conversions #31814
[SR-12723][Sema] Validate function type param representations thick-to-thin conversions #31814
Changes from all commits
8f2f14b
bacbc57
baccbde
d6bf34e
adba283
da53129
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This and some other conversions here are
thin-to-thick
which should be allowed. I think this is actually meant to work in reverse -thin-to-thick
is allowed butthick-to-thin
isn't.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.
At least this is what Joe mentioned in his comment.
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.
Hey @xedin :)
Hum, that's actually one of the questions I had, because on the param function type there is the fact that subtype relation is contravariant. So my question is if this affects the conversion in some way?
In this case, for example, converting
(@convention(c)()->Void)->Void
to(()->Void)->Void
onA -> B
toA'-> B'
respectivelly, this means the subtype relation isA'
is subtype ofA
so the()->Void
is a subtype of@convention(c)()->Void
, does this means the conversion isthick-to-thin
? ... at the time this makes sense to me, but that is actually a question I have, do you know if this makes sense or am I missing something? Let me know if I this makes sense :)Also, the logic on
matchFunctionTypes
is only allowing exactlythin-to-thick
andthick-to-thick
any other conversions are being deferred to check if representations are the same.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.
Yes, you are correct, sorry for the confusion. I was just merely trying to point out that we might want to change diagnostic or add a custom note which explains precisely why these types couldn't be converted (related to contravariance) because it appears that it's passing
thick-to-thin
.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.
I got tripped by this myself because I didn't notice that diagnostic mentioned whole function type...
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.
Right, thanks for the clarification :)
I've added in the last commit, thin to thin which seems to be allowed, so now we strictly only validating
thick-to-thin
.About the note, I think it makes sense, but I'm not sure how to phrase it because I'm not sure if we should mention variance, or only mention that the representations are not convertible in a parameter subtype context ...
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.
Yeah, it's a good question how note should be worded exactly...
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.
After thinking about this some more - I think we should just open a follow up SR for the note and merge what we have now.
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.
I think it makes sense :)