None of the code actions take effect except "Dismiss: ..." #7

sychen52 · 2022-07-06T05:10:05Z

lsp-grammarly works fine, though. Do you have any idea what could be the cause?

sychen52 · 2022-07-06T05:12:29Z

I also tried clangd, and its code actions work fine, so it seems to be grammarly related.

jcs090218 · 2022-07-06T05:20:52Z

No, I haven't really tried this package yet. I think it should related to the language server (upstream); maybe related to znck/grammarly#277.

Edit: Before server support workspace/executeCommand, everything work but the Dismiss: .... Sounds like a hint!?

sychen52 · 2022-07-10T20:37:30Z

Here is the issue I find. In eglot-code-actions function: it gets an action from the server first. Then it handles two types of action: Command or CodeAction.
The one grammarly lsp send back is similar to CodeAction. However, :edit and :command fields are missing. The following is the definition of a CodeAction and the action returned by grammarly lsp:
@jcs090218, Do you have any suggestions for further investigation?

      (CodeAction (:title) (:kind :diagnostics :edit :command :isPreferred))

(:title "Correct your spelling — overall" :kind "quickfix" :diagnostics
        [(:data "_215" :message "Correct your spelling" :range
                (:start
                 (:line 9 :character 50)
                 :end
                 (:line 9 :character 58))
                :source "Grammarly" :severity 1)]
        :data
        (:uri "file:///........org" :suggestionId "_215" :replacementId 0))

jcs090218 · 2022-07-22T05:30:42Z

Sorry it took me a while to respond to this issue. Was busy with something else!

My suggestion is to compare the two packets (or messages) from the two Emacs client (one from lsp-mode, and one from eglot). It seems like there are some implementation differences between the two lsp client, so their responds or actions are different.

Maybe @joaotavora could help us? 😕

ssl19 · 2023-05-15T21:26:04Z

It seems related to eglot's lack of support for codeAction/resolve.

ssl19 · 2023-05-15T21:31:47Z

