ObjC/ObjC++: Improve detection of duplicate method declarations, and related error messages

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

This patch improves the detection of duplicate method declarations,  
and the corresponding error messages:

 * when a duplicate method declaration (with conflicting types) is  
encountered in an interface or category,
it also prints the location of the previous, conflicting method  
declaration;

 * it changes the error to clearly state that the problem is that the  
duplicate declaration has conflicting types;

 * it extends the check to @protocols as well.  Clang does these  
checks for protocols as well, and it's totally right
in doing so - there is no reason why declaring the same method in the  
very same @protocol twice with conflicting
signatures should be allowed (it seems to be a bug in GCC that that  
check was missing);

 * it adds a check that a method is not declared twice in the same  
@protocol with conflicting optional specifications
(ie, once as @required and once as @optional).

It's fairly unusual for real code to actually trigger any of these  
errors because the checks are for conflicting methods
only inside the same interface or protocol.  On the other hand, these  
are serious errors and it is nice to have clear and
helpful diagnostic when they occur, and to have protocols (and the new  
@optional/@required) checked too.

The patch updates the existing testcases, and adds 4 relevant  
testcases for the new protocol checks.

Ok to commit ?

Thanks

Index: objc/objc-act.c
===================================================================
--- objc/objc-act.c     (revision 168341)
+++ objc/objc-act.c     (working copy)
@@ -8898,22 +8898,69 @@ add_method_to_hash_list (hash *hash_list
 static tree
 objc_add_method (tree klass, tree method, int is_class, bool  
is_optional)
 {
-  tree mth;
+  tree existing_method = NULL_TREE;

-  /* @optional methods are added to protocol's OPTIONAL list.  Note
-     that this disables checking that the methods are implemented by
-     classes implementing the protocol, since these checks only use
-     the CLASS_CLS_METHODS and CLASS_NST_METHODS.  */
-  if (is_optional)
-    {
-      gcc_assert (TREE_CODE (klass) == PROTOCOL_INTERFACE_TYPE);
-      if (!(mth = lookup_method (is_class
-                               ? PROTOCOL_OPTIONAL_CLS_METHODS (klass)
-                               : PROTOCOL_OPTIONAL_NST_METHODS (klass),
-                                                                
method)))
+  /* The first thing we do is look up the method in the list of
+     methods already defined in the interface (or implementation).  */
+  if (is_class)
+    existing_method = lookup_method (CLASS_CLS_METHODS (klass),  
method);
+  else
+    existing_method = lookup_method (CLASS_NST_METHODS (klass),  
method);
+
+  /* In the case of protocols, we have a second list of methods to
+     consider, the list of optional ones.  */
+  if (TREE_CODE (klass) == PROTOCOL_INTERFACE_TYPE)
+    {
+      /* @required methods are added to the protocol's normal list.
+        @optional methods are added to the protocol's OPTIONAL lists.
+        Note that adding the methods to the optional lists disables
+        checking that the methods are implemented by classes
+        implementing the protocol, since these checks only use the
+        CLASS_CLS_METHODS and CLASS_NST_METHODS.  */
+
+      /* First of all, if the method to add is @optional, and we found
+        it already existing as @required, emit an error.  */
+      if (is_optional && existing_method)
+       {
+         error ("method %<%c%E%> declared %<@optional%> and  
%<@required%> at the same time",
+                (is_class ? '+' : '-'),
+                METHOD_SEL_NAME (existing_method));
+         inform (DECL_SOURCE_LOCATION (existing_method),
+                 "previous declaration of %<%c%E%> as %<@required%>",
+                 (is_class ? '+' : '-'),
+                 METHOD_SEL_NAME (existing_method));
+       }
+
+      /* Now check the list of @optional methods if we didn't find the
+        method in the @required list.  */
+      if (!existing_method)
+       {
+         if (is_class)
+           existing_method = lookup_method  
(PROTOCOL_OPTIONAL_CLS_METHODS (klass), method);
+         else
+           existing_method = lookup_method  
(PROTOCOL_OPTIONAL_NST_METHODS (klass), method);
+
+         if (!is_optional && existing_method)
+           {
+             error ("method %<%c%E%> declared %<@optional%> and  
%<@required%> at the same time",
+                    (is_class ? '+' : '-'),
+                    METHOD_SEL_NAME (existing_method));
+             inform (DECL_SOURCE_LOCATION (existing_method),
+                     "previous declaration of %<%c%E%> as %<@optional 
%>",
+                     (is_class ? '+' : '-'),
+                     METHOD_SEL_NAME (existing_method));
+           }
+       }
+    }
+
+  /* If the method didn't exist already, add it.  */
+  if (!existing_method)
+    {
+      if (is_optional)
        {
          if (is_class)
            {
+             /* Put the method on the list in reverse order.  */
              TREE_CHAIN (method) = PROTOCOL_OPTIONAL_CLS_METHODS  
(klass);
              PROTOCOL_OPTIONAL_CLS_METHODS (klass) = method;
            }
@@ -8923,36 +8970,50 @@ objc_add_method (tree klass, tree method
              PROTOCOL_OPTIONAL_NST_METHODS (klass) = method;
            }
        }
-    }
-  else if (!(mth = lookup_method (is_class
-                            ? CLASS_CLS_METHODS (klass)
-                            : CLASS_NST_METHODS (klass), method)))
-    {
-      /* put method on list in reverse order */
-      if (is_class)
-       {
-         DECL_CHAIN (method) = CLASS_CLS_METHODS (klass);
-         CLASS_CLS_METHODS (klass) = method;
-       }
       else
        {
-         DECL_CHAIN (method) = CLASS_NST_METHODS (klass);
-         CLASS_NST_METHODS (klass) = method;
+         if (is_class)
+           {
+             DECL_CHAIN (method) = CLASS_CLS_METHODS (klass);
+             CLASS_CLS_METHODS (klass) = method;
+           }
+         else
+           {
+             DECL_CHAIN (method) = CLASS_NST_METHODS (klass);
+             CLASS_NST_METHODS (klass) = method;
+           }
        }
     }
   else
     {
-      /* When processing an @interface for a class or category, give  
hard
-        errors on methods with identical selectors but differing  
argument
-        and/or return types. We do not do this for @implementations,  
because
-        C/C++ will do it for us (i.e., there will be duplicate function
-        definition errors).  */
+      /* The method was already defined.  Check that the types match
+        for an @interface for a class or category, or for a
+        @protocol.  Give hard errors on methods with identical
+        selectors but differing argument and/or return types.  We do
+        not do this for @implementations, because C/C++ will do it
+        for us (i.e., there will be duplicate function definition
+        errors).  */
       if ((TREE_CODE (klass) == CLASS_INTERFACE_TYPE
-          || TREE_CODE (klass) == CATEGORY_INTERFACE_TYPE)
-         && !comp_proto_with_proto (method, mth, 1))
-       error ("duplicate declaration of method %<%c%E%>",
-               is_class ? '+' : '-',
-               METHOD_SEL_NAME (mth));
+          || TREE_CODE (klass) == CATEGORY_INTERFACE_TYPE
+          /* Starting with GCC 4.6, we emit the same error for
+             protocols too.  The situation is identical to
+             @interfaces as there is no possible meaningful reason
+             for defining the same method with different signatures
+             in the very same @protocol.  If that was allowed,
+             whenever the protocol is used (both at compile and run
+             time) there wouldn't be any meaningful way to decide
+             which of the two method signatures should be used.  */
+          || TREE_CODE (klass) == PROTOCOL_INTERFACE_TYPE)
+         && !comp_proto_with_proto (method, existing_method, 1))
+       {
+         error ("duplicate declaration of method %<%c%E%> with  
conflicting types",
+                (is_class ? '+' : '-'),
+                METHOD_SEL_NAME (existing_method));
+         inform (DECL_SOURCE_LOCATION (existing_method),
+                 "previous declaration of %<%c%E%>",
+                 (is_class ? '+' : '-'),
+                 METHOD_SEL_NAME (existing_method));
+       }
     }

   if (is_class)
Index: objc/ChangeLog
===================================================================
--- objc/ChangeLog      (revision 168341)
+++ objc/ChangeLog      (working copy)
@@ -1,4 +1,15 @@
-2010-12-30  Nicola Pero  <nicola@nicola.brainstorm.co.uk>
+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
+       of the original method.  Also, improved the error message to
+       clearly state that the conflict is due to conflicting types, and
+       produce it for protocols as well.  Emit an error if two  
identical
+       methods are declared in a protocol, but one is @required and the
+       other one is @optional.  When
+
+2010-12-30  Nicola Pero  <nicola.pero@meta-innovation.com>

        * objc-act.c (start_class): Warn when a class attribute is
        ignored.
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog (revision 168341)
+++ testsuite/ChangeLog (working copy)
@@ -1,3 +1,14 @@
+2010-12-30  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * objc.dg/class-extension-3.m: Updated.
+       * objc.dg/method-1.m: Updated.
+       * objc.dg/method-conflict-1.m: New.
+       * objc.dg/method-conflict-2.m: New.
+       * obj-c++.dg/class-extension-3.mm: Updated.
+       * obj-c++.dg/method-8.mm: Updated.
+       * obj-c++.dg/method-conflict-1.mm: New.
+       * obj-c++.dg/method-conflict-2.mm: New.
+
 2010-12-30  Janus Weil  <janus@gcc.gnu.org>

        PR fortran/47085
Index: testsuite/objc.dg/class-extension-3.m
===================================================================
--- testsuite/objc.dg/class-extension-3.m       (revision 168341)
+++ testsuite/objc.dg/class-extension-3.m       (working copy)
@@ -10,7 +10,7 @@
   Class isa;
   int count;
 }
