Implement type checking of Objective-C property attributes.

 - readonly and readwrite are mutually exclusive.
 - assign, copy, and retain are mutually exclusive.
 - copy and retain are invalid on non-object types.
 - Warn about using default 'assign' property on object types
   (attempting to follow gcc behavior).

llvm-svn: 56507

5 files changed, 109 insertions, 10 deletions

diff --git a/clang/include/clang/Basic/DiagnosticKinds.def b/clang/include/clang/Basic/DiagnosticKinds.def
index 7326fb1207d..d266adb7c7d 100644
--- a/clang/include/clang/Basic/DiagnosticKinds.def
+++ b/clang/include/clang/Basic/DiagnosticKinds.def
@@ -421,6 +421,14 @@ DIAG(err_objc_expected_equal, ERROR,
      "setter/getter expects '=' followed by name")
 DIAG(err_objc_expected_property_attr, ERROR,
      "unknown property attribute detected")
+DIAG(err_objc_property_attr_mutually_exclusive, ERROR,
+     "property attributes '%0' and '%1' are mutually exclusive")
+DIAG(warn_objc_property_no_assignment_attribute, WARNING,
+     "no 'assign', 'retain', or 'copy' attribute is specified - 'assign' is assumed")
+DIAG(warn_objc_property_default_assign_on_object, WARNING,
+     "default property attribute 'assign' not appropriate for non-gc object")
+DIAG(err_objc_property_requires_object, ERROR,
+     "property with '%0' attribute must be of object type")
 DIAG(err_objc_protocol_required, ERROR,
      "@required may be specified in protocols only")
 DIAG(err_objc_protocol_optional, ERROR,
diff --git a/clang/lib/Sema/Sema.h b/clang/lib/Sema/Sema.h
index 4fbd7351136..ef5954b6294 100644
--- a/clang/lib/Sema/Sema.h
+++ b/clang/lib/Sema/Sema.h
@@ -723,6 +723,12 @@ public:
                                        unsigned NumProtocols,
                                    llvm::SmallVectorImpl<DeclTy *> &Protocols);
   
+  /// Ensure attributes are consistent with type. 
+  /// \param [in, out] Attributes The attributes to check; they will
+  /// be modified to be consistent with \arg PropertyTy.
+  void CheckObjCPropertyAttributes(QualType PropertyTy, 
+                                   SourceLocation Loc,
+                                   unsigned &Attributes);
   void DiagnosePropertyMismatch(ObjCPropertyDecl *Property, 
                                 ObjCPropertyDecl *SuperProperty,
                                 const char *Name);
diff --git a/clang/lib/Sema/SemaDeclObjC.cpp b/clang/lib/Sema/SemaDeclObjC.cpp
index 240d760b755..dbb3cf22ad3 100644
--- a/clang/lib/Sema/SemaDeclObjC.cpp
+++ b/clang/lib/Sema/SemaDeclObjC.cpp
@@ -1038,6 +1038,68 @@ Sema::DeclTy *Sema::ActOnMethodDeclaration(
   return ObjCMethod;
 }
 
