[RFC] WIP: Signature help #1255

puremourning · 2019-05-31T22:39:28Z

Status

This opens up the discussion for adding signature help based on popup menu support that's being implemented in Vim.

It's just a prototype at the moment, but opening a PR to start off a discussion about implementation details.

Design

Current implementation is to add the signature help information to the 'completions' request. The advantage of this is that it requires the least client code change. The disadvantage is that it strictly delays completions in the general case.

An alternative would be to add /signature_help endpoint and all the client code to manage that request separately. It's an option, but I wanted something quick.

API

The ycmd API is to add to the completion request:

signature_help_state the current state of the menu. 'ACTIVE' or 'INACTIVE'. This is necessary to constantly re-trigger signature help while the popup is visible. This is the only way in LSP to know when to close th popup: when the sig help returns empty.

And on the reply:

signature_info: LSP signature help response.

Why beat around the bush? We just use the LSP format directly because why not. This decision can change if we so decide.

Misc

This branch also includes #1250 which I needed to test with clangd.
Only supports LSP right now, because that's easy. Tested with clangd and jdt.ls.

Demos

Longer demo:

This change is

bstaletic

I know @micbou wanted to have a separate request for signature hinting instead of reusing /completions.

Reviewed 4 of 7 files at r1, 14 of 15 files at r2, 7 of 7 files at r3.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

ycmd/default_settings.json, line 7 at r3 (raw file):

  "min_num_identifier_candidate_chars": 0,
  "semantic_triggers": {},
  "signature_triggers": {},

Is there any reason to expose these options?

ycmd/completers/completer.py, line 187 at r3 (raw file):

    # FIXME: these .get() are required due to the OmniCompleter, which somehow
    # doesn't seem to load the default settings. I guess there is a subset of
    # settings which the Vim client is hard-coded to know about.

All YCM client options are listed in plugin/youcompleteme.py. It was one of the first steps to decouple YCM and ycmd.

ycmd/completers/completer.py, line 193 at r3 (raw file):

              user_options.get( 'signature_triggers' ) or {} ),
            filetype_set = set( self.SupportedFiletypes() ),
            default_triggers = {} )

If I remember correctly LSP server sends the argument hint triggers. Is that right? If so, can ycmd take advantage of that?

ycmd/completers/completer.py, line 240 at r3 (raw file):

      return False

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

Do we need 'ACTIVE' and 'INACTIVE'? Why not just 1 and 0?

