ObjC/ObjC++: Another fix for detection of duplicate methods

[Nicola Pero <nicola dot pero at meta-innovation dot com>]

This patch fixes comparison of protocol qualifier lists in types when  
the compiler is trying
to detect duplicate methods in an interface, category or protocol.

The main case that didn't work is --

@protocol A
@end

@protocol B <A>
@end

@interface MyClass
- (void) method: (id <A>)x;
- (void) method: (id <B>)x;
@end

without this patch, GCC incorrectly thinks that the two methods are  
identical including the types (!!) because
of a bug in the protocol comparison code, and does not emit any  
warning or error.  (also note that if you
swap the order of the two methods, GCC suddenly realizes that they are  
different and produces an error). ;-)

The other case that didn't work is a bit more subtle, and I found it  
in the clang testsuite (this is
a shrinked down testcase of my own, but the concept is the same):

@protocol A;

@interface MyClass
- (void) method: (id <A, A>)x;
- (void) method: (id <A>)x;
@end

in this testcase, they seem to assume that "id <A, A>" is a valid type  
(and, in fairness, GCC accepts it
as valid too) and (obviously) identical to "id <A>" and hence the two  
methods have the same types
and so no warning or error should be generated.

But GCC would get confused by the duplicated protocol qualifier; it  
would compare the length of the
protocol list between "id <A, A>" and "id <A>", and conclude they are  
different types, and produce an error.

That is a bug - if we accept duplicated protocols in protocol  
qualifier lists (which we have always done
and I guess we'll keep doing - clang does it too) then we need to be  
able to compare them properly.

Anyway, this patch fixes the comparison of protocol lists in all these  
cases (testcases included).  With this
patch, it all seems to work quite well.

Ok to commit ?

Thanks

Index: objc/objc-act.c
===================================================================
--- objc/objc-act.c     (revision 168350)
+++ objc/objc-act.c     (working copy)
@@ -11925,9 +11925,8 @@ start_method_def (tree method)
   really_start_method (objc_method_context, parm_info);
 }

