bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]


Hi Joao and others!

The LSP spec supports an optional command in the completion results, so
that the server can know what completion candidate was selected.  That
information can then be used by the server to score candidates for
better completion results etc.  This simple patch adds support for this.
I have no strong opinions on _where_ exactly the command should be sent,
as in before or after the didchange notification.  I've been using this
the last couple of days.

What do you think?  Any objections to me installing this sometime later?
Should it be mentioned in NEWS, or isn't it interesting enough?

Theo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Send-command-in-eglot-completion-exit-function.patch --]
[-- Type: text/x-patch, Size: 1645 bytes --]

From a7756c7f0e1ca9aeca59b496e35000651e0a5252 Mon Sep 17 00:00:00 2001
From: Theodor Thornhill <theo@thornhill.no>
Date: Tue, 14 Feb 2023 11:52:57 +0100
Subject: [PATCH] Send command in eglot completion exit-function

* lisp/progmodes/eglot.el: Destructure optional argument command.
(eglot-completion-at-point): Send command if supplied by server.
---
 lisp/progmodes/eglot.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 6caf5894ed..4526f41812 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2923,7 +2923,7 @@ eglot-completion-at-point
                                     (window-buffer (minibuffer-selected-window))
                                   (current-buffer))
              (eglot--dbind ((CompletionItem) insertTextFormat
-                            insertText textEdit additionalTextEdits label)
+                            insertText textEdit additionalTextEdits label command)
                  (funcall
                   resolve-maybe
                   (or (get-text-property 0 'eglot--lsp-item proxy)
@@ -2963,6 +2963,9 @@ eglot-completion-at-point
                  (when (cl-plusp (length additionalTextEdits))
                    (eglot--apply-text-edits additionalTextEdits)))
                (eglot--signal-textDocument/didChange)
+               (when command
+                 (eglot--dbind ((Command) command arguments) command
+                   (eglot-execute-command server (intern command) arguments)))
                (eldoc)))))))))
 
 (defun eglot--hover-info (contents &optional _range)
-- 
2.34.1

bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

[Felician Nemeth]

Hi Theo,

> The LSP spec supports an optional command in the completion results, so
> that the server can know what completion candidate was selected.  That
> information can then be used by the server to score candidates for
> better completion results etc.

Can you list some servers that send this info?  And if it is not too
much trouble can write a simple recipe where supporting this feature
actually makes a difference?  Thanks.

> This simple patch adds support for this.
> I have no strong opinions on _where_ exactly the command should be sent,
> as in before or after the didchange notification.

The specification only says: "An optional command that is executed
*after* inserting this completion."  I think it is worth asking for
clarification at https://github.com/microsoft/language-server-protocol
(I'd guess it's safer to send the command after the didChange
notification.)

bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 14 February 2023 16:56:58 CET, Felician Nemeth <felician.nemeth@gmail.com> wrote:
>Hi Theo,
>
>> The LSP spec supports an optional command in the completion results, so
>> that the server can know what completion candidate was selected.  That
>> information can then be used by the server to score candidates for
>> better completion results etc.
>
>Can you list some servers that send this info?  And if it is not too
>much trouble can write a simple recipe where supporting this feature
>actually makes a difference?  Thanks.
>

Jdtls does. See https://github.com/eclipse/eclipse.jdt.ls/blob/c4ed39a70e8c32e055a1b136ceb6c0477687a330/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTDelegateCommandHandler.java#L152

As for proving a difference, not sure how easy that is. The code around the link I posted seems to score, so I guess over time the server should learn _something_. It's a little hard to verify anyway because eglot doesn't respect the ordering provided by the server (which is a different bug in itself).

Anyway, lsp-mode, neovim and others support this, so I see no reason we shouldn't :)


>> This simple patch adds support for this.
>> I have no strong opinions on _where_ exactly the command should be sent,
>> as in before or after the didchange notification.
>
>The specification only says: "An optional command that is executed
>*after* inserting this completion."  I think it is worth asking for
>clarification at https://github.com/microsoft/language-server-protocol
>(I'd guess it's safer to send the command after the didChange
>notification.)

I agree.

Theo

bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

[João Távora]

Theodor Thornhill <theo@thornhill.no> writes:

>>Can you list some servers that send this info?  And if it is not too
>>much trouble can write a simple recipe where supporting this feature
>>actually makes a difference?  Thanks.
>
> As for proving a difference, not sure how easy that is. The code
> around the link I posted seems to score, so I guess over time the
> server should learn _something_. It's a little hard to verify anyway
> because eglot doesn't respect the ordering provided by the server
> (which is a different bug in itself).

I don't understand what you mean.  There is this code in
eglot-completion-at-point:
 
 ...
 (sort-completions
  (lambda (completions)
    (cl-sort completions
             #'string-lessp
             :key (lambda (c)
                    (or (plist-get
                         (get-text-property 0 'eglot--lsp-item c)
                         :sortText)
                        "")))))
 ...

Is it not working?  I see sensible orderings in the servers I use.  Is
there a reported bug about this?  If there isn't, please make one.

> Anyway, lsp-mode, neovim and others support this, so I see no reason
> we shouldn't :)

FWIW I don't see this as a good enough reason in itself.  If there is
little to no discernible benefit, supporting esoteric features can
become code bloat that makes maintenance of more important features
difficult.  There is a fair amount of junk in the LSP standard, or
simply stuff that doesn't make as much sense in Emacs as it does in
other editors.

Regardless, I'm not opposed to this feature if there's a simple enough
patch.  But we should weigh pros and cons -- and Felicián's request to
measure the pros is quite reasonable.

(Not to mention that a smart enough server can already derive these
smarts from the evolution of the LSP document's contents as reported by
the client.)

João

bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

João Távora <joaotavora@gmail.com> writes:

> Theodor Thornhill <theo@thornhill.no> writes:
>
>>>Can you list some servers that send this info?  And if it is not too
>>>much trouble can write a simple recipe where supporting this feature
>>>actually makes a difference?  Thanks.
>>
>> As for proving a difference, not sure how easy that is. The code
>> around the link I posted seems to score, so I guess over time the
>> server should learn _something_. It's a little hard to verify anyway
>> because eglot doesn't respect the ordering provided by the server
>> (which is a different bug in itself).
>
> I don't understand what you mean.  There is this code in
> eglot-completion-at-point:
>  
>  ...
>  (sort-completions
>   (lambda (completions)
>     (cl-sort completions
>              #'string-lessp
>              :key (lambda (c)
>                     (or (plist-get
>                          (get-text-property 0 'eglot--lsp-item c)
>                          :sortText)
>                         "")))))
>  ...
>
> Is it not working?  I see sensible orderings in the servers I use.  Is
> there a reported bug about this?  If there isn't, please make one.
>

It seems something happens with yasnippets.  I only have the symptoms
yet, but I'll make a report when I know what's happening :)

>> Anyway, lsp-mode, neovim and others support this, so I see no reason
>> we shouldn't :)
>
> FWIW I don't see this as a good enough reason in itself.  If there is
> little to no discernible benefit, supporting esoteric features can
> become code bloat that makes maintenance of more important features
> difficult.  There is a fair amount of junk in the LSP standard, or
> simply stuff that doesn't make as much sense in Emacs as it does in
> other editors.
>

Yeah, I agree.  However, this should make sense in Emacs too, no?

> Regardless, I'm not opposed to this feature if there's a simple enough
> patch.  But we should weigh pros and cons -- and Felicián's request to
> measure the pros is quite reasonable.

The patch is simple enough, it could just be its own function, to be
executed.  Then it's a one-liner in the exit-function

>
> (Not to mention that a smart enough server can already derive these
> smarts from the evolution of the LSP document's contents as reported by
> the client.)
>

Maybe - I don't think that's reason enough to dismiss the command
argument outright, though.

> João

Theo

bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Ping :-)

>>
>> FWIW I don't see this as a good enough reason in itself.  If there is
>> little to no discernible benefit, supporting esoteric features can
>> become code bloat that makes maintenance of more important features
>> difficult.  There is a fair amount of junk in the LSP standard, or
>> simply stuff that doesn't make as much sense in Emacs as it does in
>> other editors.
>>
>
> Yeah, I agree.  However, this should make sense in Emacs too, no?
>
>> Regardless, I'm not opposed to this feature if there's a simple enough
>> patch.  But we should weigh pros and cons -- and Felicián's request to
>> measure the pros is quite reasonable.
>
> The patch is simple enough, it could just be its own function, to be
> executed.  Then it's a one-liner in the exit-function
>
>>
>> (Not to mention that a smart enough server can already derive these
>> smarts from the evolution of the LSP document's contents as reported by
>> the client.)
>>
>
> Maybe - I don't think that's reason enough to dismiss the command
> argument outright, though.
>
>> João
>
> Theo

bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

On Sat, Feb 18, 2023 at 10:20 PM Theodor Thornhill <theo@thornhill.no>
wrote:

>
> Ping :-)
>
>
Presuming you meant to ping me, I'm not sure I can provide much more input
at the moment.  As I wrote, if someone is familiar with this part of the
standard
I'll be happy to review a patch adding this to Eglot.

