yioneko / vtsls

LSP wrapper for typescript extension of vscode
Other
537 stars 8 forks source link

textEdit and completion filtering #210

Open kristijanhusak opened 4 weeks ago

kristijanhusak commented 4 weeks ago

Hi,

I noticed that each completion item has a textEdit, even when there are no differences in the label and inserted text.

That wouldn't be an issue, but it seems that the completion filtering depends on this information. In https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion, there is this section

Completion item provides an insertText / label without a text edit: in the model the client should filter against what the user has already typed using the word boundary rules of the language (e.g. resolving the word under the cursor position). The reason for this mode is that it makes it extremely easy for a server to implement a basic completion list and get it filtered on the client.

Completion Item with text edits: in this mode the server tells the client that it actually knows what it is doing. If you create a completion item with a text edit at the current cursor position no word guessing takes place and no automatic filtering (like with an insertText) should happen. This mode can be combined with a sort text and filter text to customize two things. If the text edit is a replace edit then the range denotes the word used for filtering. If the replace changes the text it most likely makes sense to specify a filter text to be used

By the specification, the completion item should already be pre-filtered if it has a textEdit, which is not the case at the moment. I tested with typescript-language-server and it seems it does not have textEdit on the completion items.

yioneko commented 4 weeks ago

See https://github.com/microsoft/language-server-protocol/issues/2042. From what I understand, the clarification means that the automatic filtering here refers to the client default filtering behavior based on language-specific word boundary and insertText or label field in completion item.

If the text edit is a replace edit then the range denotes the word used for filtering. If the replace changes the text it most likely makes sense to specify a filter text to be used

But when a replacement text edit is provided, the client should change the default word boundary to the replacement range and use the filterText for item filtering. To be honest, I'm still quite confused by the improved clarification in spec, but this seems the most reasonable interpretation to me and please correct me if you think I'm wrong.

kristijanhusak commented 3 weeks ago

It is indeed confusing. If you check the comments on that issue, you can see that everyone understands it differently. I noticed this issue after setting up https://github.com/neovim/neovim/pull/30945 and discussing with a Neovim maintainer.

I also think that automatic filtering refers to client side filtering, but the 2nd paragraph clearly states that the automatic filtering is not needed when there is a text edit:

Completion Item with text edits: in this mode the server tells the client that it actually knows what it is doing. If you create a completion item with a text edit at the current cursor position no word guessing takes place and no automatic filtering (like with an insertText) should happen.

For the part that you quoted:

If the text edit is a replace edit then the range denotes the word used for filtering. If the replace changes the text it most likely makes sense to specify a filter text to be used.

I think first sentence provides information how to extract what was used for filtering on the server, in case the client needs it, and 2nd sentence states that if textEdit is a ReplaceText action, the server should provide a filterText for filtering.

I think the general agreement is that filterText has precedence over all of this, so if we would have filterText here, I think we would be ok.

Besides the typescript-language-server I also tested Lua LSP, and it also does not provide textEdit. Should've probably test a few more LSPs, but I'm not sure if that information can be used as a decider here.

Edit: Tested few more LSPs:

  1. gopls - Provides both filterText and textEdit
  2. intelephense - also provides both filterText and textEdit
  3. vimls - Doesn't have either filterText or textEdit
yioneko commented 3 weeks ago

I think first sentence provides information how to extract what was used for filtering on the server, in case the client needs it, and 2nd sentence states that if textEdit is a ReplaceText action, the server should provide a filterText for filtering. I think the general agreement is that filterText has precedence over all of this, so if we would have filterText here, I think we would be ok.

If I've understood it correctly, the ambiguous case here is that the server provides textEdit without a filterText field? Then I think it is ok to always use textEdit if combined with filterText since we expect the client to use fuzzy matching for all the items. Another reason for using textEdit is that the server could only specify the range of completion item through the textEdit field according to the current version of LSP, and the wrapped vscode extension already handles the determination of word boundary and expects it to be used as replacement range when the item is accepted. Part of servers you mentioned may leave the determination of word boundary to the client in some cases and I would consider both of the approaches fine.

kristijanhusak commented 3 weeks ago

If I've understood it correctly, the ambiguous case here is that the server provides textEdit without a filterText field? Then I think it is ok to always use textEdit if combined with filterText since we expect the client to use fuzzy matching for all the items.

Yes, it is fine to provide textEdit all the time if filterText comes with it. From my understanding, client should first check filterText for filtering and fallback to other things if filterText is not provided.