mini.nvim icon indicating copy to clipboard operation
mini.nvim copied to clipboard

[mini.completion] Missing function / variable definition (result.detail) from popup docs for languages like TypeScript

Open GitMurf opened this issue 1 year ago • 23 comments

Contributing guidelines

Module(s)

mini.completion

Description

For languages like TypeScript I was struggling with mini.completions because the popup docs for each completion item was not showing the function / variable definition (i.e., function signature / variable type). As you can see here, for the JSON.stringify() builtin function, it only includes the documentation "annotations" explaining the function but not the function definition itself.

image

But as you can see in the LSP response, the detail property holds this information. So we need to concatenate result.detail with documentation.value.

image

Compared to a language like Lua, the function definition is included in the documentation.value prop in the markdown.

image

image

Now the one caveat is that for a language like Lua, it has function in the result.detail prop so if you concatenated it for Lua, it would be a little redundant since the full function definition is also included at the start of the documentation.value prop.

Neovim version

Neovim nightly

Steps to reproduce

Explained in detail in description...

Expected behavior

Explained in detail in description...

Actual behavior

Explained in detail in description...

GitMurf avatar Oct 05 '24 08:10 GitMurf

As an additional item which is related, but more of a "nice to have" than above which is pretty important to the core functionality of completions, there is also the labelDetails.description value which tells you the library an import statement will be pulling from in JS / TS.

image

In nvim-cmp they add this so that you can see what library will be pulled in when an auto import statement is generated from the completion.

image

This is super nice, especially for ambiguous names that may have options from multiple libraries... BUT this I could do without, as opposed to the original issue above which I consider vital as props on an object will not even show you which type they are (no docs popup comes up at all for variable props).

GitMurf avatar Oct 05 '24 08:10 GitMurf

Thanks for the suggestion!

I've only now found out about the detail field in CompletionItem. Judging by the description, it is something that is meant to be put beside completion item in the initial popup menu. However, I mostly prefer it it to not be wide.

Same goes for labelDetails.

I'll take some time to think about this, but currently I am skeptical that this will be part of 'mini.completion'.

echasnovski avatar Oct 05 '24 09:10 echasnovski

I'll take some time to think about this, but currently I am skeptical that this will be part of 'mini.completion'.

Thanks for taking a look so quickly. This is not meant to be "aggressive" 🤣 but just to clarify understanding of the impact, without having the detail I really don't think mini.completion is usable for me as a typescript dev. The function definitions is one thing, but the main deal breaker is that for an object properties, it shows no docs pop up at all because it is only in the detail field.

For example, if I have this object:

FooObj {
  foo: string;
  bar: number;
  baz: Date;
}

And if I have to supply it as a prop to a function like: fooFunc(fooObj)

When I do:

fooFunc({
  baz| // (autocomplete shows no "doc" popup so I do not know the type of the prop is a Date)
})

Hopefully this makes sense. I would agree this likely is something the typescript LSP could populate things differently, but from experience I know getting the LSP to change is not an option 😉).

GitMurf avatar Oct 05 '24 16:10 GitMurf

@echasnovski curious your thoughts on a hook that provides the docs.value, details and maybe the labelDetails as args for the hook and then the user can choose how to populate the docs popup?

If the docs popup is correct then I am not too worried about the initial completion item list. Although in a perfect world a hook for the completion menu items would be amazing. Thoughts?

GitMurf avatar Oct 05 '24 16:10 GitMurf

I suppose would also need to provide the lsp name as an arg so you could do conditional logic based on the lsp/language.

GitMurf avatar Oct 05 '24 17:10 GitMurf

@echasnovski curious your thoughts on a hook that provides the docs.value, details and maybe the labelDetails as args for the hook and then the user can choose how to populate the docs popup?

If the docs popup is correct then I am not too worried about the initial completion item list. Although in a perfect world a hook for the completion menu items would be amazing. Thoughts?

There is config.lsp_completion.process_items. Users can manipulate raw items to populate their documentation.value and label the way they like.

Also, after looking at code, the detail value of completion item should be shown beside it but not in the info window (that is what menu field is for in completion items).

echasnovski avatar Oct 05 '24 17:10 echasnovski

@echasnovski I'll do a little PR as proof of concept and make sure it could work out simple enough. No pressure to integrate it as I'm fine with just using my fork for the time being.

GitMurf avatar Oct 05 '24 18:10 GitMurf

@echasnovski I'll do a little PR as proof of concept and make sure it could work out simple enough. No pressure to integrate it as I'm fine with just using my fork for the time being.

Sorry on mobile so did not see your response before sending this.

GitMurf avatar Oct 05 '24 18:10 GitMurf

@echasnovski I'll do a little PR as proof of concept and make sure it could work out simple enough. No pressure to integrate it as I'm fine with just using my fork for the time being.

Would you mind only making a change in your fork and post the link to the branch/commit? I'd like to have PR count as low as possible and I have doubts there will be changes in 'mini.completion' regarding this issue.

