[cfe-commits] r117271 - in /cfe/trunk: lib/Sema/SemaDeclObjC.cpp test/SemaObjC/class-conforming-protocol-2.m test/SemaObjC/method-conflict-1.m test/SemaObjC/method-typecheck-3.m

[David Chisnall]

Author: theraven
Date: Mon Oct 25 12:23:52 2010
New Revision: 117271

URL: http://llvm.org/viewvc/llvm-project?rev=117271&view=rev
Log:
Only warn for mismatched types in Objective-C methods when they are incompatible, not when they are simply different.  Now we test whether the difference in types breaks the principle of substitutability, rather than whether they are different.

A common idiom in Objective-C is to provide a definition of a method in a subclass that returns a more-specified version of an object than the superclass.  This does not violate the principle of substitutability, because you can always use the object returned by the subclass anywhere that you could use the type returned by the superclass.  It was, however, generating warnings with clang, leading people to believe that semantically correct code was incorrect and requiring less accurate type specification and explicit down-casts (neither of which is a good thing to encourage).

This change ensures that any method definition has parameter and return types that make it accept anything that something conforming to the declaration may pass and return something that the caller will expect, but allows stricter definitions.  


Added:
    cfe/trunk/test/SemaObjC/method-typecheck-3.m
      - copied, changed from r117258, cfe/trunk/test/SemaObjC/method-typecheck-1.m
Modified:
    cfe/trunk/lib/Sema/SemaDeclObjC.cpp
    cfe/trunk/test/SemaObjC/class-conforming-protocol-2.m
    cfe/trunk/test/SemaObjC/method-conflict-1.m

Modified: cfe/trunk/lib/Sema/SemaDeclObjC.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclObjC.cpp?rev=117271&r1=117270&r2=117271&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaDeclObjC.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclObjC.cpp Mon Oct 25 12:23:52 2010
@@ -749,14 +749,96 @@
     << method->getDeclName();
 }
 
