[clangd][ObjC] Improve support for class properties

[dgoldman]

Summary

Class properties are always implicit short-hands for the getter/setter
class methods.

We need to explicitly visit the interface decl UIColor in UIColor.blueColor,
otherwise we instead show the method decl even while hovering over
UIColor in the expression.

[dgoldman]

• Add FIXMEs

[dgoldman]

clang-tools-extra/clangd/FindTarget.cpp

309–313

Is there a good way to handle this case where the expression can refer to multiple decls?

[sammccall]

clang-tools-extra/clangd/FindTarget.cpp

309–313

TL;DR: yes, fix the AST :-)

The design assumption is that an AST node (specifically: the tokens owned by exactly that AST node and not a descendant of it) refers to one thing. I don't see an easy way to hack around this.

Expressions like this are supposed to be split into multiple AST nodes. But ObjC seems to be missing AST nodes in a bunch of places (Protocol lists is another).

e.g. in C++ equivalent DeclRefExpr UIColor::blackColor there'd be both a NestedNameSpecifier for UIColor:: and a TagTypeLoc for UIColor. And objc [Test foo] has an ObjCInterfaceTypeLoc for Test.

---

I'm not sure this is the right place for this comment, the problem isn't what VisitObjCPropertyRefExpr is doing, the issue is that the caller is passing the wrong kind of node to findTarget() because there's no right one.

769

comment here:
// There's no contained TypeLoc node for the receiver type.

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

1056–1057

As in the other patch, we should keep these large unrelated reformattings out of substantive patches.

[dgoldman]

clang-tools-extra/clangd/FindTarget.cpp

309–313

That seems a bit invasive - is it really "fixing" the AST - is there anything wrong with the current AST design or rather any notable improvements by adding more nodes to the AST beside this one use case here?

It seems like the design could be adapted if we had a SourceLocation hint here (from the caller) to disambiguate which one we mean, but I'm guessing not all of the callers/users of this would have one?

[dgoldman]

Rebase on top of main

[dgoldman]

Fixes for review

[sammccall]

clang-tools-extra/clangd/FindTarget.cpp

309–313

That seems a bit invasive

I'm not sure it actually requires any changes to AST classes, just to RecursiveASTVisitor to wrap the getClassReceiverType() & getReceiverLocation() into n ObjCInterfaceTypeLoc (which is a value object) and visit it.

is it really "fixing" the AST

It's inconsistent with the design of the rest of the AST.
There are very few other cases where AST nodes have this kind of compound meaning rather than being to split into sub-nodes.
(In fact ObjC protocol lists are the only example I can think of. I suspect this dates back to early clang days, maybe before the patterns became established)
And similarly, there are few/no places where types are referenced that don't have a TypeLoc node traversed by RecursiveASTVisitor.

is there anything wrong with the current AST design or rather any notable improvements by adding more nodes to the AST beside this one use case here?

I guess the use case here is "precisely communicating a selection in an IDE".
Expand-selection features and show-ast-structure features don't work in clangd for a similar reason.

Outside clangd I'm less familiar of course! Matchers, clang-tidy checks and refactorings spring to mind.
For instance, if you want to write a tool to rename/split an ObjC class, then something like typeLoc(loc(asString("FooClass"))) would match all usages... except this one, so your tool would have a bug.

[dgoldman]

clang-tools-extra/clangd/FindTarget.cpp

309–313

I'm not sure it actually requires any changes to AST classes, just to RecursiveASTVisitor to wrap the getClassReceiverType() & getReceiverLocation() into n ObjCInterfaceTypeLoc (which is a value object) and visit it.

Thanks, I'll look into that in a follow up - I'd imagine it could affect lots of other things but I'm not sure.

Outside clangd I'm less familiar of course! Matchers, clang-tidy checks and refactorings spring to mind.
For instance, if you want to write a tool to rename/split an ObjC class, then something like typeLoc(loc(asString("FooClass"))) would match all usages... except this one, so your tool would have a bug.

Interesting, I wonder how Xcode works with this (e.g. for refactorings), I'll ask around to see if anyone has suggestions/further context here.

clang-tools-extra/clangd/FindTarget.cpp

