8000 feat: extend complete_add function by glepnir · Pull Request #16662 · vim/vim · GitHub
[go: up one dir, main page]
Skip to content

feat: extend complete_add function #16662

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

feat: extend complete_add function #16662

wants to merge 1 commit into from

Conversation

glepnir
Copy link
Member
@glepnir glepnir commented Feb 18, 2025

complete_add support adding matches to list of current completion

@glepnir glepnir marked this pull request as draft February 18, 2025 13:04
@glepnir glepnir mentioned this pull request Mar 10, 2025
Open
@glepnir glepnir force-pushed the complete_append branch 10 times, most recently from adf2150 to 3d21256 Compare March 11, 2025 10:01
@girishji
Copy link
Contributor
girishji commented Mar 14, 2025 •
edited
Loading

What is the benefit of this over calling complete() again with longer list? complete_add() needs to redraw the screen anyway and will cause the same type of flicker as complete(), no? Also, if list gets smaller, there is no complementary complete-remove().

@glepnir
Copy link
Member Author
glepnir commented Mar 14, 2025 •
edited
Loading

Calling complete again will not redraw? Complete will also call ins_compl_del_pum and then rebuild pum. LSP allows multiple clients to handle completion requests. Currently complete_add will use del_pum false to prevent ins_compl_add from deleting pum. You can check the linked issue to understand the problem. compelte_remove is also needed, but I want to confirm the behavior of PR first

@glepnir glepnir marked this pull request as ready for review March 14, 2025 12:32
andres290487 referenced this pull request Mar 14, 2025
@64-bitman
add more tests
fa37bb8
@chrisbra
Copy link
Member

What is this purpose of this PR?

@glepnir
Copy link
Member Author
glepnir commented Mar 17, 2025 •
edited
Loading

  1. First, the LSP specification includes an isIncomplete flag to indicate whether the returned completion results are complete. If the results are incomplete, the next input will still trigger a completion request that returns additional results, which need to be merged with the previous ones. However, if the complete function is called again, the previous matches will be cleared.

  2. Second, a buffer may be associated with multiple LSP servers. When a completion request is triggered, the results from multiple servers will not be returned simultaneously, so it is necessary to append the matches.

ref https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionList

@girishji
Copy link
Contributor
girishji commented Mar 17, 2025 •
edited
Loading

Shouldn't LSP Clients cache incomplete results in both scenarios?

  1. When 'isIncomplete' is set to true, the LSP specification does not specify whether the next response should be incremental or a full replacement. LSP clients need to handle both cases.

  2. An LSP client often aggregates results from multiple LSP servers and other sources, such as buffer searches. It may also provide an omnifunc interface. In all these cases, properly handling and presenting partial results requires caching intermediate results.

I would expect modern LSP clients to have already solved this issue. You may verify this with the authors.

@glepnir
Copy link
Member Author
glepnir commented Mar 17, 2025 •
edited
Loading

How to cache when the characters are different? . complete_add exists to extend matches but is currently too limited. linked issue is sufficient to explain the problem

@girishji
Copy link
Contributor
girishji commented Mar 17, 2025 •
edited
Loading

I read the issue you linked, but it is not clear.

If the LSP client can choose to append to the existing match list instead of replacing it entirely, it can always use complete_info() to retrieve all matches, modify the list (append/insert/delete), and then call complete() again. This approach offers greater flexibility.

The issue you linked looks like a bug in LSP client implementation in neovim. client2 should be triggered on ab, even if b is not a trigger character for client2. When gathering completion list from multiple servers, maybe the correct approach is to trigger all servers for all trigger chars (even if specific trigger char is not associated with a server) and let the server decide.

@glepnir glepnir force-pushed the complete_append branch 2 times, most recently from d3a93a6 to 84cd15d Compare March 17, 2025 12:26
@glepnir
Copy link
Member Author
glepnir commented Mar 17, 2025 •
edited
Loading

it can always use complete_info() to retrieve all matches, modify the list (append/insert/delete), and then call complete() again. This approach offers greater flexibility.

It is feasible. but the time complexity here increases with the number of items. Secondly, when the merge process is completed, calling complete again on the C side still requires memory allocation. Isn't this more wasteful? and calling compelte_info also requires memory allocation.

@glepnir
feat: extend complete_add function
e3e3c92 
complete_add support adding matches to list of current completion
@chrisbra
Copy link
Member

Hm, I am still not sure about this. The use case seems quite niche and a user reading the help file, won't be necessarily clear when to use complete() instead of complete_add(..., 1). Also having to specify the completion column seems a bit restrictive, no? Or other way around, how would the completion engine know what column to use here?

@girishji
Copy link
Contributor

Hm, I am still not sure about this. The use case seems quite niche and a user reading the help file, won't be necessarily clear when to use complete() instead of complete_add(..., 1). Also having to specify the completion column seems a bit restrictive, no? Or other way around, how would the completion engine know what column to use here?

The documentation states: “The {startcol} must match the current completion column.” In practice, startcol serves more as a placeholder to differentiate between two complete_add() signatures, rather than being meaningfully used. The two signatures serve entirely different purposes — one adds entries to the list before it is displayed, while the other appends to a list that is already visible to the user. There are other issues as well, as I explained above. I'm unsure why it's still being pursued in its current form.

@chrisbra
Copy link
Member

I tend to agree

@glepnir
Copy link
Member Author
glepnir commented Apr 16, 2025 •
edited
Loading

client2 should be triggered on ab

I think you didn't see the problem clearly. b triggers textDocumention/completion request again because isIncomplete, how to merge the new result with all existing results. Your approach is to get complete_items from complete_info and then get lsp from user_data structure and then merge. How much memory allocation efficiency is there here. This is not a niche problem. There will be many lsp clients in one buffer.

@glepnir
Copy link
Member Author
glepnir commented Apr 16, 2025 •
edited
Loading

This PR appends the new request results and updates pum, complete_add needs to specify an existing startcol to ensure that it is the same as the current. Or the complete function returns an id and uses this id as the remaining book.

@glepnir glepnir marked this pull request as draft April 16, 2025 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@glepnir @girishji @chrisbra
0