[clangd] UI for completion items that would trigger include insertion.

[ioeric]

Summary

For completion items that would trigger include insertions (i.e. index symbols
that are not #included yet), add a visual indicator "+" before the completion
label. The inserted headers will appear in the completion detail.

Open to suggestions for better visual indicators; "+" was picked because it
seems cleaner than a few other candidates I've tried (*, #, @ ...).

The displayed header would be like a/b/c.h (without quote) or <vector> for system
headers. I didn't add quotation or "#include" because they can take up limited
space and do not provide additional information after users know what the
headers are. I think a header alone should be obvious for users to infer that
this is an include header..

To align indentation, also prepend ' ' to labels of candidates that would not
trigger include insertions (only for completions where index results are
possible).

Vim:

vscode:

[sammccall]

Ooh, bikeshed first!

clangd/CodeComplete.cpp

313

I think we should avoid tokens that occur commonly in C++ (possibly with the exception of # which is used for this purpose. We should also avoid doublewide chars because of alignment (even assuming a monospace font is risky...)

Ideas:

• something like "external link" icon on wikipedia - but no such thing in unicode :-(🔗(U+1F517 link) looks truly terrible in at least some fonts
• variant on plus like ⊕ (U+2295 circled plus) - can't find one I like a lot
• some kind of arrow like ☇ (U+2607 lightning) or ⇱ (U+21F1 north west arrow to corner) or ⮣ (upwards triangle-headed arrow with long head rightwards)
• variant on hash like ﹟(U+FE5F small number sign)
• something unrelated but "special" like ※ (U+203B reference mark)

I'm not really happy with any of these :-(

[ioeric]

• s/+/•/ and make the icon an code completion option to avoid hardcodes in multiple places.

clangd/CodeComplete.cpp

313

Thanks for the suggestions!

After playing around with different options, we decided to go with • which is simple and also aligns well in vim and vscode. We tried to avoid symbols that are meaningful/complicated because they tend to add noise when users are used to the existence of the icon, and they appear less elegant when there are a whole list of indicators in the completion results.

[sammccall]

clangd/CodeComplete.cpp

191

Hmm, I'm not sure this should be conditional.
Especially in editors that render [icon]label, inconsistently having a space or not for no obvious reason seems disorienting to users.

And it adds some complexity (especially in conjunction with bundling...)

301

nit: .hasValue() would be clearer than static_cast

313

this is going to be slightly tricky with bundling, where we take the CompletionItem for an arbitrary item, and recompute the label as Name + "(...)".

Note that if we're unconditionally adding the padding character, we can just grab it out of the old CompletionItem.label :-) Yes it's a hack, but otherwise we need to pass this information around or factor quite differently.

clangd/Headers.cpp

74–75

we don't need Expected here anymore?

clangd/Headers.h

52–53

why the change from empty string here? does this anticipate adding the path to the detail/doc unconditionally in a future patch?

This is quite a hard signature to understand and describe. At first glance it seems we can just split function into bool shouldAddInclude() and calculateIncludePath(), they won't overlap?

87–94

this signature is scary :-)

One option is to separate:

• bool shouldInsert(Declaring, Inserted) (probably just log errors and return false)
• string spell(Inserted)
• TextEdit insert(Spelled)

another is a struct, but I think making the flow a little more explicit at the callsite might be clearer.

[sammccall]

Oops, forgot to link to the bundling patch: D47957
We should expect some conflicts one way or the other.

[ioeric]

• Addressed review comments.
• Merged with orgin/master

[ioeric]

Thanks for the review!

clangd/CodeComplete.cpp

191

Sure. I wasn't sure about this and wanted get your opinion before fixing tests.

[sammccall]

clangd/CodeComplete.cpp

301

This is fine for now, but can you add a FIXME to consider what to show if there's no include to be inserted, and for sema results? There's some value in showing the header consistently I think.

302

can we add the \n for now even if detail is empty?
That way the editors that "inline" the detail into the same line as the label won't show it (if we patch YCM to truncate like VSCode does).
The inconsistency (sometimes showing a return type and sometimes a header) seems surprising.

313

string(IncludeInsertionIndicator.size(), ' ')?

335–341

again, need to account for the length of the indicator

clangd/CodeComplete.h

70

if you want this to be part of the doc comment, move it to the bottom?

I think the intent was that this comment apply to all following members. So keeping the line as // and adding the new member above it would also make sense

[sammccall]

clangd/CodeComplete.cpp

313

oops, I made the same mistake - #bytes in the indicator != width of the indicator.

maybe making this customizable is too much hassle

[ioeric]

• addressed review comments.

[ioeric]

clangd/CodeComplete.h

70

Ohh, didn't know this was intentional. Done.

[ioeric]

• fix typo and clang-format

[ioeric]

• clang-format again...

clangd/CodeComplete.h

struct CodeCompleteOptions {
 
   /// Combine overloads into a single completion item where possible.
   bool BundleOverloads = false;
 
   /// Limit the number of results returned (0 means no limit).
   /// If more results are available, we set CompletionList.isIncomplete.
   size_t Limit = 0;
 
+  /// A visual indicator to prepend to the completion label to indicate whether
+  /// completion result would trigger an #include insertion or not.
+  struct IncludeInsertionIndicator {
+    std::string Insert = "•";
+    std::string NoInsert = " ";
+  } IncludeIndicator;
+
   // Populated internally by clangd, do not set.
   /// If `Index` is set, it is used to augment the code completion
   /// results.
   /// FIXME(ioeric): we might want a better way to pass the index around inside
   /// clangd.
   const SymbolIndex *Index = nullptr;
 };
 
 /// Get code completions at a specified \p Pos in \p FileName.

clangd/CodeComplete.cpp

 /// A code completion result, in clang-native form.
 /// It may be promoted to a CompletionItem if it's among the top-ranked results.
 struct CompletionCandidate {
   llvm::StringRef Name; // Used for filtering and sorting.
   // We may have a result from Sema, from the index, or both.
   const CodeCompletionResult *SemaResult = nullptr;
   const Symbol *IndexResult = nullptr;
 
   // Returns a token identifying the overload set this is part of.
   // 0 indicates it's not part of any overload set.
   size_t overloadSet() const {
     SmallString<256> Scratch;
     if (IndexResult) {
       switch (IndexResult->SymInfo.Kind) {
       case index::SymbolKind::ClassMethod:
       case index::SymbolKind::InstanceMethod:
       case index::SymbolKind::StaticMethod:
llvm::Optional<llvm::StringRef> headerToInsertIfNotPresent() const {
     }
     return IndexResult->Detail->IncludeHeader;
   }
 
   // Builds an LSP completion item.
   CompletionItem build(StringRef FileName, const CompletionItemScores &Scores,
                        const CodeCompleteOptions &Opts,
                        CodeCompletionString *SemaCCS,
-                       const IncludeInserter *Includes,
+                       const IncludeInserter &Includes,
                        llvm::StringRef SemaDocComment) const {
     assert(bool(SemaResult) == bool(SemaCCS));
     CompletionItem I;
+    bool InsertingInclude = false; // Whether a new #include will be added.
     if (SemaResult) {
       I.kind = toCompletionItemKind(SemaResult->Kind, SemaResult->Declaration);
       getLabelAndInsertText(*SemaCCS, &I.label, &I.insertText,
                             Opts.EnableSnippets);
       I.filterText = getFilterText(*SemaCCS);
       I.documentation = formatDocumentation(*SemaCCS, SemaDocComment);
       I.detail = getDetail(*SemaCCS);
     }
if (IndexResult) {
                            : IndexResult->CompletionPlainInsertText;
 
       if (auto *D = IndexResult->Detail) {
         if (I.documentation.empty())
           I.documentation = D->Documentation;
         if (I.detail.empty())
           I.detail = D->CompletionDetail;
         if (auto Inserted = headerToInsertIfNotPresent()) {
-          auto Edit = [&]() -> Expected<Optional<TextEdit>> {
+          auto IncludePath = [&]() -> Expected<std::string> {
             auto ResolvedDeclaring = toHeaderFile(
                 IndexResult->CanonicalDeclaration.FileURI, FileName);
             if (!ResolvedDeclaring)
               return ResolvedDeclaring.takeError();
             auto ResolvedInserted = toHeaderFile(*Inserted, FileName);
             if (!ResolvedInserted)
               return ResolvedInserted.takeError();
-            return Includes->insert(*ResolvedDeclaring, *ResolvedInserted);
+            if (!Includes.shouldInsertInclude(*ResolvedDeclaring,
+                                              *ResolvedInserted))
+              return "";
+            return Includes.calculateIncludePath(*ResolvedDeclaring,
+                                                 *ResolvedInserted);
           }();
-          if (!Edit) {
+          if (!IncludePath) {
             std::string ErrMsg =
                 ("Failed to generate include insertion edits for adding header "
                  "(FileURI=\"" +
                  IndexResult->CanonicalDeclaration.FileURI +
                  "\", IncludeHeader=\"" + D->IncludeHeader + "\") into " +
                  FileName)
                     .str();
-            log(ErrMsg + ":" + llvm::toString(Edit.takeError()));
-          } else if (*Edit) {
-            I.additionalTextEdits = {std::move(**Edit)};
+            log(ErrMsg + ":" + llvm::toString(IncludePath.takeError()));
+          } else if (!IncludePath->empty()) {
+            // FIXME: consider what to show when there is no #include insertion,
+            // and for sema results, for consistency.
+            if (auto Edit = Includes.insert(*IncludePath)) {
+              I.detail += ("\n" + StringRef(*IncludePath).trim('"')).str();
+              I.additionalTextEdits = {std::move(*Edit)};
+              InsertingInclude = true;
             }
           }
         }
       }
+    }
+    I.label = (InsertingInclude ? Opts.IncludeIndicator.Insert
+                                : Opts.IncludeIndicator.NoInsert) +
+              I.label;
     I.scoreInfo = Scores;
     I.sortText = sortText(Scores.finalScore, Name);
     I.insertTextFormat = Opts.EnableSnippets ? InsertTextFormat::Snippet
                                              : InsertTextFormat::PlainText;
     return I;
   }
 
   using Bundle = llvm::SmallVector<CompletionCandidate, 4>;
 
   static CompletionItem build(const Bundle &Bundle, CompletionItem First,
                               const clangd::CodeCompleteOptions &Opts) {
     if (Bundle.size() == 1)
       return First;
     // Patch up the completion item to make it look like a bundle.
     // This is a bit of a hack but most things are the same.
 
     // Need to erase the signature. All bundles are function calls.
     llvm::StringRef Name = Bundle.front().Name;
     First.insertText =
         Opts.EnableSnippets ? (Name + "(${0})").str() : Name.str();
-    First.label = (Name + "(…)").str();
+    // Keep the visual indicator of the original label.
+    bool InsertingInclude =
+        StringRef(First.label).startswith(Opts.IncludeIndicator.Insert);
+    First.label = (Twine(InsertingInclude ? Opts.IncludeIndicator.Insert
+                                          : Opts.IncludeIndicator.NoInsert) +
+                   Name + "(…)")
+                      .str();
     First.detail = llvm::formatv("[{0} overloads]", Bundle.size());
     return First;
   }
 };
 using ScoredBundle =
     std::pair<CompletionCandidate::Bundle, CompletionItemScores>;
 struct ScoredBundleGreater {
   bool operator()(const ScoredBundle &L, const ScoredBundle &R) {
private:
   CompletionList runWithSema() {
     Filter = FuzzyMatcher(
         Recorder->CCSema->getPreprocessor().getCodeCompletionFilter());
     // Sema provides the needed context to query the index.
     // FIXME: in addition to querying for extra/overlapping symbols, we should
     //        explicitly request symbols corresponding to Sema results.
     //        We can use their signals even if the index can't suggest them.
     // We must copy index results to preserve them, but there are at most Limit.
-    auto IndexResults = queryIndex();
+    auto IndexResults = (Opts.Index && allowIndex(Recorder->CCContext))
+                            ? queryIndex()
+                            : SymbolSlab();
     // Merge Sema and Index results, score them, and pick the winners.
     auto Top = mergeResults(Recorder->Results, IndexResults);
     // Convert the results to the desired LSP structs.
     CompletionList Output;
     for (auto &C : Top)
       Output.items.push_back(toCompletionItem(C.first, C.second));
     Output.isIncomplete = Incomplete;
     return Output;
   }
 
   SymbolSlab queryIndex() {
-    if (!Opts.Index || !allowIndex(Recorder->CCContext))
-      return SymbolSlab();
     trace::Span Tracer("Query index");
     SPAN_ATTACH(Tracer, "limit", Opts.Limit);
 
     SymbolSlab::Builder ResultsBuilder;
     // Build the query.
     FuzzyFindRequest Req;
     if (Opts.Limit)
       Req.MaxCandidateCount = Opts.Limit;
if (auto *SR = Bundle.front().SemaResult) {
       if (Opts.IncludeComments) {
         assert(Recorder->CCSema);
         FrontDocComment = getDocComment(Recorder->CCSema->getASTContext(), *SR,
                                         /*CommentsFromHeader=*/false);
       }
     }
     return CompletionCandidate::build(
         Bundle,
-        Bundle.front().build(FileName, Scores, Opts, SemaCCS, Includes.get(),
+        Bundle.front().build(FileName, Scores, Opts, SemaCCS, *Includes,
                              FrontDocComment),
         Opts);
   }
 };
 
 CompletionList codeComplete(PathRef FileName,
                             const tooling::CompileCommand &Command,
                             PrecompiledPreamble const *Preamble,

clangd/Headers.h

struct Inclusion {
   std::string Written; // Inclusion name as written e.g. <vector>.
   Path Resolved;       // Resolved path of included file. Empty if not resolved.
 };
 
 /// Returns a PPCallback that visits all inclusions in the main file.
 std::unique_ptr<PPCallbacks>
 collectInclusionsInMainFileCallback(const SourceManager &SM,
                                     std::function<void(Inclusion)> Callback);
 
-/// Determines the preferred way to #include a file, taking into account the
-/// search path. Usually this will prefer a shorter representation like
-/// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'.
-///
-/// \param File is an absolute file path.
-/// \param Inclusions Existing inclusions in the main file.
-/// \param DeclaringHeader is the original header corresponding to \p
-/// InsertedHeader e.g. the header that declares a symbol.
-/// \param InsertedHeader The preferred header to be inserted. This could be the
-/// same as DeclaringHeader but must be provided.
-//  \return A quoted "path" or <path>. This returns an empty string if:
-///   - Either \p DeclaringHeader or \p InsertedHeader is already (directly)
-///   in \p Inclusions (including those included via different paths).
-///   - \p DeclaringHeader or \p InsertedHeader is the same as \p File.
-llvm::Expected<std::string> calculateIncludePath(
-    PathRef File, StringRef BuildDir, HeaderSearch &HeaderSearchInfo,
-    const std::vector<Inclusion> &Inclusions, const HeaderFile &DeclaringHeader,
-    const HeaderFile &InsertedHeader);
-
 // Calculates insertion edit for including a new header in a file.
 class IncludeInserter {
 public:
   IncludeInserter(StringRef FileName, StringRef Code,
                   const format::FormatStyle &Style, StringRef BuildDir,
                   HeaderSearch &HeaderSearchInfo)
       : FileName(FileName), Code(Code), BuildDir(BuildDir),
         HeaderSearchInfo(HeaderSearchInfo),
         Inserter(FileName, Code, Style.IncludeStyle) {}
 
   void addExisting(Inclusion Inc) { Inclusions.push_back(std::move(Inc)); }
 
-  /// Returns a TextEdit that inserts a new header; if the header is not
-  /// inserted e.g. it's an existing header, this returns None. If any header is
-  /// invalid, this returns error.
+  /// Checks whether to add an #include of the header into \p File.
+  /// An #include will not be added if:
+  ///   - Either \p DeclaringHeader or \p InsertedHeader is already (directly)
+  ///   in \p Inclusions (including those included via different paths).
+  ///   - \p DeclaringHeader or \p InsertedHeader is the same as \p File.
+  ///
+  /// \param DeclaringHeader is the original header corresponding to \p
+  /// InsertedHeader e.g. the header that declares a symbol.
+  /// \param InsertedHeader The preferred header to be inserted. This could be
+  /// the same as DeclaringHeader but must be provided. \param Inclusions
+  /// Existing includes in the main file.
+  bool shouldInsertInclude(const HeaderFile &DeclaringHeader,
+                           const HeaderFile &InsertedHeader) const;
+
+  /// Determines the preferred way to #include a file, taking into account the
+  /// search path. Usually this will prefer a shorter representation like
+  /// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'.
   ///
   /// \param DeclaringHeader is the original header corresponding to \p
   /// InsertedHeader e.g. the header that declares a symbol.
   /// \param InsertedHeader The preferred header to be inserted. This could be
   /// the same as DeclaringHeader but must be provided.
-  Expected<Optional<TextEdit>> insert(const HeaderFile &DeclaringHeader,
-                                      const HeaderFile &InsertedHeader) const;
+  ///
+  /// \return A quoted "path" or <path> to be included.
+  std::string calculateIncludePath(const HeaderFile &DeclaringHeader,
 
+  /// Calculates an edit that inserts \p VerbatimHeader into code. If the header
+  /// is already included, this returns None.
+  llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader) const;
+
 private:
   StringRef FileName;
   StringRef Code;
   StringRef BuildDir;
   HeaderSearch &HeaderSearchInfo;
   std::vector<Inclusion> Inclusions;
   tooling::HeaderIncludes Inserter; // Computers insertion replacement.
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_HEADERS_H

clangd/Headers.cpp

 
 std::unique_ptr<PPCallbacks>
 collectInclusionsInMainFileCallback(const SourceManager &SM,
                                     std::function<void(Inclusion)> Callback) {
   return llvm::make_unique<RecordHeaders>(SM, std::move(Callback));
 }
 
 /// FIXME(ioeric): we might not want to insert an absolute include path if the
 /// path is not shortened.
-llvm::Expected<std::string> calculateIncludePath(
-    PathRef File, StringRef BuildDir, HeaderSearch &HeaderSearchInfo,
-    const std::vector<Inclusion> &Inclusions, const HeaderFile &DeclaringHeader,
-    const HeaderFile &InsertedHeader) {
+bool IncludeInserter::shouldInsertInclude(
+    const HeaderFile &DeclaringHeader, const HeaderFile &InsertedHeader) const {
   assert(DeclaringHeader.valid() && InsertedHeader.valid());
-  if (File == DeclaringHeader.File || File == InsertedHeader.File)
-    return "";
+  if (FileName == DeclaringHeader.File || FileName == InsertedHeader.File)
+    return false;
   llvm::StringSet<> IncludedHeaders;
   for (const auto &Inc : Inclusions) {
     IncludedHeaders.insert(Inc.Written);
     if (!Inc.Resolved.empty())
       IncludedHeaders.insert(Inc.Resolved);
   }
   auto Included = [&](llvm::StringRef Header) {
     return IncludedHeaders.find(Header) != IncludedHeaders.end();
   };
-  if (Included(DeclaringHeader.File) || Included(InsertedHeader.File))
-    return "";
-
-  bool IsSystem = false;
+  return !Included(DeclaringHeader.File) && !Included(InsertedHeader.File);
+}
 
+std::string
+IncludeInserter::calculateIncludePath(const HeaderFile &DeclaringHeader,
+                                      const HeaderFile &InsertedHeader) const {
+  assert(DeclaringHeader.valid() && InsertedHeader.valid());
   if (InsertedHeader.Verbatim)
     return InsertedHeader.File;
-
+  bool IsSystem = false;
   std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
       InsertedHeader.File, BuildDir, &IsSystem);
   if (IsSystem)
     Suggested = "<" + Suggested + ">";
   else
     Suggested = "\"" + Suggested + "\"";
-
-  log("Suggested #include for " + InsertedHeader.File + " is: " + Suggested);
   return Suggested;
 }
 
-Expected<Optional<TextEdit>>
-IncludeInserter::insert(const HeaderFile &DeclaringHeader,
-                        const HeaderFile &InsertedHeader) const {
-  auto Validate = [](const HeaderFile &Header) {
-    return Header.valid()
-               ? llvm::Error::success()
-               : llvm::make_error<llvm::StringError>(
-                     "Invalid HeaderFile: " + Header.File +
-                         " (verbatim=" + std::to_string(Header.Verbatim) + ").",
-                     llvm::inconvertibleErrorCode());
-  };
-  if (auto Err = Validate(DeclaringHeader))
-    return std::move(Err);
-  if (auto Err = Validate(InsertedHeader))
-    return std::move(Err);
-  auto Include =
-      calculateIncludePath(FileName, BuildDir, HeaderSearchInfo, Inclusions,
-                           DeclaringHeader, InsertedHeader);
-  if (!Include)
-    return Include.takeError();
-  if (Include->empty())
-    return llvm::None;
-  StringRef IncludeRef = *Include;
-  auto Insertion =
-      Inserter.insert(IncludeRef.trim("\"<>"), IncludeRef.startswith("<"));
-  if (!Insertion)
-    return llvm::None;
-  return replacementToEdit(Code, *Insertion);
+Optional<TextEdit> IncludeInserter::insert(StringRef VerbatimHeader) const {
+  Optional<TextEdit> Edit = None;
+  if (auto Insertion = Inserter.insert(VerbatimHeader.trim("\"<>"),
+                                       VerbatimHeader.startswith("<")))
+    Edit = replacementToEdit(Code, *Insertion);
+  return Edit;
 }
 
 } // namespace clangd
 } // namespace clang

test/clangd/completion-snippets.test

 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "isIncomplete": {{.*}}
 # CHECK-NEXT:    "items": [
 # CHECK:           "filterText": "func_with_args",
 # CHECK-NEXT:      "insertText": "func_with_args(${1:int a}, ${2:int b})",
 # CHECK-NEXT:      "insertTextFormat": 2,
 # CHECK-NEXT:      "kind": 3,
-# CHECK-NEXT:      "label": "func_with_args(int a, int b)",
+# CHECK-NEXT:      "label": " func_with_args(int a, int b)",
 # CHECK-NEXT:      "sortText": "{{.*}}func_with_args"
 # CHECK-NEXT:    }
 # CHECK-NEXT:    ]
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":4,"method":"shutdown"}

test/clangd/completion.test

 # CHECK-NEXT:    "isIncomplete": false,
 # CHECK-NEXT:    "items": [
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "detail": "int",
 # CHECK-NEXT:      "filterText": "a",
 # CHECK-NEXT:      "insertText": "a",
 # CHECK-NEXT:      "insertTextFormat": 1,
 # CHECK-NEXT:      "kind": 5,
-# CHECK-NEXT:      "label": "a",
+# CHECK-NEXT:      "label": " a",
 # CHECK-NEXT:      "sortText": "{{.*}}a"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  ]
 ---
 # Update the source file and check for completions again.
 {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///main.cpp","version":2},"contentChanges":[{"text":"struct S { int b; };\nint main() {\nS().\n}"}]}}
 ---
 {"jsonrpc":"2.0","id":3,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":4}}}
 #      CHECK:  "id": 3,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "isIncomplete": false,
 # CHECK-NEXT:    "items": [
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "detail": "int",
 # CHECK-NEXT:      "filterText": "b",
 # CHECK-NEXT:      "insertText": "b",
 # CHECK-NEXT:      "insertTextFormat": 1,
 # CHECK-NEXT:      "kind": 5,
-# CHECK-NEXT:      "label": "b",
+# CHECK-NEXT:      "label": " b",
 # CHECK-NEXT:      "sortText": "{{.*}}b"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  ]
 ---
 {"jsonrpc":"2.0","id":4,"method":"shutdown"}

test/clangd/protocol.test

 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "isIncomplete": false,
 # CHECK-NEXT:    "items": [
 #      CHECK:        "filterText": "a",
 # CHECK-NEXT:        "insertText": "a",
 # CHECK-NEXT:        "insertTextFormat": 1,
 # CHECK-NEXT:        "kind": 5,
-# CHECK-NEXT:        "label": "a",
+# CHECK-NEXT:        "label": " a",
 # CHECK-NEXT:        "sortText": "{{.*}}"
 #      CHECK:    ]
 # CHECK-NEXT:  }
 
 X-Test: Testing
 Content-Type: application/vscode-jsonrpc; charset-utf-8
 Content-Length: 146
 Content-Type: application/vscode-jsonrpc; charset-utf-8

 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "isIncomplete": false,
 # CHECK-NEXT:    "items": [
 #      CHECK:        "filterText": "a",
 # CHECK-NEXT:        "insertText": "a",
 # CHECK-NEXT:        "insertTextFormat": 1,
 # CHECK-NEXT:        "kind": 5,
-# CHECK-NEXT:        "label": "a",
+# CHECK-NEXT:        "label": " a",
 # CHECK-NEXT:        "sortText": "{{.*}}"
 #      CHECK:    ]
 # CHECK-NEXT:  }
 # STDERR: Warning: Duplicate Content-Length header received. The previous value for this message (10) was ignored.
 
 Content-Type: application/vscode-jsonrpc; charset-utf-8
 Content-Length: 10
 

 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "isIncomplete": false,
 # CHECK-NEXT:    "items": [
 #      CHECK:        "filterText": "a",
 # CHECK-NEXT:        "insertText": "a",
 # CHECK-NEXT:        "insertTextFormat": 1,
 # CHECK-NEXT:        "kind": 5,
-# CHECK-NEXT:        "label": "a",
+# CHECK-NEXT:        "label": " a",
 # CHECK-NEXT:        "sortText": "{{.*}}"
 #      CHECK:    ]
 # CHECK-NEXT:  }
 Content-Length: 1024
 
 {"jsonrpc":"2.0","id":5,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:/main.cpp"},"position":{"line":3,"character":5}}}
 # Test message which reads beyond the end of the stream.
 #
 # Ensure this is the last test in the file!
 # STDERR: Input was aborted. Read only {{[0-9]+}} bytes of expected {{[0-9]+}}.
 

unittests/clangd/CodeCompleteTests.cpp

 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
   void onDiagnosticsReady(PathRef File,
                           std::vector<Diag> Diagnostics) override {}
 };
 
 // GMock helpers for matching completion items.
 MATCHER_P(Named, Name, "") { return arg.insertText == Name; }
-MATCHER_P(Labeled, Label, "") { return arg.label == Label; }
+MATCHER_P(Labeled, Label, "") {
+  std::string Indented;
+  if (!StringRef(Label).startswith(
+          CodeCompleteOptions().IncludeIndicator.Insert) &&
+      !StringRef(Label).startswith(
+          CodeCompleteOptions().IncludeIndicator.NoInsert))
+    Indented =
+        (Twine(CodeCompleteOptions().IncludeIndicator.NoInsert) + Label).str();
+  else
+    Indented = Label;
+  return arg.label == Indented;
+}
+MATCHER_P(SigHelpLabeled, Label, "") { return arg.label == Label; }
 MATCHER_P(Kind, K, "") { return arg.kind == K; }
 MATCHER_P(Filter, F, "") { return arg.filterText == F; }
 MATCHER_P(Doc, D, "") { return arg.documentation == D; }
 MATCHER_P(Detail, D, "") { return arg.detail == D; }
 MATCHER_P(InsertInclude, IncludeHeader, "") {
   if (arg.additionalTextEdits.size() != 1)
     return false;
   const auto &Edit = arg.additionalTextEdits[0];
TEST(CompletionTest, IncludeInsertionPreprocessorIntegrationTests) {
   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.items,
-              ElementsAre(AllOf(Named("X"), InsertInclude("\"bar.h\""))));
+              ElementsAre(AllOf(
+                  Named("X"),
+                  Labeled(CodeCompleteOptions().IncludeIndicator.Insert + "X"),
+                  InsertInclude("\"bar.h\""))));
   // Duplicate based on inclusions in preamble.
   Results = completions(Server,
                         R"cpp(
           #include "sub/bar.h"  // not shortest, so should only match resolved.
           int main() { ns::^ }
       )cpp",
                         {Sym});
-  EXPECT_THAT(Results.items,
-              ElementsAre(AllOf(Named("X"), Not(HasAdditionalEdits()))));
+  EXPECT_THAT(Results.items, ElementsAre(AllOf(Named("X"), Labeled("X"),
+                                               Not(HasAdditionalEdits()))));
 }
 
 TEST(CompletionTest, NoIncludeInsertionWhenDeclFoundInFile) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
 
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
MATCHER_P(ParamsAre, P, "") {
   for (unsigned I = 0; I < P.size(); ++I)
     if (P[I] != arg.parameters[I].label)
       return false;
   return true;
 }
 
 Matcher<SignatureInformation> Sig(std::string Label,
                                   std::vector<std::string> Params) {
-  return AllOf(Labeled(Label), ParamsAre(Params));
+  return AllOf(SigHelpLabeled(Label), ParamsAre(Params));
 }
 
 TEST(SignatureHelpTest, Overloads) {
   auto Results = signatures(R"cpp(
     void foo(int x, int y);
     void foo(int x, float y);
     void foo(float x, int y);
     void foo(float x, float y);
TEST(CompletionTest, OverloadBundling) {
   // 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.Detail = &Detail;
   EXPECT_THAT(
       completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).items,
-      UnorderedElementsAre(AllOf(Named("GFuncC"), InsertInclude("<foo>")),
-                           Labeled("GFuncC(int)"), Labeled("GFuncD(int)")));
+      UnorderedElementsAre(
+          AllOf(
+              Named("GFuncC"),
+              Labeled(CodeCompleteOptions().IncludeIndicator.Insert + "GFuncC"),
+              InsertInclude("<foo>")),
 
   // Examine a bundled completion in detail.
   auto A = completions(Context + "int y = X().a^", {}, Opts).items.front();
-  EXPECT_EQ(A.label, "a(…)");
+  EXPECT_EQ(A.label, " a(…)");
   EXPECT_EQ(A.detail, "[2 overloads]");
   EXPECT_EQ(A.insertText, "a");
   EXPECT_EQ(A.kind, CompletionItemKind::Method);
   // For now we just return one of the doc strings arbitrarily.
   EXPECT_THAT(A.documentation, AnyOf(HasSubstr("Overload with int"),
                                      HasSubstr("Overload with bool")));
   Opts.EnableSnippets = true;
   A = completions(Context + "int y = X().a^", {}, Opts).items.front();

unittests/clangd/HeadersTests.cpp

std::vector<Inclusion> collectIncludes() {
     Clang->getPreprocessor().addPPCallbacks(collectInclusionsInMainFileCallback(
         Clang->getSourceManager(),
         [&](Inclusion Inc) { Inclusions.push_back(std::move(Inc)); }));
     EXPECT_TRUE(Action.Execute());
     Action.EndSourceFile();
     return Inclusions;
   }
 
-  // Calculates the include path, or returns "" on error.
+  // Calculates the include path, or returns "" on error or header should not be
+  // inserted.
   std::string calculate(PathRef Original, PathRef Preferred = "",
-                        const std::vector<Inclusion> &Inclusions = {},
-                        bool ExpectError = false) {
+                        const std::vector<Inclusion> &Inclusions = {}) {
     auto Clang = setupClang();
     PreprocessOnlyAction Action;
     EXPECT_TRUE(
         Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
 
     if (Preferred.empty())
       Preferred = Original;
     auto ToHeaderFile = [](llvm::StringRef Header) {
       return HeaderFile{Header,
                         /*Verbatim=*/!llvm::sys::path::is_absolute(Header)};
     };
 
-    auto Path = calculateIncludePath(
-        MainFile, CDB.getCompileCommand(MainFile)->Directory,
-        Clang->getPreprocessor().getHeaderSearchInfo(), Inclusions,
-        ToHeaderFile(Original), ToHeaderFile(Preferred));
+    IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
+                             CDB.getCompileCommand(MainFile)->Directory,
+                             Clang->getPreprocessor().getHeaderSearchInfo());
+    for (const auto &Inc : Inclusions)
+      Inserter.addExisting(Inc);
+    auto Declaring = ToHeaderFile(Original);
+    auto Inserted = ToHeaderFile(Preferred);
+    if (!Inserter.shouldInsertInclude(Declaring, Inserted))
+      return "";
+    std::string Path = Inserter.calculateIncludePath(Declaring, Inserted);
     Action.EndSourceFile();
-    if (!Path) {
-      llvm::consumeError(Path.takeError());
-      EXPECT_TRUE(ExpectError);
-      return std::string();
-    } else {
-      EXPECT_FALSE(ExpectError);
-    }
-    return std::move(*Path);
+    return Path;
   }
 
-  Expected<Optional<TextEdit>>
-  insert(const HeaderFile &DeclaringHeader, const HeaderFile &InsertedHeader,
-         const std::vector<Inclusion> &ExistingInclusions = {}) {
+  Optional<TextEdit> insert(StringRef VerbatimHeader) {
     auto Clang = setupClang();
     PreprocessOnlyAction Action;
     EXPECT_TRUE(
         Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
 
     IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
                              CDB.getCompileCommand(MainFile)->Directory,
                              Clang->getPreprocessor().getHeaderSearchInfo());
-    for (const auto &Inc : ExistingInclusions)
-      Inserter.addExisting(Inc);
-
-    auto Edit = Inserter.insert(DeclaringHeader, InsertedHeader);
+    auto Edit = Inserter.insert(VerbatimHeader);
     Action.EndSourceFile();
     return Edit;
   }
 
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   std::string MainFile = testPath("main.cpp");
   std::string Subdir = testPath("sub");
TEST_F(HeadersTest, DontInsertDuplicateResolved) {
   std::string BarHeader = testPath("sub/bar.h");
   std::vector<Inclusion> Inclusions = {
       {Range(), /*Written*/ "fake-bar.h", /*Resolved*/ BarHeader}};
   EXPECT_EQ(calculate(BarHeader, "", Inclusions), "");
   // Do not insert preferred.
   EXPECT_EQ(calculate(BarHeader, "\"BAR.h\"", Inclusions), "");
 }
 
-HeaderFile literal(StringRef Include) {
-  HeaderFile H{Include, /*Verbatim=*/true};
-  assert(H.valid());
-  return H;
-}
-
 TEST_F(HeadersTest, PreferInserted) {
-  auto Edit = insert(literal("<x>"), literal("<y>"));
-  EXPECT_TRUE(static_cast<bool>(Edit));
-  EXPECT_TRUE(llvm::StringRef((*Edit)->newText).contains("<y>"));
-}
-
-TEST_F(HeadersTest, ExistingInclusion) {
-  Inclusion Existing{Range(), /*Written=*/"<c.h>", /*Resolved=*/""};
-  auto Edit = insert(literal("<c.h>"), literal("<c.h>"), {Existing});
-  EXPECT_TRUE(static_cast<bool>(Edit));
-  EXPECT_FALSE(static_cast<bool>(*Edit));
-}
-
-TEST_F(HeadersTest, ValidateHeaders) {
-  HeaderFile InvalidHeader{"a.h", /*Verbatim=*/true};
-  assert(!InvalidHeader.valid());
-  auto Edit = insert(InvalidHeader, literal("\"c.h\""));
-  EXPECT_FALSE(static_cast<bool>(Edit));
-  llvm::consumeError(Edit.takeError());
+  auto Edit = insert("<y>");
+  EXPECT_TRUE(Edit.hasValue());
+  EXPECT_TRUE(llvm::StringRef(Edit->newText).contains("<y>"));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang