From: Alex Lorenz Date: Thu, 13 Jul 2017 11:06:22 +0000 (+0000) Subject: [ObjC] Pick a 'readwrite' property when synthesizing ambiguous X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=2eb4707ace01387dab36ccd2cc515fd33af0370c;p=clang [ObjC] Pick a 'readwrite' property when synthesizing ambiguous property and check for incompatible attributes This commit changes the way ambiguous property synthesis (i.e. when synthesizing a property that's declared in multiple protocols) is performed. Previously, Clang 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 commit 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 @end @implementation I @syntesize p; // Users previously got a warning here, and Clang synthesized // readonly 'int p' here. Now Clang synthesizes readwrite 'id' p.. @end ``` To ensure that this change is safe, the warning about incompatible types is promoted to an error when this kind of readonly/readwrite ambiguity is detected in the @implementation. This will ensure that previous code that had this subtle bug and ignored the warning now will fail to compile with an error, and users should not get suprises at runtime once they resolve the error. The commit also extends the ambiguity checker, and now it can detect conflicts among the different property attributes. An error diagnostic is used for conflicting attributes, to ensure that the user won't get "suprises" at runtime. ProtocolPropertyMap is removed in favour of a a set + vector because the map's order of iteration is non-deterministic, so it couldn't be used to select the readwrite property. rdar://31579994 Differential Revision: https://reviews.llvm.org/D35268 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@307903 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/DeclObjC.h b/include/clang/AST/DeclObjC.h index 26c0cbe82d..1cd6e004f7 100644 --- a/include/clang/AST/DeclObjC.h +++ b/include/clang/AST/DeclObjC.h @@ -1039,10 +1039,9 @@ public: typedef llvm::DenseMap, ObjCPropertyDecl*> PropertyMap; - - typedef llvm::DenseMap - ProtocolPropertyMap; - + + typedef llvm::SmallDenseSet ProtocolPropertySet; + typedef llvm::SmallVector PropertyDeclOrder; /// This routine collects list of properties to be implemented in the class. @@ -2159,7 +2158,8 @@ public: 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; } diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 5a8750e4da..709a96a6c9 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -808,8 +808,10 @@ def warn_property_types_are_incompatible : Warning< "property type %0 is incompatible with type %1 inherited from %2">, InGroup>; 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>; +def err_protocol_property_mismatch: Error; 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">; @@ -1088,7 +1090,9 @@ def err_category_property : Error< 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< diff --git a/lib/AST/DeclObjC.cpp b/lib/AST/DeclObjC.cpp index a0ec0c2b25..d8bdb6369e 100644 --- a/lib/AST/DeclObjC.cpp +++ b/lib/AST/DeclObjC.cpp @@ -1889,25 +1889,23 @@ void ObjCProtocolDecl::collectPropertiesToImplement(PropertyMap &PM, } } - 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); + for (const auto *PI : PDecl->protocols()) + PI->collectInheritedProtocolProperties(Property, PS, PO); } } diff --git a/lib/Sema/SemaObjCProperty.cpp b/lib/Sema/SemaObjCProperty.cpp index 62a771bcff..e1e85dfd5e 100644 --- a/lib/Sema/SemaObjCProperty.cpp +++ b/lib/Sema/SemaObjCProperty.cpp @@ -814,53 +814,185 @@ static void setImpliedPropertyAttributeForReadOnlyProperty( 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(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 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 (!FirsTime && AtLoc.isValid()) + + 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; + } + } + 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. @@ -996,8 +1128,9 @@ Decl *Sema::ActOnPropertyImplDecl(Scope *S, } } if (Synthesize && isa(property->getDeclContext())) - DiagnosePropertyMismatchDeclInProtocols(*this, AtLoc, IDecl, property); - + property = SelectPropertyForSynthesisFromProtocols(*this, AtLoc, IDecl, + property); + } else if ((CatImplClass = dyn_cast(ClassImpDecl))) { if (Synthesize) { Diag(AtLoc, diag::err_synthesize_category_decl); diff --git a/test/CodeGenObjC/arc-property.m b/test/CodeGenObjC/arc-property.m index b8dc18e872..edc4d1e853 100644 --- a/test/CodeGenObjC/arc-property.m +++ b/test/CodeGenObjC/arc-property.m @@ -131,4 +131,24 @@ void test3(Test3 *t) { - (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 +@property (copy, nonatomic, readwrite) Test3 *someId; +@end + +@interface ABC_Class +@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 } diff --git a/test/SemaObjC/arc-property-decl-attrs.m b/test/SemaObjC/arc-property-decl-attrs.m index 6c96ba481c..ee48d310ed 100644 --- a/test/SemaObjC/arc-property-decl-attrs.m +++ b/test/SemaObjC/arc-property-decl-attrs.m @@ -121,3 +121,107 @@ __attribute__((objc_root_class)) @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 +@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 +@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 +@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 +@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 +@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 +@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 +@end +@implementation TypeVsSetter +@synthesize prop; // expected-note {{property synthesized here}} +@end diff --git a/test/SemaObjC/property-ambiguous-synthesis.m b/test/SemaObjC/property-ambiguous-synthesis.m index 98f5945074..5c652fa472 100644 --- a/test/SemaObjC/property-ambiguous-synthesis.m +++ b/test/SemaObjC/property-ambiguous-synthesis.m @@ -2,7 +2,7 @@ // 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