+void Sema::CheckObjCPropertyAttributes(QualType PropertyTy, 
+                                       SourceLocation Loc,
+                                       unsigned &Attributes) {
+  // FIXME: Improve the reported location.
+
+  // readonly and readwrite conflict.
+  if ((Attributes & ObjCDeclSpec::DQ_PR_readonly) &&
+      (Attributes & ObjCDeclSpec::DQ_PR_readwrite)) {
+    Diag(Loc, diag::err_objc_property_attr_mutually_exclusive,
+         "readonly", "readwrite");
+    Attributes ^= ObjCDeclSpec::DQ_PR_readonly;
+  }
+
+  // Check for copy or retain on non-object types.
+  if ((Attributes & (ObjCDeclSpec::DQ_PR_copy | ObjCDeclSpec::DQ_PR_retain)) &&
+      !Context.isObjCObjectPointerType(PropertyTy)) {
+    Diag(Loc, diag::err_objc_property_requires_object,
+         Attributes & ObjCDeclSpec::DQ_PR_copy ? "copy" : "retain");
+    Attributes &= ~(ObjCDeclSpec::DQ_PR_copy | ObjCDeclSpec::DQ_PR_retain);
+  }
+
+  // Check for more than one of { assign, copy, retain }.
+  if (Attributes & ObjCDeclSpec::DQ_PR_assign) {
+    if (Attributes & ObjCDeclSpec::DQ_PR_copy) {
+      Diag(Loc, diag::err_objc_property_attr_mutually_exclusive,
+           "assign", "copy");
+      Attributes ^= ObjCDeclSpec::DQ_PR_copy;
+    } 
+    if (Attributes & ObjCDeclSpec::DQ_PR_retain) {
+      Diag(Loc, diag::err_objc_property_attr_mutually_exclusive,
+           "assign", "retain");
+      Attributes ^= ObjCDeclSpec::DQ_PR_retain;
+    }
+  } else if (Attributes & ObjCDeclSpec::DQ_PR_copy) {
+    if (Attributes & ObjCDeclSpec::DQ_PR_retain) {
+      Diag(Loc, diag::err_objc_property_attr_mutually_exclusive,
+           "copy", "retain");
+      Attributes ^= ObjCDeclSpec::DQ_PR_retain;
+    }
+  }
+
+  // Warn if user supplied no assignment attribute, property is
+  // readwrite, and this is an object type.
+  if (!(Attributes & (ObjCDeclSpec::DQ_PR_assign | ObjCDeclSpec::DQ_PR_copy |
+                      ObjCDeclSpec::DQ_PR_retain)) &&
+      !(Attributes & ObjCDeclSpec::DQ_PR_readonly) &&
+      Context.isObjCObjectPointerType(PropertyTy)) {
+    // Skip this warning in gc-only mode.
+    if (getLangOptions().getGCMode() != LangOptions::GCOnly)    
+      Diag(Loc, diag::warn_objc_property_no_assignment_attribute);
+
+    // If non-gc code warn that this is likely inappropriate.
+    if (getLangOptions().getGCMode() == LangOptions::NonGC)
+      Diag(Loc, diag::warn_objc_property_default_assign_on_object);
+    
+    // FIXME: Implement warning dependent on NSCopying being
+    // implemented. See also:
+    // <rdar://5168496&4855821&5607453&5096644&4947311&5698469&4947014&5168496>
+    // (please trim this list while you are at it).
+  }
+}
+
 Sema::DeclTy *Sema::ActOnProperty(Scope *S, SourceLocation AtLoc, 
                                   FieldDeclarator &FD,
                                   ObjCDeclSpec &ODS,
@@ -1045,6 +1107,11 @@ Sema::DeclTy *Sema::ActOnProperty(Scope *S, SourceLocation AtLoc,
                                   Selector SetterSel,
                                   tok::ObjCKeywordKind MethodImplKind) {
   QualType T = GetTypeForDeclarator(FD.D, S);
+  unsigned Attributes = ODS.getPropertyAttributes();
+
+  // May modify Attributes.
+  CheckObjCPropertyAttributes(T, AtLoc, Attributes);
+
   ObjCPropertyDecl *PDecl = ObjCPropertyDecl::Create(Context, AtLoc, 
                                                      FD.D.getIdentifier(), T);
   // Regardless of setter/getter attribute, we save the default getter/setter
@@ -1052,28 +1119,28 @@ Sema::DeclTy *Sema::ActOnProperty(Scope *S, SourceLocation AtLoc,
   PDecl->setGetterName(GetterSel);
   PDecl->setSetterName(SetterSel);
   
-  if (ODS.getPropertyAttributes() & ObjCDeclSpec::DQ_PR_readonly)
+  if (Attributes & ObjCDeclSpec::DQ_PR_readonly)
     PDecl->setPropertyAttributes(ObjCPropertyDecl::OBJC_PR_readonly);
   
-  if (ODS.getPropertyAttributes() & ObjCDeclSpec::DQ_PR_getter)
+  if (Attributes & ObjCDeclSpec::DQ_PR_getter)
     PDecl->setPropertyAttributes(ObjCPropertyDecl::OBJC_PR_getter);
   
-  if (ODS.getPropertyAttributes() & ObjCDeclSpec::DQ_PR_setter)
+  if (Attributes & ObjCDeclSpec::DQ_PR_setter)
     PDecl->setPropertyAttributes(ObjCPropertyDecl::OBJC_PR_setter);
   
-  if (ODS.getPropertyAttributes() & ObjCDeclSpec::DQ_PR_assign)
+  if (Attributes & ObjCDeclSpec::DQ_PR_assign)
     PDecl->setPropertyAttributes(ObjCPropertyDecl::OBJC_PR_assign);
   
-  if (ODS.getPropertyAttributes() & ObjCDeclSpec::DQ_PR_readwrite)
+  if (Attributes & ObjCDeclSpec::DQ_PR_readwrite)
     PDecl->setPropertyAttributes(ObjCPropertyDecl::OBJC_PR_readwrite);
   
-  if (ODS.getPropertyAttributes() & ObjCDeclSpec::DQ_PR_retain)
+  if (Attributes & ObjCDeclSpec::DQ_PR_retain)
     PDecl->setPropertyAttributes(ObjCPropertyDecl::OBJC_PR_retain);
   
-  if (ODS.getPropertyAttributes() & ObjCDeclSpec::DQ_PR_copy)
+  if (Attributes & ObjCDeclSpec::DQ_PR_copy)
     PDecl->setPropertyAttributes(ObjCPropertyDecl::OBJC_PR_copy);
   
-  if (ODS.getPropertyAttributes() & ObjCDeclSpec::DQ_PR_nonatomic)
+  if (Attributes & ObjCDeclSpec::DQ_PR_nonatomic)
     PDecl->setPropertyAttributes(ObjCPropertyDecl::OBJC_PR_nonatomic);
   
   if (MethodImplKind == tok::objc_required)
@@ -1116,7 +1183,7 @@ Sema::DeclTy *Sema::ActOnPropertyImplDecl(SourceLocation AtLoc,
     // Look for this property declaration in the @implementation's @interface
     property = IDecl->FindPropertyDeclaration(PropertyId);
     if (!property) {
-       Diag(PropertyLoc, diag::error_bad_property_decl, IDecl->getName());
+      Diag(PropertyLoc, diag::error_bad_property_decl, IDecl->getName());
       return 0;
     }
   }
diff --git a/clang/test/SemaObjC/property-1.m b/clang/test/SemaObjC/property-1.m
index 6cdf8de0211..26411051623 100644
--- a/clang/test/SemaObjC/property-1.m
+++ b/clang/test/SemaObjC/property-1.m
@@ -6,7 +6,7 @@
 	int name;
 }
 @property int d1;
-@property id  prop_id;
+@property id  prop_id; // expected-warning {{no 'assign', 'retain', or 'copy' attribute is specified - 'assign' is assumed}}, expected-warning {{default property attribute 'assign' not appropriate for non-gc object}}
 @property int name;
 @end
 
diff --git a/clang/test/SemaObjC/property-10.m b/clang/test/SemaObjC/property-10.m
new file mode 100644
index 00000000000..69a5ae80fe3
--- /dev/null
+++ b/clang/test/SemaObjC/property-10.m
@@ -0,0 +1,18 @@
+// RUN: clang -fsyntax-only -verify %s
+
+// Check property attribute consistency.
+
+@interface I0
+@property(readonly, readwrite) int p0; // expected-error {{property attributes 'readonly' and 'readwrite' are mutually exclusive}}
+
+@property(retain) int p1; // expected-error {{property with 'retain' attribute must be of object type}}
+
+@property(copy) int p2; // expected-error {{property with 'copy' attribute must be of object type}}
+
+@property(assign, copy) id p3_0; // expected-error {{property attributes 'assign' and 'copy' are mutually exclusive}} 
+@property(assign, retain) id p3_1; // expected-error {{property attributes 'assign' and 'retain' are mutually exclusive}} 
+@property(copy, retain) id p3_2; // expected-error {{property attributes 'copy' and 'retain' are mutually exclusive}} 
+@property(assign, copy, retain) id p3_3; // expected-error {{property attributes 'assign' and 'copy' are mutually exclusive}}, expected-error {{property attributes 'assign' and 'retain' are mutually exclusive}} 
+
+@property id p4; // expected-warning {{no 'assign', 'retain', or 'copy' attribute is specified - 'assign' is assumed}}, expected-warning {{default property attribute 'assign' not appropriate for non-gc object}}
+@end