Skip to content

[Typechecker] Dont infer @objc for typealiases inside @objc protocol #23346

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 19, 2019
Merged

[Typechecker] Dont infer @objc for typealiases inside @objc protocol #23346

merged 2 commits into from
Mar 19, 2019

Conversation

theblixguy
Copy link
Collaborator

Dont infer @objc for typealiases inside @objc protocol. This stops the assertion from being hit inside Serialization.cpp.

Resolves SR-10115.

@theblixguy
Copy link
Collaborator Author

theblixguy commented Mar 15, 2019 •
edited
Loading

cc @jrose-apple

I am not fully sure if this is the correct fix i.e. do we not infer @objc for all type aliases or just some special ones (like a typealias pointing to a closure).

@jrose-apple
Copy link
Contributor

We certainly don't want it for any typealiases, but I think there might be other cases where we want to do this as well. isProtocolRequirement() is probably the right check.

jrose-apple
jrose-apple reviewed Mar 16, 2019
View reviewed changes
@compnerd
Copy link
Member

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - e3c98dca554c8e117bdfd3e268bff215a1f5e20b

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - e3c98dca554c8e117bdfd3e268bff215a1f5e20b

jrose-apple
jrose-apple reviewed Mar 18, 2019
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

Thanks, this is better.

@jrose-apple
Copy link
Contributor

@swift-ci Please test

@jrose-apple
Copy link
Contributor

@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - c79e6ec

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - c79e6ec

@theblixguy
Copy link
Collaborator Author

Could you re-trigger the CI? Thanks!

@jrose-apple
Copy link
Contributor

@swift-ci Please test

@jrose-apple
Copy link
Contributor

@swift-ci Please test source compatibility

@jrose-apple jrose-apple self-assigned this Mar 18, 2019
@theblixguy
Copy link
Collaborator Author

All tests have passed 🎉

@jrose-apple jrose-apple merged commit 09847e6 into swiftlang:master Mar 19, 2019
@jrose-apple
Copy link
Contributor

Thanks, Suyash! Mind cherry-picking this to the 5.1 branch? I think it's small and safe enough that we might as well fix it.

@theblixguy theblixguy mentioned this pull request Mar 19, 2019
Merged
@theblixguy
Copy link
Collaborator Author

@jrose-apple Sure 😄 #23412

@theblixguy theblixguy mentioned this pull request Mar 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slavapestov slavapestov slavapestov left review comments

@jrose-apple jrose-apple jrose-apple approved these changes

Assignees

@jrose-apple jrose-apple

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@theblixguy @jrose-apple @compnerd @swift-ci @slavapestov