-- (int) test;
+- (int) test;        /* { dg-message "previous declaration" } */
 @property int count; /* { dg-message "originally specified here" } */
 @end

Index: testsuite/objc.dg/method-conflict-1.m
===================================================================
--- testsuite/objc.dg/method-conflict-1.m       (revision 0)
+++ testsuite/objc.dg/method-conflict-1.m       (revision 0)
@@ -0,0 +1,26 @@
+/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
December 2010.  */
+/* { dg-do compile } */
+
+#include <objc/objc.h>
+
+/* Test that you can not declare two methods, in the same protocol,
+   with the same name but conflicting method signatures.  */
+
+@protocol MyProtocol
++ (int) method1: (int)x;   /* { dg-message "previous declaration" } */
++ (float) method1: (int)x; /* { dg-error "duplicate declaration of  
method .\\+method1." } */
+
+- (int) method2: (int)x;   /* { dg-message "previous declaration" } */
+- (int) method2: (float)x; /* { dg-error "duplicate declaration of  
method .\\-method2." } */
+
+@optional
++ (int *) method3: (int)x;    /* { dg-message "previous  
declaration" } */
++ (int *) method3: (int **)x; /* { dg-error "duplicate declaration of  
method .\\+method3." } */
+
+- (id) method4: (id)x;   /* { dg-message "previous declaration" } */
+- (void) method4: (id)x; /* { dg-error "duplicate declaration of  
method .\\-method4." } */
+@end
+
+/* We don't test conflicting types between @required and @optional
+   methods, as that is tested in method-conflict-2.  */
+
Index: testsuite/objc.dg/method-1.m
===================================================================
--- testsuite/objc.dg/method-1.m        (revision 168341)
+++ testsuite/objc.dg/method-1.m        (working copy)
@@ -2,12 +2,12 @@
 /* { dg-do compile } */

 @interface class1