I share with Felician a concern: if the notification can be sent from the
exit-function
it's one thing, and the patch is possibly a one or two-liner.  If, OTOH,
the notification
has to be sent after the didChange that follows a completion choice, then
it's
probably a much more complicated change.

João

Jo~ao

[-- Attachment #2: Type: text/html, Size: 1083 bytes --]

bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

João Távora <joaotavora@gmail.com> writes:

> On Sat, Feb 18, 2023 at 10:20 PM Theodor Thornhill <theo@thornhill.no> wrote:
>
>  Ping :-)
>
> Presuming you meant to ping me, I'm not sure I can provide much more input
> at the moment.  As I wrote, if someone is familiar with this part of the standard 
> I'll be happy to review a patch adding this to Eglot.  
>
> I share with Felician a concern: if the notification can be sent from the exit-function 
> it's one thing, and the patch is possibly a one or two-liner.  If, OTOH, the notification 
> has to be sent after the didChange that follows a completion choice, then it's 
> probably a much more complicated change.
>


Yeah, the patch add this only to the exit-function, which also sends a
didChange. No need to add this to anything else than the exit-function,
I believe. Not sure whether you looked at the patch, but here it is
inlined:

```
Send command in eglot completion exit-function

* lisp/progmodes/eglot.el: Destructure optional argument command.
(eglot-completion-at-point): Send command if supplied by server.

1 file changed, 4 insertions(+), 1 deletion(-)
lisp/progmodes/eglot.el | 5 ++++-

modified   lisp/progmodes/eglot.el
@@ -2925,7 +2925,7 @@ eglot-completion-at-point
                                     (window-buffer (minibuffer-selected-window))
                                   (current-buffer))
              (eglot--dbind ((CompletionItem) insertTextFormat
-                            insertText textEdit additionalTextEdits label)
+                            insertText textEdit additionalTextEdits label command)
                  (funcall
                   resolve-maybe
                   (or (get-text-property 0 'eglot--lsp-item proxy)
@@ -2965,6 +2965,9 @@ eglot-completion-at-point
                  (when (cl-plusp (length additionalTextEdits))
                    (eglot--apply-text-edits additionalTextEdits)))
                (eglot--signal-textDocument/didChange)
+               (when command
+                 (eglot--dbind ((Command) command arguments) command
+                   (eglot-execute-command server (intern command) arguments)))
                (eldoc)))))))))
 
 (defun eglot--hover-info (contents &optional _range)
```

bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

[Felician Nemeth]

>>> I have no strong opinions on _where_ exactly the command should be sent,
>>> as in before or after the didchange notification.

I've created an issue about it here:
https://github.com/microsoft/language-server-protocol/issues/1672

bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Felician Nemeth <felician.nemeth@gmail.com> writes:

>>>> I have no strong opinions on _where_ exactly the command should be sent,
>>>> as in before or after the didchange notification.
>
> I've created an issue about it here:
> https://github.com/microsoft/language-server-protocol/issues/1672

Nice, thanks!

Theo

bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function

[Felician Nemeth]

>>>> I have no strong opinions on _where_ exactly the command should be sent,
>>>> as in before or after the didchange notification.
>
> I've created an issue about it here:
> https://github.com/microsoft/language-server-protocol/issues/1672

We've got a reply.  Dirk Bäumer writes:

> Actually the command has to be dispatch on the client side since. IF
> the client then forwards it to the server that is fine. From a timing
> perspective it should arrive at the server (if at all) after
> textDocument/didChange.

I hope this is useful.  Unfortunately, I don't really remember why I was
involved in this thread.