eglot-completion-at-point: Follow specification more closely #235
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a WIP. It seems eglot assumed that the triggerChacters couldn't be inside the completion bounds. This assumption does not hold with the vscode-json-languageserver. So the patched version of eglot-completion-at-point asks the server for possible completions and calculates the bounds from them (if the completions are consistent in this regard.) Additionally, vscode-json-languageserver returns completions with different filterText and label fields. The LSP specification is usually quite vague, but not this time: filterText: A string that should be used when filtering a set of completion items. When `falsy` the label is used. Although it never defines what it means by filtering. Anyway, the new test `json-basic` succeeds when the patch is applied, and fails otherwise. It assumes the server installed as root, e.g.: `sudo npm install -g vscode-json-languageserver` Unfortunately, the code needs polishing. eglot calls textDocument/completion twice at the beginning of a completion. But I don't understand why it is necessary to fetch the completions from the server more than once, so I haven't modified this part of the code. I haven't run the rust and the eclipse tests, but hopefully those are unaffected by this change. Feedback on how to proceed is obviously welcome. Thanks.
|
Travis says: So I need to install pyls :( |
Handle the case when there's no textEdit in the textDocument/completion reply.
|
I fixed the test named basic-completions. The failure with other one seems to predate this PR. Nevertheless, I modified the hover-after-completions test, because it stuck in an infinite loop in case of failure. |
|
@nemethf thanks for working on this. I haven't looked at it closely since I am extremely busy these weeks but these improvements (as well as the improvements to the test's stability) are greatly appreciated, especially since you've already proven you know your stuff :-) |
Closed
This would be a bug, if indeed it still happens. I vaguely remember fixing some related situation. |
|
Probably, I wasn't clear enough. This PR is not finished partially because eglot calls textDocument/completion twice after applying the proposed patch. However, it demonstrates a problem of the current code and shows a way to fix it. Hopefully. I haven't finished the PR mainly because I don't understand when and how emacs calls textDocument/completion. |
|
NPM packages can be installed locally under $HOME folder. Just add it $PATH.
|
|
Thanks @eabarbosa, I didn't know that. (I know almost nothing about modern javascript.) |
|
Thanks @eabarbosa, I didn't know that. (I know almost nothing about modern javascript.) Somehow, I commented the same thing twice, sorry :( |
|
Hi @nemethf, I'm going through these old PR's and cherry-picking interesting bits (sorry for the very long delay). This is one is promising and I think I understand how it works. I'm going to try and rebase it to the latest |
joaotavora
added a commit
that referenced
this pull request
Oct 16, 2019
Reworked important parts of eglot-completion-at-point. One of the tasks was to cleanup the nomenclature so it's easier to spot how LSP and Emacs's views of completion techniques differ. When reading this rather long function, remember an "item" is a plist representing the LSP completionItem object, and "proxy" is a propertized string that Emacs's frontends will use to represent that completion. When the completion is close to done, the :exit-function is called, to potentially rework the inserted text so that the final result might be quite different from the proxy (it might be a snippet, or even a suprising text edit). The most important change in this commit reworks the way the completion "bounds" are calculated in the buffer. This is the region that Emacs needs to know that is being targeted for the completion. A server can specify this region by using textEdit-based completions all consistently pointing to the same range. If it does so, Emacs will use that region instead of its own understanding of symbol boundaries (provided by thingatpt.el and syntax tables). To implement server-side completion filtering, the server can also provide a filterText "cookie" in each completion, which, when prefix-matched to the intended region, selects or rejects the completion. Given the feedback in microsoft/language-server-protocol#651, we have no choice but to play along with that inneficient and grotesque strategy to implement flex-style matching. Like ever in LSP, we do so while being backward-compatible to all previously supported behaviour. * eglot.el (eglot-completion-at-point): rework.
I tried to create something similar to my old test environment and ran some tests with the recent master branch and vscode-json-languageserver. Completion works with company-mode, but it does not work with C-M-i.
I created a docker image and I use eglot-x to connect its exposed 8080 port. Additionally, I need to tweak eglot-workspace-configuration. Let me know if you interested in running your own tests and I share all the details. (It is unfortunate that I cannot devote more time to eglot now when you are more active here.) |
|
Felicián Németh <notifications@github.com> writes:
I tried to create something similar to my old test environment and ran
some tests with the recent master branch and
vscode-json-languageserver. Completion works with company-mode, but it
does not work with C-M-i.
In all servers, or just vscode-json-languageserver?
Do you still have this LSP installed so you can re-test with your new
test?
I created a docker image and I use eglot-x to connect its exposed 8080
port. Additionally, I need to tweak eglot-workspace-configuration. Let
me know if you interested in running your own tests and I share all
the details.
(It is unfortunate that I cannot devote more time to eglot now when
you are more active here.)
My own recent push won't last long, unfortunately :-) But we'll get by,
I think. Soon I'll start negotiating for Eglot, or at least parts of
it, to go into Emacs, I remember there was some openness to it some time
ago.
João
|
An older pyls works in a similar situation, but vscode-json-languageserver silently fails if there's only one candidate with "No match". However, if I want to complete foo, which has to candidates (foo.bar and foo.foo), I get the following backtrace: This is the relevant LSP message:
That's interesting. I might be short-sighted here, but I don't see immediate benefits for eglot to be in Emacs instead of GNU ELPA. But it is an advantage if core developers start to enhance it. |
|
But, to clarify: the pyls situation is fine, it is only
Instead of
That is also a huge advantage. |
Yes, that's right. Sorry I wasn't clear enough. |
bhankas
pushed a commit
to bhankas/emacs
that referenced
this pull request
Sep 18, 2022
Reworked important parts of eglot-completion-at-point. One of the tasks was to cleanup the nomenclature so it's easier to spot how LSP and Emacs's views of completion techniques differ. When reading this rather long function, remember an "item" is a plist representing the LSP completionItem object, and "proxy" is a propertized string that Emacs's frontends will use to represent that completion. When the completion is close to done, the :exit-function is called, to potentially rework the inserted text so that the final result might be quite different from the proxy (it might be a snippet, or even a suprising text edit). The most important change in this commit reworks the way the completion "bounds" are calculated in the buffer. This is the region that Emacs needs to know that is being targeted for the completion. A server can specify this region by using textEdit-based completions all consistently pointing to the same range. If it does so, Emacs will use that region instead of its own understanding of symbol boundaries (provided by thingatpt.el and syntax tables). To implement server-side completion filtering, the server can also provide a filterText "cookie" in each completion, which, when prefix-matched to the intended region, selects or rejects the completion. Given the feedback in microsoft/language-server-protocol#651, we have no choice but to play along with that inneficient and grotesque strategy to implement flex-style matching. Like ever in LSP, we do so while being backward-compatible to all previously supported behaviour. * eglot.el (eglot-completion-at-point): rework.
bhankas
pushed a commit
to bhankas/emacs
that referenced
this pull request
Sep 19, 2022
Reworked important parts of eglot-completion-at-point. One of the tasks was to cleanup the nomenclature so it's easier to spot how LSP and Emacs's views of completion techniques differ. When reading this rather long function, remember an "item" is a plist representing the LSP completionItem object, and "proxy" is a propertized string that Emacs's frontends will use to represent that completion. When the completion is close to done, the :exit-function is called, to potentially rework the inserted text so that the final result might be quite different from the proxy (it might be a snippet, or even a suprising text edit). The most important change in this commit reworks the way the completion "bounds" are calculated in the buffer. This is the region that Emacs needs to know that is being targeted for the completion. A server can specify this region by using textEdit-based completions all consistently pointing to the same range. If it does so, Emacs will use that region instead of its own understanding of symbol boundaries (provided by thingatpt.el and syntax tables). To implement server-side completion filtering, the server can also provide a filterText "cookie" in each completion, which, when prefix-matched to the intended region, selects or rejects the completion. Given the feedback in microsoft/language-server-protocol#651, we have no choice but to play along with that inneficient and grotesque strategy to implement flex-style matching. Like ever in LSP, we do so while being backward-compatible to all previously supported behaviour. * eglot.el (eglot-completion-at-point): rework.
bhankas
pushed a commit
to bhankas/emacs
that referenced
this pull request
Sep 19, 2022
Reworked important parts of eglot-completion-at-point. One of the tasks was to cleanup the nomenclature so it's easier to spot how LSP and Emacs's views of completion techniques differ. When reading this rather long function, remember an "item" is a plist representing the LSP completionItem object, and "proxy" is a propertized string that Emacs's frontends will use to represent that completion. When the completion is close to done, the :exit-function is called, to potentially rework the inserted text so that the final result might be quite different from the proxy (it might be a snippet, or even a suprising text edit). The most important change in this commit reworks the way the completion "bounds" are calculated in the buffer. This is the region that Emacs needs to know that is being targeted for the completion. A server can specify this region by using textEdit-based completions all consistently pointing to the same range. If it does so, Emacs will use that region instead of its own understanding of symbol boundaries (provided by thingatpt.el and syntax tables). To implement server-side completion filtering, the server can also provide a filterText "cookie" in each completion, which, when prefix-matched to the intended region, selects or rejects the completion. Given the feedback in microsoft/language-server-protocol#651, we have no choice but to play along with that inneficient and grotesque strategy to implement flex-style matching. Like ever in LSP, we do so while being backward-compatible to all previously supported behaviour. * eglot.el (eglot-completion-at-point): rework. #235: joaotavora/eglot#235
jollaitbot
pushed a commit
to sailfishos-mirror/emacs
that referenced
this pull request
Oct 12, 2022
Reworked important parts of eglot-completion-at-point. One of the tasks was to cleanup the nomenclature so it's easier to spot how LSP and Emacs's views of completion techniques differ. When reading this rather long function, remember an "item" is a plist representing the LSP completionItem object, and "proxy" is a propertized string that Emacs's frontends will use to represent that completion. When the completion is close to done, the :exit-function is called, to potentially rework the inserted text so that the final result might be quite different from the proxy (it might be a snippet, or even a suprising text edit). The most important change in this commit reworks the way the completion "bounds" are calculated in the buffer. This is the region that Emacs needs to know that is being targeted for the completion. A server can specify this region by using textEdit-based completions all consistently pointing to the same range. If it does so, Emacs will use that region instead of its own understanding of symbol boundaries (provided by thingatpt.el and syntax tables). To implement server-side completion filtering, the server can also provide a filterText "cookie" in each completion, which, when prefix-matched to the intended region, selects or rejects the completion. Given the feedback in microsoft/language-server-protocol#651, we have no choice but to play along with that inneficient and grotesque strategy to implement flex-style matching. Like ever in LSP, we do so while being backward-compatible to all previously supported behaviour. * eglot.el (eglot-completion-at-point): rework. GitHub-reference: close joaotavora/eglot#235
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a WIP.
It seems eglot assumed that the triggerChacters couldn't be inside the
completion bounds. This assumption does not hold with the
vscode-json-languageserver. So the patched version of
eglot-completion-at-point asks the server for possible completions and
calculates the bounds from them (if the completions are consistent in
this regard.)
Additionally, vscode-json-languageserver returns completions with
different filterText and label fields. The LSP specification is
usually quite vague, but not this time:
Although it never defines what it means by filtering. Anyway, the new
test
json-basicsucceeds when the patch is applied, and failsotherwise. It assumes the server installed as root, e.g.:
Unfortunately, the code needs polishing. eglot calls
textDocument/completion twice at the beginning of a completion. But I
don't understand why it is necessary to fetch the completions from the
server more than once, so I haven't modified this part of the code.
I haven't run the rust and the eclipse tests, but hopefully those are
unaffected by this change.
Feedback on how to proceed is obviously welcome. Thanks.