bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

[Troy Brown]

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.

bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

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

Troy Brown <brownts@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

bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

[João Távora]

On Wed, Jun 19, 2024 at 10:19 PM Jeremy Bryant <jb@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

bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

[Troy Brown]

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

On Wed, Jun 19, 2024 at 6:26 PM João Távora <joaotavora@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.

[-- Attachment #2: 0001-Eglot-Fix-command-execution-bug-71642.patch --]
[-- Type: text/x-patch, Size: 2275 bytes --]

From f7c20afc4649aeeda0b36118f50b59365d1c96e4 Mon Sep 17 00:00:00 2001
From: Troy Brown <brownts@troybrown.dev>
Date: Wed, 19 Jun 2024 20:14:07 -0400
Subject: [PATCH] Eglot: Fix command execution (bug#71642)

* lisp/progmodes/eglot.el (eglot--lsp-interface-alist): Add
ExecuteCommandParams interface.
(eglot--execute): Change Command interface to ExecuteCommandParams
interface.

Copyright-paperwork-exempt: yes
---
 lisp/progmodes/eglot.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 03da5c7b22a..cf91eae5694 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -624,6 +624,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))
@@ -884,17 +885,18 @@ 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 object of either `CodeAction' or `ExecuteCommandParams' type."
   (:method
    (server action) "Default implementation."
    (eglot--dcase action
-     (((Command)) (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 (eglot--request server :workspace/executeCommand command))))
+     (((ExecuteCommandParams))
+      (eglot--request server :workspace/executeCommand action)))))
 
 (cl-defgeneric eglot-initialization-options (server)
   "JSON object to send under `initializationOptions'."
-- 
2.37.1

bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

[João Távora]

On Thu, Jun 20, 2024 at 4:55 AM Troy Brown <brownts@troybrown.dev> wrote:
>
> On Wed, Jun 19, 2024 at 6:26 PM João Távora <joaotavora@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'."

bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

[Troy Brown]

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

On Thu, Jun 20, 2024 at 5:46 AM João Távora <joaotavora@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.

[-- Attachment #2: 0001-Eglot-Fix-command-execution-bug-71642.patch --]
[-- Type: text/x-patch, Size: 2504 bytes --]

From 963e5c975f89d9c6111902694ac51facaf7fc2b8 Mon Sep 17 00:00:00 2001
From: Troy Brown <brownts@troybrown.dev>
Date: Wed, 19 Jun 2024 20:14:07 -0400
Subject: [PATCH] Eglot: Fix command execution (bug#71642)

* lisp/progmodes/eglot.el (eglot--lsp-interface-alist): Add
ExecuteCommandParams interface.
(eglot--execute): Fix handling of Command and CodeAction and add
ExecuteCommandParams.

Copyright-paperwork-exempt: yes
---
 lisp/progmodes/eglot.el | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 03da5c7b22a..df4cbe50dc0 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -624,6 +624,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))
@@ -884,17 +885,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 server 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 server command)))))))
 
 (cl-defgeneric eglot-initialization-options (server)
   "JSON object to send under `initializationOptions'."
-- 
2.37.1

bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

[João Távora]

On Fri, Jun 21, 2024 at 1:14 PM Troy Brown <brownts@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

bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

[João Távora]

On Fri, Jun 21, 2024 at 2:51 PM João Távora <joaotavora@gmail.com> wrote:
>
> On Fri, Jun 21, 2024 at 1:14 PM Troy Brown <brownts@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

bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Sat, 22 Jun 2024 09:44:59 +0100
> Cc: Jeremy Bryant <jb@jeremybryant.net>, 71642@debbugs.gnu.org
> 
> On Fri, Jun 21, 2024 at 2:51 PM João Távora <joaotavora@gmail.com> wrote:
> >
> > On Fri, Jun 21, 2024 at 1:14 PM Troy Brown <brownts@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.)

bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

[Troy Brown]

On Sat, Jun 22, 2024 at 5:48 AM Eli Zaretskii <eliz@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.

bug#71642: 30.0.50; eglot-execute doesn't support ExecuteCommandParams

[Eli Zaretskii]

> From: Troy Brown <brownts@troybrown.dev>
> Date: Sun, 23 Jun 2024 10:27:39 -0400
> Cc: João Távora <joaotavora@gmail.com>, 
> 	jb@jeremybryant.net, 71642-done@debbugs.gnu.org
> 
> On Sat, Jun 22, 2024 at 5:48 AM Eli Zaretskii <eliz@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.