From f443793d26c39a78cfedea5a8cc2ad23e1253d84 Mon Sep 17 00:00:00 2001 From: Nathan Ridge Date: Tue, 29 Jun 2021 01:54:12 -0400 Subject: [PATCH] [clangd] Ensure Ref::Container refers to an indexed symbol Fixes https://github.com/clangd/clangd/issues/806 Differential Revision: https://reviews.llvm.org/D105083 --- clang-tools-extra/clangd/index/SymbolCollector.cpp | 29 ++++++++++++- .../clangd/unittests/CallHierarchyTests.cpp | 50 ++++++++++++++++++++++ .../clangd/unittests/SymbolCollectorTests.cpp | 3 +- 3 files changed, 78 insertions(+), 4 deletions(-) diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp index 9211dd3..0591c51 100644 --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -152,6 +152,31 @@ llvm::Optional 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(Enclosing); + if (ND && SymbolCollector::shouldCollectSymbol(*ND, ND->getASTContext(), + Opts, true)) { + break; + } + Enclosing = dyn_cast_or_null(Enclosing->getDeclContext()); + } + return Enclosing; +} + } // namespace // Encapsulates decisions about how to record header paths in the index, @@ -478,8 +503,8 @@ bool SymbolCollector::handleDeclOccurrence( !isa(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 8b5069d..6f63ea2 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 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 e24c3c1..5afaa83 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 -- 2.7.4