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

Previous Next

To reply to this bug, email your comments to 61506 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH]: Send command in eglot completion exit-function
Date: Tue, 14 Feb 2023 12:00:21 +0100

[Message part 1 (text/plain, inline)]

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

[0001-Send-command-in-eglot-completion-exit-function.patch (text/x-patch, attachment)]

Message #8 received at 61506 <at> debbugs.gnu.org (full text, mbox):

From: Felician Nemeth <felician.nemeth <at> gmail.com>
To: 61506 <at> debbugs.gnu.org
Cc: Theodor Thornhill <theo <at> thornhill.no>, joaotavora <at> gmail.com
Subject: Re: bug#61506: 30.0.50; [PATCH]: Send command in eglot completion
 exit-function
Date: Tue, 14 Feb 2023 16:56:58 +0100

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.)

Message #11 received at 61506 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: Felician Nemeth <felician.nemeth <at> gmail.com>, 61506 <at> debbugs.gnu.org
Cc: joaotavora <at> gmail.com
Subject: Re: bug#61506: 30.0.50; [PATCH]: Send command in eglot completion exit-function
Date: Tue, 14 Feb 2023 18:44:01 +0100

On 14 February 2023 16:56:58 CET, Felician Nemeth <felician.nemeth <at> 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

Message #14 received at 61506 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: Felician Nemeth <felician.nemeth <at> gmail.com>, 61506 <at> debbugs.gnu.org
Subject: Re: bug#61506: 30.0.50; [PATCH]: Send command in eglot completion
 exit-function
Date: Tue, 14 Feb 2023 23:49:35 +0000

Theodor Thornhill <theo <at> 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

Message #17 received at 61506 <at> debbugs.gnu.org (full text, mbox):

From: Felician Nemeth <felician.nemeth <at> gmail.com>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: 61506 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#61506: 30.0.50; [PATCH]: Send command in eglot completion
 exit-function
Date: Wed, 15 Feb 2023 12:51:23 +0100

>>> 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

Message #20 received at 61506 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: Felician Nemeth <felician.nemeth <at> gmail.com>
Cc: 61506 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#61506: 30.0.50; [PATCH]: Send command in eglot completion
 exit-function
Date: Wed, 15 Feb 2023 13:24:02 +0100

Felician Nemeth <felician.nemeth <at> 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

Message #23 received at 61506 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: João Távora <joaotavora <at> gmail.com>
Cc: Felician Nemeth <felician.nemeth <at> gmail.com>, 61506 <at> debbugs.gnu.org
Subject: Re: bug#61506: 30.0.50; [PATCH]: Send command in eglot completion
 exit-function
Date: Wed, 15 Feb 2023 13:34:32 +0100

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

> Theodor Thornhill <theo <at> 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

Message #26 received at 61506 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: João Távora <joaotavora <at> gmail.com>
Cc: Felician Nemeth <felician.nemeth <at> gmail.com>, 61506 <at> debbugs.gnu.org
Subject: Re: bug#61506: 30.0.50; [PATCH]: Send command in eglot completion
 exit-function
Date: Sat, 18 Feb 2023 23:20:00 +0100

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

Message #29 received at 61506 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: Felician Nemeth <felician.nemeth <at> gmail.com>, 61506 <at> debbugs.gnu.org
Subject: Re: bug#61506: 30.0.50;
 [PATCH]: Send command in eglot completion exit-function
Date: Sat, 18 Feb 2023 23:23:41 +0000

[Message part 1 (text/plain, inline)]

On Sat, Feb 18, 2023 at 10:20 PM Theodor Thornhill <theo <at> 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

[Message part 2 (text/html, inline)]

Message #32 received at 61506 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: João Távora <joaotavora <at> gmail.com>
Cc: Felician Nemeth <felician.nemeth <at> gmail.com>, 61506 <at> debbugs.gnu.org
Subject: Re: bug#61506: 30.0.50; [PATCH]: Send command in eglot completion
 exit-function
Date: Mon, 20 Feb 2023 15:13:36 +0100

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

> On Sat, Feb 18, 2023 at 10:20 PM Theodor Thornhill <theo <at> 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)
```

Message #37 received at 61506 <at> debbugs.gnu.org (full text, mbox):

From: Felician Nemeth <felician.nemeth <at> gmail.com>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: 61506 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#61506: 30.0.50; [PATCH]: Send command in eglot completion
 exit-function
Date: Mon, 11 Dec 2023 16:12:18 +0100

>>>> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.