Re: Eglot cannot work with default completion-in-region?

[Spencer Baugh <sbaugh@janestreet.com>]

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.