[ObjC] Ambiguous property synthesis should pick the 'readwrite' property and check for incompatible attributes

[arphaman]

Summary

This patch changes the way ambiguous property synthesis (i.e. when synthesizing a property that's declared in multiple protocols) is performed. Previously, we synthesized the first property that was found. This lead to problems when the property was synthesized in a class that conformed to two protocols that declared that property and a second protocols had a 'readwrite' declaration - the setter was not synthesized so the class didn't really conform to the second protocol and user's code would crash at runtime when they would try to set the property.

This patch ensures that a first readwrite property is selected. This is a semantic change that changes users code in this manner:

@protocol P @property(readonly) int p; @end
@protocol P2 @property(readwrite) id p; @end
@interface I <P2> @end
@implementation I
@syntesize p; // We previously got a warning here, but we've synthesized readonly 'int p' here. Now we synthesize readwrite 'id' p..
@end

I'm not sure if this kind of change is OK. WDYT? I promoted the warning about incompatible types when this kind of readonly/readwrite ambiguity is detected in the @implementation, but the problem is that the @interface will still use int p for lookup I think. In theory this promotion to an error should avoid user code breakage I think though, but let me know if I missed something.

I also extended the ambiguity checker, and now it can detect conflicts among the different property attributes. I decided to use an error diagnostic for this kind of incompatibility. Let me know if you think this should be relaxed in some cases.

I also got rid of ProtocolPropertyMap in favour of a a set + vector because the map's order of iteration is non-deterministic, so I couldn't rely on it to select the readwrite property.

rdar://31579994

Thanks for taking a look,
Alex

[rjmccall]

This all seems reasonable. LGTM.

cfe/trunk/include/clang/AST/DeclObjC.h

public:
 
   ObjCPropertyDecl *
   FindPropertyDeclaration(const IdentifierInfo *PropertyId,
                           ObjCPropertyQueryKind QueryKind) const;
 
   typedef llvm::DenseMap<std::pair<IdentifierInfo*,
                                    unsigned/*isClassProperty*/>,
                          ObjCPropertyDecl*> PropertyMap;
-  
-  typedef llvm::DenseMap<const ObjCProtocolDecl *, ObjCPropertyDecl*>
-            ProtocolPropertyMap;
-  
+  typedef llvm::SmallDenseSet<const ObjCProtocolDecl *, 8> ProtocolPropertySet;
   typedef llvm::SmallVector<ObjCPropertyDecl*, 8> PropertyDeclOrder;
   
   /// This routine collects list of properties to be implemented in the class.
   /// This includes, class's and its conforming protocols' properties.
   /// Note, the superclass's properties are not included in the list.
   virtual void collectPropertiesToImplement(PropertyMap &PM,
                                             PropertyDeclOrder &PO) const {}
 
public:
   /// Retrieves the canonical declaration of this Objective-C protocol.
   ObjCProtocolDecl *getCanonicalDecl() override { return getFirstDecl(); }
   const ObjCProtocolDecl *getCanonicalDecl() const { return getFirstDecl(); }
 
   void collectPropertiesToImplement(PropertyMap &PM,
                                     PropertyDeclOrder &PO) const override;
 
   void collectInheritedProtocolProperties(const ObjCPropertyDecl *Property,
-                                          ProtocolPropertyMap &PM) const;
+                                          ProtocolPropertySet &PS,
+                                          PropertyDeclOrder &PO) const;
 
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == ObjCProtocol; }
 
   friend class ASTReader;
   friend class ASTDeclReader;
   friend class ASTDeclWriter;
 };

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 
 def warn_property_attribute : Warning<
   "'%1' attribute on property %0 does not match the property inherited from %2">,
   InGroup<PropertyAttr>;
 def warn_property_types_are_incompatible : Warning<
   "property type %0 is incompatible with type %1 inherited from %2">,
   InGroup<DiagGroup<"incompatible-property-type">>;
 def warn_protocol_property_mismatch : Warning<
-  "property of type %0 was selected for synthesis">,
+  "property %select{of type %1|with attribute '%1'|without attribute '%1'|with "
+  "getter %1|with setter %1}0 was selected for synthesis">,
   InGroup<DiagGroup<"protocol-property-synthesis-ambiguity">>;
