[clangd] Add support for snippet completions

[rwols]

Summary

Enhances CompletionItemsCollector in such a way that snippet completions can be presented to the client. Enable snippet completion items by specifying -enable-snippets while invoking the clangd executable.

• VSCode Toy Client: needs changes in order to account for snippets.
• Sublime Text 3 LSP plugin: works out of the box.
• Atom: ???
• Vim: ???
• Emacs: ???

See: https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#completion-request
See: https://github.com/Microsoft/vscode/blob/master/src/vs/editor/contrib/snippet/browser/snippet.md

[ilya-biryukov]

Thanks for taking your time to implement this!

We need to add some test cases for new completion features. However, adding them may be a bit of a pain, so feel free to ask for best practices if you'll run into trouble while doing that.
Please also see my inline comments.

clangd/ClangdUnit.cpp

304

Is this required for snippets?
I suggest we move the removal of sortText into a separate commit/discussion.

306

Code style: use UpperCamelCase for parameter names

308–309

Could you please also move this to a separate review?
I think we should show inaccessible items, but they should have a much lower priority than accessible ones. Others may disagree, but let's have this discussion in a separate thread.

308–309

Code style: we usually don't use explicit this-> qualifiers.

319

Maybe return CompletionItem instead of modifying a reference parameter?

319

Any reason why Context is passed by value? Maybe pass by const &?

320

This function does not modify Result. Maybe pass by const & instead of &?

324

Maybe split writes into Item.label and other Item fields?
That would simplify the function, i.e. something like this:

// Set Item.label approriately.
switch (Result.Kind) {
  case RK_Declaration: Item.label = Result.Declaration->getNameAsString();
  //... other enum value
}


// Set other Item fields.
// Note that is also works as expected for RK_Pattern.
auto Completion = Result.CreateCodeCompletionString(/*....*/);
ProcessCodeCompleteString(/*...*/, Item);
// ....

365

Maybe only set it to Snippet if we encountered CK_Placeholder?
That would allow clients of ClangdServer API to ignore snippet items if they don't support it. (We have one client like that internally)

372

Maybe add some description to the docs? For example.

Attributes: [[noreturn]], __attibute__((nonnull(1,3))

// Here goes documentation

384

We set the label in ProcessCodeCompleteResult, why is there a need to append to it here?
Maybe we should set it here and not touch it in ProcessCodeCompleteResult at all?

390

Again, do we need to set label in ProcessCodeCompleteResult or in ProcessCodeCompleteString?

403

What if Chunk.Text contains }?

409

Let's also add some description to Item.documentation. What kind of things go into CK_Informative ?

416

How is CK_CurrentParameter different from CK_Placeholder? Maybe handle them both simultaneously and add a comment on why we don't care about their differences?
I mean something like:

// We don't distinguish between CK_Placeholder and CK_CurrentParameter because ....
case CodeCompletionString::CK_Placeholder:
case CodeCompletionString::CK_CurrentParameter:
//... Code adding ${N:something}

455

Does adding vertical space to label plays well with editors? Maybe we should replace with horizontal space instead?

[rwols]

Thanks for the quick review! I'm new to Phabricator and the arc CLI tool. Is the workflow like this: "address a comment, change a few lines, do arc diff, do this multiple times", or is it like this: "address all the comments, change lots of lines, do arc diff, do this once"?

[ilya-biryukov]

In D37101#851380, @rwols wrote:

Thanks for the quick review! I'm new to Phabricator and the arc CLI tool. Is the workflow like this: "address a comment, change a few lines, do arc diff, do this multiple times", or is it like this: "address all the comments, change lots of lines, do arc diff, do this once"?

Both workflows work, but I would suggest to address a batch of comments before doing arc diff, as it may become hard to follow inline comments after multiple arc diffs.
You can also send multiple commits in a single arc diff if you use git-svn. I highly suggest looking into that workflow if you're familiar with git. There's also an arc patch that you can use to apply a patch from this review after doing a new git-svn checkout.

[rwols]

[clangd] [WIP] Add support for snippet completions

