[clangd] Support multiple #include headers in one symbol.

[ioeric]

Summary

Currently, a symbol can have only one #include header attached, which
might not work well if the symbol can be imported via different #includes depending
on where it's used. This patch stores multiple #include headers (with # references)
for each symbol, so that CodeCompletion can decide which include to insert.

In this patch, code completion simply picks the most popular include as the default inserted header. We also return all possible includes and their edits in the CodeCompletion results.

[ioeric]

@sammccall The code still needs cleanup but should be useful for providing high-level feedback, which I would like to get before moving further. Thanks!

[ioeric]

• minor cleanup

[sammccall]

Nice!

We could reduce the scope of this patch somewhat by ignoring file proximity and just switching to return the most popular header. This would be a solid improvement over current behavior, and provide the infrastructure for the file-proximity approach.

clangd/CodeComplete.cpp

1418

I remain unconvinced that providing multiple completion candidates is the right thing to do here:

• if the index hasn't seen a definition, then we're going to show one copy of the completion for each header that has a declaration
• the user isn't going to have a useful way to distinguish between them. Note that e.g. we're going to show the #include path in the detail, but the documentation is going to be identical for each.
• we lose the invariant that each completion item (pre-bundling) maps to a different symbol
• C++ embedders don't have the option of displaying the multiple options once the completion is selected

The alternative approach of sorting the includes by proximity * log(refs) or so, and then using the top one for scoring, seems less of a drastic change to the current behavior. (Visible effect: more accurate includes inserted).

clangd/Quality.cpp

190 ↗

