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