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

[arphaman]

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

[rjmccall]

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

[arphaman]

Of course, right. I will change the approach.

[arphaman]

@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.

[rjmccall]

Tagging Doug.

[rjmccall]

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.

[arphaman]

@doug.gregor Ping

[arphaman]

@doug.gregor Ping

[doug.gregor]

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]

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.

[arphaman]

In D36790#843450, @arphaman wrote:

@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.

[arphaman]

ping

[arphaman]

ping

[rjmccall]

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]

• 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.

[rjmccall]

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]

Fixed comment, will commit.

lib/Sema/SemaExprObjC.cpp

if (isSuperMessage) {
     if (ObjCMethodDecl *CurMethod = S.getCurMethodDecl())
       if (ObjCInterfaceDecl *Class = CurMethod->getClassInterface()) {
         return transferNullability(
                  Context.getObjCObjectPointerType(
                    Context.getObjCInterfaceType(Class)));
       }
   }
 
-  //   - if the receiver is the name of a class U, T is a pointer to U
+  //   - if the receiver is the name of a class U, T is a pointer to U. U should
+  //     be desugared to avoid 'decltype(self)' propagation.
   if (ReceiverType->getAsObjCInterfaceType())
-    return transferNullability(Context.getObjCObjectPointerType(ReceiverType));
+    return transferNullability(Context.getObjCObjectPointerType(
+        ReceiverType.getDesugaredType(Context)));
   //   - if the receiver is of type Class or qualified Class type,
   //     T is the declared return type of the method.
   if (ReceiverType->isObjCClassType() ||
       ReceiverType->isObjCQualifiedClassType())
     return stripObjCInstanceType(Context, 
                                  Method->getSendResultType(ReceiverType));
 
   //   - if the receiver is id, qualified id, Class, or qualified Class, T
QualType resultType = getBaseMessageSendResultType(*this, ReceiverType,
                                                      Method,
                                                      isClassMessage,
                                                      isSuperMessage);
 
   // If this is a class message, ignore the nullability of the receiver.
   if (isClassMessage)
     return resultType;
 
   // Map the nullability of the result into a table index.
   unsigned receiverNullabilityIdx = 0;
   if (auto nullability = ReceiverType->getNullability(Context))
     receiverNullabilityIdx = 1 + static_cast<unsigned>(*nullability);
 
   unsigned resultNullabilityIdx = 0;
   if (auto nullability = resultType->getNullability(Context))
     resultNullabilityIdx = 1 + static_cast<unsigned>(*nullability);
 
   // The table of nullability mappings, indexed by the receiver's nullability
   // and then the result type's nullability.
   static const uint8_t None = 0;
if (receiverIsIdLike || ReceiverType->isBlockPointerType() ||
         if (!AreMultipleMethodsInGlobalPool(Sel, Method,
                                             SourceRange(LBracLoc, RBracLoc),
                                             receiverIsIdLike, Methods))
            DiagnoseUseOfDecl(Method, SelLoc);
       }
     } else if (ReceiverType->isObjCClassOrClassKindOfType() ||
                ReceiverType->isObjCQualifiedClassType()) {
       // Handle messages to Class.
+      // Treat messages to 'self' in class methods as class messages when ARC
+      // is enabled (because self can't be reassigned when ARC is on).
+      if (Receiver->isObjCSelfExpr() && getLangOpts().ObjCAutoRefCount) {
+        assert(ReceiverType->isObjCClassType() && "expected a Class self");
+        const ObjCMethodDecl *MD = cast<ObjCMethodDecl>(
+            cast<ImplicitParamDecl>(
+                cast<DeclRefExpr>(Receiver->IgnoreParenImpCasts())->getDecl())
+                ->getDeclContext());
+        assert(MD->isClassMethod() && "expected a class method");
+        QualType ReceiverType =
+            Context.getObjCInterfaceType(MD->getClassInterface());
+        // Use a pseudo-decltype type to keep the expression in the AST.
+        assert(!Receiver->isInstantiationDependent() &&
+               "unexpected dependent expr");
+        ReceiverType = Context.getDecltypeType(Receiver, ReceiverType);
+        return BuildClassMessage(
+            Context.getTrivialTypeSourceInfo(ReceiverType, LBracLoc),
+            ReceiverType,
+            /*SuperLoc=*/SourceLocation(), Sel,
+            /*Method=*/nullptr, LBracLoc, SelectorLocs, RBracLoc, ArgsIn);
+      }
       // We allow sending a message to a qualified Class ("Class<foo>"), which
       // is ok as long as one of the protocols implements the selector (if not,
       // warn).
-      if (!ReceiverType->isObjCClassOrClassKindOfType()) {
+      else if (!ReceiverType->isObjCClassOrClassKindOfType()) {
         const ObjCObjectPointerType *QClassTy
           = ReceiverType->getAsObjCQualifiedClassType();
         // Search protocols for class methods.
         Method = LookupMethodInQualifiedType(Sel, QClassTy, false);
         if (!Method) {
           Method = LookupMethodInQualifiedType(Sel, QClassTy, true);
           // warn if instance method found for a Class message.
           if (Method && !isMethodDeclaredInRootProtocol(*this, Method)) {

test/SemaObjC/multiple-method-names-class-self.m

+// RUN: %clang_cc1 -Wobjc-multiple-method-names -x objective-c -verify %s
+// RUN: %clang_cc1 -Wobjc-multiple-method-names -x objective-c -verify -fobjc-arc -DARC %s
+#ifdef ARC
+// expected-no-diagnostics
+#endif
+
+@interface NSObj
+
++ (instancetype) alloc;
+
+@end
+
+@interface SelfAllocReturn: NSObj
+
+- (instancetype)initWithFoo:(int)x;
+#ifndef ARC
+// expected-note@-2 {{using}}
+#endif
+
+@end
+
+@interface SelfAllocReturn2: NSObj
+
+- (instancetype)initWithFoo:(SelfAllocReturn *)x;
+#ifndef ARC
+// expected-note@-2 {{also found}}
+#endif
+
+@end
+
+@implementation SelfAllocReturn
+
+- (instancetype)initWithFoo:(int)x {
+    return self;
+}
+
++ (instancetype) thingWithFoo:(int)x {
+    return [[self alloc] initWithFoo: x];
+#ifndef ARC
+// expected-warning@-2 {{multiple methods named 'initWithFoo:' found}}
+#endif
+}
+
+@end