(defun eglot--read-execute-code-action@override (actions server &optional action-kind)
  "Support for codeAction/resolve."
  (let* ((menu-items
          (or (cl-loop for a in actions
                    collect (cons (plist-get a :title) a))
              (apply #'eglot--error
                     (if action-kind `("No \"%s\" code actions here" ,action-kind)
                       `("No code actions here")))))
         (preferred-action (cl-find-if
                            (lambda (menu-item)
                              (plist-get (cdr menu-item) :isPreferred))
                            menu-items))
         (default-action (car (or preferred-action (car menu-items))))
         (chosen (if (and action-kind (null (cadr menu-items)))
                     (cdr (car menu-items))
                   (if (listp last-nonmenu-event)
                       (x-popup-menu last-nonmenu-event `("Eglot code actions:"
                                                          ("dummy" ,@menu-items)))
                     (cdr (assoc (completing-read
                                  (format "[eglot] Pick an action (default %s): "
                                          default-action)
                                  menu-items nil t nil nil default-action)
                                 menu-items))))))
    (cl-labels ((apply-code-action (chosen first-p)
                  (eglot--dcase chosen
                    (((Command) command arguments)
                     (eglot-execute-command server (intern command) arguments))
                    (((CodeAction) edit command)
                     (when edit (eglot--apply-workspace-edit edit))
                     (when command
                       (eglot--dbind ((Command) command arguments) command
                         (eglot-execute-command server (intern command) arguments)))
                     (when (and (eglot--server-capable :codeActionProvider
                                                       :resolveProvider)
                                first-p)
                       (apply-code-action (eglot--request server :codeAction/resolve chosen) nil))))))
      (apply-code-action chosen t))))


(advice-add #'eglot--read-execute-code-action :override #'eglot--read-execute-code-action@override)

You could try code actions of eglot-grammarly with this hack :)

krisbalintona · 2023-07-10T06:33:44Z

@ssl19 Wow! Awesome that you not only figured out the source of the missing functionality, but you also wrote a working fix. Thanks!

Have you considered contributing this to Eglot, since the missing functionality is with Eglot itself?

joaotavora · 2023-07-10T09:59:59Z

This patch won't apply currently, and I don't understand exactly how the last part of the logic works.

Anyway this isn't so much related to "eglot's lack of support for codeAction/resolve." as it is to this LSP server's nonobservance of the standard, which says that you shouldn't rely on a feature that the other endpoint hasn't declared support for. And Eglot indeed doesn't support codeAction/resolve for now. I'll see what I can do...

joaotavora · 2023-07-10T10:44:57Z

Can someone using a server with these capabilities try out this patch to Eglot?

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index f09c348143d..5a716cd6aef 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -463,7 +463,7 @@ eglot--accepted-formats
 (eval-and-compile
   (defvar eglot--lsp-interface-alist
     `(
-      (CodeAction (:title) (:kind :diagnostics :edit :command :isPreferred))
+      (CodeAction (:title) (:kind :diagnostics :edit :command :isPreferred :data))
       (ConfigurationItem () (:scopeUri :section))
       (Command ((:title . string) (:command . string)) (:arguments))
       (CompletionItem (:label)
@@ -739,9 +739,12 @@ eglot-execute
    (server action) "Default implementation."
    (eglot--dcase action
      (((Command)) (eglot--request server :workspace/executeCommand action))
-     (((CodeAction) edit command)
-      (when edit (eglot--apply-workspace-edit edit))
-      (when command (eglot--request server :workspace/executeCommand command))))))
+     (((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))
+        (when command (eglot--request server :workspace/executeCommand command)))))))
 
 (cl-defgeneric eglot-initialization-options (server)
   "JSON object to send under `initializationOptions'."
@@ -825,6 +828,7 @@ eglot-client-capabilities
              :documentHighlight  `(:dynamicRegistration :json-false)
              :codeAction         (list
                                   :dynamicRegistration :json-false
+                                  :resolveSupport t :dataSupport t
                                   :codeActionLiteralSupport
                                   '(:codeActionKind
                                     (:valueSet

krisbalintona · 2023-07-10T16:49:27Z

@joaotavora You're right about emacs-grammarly's non-observance of the standard. I believe the package is maintained by only one person?

Anyway, from my testing, the patch you shared results in identical functionality to the @ssl19's code (according to my very limited "testing"). (Also, thank you for your contributions to Eglot and Emacs)

joaotavora · 2023-07-10T23:03:19Z

OK. In that case I'll push the patch to master. It should appear in the next Eglot release 1.16.

On an unrelated note, I've also noticed that eglot-grammarly is and extremely thin customization that -- in my humble opinion -- didn't need to exist.

It would seem users can get the same effect as this package with this single form:

(add-to-list 
  'eglot-server-programs
    `((text-mode latex-mode org-mode markdown-mode) "grammarly-languageserver" "--stdio" 
           :initializationOptions (:clientId "client_BaDkMgx4X19X9UxxYRCXZo"))

This is untested though, as I don't have grammarly-languageserver

joaotavora · 2023-07-10T23:08:36Z

OK. In that case I'll push the patch to master. It should appear in the next Eglot release 1.16.

Done. See https://git.savannah.gnu.org/cgit/emacs.git/commit/lisp/progmodes/eglot.el?id=3b7273f4ae3623962c5d5fdc922a62af1136f448

See emacs-grammarly/eglot-grammarly#7. * lisp/progmodes/eglot.el (eglot--lsp-interface-alist): Augment CodeAction type. (eglot-execute): Consider :codeAction/resolve (eglot-client-capabilities): Advertise textDocument.codeAction.resolveSupport

jcs090218 · 2023-07-11T00:31:38Z

@joaotavora You're right about emacs-grammarly's non-observance of the standard. I believe the package is maintained by only one person?

Yeah, the entire org is maintained by me. :)

On an unrelated note, I've also noticed that eglot-grammarly is and extremely thin customization that -- in my humble opinion -- didn't need to exist.

I don't think I can change your mind, but I am going to put up my reasons why this is needed:

Save startup time (Yeah, it's stupid, but it's important to me since I have used up 500+ packages in my config)
A place (this repo) for me to track changes from the language server (grammarly)
I think it's stupid to paste a code snippet directly to your config (in this case). What something has changed from the upstream? Why not let the other (the community) help you keep track of things?

I believe the issue can be closed. Thanks to everyone who participates in this issue!

joaotavora · 2023-07-11T09:12:53Z

I think it's stupid to paste a code snippet directly to your config (in this case). What something has changed from the upstream? Why not let the other (the community) help you keep track of things?

Stupid? A one-liner? Could even be in Eglot upstream! And setting up a package doesn't need any config? And keeping it up to date? Are you sure you're not busy looking for an excuse to make a package, any package? Then why not make an useful one?

chaosite · 2023-07-25T14:46:17Z

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index f09c348143d..5a716cd6aef 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -463,7 +463,7 @@ eglot--accepted-formats
 (eval-and-compile
   (defvar eglot--lsp-interface-alist
     `(
-      (CodeAction (:title) (:kind :diagnostics :edit :command :isPreferred))
+      (CodeAction (:title) (:kind :diagnostics :edit :command :isPreferred :data))
       (ConfigurationItem () (:scopeUri :section))
       (Command ((:title . string) (:command . string)) (:arguments))
       (CompletionItem (:label)
@@ -739,9 +739,12 @@ eglot-execute
    (server action) "Default implementation."
    (eglot--dcase action
      (((Command)) (eglot--request server :workspace/executeCommand action))
-     (((CodeAction) edit command)
-      (when edit (eglot--apply-workspace-edit edit))
-      (when command (eglot--request server :workspace/executeCommand command))))))
+     (((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))
+        (when command (eglot--request server :workspace/executeCommand command)))))))
 
 (cl-defgeneric eglot-initialization-options (server)
   "JSON object to send under `initializationOptions'."
@@ -825,6 +828,7 @@ eglot-client-capabilities
              :documentHighlight  `(:dynamicRegistration :json-false)
              :codeAction         (list
                                   :dynamicRegistration :json-false
+                                  :resolveSupport t :dataSupport t

This breaks Eglot for (at least) rust-analyzer, since it doesn't match the specification of codeAction.resolveSupport: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_codeAction

joaotavora · 2023-07-25T15:03:11Z

This breaks Eglot f

Thanks for testing this patch, but as far as I know, this has already been fixed in the latest version in Emacs trunk.

jcs090218 · 2023-08-16T01:10:27Z

Stupid? A one-liner? Could even be in Eglot upstream! And setting up a package doesn't need any config? And keeping it up to date? Are you sure you're not busy looking for an excuse to make a package, any package? Then why not make an useful one?

I'm gonna let you know I have seen your comment, but I will not reply since it will probably be a waste of my time. Cheers! :)

jcs090218 added the good first issue Good for newcomers label Jul 6, 2022

jcs090218 closed this as completed Jul 11, 2023

jcs090218 mentioned this issue Aug 15, 2023

Code actions do nothing #10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

None of the code actions take effect except "Dismiss: ..." #7

None of the code actions take effect except "Dismiss: ..." #7

sychen52 commented Jul 6, 2022

sychen52 commented Jul 6, 2022

jcs090218 commented Jul 6, 2022 •

edited

sychen52 commented Jul 10, 2022 •

edited

jcs090218 commented Jul 22, 2022

ssl19 commented May 15, 2023

ssl19 commented May 15, 2023

krisbalintona commented Jul 10, 2023

joaotavora commented Jul 10, 2023

joaotavora commented Jul 10, 2023

krisbalintona commented Jul 10, 2023

joaotavora commented Jul 10, 2023

joaotavora commented Jul 10, 2023

jcs090218 commented Jul 11, 2023

joaotavora commented Jul 11, 2023

chaosite commented Jul 25, 2023

joaotavora commented Jul 25, 2023

jcs090218 commented Aug 16, 2023

None of the code actions take effect except "Dismiss: ..." #7

None of the code actions take effect except "Dismiss: ..." #7

Comments

sychen52 commented Jul 6, 2022

sychen52 commented Jul 6, 2022

jcs090218 commented Jul 6, 2022 • edited

sychen52 commented Jul 10, 2022 • edited

jcs090218 commented Jul 22, 2022

ssl19 commented May 15, 2023

ssl19 commented May 15, 2023

krisbalintona commented Jul 10, 2023

joaotavora commented Jul 10, 2023

joaotavora commented Jul 10, 2023

krisbalintona commented Jul 10, 2023

joaotavora commented Jul 10, 2023

joaotavora commented Jul 10, 2023

jcs090218 commented Jul 11, 2023

joaotavora commented Jul 11, 2023

chaosite commented Jul 25, 2023

joaotavora commented Jul 25, 2023

jcs090218 commented Aug 16, 2023

jcs090218 commented Jul 6, 2022 •

edited

sychen52 commented Jul 10, 2022 •

edited