(On Diff #162650)

This looks a bit sketchy. Particularly the += boost where everything else is *=.
What's this trying to do?

clangd/index/Index.h

220

via this header -> and include this header?

(otherwise, what does "via" mean?)

clangd/index/Merge.cpp

105

This describes the logic, and the logic always produces the right result, but it's not clear *why*. Maybe add something like:

This is associative because it preserves the invariant:
 - if we haven't seen a definition, Includes covers all declarations
 - if we have seen a definition, Includes covers declarations visible from any definition

in fact it seems hard to reason about this field in Symbol without understanding this, so maybe this invariant belongs on the IncludeHeaders field itself.

105

Thinking more about it - what's the intent here?
I'm not sure sorting by (seen by def, #refs) produces better ranking than just #refs.

But there are other possible reasons for dropping includes not seen by any def:

• remove spam from the completion list (only a problem if we clone the completion items)
• reduce size for items that are often redeclared (I can imagine this being a problem, not obvious)

Curious what your thinking is.

[ioeric]

• Addressed review comments.

[ioeric]

Thanks for the review! I reduced the scope of the patch. PTAL

clangd/CodeComplete.cpp

1418

These are all valid points. I agree that we should start with less drastic change in the first version.

My concern was that there can be cases where it's impossible for clangd to suggest the correct #include (e.g. all includes have the same proximity and popularity). But I guess there are also alternative solutions to these than creating multiple completion results. For example, we could simply give up inserting include during code completion and let an include-fixer-like feature handle it.

clangd/Quality.cpp

190 ↗

(On Diff #162650)

(This is no longer needed in this patch.)

As we were generating multiple candidates for the same symbol (for each include), I was trying to group them together using the small score differences as the tie breaker.

clangd/index/Merge.cpp

105

in fact it seems hard to reason about this field in Symbol without understanding this, so maybe this invariant belongs on the IncludeHeaders field itself.

Make sense. Thanks!

Thinking more about it - what's the intent here?

The intention is basically to filter out headers with forward declarations where definition is not seen. I could hardly think of a case where we would favor headers where def is not seen over those where definition is seen.

[ioeric]

• minor cleanup

[sammccall]

• It would be useful for the C++ API (CodeCompletion struct) to provide the includes/edits in ranked order, if possible. Frontends could experiment with showing all the options.
• Still not sure that adding more complicated behavior to Code Complete (vs just improving ranking) is the right thing for now (bigger comment below)

clangd/CodeComplete.cpp

303

I'm not sure about this:

• we're adding new user-visible behavior which is potentially confusing (not inserting even though insertion is turned on and we're offering a completion that requires insertion)
• it's not clear that first > 10 * second is the right heuristic or tuning
• this probably isn't the right place for this heuristic - e.g. it causes us not to show the location of the symbol in the detail field. (IIRC, the fact that this is only shown when header insertion is active is a limitation we wanted to fix, not furither entrench).

Can we start with the simplest behavior, closest to what we do today: just choose the most popular include?

Probably the most forward-looking way to do this is to pick the winner when we construct the CompletionCandidate and stash it in a field there. (The scoring function is going to become more complex as we add proximity, and this function gets called twice. Also this function won't have access to the info needed for proximity).

316

nit: Includes[0] and Includes[1]?

[ioeric]

• Return all possible #includes in CodeComplete.

[sammccall]

clangd/CodeComplete.cpp

407

nit: this could be std::stable_partition. Not sure if faster, but maybe clearer.

clangd/CodeComplete.h

145 ↗

(On Diff #163351)

if we've bundled together overloads that have different sets of providing headers, these includes may not be accurate for all of them. Maybe comment this limitation?

[ioeric]

• Rebase

[ioeric]

• Document limitation for overload bundling.

clangd/CodeComplete.cpp

 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/ScopedPrinter.h"
+#include <algorithm>
+#include <iterator>
 #include <queue>
 
 // We log detailed candidate here if you run with -debug-only=codecomplete.
 #define DEBUG_TYPE "CodeComplete"
 
 namespace clang {
 namespace clangd {
 namespace {
size_t overloadSet() const {
     {
       llvm::raw_svector_ostream OS(Scratch);
       D->printQualifiedName(OS);
     }
     return hash_combine(Scratch, headerToInsertIfNotPresent().getValueOr(""));
   }
 
   llvm::Optional<llvm::StringRef> 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
       // file e.g. the symbol is forward declared.
       auto &SM = SemaResult->Declaration->getASTContext().getSourceManager();
       for (const Decl *RD : SemaResult->Declaration->redecls())
         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<CompletionCandidate, 4>;
 };
 using ScoredBundle =
     std::pair<CompletionCandidate::Bundle, CodeCompletion::Scores>;
 struct ScoredBundleGreater {
   bool operator()(const ScoredBundle &L, const ScoredBundle &R) {
if (auto Inserted = C.headerToInsertIfNotPresent()) {
       }();
       if (Include) {
         Completion.Header = Include->first;
         if (Include->second)
           Completion.HeaderInsertion = Includes.insert(Include->first);
       } 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);
     }
   }
 
   void add(const CompletionCandidate &C, CodeCompletionString *SemaCCS) {
     assert(bool(C.SemaResult) == bool(SemaCCS));
     Bundled.emplace_back();
     BundledEntry &S = Bundled.back();
     if (C.SemaResult) {
       getSignature(*SemaCCS, &S.Signature, &S.SnippetSuffix,
                    &Completion.RequiredQualifier);
       S.ReturnType = getReturnType(*SemaCCS);
mergeResults(const std::vector<CodeCompletionResult> &SemaResults,
     llvm::DenseSet<const Symbol *> UsedIndexResults;
     auto CorrespondingIndexResult =
         [&](const CodeCompletionResult &SemaResult) -> const Symbol * {
       if (auto SymID = getSymbolID(SemaResult)) {
         auto I = IndexResults.find(*SymID);
         if (I != IndexResults.end()) {
           UsedIndexResults.insert(&*I);
           return &*I;
         }
       }
       return nullptr;
     };
     // Emit all Sema results, merging them with Index results if possible.
     for (auto &SemaResult : Recorder->Results)
       AddToBundles(&SemaResult, CorrespondingIndexResult(SemaResult));
     // Handle OverrideResults the same way we deal with SemaResults. Since these
     // results use the same structs as a SemaResult it is safe to do that, but

clangd/index/Index.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_INDEX_H
 
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/DenseMap.h"
 #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 <array>
 #include <string>
 #include <tuple>
 
 namespace clang {
 namespace clangd {
 
struct Symbol {
   /// A brief description of the symbol that can be appended in the completion
   /// candidate list. For example, "(X x, Y y) const" is a function signature.
   llvm::StringRef Signature;
   /// What to insert when completing this symbol, after the symbol name.
   /// This is in LSP snippet syntax (e.g. "({$0})" for a no-args function).
   /// (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<IncludeHeaderWithReferences, 1> 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
   /// and have clients resolve full symbol information for a specific candidate
   /// if needed.
   struct Details {
     /// Documentation including comment for the symbol declaration.
     llvm::StringRef Documentation;
     /// 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.
   const Details *Detail = nullptr;
 
   // FIXME: add all occurrences support.
   // FIXME: add extra fields for index scoring signals.
 };

clangd/index/Index.cpp

 //===--- Index.cpp -----------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Index.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
 using namespace llvm;
 
 raw_ostream &operator<<(raw_ostream &OS, const SymbolLocation &L) {
static void own(Symbol &S, llvm::UniqueStringSaver &Strings,
   Intern(S.Name);
   Intern(S.Scope);
   Intern(S.CanonicalDeclaration.FileURI);
   Intern(S.Definition.FileURI);
 
   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<Symbol::Details>();
     *Detail = *S.Detail;
     // Intern the actual strings.
     Intern(Detail->Documentation);
     Intern(Detail->ReturnType);
-    Intern(Detail->IncludeHeader);
     // Replace the detail pointer with our copy.
     S.Detail = Detail;
   }
 }
 
 void SymbolSlab::Builder::insert(const Symbol &S) {
   auto R = SymbolIndex.try_emplace(S.ID, Symbols.size());
   if (R.second) {

clangd/index/Merge.cpp

 
 Symbol
 mergeSymbol(const Symbol &L, const Symbol &R, Symbol::Details *Scratch) {
   assert(L.ID == R.ID);
   // We prefer information from TUs that saw the definition.
   // 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.
 
   // For each optional field, fill it from O if missing in S.
   // (It might be missing in O too, but that's a no-op).
   if (!S.Definition)
     S.Definition = O.Definition;
   if (!S.CanonicalDeclaration)
     S.CanonicalDeclaration = O.CanonicalDeclaration;
   S.References += O.References;
   if (S.Signature == "")
     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) {
       // Copy into scratch space so we can merge.
       *Scratch = *S.Detail;
       if (Scratch->Documentation == "")
         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;
   }
 
   S.Origin |= O.Origin | SymbolOrigin::Merge;
   return S;
 }
 
 std::unique_ptr<SymbolIndex> mergeIndex(const SymbolIndex *Dynamic,
                                         const SymbolIndex *Static) {
   return llvm::make_unique<MergedIndex>(Dynamic, Static);
 }
 
 } // namespace clangd
 } // namespace clang

clangd/index/SymbolCollector.cpp

bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
 
   std::string Include;
   if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
     if (auto Header =
             getIncludeHeader(Name->getName(), SM,
                              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;
 }
 
 void SymbolCollector::finish() {
   // At the end of the TU, add 1 to the refcount of all referenced symbols.
   auto IncRef = [this](const SymbolID &ID) {
if (auto Header = getIncludeHeader(
             QName, SM, SM.getExpansionLoc(ND.getLocation()), Opts))
       Include = std::move(*Header);
   }
   S.Signature = Signature;
   S.CompletionSnippetSuffix = SnippetSuffix;
   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;
   Symbols.insert(S);
   return Symbols.find(S.ID);
 }
 
 void SymbolCollector::addDefinition(const NamedDecl &ND,

clangd/index/SymbolYAML.cpp

 //===--- SymbolYAML.cpp ------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #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 {
 
 using clang::clangd::Symbol;
 using clang::clangd::SymbolID;
 using clang::clangd::SymbolLocation;
 using clang::index::SymbolInfo;
static void mapping(IO &io, SymbolInfo &SymInfo) {
     io.mapRequired("Lang", SymInfo.Lang);
   }
 };
 
 template <> struct MappingTraits<Symbol::Details> {
   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<clang::clangd::Symbol::IncludeHeaderWithReferences> {
+  static void mapping(IO &io,
+                      clang::clangd::Symbol::IncludeHeaderWithReferences &Inc) {
+    io.mapRequired("Header", Inc.IncludeHeader);
+    io.mapRequired("References", Inc.References);
   }
 };
 
 // A YamlIO normalizer for fields of type "const T*" allocated on an arena.
 // Normalizes to Optional<T>, so traits should be provided for T.
 template <typename T> struct ArenaPtr {
   ArenaPtr(IO &) {}
   ArenaPtr(IO &, const T *D) {
static void mapping(IO &IO, Symbol &Sym) {
     IO.mapOptional("CanonicalDeclaration", Sym.CanonicalDeclaration,
                    SymbolLocation());
     IO.mapOptional("Definition", Sym.Definition, SymbolLocation());
     IO.mapOptional("References", Sym.References, 0u);
     IO.mapOptional("IsIndexedForCodeCompletion", Sym.IsIndexedForCodeCompletion,
                    false);
     IO.mapOptional("Signature", Sym.Signature);
     IO.mapOptional("CompletionSnippetSuffix", Sym.CompletionSnippetSuffix);
+    IO.mapOptional("IncludeHeaders", Sym.IncludeHeaders);
     IO.mapOptional("Detail", NDetail->Opt);
   }
 };
 
 template <> struct ScalarEnumerationTraits<SymbolLanguage> {
   static void enumeration(IO &IO, SymbolLanguage &Value) {
     IO.enumCase(Value, "C", SymbolLanguage::C);
     IO.enumCase(Value, "Cpp", SymbolLanguage::CXX);

unittests/clangd/CodeCompleteTests.cpp

TEST(CompletionTest, IncludeInsertionPreprocessorIntegrationTests) {
   FS.Files[BarHeader] = "";
 
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
   Symbol::Details Scratch;
   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,
                              R"cpp(
           int main() { ns::^ }
       )cpp",
                              {Sym});
   EXPECT_THAT(Results.Completions,
TEST(CompletionTest, NoIncludeInsertionWhenDeclFoundInFile) {
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
   Symbol::Details Scratch;
   Symbol SymX = cls("ns::X");
   Symbol SymY = cls("ns::Y");
   std::string BarHeader = testPath("bar.h");
   auto BarURI = URI::createFile(BarHeader).toString();
   SymX.CanonicalDeclaration.FileURI = BarURI;
   SymY.CanonicalDeclaration.FileURI = BarURI;
-  Scratch.IncludeHeader = "<bar>";
+
+  SymX.IncludeHeaders.emplace_back("<bar>", 1);
+  SymY.IncludeHeaders.emplace_back("<bar>", 1);
+
   SymX.Detail = &Scratch;
   SymY.Detail = &Scratch;
   // Shoten include path based on search dirctory and insert.
   auto Results = completions(Server,
                              R"cpp(
           namespace ns {
             class X;
             class Y {}
TEST(CompletionTest, OverloadBundling) {
   EXPECT_THAT(
       completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).Completions,
       UnorderedElementsAre(Labeled("GFuncC(…)"), Labeled("GFuncD(int)")));
 
   // Differences in header-to-insert suppress bundling.
   Symbol::Details Detail;
   std::string DeclFile = URI::createFile(testPath("foo")).toString();
   NoArgsGFunc.CanonicalDeclaration.FileURI = DeclFile;
-  Detail.IncludeHeader = "<foo>";
+  NoArgsGFunc.IncludeHeaders.emplace_back("<foo>", 1);
+
   NoArgsGFunc.Detail = &Detail;
   EXPECT_THAT(
       completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).Completions,
       UnorderedElementsAre(AllOf(Named("GFuncC"), InsertInclude("<foo>")),
                            Labeled("GFuncC(int)"), Labeled("GFuncD(int)")));
 
   // Examine a bundled completion in detail.
   auto A =
TEST(CompletionTest, EnableSpeculativeIndexRequest) {
 
   CompleteAtPoint("3");
   // Speculation failed. Sent speculative index request and the new index
   // request after sema.
   auto Reqs3 = Requests.consumeRequests();
   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("<foo>", 2);
+  sym.IncludeHeaders.emplace_back("<bar>", 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("<foo>", 2);
+  sym.IncludeHeaders.emplace_back("<bar>", 1000);
+
+  EXPECT_THAT(
+      completions("Fun^", {sym}).Completions,
+      UnorderedElementsAre(AllOf(Named("Func"), InsertInclude("<bar>"))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

unittests/clangd/FileIndexTests.cpp

 TEST(FileIndexTest, NoIncludeCollected) {
   FileIndex M;
   update(M, "f", "class string {};");
 
   FuzzyFindRequest Req;
   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);
 }
 
 TEST(FileIndexTest, TemplateParamsInLabel) {
   auto Source = R"cpp(
 template <class Ty>

unittests/clangd/IndexTests.cpp

TEST(MergeTest, PreferSymbolWithDefn) {
 
   R.Definition.FileURI = "file:/right.cpp"; // Now right will be favored.
   M = mergeSymbol(L, R, &Scratch);
   EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:/right.h");
   EXPECT_EQ(M.Definition.FileURI, "file:/right.cpp");
   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

unittests/clangd/SymbolCollectorTests.cpp

 MATCHER_P(Doc, D, "") { return arg.Detail && arg.Detail->Documentation == D; }
 MATCHER_P(Snippet, S, "") {
   return (arg.Name + arg.CompletionSnippetSuffix).str() == S;
 }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
 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,
                   arg.CanonicalDeclaration.Start.Column,
                   arg.CanonicalDeclaration.End.Line,
                   arg.CanonicalDeclaration.End.Column) ==
          std::tie(Pos.start.line, Pos.start.character, Pos.end.line,
                   Pos.end.character);
CanonicalDeclaration:
   FileURI:        file:///path/foo.h
   Start:
     Line: 1
     Column: 0
   End:
     Line: 1
     Column: 1
 IsIndexedForCodeCompletion:    true
+IncludeHeaders:
+  - Header:    'include1'
+    References:    7
+  - Header:    'include2'
+    References:    3
 Detail:
   Documentation:    'Foo doc'
   ReturnType:    'int'
 ...
 )";
   const std::string YAML2 = R"(
 ---
 ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF858
)";
 
   auto Symbols1 = symbolsFromYAML(YAML1);
 
   EXPECT_THAT(Symbols1,
               UnorderedElementsAre(AllOf(QName("clang::Foo1"), Labeled("Foo1"),
                                          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"),
                             Not(HasReturnType()), DeclURI("file:///path/bar.h"),
                             ForCodeCompletion(false))));
 
   std::string ConcatenatedYAML;
   {
     llvm::raw_string_ostream OS(ConcatenatedYAML);
     SymbolsToYAML(Symbols1, OS);
     SymbolsToYAML(Symbols2, OS);
   }
   auto ConcatenatedSymbols = symbolsFromYAML(ConcatenatedYAML);
   EXPECT_THAT(ConcatenatedSymbols,
               UnorderedElementsAre(QName("clang::Foo1"),
                                    QName("clang::Foo2")));
 }
 
 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
 TEST_F(SymbolCollectorTest, CanonicalSTLHeader) {
   CollectorOpts.CollectIncludePath = true;
   CanonicalIncludes Includes;
   addSystemHeadersMapping(&Includes);
   CollectorOpts.Includes = &Includes;