From: Spencer Baugh <sbaugh@janestreet.com>
To: "João Távora" <joaotavora@gmail.com>
Cc: sbaugh@catern.com, emacs-devel@gnu.org, monnier@iro.umontreal.ca
Subject: Re: Eglot cannot work with default completion-in-region?
Date: Mon, 29 Jan 2024 16:14:06 -0500 [thread overview]
Message-ID: <iereddz7sgx.fsf@janestreet.com> (raw)
In-Reply-To: <87r0i1blch.fsf@gmail.com> ("João Távora"'s message of "Sun, 28 Jan 2024 14:09:50 +0000")
João Távora <joaotavora@gmail.com> writes:
> sbaugh@catern.com writes:
>> João Távora <joaotavora@gmail.com> writes:
>>> Is it? The above all return textEdits, I think. How can sortText,
>>> a JSON string, equal textEdit, a JSON object?
>>
>> If textEdit.range==the completion region and textEdit.newText==sortText,
>> then the textEdit is trivial and the default completion insertion
>> behavior is correct and needs no special handling. So we can skip the
>> textEdit behavior in the exit-function.
>
> Couple of points:
>
> * How do you know that is actually the "common" case? For example, it's
> not uncommon at all to have some language servers provide completions
> for symbols which also add in an '#include', or an 'import' when you
> choose it (according to each languages module system, of course).
Sure, but most completions are not going to add an import. That's why I
say this is the common case. Of course I don't actually know, so I do
agree with your argument to add more tests for some other LSP servers,
to see what their behavior is in practice.
> * Are you familiar with how LSP expresses edits? The very same edit can
> be expressed in a multitude of ways. Ultimately, the only way in
> which an edit can be determined to be trivial is by applying it and
> seeing it if was trivial.
Yes. But a one-element list with the values I described above is pretty
easy to check for.
>>>> In that case, this code is not necessary, and Eglot doesn't need an
>>>> :exit-function at all.
>>>
>>> Indeed. But how do you plan to know this in advance?
>>
>> And if for all the completion items, we see that we can skip the
>> textEdit behavior, then we don't need an :exit-function at all.
>
> Indeed "for all". So this would mean iterating the full completion list
> for non-trivial computation. Even if that's manageable, are you aware
> that some completion lists are incomplete at first and then are resolved
> to complete lists as the completion session progresses?
Ok, so maybe :exit-function is still needed, but it can be a no-op most
of the time.
> Also, I hope that you are aware that LSP fundamentally disrepects any
> Emacs completion style. Because unless you can somehow teach Elisp to
> arbitrary language servers on the spot, they really have no idea what
> "flex", "partial-completion", "emacs22" or "orderless" is.
>
> I consider this to be a much, much greater nuisance than the partial
> completion limitations that you picked to start this thread.
Of course. That is indeed a significant annoyance.
But to be somewhat provocative: elisp-completion-at-point also doesn't
know anything about completion styles, yet completion-styles still
affect completion with elisp-completion-at-point.
>>>> Can Eglot detect this and avoid this somewhat hacky code in that case?
>>>
>>> Not easily or cleanly...
>>
>> What's the issue with the approach I described above? Seems like it
>> would work?
>
> The only way to know is to try it. And I don't envision easy or clean
> stemming from it. But if you have this approach very clear in your
> head and demonstrate that:
>
> * it's fast, or fast enough
> * it significantly improves the completion experience compared to what
> it is now
> * it doesn't break any existing use cases
> * it doesn't add an unmanageable amount of complexity to the already
> complex function
>
> Then I'll be willing to consider it, of course.
I will try to meet these requirements.
> But for me, this is too tall an order. I just don't see the benefit.
>
> Anyway, as I said before, I think the starting point for this would have
> to be a systematic structured survey of how Eglot behaves currently in a
> number of language servers (including yours, of course, but not limited
> to it) and encode that as unit tests for Eglot (failing or passing).
>
> That effort is very valuable in itself and it is a prerequisite for any
> deep changes to that function. You can have a look at the exiting
> 'eglot-test-basic-completions' and 'eglot-test-non-unique-completions'
> tests. I'd welcome many more such tests.
Reasonable.
I can add some tests of completion with ocamllsp, rust-analyzer, and
pyright.
Do these tests run with actual language servers in any CI system? If
so, should I make sure that the tests I add, work in that CI system?
>>>> It should be a performance improvement and simplification anyway for all
>>>> completion frameworks.
>>>
>>> .. but you can try your hand at a patch. I'd love to be proven wrong.
>>>
>>>> (BTW, besides the issue with default completion-in-region, I also ran
>>>> into this while trying to write an OCaml-specific wrapper for
>>>> eglot-completion-at-point function which adds some additional
>>>> completions to those returned from the LSP. But if those completions
>>>> are chosen, then the Eglot exit-function breaks when it tries to look up
>>>> the insertText/textEdit. My LSP doesn't use textEdit at all, though, so
>>>> this is another unnecessary breakage)
>>>
>>> What contract exactly is eglot-completion-at-point breaking?
>>
>> No contract, my modification isn't staying within the bounds of any
>> explicit contract. So of course I shouldn't expect it to work. But
>> nevertheless I'd like to get it to work, and the exit-function is what
>> doesn't work, so I'd like to figure out a way to make it work.
>
> Maybe you can write an OCaml-server specific LSP-aware completion
> function for your OCaml major mode using Eglot's API. I think that's
> what makes the most sense. You'll be able to know for sure what the
> server does.
Maybe.
>> We could add a new extra property to completion-at-point-functions,
>> :text-function, which gets the completion item after it's been selected,
>> before text properties are stripped, and returns the actual text which
>> should be inserted into the buffer.
>
> That's something to pitch to Stefan. But that API is pretty hairy as it
> is.
>
>> That would be useful to Eglot, right? It would let Eglot avoid some
>> hacks to work around the fact that default completion-in-region strips
>> the text properties before calling exit-function.
>
> I don't know exactly what you're talking about or what this loss of
> text-propeties actually entails. If there's a bug, I'd like to know
> about it, but I should be able to reproduce it.
I mean the behavior of default completion-in-region that this code in
Eglot is working around:
;; When selecting from the *Completions*
;; buffer, `proxy' won't have any properties.
;; A lookup should fix that (github#148)
(get-text-property
0 'eglot--lsp-item
(cl-find proxy (funcall proxies) :test #'string=))
:text-function would let you delete this code since you could rely the
text properties being present.
> For things to be "useful for Eglot" they have to translate to actual
> tangible benefits for Eglot users in some server/major-mode combination
> (and no bugs)
>
> "Eglot users" here means user of:
>
> Emacs -Q some-file.xyz -f eglot
>
> Where "xyz" is potentially _any_ file type managed by _any_ LSP server,
> past or future.
>
> It doesn't mean users of:
>
> Emacs -Q -l my-special-library-or-configuration.el some-file.ocaml -f eglot
>
> That's _not_ to say the latter isn't useful for some, or even many,
> users. I just don't count that as "useful for Eglot".
Of course.
I intend the benefit to be better support for try-completion for any
file type and any LSP server. And also simplification of the code.
>> And then my wrapper around eglot-completion-at-point could just provide
>> its own text-function which calls Eglot's text-function whenever it sees
>> an Eglot completion, and runs its own logic on its own completions, and
>> avoid any breakage.
>
> I think you should try your hand at an OCaml-speciifc LSP-ware
> completion function. If the Elisp API of Eglot isn't sufficiently
> flexible, we can make it more flexible.
>
> João
eglot-completion-at-point is a 200 line quite hairy function. I don't
really want to duplicate its code. If Eglot exposed parts of eglot-cap
as individual APIs, that would work too, but I think the capf API is a
reasonable way for Eglot to do that. It's just that the current capf
API doesn't provide enough information to use eglot-cap without breaking
it.
next prev parent reply other threads:[~2024-01-29 21:14 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-26 20:38 Eglot cannot work with default completion-in-region? Spencer Baugh
2024-01-27 14:45 ` João Távora
2024-01-27 15:37 ` sbaugh
2024-01-28 10:23 ` Daniel Mendler via Emacs development discussions.
2024-01-28 14:09 ` João Távora
2024-01-29 21:14 ` Spencer Baugh [this message]
2024-01-29 22:21 ` João Távora
2024-01-29 22:51 ` JD Smith
2024-01-30 0:32 ` João Távora
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=iereddz7sgx.fsf@janestreet.com \
--to=sbaugh@janestreet.com \
--cc=emacs-devel@gnu.org \
--cc=joaotavora@gmail.com \
--cc=monnier@iro.umontreal.ca \
--cc=sbaugh@catern.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).