Page MenuHomePhabricator

[clangd] Add textDocument/signatureHelp
ClosedPublic

Authored by rwols on Sep 19 2017, 11:44 AM.

Details

Summary

Makes clangd respond to a client's "textDocument/signatureHelp" request by
presenting function/method overloads.
Still need to add tests.
Also, there is duplicate code in clangd::codeComplete and clangd::signatureHelp
now, maybe refactor this to a common function?

Diff Detail

Repository
rL LLVM

Event Timeline

rwols created this revision.Sep 19 2017, 11:44 AM
ilya-biryukov requested changes to this revision.Sep 20 2017, 1:53 AM

Thanks for the patch. Overall looks good, just a few comments.
Could you also add some tests?

clangd/ClangdServer.h
243 ↗(On Diff #115874)

Mentions "code completion" in the comment.

clangd/ClangdUnit.cpp
536 ↗(On Diff #115874)

Code in clangd uses FIXME instead of TODO.

589 ↗(On Diff #115874)

Ignoring CK_Optional chunks leads to weird results. They represent parameters that have default arguments. For example,

int foo(int a, int b = 10, int c = 20);

int test() {
  foo(10, 20, 30); // signatureHelp shows foo(int a) here, which is confusing.
}
691 ↗(On Diff #115874)

Most of this function is almost identical to clangd::codeComplete. They only differ in some of the CodeCompleteOpts flags and CodeCompletionConsumers they use, right?

Maybe extract a helper function that runs code completion and reuse it for both clangd::codeCompleteand clangd::signatureHelp?

This revision now requires changes to proceed.Sep 20 2017, 1:53 AM
rwols updated this revision to Diff 116461.Sep 23 2017, 7:49 AM
rwols edited edge metadata.
  • Update doxygen comment to say "signature help" instead of "code complete",
  • Refactor: put the FillDocumentation member function outside of CompletionItemsCollector so that SignatureHelpCollector can also use it.
  • Refactor: put the FillSortText member function outside of CompletionItemsCollector because it has nothing to do with CompletionItemsCollector.
  • Refactor: Make a templated function called invokeClangAction that will invoke the given CodeCompleteConsumer type. Both clangd::codeComplete and clangd::signatureHelp now call this function.
  • Present optional/defaulted parameters too (using CodeCompletionString::CK_Optional chunks).
rwols updated this revision to Diff 116466.Sep 23 2017, 8:59 AM
  • Add signature-help.test
  • Modify formatting.test to make the test pass. The test failed because clangd's response to the initialize request was changed to advertise signature help trigger characters, so the expected content length changed.
rwols marked 3 inline comments as done.Sep 23 2017, 9:05 AM
rwols added inline comments.
clangd/ClangdLSPServer.cpp
92 ↗(On Diff #116466)

This required me to adjust the formatting.test file.

clangd/ClangdUnit.cpp
650 ↗(On Diff #116466)

Not sure if these template names are according to style.

test/clangd/signature-help.test
39 ↗(On Diff #116466)

When there are multiple defaulted parameters after each other, the CK_Optional chunk consists of *all* of those parameters, instead of a CK_Optional chunk per parameter. This might require us to dive into SemaCodeComplete.cpp to fix this. I'm just leaving it as-is right now.

Harbormaster completed remote builds in B10561: Diff 116466.

Sorry for late response.

clangd/ClangdUnit.cpp
610 ↗(On Diff #116466)

Are we first concatenating the CodeCompletionString inside optional chunks and then trying to parse them again here?
Can we extract ProcessChunks function and recursively call it with Chunk.Optional->Chunks?

650 ↗(On Diff #116466)

It's ok. See my comment below, though, it looks like we can avoid making this function template in the first place.

652 ↗(On Diff #116466)

Maybe rename to invokeCodeCompletion? invokeClangAction seems too generic.

705 ↗(On Diff #116466)

Maybe receive CodeCompleteConsumer and not make function template?
We can return whether calling an action succeeded instead.

test/clangd/signature-help.test
39 ↗(On Diff #116466)

But does CodeCompletionString inside Chunk->Optional contain those extra parameters as a separate chunk?

rwols updated this revision to Diff 117261.Sep 30 2017, 12:10 PM
  • Updated to latest upstream revision.
  • Fix optional/default parameter handling: each default parameter now has its own label (thanks, ilya!).
  • Remove the templated "invokeClangAction" function and use polymorphism with CodeCompleteConsumer instead. The function is now called "invokeCodeComplete".
rwols marked 4 inline comments as done.Sep 30 2017, 12:14 PM

There were some failing tests, probably because we use an extra digit for the sortText property now. I haven't touched those tests.

clangd/ClangdUnit.cpp
610 ↗(On Diff #116466)

Yes, this was hokey. Fixed now (see the function getOptionalParameters)

test/clangd/signature-help.test
39 ↗(On Diff #116466)

Yes, they do. Thanks for the idea!

rwols marked 2 inline comments as done.Sep 30 2017, 1:59 PM
This comment was removed by rwols.
rwols added inline comments.Sep 30 2017, 2:15 PM
clangd/Protocol.h
458 ↗(On Diff #117261)

@malaperle I copied the sentences from the protocol markdown file over here, but judging from your comment here that might a problem. Do you suggest to change this or is this okay as-is?

rwols added inline comments.Oct 1 2017, 12:23 PM
clangd/ClangdUnit.cpp
442 ↗(On Diff #117261)

I seem to have inadvertently used the old fillSortText during the merge from upstream, let me fix that (this is why tests were failing probably).

rwols updated this revision to Diff 117290.Oct 1 2017, 12:33 PM
  • Fix fillSortText: used the old version, now using new version from upstream
  • Fix initialize-params.test and initialize-params-invalid.test to account for signature help
  • All tests pass now
ilya-biryukov added inline comments.Oct 2 2017, 3:03 AM
clangd/ClangdUnit.cpp
742 ↗(On Diff #117290)

Duplicated line sneaked into commit. It looks like the intention was to set IncludeCodePatterns.

Since we need to fill up CodeCompleteOpts in all callers when creating CodeCompletionConsumer anyway, maybe it's better to accept CodeCompleteOpts as a parameter?
Otherwise there's a high chance to forget syncing some of CodeCompleteOpts flags with the ones from CodeCompletionConsumer when changing the code later.

rwols updated this revision to Diff 117427.Oct 2 2017, 2:57 PM
  • Update invokeCodeComplete function to also take a CodeCompleteOptions, we assign it to the CompilerInstance's FrontendOpts.CodeCompleteOpts.
  • Remove unused variable in SignatureHelpCollector::ProcessOverloadCandidate.
rwols marked an inline comment as done.Oct 2 2017, 3:11 PM
malaperle added inline comments.Oct 2 2017, 7:57 PM
clangd/Protocol.h
458 ↗(On Diff #117261)

I'm not a lawyer and I have very limited understanding of how these things work but I would maybe reword the longer ones and keep it a short summary of a few words. I'll ask around what my other open source colleagues think though.

rwols updated this revision to Diff 117580.Oct 3 2017, 2:11 PM

Reword the documentation of the signature help structures in Protocol.h

rwols marked an inline comment as done.Oct 3 2017, 2:40 PM

Thinking ahead, it's probably best if we change the behavior of SnippetCompletionItemCollector as follows:

  1. If it finds a callable, provide the name of the callable, followed by an opening parenthesis, followed by $0, followed by a closing parenthesis.
  2. If it finds a code pattern, use the old behavior.

The point of (1) is that it should trigger textDocument/signatureHelp in the client immediately upon insertion of the completion. For example, if we have this function:

int foo(int x, int y);

Then the insertText would be foo($0). This causes the cursor to end up in-between the parentheses. Then it'll trigger textDocument/signatureHelp and that'll provide the parameter info.

The point of (2) is that we don't want to apply (1) for things like for-loop completions, or if-else constructs.

ilya-biryukov accepted this revision.Oct 4 2017, 1:47 AM

LGTM. Do you need my help in landing this?

  1. If it finds a callable, provide the name of the callable, followed by an opening parenthesis, followed by $0, followed by a closing parenthesis.

So that we don't clutter the source code with function parameters? This makes sense.
But I think some clients we might want to support don't have signatureHelp, so we may want to have a flag to switch between these two behaviors.

Another case where this might be bad is overloaded functions. I may choose one overload in completion, but signatureHelp will initially point into a different one.

clangd/Protocol.h
458 ↗(On Diff #117261)

I'm not a lawyer either, but I believe it does not matter whether you reword things or not, you're still bound to comply with the license when doing any derived work.

This revision is now accepted and ready to land.Oct 4 2017, 1:47 AM

Another case where this might be bad is overloaded functions. I may choose one overload in completion, but signatureHelp will initially point into a different one.

Hi, I'd just like to comment on this aspect to inform how this is done is different ways for Vim.

Original YouCompleteMe, for example, still doesn't support parameter hints, so it just offers function prototypes in the completion menu, for functions foo(int, char) and
foo(void *, int), the two prototypes gets listed and not mattering what function the user selects the text to be inserted will always just be the same function name, foo.

I didn't find this useful because it cluttered the menu with many overloads that the user may not be in fact looking for, it may be targeting for another function but it gets
like ten long overloads of another function that's ranked first according to what the user typed.

I also didn't find it useful, in Vim, to show full prototypes in the pop up menu because for C++ they tend to be rather long.

So I changed YouCompleteMe this way:

  • Just show unique function names in the popup menu (they're tagged as functions).
  • When the user selects a given function, the overloads are shown in a separate preview window instead, which for me is more sane for showing long information.
  • When the user goes about filling the call site with arguments, the overloads in the preview window are dynamically updated showing the current argument the user is at, for the remaining compatible overloads.
  • The overloads get reduced while the user fill arguments as well as there's type resolution of generic functions along the way.

These pictures show how this happens:

They just don't happen illustrate the reduction of multiple overloads because I didn't put multiple ones at the time of the screen capture.

This is just to illustrate some different completion approach that tries to be inline with libclang offerings.

@francisco.lopes, thanks for sharing your experience, I definitely like the final UX you achieved, we should aim for something similar in clangd.
It also looks like @rwols's idea of how things should look like is very close to your implementation.

I do think that showing signatures for non-overloaded functions in completion may still be useful. Overloaded functions can be grouped by name and include some indicators that there are more overloads (something like foo(int) void (+5 overloads).
How do you choose return type for the completion item if function is overloaded? Show it only when it's the same for all overloads? Go with one of the return types?

francisco.lopes added a comment.EditedOct 5 2017, 8:32 AM

@francisco.lopes, thanks for sharing your experience, I definitely like the final UX you achieved, we should aim for something similar in clangd.
It also looks like @rwols's idea of how things should look like is very close to your implementation.

Nice!

I do think that showing signatures for non-overloaded functions in completion may still be useful. Overloaded functions can be grouped by name and include some indicators that there are more overloads (something like foo(int) void (+5 overloads).
How do you choose return type for the completion item if function is overloaded? Show it only when it's the same for all overloads? Go with one of the return types?

In fact it didn't happen to me to think about the return information in the popup menu at the time, so it was not changed!, and it should just show one return type from one of the overloads randomly (I must never look at return types...).
I do think the (+ 5 overloads) approach is a good one too, though I prefer to not oversize the menu width with parameters.

It will be cool if clangd and its Language Server Protocol implementation manages to have feature parity with this, personally I'd be looking in migrating from YouCompleteMe to a LSP solution that's more modular and lightweight, possibly bringing to it what gets implemented here myself.

rwols added a comment.Oct 5 2017, 12:21 PM

LGTM. Do you need my help in landing this?

Yes, I don't have commit access, go ahead and merge. If you find any merge conflicts let me know I can probably fix that.
Since we're throwing around gifs for fun here's how signatureHelp looks in practice for Sublime Text:

francisco.lopes added a comment.EditedOct 5 2017, 2:06 PM

Since we're throwing around gifs for fun here's how signatureHelp looks in practice for Sublime Text

Very cool!

Makes me wish to finish implementing https://reviews.llvm.org/D6880. There were some special cases I didn't cover. For variadic parameter list I had no idea how to tackle well, but I also didn't touch constructors in the initialization list, so, there are no tips when initializing members in the initialization list, but I remember this requiring maybe just a bit of additional effort to implement, so if there's interest in having that, it may be worth taking a look :-)

This revision was automatically updated to reflect the committed changes.

@francisco.lopes, I've surely noticed there are a bunch of places where signatureHelp does not show up while I was playing around with this change.
It would be great to have it available in more cases.