-/* Return 1 if TYPE1 is equivalent to TYPE2
-   for purposes of method overloading.  */
-
+/* Return 1 if TYPE1 is equivalent to TYPE2 for purposes of method
+   overloading.  */
 static int
 objc_types_are_equivalent (tree type1, tree type2)
 {
@@ -11941,6 +11940,7 @@ objc_types_are_equivalent (tree type1, t
   if (TYPE_MAIN_VARIANT (type1) != TYPE_MAIN_VARIANT (type2))
     return 0;

+  /* Compare the protocol lists.  */
   type1 = (TYPE_HAS_OBJC_INFO (type1)
           ? TYPE_OBJC_PROTOCOL_LIST (type1)
           : NULL_TREE);
@@ -11948,14 +11948,34 @@ objc_types_are_equivalent (tree type1, t
           ? TYPE_OBJC_PROTOCOL_LIST (type2)
           : NULL_TREE);

-  if (list_length (type1) == list_length (type2))
+  /* If there are no protocols (most common case), the types are
+     identical.  */
+  if (type1 == NULL_TREE && type2 == NULL_TREE)
+    return 1;
+
+  /* If one has protocols, and the other one hasn't, they are not
+     identical.  */
+  if ((type1 == NULL_TREE && type2 != NULL_TREE)
+      || (type1 != NULL_TREE && type2 == NULL_TREE))
+    return 0;
+  else
     {
-      for (; type2; type2 = TREE_CHAIN (type2))
-       if (!lookup_protocol_in_reflist (type1, TREE_VALUE (type2)))
+      /* Else, both have protocols, and we need to do the full
+        comparison.  It is possible that either type1 or type2
+        contain some duplicate protocols in the list, so we can't
+        even just compare list_length as a first check.  */
+      tree t;
+
+      for (t = type2; t; t = TREE_CHAIN (t))
+       if (!lookup_protocol_in_reflist (type1, TREE_VALUE (t)))
+         return 0;
+
+      for (t = type1; t; t = TREE_CHAIN (t))
+       if (!lookup_protocol_in_reflist (type2, TREE_VALUE (t)))
          return 0;
+
       return 1;
     }
-  return 0;
 }

 /* Return 1 if TYPE1 has the same size and alignment as TYPE2.  */
Index: objc/ChangeLog
===================================================================
--- objc/ChangeLog      (revision 168350)
+++ objc/ChangeLog      (working copy)
@@ -1,5 +1,12 @@
 2010-12-30  Nicola Pero  <nicola.pero@meta-innovation.com>

+       * objc-act.c (objc_types_are_equivalent): Fixed comparing  
protocol
+       lists.  Check them two-ways to fix comparisons when one protocol
+       implements the other one, or when one list contains duplicated
+       protocols.
+
+2010-12-30  Nicola Pero  <nicola.pero@meta-innovation.com>
+
        * objc-act.c (objc_add_method): When emitting an error  
because a
        method with the same name but conflicting types is found in the
        same class or category interface, print a note with the  
location
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog (revision 168350)
+++ testsuite/ChangeLog (working copy)
@@ -1,4 +1,11 @@
 2010-12-30  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * objc.dg/method-conflict-3.m: New.
+       * objc.dg/method-conflict-4.m: New.
+       * obj-c++.dg/method-conflict-3.m: New.
+       * obj-c++.dg/method-conflict-4.mm: New.
+
+2010-12-30  Nicola Pero  <nicola.pero@meta-innovation.com>

        * objc.dg/class-extension-3.m: Updated.
        * objc.dg/method-1.m: Updated.
Index: testsuite/objc.dg/method-conflict-3.m
===================================================================
--- testsuite/objc.dg/method-conflict-3.m       (revision 0)
+++ testsuite/objc.dg/method-conflict-3.m       (revision 0)
@@ -0,0 +1,63 @@
+/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
December 2010.  */
+/* { dg-do compile } */
+
+#include <objc/objc.h>
+
+/* Test that the compiler can correctly compare protocols in types of
+   method signatures.  */
+
+@protocol A, B, C;
+
+@interface MyClass
+- (void) method1: (id)x;
+- (void) method1: (id)x; /* Ok */
+
+- (void) method2: (id <A>)x;
+- (void) method2: (id <A>)x; /* Ok */
+
+- (void) method3: (id <A, B>)x;
+- (void) method3: (id <A, B>)x; /* Ok */
+
+- (void) method4: (id <A, B>)x;
+- (void) method4: (id <A, B, B>)x; /* Ok */
+
+- (void) method5: (id <A, A, B>)x;
+- (void) method5: (id <A, B, B>)x; /* Ok */
+
+- (void) method6: (id <A, A, B, B, C, C>)x;
+- (void) method6: (id <C, A, B>)x; /* Ok */
+
+- (void) method7: (id)x; /* { dg-message "previous declaration" } */
+- (void) method7: (id <A>)x; /* { dg-error "duplicate declaration" } */
+
+- (void) method8: (id <A>)x; /* { dg-message "previous declaration" }  
*/
+- (void) method8: (id)x; /* { dg-error "duplicate declaration" } */
+
+- (void) method9: (id <A>)x; /* { dg-message "previous declaration" }  
*/
+- (void) method9: (id <B>)x; /* { dg-error "duplicate declaration" } */
+
+- (void) methodA: (id <A>)x; /* { dg-message "previous declaration" }  
*/
+- (void) methodA: (id <A, B>)x; /* { dg-error "duplicate  
declaration" } */
+
+- (void) methodB: (id <A, B>)x; /* { dg-message "previous  
declaration" } */
+- (void) methodB: (id <A>)x; /* { dg-error "duplicate declaration" } */
+
+- (void) methodC: (id <A, B, C>)x; /* { dg-message "previous  
declaration" } */
+- (void) methodC: (id <A, B>)x; /* { dg-error "duplicate  
declaration" } */
+
+- (void) methodD: (id <A, B, C>)x; /* { dg-message "previous  
declaration" } */
+- (void) methodD: (id <A, B, A>)x; /* { dg-error "duplicate  
declaration" } */
+
+- (void) methodE: (MyClass <A, B, C> *)x; /* { dg-message "previous  
declaration" } */
+- (void) methodE: (MyClass <A, B, A> *)x; /* { dg-error "duplicate  
declaration" } */
+
+- (void) methodF: (MyClass <A, B, A> *)x;
+- (void) methodF: (MyClass <A, B, A> *)x; /* Ok */
+
+- (void) methodG: (MyClass *)x;  /* { dg-message "previous  
declaration" } */
+- (void) methodG: (MyClass <A, B, C> *)x; /* { dg-error "duplicate  
declaration" } */
+
+- (void) methodH: (MyClass <A, C>*)x;  /* { dg-message "previous  
declaration" } */
+- (void) methodH: (MyClass *)x; /* { dg-error "duplicate  
declaration" } */
+
+@end
Index: testsuite/objc.dg/method-conflict-4.m
===================================================================
--- testsuite/objc.dg/method-conflict-4.m       (revision 0)
+++ testsuite/objc.dg/method-conflict-4.m       (revision 0)
@@ -0,0 +1,47 @@
+/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
December 2010.  */
+/* { dg-do compile } */
+
+#include <objc/objc.h>
+
+/* Test that the compiler can correctly compare protocols in types of
+   method signatures.  In this test we look at protocols implementing
+   other protocols.  The fact that one protocol implements another one
+   doesn't mean that they are identical.  */
+
+@protocol A
+- (void) doSomething;
+@end
+
+@protocol B <A>
+- (void) doSomethingElse;
+@end
+
+@protocol C <A>
+- (void) doYetSomethingElse;
+@end
+
+@interface MyClass2
+- (void) aMethod: (id <A>)x;  /* { dg-message "previous  
declaration" } */
+- (void) aMethod: (id <B>)x;  /* { dg-error "duplicate declaration" }  
*/
+
+- (void) bMethod: (id <B>)x;  /* { dg-message "previous  
declaration" } */
+- (void) bMethod: (id <A>)x;  /* { dg-error "duplicate declaration" }  
*/
+
+- (void) cMethod: (id <A, B>)x;
+- (void) cMethod: (id <B>)x;  /* Ok - because if you implement B,  
then you also implement A, so <B> == <A, B> */
+
+- (void) dMethod: (id <A, B>)x;
+- (void) dMethod: (id <B, A>)x; /* Ok */
+
+- (void) eMethod: (id <A>)x;  /* { dg-message "previous  
declaration" } */
+- (void) eMethod: (id <B, C>)x;  /* { dg-error "duplicate  
declaration" } */
+
+- (void) fMethod: (id <B, C>)x;  /* { dg-message "previous  
declaration" } */
+- (void) fMethod: (id <A>)x;  /* { dg-error "duplicate declaration" }  
*/
+
+- (void) gMethod: (id <A>)x;  /* { dg-message "previous  
declaration" } */
+- (void) gMethod: (id <A, B, C>)x;  /* { dg-error "duplicate  
declaration" } */
+
+- (void) hMethod: (id <A, B, C>)x;  /* { dg-message "previous  
declaration" } */
+- (void) hMethod: (id <A>)x;  /* { dg-error "duplicate declaration" }  
*/
+@end
Index: testsuite/obj-c++.dg/method-conflict-3.mm
===================================================================
--- testsuite/obj-c++.dg/method-conflict-3.mm   (revision 0)
+++ testsuite/obj-c++.dg/method-conflict-3.mm   (revision 0)
@@ -0,0 +1,65 @@
+/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
December 2010.  */
+/* { dg-do compile } */
+
+#include <objc/objc.h>
+
+/* Test that the compiler can correctly compare protocols in types of
+   method signatures.  */
+
+@protocol A, B, C;
+
+@interface MyClass
+- (void) method1: (id)x;
+- (void) method1: (id)x; /* Ok */
+
+- (void) method2: (id <A>)x;
+- (void) method2: (id <A>)x; /* Ok */
+
+- (void) method3: (id <A, B>)x;
+- (void) method3: (id <A, B>)x; /* Ok */
+
+- (void) method4: (id <A, B>)x;
+- (void) method4: (id <A, B, B>)x; /* Ok */
+
+- (void) method5: (id <A, A, B>)x;
+- (void) method5: (id <A, B, B>)x; /* Ok */
+
+- (void) method6: (id <A, A, B, B, C, C>)x;
+- (void) method6: (id <C, A, B>)x; /* Ok */
+
+- (void) method7: (id)x; /* { dg-warning "previous declaration" } */
+- (void) method7: (id <A>)x; /* { dg-error "duplicate declaration" } */
+
+- (void) method8: (id <A>)x; /* { dg-warning "previous declaration" }  
*/
+- (void) method8: (id)x; /* { dg-error "duplicate declaration" } */
+
+- (void) method9: (id <A>)x; /* { dg-warning "previous declaration" }  
*/
+- (void) method9: (id <B>)x; /* { dg-error "duplicate declaration" } */
+
+- (void) methodA: (id <A>)x; /* { dg-warning "previous declaration" }  
*/
+- (void) methodA: (id <A, B>)x; /* { dg-error "duplicate  
declaration" } */
+
+/* FIXME: Bug in the testsuite - the following are done Ok by the  
compiler, but
+   the testsuite barfs so we have to comment out the tests.  */
+/* - (void) methodB: (id <A, B>)x; dg-warning "previous declaration" */
+/* - (void) methodB: (id <A>)x; dg-error "duplicate declaration" */
+
+/* - (void) methodC: (id <A, B, C>)x;  dg-warning "previous  
declaration"  */
+/* - (void) methodC: (id <A, B>)x;  dg-error "duplicate declaration"   
*/
+
+/* - (void) methodD: (id <A, B, C>)x;  dg-warning "previous  
declaration"  */
+/* - (void) methodD: (id <A, B, A>)x;  dg-error "duplicate  
declaration"  */
+
+/* - (void) methodE: (MyClass <A, B, C> *)x;  dg-warning "previous  
declaration"  */
+/* - (void) methodE: (MyClass <A, B, A> *)x;  dg-error "duplicate  
declaration"  */
+
+- (void) methodF: (MyClass <A, B, A> *)x;
+- (void) methodF: (MyClass <A, B, A> *)x; /* Ok */
+
+/* - (void) methodG: (MyClass *)x;   dg-warning "previous  
declaration"  */
+/* - (void) methodG: (MyClass <A, B, C> *)x;  dg-error "duplicate  
declaration"  */
+
+/* - (void) methodH: (MyClass <A, C>*)x;  dg-warning "previous  
declaration"  */
+/* - (void) methodH: (MyClass *)x;  dg-error "duplicate declaration"   
*/
+
+@end
Index: testsuite/obj-c++.dg/method-conflict-4.mm
===================================================================
--- testsuite/obj-c++.dg/method-conflict-4.mm   (revision 0)
+++ testsuite/obj-c++.dg/method-conflict-4.mm   (revision 0)
@@ -0,0 +1,47 @@
+/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
December 2010.  */
+/* { dg-do compile } */
+
+#include <objc/objc.h>
+
+/* Test that the compiler can correctly compare protocols in types of
+   method signatures.  In this test we look at protocols implementing
+   other protocols.  The fact that one protocol implements another one
+   doesn't mean that they are identical.  */
+
+@protocol A
+- (void) doSomething;
+@end
+
+@protocol B <A>
+- (void) doSomethingElse;
+@end
+
+@protocol C <A>
+- (void) doYetSomethingElse;
+@end
+
+@interface MyClass2
+- (void) aMethod: (id <A>)x;  /* { dg-error "previous declaration" } */
+- (void) aMethod: (id <B>)x;  /* { dg-error "duplicate declaration" }  
*/
+
+- (void) bMethod: (id <B>)x;  /* { dg-error "previous declaration" } */
+- (void) bMethod: (id <A>)x;  /* { dg-error "duplicate declaration" }  
*/
+
+- (void) cMethod: (id <A, B>)x;
+- (void) cMethod: (id <B>)x;  /* Ok - because if you implement B,  
then you also implement A, so <B> == <A, B> */
+
+- (void) dMethod: (id <A, B>)x;
+- (void) dMethod: (id <B, A>)x; /* Ok */
+
+/* FIXME: The compiler works, but the testsuite produces errors  
anyway.  */
+/* - (void) eMethod: (id <A>)x;   dg-error "previous declaration"  */
+/* - (void) eMethod: (id <B, C>)x;   dg-error "duplicate  
declaration"  */
+
+/*- (void) fMethod: (id <B, C>)x;   dg-error "previous declaration"  */
+/*- (void) fMethod: (id <A>)x;   dg-error "duplicate declaration"  */
+
+/* - (void) gMethod: (id <A>)x;   dg-error "previous declaration" */
+/* - (void) gMethod: (id <A, B, C>)x;   dg-error "duplicate  
declaration" */
+
+/* - (void) hMethod: (id <A, B, C>)x;   dg-error "previous  
declaration" */
+/* - (void) hMethod: (id <A>)x;   dg-error "duplicate declaration" */
+@end