This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add support for snippet completions
ClosedPublic

Authored by rwols on Aug 24 2017, 3:03 AM.

Details

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

Event Timeline

rwols created this revision.Aug 24 2017, 3:03 AM
ilya-biryukov requested changes to this revision.Aug 24 2017, 4:09 AM

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?

This revision now requires changes to proceed.Aug 24 2017, 4:09 AM
rwols added a comment.Aug 24 2017, 6:24 AM

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"?

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 updated this revision to Diff 112609.Aug 24 2017, 1:57 PM
rwols edited edge metadata.

[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 marked 10 inline comments as done.Aug 24 2017, 2:07 PM
rwols added inline comments.
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 added a comment.Aug 27 2017, 4:55 AM

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.

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 added inline comments.Aug 28 2017, 1:55 AM
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*?

fhahn added a subscriber: fhahn.Aug 29 2017, 3:53 AM
rwols updated this revision to Diff 113219.Aug 30 2017, 3:04 AM

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 marked 10 inline comments as done.Aug 30 2017, 3:07 AM

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 added a comment.Aug 30 2017, 3:33 AM

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 added inline comments.Aug 30 2017, 5:20 AM
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 updated this revision to Diff 113258.Aug 30 2017, 6:49 AM

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

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

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

ilya-biryukov added inline comments.Sep 1 2017, 2:41 AM
clangd/ClangdUnit.cpp
417

Typo: should be Item instead of item.

Otherwise this does not compile with assertions enabled.

ilya-biryukov added inline comments.Sep 1 2017, 4:55 AM
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 added inline comments.Sep 1 2017, 9:24 AM
clangd/ClangdUnit.cpp
336

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

rwols updated this revision to Diff 113650.Sep 2 2017, 2:16 AM
rwols marked 3 inline comments as done.
  • 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 added a comment.Sep 2 2017, 2:21 AM

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

rwols marked 4 inline comments as done.Sep 2 2017, 2:51 AM
rwols added inline comments.
clangd/ClangdUnit.cpp
418

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

rwols updated this revision to Diff 113657.Sep 2 2017, 1:41 PM
  • 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.
rwols retitled this revision from [clangd] [WIP] Add support for snippet completions to [clangd] Add support for snippet completions.Sep 3 2017, 3:44 AM
rwols edited the summary of this revision. (Show Details)
rwols added a project: Restricted Project.

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 updated this revision to Diff 113899.Sep 5 2017, 12:49 PM
  • 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 updated this revision to Diff 113901.Sep 5 2017, 12:50 PM
  • Fix failing clangd tests
rwols marked 7 inline comments as done.Sep 5 2017, 12:54 PM
rwols added inline comments.
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.

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 added inline comments.Sep 6 2017, 8:02 AM
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 marked an inline comment as done.Sep 6 2017, 8:03 AM
rwols added inline comments.
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 updated this revision to Diff 114242.Sep 7 2017, 12:50 PM

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 accepted this revision.Sep 8 2017, 2:12 AM

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.

This revision is now accepted and ready to land.Sep 8 2017, 2:12 AM
krasimir accepted this revision.Sep 8 2017, 6:38 AM

Great!

rwols updated this revision to Diff 114415.Sep 8 2017, 12:32 PM

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

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

rwols added a comment.EditedSep 11 2017, 10:52 AM

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

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 closed this revision.Sep 12 2017, 7:08 AM

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

rwols added a comment.Sep 12 2017, 9:45 AM

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.

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.