[clangd] Improve handling of Objective-C protocols in types

[dgoldman]

Summary

Improve support for Objective-C protocols for types/type locs

[dgoldman]

clang-tools-extra/clangd/FindTarget.cpp

435–441

Let me know if I'm missing something here, not exactly sure what the old comment meant - is there a problem since this ObjCObjectType can refer to multiple protocols?

819

Let me know if you have something else in mind to handle the ObjC case here

917

Doesn't look like that's needed, but if we wanted to decorate the type parameters in ObjCObjectTypeLoc with typeParameter would that just go in SemanticHighlighting.cpp?

[dgoldman]

Friendly ping, not sure if there's something in here and https://reviews.llvm.org/D99975 that I'm missing to properly support the one decl/type loc to multiple references here

[sammccall]

LG apart from a few nits & reverting the whitespace changes to unrelated tests.

clang-tools-extra/clangd/FindTarget.cpp

435–441

Here's the way the AST should ideally look like (and what this comment is wishing for):

ObjCObjectTypeLoc C<Foo>
 - ObjCInterfaceTypeLoc C
 - ObjCProtocolLoc Foo // doesn't actually exist

And targetDecl() would work on the two children, but not the parent.

But there's no AST node for protocols. A piece of cleverness in this patch is getting go-to-definition to work on protocols by saying "if the cursor is on ObjCObjectType itself, then it must be on a protocol rather than the base (otherwise it'd be on the ObjCInterfaceType child)".

But it's a hack, as evidenced by the fact that operations that work on the *range* [[C<Foo>]] will still see the target as <Foo> (literally any of nothing, C, C+Foo would be better).

Nevertheless I think it's probably a net win.

---

Can you leave a comment here like

// There's no child node for just a protocol, so make all the protocols targets.
// But not the base type, which *does* have a child ObjCInterfaceTypeLoc.
// This structure is a hack, but it works well for go-to-definition.

825

this doesn't seem right: we now have a list, the Visit() call may have added things to the list, we need to adjust *all* the added things.

// Add qualifier for the newly-added refs.
for (unsigned I = InitialSize; I < Refs.size(); ++I) {
 // adjust Refs[I]
}

917

That's not something we want to do - ObjCObjectTypeLoc has type arguments, not parameters.
(Not trying to be pedantic with terminology, but the difference is critical here)

e.g. in the function call foo(42) we don't decorate 42 as a parameter, we decorate it as an integer.

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

960–971

can you add a second protocol so we can see the boundaries of this approach?

(I think C<[[Foo]], Bar> should resolve to both @protocol Foo and @protocol Bar)

1046–1047

there are a ton of whitespace changes in this patch, because the version at HEAD isn't clang-format clean :-(

Feel free to format it first in a separate commit, but I'd prefer not to reformat all the tests mixed in with other changes.

[sammccall]

Oops, meant to approve with above comments.

[dgoldman]

Address review comments

[dgoldman]

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

1046–1047

Will do if it hasn't been done already

[dgoldman]

rebase

[dgoldman]

Rebase on top of formatting changes

clang-tools-extra/clangd/FindTarget.cpp

