Clean up ObjCPropertyDecl printing

[dgoldman]

Summary

• @property(attr, attr2) instead of @property ( attr,attr2 ).
• @property NSObject *foo instead of @property NSObject * foo
• Change priority of attributes (see code/comments inline).
• Support for printing weak and unsafe_unretained attributes.

[benlangmuir]

@arphaman @akyrtzi could you take a look at this?

[benlangmuir]

@property(attr, attr2) instead of @property ( attr,attr2 ).

The style I see most often is @property (attr, attr2), which is in between those two.

[dgoldman]

Friendly ping

[jkorous]

Hi David,
I am just wondering - while you're here would you mind adding couple more tests? It would be great to have a test for each attribute.

Also, what do you think about Ben's suggestion? I think it would be nice to be consistent with the style Objective-C documentation uses:
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/EncapsulatingData/EncapsulatingData.html

[dgoldman]

In D57965#1417361, @jkorous wrote:

Hi David,
I am just wondering - while you're here would you mind adding couple more tests? It would be great to have a test for each attribute.

Also, what do you think about Ben's suggestion? I think it would be nice to be consistent with the style Objective-C documentation uses:
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/EncapsulatingData/EncapsulatingData.html

Sure, I can add a few more tests.

Hmm, I've seen it more often the other way around; to be honest I think the docs that you've linked to are outdated. See https://developer.apple.com/documentation/uikit/uiapplication/1622975-sharedapplication?language=objc for an example of what I've seen.

I do think that it would be nice to have X *prop instead of X * prop though, let me try adding that.

[dgoldman]

• Add more tests and improved printing of pointers

[dgoldman]

• Fix broken test by last change

[dgoldman]

friendly ping

[jkorous]

LGTM. Thanks for working on this!

[dgoldman]

• Rebase

lib/AST/DeclPrinter.cpp

 
 void DeclPrinter::VisitObjCCompatibleAliasDecl(ObjCCompatibleAliasDecl *AID) {
   Out << "@compatibility_alias " << *AID
       << ' ' << *AID->getClassInterface() << ";\n";
 }
 
 /// PrintObjCPropertyDecl - print a property declaration.
 ///
