[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

278

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

278

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

187

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...)

266

nit: .hasValue() would be clearer than static_cast

278

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

75

we don't need Expected here anymore?

clangd/Headers.h

63

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?

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

187

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

[sammccall]

clangd/CodeComplete.cpp

266

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.

267

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.

278

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

284–285

again, need to account for the length of the indicator

clangd/CodeComplete.h

63 ↗

(On Diff #151492)

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

278

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

63 ↗

(On Diff #151492)

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

[ioeric]

• fix typo and clang-format

[ioeric]

• clang-format again...

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;
+  // Whether or not this candidate is in a completion where index is allowed.
+  // This can affect how items are built (e.g. indent label to align with visual
+  // indicator in index results).
+  bool AllowIndexCompletion = false;
 
   // Builds an LSP completion item.
   CompletionItem build(StringRef FileName, const CompletionItemScores &Scores,
                        const CodeCompleteOptions &Opts,
                        CodeCompletionString *SemaCCS,
                        const IncludeInserter *Includes,
                        llvm::StringRef SemaDocComment) const {
     assert(bool(SemaResult) == bool(SemaCCS));
+    assert((AllowIndexCompletion || !IndexResult));
     CompletionItem I;
     bool ShouldInsertInclude = true;
+    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);
       // Avoid inserting new #include if the declaration is found in the current
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 (ShouldInsertInclude && Includes && !D->IncludeHeader.empty()) {
-          auto Edit = [&]() -> Expected<Optional<TextEdit>> {
+          auto HeaderAndEdit =
+              [&]() -> Expected<std::pair<std::string, Optional<TextEdit>>> {
             auto ResolvedDeclaring = toHeaderFile(
                 IndexResult->CanonicalDeclaration.FileURI, FileName);
             if (!ResolvedDeclaring)
               return ResolvedDeclaring.takeError();
             auto ResolvedInserted = toHeaderFile(D->IncludeHeader, FileName);
             if (!ResolvedInserted)
               return ResolvedInserted.takeError();
             return Includes->insert(*ResolvedDeclaring, *ResolvedInserted);
           }();
-          if (!Edit) {
+          if (!HeaderAndEdit) {
             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(HeaderAndEdit.takeError()));
+          } else {
+            InsertingInclude = static_cast<bool>(HeaderAndEdit->second);
+            if (InsertingInclude) {
+              I.detail += ((I.detail.empty() ? "" : "\n") +
+                           StringRef(HeaderAndEdit->first).trim('"'))
+                              .str();
+              I.additionalTextEdits = {std::move(*(HeaderAndEdit->second))};
+            }
           }
         }
       }
     }
+    if (AllowIndexCompletion)
+      I.label = (InsertingInclude ? "+" : " ") + I.label;
     I.scoreInfo = Scores;
     I.sortText = sortText(Scores.finalScore, Name);
     I.insertTextFormat = Opts.EnableSnippets ? InsertTextFormat::Snippet
                                              : InsertTextFormat::PlainText;
     return I;
   }
 };
 using ScoredCandidate = std::pair<CompletionCandidate, CompletionItemScores>;
 
 // Determine the symbol ID for a Sema code completion result, if possible.
 llvm::Optional<SymbolID> getSymbolID(const CodeCompletionResult &R) {
   switch (R.Kind) {
   case CodeCompletionResult::RK_Declaration:
   case CodeCompletionResult::RK_Pattern: {
     llvm::SmallString<128> USR;
class CodeCompleteFlow {
   PathRef FileName;
   const CodeCompleteOptions &Opts;
   // Sema takes ownership of Recorder. Recorder is valid until Sema cleanup.
   CompletionRecorder *Recorder = nullptr;
   int NSema = 0, NIndex = 0, NBoth = 0; // Counters for logging.
   bool Incomplete = false; // Would more be available with a higher limit?
   llvm::Optional<FuzzyMatcher> Filter;       // Initialized once Sema runs.
   std::unique_ptr<IncludeInserter> Includes; // Initialized once compiler runs.
+  bool AllowIndexCompletion = false;         // Initialized once Sema runs.
 
 public:
   // A CodeCompleteFlow object is only useful for calling run() exactly once.
   CodeCompleteFlow(PathRef FileName, const CodeCompleteOptions &Opts)
       : FileName(FileName), Opts(Opts) {}
 
   CompletionList run(const SemaCompleteInput &SemaCCInput) && {
     trace::Span Tracer("CodeCompleteFlow");
public:
   }
 
 private:
   // This is called by run() once Sema code completion is done, but before the
   // Sema data structures are torn down. It does all the real work.
   CompletionList runWithSema() {
     Filter = FuzzyMatcher(
         Recorder->CCSema->getPreprocessor().getCodeCompletionFilter());
+    AllowIndexCompletion = Opts.Index && allowIndex(Recorder->CCContext);
     // 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 = AllowIndexCompletion ? 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;
Optional<float> fuzzyScore(const CompletionCandidate &C) {
     return Filter->match(C.Name);
   }
 
   // Scores a candidate and adds it to the TopN structure.
   void addCandidate(TopN<ScoredCandidate, ScoredCandidateGreater> &Candidates,
                     const CodeCompletionResult *SemaResult,
                     const Symbol *IndexResult) {
     CompletionCandidate C;
+    C.AllowIndexCompletion = AllowIndexCompletion;
     C.SemaResult = SemaResult;
     C.IndexResult = IndexResult;
     C.Name = IndexResult ? IndexResult->Name : Recorder->getName(*SemaResult);
 
     SymbolQualitySignals Quality;
     SymbolRelevanceSignals Relevance;
     Relevance.Query = SymbolRelevanceSignals::CodeComplete;
     if (auto FuzzyScore = fuzzyScore(C))

clangd/Headers.h

 /// '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:
+/// \return A quoted "path" or <path> to be included and a boolean that is true
+/// if the include can be added, and false 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(
+llvm::Expected<std::pair<std::string, bool>> 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.
+  /// Returns a <Header, Edit> pair where Header is verbatim
+  /// (e.g. <vector>) and Edit, if not None, inserts the entire include
+  /// directive. Edit is None is the header is not suitable to include e.g.
+  /// it's already included. If any header is invalid, this returns error.
   ///
   /// \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;
+  Expected<std::pair<std::string, Optional<TextEdit>>>
+  insert(const HeaderFile &DeclaringHeader,
 
 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(
+llvm::Expected<std::pair<std::string, bool>> calculateIncludePath(
     PathRef File, StringRef BuildDir, HeaderSearch &HeaderSearchInfo,
     const std::vector<Inclusion> &Inclusions, const HeaderFile &DeclaringHeader,
     const HeaderFile &InsertedHeader) {
   assert(DeclaringHeader.valid() && InsertedHeader.valid());
-  if (File == DeclaringHeader.File || File == InsertedHeader.File)
-    return "";
+  bool AddInclude =
+      File != DeclaringHeader.File && File != InsertedHeader.File;
   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 "";
+  AddInclude &=
+      !Included(DeclaringHeader.File) && !Included(InsertedHeader.File);
 
   bool IsSystem = false;
 
   if (InsertedHeader.Verbatim)
-    return InsertedHeader.File;
+    return std::make_pair(InsertedHeader.File, AddInclude);
 
   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;
+  return std::make_pair(Suggested, AddInclude);
 }
 
-Expected<Optional<TextEdit>>
+Expected<std::pair<std::string, 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);
+  StringRef IncludeRef = Include->first;
+  Optional<TextEdit> Edit = None;
+  if (Include->second)
+    if (auto Insertion = Inserter.insert(IncludeRef.trim("\"<>"),
+                                         IncludeRef.startswith("<")))
+      Edit = replacementToEdit(Code, *Insertion);
+  return std::make_pair(IncludeRef, std::move(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"}

unittests/clangd/CodeCompleteTests.cpp

TEST(CompletionTest, IncludeInsertionPreprocessorIntegrationTests) {
   Scratch.IncludeHeader = BarURI;
   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\""))));
+  EXPECT_THAT(Results.items, ElementsAre(AllOf(Named("X"), Labeled("+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());

unittests/clangd/HeadersTests.cpp

std::string calculate(PathRef Original, PathRef Preferred = "",
     Action.EndSourceFile();
     if (!Path) {
       llvm::consumeError(Path.takeError());
       EXPECT_TRUE(ExpectError);
       return std::string();
     } else {
       EXPECT_FALSE(ExpectError);
     }
-    return std::move(*Path);
+    return Path->second ? std::move(Path->first) : "";
   }
 
   Expected<Optional<TextEdit>>
   insert(const HeaderFile &DeclaringHeader, const HeaderFile &InsertedHeader,
          const std::vector<Inclusion> &ExistingInclusions = {}) {
     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 HeaderAndEdit = Inserter.insert(DeclaringHeader, InsertedHeader);
     Action.EndSourceFile();
-    return Edit;
+    if (!HeaderAndEdit)
+      return HeaderAndEdit.takeError();
+    if (!HeaderAndEdit->second)
+      return llvm::None;
+    return *(HeaderAndEdit->second);
   }
 
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   std::string MainFile = testPath("main.cpp");
   std::string Subdir = testPath("sub");
   std::string SearchDirArg = (llvm::Twine("-I") + Subdir).str();
   IgnoringDiagConsumer IgnoreDiags;