struct Visitor : public TypeVisitor<Visitor> {
       }
       void VisitTemplateTypeParmType(const TemplateTypeParmType *TTPT) {
         Outer.add(TTPT->getDecl(), Flags);
       }
       void VisitObjCInterfaceType(const ObjCInterfaceType *OIT) {
         Outer.add(OIT->getDecl(), Flags);
       }
       void VisitObjCObjectType(const ObjCObjectType *OOT) {
-        // FIXME: ObjCObjectTypeLoc has no children for the protocol list, so
-        // there is no node in id<Foo> that refers to ObjCProtocolDecl Foo.
-        if (OOT->isObjCQualifiedId() && OOT->getNumProtocols() == 1)
-          Outer.add(OOT->getProtocol(0), Flags);
+        // Make all of the protocols targets since there's no child nodes for
+        // protocols. This isn't needed for the base type, which *does* have a
+        // child `ObjCInterfaceTypeLoc`. This structure is a hack, but it works
+        // well for go-to-definition.
+        unsigned NumProtocols = OOT->getNumProtocols();
+        for (unsigned I = 0; I < NumProtocols; I++)
+          Outer.add(OOT->getProtocol(I), Flags);
       }
     };
     Visitor(*this, Flags).Visit(T.getTypePtr());
   }
 
   void add(const NestedNameSpecifier *NNS, RelSet Flags) {
     if (!NNS)
       return;

 }
 
 llvm::SmallVector<ReferenceLoc>
 refInTypeLoc(TypeLoc L, const HeuristicResolver *Resolver) {
   struct Visitor : TypeLocVisitor<Visitor> {
     Visitor(const HeuristicResolver *Resolver) : Resolver(Resolver) {}
 
     const HeuristicResolver *Resolver;
-    llvm::Optional<ReferenceLoc> Ref;
+    llvm::SmallVector<ReferenceLoc> Refs;
 
     void VisitElaboratedTypeLoc(ElaboratedTypeLoc L) {
       // We only know about qualifier, rest if filled by inner locations.
+      size_t InitialSize = Refs.size();
       Visit(L.getNamedTypeLoc().getUnqualifiedLoc());
+      size_t NewSize = Refs.size();
+      // Add qualifier for the newly-added refs.
+      for (unsigned I = InitialSize; I < NewSize; ++I) {
+        ReferenceLoc *Ref = &Refs[I];
         // Fill in the qualifier.
-      if (!Ref)
-        return;
         assert(!Ref->Qualifier.hasQualifier() && "qualifier already set");
         Ref->Qualifier = L.getQualifierLoc();
       }
+    }
 
     void VisitTagTypeLoc(TagTypeLoc L) {
-      Ref = ReferenceLoc{NestedNameSpecifierLoc(),
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
                                   L.getNameLoc(),
                                   /*IsDecl=*/false,
-                         {L.getDecl()}};
+                                  {L.getDecl()}});
     }
 
     void VisitTemplateTypeParmTypeLoc(TemplateTypeParmTypeLoc L) {
-      Ref = ReferenceLoc{NestedNameSpecifierLoc(),
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
                                   L.getNameLoc(),
                                   /*IsDecl=*/false,
-                         {L.getDecl()}};
+                                  {L.getDecl()}});
     }
 
     void VisitTemplateSpecializationTypeLoc(TemplateSpecializationTypeLoc L) {
       // We must ensure template type aliases are included in results if they
       // were written in the source code, e.g. in
       //    template <class T> using valias = vector<T>;
       //    ^valias<int> x;
       // 'explicitReferenceTargets' will return:
       //    1. valias with mask 'Alias'.
       //    2. 'vector<int>' with mask 'Underlying'.
       //  we want to return only #1 in this case.
-      Ref = ReferenceLoc{
+      Refs.push_back(ReferenceLoc{
           NestedNameSpecifierLoc(), L.getTemplateNameLoc(), /*IsDecl=*/false,
           explicitReferenceTargets(DynTypedNode::create(L.getType()),
-                                   DeclRelation::Alias, Resolver)};
+                                   DeclRelation::Alias, Resolver)});
     }
     void VisitDeducedTemplateSpecializationTypeLoc(
         DeducedTemplateSpecializationTypeLoc L) {
-      Ref = ReferenceLoc{
+      Refs.push_back(ReferenceLoc{
           NestedNameSpecifierLoc(), L.getNameLoc(), /*IsDecl=*/false,
           explicitReferenceTargets(DynTypedNode::create(L.getType()),
-                                   DeclRelation::Alias, Resolver)};
+                                   DeclRelation::Alias, Resolver)});
     }
 
     void VisitInjectedClassNameTypeLoc(InjectedClassNameTypeLoc TL) {
-      Ref = ReferenceLoc{NestedNameSpecifierLoc(),
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
                                   TL.getNameLoc(),
                                   /*IsDecl=*/false,
-                         {TL.getDecl()}};
+                                  {TL.getDecl()}});
     }
 
     void VisitDependentTemplateSpecializationTypeLoc(
         DependentTemplateSpecializationTypeLoc L) {
-      Ref = ReferenceLoc{L.getQualifierLoc(), L.getTemplateNameLoc(),
-                         /*IsDecl=*/false,
-                         explicitReferenceTargets(
-                             DynTypedNode::create(L.getType()), {}, Resolver)};
+      Refs.push_back(
+          ReferenceLoc{L.getQualifierLoc(), L.getTemplateNameLoc(),
+                           DynTypedNode::create(L.getType()), {}, Resolver)});
     }
 
     void VisitDependentNameTypeLoc(DependentNameTypeLoc L) {
-      Ref = ReferenceLoc{L.getQualifierLoc(), L.getNameLoc(), /*IsDecl=*/false,
-                         explicitReferenceTargets(
-                             DynTypedNode::create(L.getType()), {}, Resolver)};
+      Refs.push_back(
+          ReferenceLoc{L.getQualifierLoc(), L.getNameLoc(),
+                       /*IsDecl=*/false,
+                           DynTypedNode::create(L.getType()), {}, Resolver)});
     }
 
     void VisitTypedefTypeLoc(TypedefTypeLoc L) {
-      Ref = ReferenceLoc{NestedNameSpecifierLoc(),
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
                                   L.getNameLoc(),
                                   /*IsDecl=*/false,
-                         {L.getTypedefNameDecl()}};
+                                  {L.getTypedefNameDecl()}});
     }
 
     void VisitObjCInterfaceTypeLoc(ObjCInterfaceTypeLoc L) {
-      Ref = ReferenceLoc{NestedNameSpecifierLoc(),
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
                                   L.getNameLoc(),
                                   /*IsDecl=*/false,
-                         {L.getIFaceDecl()}};
+                                  {L.getIFaceDecl()}});
     }
 