-- (int) meth1;
+- (int) meth1;   /* { dg-message "previous declaration" } */
 - (void) meth1;  /* { dg-error "duplicate declaration of method .\\- 
meth1." } */
 @end

 @interface class2
-+ (void) meth1;
++ (void) meth1; /* { dg-message "previous declaration" } */
 + (int) meth1;  /* { dg-error "duplicate declaration of method .\\ 
+meth1." } */
 @end
Index: testsuite/objc.dg/method-conflict-2.m
===================================================================
--- testsuite/objc.dg/method-conflict-2.m       (revision 0)
+++ testsuite/objc.dg/method-conflict-2.m       (revision 0)
@@ -0,0 +1,34 @@
+/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
December 2010.  */
+/* { dg-do compile } */
+
+#include <objc/objc.h>
+
+/* Test that you can not declare two methods, in the same protocol,
+   with the same name and method signature, but one as @required and
+   once as @optional.  */
+
+/* First, @required conflicting with @optional.  */
+@protocol MyProtocol
+
+@optional
++ (void) method1: (id)x; /* { dg-message "previous declaration" } */
+- (id) method2: (long)x; /* { dg-message "previous declaration" } */
+
+@required
++ (void) method1: (id)x; /* { dg-error "declared .@optional.  
and .@required. at the same time" } */
+- (id) method2: (long)x; /* { dg-error "declared .@optional.  
and .@required. at the same time" } */
+
+@end
+
+/* Second, @optional conflicting with @required.  */
+@protocol MyProtocol2
+
+@required
++ (void) method3: (Class)x; /* { dg-message "previous declaration" } */
+- (id *) method4: (long)x;  /* { dg-message "previous declaration" } */
+
+@optional
++ (void) method3: (Class)x; /* { dg-error "declared .@optional.  
and .@required. at the same time" } */
+- (id *) method4: (long)x;  /* { dg-error "declared .@optional.  
and .@required. at the same time" } */
+
+@end
Index: testsuite/obj-c++.dg/method-8.mm
===================================================================
--- testsuite/obj-c++.dg/method-8.mm    (revision 168341)
+++ testsuite/obj-c++.dg/method-8.mm    (working copy)
@@ -2,12 +2,12 @@
 /* { dg-do compile } */

 @interface class1
