From: Haojian Wu Date: Mon, 30 Sep 2019 10:48:02 +0000 (+0000) Subject: [clangd] Implement a smart version of HeaderSource switch. X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=fdbe5b4b6fdc49619ed40f841df271a7c353a79d;p=platform%2Fupstream%2Fllvm.git [clangd] Implement a smart version of HeaderSource switch. Summary: This patch implements another version header-source switch by incorporating the AST and index, it will be used: - to improve the current header-source switch feature (layer with the existing file heuristic); - by the incoming define-outline code action; Reviewers: kadircet Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D67907 llvm-svn: 373197 --- diff --git a/clang-tools-extra/clangd/HeaderSourceSwitch.cpp b/clang-tools-extra/clangd/HeaderSourceSwitch.cpp index 70b28e7a..535c8d6 100644 --- a/clang-tools-extra/clangd/HeaderSourceSwitch.cpp +++ b/clang-tools-extra/clangd/HeaderSourceSwitch.cpp @@ -7,6 +7,10 @@ //===----------------------------------------------------------------------===// #include "HeaderSourceSwitch.h" +#include "AST.h" +#include "Logger.h" +#include "index/SymbolCollector.h" +#include "clang/AST/Decl.h" namespace clang { namespace clangd { @@ -64,5 +68,86 @@ llvm::Optional getCorrespondingHeaderOrSource( return None; } +llvm::Optional getCorrespondingHeaderOrSource(const Path &OriginalFile, + ParsedAST &AST, + const SymbolIndex *Index) { + if (!Index) { + // FIXME: use the AST to do the inference. + return None; + } + LookupRequest Request; + // Find all symbols present in the original file. + for (const auto *D : getIndexableLocalDecls(AST)) { + if (auto ID = getSymbolID(D)) + Request.IDs.insert(*ID); + } + llvm::StringMap Candidates; // Target path => score. + auto AwardTarget = [&](const char *TargetURI) { + if (auto TargetPath = URI::resolve(TargetURI, OriginalFile)) { + if (*TargetPath != OriginalFile) // exclude the original file. + ++Candidates[*TargetPath]; + }; + }; + // If we switch from a header, we are looking for the implementation + // file, so we use the definition loc; otherwise we look for the header file, + // we use the decl loc; + // + // For each symbol in the original file, we get its target location (decl or + // def) from the index, then award that target file. + bool IsHeader = AST.getASTContext().getLangOpts().IsHeaderFile; + Index->lookup(Request, [&](const Symbol &Sym) { + if (IsHeader) + AwardTarget(Sym.Definition.FileURI); + else + AwardTarget(Sym.CanonicalDeclaration.FileURI); + }); + // FIXME: our index doesn't have any interesting information (this could be + // that the background-index is not finished), we should use the decl/def + // locations from the AST to do the inference (from .cc to .h). + if (Candidates.empty()) + return None; + + // Pickup the winner, who contains most of symbols. + // FIXME: should we use other signals (file proximity) to help score? + auto Best = Candidates.begin(); + for (auto It = Candidates.begin(); It != Candidates.end(); ++It) { + if (It->second > Best->second) + Best = It; + else if (It->second == Best->second && It->first() < Best->first()) + // Select the first one in the lexical order if we have multiple + // candidates. + Best = It; + } + return Path(Best->first()); +} + +std::vector getIndexableLocalDecls(ParsedAST &AST) { + std::vector Results; + std::function TraverseDecl = [&](Decl *D) { + auto *ND = llvm::dyn_cast(D); + if (!ND || ND->isImplicit()) + return; + if (!SymbolCollector::shouldCollectSymbol(*ND, D->getASTContext(), {}, + /*IsMainFileSymbol=*/false)) + return; + if (!llvm::isa(ND)) { + // Visit the children, but we skip function decls as we are not interested + // in the function body. + if (auto *Scope = llvm::dyn_cast(ND)) { + for (auto *D : Scope->decls()) + TraverseDecl(D); + } + } + if (llvm::isa(D)) + return; // namespace is indexable, but we're not interested. + Results.push_back(D); + }; + // Traverses the ParsedAST directly to collect all decls present in the main + // file. + for (auto *TopLevel : AST.getLocalTopLevelDecls()) + TraverseDecl(TopLevel); + return Results; +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/HeaderSourceSwitch.h b/clang-tools-extra/clangd/HeaderSourceSwitch.h index 23ecc4b..a971b38 100644 --- a/clang-tools-extra/clangd/HeaderSourceSwitch.h +++ b/clang-tools-extra/clangd/HeaderSourceSwitch.h @@ -21,6 +21,16 @@ llvm::Optional getCorrespondingHeaderOrSource( const Path &OriginalFile, llvm::IntrusiveRefCntPtr VFS); +/// Given a header file, returns the best matching source file, and vice visa. +/// The heuristics incorporate with the AST and the index (if provided). +llvm::Optional getCorrespondingHeaderOrSource(const Path &OriginalFile, + ParsedAST &AST, + const SymbolIndex *Index); + +/// Returns all indexable decls that are present in the main file of the AST. +/// Exposed for unittests. +std::vector getIndexableLocalDecls(ParsedAST &AST); + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp b/clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp index 6835fdf..1ca543d 100644 --- a/clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp +++ b/clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp @@ -10,6 +10,7 @@ #include "TestFS.h" #include "TestTU.h" +#include "index/MemIndex.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -71,6 +72,174 @@ TEST(HeaderSourceSwitchTest, FileHeuristic) { EXPECT_FALSE(PathResult.hasValue()); } +MATCHER_P(DeclNamed, Name, "") { + if (const NamedDecl *ND = dyn_cast(arg)) + if (ND->getQualifiedNameAsString() == Name) + return true; + return false; +} + +TEST(HeaderSourceSwitchTest, GetLocalDecls) { + TestTU TU; + TU.HeaderCode = R"cpp( + void HeaderOnly(); + )cpp"; + TU.Code = R"cpp( + void MainF1(); + class Foo {}; + namespace ns { + class Foo { + void method(); + int field; + }; + } // namespace ns + + // Non-indexable symbols + namespace { + void Ignore1() {} + } + + )cpp"; + + auto AST = TU.build(); + EXPECT_THAT(getIndexableLocalDecls(AST), + testing::UnorderedElementsAre( + DeclNamed("MainF1"), DeclNamed("Foo"), DeclNamed("ns::Foo"), + DeclNamed("ns::Foo::method"), DeclNamed("ns::Foo::field"))); +} + +TEST(HeaderSourceSwitchTest, FromHeaderToSource) { + // build a proper index, which contains symbols: + // A_Sym1, declared in TestTU.h, defined in a.cpp + // B_Sym[1-2], declared in TestTU.h, defined in b.cpp + SymbolSlab::Builder AllSymbols; + TestTU Testing; + Testing.HeaderFilename = "TestTU.h"; + Testing.HeaderCode = "void A_Sym1();"; + Testing.Filename = "a.cpp"; + Testing.Code = "void A_Sym1() {};"; + for (auto &Sym : Testing.headerSymbols()) + AllSymbols.insert(Sym); + + Testing.HeaderCode = R"cpp( + void B_Sym1(); + void B_Sym2(); + )cpp"; + Testing.Filename = "b.cpp"; + Testing.Code = R"cpp( + void B_Sym1() {} + void B_Sym2() {} + )cpp"; + for (auto &Sym : Testing.headerSymbols()) + AllSymbols.insert(Sym); + auto Index = MemIndex::build(std::move(AllSymbols).build(), {}, {}); + + // Test for swtich from .h header to .cc source + struct { + llvm::StringRef HeaderCode; + llvm::Optional ExpectedSource; + } TestCases[] = { + {"// empty, no header found", llvm::None}, + {R"cpp( + // no definition found in the index. + void NonDefinition(); + )cpp", + llvm::None}, + {R"cpp( + void A_Sym1(); + )cpp", + testPath("a.cpp")}, + {R"cpp( + // b.cpp wins. + void A_Sym1(); + void B_Sym1(); + void B_Sym2(); + )cpp", + testPath("b.cpp")}, + {R"cpp( + // a.cpp and b.cpp have same scope, but a.cpp because "a.cpp" < "b.cpp". + void A_Sym1(); + void B_Sym1(); + )cpp", + testPath("a.cpp")}, + }; + for (const auto &Case : TestCases) { + TestTU TU = TestTU::withCode(Case.HeaderCode); + TU.Filename = "TestTU.h"; + TU.ExtraArgs.push_back("-xc++-header"); // inform clang this is a header. + auto HeaderAST = TU.build(); + EXPECT_EQ(Case.ExpectedSource, + getCorrespondingHeaderOrSource(testPath(TU.Filename), HeaderAST, + Index.get())); + } +} + +TEST(HeaderSourceSwitchTest, FromSourceToHeader) { + // build a proper index, which contains symbols: + // A_Sym1, declared in a.h, defined in TestTU.cpp + // B_Sym[1-2], declared in b.h, defined in TestTU.cpp + TestTU TUForIndex = TestTU::withCode(R"cpp( + #include "a.h" + #include "b.h" + + void A_Sym1() {} + + void B_Sym1() {} + void B_Sym2() {} + )cpp"); + TUForIndex.AdditionalFiles["a.h"] = R"cpp( + void A_Sym1(); + )cpp"; + TUForIndex.AdditionalFiles["b.h"] = R"cpp( + void B_Sym1(); + void B_Sym2(); + )cpp"; + TUForIndex.Filename = "TestTU.cpp"; + auto Index = TUForIndex.index(); + + // Test for switching from .cc source file to .h header. + struct { + llvm::StringRef SourceCode; + llvm::Optional ExpectedResult; + } TestCases[] = { + {"// empty, no header found", llvm::None}, + {R"cpp( + // symbol not in index, no header found + void Local() {} + )cpp", + llvm::None}, + + {R"cpp( + // a.h wins. + void A_Sym1() {} + )cpp", + testPath("a.h")}, + + {R"cpp( + // b.h wins. + void A_Sym1() {} + void B_Sym1() {} + void B_Sym2() {} + )cpp", + testPath("b.h")}, + + {R"cpp( + // a.h and b.h have same scope, but a.h wins because "a.h" < "b.h". + void A_Sym1() {} + void B_Sym1() {} + )cpp", + testPath("a.h")}, + }; + for (const auto &Case : TestCases) { + TestTU TU = TestTU::withCode(Case.SourceCode); + TU.Filename = "Test.cpp"; + auto AST = TU.build(); + EXPECT_EQ(Case.ExpectedResult, + getCorrespondingHeaderOrSource(testPath(TU.Filename), AST, + Index.get())); + } +} + } // namespace } // namespace clangd } // namespace clang