Skip to content

Always try to call the most-derived ObjC protocol method #9716

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

Conversation

rjmccall
Copy link
Contributor

Currently, when the Clang importer sees that an imported ObjC class conforms to an ObjC protocol, it creates concrete methods on the class for every method in the protocol hierarchy. These are called "mirror imports". It's a hack that we want to eliminate long-term, but in 4.0, at least, we have to live with it.

Now, a method can be declared multiple times in an ObjC protocol hierarchy, and the signatures don't actually have to line up. ObjC has complex but ultimately somewhat arbitrary rules for picking which method to use for type-checking a message send. Swift, apparently, picks the first one it sees, according to some unspecified walk of the protocol hierarchy, which is even more arbitrary than ObjC. That walk appears to correctly prioritize more-derived methods when the protocol hierarchy is a tree, and probably when the more-derived method dominates the less-derived one, but when there are multiple paths to a protocol it doesn't seem to always honor overriders. Worse, the walk order appears to have changed over time; we have a test case which fails in master but succeeds in previous releases because it now finds the less-derived protocol method instead of the more-derived one.

This patch implements more-principled behavior: it drops methods from the candidate set if they are overridden by another method in the set, then picks the first method remaining.

This is the swift-4.0-branch version of #9681.

This patch can change what method we use to type-check ObjC method calls when the method is not redeclared on the class, which is quite common. Using a different method can have a drastic impact on the type-checking of that call. However, it only actually matters if there are multiple declarations of the method in the protocol hierarchy, and those declarations have different signatures, and we were previously using a less-derived one (which should only arise in complex protocol hierarchies).

This patch fixes rdar://31471034.

The risk of this change is moderate: it affects what types are used in type-checking, and it's certainly possible that there is longstanding code which is somehow relying on our previous behavior. However, this should be largely mitigated by the considerations above. Furthermore, overrides are usually type-theoretically sound, so using a more-derived method instead of a less-derived method should always be more permissive about arguments and more informative about results. Moreover, we know that there are projects which are relying on the previous behavior of the compiler, which at least in some cases used the more-derived method; those projects will see not taking this patch as a source-compatibility regression. But I can't pretend that there's no risk from the patch working as intended.

This has been tested with ordinary regression tests and by verifying that it fixes a source-compatibility regression with a particular project.

the class's protocol hierarchy that overrides it.

rdar://31471034
@rjmccall
Copy link
Contributor Author

@swift-ci Please test.

@rjmccall rjmccall merged commit 640908f into swiftlang:swift-4.0-branch May 18, 2017
@rjmccall rjmccall deleted the mirror-import-overrides-4.0 branch May 18, 2017 00:55
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.

1 participant