+/// Determines if type B can be substituted for type A.  Returns true if we can
+/// guarantee that anything that the user will do to an object of type A can 
+/// also be done to an object of type B.  This is trivially true if the two 
+/// types are the same, or if B is a subclass of A.  It becomes more complex
+/// in cases where protocols are involved.
+///
+/// Object types in Objective-C describe the minimum requirements for an
+/// object, rather than providing a complete description of a type.  For
+/// example, if A is a subclass of B, then B* may refer to an instance of A.
+/// The principle of substitutability means that we may use an instance of A
+/// anywhere that we may use an instance of B - it will implement all of the
+/// ivars of B and all of the methods of B.  
+///
+/// This substitutability is important when type checking methods, because 
+/// the implementation may have stricter type definitions than the interface.
+/// The interface specifies minimum requirements, but the implementation may
+/// have more accurate ones.  For example, a method may privately accept 
+/// instances of B, but only publish that it accepts instances of A.  Any
+/// object passed to it will be type checked against B, and so will implicitly
+/// by a valid A*.  Similarly, a method may return a subclass of the class that
+/// it is declared as returning.
+///
+/// This is most important when considering subclassing.  A method in a
+/// subclass must accept any object as an argument that its superclass's
+/// implementation accepts.  It may, however, accept a more general type
+/// without breaking substitutability (i.e. you can still use the subclass
+/// anywhere that you can use the superclass, but not vice versa).  The
+/// converse requirement applies to return types: the return type for a
+/// subclass method must be a valid object of the kind that the superclass
+/// advertises, but it may be specified more accurately.  This avoids the need
+/// 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;
+
+  // 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.
+  // For example, MyClass<A> can be assigned to id<A>, but MyClass<A> is a
+  // stricter definition so it is not substitutable for id<A>.
+  if (B->isObjCQualifiedIdType()) {
+    return A->isObjCQualifiedIdType() &&
+      C.ObjCQualifiedIdTypesAreCompatible(A, B, false);
+  }
+
+  /*
+  // id is a special type that bypasses type checking completely.  We want a
+  // warning when it is used in one place but not another.
+  if (C.isObjCIdType(A) || C.isObjCIdType(B)) return false;
+
+
+  // If B is a qualified id, then A must also be a qualified id (which it isn't
+  // if we've got this far)
+  if (B->isObjCQualifiedIdType()) return false;
+  */
+
+  // Now we know that A and B are (potentially-qualified) class types.  The
+  // normal rules for assignment apply.
+  return C.canAssignObjCInterfaces(a, b);
+}
+
 void Sema::WarnConflictingTypedMethods(ObjCMethodDecl *ImpMethodDecl,
                                        ObjCMethodDecl *IntfMethodDecl) {
   if (!Context.hasSameType(IntfMethodDecl->getResultType(),
                            ImpMethodDecl->getResultType())) {
-    Diag(ImpMethodDecl->getLocation(), diag::warn_conflicting_ret_types)
-      << ImpMethodDecl->getDeclName() << IntfMethodDecl->getResultType()
-      << ImpMethodDecl->getResultType();
-    Diag(IntfMethodDecl->getLocation(), diag::note_previous_definition);
+    // 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(Context, IntfMethodDecl->getResultType(),
+          ImpMethodDecl->getResultType())) {
+      Diag(ImpMethodDecl->getLocation(), diag::warn_conflicting_ret_types)
+        << ImpMethodDecl->getDeclName() << IntfMethodDecl->getResultType()
+        << ImpMethodDecl->getResultType();
+      Diag(IntfMethodDecl->getLocation(), diag::note_previous_definition);
+    }
   }
 
   for (ObjCMethodDecl::param_iterator IM = ImpMethodDecl->param_begin(),
@@ -767,6 +849,14 @@
     if (Context.hasSameType(ParmDeclTy, ParmImpTy))
       continue;
 
+    // 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(Context, ParmImpTy, ParmDeclTy, true))
+      continue;
+
     Diag((*IM)->getLocation(), diag::warn_conflicting_param_types)
       << ImpMethodDecl->getDeclName() << (*IF)->getType()
       << (*IM)->getType();

Modified: cfe/trunk/test/SemaObjC/class-conforming-protocol-2.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/class-conforming-protocol-2.m?rev=117271&r1=117270&r2=117271&view=diff
==============================================================================
--- cfe/trunk/test/SemaObjC/class-conforming-protocol-2.m (original)
+++ cfe/trunk/test/SemaObjC/class-conforming-protocol-2.m Mon Oct 25 12:23:52 2010
@@ -2,13 +2,14 @@
 
 @protocol NSWindowDelegate @end
 
+ at protocol IBStringsTableWindowDelegate <NSWindowDelegate>
+ at end
+
 @interface NSWindow
 - (void)setDelegate:(id <NSWindowDelegate>)anObject; // expected-note {{previous definition is here}}
-- (id <NSWindowDelegate>) delegate; // expected-note {{previous definition is here}}
+- (id <IBStringsTableWindowDelegate>) delegate; // expected-note {{previous definition is here}}
 @end
 
- at protocol IBStringsTableWindowDelegate <NSWindowDelegate>
- at end
 
 @interface IBStringsTableWindow : NSWindow {}
 @end
@@ -16,7 +17,7 @@
 @implementation IBStringsTableWindow
 - (void)setDelegate:(id <IBStringsTableWindowDelegate>)delegate { // expected-warning {{conflicting parameter types in implementation of 'setDelegate:'}}
 }
-- (id <IBStringsTableWindowDelegate>)delegate { // expected-warning {{conflicting return type in implementation of 'delegate':}}
+- (id <NSWindowDelegate>)delegate { // expected-warning {{conflicting return type in implementation of 'delegate':}}
         return 0;
 }
 @end

Modified: cfe/trunk/test/SemaObjC/method-conflict-1.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/method-conflict-1.m?rev=117271&r1=117270&r2=117271&view=diff
==============================================================================
--- cfe/trunk/test/SemaObjC/method-conflict-1.m (original)
+++ cfe/trunk/test/SemaObjC/method-conflict-1.m Mon Oct 25 12:23:52 2010
@@ -7,12 +7,12 @@
 
 @interface MyClass : NSObject {
 }
-- (void)myMethod:(NSArray *)object; // expected-note {{previous definition is here}}
+- (void)myMethod:(NSArray *)object; 
 - (void)myMethod1:(NSObject *)object; // expected-note {{previous definition is here}}
 @end
 
 @implementation MyClass
-- (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 *'}}
 }

Copied: cfe/trunk/test/SemaObjC/method-typecheck-3.m (from r117258, cfe/trunk/test/SemaObjC/method-typecheck-1.m)
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/method-typecheck-3.m?p2=cfe/trunk/test/SemaObjC/method-typecheck-3.m&p1=cfe/trunk/test/SemaObjC/method-typecheck-1.m&r1=117258&r2=117271&rev=117271&view=diff
==============================================================================
--- cfe/trunk/test/SemaObjC/method-typecheck-1.m (original)
+++ cfe/trunk/test/SemaObjC/method-typecheck-3.m Mon Oct 25 12:23:52 2010
@@ -1,36 +1,21 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 @interface A
-- (void) setMoo: (int) x;	//  expected-note {{previous definition is here}}
-- (int) setMoo1: (int) x;	//  expected-note {{previous definition is here}}
-- (int) setOk : (int) x : (double) d;
- at end
-
- at implementation A 
--(void) setMoo: (float) x {}	//  expected-warning {{conflicting parameter types in implementation of 'setMoo:': 'int' vs 'float'}}
-- (char) setMoo1: (int) x { return 0; }	//  expected-warning {{conflicting return type in implementation of 'setMoo1:': 'int' vs 'char'}}
-- (int) setOk : (int) x : (double) d { return 0; }
- at end
-
-
-
- at interface C
-+ (void) cMoo: (int) x;	//  expected-note 2 {{previous definition is here}}
- at end
-
- at implementation C 
-+(float) cMoo:   // expected-warning {{conflicting return type in implementation of 'cMoo:': 'void' vs 'float'}}
-   (float) x { return 0; }	//  expected-warning {{conflicting parameter types in implementation of 'cMoo:': 'int' vs 'float'}}
+- (id)obj;
+- (A*)a;
+- (void)takesA: (A*)a; // expected-note {{previous definition is here}}
+- (void)takesId: (id)a; // expected-note {{previous definition is here}}
 @end
 
 
- at interface A(CAT)
-- (void) setCat: (int) x;	// expected-note 2 {{previous definition is here}}
-+ (void) cCat: (int) x;	//  expected-note {{previous definition is here}}
+ at interface B : A
 @end
 
- at implementation A(CAT) 
--(float) setCat:  // expected-warning {{conflicting return type in implementation of 'setCat:': 'void' vs 'float'}}
-(float) x { return 0; }	//  expected-warning {{conflicting parameter types in implementation of 'setCat:': 'int' vs 'float'}}
-+ (int) cCat: (int) x { return 0; }	//  expected-warning {{conflicting return type in implementation of 'cCat:': 'void' vs 'int'}}
+ at implementation B
+- (B*)obj {return self;}
+- (B*)a { return self;} 
+- (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:'}}
+{}
 @end