+def err_protocol_property_mismatch: Error<warn_protocol_property_mismatch.Text>;
 def err_undef_interface : Error<"cannot find interface declaration for %0">;
 def err_category_forward_interface : Error<
   "cannot define %select{category|class extension}0 for undefined class %1">;
 def err_class_extension_after_impl : Error<
   "cannot declare class extension for %0 after class implementation">;
 def note_implementation_declared : Note<
   "class implementation is declared here">;
 def note_while_in_implementation : Note<
def err_bad_property_decl : Error<
   "property implementation must have its declaration in interface %0 or one of "
   "its extensions">;
 def err_category_property : Error<
   "property declared in category %0 cannot be implemented in "
   "class implementation">;
 def note_property_declare : Note<
   "property declared here">;
 def note_protocol_property_declare : Note<
-  "it could also be property of type %0 declared here">;
+  "it could also be property "
+  "%select{of type %1|without attribute '%1'|with attribute '%1'|with getter "
+  "%1|with setter %1}0 declared here">;
 def note_property_synthesize : Note<
   "property synthesized here">;
 def err_synthesize_category_decl : Error<
   "@synthesize not allowed in a category's implementation">;
 def err_synthesize_on_class_property : Error<
   "@synthesize not allowed on a class property %0">;
 def err_missing_property_interface : Error<
   "property implementation in a category with no category declaration">;

cfe/trunk/lib/AST/DeclObjC.cpp

for (auto *Prop : PDecl->properties()) {
       PO.push_back(Prop);
     }
     // Scan through protocol's protocols.
     for (const auto *PI : PDecl->protocols())
       PI->collectPropertiesToImplement(PM, PO);
   }
 }
 
