[READY] SignatureHelp: Support signature help for c-family using clangd

[puremourning]

This implements the LSP signature help support and tests it for clangd.

A new options was added: disable_signature_help. if for some reason you
don't like signature help, this can be set to disable it.

NOTE: No special changes were required to clangd_completer as this is
implemnented completesly in the language_server_completer layer.
Therefore, this change acutally makes signature help work for any LSP
based completer, while tests are only provided (for now) for c-family
using clangd..

This isn't perfect yet: there's no capability mechanism to indicate
that signature help isn't supported for a filetype so the client will
just constantly send requests that return emnpty. For now, we don't
solve that, instead deferring that decision to either send an error
(like we do with the message poll), or add a new capability response,
e.g. to the completer available reqeust (such as adding a version 2 of
that request that returns an objext).

---

This change is 

[codecov]

Codecov Report

Merging #1324 into master will increase coverage by 0.02%.
The diff coverage is 99.07%.

@@            Coverage Diff             @@
##           master    #1324      +/-   ##
==========================================
+ Coverage   97.16%   97.18%   +0.02%     
==========================================
  Files          96       96              
  Lines        7446     7543      +97     
  Branches      153      153              
==========================================
+ Hits         7235     7331      +96     
- Misses        170      171       +1     
  Partials       41       41

[bstaletic]

First of all, thanks a lot. This has been a ton of work.
I made a few comments, but none are blocking. This is LGTM as far as I'm concerned.

I'm going to completely ignore what codecov is currently saying, because other tests for this will come and the codecov data isn't quite consistent on the website.

Reviewed 21 of 21 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

docs/_config.yml, line 0 at r1 (raw file):
Intentionally deleted?

---

docs/openapi.yml, line 920 at r1 (raw file):

collude

Is that a word? *googling*

collude

intransitive verb
To act together, often in secret, to achieve an illegal or improper purpose.

Conclusion: It is a word and ycmd is doing something illegal. I'm fine with that.

---

docs/openapi.yml, line 973 at r1 (raw file):

              extra_conf_data:
                $ref: "#/definitions/ExtraConfData"
              signature_help_state:

Do we need all this data in the request? I guess the answer, strictly, is we don't, but it's consistent with all other requests, especially the completion request.

---

ycmd/completers/completer.py, line 238 at r1 (raw file):

      return False

    state = request_data.get( 'signature_help_state', 'INACTIVE' )

Unless I missed something, the state can only be ACTIVE or INACTIVE. What about using 1 and 0 respectively? I'm guessing your choice of using actual words was readability. Do we care about 5 or 7 bytes extra constantly being marshaled YCM<->ycmd?

---

ycmd/completers/language_server/language_server_completer.py, line 1674 at r1 (raw file):

        server_trigger_characters = (
          ( self._server_capabilities.get( 'signatureHelpProvider' ) or {} )
                                     .get( 'triggerCharacters' ) or []

Just curious, what happens with servers who declare signature help capability, but then return[] trigger characters?

---

ycmd/completers/language_server/language_server_protocol.py, line 267 at r1 (raw file):

          'signatureInformation': {
            'parameterInformation': {
              'labelOffsetSupport': False, # For now.

Should we care?

[puremourning]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

---

docs/_config.yml, line at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Intentionally deleted?

Actually no, this file was specifying a template and causing a warning in GitHub config page. As part of updating the docs, I squished it.

---

docs/openapi.yml, line 920 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

collude

Is that a word? *googling*

collude

intransitive verb
To act together, often in secret, to achieve an illegal or improper purpose.

Conclusion: It is a word and ycmd is doing something illegal. I'm fine with that.

It's a word :)

---

docs/openapi.yml, line 941 at r1 (raw file):

        Identifying the position of an argument in the `label` for a signature
        required matching the position of the text supplied in the `label` for
        the active parameter of the active signature.

We inherited this nonsense from the LSP. Maybe ycmd protocol should always supply the correct range, and any hacking required to implement this string matching should be done in ycmd rather than in every client.

---

docs/openapi.yml, line 973 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Do we need all this data in the request? I guess the answer, strictly, is we don't, but it's consistent with all other requests, especially the completion request.

do you mean the signature_help_state? If so, yes we need it. If yo mean the other stuff, then possibly we strictly don't, and this could be a potentially significant optimisation maybe.

One thing I am a little worried about with this PR is a performance regression in the sense that the client is sending a lot of these requests, which contain all of the dirty burners and everything.

---

ycmd/completers/completer.py, line 238 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Unless I missed something, the state can only be ACTIVE or INACTIVE. What about using 1 and 0 respectively? I'm guessing your choice of using actual words was readability. Do we care about 5 or 7 bytes extra constantly being marshaled YCM<->ycmd?

Yeah words for readability, and the possibility of other states in the future. I'm not super worried about the extra bytes on the basis that the file data dwarfs this.

---

ycmd/completers/language_server/language_server_completer.py, line 1674 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Just curious, what happens with servers who declare signature help capability, but then return[] trigger characters?

well, funny enough jdt.ls does exactly that I think. We have the ability to override them with a completer callback in the same way we can for completion trigger characters, and I think that's what we do.

---

ycmd/completers/language_server/language_server_protocol.py, line 267 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Should we care?

I worried that if they were sending label offsets they might not send something that works with our somewhat dumber scheme. though I haven't found a server that supports label offsets, it makes sense to be honest in the capabilities negotiation.

[bstaletic]

as far as I'm concerned. Once again, thanks for all the work.

Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

---

docs/openapi.yml, line 941 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

        Identifying the position of an argument in the `label` for a signature
        required matching the position of the text supplied in the `label` for
        the active parameter of the active signature.

We inherited this nonsense from the LSP. Maybe ycmd protocol should always supply the correct range, and any hacking required to implement this string matching should be done in ycmd rather than in every client.

Am I reading this correctly? This means someone (either the client or the server) needs to basically do int offset = strstr( active_sig, active_arg );?

If someone has to do it, does it really matter who?

---

docs/openapi.yml, line 973 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

do you mean the signature_help_state? If so, yes we need it. If yo mean the other stuff, then possibly we strictly don't, and this could be a potentially significant optimisation maybe.

One thing I am a little worried about with this PR is a performance regression in the sense that the client is sending a lot of these requests, which contain all of the dirty burners and everything.

I wasn't talking strictly about signature_help_state, just a long list of parameters.
You've raised that concern before. So there are two things that could significantly improve performance:

• A way to make the client stop asking for /signature_help.
• Slimmer API.

I'm unsure if we should block the PR until we figure those two out. I'm leaning towards "no" and just improve over time. Especially since there's a way to turn off the signature help from the user's side.

---

ycmd/completers/language_server/language_server_protocol.py, line 267 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I worried that if they were sending label offsets they might not send something that works with our somewhat dumber scheme. though I haven't found a server that supports label offsets, it makes sense to be honest in the capabilities negotiation.

So another "don't worry until you have to" part of the protocol. That's fine by me.

[puremourning]

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

---

docs/openapi.yml, line 941 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Am I reading this correctly? This means someone (either the client or the server) needs to basically do int offset = strstr( active_sig, active_arg );?

If someone has to do it, does it really matter who?

Yeah you read correctly. I guess if ycmd does it, then it is done once (rather than in every client), and we can more easily change it to iron out the badness in LSP.

---

docs/openapi.yml, line 973 at r1 (raw file):

there's a way to turn off the signature help from the user's side.

Actually, there isn't a way to tell Vim client not to send the request. Should probably add that (currently it will always send the request if the Vim suports it), but it's trivial to use the same flag to just totally turn it off; that would be a good get-out-of-jail.

We can even say that the whole signature help is experimental and anything goes, including removing the switch in future.

[bstaletic]

Reviewed 1 of 1 files at r2.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

---

docs/openapi.yml, line 941 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Yeah you read correctly. I guess if ycmd does it, then it is done once (rather than in every client), and we can more easily change it to iron out the badness in LSP.

Alright, I guess we should do it on the ycmd side. How about sending a begin and end offset and not sending active argument text at all? We'd be seting something resembling C++ iterator pair instead of a piece of text.

---

docs/openapi.yml, line 973 at r1 (raw file):

there's a way to turn off the signature help from the user's side.

Actually, there isn't a way to tell Vim client not to send the request.

Yeah, but there will be a disable_signature_help option. I still think the option will be useless once we make the client aware that it shouldn't send the request when the completer doesn't support signature help and trim down the signature help request parameters, so maybe marking this whole thing ex experimental isn't a bad idea. That said, I think it's perfectly fine to iteratively optimize signature help and not get it absolutely perfect on the first try.

[puremourning]

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic)

---

docs/openapi.yml, line 973 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

there's a way to turn off the signature help from the user's side.

Actually, there isn't a way to tell Vim client not to send the request.

Yeah, but there will be a disable_signature_help option. I still think the option will be useless once we make the client aware that it shouldn't send the request when the completer doesn't support signature help and trim down the signature help request parameters, so maybe marking this whole thing ex experimental isn't a bad idea. That said, I think it's perfectly fine to iteratively optimize signature help and not get it absolutely perfect on the first try.

Thanks, you're making this happen because you're awesome.

[puremourning]

Reviewed 13 of 21 files at r1, 6 of 7 files at r3, 4 of 4 files at r4.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic)

[bstaletic]

Reviewed 7 of 7 files at r3, 4 of 4 files at r4, 2 of 2 files at r5.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

---

docs/openapi.yml, line 973 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Thanks, you're making this happen because you're awesome.

Nah, I just want to get this merged, so we merge the client side and then I'll be able to file a couple of bugs which are hard to describe without vimscript test framework.
So, I can't take all the credit!

[bstaletic]

Anything else before we merge this?

Reviewed 1 of 4 files at r6, 10 of 10 files at r7.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

---

ycmd/completers/language_server/language_server_completer.py, line 1083 at r7 (raw file):

  def ComputeSignaturesInner( self, request_data ):
    if not self.ServerIsReady():
      return {}

This line isn't covered. Is it dead code? If so, let's remove it, if not, who cares.

Definitely non-blocking.

[puremourning]

I thin this is good to go now.

Once this is in, I have PRs for python (required for the client tests to pass!), go and java.

Reviewed 1 of 4 files at r6, 10 of 10 files at r7.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

[bstaletic]

Thanks a lot for all the work.

Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

[puremourning]

Reviewed 2 of 2 files at r8.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

---

ycmd/handlers.py, line 150 at r8 (raw file):

  if not _server_state.FiletypeCompletionUsable( request_data[ 'filetypes' ],
                                                 silent = True ):
    # FIXME: return an error that indicates the filetype completion isn't

we can remove this FIXME

---

ycmd/completers/language_server/language_server_completer.py, line 1083 at r7 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

This line isn't covered. Is it dead code? If so, let's remove it, if not, who cares.

Definitely non-blocking.

not going to loose sleep over it.

[puremourning]

I think this is good to merge now. finally.

Reviewed 1 of 1 files at r9.
Reviewable status: complete! 2 of 2 LGTMs obtained (waiting on @bstaletic)

[mergify]

Thanks for sending a PR!

[bstaletic]

Woohoo!
@puremourning thanks for all the work you put into this.