Implement the newest status quo for method override checking. The idea now

is that we need more information to decide the exact conditions for whether
one ObjCObjectPointer is an acceptable return/parameter override for another,
so we're going to disable that entire class of warning for now.  The
"forward developement" warning category, -Wmethod-signatures, can receive
unrestricted feature work, and when we're happy with how it acts, we'll
turn it on by default.

This is a pretty conservative change, and nobody's totally content with it.


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@117524 91177308-0d34-0410-b5e6-96231b3b80d8

9 files changed, 161 insertions, 84 deletions

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index ae177761cf..7efc7d5d41 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -326,15 +326,15 @@ def note_required_for_protocol_at :
 
 def warn_conflicting_ret_types : Warning<
   "conflicting return type in implementation of %0: %1 vs %2">;
-def warn_covariant_ret_types : Warning<
+def warn_non_covariant_ret_types : Warning<
   "conflicting return type in implementation of %0: %1 vs %2">,
-  InGroup<DiagGroup<"objc-covariant-overrides">>;
+  InGroup<DiagGroup<"method-signatures">>, DefaultIgnore;
 
 def warn_conflicting_param_types : Warning<
   "conflicting parameter types in implementation of %0: %1 vs %2">;
-def warn_contravariant_param_types : Warning<
+def warn_non_contravariant_param_types : Warning<
   "conflicting parameter types in implementation of %0: %1 vs %2">,
-  InGroup<DiagGroup<"objc-covariant-overrides">>;
+  InGroup<DiagGroup<"method-signatures">>, DefaultIgnore;
 def warn_conflicting_variadic :Warning<
   "conflicting variadic declaration of method and its implementation">;
 
diff --git a/lib/Sema/SemaDeclObjC.cpp b/lib/Sema/SemaDeclObjC.cpp
index ed3f5a29b8..bd16fa3994 100644
--- a/lib/Sema/SemaDeclObjC.cpp
+++ b/lib/Sema/SemaDeclObjC.cpp
@@ -782,22 +782,12 @@ void Sema::WarnUndefinedMethod(SourceLocation ImpLoc, ObjCMethodDecl *method,
 /// for explicit down-casting by callers.
 ///
 /// Note: This is a stricter requirement than for assignment.  
-static bool isObjCTypeSubstitutable(ASTContext &C, QualType A, QualType B, bool
-    rejectId=false) {
-  ObjCObjectPointerType *a = dyn_cast<ObjCObjectPointerType>(
-      C.getCanonicalType(A));
-  ObjCObjectPointerType *b = dyn_cast<ObjCObjectPointerType>(
-      C.getCanonicalType(B));
-  // Ignore non-ObjC types.
-  if (!(a && b)) 
-  {
-	  //a->dump(); b->dump();
-	  return false;
-  }
-  // A type is always substitutable for itself
-  if (C.hasSameType(A, B)) return true;
-
-  if (rejectId && C.isObjCIdType(B)) return false;
+static bool isObjCTypeSubstitutable(ASTContext &Context,
+                                    const ObjCObjectPointerType *A,
+                                    const ObjCObjectPointerType *B,
+                                    bool rejectId) {
+  // Reject a protocol-unqualified id.
+  if (rejectId && B->isObjCIdType()) return false;
 
   // If B is a qualified id, then A must also be a qualified id and it must
   // implement all of the protocols in B.  It may not be a qualified class.
@@ -805,7 +795,9 @@ static bool isObjCTypeSubstitutable(ASTContext &C, QualType A, QualType B, bool
   // stricter definition so it is not substitutable for id<A>.
   if (B->isObjCQualifiedIdType()) {
     return A->isObjCQualifiedIdType() &&
-      C.ObjCQualifiedIdTypesAreCompatible(A, B, false);
+           Context.ObjCQualifiedIdTypesAreCompatible(QualType(A, 0),
+                                                     QualType(B,0),
+                                                     false);
   }
 
   /*
@@ -821,57 +813,94 @@ static bool isObjCTypeSubstitutable(ASTContext &C, QualType A, QualType B, bool
 
   // Now we know that A and B are (potentially-qualified) class types.  The
   // normal rules for assignment apply.
-  return C.canAssignObjCInterfaces(a, b);
+  return Context.canAssignObjCInterfaces(A, B);
 }
 
-void Sema::WarnConflictingTypedMethods(ObjCMethodDecl *ImpMethodDecl,
-                                       ObjCMethodDecl *IntfMethodDecl) {
-  if (!Context.hasSameType(IntfMethodDecl->getResultType(),
-                           ImpMethodDecl->getResultType())) {
-    // Allow non-matching return types as long as they don't violate the
-    // principle of substitutability.  Specifically, we permit return types
-    // that are subclasses of the declared return type, or that are
-    // more-qualified versions of the declared type.
-
-    // As a possibly-temporary adjustment, we still warn in the
-    // covariant case, just with a different diagnostic (mapped to
-    // nothing under certain circumstances).
-    unsigned DiagID = diag::warn_conflicting_ret_types;
-    if (isObjCTypeSubstitutable(Context, IntfMethodDecl->getResultType(),
-          ImpMethodDecl->getResultType()))
-      DiagID = diag::warn_covariant_ret_types;
-
-    Diag(ImpMethodDecl->getLocation(), DiagID)
-      << ImpMethodDecl->getDeclName() << IntfMethodDecl->getResultType()
-      << ImpMethodDecl->getResultType();
-    Diag(IntfMethodDecl->getLocation(), diag::note_previous_definition);
+static SourceRange getTypeRange(TypeSourceInfo *TSI) {
+  return (TSI ? TSI->getTypeLoc().getSourceRange() : SourceRange());
+}
+
+static void CheckMethodOverrideReturn(Sema &S,
+                                      ObjCMethodDecl *MethodImpl,
+                                      ObjCMethodDecl *MethodIface) {
+  if (S.Context.hasSameUnqualifiedType(MethodImpl->getResultType(),
+                                       MethodIface->getResultType()))
+    return;
+
+  unsigned DiagID = diag::warn_conflicting_ret_types;
+
+  // Mismatches between ObjC pointers go into a different warning
+  // category, and sometimes they're even completely whitelisted.
+  if (const ObjCObjectPointerType *ImplPtrTy =
+        MethodImpl->getResultType()->getAs<ObjCObjectPointerType>()) {
+    if (const ObjCObjectPointerType *IfacePtrTy =
+          MethodIface->getResultType()->getAs<ObjCObjectPointerType>()) {
+      // Allow non-matching return types as long as they don't violate
+      // the principle of substitutability.  Specifically, we permit
+      // return types that are subclasses of the declared return type,
+      // or that are more-qualified versions of the declared type.
+      if (isObjCTypeSubstitutable(S.Context, IfacePtrTy, ImplPtrTy, false))
+        return;
+
+      DiagID = diag::warn_non_covariant_ret_types;
+    }
   }
 
-  for (ObjCMethodDecl::param_iterator IM = ImpMethodDecl->param_begin(),
-       IF = IntfMethodDecl->param_begin(), EM = ImpMethodDecl->param_end();
-       IM != EM; ++IM, ++IF) {
-    QualType ParmDeclTy = (*IF)->getType().getUnqualifiedType();
-    QualType ParmImpTy = (*IM)->getType().getUnqualifiedType();
-    if (Context.hasSameType(ParmDeclTy, ParmImpTy))
-      continue;
+  S.Diag(MethodImpl->getLocation(), DiagID)
+    << MethodImpl->getDeclName()
+    << MethodIface->getResultType()
+    << MethodImpl->getResultType()
+    << getTypeRange(MethodImpl->getResultTypeSourceInfo());
+  S.Diag(MethodIface->getLocation(), diag::note_previous_definition)
+    << getTypeRange(MethodIface->getResultTypeSourceInfo());
+}
 
-    // Allow non-matching argument types as long as they don't violate the
-    // principle of substitutability.  Specifically, the implementation must
-    // accept any objects that the superclass accepts, however it may also
-    // accept others.
+static void CheckMethodOverrideParam(Sema &S,
+                                     ObjCMethodDecl *MethodImpl,
+                                     ObjCMethodDecl *MethodIface,
+                                     ParmVarDecl *ImplVar,
+                                     ParmVarDecl *IfaceVar) {
+  QualType ImplTy = ImplVar->getType();
+  QualType IfaceTy = IfaceVar->getType();
+  if (S.Context.hasSameUnqualifiedType(ImplTy, IfaceTy))
+    return;
 
-    // As a possibly-temporary adjustment, we still warn in the
-    // contravariant case, just with a different diagnostic (mapped to
-    // nothing under certain circumstances).
-    unsigned DiagID = diag::warn_conflicting_param_types;
-    if (isObjCTypeSubstitutable(Context, ParmImpTy, ParmDeclTy, true))
-      DiagID = diag::warn_contravariant_param_types;
+  unsigned DiagID = diag::warn_conflicting_param_types;
+
+  // Mismatches between ObjC pointers go into a different warning
+  // category, and sometimes they're even completely whitelisted.
+  if (const ObjCObjectPointerType *ImplPtrTy =
+        ImplTy->getAs<ObjCObjectPointerType>()) {
+    if (const ObjCObjectPointerType *IfacePtrTy =
+          IfaceTy->getAs<ObjCObjectPointerType>()) {
+      // Allow non-matching argument types as long as they don't
+      // violate the principle of substitutability.  Specifically, the
+      // implementation must accept any objects that the superclass
+      // accepts, however it may also accept others.
+      if (isObjCTypeSubstitutable(S.Context, ImplPtrTy, IfacePtrTy, true))
+        return;
 
-    Diag((*IM)->getLocation(), DiagID)
-      << ImpMethodDecl->getDeclName() << (*IF)->getType()
-      << (*IM)->getType();
-    Diag((*IF)->getLocation(), diag::note_previous_definition);
+      DiagID = diag::warn_non_contravariant_param_types;
+    }
   }
+
+  S.Diag(ImplVar->getLocation(), DiagID)
+    << getTypeRange(ImplVar->getTypeSourceInfo())
+    << MethodImpl->getDeclName() << IfaceTy << ImplTy;
+  S.Diag(IfaceVar->getLocation(), diag::note_previous_definition)
+    << getTypeRange(IfaceVar->getTypeSourceInfo());
+}
+                                     
+
+void Sema::WarnConflictingTypedMethods(ObjCMethodDecl *ImpMethodDecl,
+                                       ObjCMethodDecl *IntfMethodDecl) {
+  CheckMethodOverrideReturn(*this, ImpMethodDecl, IntfMethodDecl);
+
+  for (ObjCMethodDecl::param_iterator IM = ImpMethodDecl->param_begin(),
+       IF = IntfMethodDecl->param_begin(), EM = ImpMethodDecl->param_end();
+       IM != EM; ++IM, ++IF)
+    CheckMethodOverrideParam(*this, ImpMethodDecl, IntfMethodDecl, *IM, *IF);
+
   if (ImpMethodDecl->isVariadic() != IntfMethodDecl->isVariadic()) {
     Diag(ImpMethodDecl->getLocation(), diag::warn_conflicting_variadic);
     Diag(IntfMethodDecl->getLocation(), diag::note_previous_declaration);
diff --git a/test/SemaObjC/class-conforming-protocol-2.m b/test/SemaObjC/class-conforming-protocol-2.m
index 29e5810338..a3bd0b1d23 100644
--- a/test/SemaObjC/class-conforming-protocol-2.m
+++ b/test/SemaObjC/class-conforming-protocol-2.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1  -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wmethod-signatures -fsyntax-only -verify %s
 
 @protocol NSWindowDelegate @end
 
diff --git a/test/SemaObjC/comptypes-a.m b/test/SemaObjC/comptypes-a.m
index d7dddaad52..8480f524dc 100644
--- a/test/SemaObjC/comptypes-a.m
+++ b/test/SemaObjC/comptypes-a.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -pedantic %s
+// RUN: %clang_cc1 -fsyntax-only -Wmethod-signatures -verify -pedantic %s
 typedef signed char BOOL;
 typedef int NSInteger;
 
diff --git a/test/SemaObjC/method-conflict-1.m b/test/SemaObjC/method-conflict-1.m
index fb62e2ef35..3cf2c6b5a9 100644
--- a/test/SemaObjC/method-conflict-1.m
+++ b/test/SemaObjC/method-conflict-1.m
@@ -1,4 +1,7 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// This test case tests the default behavior.
+
 // rdar://7933061
 
 @interface NSObject @end
@@ -7,15 +10,14 @@
 
 @interface MyClass : NSObject {
 }
-- (void)myMethod:(NSArray *)object; // expected-note {{previous definition is here}}
-- (void)myMethod1:(NSObject *)object; // expected-note {{previous definition is here}}
+- (void)myMethod:(NSArray *)object;
+- (void)myMethod1:(NSObject *)object; // broken-note {{previous definition is here}}
 @end
 
 @implementation MyClass
-// Warn about this contravariant use for now:
-- (void)myMethod:(NSObject *)object { // expected-warning {{conflicting parameter types in implementation of 'myMethod:': 'NSArray *' vs 'NSObject *'}}
+- (void)myMethod:(NSObject *)object {
 }
-- (void)myMethod1:(NSArray *)object { // expected-warning {{conflicting parameter types in implementation of 'myMethod1:': 'NSObject *' vs 'NSArray *'}}
+- (void)myMethod1:(NSArray *)object { // broken-warning {{conflicting parameter types in implementation of 'myMethod1:': 'NSObject *' vs 'NSArray *'}}
 }
 @end
 
@@ -24,13 +26,58 @@
 
 @interface MyOtherClass : NSObject <MyProtocol> {
 }
-- (void)myMethod:(id <MyProtocol>)object; // expected-note {{previous definition is here}}
-- (void)myMethod1:(id <MyProtocol>)object; // expected-note {{previous definition is here}}
+- (void)myMethod:(id <MyProtocol>)object; // broken-note {{previous definition is here}}
+- (void)myMethod1:(id <MyProtocol>)object; // broken-note {{previous definition is here}}
 @end
 
 @implementation MyOtherClass
-- (void)myMethod:(MyClass *)object { // expected-warning {{conflicting parameter types in implementation of 'myMethod:': 'id<MyProtocol>' vs 'MyClass *'}}
+- (void)myMethod:(MyClass *)object { // broken-warning {{conflicting parameter types in implementation of 'myMethod:': 'id<MyProtocol>' vs 'MyClass *'}}
 }
-- (void)myMethod1:(MyClass<MyProtocol> *)object { // expected-warning {{conflicting parameter types in implementation of 'myMethod1:': 'id<MyProtocol>' vs 'MyClass<MyProtocol> *'}}
+- (void)myMethod1:(MyClass<MyProtocol> *)object { // broken-warning {{conflicting parameter types in implementation of 'myMethod1:': 'id<MyProtocol>' vs 'MyClass<MyProtocol> *'}}
 }
 @end
+
+
+
+@interface A @end
+@interface B : A @end
+
+@interface Test1 {}
+- (void) test1:(A*) object; // broken-note {{previous definition is here}} 
+- (void) test2:(B*) object;
+@end
+
+@implementation Test1
+- (void) test1:(B*) object {} // broken-warning {{conflicting parameter types in implementation of 'test1:': 'A *' vs 'B *'}}
+- (void) test2:(A*) object {}
+@end
+
+// rdar://problem/8597621 wants id -> A* to be an exception
+@interface Test2 {}
+- (void) test1:(id) object; // broken-note {{previous definition is here}} 
+- (void) test2:(A*) object;
+@end
+@implementation Test2
+- (void) test1:(A*) object {} // broken-warning {{conflicting parameter types in implementation of 'test1:': 'id' vs 'A *'}}
+- (void) test2:(id) object {}
+@end
+
+@interface Test3 {}
+- (A*) test1;
+- (B*) test2; // broken-note {{previous definition is here}} 
+@end
+
+@implementation Test3
+- (B*) test1 { return 0; }
+- (A*) test2 { return 0; } // broken-warning {{conflicting return type in implementation of 'test2': 'B *' vs 'A *'}}
+@end
+
+// The particular case of overriding with an id return is white-listed.
+@interface Test4 {}
+- (id) test1;
+- (A*) test2;
+@end
+@implementation Test4
+- (A*) test1 { return 0; } // id -> A* is rdar://problem/8596987
+- (id) test2 { return 0; }
+@end
diff --git a/test/SemaObjC/method-conflict-2.m b/test/SemaObjC/method-conflict-2.m
index fd47282455..7b5a08ad9e 100644
--- a/test/SemaObjC/method-conflict-2.m
+++ b/test/SemaObjC/method-conflict-2.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -Wno-objc-covariant-overrides -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wmethod-signatures -fsyntax-only -verify %s
 
 @interface A @end
 @interface B : A @end
@@ -39,6 +39,6 @@
 - (A*) test2;
 @end
 @implementation Test4
-- (A*) test1 { return 0; }
+- (A*) test1 { return 0; } // id -> A* is rdar://problem/8596987
 - (id) test2 { return 0; }
 @end
diff --git a/test/SemaObjC/method-conflict.m b/test/SemaObjC/method-conflict.m
index ecdf1d88ca..4eb7290612 100644
--- a/test/SemaObjC/method-conflict.m
+++ b/test/SemaObjC/method-conflict.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wmethod-signatures -fsyntax-only -verify %s
 
 typedef signed char BOOL;
 typedef unsigned int NSUInteger;
diff --git a/test/SemaObjC/method-def-1.m b/test/SemaObjC/method-def-1.m
index 7630ad0fbd..bc7ea7bc44 100644
--- a/test/SemaObjC/method-def-1.m
+++ b/test/SemaObjC/method-def-1.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wmethod-signatures -fsyntax-only -verify %s
 
 @interface foo
 - (int)meth;
diff --git a/test/SemaObjC/method-typecheck-3.m b/test/SemaObjC/method-typecheck-3.m
index 23036e65d5..1999b7d477 100644
--- a/test/SemaObjC/method-typecheck-3.m
+++ b/test/SemaObjC/method-typecheck-3.m
@@ -1,8 +1,9 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wmethod-signatures -fsyntax-only -verify %s
 
+@class B;
 @interface A
-- (id)obj; // expected-note {{previous definition is here}}
-- (A*)a; // expected-note {{previous definition is here}}
+- (B*)obj;
+- (B*)a; // expected-note {{previous definition is here}}
 - (void)takesA: (A*)a; // expected-note {{previous definition is here}}
 - (void)takesId: (id)a; // expected-note {{previous definition is here}}
 @end
@@ -12,8 +13,8 @@
 @end
 
 @implementation B
-- (B*)obj {return self;} // expected-warning {{conflicting return type in implementation of 'obj'}}
-- (B*)a { return self;}  // expected-warning {{conflicting return type in implementation of 'a'}}
+- (id)obj {return self;} // 'id' overrides are white-listed?
+- (A*)a { return self;}  // expected-warning {{conflicting return type in implementation of 'a'}}
 - (void)takesA: (B*)a  // expected-warning {{conflicting parameter types in implementation of 'takesA:'}}
 {}
 - (void)takesId: (B*)a // expected-warning {{conflicting parameter types in implementation of 'takesId:'}}