echasnovski avatar Oct 05 '24 18:10 echasnovski

@echasnovski I'll do a little PR as proof of concept and make sure it could work out simple enough. No pressure to integrate it as I'm fine with just using my fork for the time being.

Would you mind only making a change in your fork and post the link to the branch/commit? I'd like to have PR count as low as possible and I have doubts there will be changes in 'mini.completion' regarding this issue.

No problem. And see my latest message above. I did not see your other message before I sent that. I will dig into the processitems config and see if it gives what is needed but last time I checked it did not. Part of it may be that the docs come through after the initial request and only when you ctrl-n/p select an item and it gets the secondary response / request from the LSP in your code. Not at a computer right now so explaining off memory. Will take a look when back at computer tonight likely after I put the kids to bed.

GitMurf avatar Oct 05 '24 18:10 GitMurf

Part of it may be that the docs come through after the initial request and only when you ctrl-n/p select an item and it gets the secondary response / request from the LSP

@echasnovski so I think this may be the issue. See the lines from your code below. Correct me if I'm wrong, but this "secondary" request when there is no documentation returned sends completionItem/resolve after "cycling" through over items in the completion menu (<C-n/p>) and then that response updates the documentation popup window. I am not seeing it come through in items with the process_items hook.

https://github.com/echasnovski/mini.nvim/blob/61e5d46fc0cf71306c51275383767d996f559a60/lua/mini/completion.lua#L1034-L1052

Does what I am saying make sense? I am still somewhat new to diving into the LSP completions stuff so if I am not describing it very well let me know and I can try to clarify.

GitMurf avatar Oct 05 '24 22:10 GitMurf

Also, after looking at code, the detail value of completion item should be shown beside it but not in the info window (that is what menu field is for in completion items).

Are you sure you aren't mixing up labelDetails with detail? Not saying just because nvim-cmp does something, it is correct, but they put to the side (in menu) the labelDetails.

image

GitMurf avatar Oct 05 '24 22:10 GitMurf

Here is where nvim-cmp sets menu to labelDetails (not detail).

https://github.com/hrsh7th/nvim-cmp/blob/ae644feb7b67bf1ce4260c231d1d4300b19c6f30/lua/cmp/entry.lua#L262-L272

GitMurf avatar Oct 05 '24 22:10 GitMurf

And here is where nvim-cmp handles the detail that I was referring to in my origin post above where the full function definition is and should be pre-pended to the documentation popup. It looks like that is exactly what nvim-cmp does here too:

https://github.com/hrsh7th/nvim-cmp/blob/ae644feb7b67bf1ce4260c231d1d4300b19c6f30/lua/cmp/entry.lua#L448-L459

GitMurf avatar Oct 05 '24 23:10 GitMurf

Yes, using labelDetails in the menu seems to be more idiomatic than detail. Maybe preferring it with fallback to currently used detail is a good decision.

I think the reason labelDetails is not used in 'mini.completion' is that 3.17 version was still in development when 'mini.completion' was created.

echasnovski avatar Oct 06 '24 10:10 echasnovski

I think the reason labelDetails is not used in 'mini.completion' is that 3.17 version was still in development when 'mini.completion' was created.

Yeah that makes sense! labelDetails I recall only showed up in nvim-cmp more recently (in the last few months) so likely similar reason.

The problem with detail though is I don't believe it actually belongs in menu. As you said, it would make the menu way too wide. But also judging by the descriptions in LSP, it starts with "Human readable..." exactly like the docs property does. Point being, I still believe detail belongs in the docs popup window prepended ahead of docs.value.

Additionally the reason I think this is true is because detail doesn't even show up in initial response from LSP. it seems to typically only be returned by LSP after you <c-n/p> cycle over a choice in the completions menu, so it can't be part of the menu itself because it won't show up until you select an item, hence why I believe it goes in the docs popup.

GitMurf avatar Oct 06 '24 14:10 GitMurf

As well, label and labelDetails are next to each other in the lsp docs.

image

And then detail appears next to documentation and as mentioned above, start with a similar description "human readable..."

image

TLDR: label / labelDetails should be together in the completions menu. And documentation / detail should go together in the docs popup.

GitMurf avatar Oct 06 '24 14:10 GitMurf

Additionally the reason I think this is true is because detail doesn't even show up in initial response from LSP. it seems to typically only be returned by LSP after you <c-n/p> cycle over a choice in the completions menu, so it can't be part of the menu itself because it won't show up until you select an item, hence why I believe it goes in the docs popup.

This depends on the LSP server. clangd, for example, includes it in the initial response and contains things like char *, enum <anonymous>, etc.

So, I am afraid, it is not as black and white. I'll take a look later.

echasnovski avatar Oct 06 '24 14:10 echasnovski

So, I am afraid, it is not as black and white.

Should be the tag line for LSP generally 🤣

GitMurf avatar Oct 06 '24 14:10 GitMurf