+/// Print attributes in the following order:
+/// - class
+/// - nonatomic | atomic
+/// - assign | retain | strong | copy | weak | unsafe_unretained
+/// - readwrite | readonly
+/// - getter & setter
+/// - nullability
 void DeclPrinter::VisitObjCPropertyDecl(ObjCPropertyDecl *PDecl) {
   if (PDecl->getPropertyImplementation() == ObjCPropertyDecl::Required)
     Out << "@required\n";
   else if (PDecl->getPropertyImplementation() == ObjCPropertyDecl::Optional)
     Out << "@optional\n";
 
   QualType T = PDecl->getType();
 
   Out << "@property";
   if (PDecl->getPropertyAttributes() != ObjCPropertyDecl::OBJC_PR_noattr) {
     bool first = true;
-    Out << " (";
-    if (PDecl->getPropertyAttributes() &
-        ObjCPropertyDecl::OBJC_PR_readonly) {
-      Out << (first ? ' ' : ',') << "readonly";
+    Out << "(";
+    if (PDecl->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_class) {
+      Out << (first ? "" : ", ") << "class";
       first = false;
     }
 
-    if (PDecl->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_getter) {
-      Out << (first ? ' ' : ',') << "getter = ";
-      PDecl->getGetterName().print(Out);
+    if (PDecl->getPropertyAttributes() &
+        ObjCPropertyDecl::OBJC_PR_nonatomic) {
+      Out << (first ? "" : ", ") << "nonatomic";
       first = false;
     }
-    if (PDecl->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_setter) {
-      Out << (first ? ' ' : ',') << "setter = ";
-      PDecl->getSetterName().print(Out);
+    if (PDecl->getPropertyAttributes() &
+        ObjCPropertyDecl::OBJC_PR_atomic) {
+      Out << (first ? "" : ", ") << "atomic";
       first = false;
     }
 
     if (PDecl->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_assign) {
-      Out << (first ? ' ' : ',') << "assign";
-      first = false;
-    }
-
-    if (PDecl->getPropertyAttributes() &
-        ObjCPropertyDecl::OBJC_PR_readwrite) {
-      Out << (first ? ' ' : ',') << "readwrite";
+      Out << (first ? "" : ", ") << "assign";
       first = false;
     }
-
     if (PDecl->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_retain) {
-      Out << (first ? ' ' : ',') << "retain";
+      Out << (first ? "" : ", ") << "retain";
       first = false;
     }
 
     if (PDecl->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_strong) {
-      Out << (first ? ' ' : ',') << "strong";
+      Out << (first ? "" : ", ") << "strong";
       first = false;
     }
-
     if (PDecl->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_copy) {
-      Out << (first ? ' ' : ',') << "copy";
+      Out << (first ? "" : ", ") << "copy";
+      first = false;
+    }
+    if (PDecl->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak) {
+      Out << (first ? "" : ", ") << "weak";
+      first = false;
+    }
+    if (PDecl->getPropertyAttributes()
+        & ObjCPropertyDecl::OBJC_PR_unsafe_unretained) {
+      Out << (first ? "" : ", ") << "unsafe_unretained";
       first = false;
     }
 
     if (PDecl->getPropertyAttributes() &
-        ObjCPropertyDecl::OBJC_PR_nonatomic) {
-      Out << (first ? ' ' : ',') << "nonatomic";
+        ObjCPropertyDecl::OBJC_PR_readwrite) {
+      Out << (first ? "" : ", ") << "readwrite";
       first = false;
     }
     if (PDecl->getPropertyAttributes() &
-        ObjCPropertyDecl::OBJC_PR_atomic) {
-      Out << (first ? ' ' : ',') << "atomic";
+        ObjCPropertyDecl::OBJC_PR_readonly) {
+      Out << (first ? "" : ", ") << "readonly";
+      first = false;
+    }
+
+    if (PDecl->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_getter) {
+      Out << (first ? "" : ", ") << "getter = ";
+      PDecl->getGetterName().print(Out);
+      first = false;
+    }
+    if (PDecl->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_setter) {
+      Out << (first ? "" : ", ") << "setter = ";
+      PDecl->getSetterName().print(Out);
       first = false;
     }
 
     if (PDecl->getPropertyAttributes() &
         ObjCPropertyDecl::OBJC_PR_nullability) {
       if (auto nullability = AttributedType::stripOuterNullability(T)) {
         if (*nullability == NullabilityKind::Unspecified &&
             (PDecl->getPropertyAttributes() &
                ObjCPropertyDecl::OBJC_PR_null_resettable)) {
-          Out << (first ? ' ' : ',') << "null_resettable";
+          Out << (first ? "" : ", ") << "null_resettable";
         } else {
-          Out << (first ? ' ' : ',')
+          Out << (first ? "" : ", ")
               << getNullabilitySpelling(*nullability, true);
         }
         first = false;
       }
     }
 
-    if (PDecl->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_class) {
-      Out << (first ? ' ' : ',') << "class";
-      first = false;
-    }
-
     (void) first; // Silence dead store warning due to idiomatic code.
-    Out << " )";
+    Out << ")";
   }
   Out << ' ' << PDecl->getASTContext().getUnqualifiedObjCPointerType(T).
                   getAsString(Policy) << ' ' << *PDecl;
   if (Policy.PolishForDeclaration)
     Out << ';';
 }
 
 void DeclPrinter::VisitObjCPropertyImplDecl(ObjCPropertyImplDecl *PID) {

test/Index/comment-objc-decls.m

 @property (copy, nonnull) id PropertyMyProto;
 /**
  * \brief ClassMethodMyProto
 */
 + ClassMethodMyProto;
 @end
 // CHECK: <Declaration>@protocol MyProto\n@end</Declaration>
 // CHECK: <Declaration>- (unsigned int)MethodMyProto:(nullable id)anObject inRange:(unsigned int)range;</Declaration>
-// CHECK: <Declaration>@optional\n@property(readwrite, copy, atomic, nonnull) id PropertyMyProto;</Declaration>
+// CHECK: <Declaration>@optional\n@property(atomic, copy, readwrite, nonnull) id PropertyMyProto;</Declaration>
 // CHECK: <Declaration>+ (id)ClassMethodMyProto;</Declaration>
 
 /**
  * \brief NSObject is the root class.
 */
 @interface NSObject {
 /**
  * \brief IvarNSObject

  * \brief PropertyMyClass - This is class's property.
 */
 @property (copy) id PropertyMyClass;
 @end
 // CHECK: <Declaration>@interface MyClass : NSObject &lt;MyProto&gt; {\n    id IvarMyClass;\n}\n@end</Declaration>
 // CHECK: <Declaration>id IvarMyClass</Declaration>
 // CHECK: <Declaration>- (id)MethodMyClass;</Declaration>
 // CHECK: <Declaration>+ (id)ClassMethodMyClass;</Declaration>
-// CHECK: <Declaration>@property(readwrite, copy, atomic) id PropertyMyClass;</Declaration
+// CHECK: <Declaration>@property(atomic, copy, readwrite) id PropertyMyClass;</Declaration
 
 /**
  * \brief - This is class extension of MyClass
 */
 @interface MyClass()
 {
 /**
  * \brief IvarMyClassExtension - IvarMyClassExtension private to class extension

 
 /**
  * \brief PropertyMyClassCategory - This is class's private property.
 */
 @property (copy) id PropertyMyClassCategory;
 @end
 // CHECK: <Declaration>@interface MyClass (Category)\n@end</Declaration>
 // CHECK: <Declaration>- (void)MethodMyClassCategory;</Declaration>
-// CHECK: <Declaration>@property(readwrite, copy, atomic) id PropertyMyClassCategory;</Declaration>
+// CHECK: <Declaration>@property(atomic, copy, readwrite) id PropertyMyClassCategory;</Declaration>
 // CHECK: <Declaration>- (id)PropertyMyClassCategory;</Declaration>
 // CHECK: <Declaration>- (void)setPropertyMyClassCategory:(id)arg;</Declaration>
 
 /// @implementation's
 
 /**
  * \brief implementation of MyClass class.
 */

test/Index/comment-unqualified-objc-pointer.m

//! This is the name.
   NSString *Name;
 }
 //! This is WithLabel comment.
 - (NSString *)WithLabel:(NSString * const)label;
 // CHECK: <Declaration>- (NSString *)WithLabel:(NSString *const)label;</Declaration> 
 
 //! This is a property to get the Name.
 @property (copy) NSString *Name;
-// CHECK: <Declaration>@property(readwrite, copy, atomic) NSString *Name;</Declaration>
+// CHECK: <Declaration>@property(atomic, copy, readwrite) NSString *Name;</Declaration>
 @end
 
 @implementation NSMutableArray
 {
 //! This is private ivar
   NSString *NickName;
 // CHECK: <Declaration>NSString *NickName</Declaration>
 }
 
 - (NSString *)WithLabel:(NSString * const)label {
     return 0;
 }
 @synthesize Name = Name;
 @end

test/PCH/chain-remap-types.m

 // RUN: %clang_cc1 -emit-pch -x objective-c-header -o %t1 %S/Inputs/chain-remap-types1.h
 // RUN: %clang_cc1 -emit-pch -x objective-c-header -o %t2 %S/Inputs/chain-remap-types2.h -include-pch %t1
 // RUN: %clang_cc1 -include-pch %t2 -fsyntax-only -verify %s
 // RUN: %clang_cc1 -ast-print -include-pch %t2 %s | FileCheck %s
 // expected-no-diagnostics
 
 // CHECK: @class X;
 // CHECK: struct Y 
-// CHECK: @property ( assign,readwrite,atomic ) X * prop
+// CHECK: @property(atomic, assign, unsafe_unretained, readwrite) X * prop
 // CHECK: void h(X *);
 // CHECK: @interface X(Blah)
 // CHECK: void g(X *);