[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

1367

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

1367

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

1364

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.

include/clang/Sema/Sema.h

   /// checkUnsafeExprAssigns - Check whether +1 expr is being assigned
   /// to weak/__unsafe_unretained expression.
   void checkUnsafeExprAssigns(SourceLocation Loc, Expr *LHS, Expr *RHS);
 
   /// CheckMessageArgumentTypes - Check types in an Obj-C message send.
   /// \param Method - May be null.
   /// \param [out] ReturnType - The return type of the send.
   /// \return true iff there were any incompatible types.
-  bool CheckMessageArgumentTypes(QualType ReceiverType,
+  bool CheckMessageArgumentTypes(const Expr *Receiver, QualType ReceiverType,
                                  MultiExprArg Args, Selector Sel,
                                  ArrayRef<SourceLocation> SelectorLocs,
                                  ObjCMethodDecl *Method, bool isClassMessage,
-                                 bool isSuperMessage,
-                                 SourceLocation lbrac, SourceLocation rbrac,
-                                 SourceRange RecRange,
+                                 bool isSuperMessage, SourceLocation lbrac,
+                                 SourceLocation rbrac, SourceRange RecRange,
                                  QualType &ReturnType, ExprValueKind &VK);
 
   /// Determine the result of a message send expression based on
   /// the type of the receiver, the method expected to receive the message,
   /// and the form of the message send.
-  QualType getMessageSendResultType(QualType ReceiverType,
-                                    ObjCMethodDecl *Method,
-                                    bool isClassMessage, bool isSuperMessage);
+  QualType getMessageSendResultType(const Expr *Receiver, QualType ReceiverType,
+                                    ObjCMethodDecl *Method, bool isClassMessage,
+                                    bool isSuperMessage);
 
   /// If the given expression involves a message send to a method
   /// with a related result type, emit a note describing what happened.
   void EmitRelatedResultTypeNote(const Expr *E);
 
   /// Given that we had incompatible pointer types in a return
   /// statement, check whether we're in a method with a related result
   /// type, and if so, emit a note describing what happened.

lib/Sema/SemaExprObjC.cpp

return stripObjCInstanceType(Context,
                                  Method->getSendResultType(ReceiverType));
 
   //   - if the receiver is id, qualified id, Class, or qualified Class, T
   //     is the receiver type, otherwise
   //   - T is the type of the receiver expression.
   return transferNullability(ReceiverType);
 }
 
-QualType Sema::getMessageSendResultType(QualType ReceiverType,
+QualType Sema::getMessageSendResultType(const Expr *Receiver,
+                                        QualType ReceiverType,
                                         ObjCMethodDecl *Method,
                                         bool isClassMessage,
                                         bool isSuperMessage) {
   // Produce the result type.
   QualType resultType = getBaseMessageSendResultType(*this, ReceiverType,
                                                      Method,
                                                      isClassMessage,
                                                      isSuperMessage);
 
   // If this is a class message, ignore the nullability of the receiver.
-  if (isClassMessage)
+  if (isClassMessage) {
+    // 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.
+    if (Receiver && Receiver->isObjCSelfExpr()) {
+      assert(ReceiverType->isObjCClassType() && "expected a Class self");
+      QualType T = Method->getSendResultType(ReceiverType);
+      AttributedType::stripOuterNullability(T);
+      if (T == Context.getObjCInstanceType()) {
+        const ObjCMethodDecl *MD = cast<ObjCMethodDecl>(
+            cast<ImplicitParamDecl>(
+                cast<DeclRefExpr>(Receiver->IgnoreParenImpCasts())->getDecl())
+                ->getDeclContext());
+        assert(MD->isClassMethod() && "expected a class method");
+        QualType NewResultType = Context.getObjCObjectPointerType(
+            Context.getObjCInterfaceType(MD->getClassInterface()));
+        if (auto Nullability = resultType->getNullability(Context))
+          NewResultType = Context.getAttributedType(
+              AttributedType::getNullabilityAttrKind(*Nullability),
+              NewResultType, NewResultType);
+        return NewResultType;
+      }
+    }
     return resultType;
+  }
 
   // There is nothing left to do if the result type cannot have a nullability
   // specifier.
   if (!resultType->canHaveNullability())
     return resultType;
 
   // Map the nullability of the result into a table index.
   unsigned receiverNullabilityIdx = 0;
if (!Context.hasSameUnqualifiedType(Method->getReturnType(),
                                       Context.getObjCInstanceType()))
     return;
 
   Diag(Method->getLocation(), diag::note_related_result_type_inferred)
     << Method->isInstanceMethod() << Method->getSelector()
     << MsgSend->getType();
 }
 
-bool Sema::CheckMessageArgumentTypes(QualType ReceiverType,
-                                     MultiExprArg Args,
-                                     Selector Sel,
-                                     ArrayRef<SourceLocation> SelectorLocs,
-                                     ObjCMethodDecl *Method,
-                                     bool isClassMessage, bool isSuperMessage,
-                                     SourceLocation lbrac, SourceLocation rbrac,
-                                     SourceRange RecRange,
-                                     QualType &ReturnType, ExprValueKind &VK) {
+bool Sema::CheckMessageArgumentTypes(
+    const Expr *Receiver, QualType ReceiverType, MultiExprArg Args,
+    Selector Sel, ArrayRef<SourceLocation> SelectorLocs, ObjCMethodDecl *Method,
+    bool isClassMessage, bool isSuperMessage, SourceLocation lbrac,
+    SourceLocation rbrac, SourceRange RecRange, QualType &ReturnType,
+    ExprValueKind &VK) {
   SourceLocation SelLoc;
   if (!SelectorLocs.empty() && SelectorLocs.front().isValid())
     SelLoc = SelectorLocs.front();
   else
     SelLoc = lbrac;
 
   if (!Method) {
     // Apply default argument promotion as for (C99 6.5.2.2p6).
if (getLangOpts().DebuggerSupport) {
       ReturnType = Context.UnknownAnyTy;
     } else {
       ReturnType = Context.getObjCIdType();
     }
     VK = VK_RValue;
     return false;
   }
 
-  ReturnType = getMessageSendResultType(ReceiverType, Method, isClassMessage,
-                                        isSuperMessage);
+  ReturnType = getMessageSendResultType(Receiver, ReceiverType, Method,
+                                        isClassMessage, isSuperMessage);
   VK = Expr::getValueKindForType(Method->getReturnType());
 
   unsigned NumNamedArgs = Sel.getNumArgs();
   // Method might have more arguments than selector indicates. This is due
   // to addition of c-style arguments in method.
   if (Method->param_size() > Sel.getNumArgs())
     NumNamedArgs = Method->param_size();
   // FIXME. This need be cleaned up.
ExprResult Sema::BuildClassMessage(TypeSourceInfo *ReceiverTypeInfo,
   }
 
   // Check the argument types and determine the result type.
   QualType ReturnType;
   ExprValueKind VK = VK_RValue;
 
   unsigned NumArgs = ArgsIn.size();
   Expr **Args = ArgsIn.data();
-  if (CheckMessageArgumentTypes(ReceiverType, MultiExprArg(Args, NumArgs),
-                                Sel, SelectorLocs,
-                                Method, true,
-                                SuperLoc.isValid(), LBracLoc, RBracLoc,
-                                SourceRange(),
-                                ReturnType, VK))
+  if (CheckMessageArgumentTypes(/*Receiver=*/nullptr, ReceiverType,
+                                MultiExprArg(Args, NumArgs), Sel, SelectorLocs,
+                                Method, true, SuperLoc.isValid(), LBracLoc,
+                                RBracLoc, SourceRange(), ReturnType, VK))
     return ExprError();
 
   if (Method && !Method->getReturnType()->isVoidType() &&
       RequireCompleteType(LBracLoc, Method->getReturnType(),
                           diag::err_illegal_message_expr_incomplete_type))
     return ExprError();
 
   // Warn about explicit call of +initialize on its own class. But not on 'super'.
ExprResult Sema::BuildInstanceMessage(Expr *Receiver,
 
   // Check the message arguments.
   unsigned NumArgs = ArgsIn.size();
   Expr **Args = ArgsIn.data();
   QualType ReturnType;
   ExprValueKind VK = VK_RValue;
   bool ClassMessage = (ReceiverType->isObjCClassType() ||
                        ReceiverType->isObjCQualifiedClassType());
-  if (CheckMessageArgumentTypes(ReceiverType, MultiExprArg(Args, NumArgs),
-                                Sel, SelectorLocs, Method,
-                                ClassMessage, SuperLoc.isValid(),
+  if (CheckMessageArgumentTypes(Receiver, ReceiverType,
+                                MultiExprArg(Args, NumArgs), Sel, SelectorLocs,
+                                Method, ClassMessage, SuperLoc.isValid(),
                                 LBracLoc, RBracLoc, RecRange, ReturnType, VK))
     return ExprError();
 
   if (Method && !Method->getReturnType()->isVoidType() &&
       RequireCompleteType(LBracLoc, Method->getReturnType(),
                           diag::err_illegal_message_expr_incomplete_type))
     return ExprError();
 

test/SemaObjC/multiple-method-names-in-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 %s
+// expected-no-diagnostics
+
+@interface NSObj
+
++ (instancetype) alloc;
+
++ (_Nonnull instancetype) globalObject;
+
+@end
+
+@interface SelfAllocReturn: NSObj
+
+- (instancetype)initWithFoo:(int)x;
+
+@end
+
+@interface SelfAllocReturn2: NSObj
+
+- (instancetype)initWithFoo:(SelfAllocReturn *)x;
+
+@end
+
+@implementation SelfAllocReturn
+
+- (instancetype)initWithFoo:(int)x {
+    return self;
+}
+
++ (instancetype) thingWithFoo:(int)x {
+    return [[self alloc] initWithFoo: x];
+}
+
++ (void) initGlobal {
+  (void)[[self globalObject] initWithFoo: 20];
+}
+
+@end