@echasnovski to one of my earlier points, a lot of this could be solved by the user if the process_items hook also processed the additional LSP response when items are selected. Or a second hook. Because none of that passes through it so users have no way to customize that part (which for typescript is where all the detail and docs comes through).

See here: https://github.com/echasnovski/mini.nvim/issues/1258#issuecomment-2395215176

GitMurf avatar Oct 06 '24 14:10 GitMurf

@echasnovski to one of my earlier points, a lot of this could be solved by the user if the process_items hook also processed the additional LSP response when items are selected. Or a second hook. Because none of that passes through it so users have no way to customize that part (which for typescript is where all the detail and docs comes through).

See here: #1258 (comment)

Looks like what I am referring to (for clarity) is exactly what you called out in your docs / notes here:

https://github.com/echasnovski/mini.nvim/blob/61e5d46fc0cf71306c51275383767d996f559a60/lua/mini/completion.lua#L99-L106

And also similarly here:

https://github.com/echasnovski/mini.nvim/blob/61e5d46fc0cf71306c51275383767d996f559a60/lua/mini/completion.lua#L930-L934

GitMurf avatar Oct 06 '24 15:10 GitMurf

While I am digging into it, documenting for future references.

LUA

textDocument/completion

No detail or documentation in initial LSP response for the completion menu items, BUT label shows full function signature / definition from the Lua LSP.

image

image

completionItem/resolve

The documentation is returned after selecting an item in menu. Notice detail is just "function" and not the full signature. Lua LSP includes the full signature in the documentation.value.

image

image

TypeScript (vtsls / tsserver)

textDocument/completion

  • No detail or documentation in initial LSP response for the completion menu items
  • The label only shows the function / method (parse) without any details of the signature
  • Unlike Lua, for TypeScript you will only see the function/method name, which is fine as long as the docs popup (see next section below) includes the detail in the popup.
  • There is no labelDetails value in this case (value return vim.empty_dict()) but typically this would be populated with things like the auto import statement file / library it comes from (if applicable).
  • For an example of labelDetails see comment from earlier in this thread here: https://github.com/echasnovski/mini.nvim/issues/1258#issuecomment-2395218945
  • Here is how nvim-cmp handles labelDetails: https://github.com/echasnovski/mini.nvim/issues/1258#issuecomment-2395219231

image

image

completionItem/resolve

  • The documentation is returned after selecting an item in menu.
  • Notice detail has the full function signature / definition.
  • TypeScript LSP does NOT include anything about the function definition / type info in the documentation.value (hence screenshot of docs popup below provides no info about the function signature, which is a problem).
  • This is why for TypeScript the detail is needed to be prepended to the documentation.value in the docs popup.
  • Here is how nvim-cmp handles detail combined with documentation.value: https://github.com/echasnovski/mini.nvim/issues/1258#issuecomment-2395220862

image

image

GitMurf avatar Oct 06 '24 16:10 GitMurf

@echasnovski here is my hacky working fork for completions for everything discussed above: https://github.com/GitMurf/mini.nvim/commit/cc8101f390acb7dd0d68c17a973b3c54ca0f9418

One thing that is not ideal is for languages like lua, where the LSP provides details as well as includes function signature / variable type info in the documentation.value, there is a bit of redundancy in the docs popup (prepends "function" to top of docs popup). Could conditionally program around that but I didn't care enough to go any more complex than this for POC.

GitMurf avatar Oct 06 '24 20:10 GitMurf

This should (finally) be resolved on latest main.

After spending what felt like countless hours of reading LSP spec and testing different popular LSP servers, I did end up agreeing with all key parts of your arguments, @GitMurf. Huge thanks for detailed and meticulous analysis, it helped a lot!

The way 'mini.completion' now treats detail, documentation, and labelDetails is mostly similar to how 'hrsh7th/nvim-cmp' (and partially your fork) does it:

  • The detail and documentation are shown in info window. The detail is shown only if it not exactly present in documentation (something I've seen done in 'Saghen/blink.cmp') and is also wrapped into markdown code block for the language (or filetype, to be precise) of the current buffer. Both of these approaches have false positives, but seems to work as expected in vast majority of cases.
  • The labelDetails are now shown inside popup menu. There is no truncation (it can be set up by the user in the config.lsp_completion.process_items) and both detail and description fields ofo labelDetails are used (through direct concatenation, as is done in 'nvim-cmp').

All in all, together with resolved #1020 I really like how info window now looks in 'mini.completion'. There are small issues with computing dimensions and concealing characters (like for some reason present markdown links inflate window width sometimes), but they might be fixed in the future. Thanks again for raising this issue and providing useful feedback!

echasnovski avatar Feb 21 '25 18:02 echasnovski

I did end up agreeing with all key parts of your arguments, @GitMurf. Huge thanks for detailed and meticulous analysis, it helped a lot!

High praise coming from a goat like you honestly made my week!! Glad I could help :) 🎉

GitMurf avatar Feb 21 '25 23:02 GitMurf