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 to -rac_signalForSelector: properly implement -respondsToSelector: for optional method from a protocol #926

Merged

Conversation

ikesyo
Copy link
Member

@ikesyo ikesyo commented Nov 8, 2013

If an instance is callded -rac_signalForSelector:fromProtocol: for a certain protocol's optional method (its class doesn't implement that), the instance should respond to the selector. Then, another instance of the same class that is called -rac_signalForSelector: with other selectors, but not called with the optional method selector, should not respond to the selector.

Originally, class_addMethod() makes all instances of the class respondable to the optional method selector. It leads to a crash from -doesNotRecognizeSelector: at RACSwizzleForwardInvocation in following pattern:

if ([anotherInstance respondsToSelector:@selector(someOptionalMethod:)]) {
    // The instance responds to the selector, but it doesn't have a signal for the selector.
    // So, `RACForwardInvocation()` returns NO.
    [anotherInstance someOptionalMethod:@1];
}

@ghost ghost assigned jspahrsummers Nov 8, 2013
id newRespondsToSelector = ^ BOOL (id self, SEL selector) {
SEL aliasSelector = RACAliasForSelector(selector);
BOOL hasSignalForSelector = objc_getAssociatedObject(self, aliasSelector) != nil;
Copy link
Member

Choose a reason for hiding this comment

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

Associated objects are somewhat expensive. We might want to save this check until we've verified that the method exists (below).

@jspahrsummers
Copy link
Member

This fix makes sense to me. Thanks! ✨

@kastiglione Do you want to take a look too?

Save accessing a associated object until we've verified that the method exists.
@ikesyo
Copy link
Member Author

ikesyo commented Nov 11, 2013

🚀

return originalRespondsToSelector(self, respondsToSelectorSEL, selector);
BOOL hasMessageForwardingImplementation = (method != NULL && method_getImplementation(method) == _objc_msgForward);

if (hasMessageForwardingImplementation || originalRespondsToSelector(self, respondsToSelectorSEL, selector)) {
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason for moving the call to originalRespondsToSelector into this condition? Is there a case where hasMessageForwardingImplementation is false, yet an associated subject will exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kastiglione Thanks to point out it! It's just my misunderstanding / too much thinking. I made it simple.

@kastiglione
Copy link
Member

@jspahrsummers Just the one question to @ikesyo.

Thanks @ikesyo, good fix.

Save accessing a associated object until we've verified that the method exists.
@ikesyo
Copy link
Member Author

ikesyo commented Nov 12, 2013

@jspahrsummers @kastiglione I made it more cleaner / simpler. Re-ready for review.

@kastiglione
Copy link
Member

:shipit:

@jspahrsummers
Copy link
Member

💎💎💎💎💎💎💎💎💎💎🚢💎💎💎💎💎💎💎💎💎
💎💎💎💎💎💎💎💎💎🚢🚢🚢💎💎💎💎💎💎💎💎
💎💎💎💎💎💎💎💎🚢💎🚢💎🚢💎💎💎💎💎💎💎
💎💎💎💎💎💎💎🚢💎💎🚢💎💎🚢💎💎💎💎💎💎
💎💎💎💎💎💎🚢💎💎💎🚢💎💎💎🚢💎💎💎💎💎
💎💎💎💎💎🚢💎💎💎💎🚢💎💎💎💎🚢💎💎💎💎
💎💎💎💎🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢💎💎💎
💎🚢🚢💎💎💎💎💎💎💎🚢💎💎💎💎💎💎🚢🚢💎
💎💎🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢💎💎
💎💎💎💎🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢🚢💎💎💎

jspahrsummers added a commit that referenced this pull request Nov 12, 2013
Fix to -rac_signalForSelector: properly implement -respondsToSelector: for optional method from a protocol
@jspahrsummers jspahrsummers merged commit ecd8947 into ReactiveCocoa:master Nov 12, 2013
@ikesyo ikesyo deleted the fix-respondstoselector-replacement branch November 12, 2013 06:38
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.

None yet

3 participants