Allow for direct jump to definition

[djrenren]

For identifiers that only have a single definition location it's annoying to have to open the split just to jump to definition. I think one of the following could help alleviate this:

• Jump directly to definition if the response list is of length 1
• Provide :LspGoToDefinition

Thoughts?

[prabirshrestha]

This is a known issue and I do plan to jump directly if there is only one item.

The only reason I currently don't jump if only one item is in the list is because you can register multiple language servers for the same filetype so even though each langserver provides only 1 result, you could have in total 2 or more results. For now to be safe I don't jump.

Why do we need :LspGoToDefinition? Isn't :LspDocumentSymbol with the above fix enough? Also I would like to have Ctrl-] and Ctrl-t working with tag stack.

[djrenren]

Gotcha, yeah that would require mechanisms to determine when all servers have returned which is nontrivial.

Isn't :LspDocumentSymbol with the above fix enough?

Just making sure you mean :LspDefinition. If so then yeah that fix would work. Was just throwing :LspGoToDefinition out in case you didn't want to jump directly when possible.

Would you be interested in a PR for a fix when there's only one language server because I think that case is pretty easily detectable and handleable.

[prabirshrestha]

Sorry for the confusion. I do mean :LspDefinition.

PRs are always welcome. The easiest solution would be to always set the total count and decrement on every response. If the count goes to zero check the total, if the total is 0 can't go to definition, if 1 can directly jump, if more than 1 show the list.

let l:ctx = { 'counter': len(l:server), 'list': [] }
...
 'on_notification': function('s:handle_location', [l:ctx, l:server, s:last_req_id, 'definition']),
...

function! s:handle_location(ctx, server, last_req_id, type, data) abort
     let l:ctx['counter'] = l:ctx['counter'] - 1
     call add(l:ctx['list'], lsp#ui#vim#utils#locations_to_loc_list(a:data))

     if l:ctx['counter'] == 0
        if len(l:ctx['list']) == 1
             " jump without showing quickfix list
        endif
     else
     ....
     endif
endfunction

This would handle for all cases, but then you might get a flicker if there are more than 2 servers that support goto definition and the list length is only 1. I would have a a flag that would allow us to enable/disable it. I would be fine even with default being enabled.

[djrenren]

Yeah that sounds like a great approach!

We could avoid the flicker by waiting for responses or timeouts from all the servers before displaying the panel. Something like the following (my VimL is rusty so hopefully this makes sense):

let l:ctx = { 'counter': len(l:server), 'list': [], 'timeout': 0 }
...
 'on_notification': function('s:handle_location', [l:ctx, l:server, s:last_req_id, 'definition']),
...
call timer_start(1000, function('s:timeout', [l:ctx]))
...
function! s:timeout(ctx)
    l:ctx['timeout'] = 1
    if l:ctx['counter'] == 0
         " show quickfix
    endif
endfunction

function! s:handle_location(ctx, server, last_req_id, type, data) abort
     if l:ctx['timeout']
        return 0
     endif
     let l:ctx['counter'] = l:ctx['counter'] - 1
     call add(l:ctx['list'], lsp#ui#vim#utils#locations_to_loc_list(a:data))

     if l:ctx['counter'] == 0
        if len(l:ctx['list']) == 1
             " jump without showing quickfix list
        else
             " show quickfix
        endif
     endif
endfunction

Afterthought

There's probably a more sophisticated version of this that makes timeout handling a part of lsp_start. With a little plumbing it would allow for an onTimeout parameter. It seems like timeouts in general are a good idea because technically the server could respond half an hour after you ask it for something and it would be annoying to present UI at that point, but that could be done later

[prabirshrestha]

For now I wouldn't worry about timeouts as it would increase complexity. For now we can assume that majority will only have 1 definitionProvider. Currently I think other servers will most likely be linters or formatters that doesn't support definitionProvider.

Out of curiosity for what language are you using LSP?

[djrenren]

Dramatic answer: all of them

But actually, I'm working on google/kythe which creates static indexes of large multilingual codebases. So I've written a languageserver that taps into that static index to provide things like go to definition and references. This is really helpful in the cases of C bindings, proto files, etc but also in cases where the build system is just too complex for the regular old language server to figure it out. Unfortunately cuz it's a static index, it's not very good at managing dirty state so it definitely works best alongside another languageserver that does. So my optimal use-case does involve multiple definition providers but I totally agree it's not the norm.

[prabirshrestha]

That make sense. Would let g:lsp_autoclose_list_on_select = 1 be better for now?

[djrenren]

I think it would be really nice but ultimately its an orthogonal feature that could be used to alleviate the pain of not having jumping directly to definition working.

I've posted a PR for getting jump to definition working when we only get one location back from all the language servers. #24

[prabirshrestha]

PR is merged. For now I will close this but please do feel free to file bugs/feature requests.

[grajhn]

Hi, do you know if this is addressed? I am using OpenGROK 1.2.7 and still see the same issue of not not jumping to the function definition if there is only definition. If the definition happens to be in the same file, it works. But it doesn't work if the definition is in another file.