[clangd] Add ObjC method support to prepareCallHierarchy

[sheldonneuberger-sc]

Summary

This fixes "textDocument/prepareCallHierarchy" in clangd for ObjC methods. Details at https://github.com/clangd/vscode-clangd/issues/247.

clangd uses Decl::isFunctionOrFunctionTemplate to check if the decl given in a prepareCallHierarchy request is eligible for prepareCallHierarchy. We change to use isFunctionOrMethod which includes functions and ObjC methods.

[nridge]

Thanks for the patch.

Could you add a test exercising call hierarchy for obj-c methods to CallHierarchyTests.cpp please?

In terms of the actual change, this function has some other callers in Parser and Sema code, and I don't know how this change will affect them. It may be better to limit the change to the clangd call site for now (that is, change that call site to use a local helper function, which checks isFunctionOrFunctionTemplate() || <obj-c stuff>), and defer changing Decl::isFunctionOrFunctionTemplate() itself to a subsequent refactor.

[kadircet]

I agree with Nathan on this one. It's unclear why there are two functions that does the same thing in different ways and some usages in sema looks suspicious enough to require caution (there are subtle checks for shadowing and whatnot and I don't know how these rules come into play in objc contexts).

So I would suggest updating clangd side to look like:

(isa<DeclContext>(Decl) && cast<DeclContext>(Decl)->isFunctionOrMethod()) || Decl->getlKind() == FunctionTemplate

[sheldonneuberger-sc]

• move change to clangd callsite
• add tests

[sheldonneuberger-sc]

• clang-format

[sheldonneuberger-sc]

• fix test name

[sheldonneuberger-sc]

Thanks for the detailed comments. I moved the patch into the clangd callsite. I also added a couple ObjC tests for CallHierarchy (I basically parameterized two of the existing tests).

[nridge]

Thanks, LGTM!

I have a couple of nits about the test changes, but with those I think this is good to merged.

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

54

nit: rather than passing std::string by value, let's use llvm::StringRef

(const std::string& would also be fine, but I think llvm::StringRef is more conventional in this codebase)

125

nit: the intendation here seems a bit inconsistent. Can we either indent everything between @implementation and @end, or alternatively un-indent just this line?

[kadircet]

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

54