ycmd/completers/language_server/language_server_protocol.py, line 258 at r3 (raw file):

          'contentFormat': [
            'plaintext',
            'markdown'

Why markdown? What are we going to do with markdown formatted hover response?

ycmd/completers/language_server/language_server_protocol.py, line 264 at r3 (raw file):

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

Forgive my lazyness, but what is labelOffsetSupport? Or rather what is labelOffset in terms of signature help?

ycmd/completers/language_server/language_server_protocol.py, line 268 at r3 (raw file):

            'documentationFormat': [
              'plaintext',
              'markdown'

Again, what to do with markdown formated response?

puremourning

Yah I think he's right. This is implementation is convenient but not optimal.

Thanks for taking a look

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

ycmd/default_settings.json, line 7 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Is there any reason to expose these options?

only put it here to ensure it is set in the dict and to be consistent with semantic_triggers. I guess we could remove it or at least not document it (I certainly haven't tested it)

ycmd/completers/completer.py, line 193 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

If I remember correctly LSP server sends the argument hint triggers. Is that right? If so, can ycmd take advantage of that?

It does, and we do. In language_server_completer.py it gets the signature triggers and calls the method update them with server triggers (just like semantic triggers)

ycmd/completers/completer.py, line 240 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Do we need 'ACTIVE' and 'INACTIVE'? Why not just 1 and 0?

I prefer readable code. 1 and 0 are magic numbers, and don't allow for any tertiary state.

ycmd/completers/language_server/language_server_protocol.py, line 258 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why markdown? What are we going to do with markdown formatted hover response?

This is expressing a preference order of supported formats. We prefer plaintext, but as markdown is plaintext, too, we can render it and users can still read it.

ycmd/completers/language_server/language_server_protocol.py, line 264 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Forgive my lazyness, but what is labelOffsetSupport? Or rather what is labelOffset in terms of signature help?

labelOffsets are kinda dumb. The way the protocol is set up, is you get something like this (without label offsets):

{
    currentParameter: 1,
    currentSignature: 0,
    signature: {
        label: "function_name( arg one, arg& two )" ,
        parameters: [ 
            { label: "arg one" },
            { label" "arg& two" }
        ]
}

So what we do in the YCM client to highlight the current param is:

for the active parameter, find the string within the signature label
Use a text property to span that range with the "PMenuSel" highlight group

So in order to find the "range" of the param, you have to use string match.

The purpose of the labelOffset is to say where within the signature's label the parameter actually falls, to (presumably) avoid the string matching, which I suppose could be borked.

It's a really dumb API IMO, but it's simple enough to make it work.

ycmd/completers/language_server/language_server_protocol.py, line 268 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Again, what to do with markdown formated response?

As above. Markdown is plaintext, so we can render it if we have to.

bstaletic

Reviewed 1 of 22 files at r4, 14 of 15 files at r5, 7 of 7 files at r6.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

ycmd/default_settings.json, line 7 at r3 (raw file):

Previously, puremourning (Ben Jackson) wrote…

only put it here to ensure it is set in the dict and to be consistent with semantic_triggers. I guess we could remove it or at least not document it (I certainly haven't tested it)

I'm just worried what will happen if a user starts to mess with these.

ycmd/completers/completer.py, line 240 at r3 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I prefer readable code. 1 and 0 are magic numbers, and don't allow for any tertiary state.

Fair enough. Carry on.

oblitum · 2019-06-07T21:20:07Z

The purpose of the labelOffset is to say where within the signature's label the parameter actually falls, to (presumably) avoid the string matching, which I suppose could be borked.

It's a really dumb API IMO, but it's simple enough to make it work.

tips: neoclide/coc.nvim#269 (comment)

bstaletic

Now that there is /signature_help, we should document it.

Reviewed 1 of 12 files at r7, 11 of 11 files at r8.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

README.md, line 105 at r8 (raw file):

for Go (using [Godef][godef] for jumping to definitions), a TSServer-based
completer for JavaScript and TypeScript, a [jdt.ls][jdtls]-based server for
Java, and a [RLS][]-based completer for Rust.  More will be added with time.

I know it's a stupid nitpick, but why two spaces after a full stop?

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

  request_data = RequestWrap( request.json )

  if not  _server_state.ShouldUseFiletypeCompleter( request_data ):

if not is followed by two spaces, causing flake8 to fail the build.

bstaletic

Reviewed 13 of 15 files at r10, 4 of 4 files at r11.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

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

Previously, bstaletic (Boris Staletic) wrote…

if not is followed by two spaces, causing flake8 to fail the build.

Please fix this, so that flake8 stops having fits.

puremourning

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

ycmd/default_settings.json, line 7 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I'm just worried what will happen if a user starts to mess with these.

it works the same as the semantic triggers that are already exposed. they are essentially additive. I'm not worried.

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

Previously, bstaletic (Boris Staletic) wrote…

Please fix this, so that flake8 stops having fits.

Done.

bstaletic

Reviewed 13 of 15 files at r13, 4 of 4 files at r14.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

ycmd/default_settings.json, line 7 at r3 (raw file):

Previously, puremourning (Ben Jackson) wrote…

it works the same as the semantic triggers that are already exposed. they are essentially additive. I'm not worried.

Alright, moving on.

bstaletic

The tests are passing, but there's something wrong with codecov. It seems completely busted.

Reviewed 4 of 16 files at r15, 4 of 5 files at r16, 1 of 1 files at r17, 2 of 2 files at r18, 4 of 4 files at r19.
Reviewable status: 0 of 2 LGTMs obtained

codecov · 2019-06-26T23:42:40Z

Codecov Report

Merging #1255 into master will decrease coverage by 1.52%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##           master    #1255      +/-   ##
==========================================
- Coverage    97.2%   95.68%   -1.53%     
==========================================
  Files          96       96              
  Lines        7436     7250     -186     
  Branches      153      151       -2     
==========================================
- Hits         7228     6937     -291     
- Misses        167      265      +98     
- Partials       41       48       +7

bstaletic

Reviewed 8 of 8 files at r20, 7 of 7 files at r21, 8 of 8 files at r22.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

ycmd/tests/clang/signature_help_test.py, line 38 at r22 (raw file):

@SharedYcmd
def SignatureHelp_NotImplemented_test( app ):

Why is this in clang directory if we don't support signature help in libclang completer?

puremourning

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

ycmd/tests/clang/signature_help_test.py, line 38 at r22 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why is this in clang directory if we don't support signature help in libclang completer?

It's actually testing the behaviour of sending signature help to a completer that doesn't implement it and checking that works. Ultimately I'm toying with ideas about how we communicate that (completer doesn't support sig help) to the ycmd client. But for now, this just covers the code in Completer class which returns the default empty response.

bstaletic

The only issue that I have found is on the client side, so

Reviewable status: 1 of 2 LGTMs obtained

codecov-io · 2019-07-14T18:34:02Z

Codecov Report

Merging #1255 into master will decrease coverage by 0.07%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##           master    #1255      +/-   ##
==========================================
- Coverage    97.2%   97.12%   -0.08%     
==========================================
  Files          96       96              
  Lines        7435     7514      +79     
  Branches      153      153              
==========================================
+ Hits         7227     7298      +71     
- Misses        167      175       +8     
  Partials       41       41

codecov-io · 2019-07-14T18:34:02Z

Codecov Report

Merging #1255 into master will decrease coverage by 0.07%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #1255      +/-   ##
==========================================
- Coverage   97.19%   97.11%   -0.08%     
==========================================
  Files          96       96              
  Lines        7457     7534      +77     
  Branches      153      153              
==========================================
+ Hits         7248     7317      +69     
- Misses        168      176       +8     
  Partials       41       41

bstaletic

So what is missing at this point? I know you've mentioned that currently YCM/ycmd are wasteful about signature help requests for completers that don't support it, but if we decide to make a /signature_help_available endpoint, we can do that in a separate PR.

Reviewed 2 of 2 files at r23, 9 of 9 files at r24, 1 of 1 files at r25, 1 of 1 files at r26, 1 of 1 files at r27.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale)

bstaletic

I hit "publish" too fast...

Codecov is complaining about low test coverage. The lines that aren't covered are exceptions and "if not ready, return empty". I'm fine if you decide not to care about it.

Thanks a lot for this PR, it was a massive amount of work.

Reviewable status: 1 of 2 LGTMs obtained

bstaletic

Reviewed 2 of 2 files at r28, 2 of 2 files at r29, 3 of 4 files at r30.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

.gitignore, line 93 at r30 (raw file):

# clangd index data
.clangd/

I though I was the only one being annoyed by this.

ycmd/completers/completer_utils.py, line 144 at r30 (raw file):

    if start_codepoint <= match.end() and match.end() <= column_codepoint:
      return True
  return False

Why is this not returning a codepoint any more?

puremourning

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

ycmd/completers/completer_utils.py, line 144 at r30 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why is this not returning a codepoint any more?

I removed some WIP commits that aren’t used. The idea was to align the popup with the trigger character. I never finished it and we can defer it if we want to investigate later. This just reverts it to the current state.

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

puremourning · 2019-09-19T17:11:32Z

Superseded by individual PRs, specifically #1324

puremourning mentioned this pull request May 31, 2019

[READY]: Signature help ycm-core/YouCompleteMe#3412

Merged

4 tasks

bstaletic requested changes Jun 1, 2019

View reviewed changes

puremourning commented Jun 1, 2019

View reviewed changes

puremourning force-pushed the signature-help branch from 07d25f4 to 957bddd Compare June 6, 2019 19:35

bstaletic requested changes Jun 7, 2019

View reviewed changes

puremourning force-pushed the signature-help branch from 957bddd to 9cfe412 Compare June 8, 2019 14:18

bstaletic requested changes Jun 8, 2019

View reviewed changes

puremourning force-pushed the signature-help branch 2 times, most recently from 9b99fe7 to aea26f5 Compare June 12, 2019 18:20

bstaletic requested changes Jun 15, 2019

View reviewed changes

puremourning force-pushed the signature-help branch from aea26f5 to a6aa0b6 Compare June 15, 2019 18:18

puremourning commented Jun 15, 2019

View reviewed changes

bstaletic reviewed Jun 15, 2019

View reviewed changes

puremourning force-pushed the signature-help branch 3 times, most recently from bc1210b to b524a3e Compare June 25, 2019 20:13

puremourning force-pushed the signature-help branch from b524a3e to 40669b7 Compare June 26, 2019 16:05

bstaletic approved these changes Jun 26, 2019

View reviewed changes

puremourning force-pushed the signature-help branch 2 times, most recently from 2061b3b to e40ef92 Compare June 26, 2019 22:27

puremourning force-pushed the signature-help branch from e40ef92 to 006488a Compare June 27, 2019 21:02

puremourning force-pushed the signature-help branch from 006488a to 73141ca Compare July 5, 2019 20:40

bstaletic requested changes Jul 6, 2019

View reviewed changes

puremourning commented Jul 6, 2019

View reviewed changes

bstaletic approved these changes Jul 6, 2019

View reviewed changes

puremourning force-pushed the signature-help branch 3 times, most recently from c3d0f0a to dff3476 Compare July 14, 2019 18:33

puremourning force-pushed the signature-help branch 3 times, most recently from a7838ff to d7e1cbb Compare July 27, 2019 16:29

puremourning force-pushed the signature-help branch from d7e1cbb to d63374c Compare August 9, 2019 22:53

bstaletic approved these changes Aug 10, 2019

View reviewed changes

puremourning force-pushed the signature-help branch from d63374c to a4316c7 Compare August 11, 2019 22:15

puremourning force-pushed the signature-help branch from 6f01d49 to ed22277 Compare August 26, 2019 10:58

bstaletic mentioned this pull request Sep 13, 2019

Question : Completion Not Contextual ycm-core/YouCompleteMe#3485

Closed

8 tasks

puremourning force-pushed the signature-help branch from 50aba5f to c47e411 Compare September 18, 2019 12:29

bstaletic requested changes Sep 18, 2019

View reviewed changes

puremourning commented Sep 18, 2019

View reviewed changes

puremourning added 6 commits September 18, 2019 21:48

Ignore build output directories created by roslyn

ba1d681

SignatureHelp: Document API

f0e3001

Python: implement signature help

773ca21

Java: Implement and test signatiure help

b341e1f

Go: Implement and test signature help

06e03ff

puremourning force-pushed the signature-help branch from c47e411 to 06e03ff Compare September 18, 2019 21:36

puremourning closed this Sep 19, 2019

ericvw mentioned this pull request Sep 24, 2021

Support displaying signature after selecting completion hrsh7th/nvim-cmp#247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] WIP: Signature help #1255

[RFC] WIP: Signature help #1255

puremourning commented May 31, 2019 •

edited by Valloric

Loading

bstaletic left a comment

puremourning left a comment

bstaletic left a comment

oblitum commented Jun 7, 2019

bstaletic left a comment

bstaletic left a comment

puremourning left a comment

bstaletic left a comment

bstaletic left a comment

codecov bot commented Jun 26, 2019 •

edited

Loading

bstaletic left a comment

puremourning left a comment

bstaletic left a comment

codecov-io commented Jul 14, 2019

codecov-io commented Jul 14, 2019 •

edited by codecov bot

Loading

bstaletic left a comment

bstaletic left a comment

bstaletic left a comment

puremourning left a comment

puremourning commented Sep 19, 2019

[RFC] WIP: Signature help #1255

[RFC] WIP: Signature help #1255

Conversation

puremourning commented May 31, 2019 • edited by Valloric Loading

Status

Design

API

Misc

Demos

bstaletic left a comment

Choose a reason for hiding this comment

puremourning left a comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

oblitum commented Jun 7, 2019

bstaletic left a comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

puremourning left a comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 26, 2019 • edited Loading

Codecov Report

bstaletic left a comment

Choose a reason for hiding this comment

puremourning left a comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

codecov-io commented Jul 14, 2019

Codecov Report

codecov-io commented Jul 14, 2019 • edited by codecov bot Loading

Codecov Report

bstaletic left a comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

puremourning left a comment

Choose a reason for hiding this comment

puremourning commented Sep 19, 2019

puremourning commented May 31, 2019 •

edited by Valloric

Loading

codecov bot commented Jun 26, 2019 •

edited

Loading

codecov-io commented Jul 14, 2019 •

edited by codecov bot

Loading