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

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

Open
3 tasks done
GitMurf opened this issue Oct 5, 2024 · 23 comments
Labels
feature-request Request for a feature to existing module mini.completion

Comments

@GitMurf
Copy link

GitMurf commented Oct 5, 2024

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 GitMurf added the bug Something isn't working label Oct 5, 2024
@GitMurf
Copy link
Author

GitMurf commented Oct 5, 2024

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

@echasnovski echasnovski added feature-request Request for a feature to existing module mini.completion and removed bug Something isn't working labels Oct 5, 2024
@echasnovski
Copy link
Owner

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

@GitMurf
Copy link
Author

GitMurf commented Oct 5, 2024

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

GitMurf commented Oct 5, 2024

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

GitMurf commented Oct 5, 2024

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

@echasnovski
Copy link
Owner

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

@GitMurf
Copy link
Author

GitMurf commented Oct 5, 2024

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

GitMurf commented Oct 5, 2024

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

@echasnovski
Copy link
Owner

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

@GitMurf
Copy link
Author

GitMurf commented Oct 5, 2024

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

GitMurf commented Oct 5, 2024

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.

-- Finally, try request to resolve current completion to add documentation
local bufnr = vim.api.nvim_get_current_buf()
local params = lsp_completion_item
local current_id = H.info.lsp.id + 1
H.info.lsp.id = current_id
H.info.lsp.status = 'sent'
local cancel_fun = vim.lsp.buf_request_all(bufnr, 'completionItem/resolve', params, function(result)
-- Don't do anything if there is other LSP request in action
if not H.is_lsp_current(H.info, current_id) then return end
H.info.lsp.status = 'received'
-- Don't do anything if completion item was changed
if H.info.id ~= info_id then return end
H.info.lsp.result = result
H.show_info_window()

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

GitMurf commented Oct 5, 2024

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

GitMurf commented Oct 5, 2024

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

GitMurf commented Oct 5, 2024

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

@echasnovski
Copy link
Owner

echasnovski commented Oct 6, 2024

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.

@GitMurf
Copy link
Author

GitMurf commented Oct 6, 2024

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

GitMurf commented Oct 6, 2024

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.

@echasnovski
Copy link
Owner

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.

@GitMurf
Copy link
Author

GitMurf commented Oct 6, 2024

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

Should be the tag line for LSP generally 🤣

@GitMurf
Copy link
Author

GitMurf commented Oct 6, 2024

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

@GitMurf
Copy link
Author

GitMurf commented Oct 6, 2024

@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:

--- - Support of `additionalTextEdits` tries to handle both types of servers:
--- - When `additionalTextEdits` are supplied in response to
--- 'textDocument/completion' request (like currently in 'pyright').
--- - When `additionalTextEdits` are supplied in response to
--- 'completionItem/resolve' request (like currently in
--- 'typescript-language-server'). In this case to apply edits user needs
--- to trigger such request, i.e. select completion item and wait for
--- `MiniCompletion.config.delay.info` time plus server response time.

And also similarly here:

-- Try to get `additionalTextEdits`. First from 'completionItem/resolve';
-- then - from selected item. The reason for this is inconsistency in how
-- servers provide `additionTextEdits`: on 'textDocument/completion' or
-- 'completionItem/resolve'.
local resolve_data = H.process_lsp_response(H.info.lsp.result, function(response, client_id)

@GitMurf
Copy link
Author

GitMurf commented Oct 6, 2024

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

image

image

completionItem/resolve

image

image

@GitMurf
Copy link
Author

GitMurf commented Oct 6, 2024

@echasnovski here is my hacky working fork for completions for everything discussed above: GitMurf@cc8101f

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for a feature to existing module mini.completion
Projects
None yet
Development

No branches or pull requests

2 participants