Special case ObjCPropertyDecl for printing

[dgoldman]

Summary

ObjCPropertyDecl should use the category interface as a context similar to what is done for methods.

Previously category methods would be printed as ::property; now they are printed as Class::property.

[dgoldman]

• Add test

[sammccall]

This is definitely an improvement, though I don't know if it's *right*. @akyrtzi, thoughts?

unittests/AST/NamedDeclPrinterTest.cpp

206

One concern is that similarly-named members in extensions of different classes will have the same name (whereas categories have names, so the collision is less likely).
It's also harder to go from printed name -> source code.

Obj::(class extension)::property might be a more useful QName (including *also* the class name).
But I don't know obj-c well, would be good if Apple folks could chime in.

(class extension)::property is definitely an improvement over the current ::property, though.

[dgoldman]

In D56924#1367450, @sammccall wrote:

This is definitely an improvement, though I don't know if it's *right*. @akyrtzi, thoughts?

Yeah, I'm not sure what the desired behavior is. When writing up the test I noticed there is a Obj::property, presumably for the auto-generated getter method (which I then filtered out via objcPropertyDecl).

Where is the qualified name used?

[arphaman]

Do you know if this have an effect on the output of completion results or other tooling-based output?

A couple of requests:

• This behavior should be controlled by a printing policy flag SupressUnwrittenScope (specifically for the '::(class extension)').
• I also agree with Sam's comment. The property should still be qualified by the class name as well, e.g. Obj::(class extension)::property.

[dgoldman]

• Output class scope for ObjCPropertyDecl

[dgoldman]

In D56924#1379342, @arphaman wrote:

Do you know if this have an effect on the output of completion results or other tooling-based output?

A couple of requests:

• This behavior should be controlled by a printing policy flag SupressUnwrittenScope (specifically for the '::(class extension)').
• I also agree with Sam's comment. The property should still be qualified by the class name as well, e.g. Obj::(class extension)::property.

It's possible - clangd for instance was crashing because of the current behavior: ::property instead of Obj::property or (class extension)::property.

• Added although see below
• I've modified this to be more in line with the current handling of Objc methods, but this no longer outputs the "(class extension)", do you think it should output both?

[dgoldman]

ping

[dgoldman]

ping, looking to get this in to fix a clangd assertion failure

[arphaman]

Please add a test that covers the '(class extension)' output as well.

[dgoldman]

• Remove (class extension) as it's no longer needed

[dgoldman]

In D56924#1407836, @arphaman wrote:

Please add a test that covers the '(class extension)' output as well.

Removed the (class extension) output as the property getter if statement should now handle this.

[dgoldman]

friendly ping, think this is good to go now

[dgoldman]

ping

[dgoldman]

ping

[dgoldman]

ping

[arphaman]

Sorry, I missed the pings. It LGTM.

[dgoldman]

Rebase

[dgoldman]

Closed via rL357720

[gribozavr]

lib/AST/Decl.cpp

1543

Like you said in a private conversation, yes, support for ObjCIvarDecl also seems necessary.

unittests/AST/NamedDeclPrinterTest.cpp

220

I don't think that Obj::property is the preferred syntax. Obj.property? I'd want a review from someone from Apple to confirm.

[dexonsmith]

unittests/AST/NamedDeclPrinterTest.cpp

220

@arphaman, @jkorous, or @benlangmuir: can you check on this?

[dgoldman]

unittests/AST/NamedDeclPrinterTest.cpp

220

For more context here: I'm seeing similar issues for Objective-C instance variables (they can be declared inside a class extension --> meaning the Decl has no name --> clangd fails with an assertion that the name shouldn't be empty).

It seems to me that for Objective-C we could do something like Obj(Extension::property or alternatively Obj(Extension).property and Obj(class extension)->_instanceVar, but I'm not sure about what relies upon this current behavior (if anything).

lib/AST/Decl.cpp

 void NamedDecl::printQualifiedName(raw_ostream &OS) const {
   printQualifiedName(OS, getASTContext().getPrintingPolicy());
 }
 
 void NamedDecl::printQualifiedName(raw_ostream &OS,
                                    const PrintingPolicy &P) const {
   const DeclContext *Ctx = getDeclContext();
 
-  // For ObjC methods, look through categories and use the interface as context.
+  // For ObjC methods and properties, look through categories and use the
+  // interface as context.
   if (auto *MD = dyn_cast<ObjCMethodDecl>(this))
     if (auto *ID = MD->getClassInterface())
       Ctx = ID;
+  if (auto *PD = dyn_cast<ObjCPropertyDecl>(this)) {
+    if (auto *MD = PD->getGetterMethodDecl())
+      if (auto *ID = MD->getClassInterface())
+        Ctx = ID;
+  }
 
   if (Ctx->isFunctionOrMethod()) {
     printName(OS);
     return;
   }
 
   using ContextsTy = SmallVector<const DeclContext *, 8>;
   ContextsTy Contexts;

unittests/AST/NamedDeclPrinterTest.cpp

PrintedWrittenNamedDeclCXX11Matches(StringRef Code, StringRef DeclName,
   return PrintedNamedDeclMatches(Code,
                                  Args,
                                  /*SuppressUnwrittenScope*/ true,
                                  namedDecl(hasName(DeclName)).bind("id"),
                                  ExpectedPrinted,
                                  "input.cc");
 }
 
+::testing::AssertionResult
+PrintedWrittenPropertyDeclObjCMatches(StringRef Code, StringRef DeclName,
+                                   StringRef ExpectedPrinted) {
+  std::vector<std::string> Args{"-std=c++11", "-xobjective-c++"};
+  return PrintedNamedDeclMatches(Code,
+                                 Args,
+                                 /*SuppressUnwrittenScope*/ true,
+                                 objcPropertyDecl(hasName(DeclName)).bind("id"),
+                                 ExpectedPrinted,
+                                 "input.m");
+}
+
 } // unnamed namespace
 
 TEST(NamedDeclPrinter, TestNamespace1) {
   ASSERT_TRUE(PrintedNamedDeclCXX98Matches(
     "namespace { int A; }",
     "A",
     "(anonymous namespace)::A"));
 }

 }
 
 TEST(NamedDeclPrinter, TestLinkageInNamespace) {
   ASSERT_TRUE(PrintedWrittenNamedDeclCXX11Matches(
     "namespace X { extern \"C\" { int A; } }",
     "A",
     "X::A"));
 }
+
+TEST(NamedDeclPrinter, TestObjCClassExtension) {
+  ASSERT_TRUE(PrintedWrittenPropertyDeclObjCMatches(
+    R"(
+      @interface Obj
+      @end
+
+      @interface Obj ()
+      @property(nonatomic) int property;
+      @end
+    )",
+    "property",
+    "Obj::property"));
+}
+
+TEST(NamedDeclPrinter, TestObjCClassExtensionWithGetter) {
+  ASSERT_TRUE(PrintedWrittenPropertyDeclObjCMatches(
+    R"(
+      @interface Obj
+      @end
+
+      @interface Obj ()
+      @property(nonatomic, getter=myPropertyGetter) int property;
+      @end
+    )",
+    "property",
+    "Obj::property"));
+}