arphaman created this revision.
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 breaks users code like this:
@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
Repository:
rL LLVM
https://reviews.llvm.org/D35268
Files:
include/clang/AST/DeclObjC.h
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/DeclObjC.cpp
lib/Sema/SemaObjCProperty.cpp
test/CodeGenObjC/arc-property.m
test/SemaObjC/arc-property-decl-attrs.m
test/SemaObjC/property-ambiguous-synthesis.m
Index: test/SemaObjC/property-ambiguous-synthesis.m
===================================================================
--- test/SemaObjC/property-ambiguous-synthesis.m
+++ 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
Index: test/SemaObjC/arc-property-decl-attrs.m
===================================================================
--- test/SemaObjC/arc-property-decl-attrs.m
+++ test/SemaObjC/arc-property-decl-attrs.m
@@ -121,3 +121,107 @@
@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
Index: test/CodeGenObjC/arc-property.m
===================================================================
--- test/CodeGenObjC/arc-property.m
+++ test/CodeGenObjC/arc-property.m
@@ -131,4 +131,24 @@
- (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 }
Index: lib/Sema/SemaObjCProperty.cpp
===================================================================
--- lib/Sema/SemaObjCProperty.cpp
+++ lib/Sema/SemaObjCProperty.cpp
@@ -814,53 +814,185 @@
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 (!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 @@
}
}
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);
Index: lib/AST/DeclObjC.cpp
===================================================================
--- lib/AST/DeclObjC.cpp
+++ lib/AST/DeclObjC.cpp
@@ -1889,25 +1889,23 @@
}
}
-
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);
}
}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -808,8 +808,10 @@
"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">;
@@ -1088,7 +1090,9 @@
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<
Index: include/clang/AST/DeclObjC.h
===================================================================
--- include/clang/AST/DeclObjC.h
+++ include/clang/AST/DeclObjC.h
@@ -1039,10 +1039,9 @@
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.
@@ -2159,7 +2158,8 @@
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; }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