-    
 void ObjCProtocolDecl::collectInheritedProtocolProperties(
-                                                const ObjCPropertyDecl *Property,
-                                                ProtocolPropertyMap &PM) const {
+    const ObjCPropertyDecl *Property, ProtocolPropertySet &PS,
+    PropertyDeclOrder &PO) const {
   if (const ObjCProtocolDecl *PDecl = getDefinition()) {
-    bool MatchFound = false;
+    if (!PS.insert(PDecl).second)
+      return;
     for (auto *Prop : PDecl->properties()) {
       if (Prop == Property)
         continue;
       if (Prop->getIdentifier() == Property->getIdentifier()) {
-        PM[PDecl] = Prop;
-        MatchFound = true;
-        break;
+        PO.push_back(Prop);
+        return;
       }
     }
     // Scan through protocol's protocols which did not have a matching property.
-    if (!MatchFound)
-      for (const auto *PI : PDecl->protocols())
-        PI->collectInheritedProtocolProperties(Property, PM);
+      PI->collectInheritedProtocolProperties(Property, PS, PO);
   }
 }
 
 StringRef
 ObjCProtocolDecl::getObjCRuntimeNameAsString() const {
   if (ObjCRuntimeNameAttr *ObjCRTName = getAttr<ObjCRuntimeNameAttr>())
     return ObjCRTName->getMetadataName();
 

cfe/trunk/lib/Sema/SemaObjCProperty.cpp

static void setImpliedPropertyAttributeForReadOnlyProperty(
   QualType ivarType = ivar->getType();
   Qualifiers::ObjCLifetime ivarLifetime = ivarType.getObjCLifetime();
   if (ivarLifetime == Qualifiers::OCL_Strong)
     property->setPropertyAttributes(ObjCPropertyDecl::OBJC_PR_strong);
   else if (ivarLifetime == Qualifiers::OCL_Weak)
     property->setPropertyAttributes(ObjCPropertyDecl::OBJC_PR_weak);
 }
 
-/// DiagnosePropertyMismatchDeclInProtocols - diagnose properties declared
-/// in inherited protocols with mismatched types. Since any of them can
-/// be candidate for synthesis.
-static void
-DiagnosePropertyMismatchDeclInProtocols(Sema &S, SourceLocation AtLoc,
+static bool
+isIncompatiblePropertyAttribute(unsigned Attr1, unsigned Attr2,
+                                ObjCPropertyDecl::PropertyAttributeKind Kind) {
+  return (Attr1 & Kind) != (Attr2 & Kind);
+}
+
+static bool areIncompatiblePropertyAttributes(unsigned Attr1, unsigned Attr2,
+                                              unsigned Kinds) {
+  return ((Attr1 & Kinds) != 0) != ((Attr2 & Kinds) != 0);
+}
+
+/// SelectPropertyForSynthesisFromProtocols - Finds the most appropriate
+/// property declaration that should be synthesised in all of the inherited
+/// protocols. It also diagnoses properties declared in inherited protocols with
+/// mismatched types or attributes, since any of them can be candidate for
+/// synthesis.
+static ObjCPropertyDecl *
+SelectPropertyForSynthesisFromProtocols(Sema &S, SourceLocation AtLoc,
                                         ObjCInterfaceDecl *ClassDecl,
                                         ObjCPropertyDecl *Property) {
-  ObjCInterfaceDecl::ProtocolPropertyMap PropMap;
+  assert(isa<ObjCProtocolDecl>(Property->getDeclContext()) &&
+         "Expected a property from a protocol");
+  ObjCInterfaceDecl::ProtocolPropertySet ProtocolSet;
+  ObjCInterfaceDecl::PropertyDeclOrder Properties;
   for (const auto *PI : ClassDecl->all_referenced_protocols()) {
     if (const ObjCProtocolDecl *PDecl = PI->getDefinition())
-      PDecl->collectInheritedProtocolProperties(Property, PropMap);
+      PDecl->collectInheritedProtocolProperties(Property, ProtocolSet,
+                                                Properties);
   }
-  if (ObjCInterfaceDecl *SDecl = ClassDecl->getSuperClass())
+  if (ObjCInterfaceDecl *SDecl = ClassDecl->getSuperClass()) {
     while (SDecl) {
       for (const auto *PI : SDecl->all_referenced_protocols()) {
         if (const ObjCProtocolDecl *PDecl = PI->getDefinition())
-          PDecl->collectInheritedProtocolProperties(Property, PropMap);
+          PDecl->collectInheritedProtocolProperties(Property, ProtocolSet,
+                                                    Properties);
       }
       SDecl = SDecl->getSuperClass();
     }
-  
-  if (PropMap.empty())
-    return;
-  
+  }
+  if (Properties.empty())
+    return Property;
+
+  ObjCPropertyDecl *OriginalProperty = Property;
+  size_t SelectedIndex = 0;
+  for (const auto &Prop : llvm::enumerate(Properties)) {
+    // Select the 'readwrite' property if such property exists.
+    if (Property->isReadOnly() && !Prop.value()->isReadOnly()) {
+      Property = Prop.value();
+      SelectedIndex = Prop.index();
+    }
+  }
+  if (Property != OriginalProperty) {
+    // Check that the old property is compatible with the new one.
+    Properties[SelectedIndex] = OriginalProperty;
+  }
   QualType RHSType = S.Context.getCanonicalType(Property->getType());
-  bool FirsTime = true;
-  for (ObjCInterfaceDecl::ProtocolPropertyMap::iterator
-       I = PropMap.begin(), E = PropMap.end(); I != E; I++) {
-    ObjCPropertyDecl *Prop = I->second;
+  unsigned OriginalAttributes = Property->getPropertyAttributes();
+  enum MismatchKind {
+    IncompatibleType = 0,
+    HasNoExpectedAttribute,
+    HasUnexpectedAttribute,
+    DifferentGetter,
+    DifferentSetter
+  };
+  // Represents a property from another protocol that conflicts with the
+  // selected declaration.
+  struct MismatchingProperty {
+    const ObjCPropertyDecl *Prop;
+    MismatchKind Kind;
+    StringRef AttributeName;
+  };
+  SmallVector<MismatchingProperty, 4> Mismatches;
+  for (ObjCPropertyDecl *Prop : Properties) {
+    // Verify the property attributes.
+    unsigned Attr = Prop->getPropertyAttributes();
+    if (Attr != OriginalAttributes) {
+      auto Diag = [&](bool OriginalHasAttribute, StringRef AttributeName) {
+        MismatchKind Kind = OriginalHasAttribute ? HasNoExpectedAttribute
+                                                 : HasUnexpectedAttribute;
+        Mismatches.push_back({Prop, Kind, AttributeName});
+      };
+      if (isIncompatiblePropertyAttribute(OriginalAttributes, Attr,
+                                          ObjCPropertyDecl::OBJC_PR_copy)) {
+        Diag(OriginalAttributes & ObjCPropertyDecl::OBJC_PR_copy, "copy");
+        continue;
+      }
+      if (areIncompatiblePropertyAttributes(
+              OriginalAttributes, Attr, ObjCPropertyDecl::OBJC_PR_retain |
+                                            ObjCPropertyDecl::OBJC_PR_strong)) {
+        Diag(OriginalAttributes & (ObjCPropertyDecl::OBJC_PR_retain |
+                                   ObjCPropertyDecl::OBJC_PR_strong),
+             "retain (or strong)");
+        continue;
+      }
+      if (isIncompatiblePropertyAttribute(OriginalAttributes, Attr,
+                                          ObjCPropertyDecl::OBJC_PR_atomic)) {
+        Diag(OriginalAttributes & ObjCPropertyDecl::OBJC_PR_atomic, "atomic");
+        continue;
+      }
+    }
+    if (Property->getGetterName() != Prop->getGetterName()) {
+      Mismatches.push_back({Prop, DifferentGetter, ""});
+      continue;
+    }
+    if (!Property->isReadOnly() && !Prop->isReadOnly() &&
+        Property->getSetterName() != Prop->getSetterName()) {
+      Mismatches.push_back({Prop, DifferentSetter, ""});
+      continue;
+    }
     QualType LHSType = S.Context.getCanonicalType(Prop->getType());
     if (!S.Context.propertyTypesAreCompatible(LHSType, RHSType)) {
       bool IncompatibleObjC = false;
       QualType ConvertedType;
       if (!S.isObjCPointerConversion(RHSType, LHSType, ConvertedType, IncompatibleObjC)
           || IncompatibleObjC) {
-        if (FirsTime) {
-          S.Diag(Property->getLocation(), diag::warn_protocol_property_mismatch)
-            << Property->getType();
-          FirsTime = false;
-        }
-        S.Diag(Prop->getLocation(), diag::note_protocol_property_declare)
-          << Prop->getType();
-      }
-    }
+        Mismatches.push_back({Prop, IncompatibleType, ""});
+        continue;
+
+  if (Mismatches.empty())
+    return Property;
+
+  // Diagnose incompability.
+  {
+    bool HasIncompatibleAttributes = false;
+    for (const auto &Note : Mismatches)
+      HasIncompatibleAttributes =
+          Note.Kind != IncompatibleType ? true : HasIncompatibleAttributes;
+    // Promote the warning to an error if there are incompatible attributes or
+    // incompatible types together with readwrite/readonly incompatibility.
+    auto Diag = S.Diag(Property->getLocation(),
+                       Property != OriginalProperty || HasIncompatibleAttributes
+                           ? diag::err_protocol_property_mismatch
+                           : diag::warn_protocol_property_mismatch);
+    Diag << Mismatches[0].Kind;
+    switch (Mismatches[0].Kind) {
+    case IncompatibleType:
+      Diag << Property->getType();
+      break;
+    case HasNoExpectedAttribute:
+    case HasUnexpectedAttribute:
+      Diag << Mismatches[0].AttributeName;
+      break;
+    case DifferentGetter:
+      Diag << Property->getGetterName();
+      break;
+    case DifferentSetter:
+      Diag << Property->getSetterName();
+      break;
     }
-  if (!FirsTime && AtLoc.isValid())
+  }
+  for (const auto &Note : Mismatches) {
+    auto Diag =
+        S.Diag(Note.Prop->getLocation(), diag::note_protocol_property_declare)
+        << Note.Kind;
+    switch (Note.Kind) {
+    case IncompatibleType:
+      Diag << Note.Prop->getType();
+      break;
+    case HasNoExpectedAttribute:
+    case HasUnexpectedAttribute:
+      Diag << Note.AttributeName;
+      break;
+    case DifferentGetter:
+      Diag << Note.Prop->getGetterName();
+      break;
+    case DifferentSetter:
+      Diag << Note.Prop->getSetterName();
+      break;
+    }
+  }
+  if (AtLoc.isValid())
     S.Diag(AtLoc, diag::note_property_synthesize);
+
+  return Property;
 }
 
 /// Determine whether any storage attributes were written on the property.
 static bool hasWrittenStorageAttribute(ObjCPropertyDecl *Prop,
                                        ObjCPropertyQueryKind QueryKind) {
   if (Prop->getPropertyAttributesAsWritten() & OwnershipMask) return true;
 
   // If this is a readwrite property in a class extension that refines
if (Synthesize&&
           SourceRange ReadonlySourceRange(readonlyLoc, endLoc);
           Diag(property->getLocation(), 
                diag::note_auto_readonly_iboutlet_fixup_suggest) <<
           FixItHint::CreateReplacement(ReadonlySourceRange, "readwrite");
         }
       }
     }
     if (Synthesize && isa<ObjCProtocolDecl>(property->getDeclContext()))
-      DiagnosePropertyMismatchDeclInProtocols(*this, AtLoc, IDecl, property);
-        
+      property = SelectPropertyForSynthesisFromProtocols(*this, AtLoc, IDecl,
+                                                         property);
   } else if ((CatImplClass = dyn_cast<ObjCCategoryImplDecl>(ClassImpDecl))) {
     if (Synthesize) {
       Diag(AtLoc, diag::err_synthesize_category_decl);
       return nullptr;
     }
     IDecl = CatImplClass->getClassInterface();
     if (!IDecl) {
       Diag(AtLoc, diag::err_missing_property_interface);

cfe/trunk/test/CodeGenObjC/arc-property.m

 }
 // CHECK:    define internal i8* @"\01-[Test3 copyMachine]"(
 // CHECK:      [[T0:%.*]] = call i8* @test3_helper()
 // CHECK-NEXT: [[T1:%.*]] = call i8* @objc_retainAutoreleasedReturnValue(i8* [[T0]])
 // CHECK-NEXT: ret i8* [[T1]]
 - (void) setCopyMachine: (id) x {}
 @end
 
+// rdar://31579994
+// When synthesizing a property that's declared in multiple protocols, ensure
+// that the setter is emitted if any of these declarations is readwrite.
+@protocol ABC
+@property (copy, nonatomic,  readonly) Test3 *someId;
+@end
+@protocol ABC__Mutable <ABC>
+@property (copy, nonatomic, readwrite) Test3 *someId;
+@end
+
+@interface ABC_Class <ABC, ABC__Mutable>
+@end
+
+@implementation ABC_Class
+@synthesize someId = _someId;
+// CHECK:  define internal %{{.*}}* @"\01-[ABC_Class someId]"
+// CHECK:  define internal void @"\01-[ABC_Class setSomeId:]"(
+@end
+
+
 // CHECK: attributes [[NUW]] = { nounwind }

cfe/trunk/test/SemaObjC/arc-property-decl-attrs.m

 
 @interface I2()
 @property (readwrite) id prop;
 @end
 
 @implementation I2
 @synthesize prop;
 @end
+
+// rdar://31579994
+// Verify that the all of the property declarations in inherited protocols are
+// compatible when synthesing a property from a protocol.
+
+@protocol CopyVsAssign1
+@property (copy, nonatomic,  readonly) id prop; // expected-error {{property with attribute 'copy' was selected for synthesis}}
+@end
+@protocol CopyVsAssign2
+@property (assign, nonatomic, readonly) id prop; // expected-note {{it could also be property without attribute 'copy' declared here}}
+@end
+
+@interface CopyVsAssign: Foo <CopyVsAssign1, CopyVsAssign2>
+@end
+@implementation CopyVsAssign
+@synthesize prop; // expected-note {{property synthesized here}}
+@end
+
+@protocol RetainVsNonRetain1
+@property (readonly) id prop; // expected-error {{property without attribute 'retain (or strong)' was selected for synthesis}}
+@end
+@protocol RetainVsNonRetain2
+@property (retain, readonly) id prop; // expected-note {{it could also be property with attribute 'retain (or strong)' declared here}}
+@end
+
+@interface RetainVsNonRetain: Foo <RetainVsNonRetain1, RetainVsNonRetain2>
+@end
+@implementation RetainVsNonRetain
+@synthesize prop; // expected-note {{property synthesized here}}
+@end
+
+@protocol AtomicVsNonatomic1
+@property (copy, nonatomic, readonly) id prop; // expected-error {{property without attribute 'atomic' was selected for synthesis}}
+@end
+@protocol AtomicVsNonatomic2
+@property (copy, atomic, readonly) id prop; // expected-note {{it could also be property with attribute 'atomic' declared here}}
+@end
+
+@interface AtomicVsNonAtomic: Foo <AtomicVsNonatomic1, AtomicVsNonatomic2>
+@end
+@implementation AtomicVsNonAtomic
+@synthesize prop; // expected-note {{property synthesized here}}
+@end
+
+@protocol Getter1
+@property (copy, readonly) id prop; // expected-error {{property with getter 'prop' was selected for synthesis}}
+@end
+@protocol Getter2
+@property (copy, getter=x, readonly) id prop; // expected-note {{it could also be property with getter 'x' declared here}}
+@end
+
+@interface GetterVsGetter: Foo <Getter1, Getter2>
+@end
+@implementation GetterVsGetter
+@synthesize prop; // expected-note {{property synthesized here}}
+@end
+
+@protocol Setter1
+@property (copy, readonly) id prop;
+@end
+@protocol Setter2
+@property (copy, setter=setp:, readwrite) id prop; // expected-error {{property with setter 'setp:' was selected for synthesis}}
+@end
+@protocol Setter3
+@property (copy, readwrite) id prop; // expected-note {{it could also be property with setter 'setProp:' declared here}}
+@end
+
+@interface SetterVsSetter: Foo <Setter1, Setter2, Setter3>
+@end
+@implementation SetterVsSetter
+@synthesize prop; // expected-note {{property synthesized here}}
+@end
+
+@protocol TypeVsAttribute1
+@property (assign, atomic, readonly) int prop; // expected-error {{property of type 'int' was selected for synthesis}}
+@end
+@protocol TypeVsAttribute2
+@property (assign, atomic, readonly) id prop; // expected-note {{it could also be property of type 'id' declared here}}
+@end
+@protocol TypeVsAttribute3
+@property (copy, readonly) id prop; // expected-note {{it could also be property with attribute 'copy' declared here}}
+@end
+
+@interface TypeVsAttribute: Foo <TypeVsAttribute1, TypeVsAttribute2, TypeVsAttribute3>
+@end
+@implementation TypeVsAttribute
+@synthesize prop; // expected-note {{property synthesized here}}
+@end
+
+@protocol TypeVsSetter1
+@property (assign, nonatomic, readonly) int prop; // expected-note {{it could also be property of type 'int' declared here}}
+@end
+@protocol TypeVsSetter2
+@property (assign, nonatomic, readonly) id prop; // ok
+@end
+@protocol TypeVsSetter3
+@property (assign, nonatomic, readwrite) id prop; // expected-error {{property of type 'id' was selected for synthesis}}
+@end
+
+@interface TypeVsSetter: Foo <TypeVsSetter1, TypeVsSetter2, TypeVsSetter3>
+@end
+@implementation TypeVsSetter
+@synthesize prop; // expected-note {{property synthesized here}}
+@end

cfe/trunk/test/SemaObjC/property-ambiguous-synthesis.m

 // RUN: %clang_cc1  -fsyntax-only -verify -Wno-objc-root-class %s
 // rdar://13075400
 
 @protocol FooAsID
-@property (copy) id foo; // expected-note 2 {{it could also be property of type 'id' declared here}} \\
+@property (assign) id foo; // expected-note 2 {{it could also be property of type 'id' declared here}} \\
        // expected-warning {{property of type 'id' was selected for synthesis}}
 @end
 
 @protocol FooAsDouble
 @property double foo; // expected-warning 2 {{property of type 'double' was selected for synthesis}} \
           // expected-note {{it could also be property of type 'double' declared here}}
 @end