Index: clangd/CodeComplete.cpp =================================================================== --- clangd/CodeComplete.cpp +++ clangd/CodeComplete.cpp @@ -48,6 +48,8 @@ #include "llvm/Support/Format.h" #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/ScopedPrinter.h" +#include +#include #include // We log detailed candidate here if you run with -debug-only=codecomplete. @@ -285,8 +287,7 @@ } llvm::Optional headerToInsertIfNotPresent() const { - if (!IndexResult || !IndexResult->Detail || - IndexResult->Detail->IncludeHeader.empty()) + if (!IndexResult || IndexResult->IncludeHeaders.empty()) return llvm::None; if (SemaResult && SemaResult->Declaration) { // Avoid inserting new #include if the declaration is found in the current @@ -296,7 +297,26 @@ if (SM.isInMainFile(SM.getExpansionLoc(RD->getBeginLoc()))) return llvm::None; } - return IndexResult->Detail->IncludeHeader; + if (IndexResult->IncludeHeaders.size() == 1) + return IndexResult->IncludeHeaders.front().IncludeHeader; + + // Pick the most popular include, only if it has way more references than + // the rest of includes; otherwise, we just give up and don't insert + // includes. + // FIXME(ioeric): we might want to store possible #includes in the + // completion result so that clients can decide what to do with them. + auto Includes = IndexResult->IncludeHeaders; + assert(Includes.size() > 1); + // Sort in descending order by reference count. + std::sort(Includes.begin(), Includes.end(), + [](const Symbol::IncludeHeaderWithReferences &LHS, + const Symbol::IncludeHeaderWithReferences &RHS) { + return LHS.References > RHS.References; + }); + if (Includes.begin()->References > + 10 * std::next(Includes.begin())->References) + return Includes.front().IncludeHeader; + return llvm::None; } using Bundle = llvm::SmallVector; @@ -381,8 +401,7 @@ } else log("Failed to generate include insertion edits for adding header " "(FileURI='{0}', IncludeHeader='{1}') into {2}", - C.IndexResult->CanonicalDeclaration.FileURI, - C.IndexResult->Detail->IncludeHeader, FileName); + C.IndexResult->CanonicalDeclaration.FileURI, *Inserted, FileName); } } Index: clangd/index/Index.h =================================================================== --- clangd/index/Index.h +++ clangd/index/Index.h @@ -16,7 +16,9 @@ #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/Hashing.h" #include "llvm/ADT/Optional.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringExtras.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Support/StringSaver.h" #include #include @@ -200,6 +202,30 @@ /// (When snippets are disabled, the symbol name alone is used). llvm::StringRef CompletionSnippetSuffix; + struct IncludeHeaderWithReferences { + IncludeHeaderWithReferences() = default; + + IncludeHeaderWithReferences(llvm::StringRef IncludeHeader, + unsigned References) + : IncludeHeader(IncludeHeader), References(References) {} + + /// This can be either a URI of the header to be #include'd + /// for this symbol, or a literal header quoted with <> or "" that is + /// suitable to be included directly. When it is a URI, the exact #include + /// path needs to be calculated according to the URI scheme. + /// + /// Note that the include header is a canonical include for the symbol and + /// can be different from FileURI in the CanonicalDeclaration. + llvm::StringRef IncludeHeader = ""; + /// The number of translation units that reference this symbol and include + /// this header. This number is only meaningful if aggregated in an index. + unsigned References = 0; + }; + /// One Symbol can potentially be incuded via different headers. + /// - If we haven't seen a definition, this covers all declarations. + /// - If we have seen a definition, this covers declarations visible from + /// any definition. + llvm::SmallVector IncludeHeaders; /// Optional symbol details that are not required to be set. For example, an /// index fuzzy match can return a large number of symbol candidates, and it /// is preferable to send only core symbol information in the batched results @@ -211,14 +237,6 @@ /// Type when this symbol is used in an expression. (Short display form). /// e.g. return type of a function, or type of a variable. llvm::StringRef ReturnType; - /// This can be either a URI of the header to be #include'd for this symbol, - /// or a literal header quoted with <> or "" that is suitable to be included - /// directly. When this is a URI, the exact #include path needs to be - /// calculated according to the URI scheme. - /// - /// This is a canonical include for the symbol and can be different from - /// FileURI in the CanonicalDeclaration. - llvm::StringRef IncludeHeader; }; // Optional details of the symbol. Index: clangd/index/Index.cpp =================================================================== --- clangd/index/Index.cpp +++ clangd/index/Index.cpp @@ -9,6 +9,7 @@ #include "Index.h" #include "llvm/ADT/StringExtras.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Support/SHA1.h" #include "llvm/Support/raw_ostream.h" @@ -91,6 +92,9 @@ Intern(S.Signature); Intern(S.CompletionSnippetSuffix); + for (auto &I : S.IncludeHeaders) + Intern(I.IncludeHeader); + if (S.Detail) { // Copy values of StringRefs into arena. auto *Detail = Arena.Allocate(); @@ -98,7 +102,6 @@ // Intern the actual strings. Intern(Detail->Documentation); Intern(Detail->ReturnType); - Intern(Detail->IncludeHeader); // Replace the detail pointer with our copy. S.Detail = Detail; } Index: clangd/index/Merge.cpp =================================================================== --- clangd/index/Merge.cpp +++ clangd/index/Merge.cpp @@ -100,6 +100,10 @@ // Classes: this is the def itself. Functions: hopefully the header decl. // If both did (or both didn't), continue to prefer L over R. bool PreferR = R.Definition && !L.Definition; + // Merge include headers only if both have definitions or both have no + // definition; otherwise, only accumulate references of common includes. + bool MergeIncludes = + L.Definition.FileURI.empty() == R.Definition.FileURI.empty(); Symbol S = PreferR ? R : L; // The target symbol we're merging into. const Symbol &O = PreferR ? L : R; // The "other" less-preferred symbol. @@ -114,6 +118,18 @@ S.Signature = O.Signature; if (S.CompletionSnippetSuffix == "") S.CompletionSnippetSuffix = O.CompletionSnippetSuffix; + for (const auto &OI : O.IncludeHeaders) { + bool Found = false; + for (auto &SI : S.IncludeHeaders) { + if (SI.IncludeHeader == OI.IncludeHeader) { + Found = true; + SI.References += OI.References; + break; + } + } + if (!Found && MergeIncludes) + S.IncludeHeaders.emplace_back(OI.IncludeHeader, OI.References); + } if (O.Detail) { if (S.Detail) { @@ -123,8 +139,6 @@ Scratch->Documentation = O.Detail->Documentation; if (Scratch->ReturnType == "") Scratch->ReturnType = O.Detail->ReturnType; - if (Scratch->IncludeHeader == "") - Scratch->IncludeHeader = O.Detail->IncludeHeader; S.Detail = Scratch; } else S.Detail = O.Detail; Index: clangd/index/SymbolCollector.cpp =================================================================== --- clangd/index/SymbolCollector.cpp +++ clangd/index/SymbolCollector.cpp @@ -403,10 +403,12 @@ SM.getExpansionLoc(MI->getDefinitionLoc()), Opts)) Include = std::move(*Header); } + if (!Include.empty()) + S.IncludeHeaders.emplace_back(Include, 1); + S.Signature = Signature; S.CompletionSnippetSuffix = SnippetSuffix; Symbol::Details Detail; - Detail.IncludeHeader = Include; S.Detail = &Detail; Symbols.insert(S); return true; @@ -489,7 +491,8 @@ Symbol::Details Detail; Detail.Documentation = Documentation; Detail.ReturnType = ReturnType; - Detail.IncludeHeader = Include; + if (!Include.empty()) + S.IncludeHeaders.emplace_back(Include, 1); S.Detail = &Detail; S.Origin = Opts.Origin; Index: clangd/index/SymbolYAML.cpp =================================================================== --- clangd/index/SymbolYAML.cpp +++ clangd/index/SymbolYAML.cpp @@ -10,11 +10,13 @@ #include "SymbolYAML.h" #include "Index.h" #include "llvm/ADT/Optional.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/Support/Errc.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/raw_ostream.h" LLVM_YAML_IS_DOCUMENT_LIST_VECTOR(clang::clangd::Symbol) +LLVM_YAML_IS_SEQUENCE_VECTOR(clang::clangd::Symbol::IncludeHeaderWithReferences) namespace llvm { namespace yaml { @@ -70,7 +72,15 @@ static void mapping(IO &io, Symbol::Details &Detail) { io.mapOptional("Documentation", Detail.Documentation); io.mapOptional("ReturnType", Detail.ReturnType); - io.mapOptional("IncludeHeader", Detail.IncludeHeader); + } +}; + +template <> +struct MappingTraits { + static void mapping(IO &io, + clang::clangd::Symbol::IncludeHeaderWithReferences &Inc) { + io.mapRequired("Header", Inc.IncludeHeader); + io.mapRequired("References", Inc.References); } }; @@ -112,6 +122,7 @@ false); IO.mapOptional("Signature", Sym.Signature); IO.mapOptional("CompletionSnippetSuffix", Sym.CompletionSnippetSuffix); + IO.mapOptional("IncludeHeaders", Sym.IncludeHeaders); IO.mapOptional("Detail", NDetail->Opt); } }; Index: unittests/clangd/CodeCompleteTests.cpp =================================================================== --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -568,7 +568,8 @@ auto BarURI = URI::createFile(BarHeader).toString(); Symbol Sym = cls("ns::X"); Sym.CanonicalDeclaration.FileURI = BarURI; - Scratch.IncludeHeader = BarURI; + Sym.IncludeHeaders.emplace_back(BarURI, 1); + Sym.Detail = &Scratch; // Shoten include path based on search dirctory and insert. auto Results = completions(Server, @@ -602,7 +603,10 @@ auto BarURI = URI::createFile(BarHeader).toString(); SymX.CanonicalDeclaration.FileURI = BarURI; SymY.CanonicalDeclaration.FileURI = BarURI; - Scratch.IncludeHeader = ""; + + SymX.IncludeHeaders.emplace_back("", 1); + SymY.IncludeHeaders.emplace_back("", 1); + SymX.Detail = &Scratch; SymY.Detail = &Scratch; // Shoten include path based on search dirctory and insert. @@ -1130,7 +1134,8 @@ Symbol::Details Detail; std::string DeclFile = URI::createFile(testPath("foo")).toString(); NoArgsGFunc.CanonicalDeclaration.FileURI = DeclFile; - Detail.IncludeHeader = ""; + NoArgsGFunc.IncludeHeaders.emplace_back("", 1); + NoArgsGFunc.Detail = &Detail; EXPECT_THAT( completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).Completions, @@ -1782,6 +1787,29 @@ ASSERT_EQ(Reqs3.size(), 2u); } +TEST(CompletionTest, MultipleIncludeHeadersNoInsertion) { + std::string DeclFile = URI::createFile(testPath("foo")).toString(); + Symbol sym = func("Func"); + sym.CanonicalDeclaration.FileURI = DeclFile; + sym.IncludeHeaders.emplace_back("", 2); + sym.IncludeHeaders.emplace_back("", 1); + + EXPECT_THAT(completions("Fun^", {sym}).Completions, + UnorderedElementsAre(AllOf(Named("Func"), Not(InsertInclude())))); +} + +TEST(CompletionTest, MultipleIncludeHeadersAndInsert) { + std::string DeclFile = URI::createFile(testPath("foo")).toString(); + Symbol sym = func("Func"); + sym.CanonicalDeclaration.FileURI = DeclFile; + sym.IncludeHeaders.emplace_back("", 2); + sym.IncludeHeaders.emplace_back("", 1000); + + EXPECT_THAT( + completions("Fun^", {sym}).Completions, + UnorderedElementsAre(AllOf(Named("Func"), InsertInclude("")))); +} + } // namespace } // namespace clangd } // namespace clang Index: unittests/clangd/FileIndexTests.cpp =================================================================== --- unittests/clangd/FileIndexTests.cpp +++ unittests/clangd/FileIndexTests.cpp @@ -177,7 +177,7 @@ Req.Query = ""; bool SeenSymbol = false; M.fuzzyFind(Req, [&](const Symbol &Sym) { - EXPECT_TRUE(Sym.Detail->IncludeHeader.empty()); + EXPECT_TRUE(Sym.IncludeHeaders.empty()); SeenSymbol = true; }); EXPECT_TRUE(SeenSymbol); Index: unittests/clangd/IndexTests.cpp =================================================================== --- unittests/clangd/IndexTests.cpp +++ unittests/clangd/IndexTests.cpp @@ -243,6 +243,48 @@ EXPECT_EQ(M.Name, "right"); } +MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") { + return (arg.IncludeHeader == IncludeHeader) && (arg.References == References); +} + +TEST(MergeTest, MergeIncludesOnDifferentDefinitions) { + Symbol L, R; + L.Name = "left"; + R.Name = "right"; + L.ID = R.ID = SymbolID("hello"); + L.IncludeHeaders.emplace_back("common", 1); + R.IncludeHeaders.emplace_back("common", 1); + R.IncludeHeaders.emplace_back("new", 1); + + Symbol::Details Scratch; + + // Both have no definition. + Symbol M = mergeSymbol(L, R, &Scratch); + EXPECT_THAT(M.IncludeHeaders, + UnorderedElementsAre(IncludeHeaderWithRef("common", 2u), + IncludeHeaderWithRef("new", 1u))); + + // Only merge references of the same includes but do not merge new #includes. + L.Definition.FileURI = "file:/left.h"; + M = mergeSymbol(L, R, &Scratch); + EXPECT_THAT(M.IncludeHeaders, + UnorderedElementsAre(IncludeHeaderWithRef("common", 2u))); + + // Definitions are the same. + R.Definition.FileURI = "file:/right.h"; + M = mergeSymbol(L, R, &Scratch); + EXPECT_THAT(M.IncludeHeaders, + UnorderedElementsAre(IncludeHeaderWithRef("common", 2u), + IncludeHeaderWithRef("new", 1u))); + + // Definitions are different. + R.Definition.FileURI = "file:/right.h"; + M = mergeSymbol(L, R, &Scratch); + EXPECT_THAT(M.IncludeHeaders, + UnorderedElementsAre(IncludeHeaderWithRef("common", 2u), + IncludeHeaderWithRef("new", 1u))); +} + } // namespace } // namespace clangd } // namespace clang Index: unittests/clangd/SymbolCollectorTests.cpp =================================================================== --- unittests/clangd/SymbolCollectorTests.cpp +++ unittests/clangd/SymbolCollectorTests.cpp @@ -53,7 +53,11 @@ MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; } MATCHER_P(DefURI, P, "") { return arg.Definition.FileURI == P; } MATCHER_P(IncludeHeader, P, "") { - return arg.Detail && arg.Detail->IncludeHeader == P; + return (arg.IncludeHeaders.size() == 1) && + (arg.IncludeHeaders.begin()->IncludeHeader == P); +} +MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") { + return (arg.IncludeHeader == IncludeHeader) && (arg.References == References); } MATCHER_P(DeclRange, Pos, "") { return std::tie(arg.CanonicalDeclaration.Start.Line, @@ -696,6 +700,11 @@ Line: 1 Column: 1 IsIndexedForCodeCompletion: true +IncludeHeaders: + - Header: 'include1' + References: 7 + - Header: 'include2' + References: 3 Detail: Documentation: 'Foo doc' ReturnType: 'int' @@ -730,6 +739,11 @@ Doc("Foo doc"), ReturnType("int"), DeclURI("file:///path/foo.h"), ForCodeCompletion(true)))); + auto &Sym1 = *Symbols1.begin(); + assert(Sym1.Detail); + EXPECT_THAT(Sym1.IncludeHeaders, + UnorderedElementsAre(IncludeHeaderWithRef("include1", 7u), + IncludeHeaderWithRef("include2", 3u))); auto Symbols2 = symbolsFromYAML(YAML2); EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf( QName("clang::Foo2"), Labeled("Foo2-sig"), @@ -751,9 +765,10 @@ TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) { CollectorOpts.CollectIncludePath = true; runSymbolCollector("class Foo {};", /*Main=*/""); - EXPECT_THAT(Symbols, - UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI), - IncludeHeader(TestHeaderURI)))); + EXPECT_THAT(Symbols, UnorderedElementsAre( + AllOf(QName("Foo"), DeclURI(TestHeaderURI)))); + EXPECT_THAT(Symbols.begin()->IncludeHeaders, + UnorderedElementsAre(IncludeHeaderWithRef(TestHeaderURI, 1u))); } #ifndef _WIN32