-- (int) meth1;
+- (int) meth1;   /* { dg-error "previous declaration" } */
 - (void) meth1;  /* { dg-error "duplicate declaration of method .\\- 
meth1." } */
 @end

 @interface class2
-+ (void) meth1;
++ (void) meth1; /* { dg-error "previous declaration" } */
 + (int) meth1;  /* { dg-error "duplicate declaration of method .\\ 
+meth1." } */
 @end
 Index: testsuite/obj-c++.dg/method-conflict-1.mm
===================================================================
--- testsuite/obj-c++.dg/method-conflict-1.mm   (revision 0)
+++ testsuite/obj-c++.dg/method-conflict-1.mm   (revision 0)
@@ -0,0 +1,26 @@
+/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
December 2010.  */
+/* { dg-do compile } */
+
+#include <objc/objc.h>
+
+/* Test that you can not declare two methods, in the same protocol,
+   with the same name but conflicting method signatures.  */
+
+@protocol MyProtocol
++ (int) method1: (int)x;   /* { dg-error "previous declaration" } */
++ (float) method1: (int)x; /* { dg-error "duplicate declaration of  
method .\\+method1." } */
+
+- (int) method2: (int)x;   /* { dg-error "previous declaration" } */
+- (int) method2: (float)x; /* { dg-error "duplicate declaration of  
method .\\-method2." } */
+
+@optional
++ (int *) method3: (int)x;    /* { dg-error "previous declaration" } */
++ (int *) method3: (int **)x; /* { dg-error "duplicate declaration of  
method .\\+method3." } */
+
+- (id) method4: (id)x;   /* { dg-error "previous declaration" } */
+- (void) method4: (id)x; /* { dg-error "duplicate declaration of  
method .\\-method4." } */
+@end
+
+/* We don't test conflicting types between @required and @optional
+   methods, as that is tested in method-conflict-2.  */
+
Index: testsuite/obj-c++.dg/class-extension-3.mm
===================================================================
--- testsuite/obj-c++.dg/class-extension-3.mm   (revision 168341)
+++ testsuite/obj-c++.dg/class-extension-3.mm   (working copy)
@@ -10,7 +10,7 @@
   Class isa;
   int count;
 }
-- (int) test;
+- (int) test;        /* { dg-warning "previous declaration" } */
 @property int count; /* { dg-warning "originally specified here" } */
 @end
 Index: testsuite/obj-c++.dg/method-conflict-2.mm
===================================================================
--- testsuite/obj-c++.dg/method-conflict-2.mm   (revision 0)
+++ testsuite/obj-c++.dg/method-conflict-2.mm   (revision 0)
@@ -0,0 +1,34 @@
+/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
December 2010.  */
+/* { dg-do compile } */
+
+#include <objc/objc.h>
+
+/* Test that you can not declare two methods, in the same protocol,
+   with the same name and method signature, but one as @required and
+   once as @optional.  */
+
+/* First, @required conflicting with @optional.  */
+@protocol MyProtocol
+
+@optional
++ (void) method1: (id)x; /* { dg-error "previous declaration" } */
+- (id) method2: (long)x; /* { dg-error "previous declaration" } */
+
+@required
++ (void) method1: (id)x; /* { dg-error "declared .@optional.  
and .@required. at the same time" } */
+- (id) method2: (long)x; /* { dg-error "declared .@optional.  
and .@required. at the same time" } */
+
+@end
+
+/* Second, @optional conflicting with @required.  */
+@protocol MyProtocol2
+
+@required
++ (void) method3: (Class)x; /* { dg-error "previous declaration" } */
+- (id *) method4: (long)x;  /* { dg-error "previous declaration" } */
+
+@optional
++ (void) method3: (Class)x; /* { dg-error "declared .@optional.  
and .@required. at the same time" } */
+- (id *) method4: (long)x;  /* { dg-error "declared .@optional.  
and .@required. at the same time" } */
+
+@end