• Restore the sortText logic
• Return CompletionItem instead of modifying a ref param
• Make all helper methods const
• Only set to Snippet once we encounter CK_Placeholder/CK_CurrentParameter
• More clear annotation handling
• Escape any $, } and \ for a snippet

[rwols]

clangd/ClangdUnit.cpp

324

From some experimentation and skimming over SemaCodeComplete.cpp I notice the result of CreateCodeCompletionString(/*....*/) is never null anyway, so one can just skip those switch cases. I'm not sure why a pointer is returned.

384

It's possible that maybe too much is going into the label now, I agree. For instance for a function the name of the function as well as the opening parenthesis, all of its parameters, and the closing brace go into the label. Maybe it's enough to only have the name of the function in the label. On the other hand, I don't think this will play nicely with VSCode and SublimeText, but this will ultimately depend on the language client plugins.

409

I've seen " const", " volatile" go into CK_Informative (note the prefix space). Another major chunk that goes into CK_Informative is the base class name suffixed with :: if the completion is a method of the base class not overridden in the current class.

416

I'm still figuring out exactly what the difference is...

455

I'll fix this later.

[rwols]

After digging into VSCode, I now think that presenting snippets is the wrong way forward, and I believe the right way is to implement the signatureHelpProvider protocol for method/function parameters. See: https://github.com/Microsoft/vscode-docs/blob/master/docs/extensionAPI/language-support.md#help-with-function-and-method-signatures.

I'll update this diff accordingly.

[ilya-biryukov]

In D37101#853509, @rwols wrote:

After digging into VSCode, I now think that presenting snippets is the wrong way forward, and I believe the right way is to implement the signatureHelpProvider protocol for method/function parameters. See: https://github.com/Microsoft/vscode-docs/blob/master/docs/extensionAPI/language-support.md#help-with-function-and-method-signatures.

I'll update this diff accordingly.

I still think having support for snippets is useful in completion. Let's not have snippets for functions and only present them for RK_Patterns.

Signature help is useful, but it's a separate feature and should also go in as a separate commit.
For example, consider the differences between completion and signature help.

