[clang-tools-extra] f443793 - [clangd] Ensure Ref::Container refers to an indexed symbol

[Nathan Ridge via cfe-commits]

Author: Nathan Ridge
Date: 2021-07-22T03:33:40-04:00
New Revision: f443793d26c39a78cfedea5a8cc2ad23e1253d84

URL: https://github.com/llvm/llvm-project/commit/f443793d26c39a78cfedea5a8cc2ad23e1253d84
DIFF: https://github.com/llvm/llvm-project/commit/f443793d26c39a78cfedea5a8cc2ad23e1253d84.diff

LOG: [clangd] Ensure Ref::Container refers to an indexed symbol

Fixes https://github.com/clangd/clangd/issues/806

Differential Revision: https://reviews.llvm.org/D105083

Added: 
    

Modified: 
    clang-tools-extra/clangd/index/SymbolCollector.cpp
    clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
    clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index 9211dd3eb48a5..0591c5113a23a 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -152,6 +152,31 @@ llvm::Optional<RelationKind> indexableRelation(const index::SymbolRelation &R) {
   return None;
 }
 
+// Given a ref contained in enclosing decl `Enclosing`, return
+// the decl that should be used as that ref's Ref::Container. This is
+// usually `Enclosing` itself, but in cases where `Enclosing` is not
+// indexed, we walk further up because Ref::Container should always be
+// an indexed symbol.
+// Note: we don't use DeclContext as the container as in some cases
+// it's useful to use a Decl which is not a DeclContext. For example,
+// for a ref occurring in the initializer of a namespace-scope variable,
+// it's useful to use that variable as the container, as otherwise the
+// next enclosing DeclContext would be a NamespaceDecl or TranslationUnitDecl,
+// which are both not indexed and less granular than we'd like for use cases
+// like call hierarchy.
+const Decl *getRefContainer(const Decl *Enclosing,
+                            const SymbolCollector::Options &Opts) {
+  while (Enclosing) {
+    const auto *ND = dyn_cast<NamedDecl>(Enclosing);
+    if (ND && SymbolCollector::shouldCollectSymbol(*ND, ND->getASTContext(),
+                                                   Opts, true)) {
+      break;
+    }
+    Enclosing = dyn_cast_or_null<Decl>(Enclosing->getDeclContext());
+  }
+  return Enclosing;
+}
+
 } // namespace
 
 // Encapsulates decisions about how to record header paths in the index,
@@ -478,8 +503,8 @@ bool SymbolCollector::handleDeclOccurrence(
       !isa<NamespaceDecl>(ND) &&
       (Opts.RefsInHeaders ||
        SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
-    DeclRefs[ND].push_back(
-        SymbolRef{SM.getFileLoc(Loc), Roles, ASTNode.Parent});
+    DeclRefs[ND].push_back(SymbolRef{SM.getFileLoc(Loc), Roles,
+                                     getRefContainer(ASTNode.Parent, Opts)});
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
     return true;

diff  --git a/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp b/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
index 8b5069d8dae80..6f63ea2bc18b3 100644
--- a/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
@@ -26,6 +26,22 @@
 
 namespace clang {
 namespace clangd {
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
+                              const CallHierarchyItem &Item) {
+  return Stream << Item.name << "@" << Item.selectionRange;
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
+                              const CallHierarchyIncomingCall &Call) {
+  Stream << "{ from: " << Call.from << ", ranges: [";
+  for (const auto &R : Call.fromRanges) {
+    Stream << R;
+    Stream << ", ";
+  }
+  return Stream << "] }";
+}
+
 namespace {
 
 using ::testing::AllOf;
@@ -252,6 +268,40 @@ TEST(CallHierarchy, IncomingMultiFile) {
   CheckCallHierarchy(*AST, CalleeC.point(), testPath("callee.cc"));
 }
 
+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
+  // that would be a useful caller.
+  Annotations Source(R"cpp(
+    int call^ee();
+    void caller1() {
+      $call1[[callee]]();
+    }
+    void caller2() {
+      int localVar = $call2[[callee]]();
+    }
+    int caller3 = $call3[[callee]]();
+  )cpp");
+  TestTU TU = TestTU::withCode(Source.code());
+  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 Incoming = incomingCalls(Items[0], Index.get());
+  ASSERT_THAT(
+      Incoming,
+      ElementsAre(
+          AllOf(From(WithName("caller1")), FromRanges(Source.range("call1"))),
+          AllOf(From(WithName("caller2")), FromRanges(Source.range("call2"))),
+          AllOf(From(WithName("caller3")), FromRanges(Source.range("call3")))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

diff  --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
index e24c3c1d60b47..5afaa83bfdcbc 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -811,8 +811,7 @@ TEST_F(SymbolCollectorTest, RefContainers) {
   };
   EXPECT_EQ(Container("ref1a"),
             findSymbol(Symbols, "f2").ID); // function body (call)
-  // FIXME: This is wrongly contained by fptr and not f2.
-  EXPECT_NE(Container("ref1b"),
+  EXPECT_EQ(Container("ref1b"),
             findSymbol(Symbols, "f2").ID); // function body (address-of)
   EXPECT_EQ(Container("ref2"),
             findSymbol(Symbols, "v1").ID); // variable initializer