Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] WIP: Signature help #1255

Closed
wants to merge 6 commits into from

Conversation

puremourning
Copy link
Member

@puremourning puremourning commented May 31, 2019

Ref: ycm-core/YouCompleteMe#234

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:

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

Screenshot 2019-05-31 at 23 32 31

Screenshot 2019-05-31 at 23 33 05

Screenshot 2019-05-31 at 23 33 39

image

Longer demo:

YCM-popup-win-arg


This change is Reviewable

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

oblitum commented Jun 7, 2019

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)

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

codecov bot commented Jun 26, 2019

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

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Reviewable status: 1 of 2 LGTMs obtained

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

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
Copy link

codecov-io commented Jul 14, 2019

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

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :lgtm_strong:

Reviewable status: 1 of 2 LGTMs obtained

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

Superseded by individual PRs, specifically #1324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants