emacs-lsp / lsp-mode

Emacs client/library for the Language Server Protocol
https://emacs-lsp.github.io/lsp-mode
GNU General Public License v3.0
4.79k stars 889 forks source link

Seems CAPF in lsp is maybe calculating completions from the string-at-point which prevent helm updating #2142

Open thierryvolpiatto opened 4 years ago

thierryvolpiatto commented 4 years ago

I am not making a bug report as I am not sure for now how things works here in lsp, I had only a quick look at the code but it is hard to read as I don't know what all these dash.el fns do. When doing completion in lsp or anywhere else, you start with some text at point in a buffer and completion is done against this text with TAB, each time you add or remove a character to this text, you hit TAB to have completion against this new text, alright you already know this. Backend like company calculate completion automatically against this text, preventing user from hitting TAB every time the text at point change. Helm UI is quite different, it calculate completion initially from text at point but once started continue calculating completion not from text at point but now from the pattern inside the minibuffer, it is here things start not working in LSP. From my guess, it seems LSP CAPF is constantly calculating completion against the text at point (correct me if I am wrong) so the pattern entered in minibuffer by helm is ignored because the resulting completion table is constantly calculating from the initial text, as a result the helm buffer is not narrowed, you have to quit helm, enter more stuff in text at point and hit TAB again to have the helm buffer updated (See https://github.com/emacs-helm/helm/issues/2165 for more info). See documentation of completion-at-point-functions:

NOTE: These functions should be cheap to run since they're sometimes
run from `post-command-hook'; and they should ideally only choose
which kind of completion table to use, and not pre-filter it based
on the current text between START and END (e.g., they should not
obey `completion-styles').

I will try to give another try to LSP as soon as possible and look more carefully at the code. I think things are working correctly on the helm side (completion-at-point works fine actually almost everywhere).

thierryvolpiatto commented 4 years ago

This in lsp-completion-at-point is wrong IMO:

(apply #'lsp--capf-filter-candidates
                      pred
                      (buffer-substring-no-properties (or start-point (point)) (point))
                      (cdr lsp--capf-cache))

Don't know what lsp--capf-filter-candidates does exactly but IMHO it should not filter anything and at least it should not use the text at point i.e. (buffer-substring-no-properties...).

yyoncho commented 4 years ago

There are multiple issues related to Language Server Protocol completion being a poor fit for emacs completion system:

  1. The filtering might not occur against the display text(the label). There is additional property in each candidate filterText which when present should be used for filtering.
  2. The prefix (corresponds to company-prefix and related) is not known until we retrieve the candidates from the server.
  3. Each candidate can have different prefix depending on its textEdit. This implies lsp-mode using buffer-substring to filter the candidates. For example, you may have:
foo.bar

and 2 candidates, one with prefix .bar and one with prefix bar. Both will have textEdit with different range.

Based on your report I think that 3 is breaking helm but I am not sure what would be the correct fix. The only way to make that work seems to be helm to insert the char into the buffer not only in the helm mini-buffer.

thierryvolpiatto commented 4 years ago

Ivan Yonchovski notifications@github.com writes:

and 2 candidates, one with prefix .bar and one with prefix bar. Both will have textEdit with different range.

Don't know what textEdit is, I am testing here with python with completion on e.g. os.path.

Based on your report I think that 3 is breaking helm but I am not sure what would be the correct fix.

I guess the correct fix is to constantly pass the new string to the server, don't know how though as I don't know how works lsp.

The only way to make that work seems to be helm to insert the char into the buffer not only in the helm mini-buffer.

No, this is not the right way to fix this, the capf should not use buffer-substring to filter candidates, sly and possibly eglot (not sure for this one) are creating a new style called backend to solve this issue but I don't know much about it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.*

-- Thierry

yyoncho commented 4 years ago

Don't know what textEdit is, I am testing here with python with completion on e.g. os.path.

Testing with one server is not enough. There are maybe 10+ different combinations of completion behavior that has to be handled and for which @kiennq has spent like an year in order to be able to reproduce the VScode completion behavior and we have maybe 30+ discussions here, in gitter, and in LSP protocol repo regarding the right way to do the completion. textEdit narrows down to (:start-point :end-point :new-text). :start-point is a reference to buffer position. So, we should use the buffer to get the text to filter against when it is present.

I guess the correct fix is to constantly pass the new string to the server, don't know how though as I don't know how works lsp.