vector<int> vec;
vec.push_back(/*cursor*/

In that case,

• signature help must show all overloads of push_back, show documentation for the first parameter, if any;
• code completion must show global completions.

[ilya-biryukov]

clangd/ClangdUnit.cpp

324

Probably just a way to indicate that memory for returned item is managed by Allocator.

327

Probably meant to be InsertTextFormat::Snippet, not InsertTextFormat::PlainText.

390

Style guide: use FIXME instead of TODO

404

Style guide: use FIXME instead of TODO

404

Maybe reference function name instead of the line number? Line numbers change too often.

406

After looking at code completion code, it seems that informative chunks are parts of function signature (that have custom highlightings in XCode, probably).
Maybe we could put them into label? It feels that const belongs to the parameter list and we put parameter list into label now.
That would also make the Chunk.Text + 1 part unnecessary.

463

Maybe use StringRef instead of const char*?

472

Maybe use StringRef instead of const char*?

[rwols]

Tidy up snippet handling

• Put CK_Informative chunks in the label
• Assert that there's at most one CK_ResultType chunk
• CK_CurrentParameter never occurs while collecting completions, only while handling overloads.
• For CK_VerticalSpace, use a space for the label (but a newline for the insertText).
• Move the SnippetEscape function outside the class and into an unnamed namespace.

[rwols]

I followed your advice and kept the snippet functionality. We'll do the SignatureHelp stuff in another review.

A "major" change is that, because CK_Informative chunks are put into the label now, we have to use the insertText, always.

[rwols]

So at this point the C++ changes are basically done, save for some minor things I guess. The problem is still that the VSCode extension doesn't do anything clever with the snippets. I have zero experience with TypeScript let alone extension development for VSCode, so it can take a while for me to investigate. I'd appreciate if someone else could take a look.

[krasimir]

clangd/ClangdUnit.cpp

302

Move this assert in the CompletionItemsCollector constructor.

322

This comment is unnecessary.

339

In LLVM, prefer using an explicit variable for the limit of iteration. In this case, since you're using getAnnotationCount before, you could directly put it into a variable above.

[rwols]

Some more tweaks

• Move assert to constructor of CompletionItemsCollector
• Use a local variable for the annotations count
• Move the documentation handling to its own private const member function

[ilya-biryukov]

This looks like a useful change even without prior changes to VSCode.

Maybe add a command-line flag to clangd(--enable-snippets) and commit that?
When snippets are disabled, we could simply do insertText = /*<item name>*/ after ProcessChunks, we will deprecate and remove that flag eventually when all the clients support it.

clangd/ClangdUnit.cpp

336

Could we also set Item.filterText to completion item name?
So that various pieces of function signature would not match on user input.

418

Won't that assertion fail with function return types? Let's add a test for that.

int (*foo(int a))(float);

[ilya-biryukov]

Also wanted to stress once again that we need some tests for this change.

[ilya-biryukov]

clangd/ClangdUnit.cpp

417

Typo: should be Item instead of item.

Otherwise this does not compile with assertions enabled.

[ilya-biryukov]

clangd/ClangdUnit.cpp

336

After some investigation: CK_TypedText chunks are exactly the things that should go into filterText.
Also, VSCode seems to ignore filterText right now, but that's certainly a bug in VSCode, I still propose to set filterText properly.

[ilya-biryukov]

clangd/ClangdUnit.cpp

336

Sorry for confusion, after updating VSCode to the last version, filterText works just fine.

[rwols]

• Split up the CompletionItemsCollector into two classes called PlainTextCompletionItemsCollector and SnippetCompletionItemsCollector to allow collecting both types of items.
• Add a command-line setting to clangd called "--enable-snippets" that, when set, will make clangd use the SnippetCompletionItemsCollector. The default is to use the PlainTextCompletionItemsCollector.
• Enable "code pattern" completions when "--enable-snippets" is true.
• Do not add a space in the completion label when we encounter a CK_VerticalSpace in the SnippetCompletionItemsCollector. A space looks weird.

I will add tests now.

[rwols]

Forgot to mention I've also made sure the filterText field is taken care of now (both for snippet and for plaintext items).

[rwols]

clangd/ClangdUnit.cpp

418

I'll make sure to add a test case for this!

[rwols]

• Adjust SnippetCompletionItemCollector such that it only sets insertTextFormat to InsertTextFormat::Snippet when it's actually needed (i.e. we encounter a CK_Placeholder chunk).
• Fix failing tests in test/clangd/{completion.test,authority-less-uri.test} because of the new changes.
• Add new tests in test/clangd/completion-snippet.test that tests the snippet functionality.

[ilya-biryukov]

Looks good overall, just a bunch of comments before accepting the revision. Mostly minor code style issues, sorry for spamming you with those.

clangd/ClangdLSPServer.h

30 ↗

(On Diff #113657)

Code style: we don't use const for non-reference parameters.

clangd/ClangdUnit.cpp

275

Code style: function names are lowerCamelCase.
Also maybe rename to escapeSnippet to follow the style guide: "function names should be verb phrases, e.g. openFile() or isFoo()" (see https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)

319

Code style: we put all field definitions at the bottom of the class in clangd.
Also: why not make the fields private?

380

Implementations of this function in PlainTextCompletionItemsCollector and SnippetCompletionItemsCollector are the same.
Maybe make ProcessChunks virtual instead?

Or maybe consider replacing inheritance with a bool flag. Inheritance does not seem to buy us much here. This looks simpler:

if (EnableSnippets)
  ProcessChunksWithSnippets(CCS, Item);
else
  ProcessChunksWithoutSnippets(CCS, Item);

458

Maybe add a comment that fallthrough to the next case is intended?
It's very easy to miss that there's no break intentionally when reading the code.

537

Maybe add break here?
To avoid unintentional fallthrough if new case statement gets added.

clangd/Protocol.h

393 ↗

(On Diff #113657)

Maybe remove these constructors?
It seems weird that insertTextFormat is initialized on construction, while all other fields are filled up later.
Having to write all the fields explicitly is pretty low-level, but it's consistent and is done in all other structs from Protocol.h.

clangd/tool/ClangdMain.cpp

31 ↗

(On Diff #113657)

I've managed to get snippet completion working in VSCode.
The problem is that we're using outdated version of vscode-languageclient dependency in our npm module.
Simply bumping versions of dependencies in clangd\clients\clangd-vscode\package.json worked.
Had to change

"dependencies": {
    "vscode-languageclient": "^2.6.3",
    "vscode-languageserver": "^2.6.2"
},

to

"dependencies": {
    "vscode-languageclient": "^3.3.0",
    "vscode-languageserver": "^3.3.0"
},

Maybe include this in this commit and make -enable-snippets true by default?

[rwols]

• Don't use const for non-reference parameters
• lowerCamelCase for functions, verb phrase
• Field definitions at the bottom of the class
• Make ProcessChunks virtual
• Add comment that fallthrough to the next case is intended
• Revert new constructors for CompletionItem, they are pointless
• Bump minimum version for vscode-languageclient and vscode-languageserver to 3.3.0

[rwols]

• Fix failing clangd tests

[rwols]

clangd/ClangdUnit.cpp

380

I went with the "make ProcessChunks virtual" approach, wouldn't your suggestion have an impact on performance? There'd be a check for every completion item now.

[ilya-biryukov]

Looks good, just one major drawback to address. We shouldn't break clients by default (see comment in ClangdMain.cpp).
And a minor comment about flag naming.

clangd/tool/ClangdMain.cpp

30 ↗

(On Diff #113901)

Maybe replace a negated flag with positive alternative, e.g. enable-snippets or use-snippets?
Reading such "implicit" double negations is sometimes confusing (e.g. !DisableSnippets instead of EnableSnippets).

33 ↗

(On Diff #113901)

After putting some thought into it, let's disable snippets by default. Sorry for driving this in a wrong direction in my previous comments.
Instead of using flag, LSP servers should look at initial initialize request and a value of TextDocumentClientCapabilities.completion.completionItem.snippetSupport.
(see https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#initialize-request).
And it seems bad from clangd side to break clients that don't support snippets by default. It's probably ok if user passed custom command-line arguments, overriding the defaults.

The problem is that we don't parse the ClientCapabilities now. It's not hard to add parsing of it, but that should probably be done in a separate commit and I can do this work if you want.
I think it's fine to commit this as is, defaulting to not return snippets, and add parsing of ClientCapabilities with a follow-up commit.

[ilya-biryukov]

clangd/ClangdUnit.cpp

380

Oh, that should not make any difference performance-wise. There is many more branching on each completion item even if we assume all the function calls are inlined.
And virtual calls are not free either.

[rwols]

clangd/tool/ClangdMain.cpp

33 ↗

(On Diff #113901)

Yes, I agree snippets should be enabled/disabled by reading the client's capabilities. This command line flag should be a temporary solution.

[rwols]

Change command-line option back to "-enable-snippets", disable snippets by default. This is a temporary solution and we should instead inspect the "initialize" request from the client to check wether the client supports snippets.

[ilya-biryukov]

Thanks for the change! Looks good minus an outdated description of enable-snippets flag.
Do you have commit access to llvm repository?

clangd/tool/ClangdMain.cpp

31 ↗

(On Diff #114242)

Description still mentions disable-snippets.

[krasimir]

Great!

[rwols]

Update the description for the "-enable-snippets" option.

[ilya-biryukov]

@rwols, do you have access to svn repo? If not, I can submit this change for you.

[rwols]

No, I don't have access to the repo, sorry forgot to mention this. Go ahead and merge.

[ilya-biryukov]

Done. It's now in. Thanks for the contribution!

Had to make a few changes before committing.

• Fix compilation of tests.
• Update VSCode "engine" dependency in vscode toy client.

[ilya-biryukov]

Marking as closed, had to commit by hand without arc patch as it couldn't find base revision to apply the patch on.

[rwols]

Thanks! Thinking ahead now so we're on the same page, you will be implementing the client's initialize request, and I'll start work on textDocument/signatureHelp.

[ilya-biryukov]

In D37101#868207, @rwols wrote:

Thanks! Thinking ahead now so we're on the same page, you will be implementing the client's initialize request, and I'll start work on textDocument/signatureHelp.

Sounds good.

clangd/ClangdUnit.cpp

CompletionItemKind getKind(CXCursorKind K) {
   case CXCursor_MemberRef:
   case CXCursor_TypeRef:
     return CompletionItemKind::Reference;
   default:
     return CompletionItemKind::Missing;
   }
 }
 
+std::string SnippetEscape(const llvm::StringRef Text) {
+  std::string Result;
+  Result.reserve(Text.size()); // Assume '$', '}' and '\\' are rare.
+  for (const auto Character : Text) {
+    if (Character == '$' || Character == '}' || Character == '\\')
+      Result.push_back('\\');
+    Result.push_back(Character);
+  }
+  return Result;
+}
+
 class CompletionItemsCollector : public CodeCompleteConsumer {
   std::vector<CompletionItem> *Items;
   std::shared_ptr<clang::GlobalCodeCompletionAllocator> Allocator;
   CodeCompletionTUInfo CCTUInfo;
 
 public:
   CompletionItemsCollector(std::vector<CompletionItem> *Items,
                            const CodeCompleteOptions &CodeCompleteOpts)
       : CodeCompleteConsumer(CodeCompleteOpts, /*OutputIsBinary=*/false),
         Items(Items),
         Allocator(std::make_shared<clang::GlobalCodeCompletionAllocator>()),
         CCTUInfo(Allocator) {}
 
   void ProcessCodeCompleteResults(Sema &S, CodeCompletionContext Context,
                                   CodeCompletionResult *Results,
                                   unsigned NumResults) override {
-    for (unsigned I = 0; I != NumResults; ++I) {
-      CodeCompletionResult &Result = Results[I];
-      CodeCompletionString *CCS = Result.CreateCodeCompletionString(
+    assert(Items && "We need a non-null items container here");
+    Items->reserve(NumResults);
+    for (unsigned I = 0; I < NumResults; ++I) {
+      auto &Result = Results[I];
+      const auto *CCS = Result.CreateCodeCompletionString(
           S, Context, *Allocator, CCTUInfo,
           CodeCompleteOpts.IncludeBriefComments);
-      if (CCS) {
-        CompletionItem Item;
-        for (CodeCompletionString::Chunk C : *CCS) {
-          switch (C.Kind) {
-          case CodeCompletionString::CK_ResultType:
-            Item.detail = C.Text;
-            break;
-          case CodeCompletionString::CK_Optional:
-            break;
-          default:
-            Item.label += C.Text;
-            break;
-          }
-        }
-        assert(CCS->getTypedText());
-        Item.kind = getKind(Result.CursorKind);
-        // Priority is a 16-bit integer, hence at most 5 digits.
-        assert(CCS->getPriority() < 99999 && "Expecting code completion result "
-                                             "priority to have at most "
-                                             "5-digits");
-        llvm::raw_string_ostream(Item.sortText)
-            << llvm::format("%05d%s", CCS->getPriority(), CCS->getTypedText());
-        Item.insertText = Item.filterText = CCS->getTypedText();
-        if (CCS->getBriefComment())
-          Item.documentation = CCS->getBriefComment();
-        Items->push_back(std::move(Item));
+      assert(CCS && "Expected the CodeCompletionString to be non-null");
+      Items->push_back(ProcessCodeCompleteResult(Result, *CCS));
+    }
+  }
+
+  GlobalCodeCompletionAllocator &getAllocator() override { return *Allocator; }
+
+  CodeCompletionTUInfo &getCodeCompletionTUInfo() override { return CCTUInfo; }
+
+private:
+  CompletionItem
+  ProcessCodeCompleteResult(const CodeCompletionResult &Result,
+                            const CodeCompletionString &CCS) const {
+    // The object that we'll return.
+
+    // Always a snippet. This is because CK_Informative chunks should not be
+    // inserted into the text buffer, but they are part of the label. For
+    // example, "foo::bar() const" is the label, but "bar()" is the insertText.
+    Item.insertTextFormat = InsertTextFormat::Snippet;
+
+    // Things like __attribute__((nonnull(1,3))) and [[noreturn]]. Present this
+    // information in the documentation field.
+    if (CCS.getAnnotationCount() > 0) {
+      Item.documentation += "Annotation";
+      if (CCS.getAnnotationCount() == 1) {
+        Item.documentation += ": ";
+      } else /* CCS.getAnnotationCount() > 1 */ {
+        Item.documentation += "s: ";
+      }
+      for (unsigned j = 0; j < CCS.getAnnotationCount(); ++j) {
+        Item.documentation += CCS.getAnnotation(j);
+        Item.documentation += j == CCS.getAnnotationCount() - 1 ? "\n\n" : " ";
+      }
+
+    // Add brief documentation (if there is any).
+    if (CCS.getBriefComment() != nullptr) {
+      Item.documentation += CCS.getBriefComment();
+
+    // Fill in the label, detail and insertText fields of the CompletionItem.
+    ProcessChunks(CCS, Item);
+
+    // Fill in the kind field of the CompletionItem.
+
+    // Fill in the sortText of the CompletionItem.
+    assert(CCS.getPriority() < 99999 && "Expecting code completion result "
+                                        "priority to have at most 5-digits");
+        << llvm::format("%05d%s", CCS.getPriority(), CCS.getTypedText());
+
+    return Item;
+  }
+
+  void ProcessChunks(const CodeCompletionString &CCS,
+                     CompletionItem &Item) const {
+    unsigned ArgCount = 0;
+    for (const auto &Chunk : CCS) {
+      switch (Chunk.Kind) {
+      case CodeCompletionString::CK_TypedText:
+        // The piece of text that the user is expected to type to match
+        // the code-completion string, typically a keyword or the name of
+        // a declarator or macro.
+      case CodeCompletionString::CK_Text:
+        // A piece of text that should be placed in the buffer,
+        // e.g., parentheses or a comma in a function call.
+        Item.label += Chunk.Text;
+        Item.insertText += Chunk.Text;
+        break;
+      case CodeCompletionString::CK_Optional:
+        // A code completion string that is entirely optional.
+        // For example, an optional code completion string that
+        // describes the default arguments in a function call.
+
+        // FIXME: Maybe add an option to allow presenting the optional chunks?
+        break;
+      case CodeCompletionString::CK_Placeholder:
+        // A string that acts as a placeholder for, e.g., a function call
+        // argument.
+        ++ArgCount;
+        Item.insertText += "${" + std::to_string(ArgCount) + ':' +
+                           SnippetEscape(Chunk.Text) + '}';
+        Item.label += Chunk.Text;
+        break;
+      case CodeCompletionString::CK_Informative:
+        // A piece of text that describes something about the result
+        // but should not be inserted into the buffer.
+        // For example, the word "const" for a const method, or the name of the
+        // base class for methods that are part of the base class.
+        Item.label += Chunk.Text;
+        // Don't put the informative chunks in the insertText.
+        break;
+      case CodeCompletionString::CK_ResultType:
+        // A piece of text that describes the type of an entity or,
+        // for functions and methods, the return type.
+        assert(item.detail.empty() && "Unexpected extraneous CK_ResultType");
+        Item.detail = Chunk.Text;
+        break;
+      case CodeCompletionString::CK_CurrentParameter:
+        // A piece of text that describes the parameter that corresponds to the
+        // code-completion location within a function call, message send, macro
+        // invocation, etc.
+        //
+        // This should never be present while collecting completion items, only
+        // while collecting overload candidates.
+        llvm_unreachable(
+            "Unexpected CK_CurrentParameter while collecting CompletionItems");
+      case CodeCompletionString::CK_LeftParen:
+        // A left parenthesis ('(').
+      case CodeCompletionString::CK_RightParen:
+        // A right parenthesis (')').
+      case CodeCompletionString::CK_LeftBracket:
+        // A left bracket ('[').
+      case CodeCompletionString::CK_RightBracket:
+        // A right bracket (']').
+      case CodeCompletionString::CK_LeftBrace:
+        // A left brace ('{').
+      case CodeCompletionString::CK_RightBrace:
+        // A right brace ('}').
+      case CodeCompletionString::CK_LeftAngle:
+        // A left angle bracket ('<').
+      case CodeCompletionString::CK_RightAngle:
+        // A right angle bracket ('>').
+      case CodeCompletionString::CK_Comma:
+        // A comma separator (',').
+      case CodeCompletionString::CK_Colon:
+        // A colon (':').
+      case CodeCompletionString::CK_SemiColon:
+        // A semicolon (';').
+      case CodeCompletionString::CK_Equal:
+        // An '=' sign.
+      case CodeCompletionString::CK_HorizontalSpace:
+        // Horizontal whitespace (' ').
+        Item.insertText += Chunk.Text;
+        Item.label += Chunk.Text;
+        break;
+      case CodeCompletionString::CK_VerticalSpace:
+        // Vertical whitespace ('\n' or '\r\n', depending on the
+        // platform).
+        Item.insertText += Chunk.Text;
+        Item.label.push_back(' '); // Just a space for the label.
       }
     }
   }
 
-  GlobalCodeCompletionAllocator &getAllocator() override { return *Allocator; }
-
-  CodeCompletionTUInfo &getCodeCompletionTUInfo() override { return CCTUInfo; }
-};
+}; // CompletionItemsCollector
 } // namespace
 
 std::vector<CompletionItem>
 clangd::codeComplete(PathRef FileName, tooling::CompileCommand Command,
                      PrecompiledPreamble const *Preamble, StringRef Contents,
                      Position Pos, IntrusiveRefCntPtr<vfs::FileSystem> VFS,
                      std::shared_ptr<PCHContainerOperations> PCHs) {
   std::vector<const char *> ArgStrs;
   for (const auto &S : Command.CommandLine)
     ArgStrs.push_back(S.c_str());
 
   VFS->setCurrentWorkingDirectory(Command.Directory);
 
   std::unique_ptr<CompilerInvocation> CI;
   EmptyDiagsConsumer DummyDiagsConsumer;
   {
     IntrusiveRefCntPtr<DiagnosticsEngine> CommandLineDiagsEngine =
         CompilerInstance::createDiagnostics(new DiagnosticOptions,
                                             &DummyDiagsConsumer, false);
     CI = createCompilerInvocation(ArgStrs, CommandLineDiagsEngine, VFS);
   }
   assert(CI && "Couldn't create CompilerInvocation");
 
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =

 }
 
 llvm::Optional<ParsedAST>
 ParsedAST::Build(std::unique_ptr<clang::CompilerInvocation> CI,
                  const PrecompiledPreamble *Preamble,
                  ArrayRef<serialization::DeclID> PreambleDeclIDs,
                  std::unique_ptr<llvm::MemoryBuffer> Buffer,
                  std::shared_ptr<PCHContainerOperations> PCHs,
                  IntrusiveRefCntPtr<vfs::FileSystem> VFS) {
 
   std::vector<DiagWithFixIts> ASTDiags;
   StoreDiagsConsumer UnitDiagsConsumer(/*ref*/ ASTDiags);
 
   auto Clang =
       prepareCompilerInstance(std::move(CI), Preamble, std::move(Buffer), PCHs,
                               VFS, /*ref*/ UnitDiagsConsumer);
 

clangd/Protocol.cpp

Os << R"("documentation":")" << llvm::yaml::escape(CI.documentation)
        << R"(",)";
   if (!CI.sortText.empty())
     Os << R"("sortText":")" << llvm::yaml::escape(CI.sortText) << R"(",)";
   if (!CI.filterText.empty())
     Os << R"("filterText":")" << llvm::yaml::escape(CI.filterText) << R"(",)";
   if (!CI.insertText.empty())
     Os << R"("insertText":")" << llvm::yaml::escape(CI.insertText) << R"(",)";
   if (CI.insertTextFormat != InsertTextFormat::Missing) {
-    Os << R"("insertTextFormat":")" << static_cast<int>(CI.insertTextFormat)
-       << R"(",)";
+    Os << R"("insertTextFormat":)" << static_cast<int>(CI.insertTextFormat)
+       << R"(,)";
   }
   if (CI.textEdit)
     Os << R"("textEdit":)" << TextEdit::unparse(*CI.textEdit) << ',';
   if (!CI.additionalTextEdits.empty()) {
     Os << R"("additionalTextEdits":[)";
     for (const auto &Edit : CI.additionalTextEdits)
       Os << TextEdit::unparse(Edit) << ",";
     Os.flush();