struct Visitor : public ConstStmtVisitor<Visitor> {
       }
       void VisitObjCIvarRefExpr(const ObjCIvarRefExpr *OIRE) {
         Outer.add(OIRE->getDecl(), Flags);
       }
       void VisitObjCMessageExpr(const ObjCMessageExpr *OME) {
         Outer.add(OME->getMethodDecl(), Flags);
       }
       void VisitObjCPropertyRefExpr(const ObjCPropertyRefExpr *OPRE) {
+        // FIXME: We miss visiting the class receiver if one exists, which
+        // means we skip the corresponding ObjCInterfaceDecl ref since it
+        // doesn't have a corresponding node.
         if (OPRE->isExplicitProperty())
           Outer.add(OPRE->getExplicitProperty(), Flags);
         else {
           if (OPRE->isMessagingGetter())
             Outer.add(OPRE->getImplicitPropertyGetter(), Flags);
           if (OPRE->isMessagingSetter())
             Outer.add(OPRE->getImplicitPropertySetter(), Flags);
         }
       }
       void VisitObjCProtocolExpr(const ObjCProtocolExpr *OPE) {
struct Visitor : ConstStmtVisitor<Visitor> {
     void VisitSizeOfPackExpr(const SizeOfPackExpr *E) {
       Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
                                   E->getPackLoc(),
                                   /*IsDecl=*/false,
                                   {E->getPack()}});
     }
 
     void VisitObjCPropertyRefExpr(const ObjCPropertyRefExpr *E) {
+      // There's no contained TypeLoc node for a class receiver type.
+      if (E->isClassReceiver()) {
+        Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+                                    E->getReceiverLocation(),
+                                    /*IsDecl=*/false,
+                                    {E->getClassReceiver()}});
+      }
       Refs.push_back(ReferenceLoc{
           NestedNameSpecifierLoc(), E->getLocation(),
           /*IsDecl=*/false,
           // Select the getter, setter, or @property depending on the call.
           explicitReferenceTargets(DynTypedNode::create(*E), {}, Resolver)});
     }
 
     void VisitObjCMessageExpr(const ObjCMessageExpr *E) {

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

Code = R"cpp(
     @protocol Foo
     @end
     @protocol Bar
     @end
     void test(C<[[Foo]], Bar> *p);
   )cpp";
   // FIXME: We currently can't disambiguate between multiple protocols.
   EXPECT_DECLS("ObjCObjectTypeLoc", "@protocol Foo", "@protocol Bar");
+
+  Code = R"cpp(
+    @interface Foo
+    + (id)sharedInstance;
+    @end
+    @implementation Foo
+    + (id)sharedInstance { return 0; }
+    @end
+    void test() {
+      id value = [[Foo]].sharedInstance;
+    }
+  )cpp";
+  // FIXME: We currently can't identify the interface here.
+  EXPECT_DECLS("ObjCPropertyRefExpr", "+ (id)sharedInstance");
+
+  Code = R"cpp(
+    @interface Foo
+    + (id)sharedInstance;
+    @end
+    @implementation Foo
+    + (id)sharedInstance { return 0; }
+    @end
+    void test() {
+      id value = Foo.[[sharedInstance]];
+    }
+  )cpp";
+  EXPECT_DECLS("ObjCPropertyRefExpr", "+ (id)sharedInstance");
 }
 
 class FindExplicitReferencesTest : public ::testing::Test {
 protected:
   struct AllRefs {
     std::string AnnotatedCode;
     std::string DumpedReferences;
   };
AllRefs annotateReferencesInFoo(llvm::StringRef Code) {
     for (unsigned I = 0; I < Refs.size(); ++I)
       DumpedReferences += std::string(llvm::formatv("{0}: {1}\n", I, Refs[I]));
 
     return AllRefs{std::move(AnnotatedCode), std::move(DumpedReferences)};
   }
 };
 
 TEST_F(FindExplicitReferencesTest, All) {
   std::pair</*Code*/ llvm::StringRef, /*References*/ llvm::StringRef> Cases[] =
       {// Simple expressions.
        {R"cpp(
         int global;
         int func();
         void foo(int param) {
           $0^global = $1^param + $2^func();
         }
         )cpp",
         "0: targets = {global}\n"
#endif
             I *f;
             void foo() {
               $0^f.$1^x.$2^y = 0;
             }
           )cpp",
            "0: targets = {f}\n"
            "1: targets = {I::x}\n"
            "2: targets = {I::setY:}\n"},
+       // Objective-C: class properties
+       {
+           R"cpp(
+            @interface I {}
+            @property(class) I *x;
+            @end
+            id local;
+            void foo() {
+              $0^I.$1^x = 0;
+              $2^local = $3^I.$4^x;
+            }
+          )cpp",
+           "0: targets = {I}\n"
+           "1: targets = {I::setX:}\n"
+           "2: targets = {local}\n"
+           "3: targets = {I}\n"
+           "4: targets = {I::x}\n"},
+       // Objective-C: implicit class properties
+       {
+           R"cpp(
+            @interface I {}
+            +(I*)x;
+            +(void)setX:(I*)x;
+            @end
+            id local;
+            void foo() {
+              $0^I.$1^x = 0;
+              $2^local = $3^I.$4^x;
+            }
+          )cpp",
+           "0: targets = {I}\n"
+           "1: targets = {I::setX:}\n"
+           "2: targets = {local}\n"
+           "3: targets = {I}\n"
+           "4: targets = {I::x}\n"},
        {// Objective-C: methods
         R"cpp(
             @interface I
               -(void) a:(int)x b:(int)y;
             @end
             void foo(I *i) {
               [$0^i $1^a:1 b:2];
             }