GNU bug report logs - #71642
30.0.50; eglot-execute doesn't support ExecuteCommandParams

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 71642 in the body.
You can then email your comments to 71642 AT debbugs.gnu.org in the normal way.

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

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

From: Troy Brown <brownts <at> troybrown.dev>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; eglot-execute doesn't support ExecuteCommandParams
Date: Tue, 18 Jun 2024 23:24:04 -0400

With the deprecation of eglot-execute-command and its referral to
eglot-execute, it would seem that eglot-execute should support the
same command and arguments to send the workspace/executeCommand
request to the server.  Unfortunately, this doesn't seem to work as
expected.  I would have expected the following to work:

(eglot-execute (eglot-current-server) '(:command "my-command"))

The reason this doesn't work is because within eglot-execute, the use
of eglot--dcase is configured to only match Command or CodeAction.
Command requires that there be a "title" parameter, but this doesn't
exist in the workspace/executeCommand request.  In fact, if the above
"eglot-execute" example is changed to add a ":title" parameter, it
will erroneously send a workspace/executeCommand with a title
parameter.

The following example can be used to illustrate that the Command
interface (which is intended for code actions, not for sending an
executeCommand) doesn't match for this simple case:

(condition-case err
    (eglot--dcase '(:command "my-command")
      (((Command)) (message "Matched command")))
  (error
   (message "%s" (cdr err))))

It appears that an ExecuteCommandParams interface should be added to
eglot--lsp-interface-alist and that a matcher for ExecuteCommandParams
should be added to the eglot--dcase in eglot-execute.  Additionally,
the Command matcher should be changed to not send the "title"
parameter in the subsequent workspace/executeCommand request.

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

From: Jeremy Bryant <jb <at> jeremybryant.net>
To: Troy Brown <brownts <at> troybrown.dev>
Cc: 71642 <at> debbugs.gnu.org,
 João Távora <joaotavora <at> gmail.com>
Subject: Re: bug#71642: 30.0.50; eglot-execute doesn't support
 ExecuteCommandParams
Date: Wed, 19 Jun 2024 22:19:17 +0100

Troy Brown <brownts <at> troybrown.dev> writes:

> With the deprecation of eglot-execute-command and its referral to
> eglot-execute, it would seem that eglot-execute should support the
> same command and arguments to send the workspace/executeCommand
> request to the server.  Unfortunately, this doesn't seem to work as
> expected.  I would have expected the following to work:
>
> (eglot-execute (eglot-current-server) '(:command "my-command"))
>
> The reason this doesn't work is because within eglot-execute, the use
> of eglot--dcase is configured to only match Command or CodeAction.
> Command requires that there be a "title" parameter, but this doesn't
> exist in the workspace/executeCommand request.  In fact, if the above
> "eglot-execute" example is changed to add a ":title" parameter, it
> will erroneously send a workspace/executeCommand with a title
> parameter.
>
> The following example can be used to illustrate that the Command
> interface (which is intended for code actions, not for sending an
> executeCommand) doesn't match for this simple case:
>
> (condition-case err
>     (eglot--dcase '(:command "my-command")
>       (((Command)) (message "Matched command")))
>   (error
>    (message "%s" (cdr err))))
>
> It appears that an ExecuteCommandParams interface should be added to
> eglot--lsp-interface-alist and that a matcher for ExecuteCommandParams
> should be added to the eglot--dcase in eglot-execute.  Additionally,
> the Command matcher should be changed to not send the "title"
> parameter in the subsequent workspace/executeCommand request.

Adding João as author of eglot

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

From: João Távora <joaotavora <at> gmail.com>
To: Jeremy Bryant <jb <at> jeremybryant.net>
Cc: 71642 <at> debbugs.gnu.org, Troy Brown <brownts <at> troybrown.dev>
Subject: Re: bug#71642: 30.0.50;
 eglot-execute doesn't support ExecuteCommandParams
Date: Wed, 19 Jun 2024 23:26:20 +0100

On Wed, Jun 19, 2024 at 10:19 PM Jeremy Bryant <jb <at> jeremybryant.net> wrote:
>
> > It appears that an ExecuteCommandParams interface should be added to
> > eglot--lsp-interface-alist and that a matcher for ExecuteCommandParams
> > should be added to the eglot--dcase in eglot-execute.
>
> Adding João as author of eglot

Thanks.

Troy, you seem to be on to something.  It would seem Command shouldn't
be there in the dcase matcher in eglot-execute at all.  Instead
ExecuteCommandParams should be there.  If so, patches welcome, as you
seem to have grasped how this code is supposed to work.

> Additionally,
> > the Command matcher should be changed to not send the "title"
> > parameter in the subsequent workspace/executeCommand request.

Looks like it shouldn't even be there right?

I may just have been thrown off  by the confusing naming (naively
thinking that executeCommand executes Commands, but it doesnt).

The reason no one has complained is probably that there aren't
many users invoking custom commands, and if there are, they could
be just using `jsonrpc-request` directly or using the deprecated
interface.

João

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

From: Troy Brown <brownts <at> troybrown.dev>
To: João Távora <joaotavora <at> gmail.com>
Cc: Jeremy Bryant <jb <at> jeremybryant.net>, 71642 <at> debbugs.gnu.org
Subject: Re: bug#71642: 30.0.50;
 eglot-execute doesn't support ExecuteCommandParams
Date: Wed, 19 Jun 2024 23:55:10 -0400

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

On Wed, Jun 19, 2024 at 6:26 PM João Távora <joaotavora <at> gmail.com> wrote:
>
> Troy, you seem to be on to something.  It would seem Command shouldn't
> be there in the dcase matcher in eglot-execute at all.  Instead
> ExecuteCommandParams should be there.
>

I suspected that might be the case (that Command was really supposed
to be ExecuteCommandParams), but I hadn't exhaustively looked at
everything, however you have now confirmed that suspicion.

>
> I may just have been thrown off  by the confusing naming (naively
> thinking that executeCommand executes Commands, but it doesnt).
>

Yes, that makes sense, I was confused for a bit as well until I had
investigated why it had the "title" parameter, looked at the LSP
specification and realized that it was meant for something else.

> The reason no one has complained is probably that there aren't
> many users invoking custom commands, and if there are, they could
> be just using `jsonrpc-request` directly or using the deprecated
> interface.

Yes, that makes sense, I'd imagine people who are using custom
commands are using the older eglot-execute-command interface since
it's still functional.  I had looked at it and saw that it was now
deprecated so I attempted to use the new interface.

>  If so, patches welcome, as you
> seem to have grasped how this code is supposed to work.
>

I've attached a patch which I believe addresses the issue.  I did have
to reorder the interfaces to put CodeAction before
ExecuteCommandParams as both contain "command" parameters.  Since
eglot-strict-mode doesn't normally contain disallow-non-standard-keys,
it causes eglot--dcase to match ExecuteCommandParams for a CodeAction
interface if the optional "command" parameter is present.  Reordering
the interfaces allows them to match correctly since CodeAction
contains a required "title" parameter which is not present in the
ExecuteCommandParams interface.

[0001-Eglot-Fix-command-execution-bug-71642.patch (text/x-patch, attachment)]

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

From: João Távora <joaotavora <at> gmail.com>
To: Troy Brown <brownts <at> troybrown.dev>
Cc: Jeremy Bryant <jb <at> jeremybryant.net>, 71642 <at> debbugs.gnu.org
Subject: Re: bug#71642: 30.0.50;
 eglot-execute doesn't support ExecuteCommandParams
Date: Thu, 20 Jun 2024 10:46:38 +0100

On Thu, Jun 20, 2024 at 4:55 AM Troy Brown <brownts <at> troybrown.dev> wrote:
>
> On Wed, Jun 19, 2024 at 6:26 PM João Távora <joaotavora <at> gmail.com> wrote:
> >
> > Troy, you seem to be on to something.  It would seem Command shouldn't
> > be there in the dcase matcher in eglot-execute at all.  Instead
> > ExecuteCommandParams should be there.
> I suspected that might be the case (that Command was really supposed
> to be ExecuteCommandParams), but I hadn't exhaustively looked at
> everything, however you have now confirmed that suspicion.

No, no, after closer analysis, this is not right.  Command must
absolutely be there in the matcher, as textDocument/codeAction
can simply return an array of them.

> > I may just have been thrown off  by the confusing naming (naively
> > thinking that executeCommand executes Commands, but it doesnt).
> Yes, that makes sense, I was confused for a bit as well until I had
> investigated why it had the "title" parameter, looked at the LSP
> specification and realized that it was meant for something else.

On closer analysis the only confusing thing is that the spec
encourages clients to call textDocument/executeCommand with
illegal ExecuteCommandParams objects that contain an extraneous
'title'

Most servers apparently don't care, but Eglot is doing this and
it's a subtle bug.

I think the right thing to do is to have Command _and_
ExecuteCommandParams in the matcher.

After that:

* custom callers of `textDocument/executeCommand` can do their thing
   (they could already but now it's arguably easiest)
* Eglot won't suffer from this subtle bug
* Hopefully the situation is clearer

While none of this is super-serious, I think we can fix it and
we'll benefit in clarity.

> Since
> eglot-strict-mode doesn't normally contain disallow-non-standard-keys,
> it causes eglot--dcase to match ExecuteCommandParams for a CodeAction
> interface if the optional "command" parameter is present.

But if it is present, it shouldn't be a string, so it should still
work in the order.

I've done a tweak to your patch.  See after my sig.  It's untested, so
please have a look. If you can, test, add more comments, change the order
if indeed it's needed.  You can also change the implementation if you think
the recursiveness makes it more confusing.

When that is done, please resubmit the patch with the nicely formatted
commit message (just like you did before) and I'll ask someone to
install it (or install it myself).

João

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 6896baf30ce..450f5a507f6 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -631,6 +631,7 @@ eglot--uri-path-allowed-chars
                              :command :data :tags))
       (Diagnostic (:range :message) (:severity :code :source
:relatedInformation :codeDescription :tags))
       (DocumentHighlight (:range) (:kind))
+      (ExecuteCommandParams ((:command . string)) (:arguments))
       (FileSystemWatcher (:globPattern) (:kind))
       (Hover (:contents) (:range))
       (InitializeResult (:capabilities) (:serverInfo))
@@ -891,17 +892,25 @@ eglot-execute-command

 (cl-defgeneric eglot-execute (server action)
   "Ask SERVER to execute ACTION.
-ACTION is an LSP object of either `CodeAction' or `Command' type."
+ACTION is an LSP `CodeAction', `Command' or `ExecuteCommandParams'
+object."
   (:method
    (server action) "Default implementation."
    (eglot--dcase action
-     (((Command)) (eglot--request server :workspace/executeCommand action))
+     (((Command))
+      ;; Convert to ExecuteCommandParams and recurse (bug#71642)
+      (cl-remf action :title)
+      (eglot-execute action))
+     (((ExecuteCommandParams))
+      (eglot--request server :workspace/executeCommand action))
      (((CodeAction) edit command data)
       (if (and (null edit) (null command) data
                (eglot-server-capable :codeActionProvider :resolveProvider))
           (eglot-execute server (eglot--request server
:codeAction/resolve action))
         (when edit (eglot--apply-workspace-edit edit this-command))
-        (when command (eglot--request server
:workspace/executeCommand command)))))))
+        (when command
+          ;; Recursive call with what must be a Command object (bug#71642)
+          (eglot-execute command)))))))

 (cl-defgeneric eglot-initialization-options (server)
   "JSON object to send under `initializationOptions'."

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

From: João Távora <joaotavora <at> gmail.com>
To: Troy Brown <brownts <at> troybrown.dev>
Cc: Jeremy Bryant <jb <at> jeremybryant.net>, 71642 <at> debbugs.gnu.org
Subject: Re: bug#71642: 30.0.50;
 eglot-execute doesn't support ExecuteCommandParams
Date: Fri, 21 Jun 2024 14:51:26 +0100

On Fri, Jun 21, 2024 at 1:14 PM Troy Brown <brownts <at> troybrown.dev> wrote:

> I've updated the patch based on your suggestions.

Looks good, thanks!

The only doubt I have is the Copyright Assignment.  This is still a
"trivial" patch  (< 15 LOC), I think but I'll need to ask Eli for
his opinion to be sure.

Eli, if you're OK with the copyright aspects, please install this
and close the bug.

João

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

From: Troy Brown <brownts <at> troybrown.dev>
To: João Távora <joaotavora <at> gmail.com>
Cc: Jeremy Bryant <jb <at> jeremybryant.net>, 71642 <at> debbugs.gnu.org
Subject: Re: bug#71642: 30.0.50;
 eglot-execute doesn't support ExecuteCommandParams
Date: Fri, 21 Jun 2024 08:14:02 -0400

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

On Thu, Jun 20, 2024 at 5:46 AM João Távora <joaotavora <at> gmail.com> wrote:
>
> On closer analysis the only confusing thing is that the spec
> encourages clients to call textDocument/executeCommand with
> illegal ExecuteCommandParams objects that contain an extraneous
> 'title'
>
> Most servers apparently don't care, but Eglot is doing this and
> it's a subtle bug.
>
> I think the right thing to do is to have Command _and_
> ExecuteCommandParams in the matcher.
>

Thanks for the explanation.

> I've done a tweak to your patch.  See after my sig.  It's untested, so
> please have a look. If you can, test, add more comments, change the order
> if indeed it's needed.  You can also change the implementation if you think
> the recursiveness makes it more confusing.
>

I've updated the patch based on your suggestions.  I've tested all
invocations of executeCommand through Command, ExecuteCommandParams
and CodeAction and they all appear to be working as expected now.  I
think the recursion is fine, it's simple and elegant.  With regards to
the order, you are correct, since the command in CodeAction isn't a
string, it's not a problem.  I was previously erroneously only testing
with a string.

[0001-Eglot-Fix-command-execution-bug-71642.patch (text/x-patch, attachment)]

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

From: João Távora <joaotavora <at> gmail.com>
To: Troy Brown <brownts <at> troybrown.dev>, Eli Zaretskii <eliz <at> gnu.org>
Cc: Jeremy Bryant <jb <at> jeremybryant.net>, 71642 <at> debbugs.gnu.org
Subject: Re: bug#71642: 30.0.50;
 eglot-execute doesn't support ExecuteCommandParams
Date: Sat, 22 Jun 2024 09:44:59 +0100

On Fri, Jun 21, 2024 at 2:51 PM João Távora <joaotavora <at> gmail.com> wrote:
>
> On Fri, Jun 21, 2024 at 1:14 PM Troy Brown <brownts <at> troybrown.dev> wrote:
>
> > I've updated the patch based on your suggestions.
>
> Looks good, thanks!
>
> The only doubt I have is the Copyright Assignment.  This is still a
> "trivial" patch  (< 15 LOC), I think but I'll need to ask Eli for
> his opinion to be sure.
>
> Eli, if you're OK with the copyright aspects, please install this
> and close the bug.

Now actually copying in Eli.
João

Message #31 received at 71642-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: João Távora <joaotavora <at> gmail.com>
Cc: jb <at> jeremybryant.net, brownts <at> troybrown.dev, 71642-done <at> debbugs.gnu.org
Subject: Re: bug#71642: 30.0.50;
 eglot-execute doesn't support ExecuteCommandParams
Date: Sat, 22 Jun 2024 12:48:52 +0300

> From: João Távora <joaotavora <at> gmail.com>
> Date: Sat, 22 Jun 2024 09:44:59 +0100
> Cc: Jeremy Bryant <jb <at> jeremybryant.net>, 71642 <at> debbugs.gnu.org
> 
> On Fri, Jun 21, 2024 at 2:51 PM João Távora <joaotavora <at> gmail.com> wrote:
> >
> > On Fri, Jun 21, 2024 at 1:14 PM Troy Brown <brownts <at> troybrown.dev> wrote:
> >
> > > I've updated the patch based on your suggestions.
> >
> > Looks good, thanks!
> >
> > The only doubt I have is the Copyright Assignment.  This is still a
> > "trivial" patch  (< 15 LOC), I think but I'll need to ask Eli for
> > his opinion to be sure.
> >
> > Eli, if you're OK with the copyright aspects, please install this
> > and close the bug.
> 
> Now actually copying in Eli.

Thanks, I installed this, and I'm therefore closing the bug.

Troy, if you want us to be able to accept your contributions in the
future, please consider starting the copyright assignment paperwork at
this time.  (If you agree, I will send you the form to fill and the
instructions to go with it.)

Message #34 received at 71642-done <at> debbugs.gnu.org (full text, mbox):

From: Troy Brown <brownts <at> troybrown.dev>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: jb <at> jeremybryant.net,
 João Távora <joaotavora <at> gmail.com>,
 71642-done <at> debbugs.gnu.org
Subject: Re: bug#71642: 30.0.50;
 eglot-execute doesn't support ExecuteCommandParams
Date: Sun, 23 Jun 2024 10:27:39 -0400

On Sat, Jun 22, 2024 at 5:48 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> Troy, if you want us to be able to accept your contributions in the
> future, please consider starting the copyright assignment paperwork at
> this time.  (If you agree, I will send you the form to fill and the
> instructions to go with it.)

Yes please send me the form and instructions, thanks Eli.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Troy Brown <brownts <at> troybrown.dev>
Cc: jb <at> jeremybryant.net, joaotavora <at> gmail.com, 71642-done <at> debbugs.gnu.org
Subject: Re: bug#71642: 30.0.50;
 eglot-execute doesn't support ExecuteCommandParams
Date: Sun, 23 Jun 2024 17:35:29 +0300

> From: Troy Brown <brownts <at> troybrown.dev>
> Date: Sun, 23 Jun 2024 10:27:39 -0400
> Cc: João Távora <joaotavora <at> gmail.com>, 
> 	jb <at> jeremybryant.net, 71642-done <at> debbugs.gnu.org
> 
> On Sat, Jun 22, 2024 at 5:48 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
> >
> > Troy, if you want us to be able to accept your contributions in the
> > future, please consider starting the copyright assignment paperwork at
> > this time.  (If you agree, I will send you the form to fill and the
> > instructions to go with it.)
> 
> Yes please send me the form and instructions, thanks Eli.

Thanks, form sent off-list.

Previous Next

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