i am not sure if these helpers makes tests easier to read (i believe they're actually making it harder).

please inline them back. if we want to have better infrastructure for testing this, we need to do it in its own patch (and I definitely agree that we need a better infra for testing here. but we need to think about something that can generalize rather than forcing a particular pattern).

[sheldonneuberger-sc]

• inline test helper for readability
• fix indentation

[sheldonneuberger-sc]

• revert whitespace change

[sheldonneuberger-sc]

Made suggested changes, ready for another review.

[kadircet]

thanks, lgtm! let me know of your email address (for commit attribution) if you want me to land this for you.

[nridge]

In D114058#3150606, @kadircet wrote:

thanks, lgtm! let me know of your email address (for commit attribution) if you want me to land this for you.

While I don't see it surfaced anywhere in the Phabricator UI, there is in fact an email address associated with this changeset, and it shows up in the commit created by arc patch D114058.

clang-tools-extra/clangd/XRefs.cpp

prepareCallHierarchy(ParsedAST &AST, Position Pos, PathRef TUPath) {
   auto Loc = sourceLocationInMainFile(SM, Pos);
   if (!Loc) {
     elog("prepareCallHierarchy failed to convert position to source location: "
          "{0}",
          Loc.takeError());
     return Result;
   }
   for (const NamedDecl *Decl : getDeclAtPosition(AST, *Loc, {})) {
-    if (!Decl->isFunctionOrFunctionTemplate())
+    if (!(isa<DeclContext>(Decl) &&
+          cast<DeclContext>(Decl)->isFunctionOrMethod()) &&
+        Decl->getKind() != Decl::Kind::FunctionTemplate)
       continue;
     if (auto CHI = declToCallHierarchyItem(*Decl))
       Result.emplace_back(std::move(*CHI));
   }
   return Result;
 }
 
 std::vector<CallHierarchyIncomingCall>

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

 
 using ::testing::AllOf;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::IsEmpty;
 using ::testing::Matcher;
 using ::testing::UnorderedElementsAre;
 
+void verifyIncomingMultiFile(std::string SourceExt, std::string HeaderExt,
+                             Annotations &CalleeH, Annotations &Caller1H,
+                             Annotations &Caller2H, Annotations &CalleeC,
+                             Annotations &Caller1C, Annotations &Caller2C,
+                             Annotations &Caller3C);
+void verifyIncomingOneFile(Annotations &Source, std::string Filename);
+
 // Helpers for matching call hierarchy data structures.
 MATCHER_P(WithName, N, "") { return arg.name == N; }
 MATCHER_P(WithSelectionRange, R, "") { return arg.selectionRange == R; }
 
 template <class ItemMatcher>
 ::testing::Matcher<CallHierarchyIncomingCall> From(ItemMatcher M) {
   return Field(&CallHierarchyIncomingCall::from, M);
 }
 template <class... RangeMatchers>
 ::testing::Matcher<CallHierarchyIncomingCall> FromRanges(RangeMatchers... M) {
   return Field(&CallHierarchyIncomingCall::fromRanges,
                UnorderedElementsAre(M...));
 }
 
-TEST(CallHierarchy, IncomingOneFile) {
-  Annotations Source(R"cpp(
-    void call^ee(int);
-    void caller1() {
-      $Callee[[callee]](42);
-    }
-    void caller2() {
-      $Caller1A[[caller1]]();
-      $Caller1B[[caller1]]();
-    }
-    void caller3() {
-      $Caller1C[[caller1]]();
-      $Caller2[[caller2]]();
-    }
-  )cpp");
+void verifyIncomingOneFile(Annotations &Source, std::string Filename) {
   TestTU TU = TestTU::withCode(Source.code());
+  TU.Filename = Filename;
   auto AST = TU.build();
   auto Index = TU.index();
-
   std::vector<CallHierarchyItem> Items =
       prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
   ASSERT_THAT(Items, ElementsAre(WithName("callee")));
   auto IncomingLevel1 = incomingCalls(Items[0], Index.get());
   ASSERT_THAT(IncomingLevel1,
               ElementsAre(AllOf(From(WithName("caller1")),
                                 FromRanges(Source.range("Callee")))));
-
   auto IncomingLevel2 = incomingCalls(IncomingLevel1[0].from, Index.get());
   ASSERT_THAT(IncomingLevel2,
               ElementsAre(AllOf(From(WithName("caller2")),
                                 FromRanges(Source.range("Caller1A"),
                                            Source.range("Caller1B"))),
                           AllOf(From(WithName("caller3")),
                                 FromRanges(Source.range("Caller1C")))));
 
   auto IncomingLevel3 = incomingCalls(IncomingLevel2[0].from, Index.get());
   ASSERT_THAT(IncomingLevel3,
               ElementsAre(AllOf(From(WithName("caller3")),
                                 FromRanges(Source.range("Caller2")))));
 
   auto IncomingLevel4 = incomingCalls(IncomingLevel3[0].from, Index.get());
   EXPECT_THAT(IncomingLevel4, IsEmpty());
 }
 
+TEST(CallHierarchy, IncomingOneFileCpp) {
+  Annotations Source(R"cpp(
+    void call^ee(int);
+    void caller1() {
+      $Callee[[callee]](42);
+    }
+    void caller2() {
+      $Caller1A[[caller1]]();
+      $Caller1B[[caller1]]();
+    }
+    void caller3() {
+      $Caller1C[[caller1]]();
+      $Caller2[[caller2]]();
+    }
+  )cpp");
+  verifyIncomingOneFile(Source, "TestTU.cpp");
+}
+
+TEST(CallHierarchy, IncomingOneFileObjC) {
+  Annotations Source(R"objc(
+    @implementation MyClass {}
+      +(void)call^ee {
+    }
+    +(void) caller1 {
+      [MyClass $Callee[[callee]]];
+    }
+    +(void) caller2 {
+      [MyClass $Caller1A[[caller1]]];
+      [MyClass $Caller1B[[caller1]]];
+    }
+    +(void) caller3 {
+      [MyClass $Caller1C[[caller1]]];
+      [MyClass $Caller2[[caller2]]];
+    }
+    @end
+  )objc");
+  verifyIncomingOneFile(Source, "TestTU.m");
+}
+
 TEST(CallHierarchy, MainFileOnlyRef) {
   // In addition to testing that we store refs to main-file only symbols,
   // this tests that anonymous namespaces do not interfere with the
   // symbol re-identification process in callHierarchyItemToSymbo().
   Annotations Source(R"cpp(
     void call^ee(int);
     namespace {
       void caller1() {
TEST(CallHierarchy, IncomingQualified) {
   auto Incoming = incomingCalls(Items[0], Index.get());
   EXPECT_THAT(Incoming,
               ElementsAre(AllOf(From(WithName("caller1")),
                                 FromRanges(Source.range("Caller1"))),
                           AllOf(From(WithName("caller2")),
                                 FromRanges(Source.range("Caller2")))));
 }
 
-TEST(CallHierarchy, IncomingMultiFile) {
+TEST(CallHierarchy, IncomingMultiFileCpp) {
   // The test uses a .hh suffix for header files to get clang
   // to parse them in C++ mode. .h files are parsed in C mode
   // by default, which causes problems because e.g. symbol
   // USRs are different in C mode (do not include function signatures).
 
   Annotations CalleeH(R"cpp(
     void calle^e(int);
   )cpp");
Annotations Caller3C(R"cpp(
     #include "caller1.hh"
     #include "caller2.hh"
     void caller3() {
       $Caller1[[caller1]]();
       $Caller2[[caller2]]();
     }
   )cpp");
 
-  TestWorkspace Workspace;
-  Workspace.addSource("callee.hh", CalleeH.code());
-  Workspace.addSource("caller1.hh", Caller1H.code());
-  Workspace.addSource("caller2.hh", Caller2H.code());
-  Workspace.addMainFile("callee.cc", CalleeC.code());
-  Workspace.addMainFile("caller1.cc", Caller1C.code());
-  Workspace.addMainFile("caller2.cc", Caller2C.code());
-  Workspace.addMainFile("caller3.cc", Caller3C.code());
+  verifyIncomingMultiFile(".cc", ".hh", CalleeH, Caller1H, Caller2H, CalleeC,
+                          Caller1C, Caller2C, Caller3C);
+}
 
+TEST(CallHierarchy, IncomingMultiFileObjC) {
+  // The test uses a .mi suffix for header files to get clang
+  // to parse them in ObjC mode. .h files are parsed in C mode
+  // by default, which causes problems because e.g. symbol
+  // USRs are different in C mode (do not include function signatures).
+
+  Annotations CalleeH(R"objc(
+    @interface CalleeClass
+      +(void)call^ee;
+    @end
+  )objc");
+  Annotations CalleeC(R"objc(
+    #import "callee.mi"
+    @implementation CalleeClass {}
+      +(void)call^ee {}
+    @end
+  )objc");
+  Annotations Caller1H(R"objc(
+    @interface Caller1Class
+      +(void)caller1;
+    @end
+  )objc");
+  Annotations Caller1C(R"objc(
+    #import "callee.mi"
+    #import "caller1.mi"
+    @implementation Caller1Class {}
+      +(void)caller1 {
+        [CalleeClass [[calle^e]]];
+      }
+    @end
+  )objc");
+  Annotations Caller2H(R"objc(
+    @interface Caller2Class
+      +(void)caller2;
+    @end
+  )objc");
+  Annotations Caller2C(R"objc(
+    #import "caller1.mi"
+    #import "caller2.mi"
+    @implementation Caller2Class {}
+      +(void)caller2 {
+        [Caller1Class $A[[caller1]]];
+        [Caller1Class $B[[caller1]]];
+      }
+    @end
+  )objc");
+  Annotations Caller3C(R"objc(
+    #import "caller1.mi"
+    #import "caller2.mi"
+    @implementation Caller3Class {}
+      +(void)caller3 {
+        [Caller1Class $Caller1[[caller1]]];
+        [Caller2Class $Caller2[[caller2]]];
+      }
+    @end
+  )objc");
+
+  verifyIncomingMultiFile(".m", ".mi", CalleeH, Caller1H, Caller2H, CalleeC,
+                          Caller1C, Caller2C, Caller3C);
+}
+
+void verifyIncomingMultiFile(std::string SourceExt, std::string HeaderExt,
+                             Annotations &CalleeH, Annotations &Caller1H,
+                             Annotations &Caller2H, Annotations &CalleeC,
+                             Annotations &Caller1C, Annotations &Caller2C,
+                             Annotations &Caller3C) {
+  TestWorkspace Workspace;
+  Workspace.addSource("callee" + HeaderExt, CalleeH.code());
+  Workspace.addSource("caller1" + HeaderExt, Caller1H.code());
+  Workspace.addSource("caller2" + HeaderExt, Caller2H.code());
+  Workspace.addMainFile("callee" + SourceExt, CalleeC.code());
+  Workspace.addMainFile("caller1" + SourceExt, Caller1C.code());
+  Workspace.addMainFile("caller2" + SourceExt, Caller2C.code());
+  Workspace.addMainFile("caller3" + SourceExt, Caller3C.code());
   auto Index = Workspace.index();
 
   auto CheckCallHierarchy = [&](ParsedAST &AST, Position Pos, PathRef TUPath) {
     std::vector<CallHierarchyItem> Items =
         prepareCallHierarchy(AST, Pos, TUPath);
     ASSERT_THAT(Items, ElementsAre(WithName("callee")));
     auto IncomingLevel1 = incomingCalls(Items[0], Index.get());
     ASSERT_THAT(IncomingLevel1,
ASSERT_THAT(IncomingLevel3,
                 ElementsAre(AllOf(From(WithName("caller3")),
                                   FromRanges(Caller3C.range("Caller2")))));
 
     auto IncomingLevel4 = incomingCalls(IncomingLevel3[0].from, Index.get());
     EXPECT_THAT(IncomingLevel4, IsEmpty());
   };
 
   // Check that invoking from a call site works.
-  auto AST = Workspace.openFile("caller1.cc");
+  auto AST = Workspace.openFile("caller1" + SourceExt);
   ASSERT_TRUE(bool(AST));
-  CheckCallHierarchy(*AST, Caller1C.point(), testPath("caller1.cc"));
+  CheckCallHierarchy(*AST, Caller1C.point(), testPath("caller1" + SourceExt));
 
   // Check that invoking from the declaration site works.
-  AST = Workspace.openFile("callee.hh");
+  AST = Workspace.openFile("callee" + HeaderExt);
   ASSERT_TRUE(bool(AST));
-  CheckCallHierarchy(*AST, CalleeH.point(), testPath("callee.hh"));
+  CheckCallHierarchy(*AST, CalleeH.point(), testPath("callee" + HeaderExt));
 
   // Check that invoking from the definition site works.
-  AST = Workspace.openFile("callee.cc");
+  AST = Workspace.openFile("callee" + SourceExt);
   ASSERT_TRUE(bool(AST));
-  CheckCallHierarchy(*AST, CalleeC.point(), testPath("callee.cc"));
+  CheckCallHierarchy(*AST, CalleeC.point(), testPath("callee" + SourceExt));
 }
 
 TEST(CallHierarchy, CallInLocalVarDecl) {
   // Tests that local variable declarations are not treated as callers
   // (they're not indexed, so they can't be represented as call hierarchy
   // items); instead, the caller should be the containing function.
   // However, namespace-scope variable declarations should be treated as
   // callers because those are indexed and there is no enclosing entity