r307903 - [ObjC] Pick a 'readwrite' property when synthesizing ambiguous
Alex Lorenz via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 13 04:06:23 PDT 2017
Author: arphaman
Date: Thu Jul 13 04:06:22 2017
New Revision: 307903
URL: http://llvm.org/viewvc/llvm-project?rev=307903&view=rev
Log:
[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 <P2> @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
Modified:
cfe/trunk/include/clang/AST/DeclObjC.h
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/AST/DeclObjC.cpp
cfe/trunk/lib/Sema/SemaObjCProperty.cpp
cfe/trunk/test/CodeGenObjC/arc-property.m
cfe/trunk/test/SemaObjC/arc-property-decl-attrs.m
cfe/trunk/test/SemaObjC/property-ambiguous-synthesis.m
Modified: cfe/trunk/include/clang/AST/DeclObjC.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclObjC.h?rev=307903&r1=307902&r2=307903&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/DeclObjC.h (original)
+++ cfe/trunk/include/clang/AST/DeclObjC.h Thu Jul 13 04:06:22 2017
@@ -1039,10 +1039,9 @@ public:
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 @@ 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; }
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=307903&r1=307902&r2=307903&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 13 04:06:22 2017
@@ -808,8 +808,10 @@ def warn_property_types_are_incompatible
"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 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<
Modified: cfe/trunk/lib/AST/DeclObjC.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclObjC.cpp?rev=307903&r1=307902&r2=307903&view=diff
==============================================================================
--- cfe/trunk/lib/AST/DeclObjC.cpp (original)
+++ cfe/trunk/lib/AST/DeclObjC.cpp Thu Jul 13 04:06:22 2017
@@ -1889,25 +1889,23 @@ void ObjCProtocolDecl::collectProperties
}
}
-
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);
}
}
Modified: cfe/trunk/lib/Sema/SemaObjCProperty.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaObjCProperty.cpp?rev=307903&r1=307902&r2=307903&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaObjCProperty.cpp (original)
+++ cfe/trunk/lib/Sema/SemaObjCProperty.cpp Thu Jul 13 04:06:22 2017
@@ -814,53 +814,185 @@ static void setImpliedPropertyAttributeF
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 @@ Decl *Sema::ActOnPropertyImplDecl(Scope
}
}
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);
Modified: cfe/trunk/test/CodeGenObjC/arc-property.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjC/arc-property.m?rev=307903&r1=307902&r2=307903&view=diff
==============================================================================
--- cfe/trunk/test/CodeGenObjC/arc-property.m (original)
+++ cfe/trunk/test/CodeGenObjC/arc-property.m Thu Jul 13 04:06:22 2017
@@ -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.
+ at protocol ABC
+ at property (copy, nonatomic, readonly) Test3 *someId;
+ at end
+ at protocol ABC__Mutable <ABC>
+ at property (copy, nonatomic, readwrite) Test3 *someId;
+ at end
+
+ at interface ABC_Class <ABC, ABC__Mutable>
+ at end
+
+ at implementation ABC_Class
+ at synthesize someId = _someId;
+// CHECK: define internal %{{.*}}* @"\01-[ABC_Class someId]"
+// CHECK: define internal void @"\01-[ABC_Class setSomeId:]"(
+ at end
+
+
// CHECK: attributes [[NUW]] = { nounwind }
Modified: cfe/trunk/test/SemaObjC/arc-property-decl-attrs.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/arc-property-decl-attrs.m?rev=307903&r1=307902&r2=307903&view=diff
==============================================================================
--- cfe/trunk/test/SemaObjC/arc-property-decl-attrs.m (original)
+++ cfe/trunk/test/SemaObjC/arc-property-decl-attrs.m Thu Jul 13 04:06:22 2017
@@ -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.
+
+ at protocol CopyVsAssign1
+ at property (copy, nonatomic, readonly) id prop; // expected-error {{property with attribute 'copy' was selected for synthesis}}
+ at end
+ at protocol CopyVsAssign2
+ at property (assign, nonatomic, readonly) id prop; // expected-note {{it could also be property without attribute 'copy' declared here}}
+ at end
+
+ at interface CopyVsAssign: Foo <CopyVsAssign1, CopyVsAssign2>
+ at end
+ at implementation CopyVsAssign
+ at synthesize prop; // expected-note {{property synthesized here}}
+ at end
+
+ at protocol RetainVsNonRetain1
+ at property (readonly) id prop; // expected-error {{property without attribute 'retain (or strong)' was selected for synthesis}}
+ at end
+ at protocol RetainVsNonRetain2
+ at property (retain, readonly) id prop; // expected-note {{it could also be property with attribute 'retain (or strong)' declared here}}
+ at end
+
+ at interface RetainVsNonRetain: Foo <RetainVsNonRetain1, RetainVsNonRetain2>
+ at end
+ at implementation RetainVsNonRetain
+ at synthesize prop; // expected-note {{property synthesized here}}
+ at end
+
+ at protocol AtomicVsNonatomic1
+ at property (copy, nonatomic, readonly) id prop; // expected-error {{property without attribute 'atomic' was selected for synthesis}}
+ at end
+ at protocol AtomicVsNonatomic2
+ at property (copy, atomic, readonly) id prop; // expected-note {{it could also be property with attribute 'atomic' declared here}}
+ at end
+
+ at interface AtomicVsNonAtomic: Foo <AtomicVsNonatomic1, AtomicVsNonatomic2>
+ at end
+ at implementation AtomicVsNonAtomic
+ at synthesize prop; // expected-note {{property synthesized here}}
+ at end
+
+ at protocol Getter1
+ at property (copy, readonly) id prop; // expected-error {{property with getter 'prop' was selected for synthesis}}
+ at end
+ at protocol Getter2
+ at property (copy, getter=x, readonly) id prop; // expected-note {{it could also be property with getter 'x' declared here}}
+ at end
+
+ at interface GetterVsGetter: Foo <Getter1, Getter2>
+ at end
+ at implementation GetterVsGetter
+ at synthesize prop; // expected-note {{property synthesized here}}
+ at end
+
+ at protocol Setter1
+ at property (copy, readonly) id prop;
+ at end
+ at protocol Setter2
+ at property (copy, setter=setp:, readwrite) id prop; // expected-error {{property with setter 'setp:' was selected for synthesis}}
+ at end
+ at protocol Setter3
+ at property (copy, readwrite) id prop; // expected-note {{it could also be property with setter 'setProp:' declared here}}
+ at end
+
+ at interface SetterVsSetter: Foo <Setter1, Setter2, Setter3>
+ at end
+ at implementation SetterVsSetter
+ at synthesize prop; // expected-note {{property synthesized here}}
+ at end
+
+ at protocol TypeVsAttribute1
+ at property (assign, atomic, readonly) int prop; // expected-error {{property of type 'int' was selected for synthesis}}
+ at end
+ at protocol TypeVsAttribute2
+ at property (assign, atomic, readonly) id prop; // expected-note {{it could also be property of type 'id' declared here}}
+ at end
+ at protocol TypeVsAttribute3
+ at property (copy, readonly) id prop; // expected-note {{it could also be property with attribute 'copy' declared here}}
+ at end
+
+ at interface TypeVsAttribute: Foo <TypeVsAttribute1, TypeVsAttribute2, TypeVsAttribute3>
+ at end
+ at implementation TypeVsAttribute
+ at synthesize prop; // expected-note {{property synthesized here}}
+ at end
+
+ at protocol TypeVsSetter1
+ at property (assign, nonatomic, readonly) int prop; // expected-note {{it could also be property of type 'int' declared here}}
+ at end
+ at protocol TypeVsSetter2
+ at property (assign, nonatomic, readonly) id prop; // ok
+ at end
+ at protocol TypeVsSetter3
+ at property (assign, nonatomic, readwrite) id prop; // expected-error {{property of type 'id' was selected for synthesis}}
+ at end
+
+ at interface TypeVsSetter: Foo <TypeVsSetter1, TypeVsSetter2, TypeVsSetter3>
+ at end
+ at implementation TypeVsSetter
+ at synthesize prop; // expected-note {{property synthesized here}}
+ at end
Modified: cfe/trunk/test/SemaObjC/property-ambiguous-synthesis.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/property-ambiguous-synthesis.m?rev=307903&r1=307902&r2=307903&view=diff
==============================================================================
--- cfe/trunk/test/SemaObjC/property-ambiguous-synthesis.m (original)
+++ cfe/trunk/test/SemaObjC/property-ambiguous-synthesis.m Thu Jul 13 04:06:22 2017
@@ -2,7 +2,7 @@
// rdar://13075400
@protocol FooAsID
- at property (copy) id foo; // expected-note 2 {{it could also be property of type 'id' declared here}} \\
+ at 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
More information about the cfe-commits
mailing list