-    // FIXME: add references to protocols in ObjCObjectTypeLoc and maybe
-    // ObjCObjectPointerTypeLoc.
+    void VisitObjCObjectTypeLoc(ObjCObjectTypeLoc L) {
+      unsigned NumProtocols = L.getNumProtocols();
+      for (unsigned I = 0; I < NumProtocols; I++) {
+        Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+                                    L.getProtocolLoc(I),
+                                    /*IsDecl=*/false,
+                                    {L.getProtocol(I)}});
+      }
+    }
   };
 
   Visitor V{Resolver};
   V.Visit(L.getUnqualifiedLoc());
-  if (!V.Ref)
-    return {};
-  return {*V.Ref};
+  return V.Refs;
 }
 
 class ExplicitReferenceCollector
     : public RecursiveASTVisitor<ExplicitReferenceCollector> {
 public:
   ExplicitReferenceCollector(llvm::function_ref<void(ReferenceLoc)> Out,
                              const HeuristicResolver *Resolver)
       : Out(Out), Resolver(Resolver) {

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

Code = R"cpp(
     void test([[id<Foo>]] p);
   )cpp";
   EXPECT_DECLS("ObjCObjectTypeLoc", "@protocol Foo");
 
   Code = R"cpp(
     @class C;
     @protocol Foo
     @end
+    void test([[C]]<Foo> *p);
+  )cpp";
+  EXPECT_DECLS("ObjCInterfaceTypeLoc", "@class C;");
+
+  Code = R"cpp(
+    @class C;
+    @protocol Foo
+    @end
     void test(C<[[Foo]]> *p);
   )cpp";
-  // FIXME: there's no AST node corresponding to 'Foo', so we're stuck.
-  EXPECT_DECLS("ObjCObjectTypeLoc");
+  EXPECT_DECLS("ObjCObjectTypeLoc", "@protocol Foo");
+
+  Code = R"cpp(
+    @class C;
+    @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");
 }
 
 class FindExplicitReferencesTest : public ::testing::Test {
 protected:
   struct AllRefs {
     std::string AnnotatedCode;
     std::string 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"
         "1: targets = {param}\n"
         "2: targets = {func}\n"},
        {R"cpp(
         struct X { int a; };
         void foo(X x) {
           $0^x.$1^a = 10;
         }
         )cpp",
#endif
         "1: targets = {I::a:b:}\n"},
        {// Objective-C: protocols
         R"cpp(
             @interface I
             @end
             @protocol P
             @end
             void foo() {
-              // FIXME: should reference P
-              $0^I<P> *$1^x;
+              $0^I<$1^P> *$2^x;
             }
           )cpp",
         "0: targets = {I}\n"
-        "1: targets = {x}, decl\n"},
+        "1: targets = {P}\n"
+        "2: targets = {x}, decl\n"},
 
        // Designated initializers.
        {R"cpp(
             void foo() {
               struct $0^Foo {
                 int $1^Bar;
               };
               $2^Foo $3^f { .$4^Bar = 42 };

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

)cpp",
       R"cpp(
         // ObjC: Protocols
         @protocol $Interface_decl[[Protocol]]
         @end
         @protocol $Interface_decl[[Protocol2]] <$Interface[[Protocol]]>
         @end
         @interface $Class_decl[[Klass]] <$Interface[[Protocol]]>
         @end
-        // FIXME: protocol list in ObjCObjectType should be highlighted.
-        id<Protocol> $Variable_decl[[x]];
+        id<$Interface[[Protocol]]> $Variable_decl[[x]];
       )cpp",
       R"cpp(
         // ObjC: Categories
         @interface $Class_decl[[Foo]]
         @end
         @interface $Class[[Foo]]($Namespace_decl[[Bar]])
         @end
         @implementation $Class[[Foo]]($Namespace_decl[[Bar]])