LSP server needs the char to be inserted in the buffer in order to perform completion. Note that the server might provide partial results in the initial completion and subsequent more precise one happen based on the buffer content.

No, this is not the right way to fix this, the capf should not use buffer-substring to filter candidates, sly and possibly eglot (not sure for this one) are creating a new style called backend to solve this issue but I don't know much about it.

This is how the protocol works, I am not sure what your suggestion is.

thierryvolpiatto commented 4 years ago

Ivan Yonchovski notifications@github.com writes:

Don't know what textEdit is, I am testing here with python with completion on
e.g. os.path.

Testing with one server is not enough. There are maybe 10+ different combinations of completion behavior that has to be handled and for which @kiennq has spent like an year in order to be able to reproduce the VScode completion behavior and we have maybe 30+ discussions here, in gitter, and in LSP protocol repo regarding the right way to do the completion. textEdit narrows down to (:start-point :end-point :new-text). :start-point is a reference to buffer position. So, we should use the buffer to get the text to filter against when it is present.

I guess the correct fix is to constantly pass the new string to the server,
don't know how though as I don't know how works lsp.

LSP server needs the char to be inserted in the buffer in order to perform completion. Note that the server might provide partial results in the initial completion and subsequent more precise one happen based on the buffer content.

Hmm, I am sure something better can be done, unfortunately I don't understand enough lsp. So for now if user keep the default settings she will have a helm buffer that doesn't change when you modify pattern, as a workaround, one can configure helm like this for the mode where you want lsp completion, here e.g. for python-mode:

(add-to-list 'helm-completion-styles-alist '(python-mode . helm-fuzzy))

With this the completion is computed by lsp according to text at point and then helm is handling this set of candidates with its matching functions so that user can narrow down its helm buffer.

NOTE: You can use either helm-fuzzy or helm in the example above.

-- Thierry

kiennq commented 4 years ago

lsp-completion-at-point function in lsp-mode is different from normal capf in order to fit with language server protocol.

Note that the logic for sorting in lsp-completion-at-point is special since it also incorporate server returns information (:score multiplier) to decide the sort score for each item.

@thierryvolpiatto Is it possible to helm to insert both in minibuffer and current buffer? Since there're servers (like gopls, clangd ...) that intelligently returns only a small number of completion items each key strokes to prevent server and client overloading. After a few key strokes, when the completed number of completion items are small enough, the complete list will be sent to client, and client can cache and handling filtering without server response.

thierryvolpiatto commented 4 years ago

kiennq notifications@github.com writes:

lsp-completion-at-point function in lsp-mode is different from normal capf in order to fit with language server protocol.

• It sends symbol-at-point range as bogus (start end) which is required by capf. We need to do this since LSP doesn't care about range of text requested by client, it only care about the buffer content itself and the cursor position. With that language server can infer the text needed to be completed and send the candidates back to client. The actually (start end) will be inferred by lsp-mode from server return, and I think this is a great way to comply with language server, which should have more knowledge about language itself than whatever Emacs can infer from that major mode. • As the consequent of previous logical choice, since the range that quickly returned from lsp-completion-at-point is bogus, we have to handle the filtering ourselves (not respecting completion-styles). Else the caller of completion-at-point will mostly use begin, end to filtering and can over/under filtering completion items. Another option could be define new completion style for lsp-capf completion category, but it will be more work and provide no real value compare with the current filter right in lsp-completion-at-point, which ignores passed probe string.

Note that the logic for sorting in lsp-completion-at-point is special since it also incorporate server returns information (:score multiplier) to decide the sort score for each item.

Thanks for explanations.

@thierryvolpiatto Is it possible to helm to insert both in minibuffer and current buffer?

No.

Since there're servers (like gopls, clangd ...) that intelligently returns only a small number of completion items each key strokes to prevent server and client overloading. After a few key strokes, when the completed number of completion items are small enough, the complete list will be sent to client, and client can cache and handling filtering without server response.

This is what does

(add-to-list 'helm-completion-styles-alist '(python-mode . helm-fuzzy))

You can add here any mode that is handled by lsp, if you want fuzzy match use helm-fuzzy otherwise helm.

-- Thierry

yyoncho commented 4 years ago

(add-to-list 'helm-completion-styles-alist '(python-mode . helm-fuzzy))

I guess we can use that and document that it will work only on the initial set of completion items(and most likely when there is prefix mismatch). It is better than what we currently have.