Objective-C properties: merge attributes when redeclaring 'readonly' as 'readwrite' in an extension.

r251874 stopped back-patching the AST when an Objective-C 'readonly'
property is redeclared in a class extension as 'readwrite'. However,
it did not properly handle merging of Objective-C property attributes
(e.g., getter name, ownership, atomicity) to the redeclaration,
leading to bad metadata. Merge (and check!) those property attributes
so we get the right metadata and reasonable ASTs. Fixes
rdar://problem/23823989.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@255309 91177308-0d34-0410-b5e6-96231b3b80d8

1 files changed, 158 insertions, 89 deletions

diff --git a/lib/Sema/SemaObjCProperty.cpp b/lib/Sema/SemaObjCProperty.cpp
index 0beb336092..f42daced86 100644
--- a/lib/Sema/SemaObjCProperty.cpp
+++ b/lib/Sema/SemaObjCProperty.cpp
@@ -153,13 +153,26 @@ static unsigned deducePropertyOwnershipFromType(Sema &S, QualType T) {
   return 0;
 }
 
+static const unsigned OwnershipMask =
+  (ObjCPropertyDecl::OBJC_PR_assign |
+   ObjCPropertyDecl::OBJC_PR_retain |
+   ObjCPropertyDecl::OBJC_PR_copy   |
+   ObjCPropertyDecl::OBJC_PR_weak   |
+   ObjCPropertyDecl::OBJC_PR_strong |
+   ObjCPropertyDecl::OBJC_PR_unsafe_unretained);
+
 static unsigned getOwnershipRule(unsigned attr) {
-  return attr & (ObjCPropertyDecl::OBJC_PR_assign |
-                 ObjCPropertyDecl::OBJC_PR_retain |
-                 ObjCPropertyDecl::OBJC_PR_copy   |
-                 ObjCPropertyDecl::OBJC_PR_weak   |
-                 ObjCPropertyDecl::OBJC_PR_strong |
-                 ObjCPropertyDecl::OBJC_PR_unsafe_unretained);
+  unsigned result = attr & OwnershipMask;
+
+  // From an ownership perspective, assign and unsafe_unretained are
+  // identical; make sure one also implies the other.
+  if (result & (ObjCPropertyDecl::OBJC_PR_assign |
+                ObjCPropertyDecl::OBJC_PR_unsafe_unretained)) {
+    result |= ObjCPropertyDecl::OBJC_PR_assign |
+              ObjCPropertyDecl::OBJC_PR_unsafe_unretained;
+  }
+
+  return result;
 }
 
 Decl *Sema::ActOnProperty(Scope *S, SourceLocation AtLoc,
@@ -168,7 +181,6 @@ Decl *Sema::ActOnProperty(Scope *S, SourceLocation AtLoc,
                           ObjCDeclSpec &ODS,
                           Selector GetterSel,
                           Selector SetterSel,
-                          bool *isOverridingProperty,
                           tok::ObjCKeywordKind MethodImplKind,
                           DeclContext *lexicalDC) {
   unsigned Attributes = ODS.getPropertyAttributes();
@@ -182,19 +194,6 @@ Decl *Sema::ActOnProperty(Scope *S, SourceLocation AtLoc,
                       // default is readwrite!
                       !(Attributes & ObjCDeclSpec::DQ_PR_readonly));
 
-  // Property defaults to 'assign' if it is readwrite, unless this is ARC
-  // and the type is retainable.
-  bool isAssign;
-  if (Attributes & (ObjCDeclSpec::DQ_PR_assign |
-                    ObjCDeclSpec::DQ_PR_unsafe_unretained)) {
-    isAssign = true;
-  } else if (getOwnershipRule(Attributes) || !isReadWrite) {
-    isAssign = false;
-  } else {
-    isAssign = (!getLangOpts().ObjCAutoRefCount ||
-                !T->isObjCRetainableType());
-  }
-
   // Proceed with constructing the ObjCPropertyDecls.
   ObjCContainerDecl *ClassDecl = cast<ObjCContainerDecl>(CurContext);
   ObjCPropertyDecl *Res = nullptr;
@@ -202,11 +201,10 @@ Decl *Sema::ActOnProperty(Scope *S, SourceLocation AtLoc,
     if (CDecl->IsClassExtension()) {
       Res = HandlePropertyInClassExtension(S, AtLoc, LParenLoc,
                                            FD, GetterSel, SetterSel,
-                                           isAssign, isReadWrite,
+                                           isReadWrite,
                                            Attributes,
                                            ODS.getPropertyAttributes(),
-                                           isOverridingProperty, T, TSI,
-                                           MethodImplKind);
+                                           T, TSI, MethodImplKind);
       if (!Res)
         return nullptr;
     }
@@ -214,7 +212,7 @@ Decl *Sema::ActOnProperty(Scope *S, SourceLocation AtLoc,
 
   if (!Res) {
     Res = CreatePropertyDecl(S, ClassDecl, AtLoc, LParenLoc, FD,
-                             GetterSel, SetterSel, isAssign, isReadWrite,
+                             GetterSel, SetterSel, isReadWrite,
                              Attributes, ODS.getPropertyAttributes(),
                              T, TSI, MethodImplKind);
     if (lexicalDC)
@@ -342,12 +340,15 @@ static bool LocPropertyAttribute( ASTContext &Context, const char *attrName,
 /// Check for a mismatch in the atomicity of the given properties.
 static void checkAtomicPropertyMismatch(Sema &S,
                                         ObjCPropertyDecl *OldProperty,
-                                        ObjCPropertyDecl *NewProperty) {
+                                        ObjCPropertyDecl *NewProperty,
+                                        bool PropagateAtomicity) {
   // If the atomicity of both matches, we're done.
   bool OldIsAtomic =
-    (OldProperty->getPropertyAttributes() & ObjCDeclSpec::DQ_PR_nonatomic) == 0;
+    (OldProperty->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_nonatomic)
+      == 0;
   bool NewIsAtomic =
-    (NewProperty->getPropertyAttributes() & ObjCDeclSpec::DQ_PR_nonatomic) == 0;
+    (NewProperty->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_nonatomic)
+      == 0;
   if (OldIsAtomic == NewIsAtomic) return;
 
   // Determine whether the given property is readonly and implicitly
@@ -355,18 +356,36 @@ static void checkAtomicPropertyMismatch(Sema &S,
   auto isImplicitlyReadonlyAtomic = [](ObjCPropertyDecl *Property) -> bool {
     // Is it readonly?
     auto Attrs = Property->getPropertyAttributes();
-    if ((Attrs & ObjCDeclSpec::DQ_PR_readonly) == 0) return false;
+    if ((Attrs & ObjCPropertyDecl::OBJC_PR_readonly) == 0) return false;
 
     // Is it nonatomic?
-    if (Attrs & ObjCDeclSpec::DQ_PR_nonatomic) return false;
+    if (Attrs & ObjCPropertyDecl::OBJC_PR_nonatomic) return false;
 
     // Was 'atomic' specified directly?
-    if (Property->getPropertyAttributesAsWritten() & ObjCDeclSpec::DQ_PR_atomic)
+    if (Property->getPropertyAttributesAsWritten() & 
+          ObjCPropertyDecl::OBJC_PR_atomic)
       return false;
 
     return true;
   };
 
+  // If we're allowed to propagate atomicity, and the new property did
+  // not specify atomicity at all, propagate.
+  const unsigned AtomicityMask =
+    (ObjCPropertyDecl::OBJC_PR_atomic | ObjCPropertyDecl::OBJC_PR_nonatomic);
+  if (PropagateAtomicity &&
+      ((NewProperty->getPropertyAttributesAsWritten() & AtomicityMask) == 0)) {
+    unsigned Attrs = NewProperty->getPropertyAttributes();
+    Attrs = Attrs & ~AtomicityMask;
+    if (OldIsAtomic)
+      Attrs |= ObjCPropertyDecl::OBJC_PR_atomic;
+    else 
+      Attrs |= ObjCPropertyDecl::OBJC_PR_nonatomic;
+
+    NewProperty->overwritePropertyAttributes(Attrs);
+    return;
+  }
+
   // One of the properties is atomic; if it's a readonly property, and
   // 'atomic' wasn't explicitly specified, we're okay.
   if ((OldIsAtomic && isImplicitlyReadonlyAtomic(OldProperty)) ||
@@ -393,11 +412,9 @@ Sema::HandlePropertyInClassExtension(Scope *S,
                                      SourceLocation LParenLoc,
                                      FieldDeclarator &FD,
                                      Selector GetterSel, Selector SetterSel,
-                                     const bool isAssign,
                                      const bool isReadWrite,
-                                     const unsigned Attributes,
+                                     unsigned &Attributes,
                                      const unsigned AttributesAsWritten,
-                                     bool *isOverridingProperty,
                                      QualType T,
                                      TypeSourceInfo *TSI,
                                      tok::ObjCKeywordKind MethodImplKind) {
@@ -411,7 +428,6 @@ Sema::HandlePropertyInClassExtension(Scope *S,
   // already declared.
   if (!CCPrimary) {
     Diag(CDecl->getLocation(), diag::err_continuation_class);
-    *isOverridingProperty = true;
     return nullptr;
   }
 
@@ -427,11 +443,73 @@ Sema::HandlePropertyInClassExtension(Scope *S,
     return nullptr;
   }
 
+  // Check for consistency with the previous declaration, if there is one.
+  if (PIDecl) {
+    // A readonly property declared in the primary class can be refined
+    // by adding a readwrite property within an extension.
+    // Anything else is an error.
+    if (!(PIDecl->isReadOnly() && isReadWrite)) {
+      // Tailor the diagnostics for the common case where a readwrite
+      // property is declared both in the @interface and the continuation.
+      // This is a common error where the user often intended the original
+      // declaration to be readonly.
+      unsigned diag =
+        (Attributes & ObjCDeclSpec::DQ_PR_readwrite) &&
+        (PIDecl->getPropertyAttributesAsWritten() &
+           ObjCPropertyDecl::OBJC_PR_readwrite)
+        ? diag::err_use_continuation_class_redeclaration_readwrite
+        : diag::err_use_continuation_class;
+      Diag(AtLoc, diag)
+        << CCPrimary->getDeclName();
+      Diag(PIDecl->getLocation(), diag::note_property_declare);
+      return nullptr;
+    }
+
+    // Check for consistency of getters.
+    if (PIDecl->getGetterName() != GetterSel) {
+     // If the getter was written explicitly, complain.
+      if (AttributesAsWritten & ObjCDeclSpec::DQ_PR_getter) {
+        Diag(AtLoc, diag::warn_property_redecl_getter_mismatch)
+          << PIDecl->getGetterName() << GetterSel;
+        Diag(PIDecl->getLocation(), diag::note_property_declare);
+      }
+      
+      // Always adopt the getter from the original declaration.
+      GetterSel = PIDecl->getGetterName();
+      Attributes |= ObjCDeclSpec::DQ_PR_getter;
+    }
+
+    // Check consistency of ownership.
+    unsigned ExistingOwnership
+      = getOwnershipRule(PIDecl->getPropertyAttributes());
+    unsigned NewOwnership = getOwnershipRule(Attributes);
+    if (ExistingOwnership && NewOwnership != ExistingOwnership) {
+      // If the ownership was written explicitly, complain.
+      if (getOwnershipRule(AttributesAsWritten)) {
+        Diag(AtLoc, diag::warn_property_attr_mismatch);
+        Diag(PIDecl->getLocation(), diag::note_property_declare);
+      }
+
+      // Take the ownership from the original property.
+      Attributes = (Attributes & ~OwnershipMask) | ExistingOwnership;
+    }
+
+    // If the redeclaration is 'weak' but the original property is not, 
+    if ((Attributes & ObjCPropertyDecl::OBJC_PR_weak) &&
+        !(PIDecl->getPropertyAttributesAsWritten()
+            & ObjCPropertyDecl::OBJC_PR_weak) &&
+        PIDecl->getType()->getAs<ObjCObjectPointerType>() &&
+        PIDecl->getType().getObjCLifetime() == Qualifiers::OCL_None) {
+      Diag(AtLoc, diag::warn_property_implicitly_mismatched);
+      Diag(PIDecl->getLocation(), diag::note_property_declare);
+    }        
+  }
+
   // Create a new ObjCPropertyDecl with the DeclContext being
   // the class extension.
   ObjCPropertyDecl *PDecl = CreatePropertyDecl(S, CDecl, AtLoc, LParenLoc,
                                                FD, GetterSel, SetterSel,
-                                               isAssign, isReadWrite,
+                                               isReadWrite,
                                                Attributes, AttributesAsWritten,
                                                T, TSI, MethodImplKind, DC);
 
@@ -464,57 +542,10 @@ Sema::HandlePropertyInClassExtension(Scope *S,
       return nullptr;
     }
   }
-
-  // A readonly property declared in the primary class can be refined
-  // by adding a readwrite property within an extension.
-  // Anything else is an error.
-  unsigned PIkind = PIDecl->getPropertyAttributesAsWritten();
-  if (!(isReadWrite && (PIkind & ObjCPropertyDecl::OBJC_PR_readonly))) {
-    // Tailor the diagnostics for the common case where a readwrite
-    // property is declared both in the @interface and the continuation.
-    // This is a common error where the user often intended the original
-    // declaration to be readonly.
-    unsigned diag =
-      (Attributes & ObjCDeclSpec::DQ_PR_readwrite) &&
-      (PIkind & ObjCPropertyDecl::OBJC_PR_readwrite)
-      ? diag::err_use_continuation_class_redeclaration_readwrite
-      : diag::err_use_continuation_class;
-    Diag(AtLoc, diag)
-      << CCPrimary->getDeclName();
-    Diag(PIDecl->getLocation(), diag::note_property_declare);
-    return nullptr;
-  }
-
-  PIkind &= ~ObjCPropertyDecl::OBJC_PR_readonly;
-  PIkind |= ObjCPropertyDecl::OBJC_PR_readwrite;
-  PIkind |= deducePropertyOwnershipFromType(*this, PIDecl->getType());
-  unsigned ClassExtensionMemoryModel = getOwnershipRule(Attributes);
-  unsigned PrimaryClassMemoryModel = getOwnershipRule(PIkind);
-  if (PrimaryClassMemoryModel && ClassExtensionMemoryModel &&
-      (PrimaryClassMemoryModel != ClassExtensionMemoryModel)) {
-    Diag(AtLoc, diag::warn_property_attr_mismatch);
-    Diag(PIDecl->getLocation(), diag::note_property_declare);
-  } else if (getLangOpts().ObjCAutoRefCount) {
-    QualType PrimaryPropertyQT =
-      Context.getCanonicalType(PIDecl->getType()).getUnqualifiedType();
-    if (isa<ObjCObjectPointerType>(PrimaryPropertyQT)) {
-      bool PropertyIsWeak = ((PIkind & ObjCPropertyDecl::OBJC_PR_weak) != 0);
-      Qualifiers::ObjCLifetime PrimaryPropertyLifeTime =
-        PrimaryPropertyQT.getObjCLifetime();
-      if (PrimaryPropertyLifeTime == Qualifiers::OCL_None &&
-          (Attributes & ObjCDeclSpec::DQ_PR_weak) &&
-          !PropertyIsWeak) {
-            Diag(AtLoc, diag::warn_property_implicitly_mismatched);
-            Diag(PIDecl->getLocation(), diag::note_property_declare);
-          }
-      }
-  }
   
   // Check that atomicity of property in class extension matches the previous
   // declaration.
-  checkAtomicPropertyMismatch(*this, PIDecl, PDecl);
-
-  *isOverridingProperty = true;
+  checkAtomicPropertyMismatch(*this, PIDecl, PDecl, true);
 
   // Make sure getter/setter are appropriately synthesized.
   ProcessPropertyDecl(PDecl);
@@ -528,7 +559,6 @@ ObjCPropertyDecl *Sema::CreatePropertyDecl(Scope *S,
                                            FieldDeclarator &FD,
                                            Selector GetterSel,
                                            Selector SetterSel,
-                                           const bool isAssign,
                                            const bool isReadWrite,
                                            const unsigned Attributes,
                                            const unsigned AttributesAsWritten,
@@ -538,10 +568,23 @@ ObjCPropertyDecl *Sema::CreatePropertyDecl(Scope *S,
                                            DeclContext *lexicalDC){
   IdentifierInfo *PropertyId = FD.D.getIdentifier();
 
-  // Issue a warning if property is 'assign' as default and its object, which is
-  // gc'able conforms to NSCopying protocol
+  // Property defaults to 'assign' if it is readwrite, unless this is ARC
+  // and the type is retainable.
+  bool isAssign;
+  if (Attributes & (ObjCDeclSpec::DQ_PR_assign |
+                    ObjCDeclSpec::DQ_PR_unsafe_unretained)) {
+    isAssign = true;
+  } else if (getOwnershipRule(Attributes) || !isReadWrite) {
+    isAssign = false;
+  } else {
+    isAssign = (!getLangOpts().ObjCAutoRefCount ||
+                !T->isObjCRetainableType());
+  }
+
+  // Issue a warning if property is 'assign' as default and its
+  // object, which is gc'able conforms to NSCopying protocol
   if (getLangOpts().getGC() != LangOptions::NonGC &&
-      isAssign && !(Attributes & ObjCDeclSpec::DQ_PR_assign))
+      isAssign && !(Attributes & ObjCDeclSpec::DQ_PR_assign)) {
     if (const ObjCObjectPointerType *ObjPtrTy =
           T->getAs<ObjCObjectPointerType>()) {
       ObjCInterfaceDecl *IDecl = ObjPtrTy->getObjectType()->getInterface();
@@ -551,6 +594,7 @@ ObjCPropertyDecl *Sema::CreatePropertyDecl(Scope *S,
           if (IDecl->ClassImplementsProtocol(PNSCopying, true))
             Diag(AtLoc, diag::warn_implements_nscopying) << PropertyId;
     }
+  }
 
   if (T->isObjCObjectType()) {
     SourceLocation StarLoc = TInfo->getTypeLoc().getLocEnd();
@@ -802,6 +846,31 @@ DiagnosePropertyMismatchDeclInProtocols(Sema &S, SourceLocation AtLoc,
     S.Diag(AtLoc, diag::note_property_synthesize);
 }
 
+/// Determine whether any storage attributes were written on the property.
+static bool hasWrittenStorageAttribute(ObjCPropertyDecl *Prop) {
+  if (Prop->getPropertyAttributesAsWritten() & OwnershipMask) return true;
+
+  // If this is a readwrite property in a class extension that refines
+  // a readonly property in the original class definition, check it as
+  // well.
+
+  // If it's a readonly property, we're not interested.
+  if (Prop->isReadOnly()) return false;
+
+  // Is it declared in an extension?
+  auto Category = dyn_cast<ObjCCategoryDecl>(Prop->getDeclContext());
+  if (!Category || !Category->IsClassExtension()) return false;
+
+  // Find the corresponding property in the primary class definition.
+  auto OrigClass = Category->getClassInterface();
+  for (auto Found : OrigClass->lookup(Prop->getDeclName())) {
+    if (ObjCPropertyDecl *OrigProp = dyn_cast<ObjCPropertyDecl>(Found))
+      return OrigProp->getPropertyAttributesAsWritten() & OwnershipMask;
+  }
+
+  return false;
+}
+
 /// ActOnPropertyImplDecl - This routine performs semantic checks and
 /// builds the AST node for a property implementation declaration; declared
 /// as \@synthesize or \@dynamic.
@@ -1029,7 +1098,7 @@ Decl *Sema::ActOnPropertyImplDecl(Scope *S,
 
         // It's an error if we have to do this and the user didn't
         // explicitly write an ownership attribute on the property.
-        if (!property->hasWrittenStorageAttribute() &&
+        if (!hasWrittenStorageAttribute(property) &&
             !(kind & ObjCPropertyDecl::OBJC_PR_strong)) {
           Diag(PropertyDiagLoc,
                diag::err_arc_objc_property_default_assign_on_object);
@@ -1364,7 +1433,7 @@ Sema::DiagnosePropertyMismatch(ObjCPropertyDecl *Property,
   // Check for nonatomic; note that nonatomic is effectively
   // meaningless for readonly properties, so don't diagnose if the
   // atomic property is 'readonly'.
-  checkAtomicPropertyMismatch(*this, SuperProperty, Property);
+  checkAtomicPropertyMismatch(*this, SuperProperty, Property, false);
   if (Property->getSetterName() != SuperProperty->getSetterName()) {
     Diag(Property->getLocation(), diag::warn_property_attribute)
       << Property->getDeclName() << "setter" << inheritedName;