Re: preliminary patch for objc_comptypes() fixes

[Stan Shebs <shebs at apple dot com>]

Nicola Pero wrote:

> > Then, I modified the compiler to work properly in the testcase - after
> > patching it, it emits all warnings right in the testcase at least.  The
> > objc testsuite still passes ok, but I would need to recheck the code and
> > try the compiler on something more serious.
>  
> I tried the compiler on something more serious, and I think I exaggerated
> with the warnings :-)
>
> It should be possible to use anything which is of type 'id' in any
> assignment, comparison etc without getting any warnings.
>   
> Otherwise standard stuff like NSObject *o = [[NSObject alloc] init]; would
> generate a warning.
>  
> It should also be possible to compare an object of a class implementing a
> protocol with a generic object implementing that protocol, without
> warnings.
>     
> So here is an amended testcase, and an amended objc_comptypes() function.
> The differences with GCC 2.95.4 are now only in some cases involving
> protocols (which are inherently more rare and complex), except one or two
> cases which have already been reported as bugs.
>  
> It still needs work, but it looks it's getting better. :-)
>
> Comments are appreciated.
This all looks pretty reasonable to me.  It overlaps with Zem's patch,
so you'll need to merge with that.  If this clears up a bunch of
spurious GNUstep and/or Swarm warnings (for instance), then this
could qualify as a bug fix for 3.3, and we should get it in sooner
rather than later so it gets plenty of exercise (for instance on
Apple's ObjC sources after the next import, heh-heh).

Stan

> testsuite/objc.dg/comp-types-1.m
>
> /* Test various ObjC types assignments and comparisons.  */
> /* Author: Nicola Pero <nicola@brainstorm.co.uk>.  */
> /* { dg-do compile } */
> #include <objc/objc.h>
>
> @protocol MyProtocol
> - (void) foo;
> @end
>
> @interface MyClass
> @end
>
> @interface MyOtherClass <MyProtocol>
> - (void) foo;
> @end
>
> int main()
> {
>  id obj = nil;
>  id<MyProtocol> obj_p = nil;
>  MyClass *obj_c = nil;
>  MyOtherClass *obj_cp = nil;
>
>  /* Assigning to an 'id' variable should never
>     generate a warning.  */
>
>  obj = obj_p;  /* Ok  */
>  obj = obj_c;  /* Ok  */
>  obj = obj_cp; /* Ok  */
>  
>  /* Assigning to a 'MyClass *' variable should always generate a
>     warning, unless done from an 'id'.  */
>
>  obj_c = obj;    /* Ok */
>  obj_c = obj_p;  /* { dg-warning "incompatible pointer type" } */ /*Missing in 2.95.4*/
>  obj_c = obj_cp; /* { dg-warning "incompatible pointer type" } */ /*Missing in 2.95.4*/
>
>  /* Assigning to an 'id<MyProtocol>' variable should generate a
>     warning if done from a 'MyClass *' (which doesn't implement
>     MyProtocol), but not from an 'id'or from a 'MyOtherClass *'
>     (which implements MyProtocol).  */
>
>  obj_p = obj;    /* Ok */
>  obj_p = obj_c;  /* { dg-warning "does not implement" } */ /*Duplicated in 2.95.4*/
>  obj_p = obj_cp; /* Ok  */
>
>  /* Assigning to a 'MyOtherClass *' variable should always generate
>     a warning, unless done from an 'id'  */
>  obj_cp = obj;    /* Ok */
>  obj_cp = obj_c;  /* { dg-warning "incompatible pointer type" } */ /*Missing in 2.95.4, fixed in 3.3*/
>  obj_cp = obj_p;  /* { dg-warning "incompatible pointer type" } */ /*Missing in 2.95.4*/
>
>  /* Any comparison involving an 'id' must be without warnings.  */
>  if (obj == obj_p) ;  /* Ok  */ /*Bogus warning here in 2.95.4*/
>  if (obj_p == obj) ;  /* Ok  */
>  if (obj == obj_c) ;  /* Ok  */
>  if (obj_c == obj) ;  /* Ok  */
>  if (obj == obj_cp) ; /* Ok  */
>  if (obj_cp == obj) ; /* Ok  */
>
>  /* Any comparison between 'MyClass *' and anything which is not an 'id'
>     must generate a warning.  */
>  if (obj_c == obj_p) ; /* { dg-warning "does not implement" } */ /*Missing in 2.95.4*/
>  if (obj_p == obj_c) ; /* { dg-warning "does not implement" } */
>  if (obj_c == obj_cp) ; /* { dg-warning "lacks a cast" } */
>  if (obj_cp == obj_c) ; /* { dg-warning "lacks a cast" } */
>
>  /* Any comparison between 'MyOtherClass *' (which implements
>     MyProtocol) and an 'id' implementing MyProtocol are Ok.  */
>  if (obj_cp == obj_p) ; /* Ok */
>  if (obj_p == obj_cp) ; /* Ok */
>
>  return 0;
> }
>
> Index: c-typeck.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/c-typeck.c,v
> retrieving revision 1.203
> diff -u -r1.203 c-typeck.c
> --- c-typeck.c	15 Aug 2002 21:16:23 -0000	1.203
> +++ c-typeck.c	1 Sep 2002 18:08:48 -0000
> @@ -51,7 +51,7 @@
> static int undeclared_variable_notice;
>
> static tree qualify_type		PARAMS ((tree, tree));
> -static int comp_target_types		PARAMS ((tree, tree));
> +static int comp_target_types		PARAMS ((tree, tree, int));
> static int function_types_compatible_p	PARAMS ((tree, tree));
> static int type_lists_compatible_p	PARAMS ((tree, tree));
> static tree decl_constant_value_for_broken_optimization PARAMS ((tree));
> @@ -579,16 +579,21 @@
> }
>
> /* Return 1 if TTL and TTR are pointers to types that are equivalent,
> -   ignoring their qualifiers.  */
> +   ignoring their qualifiers.  REFLEXIVE is only used by ObjC - set it
> +   to 1 or 0 depending if the check of the pointer types is meant to
> +   be reflexive or not (typically, assignments are not reflexive,
> +   while comparisons are reflexive).
> +*/
>
> static int
> -comp_target_types (ttl, ttr)
> +comp_target_types (ttl, ttr, reflexive)
>      tree ttl, ttr;
> +     int reflexive;
> {
>   int val;
>
>   /* Give maybe_objc_comptypes a crack at letting these types through.  */
> -  if ((val = objc_comptypes (ttl, ttr, 1)) >= 0)
> +  if ((val = objc_comptypes (ttl, ttr, reflexive)) >= 0)
>     return val;
>
>   val = comptypes (TYPE_MAIN_VARIANT (TREE_TYPE (ttl)),
> @@ -1958,7 +1963,7 @@
>       /* Subtraction of two similar pointers.
> 	 We must subtract them as integers, then divide by object size.  */
>       if (code0 == POINTER_TYPE && code1 == POINTER_TYPE
> -	  && comp_target_types (type0, type1))
> +	  && comp_target_types (type0, type1, 1))
> 	return pointer_diff (op0, op1);
>       /* Handle pointer minus int.  Just like pointer plus int.  */
>       else if (code0 == POINTER_TYPE && code1 == INTEGER_TYPE)
> @@ -2148,7 +2153,7 @@
> 	  /* Anything compares with void *.  void * compares with anything.
> 	     Otherwise, the targets must be compatible
> 	     and both must be object or both incomplete.  */
> -	  if (comp_target_types (type0, type1))
> +	  if (comp_target_types (type0, type1, 1))
> 	    result_type = common_type (type0, type1);
> 	  else if (VOID_TYPE_P (tt0))
> 	    {
> @@ -2195,7 +2200,7 @@
> 	shorten = 1;
>       else if (code0 == POINTER_TYPE && code1 == POINTER_TYPE)
> 	{
> -	  if (comp_target_types (type0, type1))
> +	  if (comp_target_types (type0, type1, 1))
> 	    {
> 	      result_type = common_type (type0, type1);
> 	      if (pedantic 
> @@ -2220,7 +2225,7 @@
> 	short_compare = 1;
>       else if (code0 == POINTER_TYPE && code1 == POINTER_TYPE)
> 	{
> -	  if (comp_target_types (type0, type1))
> +	  if (comp_target_types (type0, type1, 1))
> 	    {
> 	      result_type = common_type (type0, type1);
> 	      if (!COMPLETE_TYPE_P (TREE_TYPE (type0))
> @@ -3422,7 +3427,7 @@
>     }
>   else if (code1 == POINTER_TYPE && code2 == POINTER_TYPE)
>     {
> -      if (comp_target_types (type1, type2))
> +      if (comp_target_types (type1, type2, 1))
> 	result_type = common_type (type1, type2);
>       else if (integer_zerop (op1) && TREE_TYPE (type1) == void_type_node
> 	       && TREE_CODE (orig_op1) != NOP_EXPR)
> @@ -3984,13 +3989,12 @@
>   if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT (rhstype))
>     {
>       overflow_warning (rhs);
> -      /* Check for Objective-C protocols.  This will issue a warning if
> -	 there are protocol violations.  No need to use the return value.  */
> +      /* Check for Objective-C protocols.  This will automatically
> +	 issue a warning if there are protocol violations.  */
>       if (flag_objc)
> 	objc_comptypes (type, rhstype, 0);
>       return rhs;
>     }
> -
>   if (coder == VOID_TYPE)
>     {
>       error ("void value not ignored as it ought to be");
> @@ -4060,7 +4064,7 @@
> 		 Meanwhile, the lhs target must have all the qualifiers of
> 		 the rhs.  */
> 	      if (VOID_TYPE_P (ttl) || VOID_TYPE_P (ttr)
> -		  || comp_target_types (memb_type, rhstype))
> +		  || comp_target_types (memb_type, rhstype, 0))
> 		{
> 		  /* If this type won't generate any warnings, use it.  */
> 		  if (TYPE_QUALS (ttl) == TYPE_QUALS (ttr)
> @@ -4135,7 +4139,7 @@
> 	 and vice versa; otherwise, targets must be the same.
> 	 Meanwhile, the lhs target must have all the qualifiers of the rhs.  */
>       if (VOID_TYPE_P (ttl) || VOID_TYPE_P (ttr)
> -	  || comp_target_types (type, rhstype)
> +	  || comp_target_types (type, rhstype, 0)
> 	  || (c_common_unsigned_type (TYPE_MAIN_VARIANT (ttl))
> 	      == c_common_unsigned_type (TYPE_MAIN_VARIANT (ttr))))
> 	{
> @@ -4160,7 +4164,7 @@
> 	      /* If this is not a case of ignoring a mismatch in signedness,
> 		 no warning.  */
> 	      else if (VOID_TYPE_P (ttl) || VOID_TYPE_P (ttr)
> -		       || comp_target_types (type, rhstype))
> +		       || comp_target_types (type, rhstype, 0))
> 		;
> 	      /* If there is a mismatch, do warn.  */
> 	      else if (pedantic)
>
> /* The new objc-act.c/@objc_comptypes */
>
> /* Return 1 if LHS and RHS are compatible types for assignment or
>   various other operations.  Return 0 if they are incompatible, and
>   return -1 if we choose to not decide (because the types are really
>   just C types, not ObjC specific ones).  When the operation is
>   REFLEXIVE (typically comparisons), check for compatibility in
>   either direction; when it's not (typically assignments), dont'.
>
>   This function is called in two cases: when both lhs and rhs are
>   pointers to records (in which case we check protocols too), and
>   when both lhs and rhs are records (in which case we check class
>   inheritance only).
>
>   Warnings about classes/protocols not implementing a protocol are
>   emitted here (multiple of those warnings might be emitted for a
>   single line!); generic warnings about incompatible assignments and
>   lacks of casts in comparisons are/must be emitted by the caller if
>   we return 0.
> */
>
> int
> objc_comptypes (lhs, rhs, reflexive)
>     tree lhs;
>     tree rhs;
>     int reflexive;
> {
>  /* New clause for protocols.  */
>
>  /* Here we manage the case of a POINTER_TYPE = POINTER_TYPE.  We only
>     manage the ObjC ones, and leave the rest to the C code.  */
>  if (TREE_CODE (lhs) == POINTER_TYPE
>      && TREE_CODE (TREE_TYPE (lhs)) == RECORD_TYPE
>      && TREE_CODE (rhs) == POINTER_TYPE
>      && TREE_CODE (TREE_TYPE (rhs)) == RECORD_TYPE)
>    {
>      int lhs_is_proto = IS_PROTOCOL_QUALIFIED_ID (lhs);
>      int rhs_is_proto = IS_PROTOCOL_QUALIFIED_ID (rhs);
>
>      if (lhs_is_proto)
>        {
> 	  tree lproto, lproto_list = TYPE_PROTOCOL_LIST (lhs);
> 	  tree rproto, rproto_list;
> 	  tree p;
>
> 	  /* <Protocol> = <Protocol>  */
> 	  if (rhs_is_proto)
> 	    {
> 	      rproto_list = TYPE_PROTOCOL_LIST (rhs);
> 	      
> 	      if (!reflexive)
> 		{
> 		  /* An assignment between objects of type 'id
> 		     <Protocol>'; make sure the protocol on the lhs is
> 		     supported by the object on the rhs.  */
> 		  for (lproto = lproto_list; lproto; 
> 		       lproto = TREE_CHAIN (lproto))
> 		    {
> 		      p = TREE_VALUE (lproto);
> 		      rproto = lookup_protocol_in_reflist (rproto_list, p);
> 		      
> 		      if (!rproto)
> 			warning 
> 			  ("object does not conform to the `%s' protocol",
> 			   IDENTIFIER_POINTER (PROTOCOL_NAME (p)));
> 		    }
> 		  return 1;
> 		}
> 	      else
> 		{
> 		  /* Obscure case - a comparison between two objects
> 		     of type 'id <Protocol>'.  Check that either the
> 		     protocol on the lhs is supported by the object on
> 		     the rhs, or viceversa.  */
> 		  
> 		  /* Check if the protocol on the lhs is supported by the
> 		     object on the rhs.  */
> 		  for (lproto = lproto_list; lproto; 
> 		       lproto = TREE_CHAIN (lproto))
> 		    {
> 		      p = TREE_VALUE (lproto);
> 		      rproto = lookup_protocol_in_reflist (rproto_list, p);
> 		      
> 		      if (!rproto)
> 			{
> 			  /* Check failed - check if the protocol on the rhs
> 			     is supported by the object on the lhs.  */
> 			  for (rproto = rproto_list; rproto; 
> 			       rproto = TREE_CHAIN (rproto))
> 			    {
> 			      p = TREE_VALUE (rproto);
> 			      lproto = lookup_protocol_in_reflist (lproto_list,
> 								   p);
> 			      
> 			      if (!lproto)
> 				{
> 				  /* This check failed too: incompatible  */
> 				  return 0;
> 				}
> 			    }
> 			  return 1;
> 			}
> 		    }
> 		  return 1;
> 		}
> 	    }
> 	  /* <Protocol> = <class> *  */
> 	  else if (TYPED_OBJECT (TREE_TYPE (rhs)))
> 	    {
> 	      tree rname = TYPE_NAME (TREE_TYPE (rhs));
> 	      tree rinter;
> 	      
> 	      /* Make sure the protocol is supported by the object on
> 		 the rhs.  */
> 	      for (lproto = lproto_list; lproto; lproto = TREE_CHAIN (lproto))
> 		{
> 		  p = TREE_VALUE (lproto);
> 		  rproto = 0;
> 		  rinter = lookup_interface (rname);
> 		  
> 		  while (rinter && !rproto)
> 		    {
> 		      tree cat;
> 		      
> 		      rproto_list = CLASS_PROTOCOL_LIST (rinter);
> 		      /* If the underlying ObjC class does not have
> 			 protocols attached to it, perhaps there are
> 			 "one-off" protocols attached to the rhs?
> 			 E.g., 'id<MyProt> foo;'.  */
> 		      if (!rproto_list)
> 			rproto_list = TYPE_PROTOCOL_LIST (TREE_TYPE (rhs));
> 		      rproto = lookup_protocol_in_reflist (rproto_list, p);
> 		      
> 		      /* Check for protocols adopted by categories.  */
> 		      cat = CLASS_CATEGORY_LIST (rinter);
> 		      while (cat && !rproto)
> 			{
> 			  rproto_list = CLASS_PROTOCOL_LIST (cat);
> 			  rproto = lookup_protocol_in_reflist (rproto_list, p);
> 			  cat = CLASS_CATEGORY_LIST (cat);
> 			}
> 		      
> 		      rinter = lookup_interface (CLASS_SUPER_NAME (rinter));
> 		    }
> 		  
> 		  if (!rproto)
> 		    warning ("class `%s' does not implement the `%s' protocol",
> 			     IDENTIFIER_POINTER (TYPE_NAME (TREE_TYPE (rhs))),
> 			     IDENTIFIER_POINTER (PROTOCOL_NAME (p)));
> 		}
> 	      return 1;
> 	    }
> 	  /* <Protocol> = id */
> 	  else if (TYPE_NAME (TREE_TYPE (rhs)) == objc_object_id)
> 	    {
> 	      return 1;
> 	    }
> 	  /* <Protocol> = Class */
> 	  else if (TYPE_NAME (TREE_TYPE (rhs)) == objc_class_id)
> 	    {
> 	      return 0;
> 	    }
> 	  /* <Protocol> = ?? : let comptypes decide.  */
>          return -1;
>        }
>      else if (rhs_is_proto)
> 	{
> 	  /* <class> * = <Protocol> */
> 	  if (TYPED_OBJECT (TREE_TYPE (lhs)))
> 	    {
> 	      if (reflexive)
> 		{
> 		  tree rname = TYPE_NAME (TREE_TYPE (lhs));
> 		  tree rinter;
> 		  tree rproto, rproto_list = TYPE_PROTOCOL_LIST (rhs);
> 		  
> 		  /* Make sure the protocol is supported by the object on
> 		     the lhs.  */
> 		  for (rproto = rproto_list; rproto; 
> 		       rproto = TREE_CHAIN (rproto))
> 		    {
> 		      tree p = TREE_VALUE (rproto);
> 		      tree lproto = 0;
> 		      rinter = lookup_interface (rname);
> 		  
> 		      while (rinter && !lproto)
> 			{
> 			  tree cat;
> 			  
> 			  tree lproto_list = CLASS_PROTOCOL_LIST (rinter);
> 			  /* If the underlying ObjC class does not have
> 			     protocols attached to it, perhaps there are
> 			     "one-off" protocols attached to the rhs?
> 			     E.g., 'id<MyProt> foo;'.  */
> 			  if (!lproto_list)
> 			    lproto_list = TYPE_PROTOCOL_LIST (TREE_TYPE (lhs));
> 			  lproto = lookup_protocol_in_reflist (lproto_list, p);
> 			  
> 			  /* Check for protocols adopted by categories.  */
> 			  cat = CLASS_CATEGORY_LIST (rinter);
> 			  while (cat && !lproto)
> 			    {
> 			      lproto_list = CLASS_PROTOCOL_LIST (cat);
> 			      lproto = lookup_protocol_in_reflist (lproto_list,
> 								   p);
> 			      cat = CLASS_CATEGORY_LIST (cat);
> 			    }
> 			  
> 			  rinter = lookup_interface (CLASS_SUPER_NAME 
> 						     (rinter));
> 			}
> 		      
> 		      if (!lproto)
> 			warning ("class `%s' does not implement the `%s' protocol",
> 				 IDENTIFIER_POINTER (TYPE_NAME 
> 						     (TREE_TYPE (lhs))),
> 				 IDENTIFIER_POINTER (PROTOCOL_NAME (p)));
> 		    }
> 		  return 1;
> 		}
> 	      else
> 		return 0;
> 	    }
> 	  /* id = <Protocol> */
> 	  else if (TYPE_NAME (TREE_TYPE (lhs)) == objc_object_id)
> 	    {
> 	      return 1;
> 	    }
> 	  /* Class = <Protocol> */
> 	  else if (TYPE_NAME (TREE_TYPE (lhs)) == objc_class_id)
> 	    {
> 	      return 0;
> 	    }
> 	  /* ??? = <Protocol> : let comptypes decide */
> 	  else
> 	    {
> 	      return -1;
> 	    }
> 	}
>      else
> 	{
> 	  /* Attention: we shouldn't defer to comptypes here.  One bad
> 	     side effect would be that we might loose the REFLEXIVE
> 	     information.
> 	  */
> 	  lhs = TREE_TYPE (lhs);
> 	  rhs = TREE_TYPE (rhs);
> 	}
>    }
>
>  if (TREE_CODE (lhs) != RECORD_TYPE || TREE_CODE (rhs) != RECORD_TYPE)
>    {
>      /* Nothing to do with ObjC - let immediately comptypes take
> 	 responsibility for checking.  */
>      return -1;
>    }
>
>  /* `id' = `<class> *' `<class> *' = `id': always allow it.
>     Please note that 
>     'Object *o = [[Object alloc] init]; falls
>     in the case <class> * = `id'.
>  */
>  if ((TYPE_NAME (lhs) == objc_object_id && TYPED_OBJECT (rhs))
>      || (TYPE_NAME (rhs) == objc_object_id && TYPED_OBJECT (lhs)))
>    return 1;
>
>  /* `id' = `Class', `Class' = `id' */
>
>  else if ((TYPE_NAME (lhs) == objc_object_id
> 	    && TYPE_NAME (rhs) == objc_class_id)
> 	   || (TYPE_NAME (lhs) == objc_class_id
> 	       && TYPE_NAME (rhs) == objc_object_id))
>    return 1;
>
>  /* `<class> *' = `<class> *' */
>
>  else if (TYPED_OBJECT (lhs) && TYPED_OBJECT (rhs))
>    {
>      tree lname = TYPE_NAME (lhs);
>      tree rname = TYPE_NAME (rhs);
>      tree inter;
>
>      if (lname == rname)
> 	return 1;
>
>      /* If the left hand side is a super class of the right hand side,
> 	 allow it.  */
>      for (inter = lookup_interface (rname); inter;
> 	   inter = lookup_interface (CLASS_SUPER_NAME (inter)))
> 	if (lname == CLASS_SUPER_NAME (inter))
> 	  return 1;
>
>      /* Allow the reverse when reflexive.  */
>      if (reflexive)
> 	for (inter = lookup_interface (lname); inter;
> 	     inter = lookup_interface (CLASS_SUPER_NAME (inter)))
> 	  if (rname == CLASS_SUPER_NAME (inter))
> 	    return 1;
>
>      return 0;
>    }
>  else
>    /* Not an ObjC type - let comptypes do the check.  */
>    return -1;
> }