This is an archive of the discontinued LLVM Phabricator instance.

[ObjC] Messages to 'self' in class methods that return 'instancetype' should use the pointer to the class as the result type of the message
ClosedPublic

Authored by arphaman on Aug 16 2017, 6:43 AM.

Details

Summary

Prior to this patch, messages to self in class methods were treated as instance methods to a Class value. When these methods returned instancetype the compiler only saw id through the instancetype, and not the Interface *. This caused problems when that return value was a receiver in a message send, as the compiler couldn't select the right method declaration and had to rely on a selection from the global method pool.

This patch modifies the semantics of such message sends and uses class messages that are dispatched to the interface that corresponds to the class that contains the class method. This ensures that instancetypes are correctly interpreted by the compiler. This is an ARC only change, since non-ARC code can reassign self. The type dectltype(self) is used to store the self receiver in the class message to ensure that self remains in the AST.

Thanks for taking a look.

rdar://20940997

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Aug 16 2017, 6:43 AM
rjmccall requested changes to this revision.Aug 16 2017, 8:22 AM

Class methods can be inherited; this entire approach is bogus.

This revision now requires changes to proceed.Aug 16 2017, 8:22 AM

Of course, right. I will change the approach.

@rjmccall Do you think that the rules for the return types in overridden methods that return instancetype should be strengthened first? For example, if we have the following code:

@interface Unrelated
- (void)method:(int)x;
@end

@interface CallsSelfSuper: NSObject
+ (void) target;
- (void) method:(CallsSelfSuper *)x;
@end

@implementation CallsSelfSuper
+ (void) target {
  [[self alloc] method: 12]; // Can't assume 'method' is CallsSelfSuper's 'method' declaration unless we guarantee that OverrideAlloc's alloc actually returns `instancetype` and not id.
}
@end

@interface OverrideAlloc: CallsSelfSuper
@end

@implementation OverrideAlloc
+ (id) alloc {
  return [NSString alloc];
}
@end

We can't really make any assumptions about what [self alloc] will return. But in my opinion we could assume that [self alloc] will return a CallsSelfSuper * if we could ensure that methods that override methods that return instancetype will compile only if they return instancetype type as well.

I think it's fair to treat instancetype as an inherited requirement — that is, the rules of covariant override always apply, which essentially means that overriders of instance-returning methods must also return instancetype whether they say so explicitly or not. But that's what I'd like Doug to weigh in about; it's possible there are exceptions to that that affect the analysis here.

doug.gregor edited edge metadata.Oct 24 2017, 9:49 AM

Yes, I think it's reasonable to treat instancetype as an inherited requirement. I guess the only exception would be if we had some notion of final classes or methods in Objective-C, in which case they'd be able to return a concrete type.

arphaman updated this revision to Diff 120157.Oct 24 2017, 5:31 PM
arphaman edited edge metadata.
arphaman retitled this revision from [ObjC] Messages to 'self' in class methods should use class method dispatch to avoid multiple method ambiguities to [ObjC] Messages to 'self' in class methods that return 'instancetype' should use the pointer to the class as the result type of the message.

I changed the approach for this patch. Now, instead of using class method dispatch Clang instead adjusts the result type of the message sends to methods that return instancetype with self receivers in class methods.

@rjmccall Do you think that the rules for the return types in overridden methods that return instancetype should be strengthened first? For example, if we have the following code:

@interface Unrelated
- (void)method:(int)x;
@end

@interface CallsSelfSuper: NSObject
+ (void) target;
- (void) method:(CallsSelfSuper *)x;
@end

@implementation CallsSelfSuper
+ (void) target {
  [[self alloc] method: 12]; // Can't assume 'method' is CallsSelfSuper's 'method' declaration unless we guarantee that OverrideAlloc's alloc actually returns `instancetype` and not id.
}
@end

@interface OverrideAlloc: CallsSelfSuper
@end

@implementation OverrideAlloc
+ (id) alloc {
  return [NSString alloc];
}
@end

We can't really make any assumptions about what [self alloc] will return. But in my opinion we could assume that [self alloc] will return a CallsSelfSuper * if we could ensure that methods that override methods that return instancetype will compile only if they return instancetype type as well.

I've looked at this again and it seems that Clang already has warnings in cases like this, so instancetype is already treated as somewhat of an inherited requirement. I now think it should be add the behavior in this patch without imposing additional restrictions.

rjmccall added inline comments.Dec 14 2017, 10:13 PM
lib/Sema/SemaExprObjC.cpp
1358

What's the purpose of these two checks? I believe one of these is required in order to have decided that we're making a class message anyway, and regardless I don't understand why they would affect your logic.

I don't think you should vary the logic here based on whether you're in ARC. Just assume that the user isn't doing silly stuff like reassigning self to random values. No, it's not strictly sound, but it's sound enough.

arphaman updated this revision to Diff 127192.Dec 15 2017, 2:16 PM
arphaman marked an inline comment as done.
  • Remove redundant checks.
  • Remove the ARC-specific check.
lib/Sema/SemaExprObjC.cpp
1358

You're right, "isClassMessage" already checks these. The original intent was to ensure that self is an instancetype. I removed them.

LGTM outside of a comment request; please feel free to commit when you'd made that change.

lib/Sema/SemaExprObjC.cpp
1355

You should update the comment to reflect that this isn't ARC-specific; I would suggest something like:

// In a class method, class messages to 'self' that return instancetype can be
// typed as the current class.  We can safely do this in ARC because self can't
// be reassigned, and we do it unsafely outside of ARC because in practice people
// never reassign self in class methods and there's some virtue in not being
// aggressively pedantic.
arphaman marked an inline comment as done.Dec 20 2018, 2:14 PM

Fixed comment, will commit.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 20 2018, 2:14 PM
This revision was automatically updated to reflect the committed changes.