Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interesting, but shouldn't this plug into jsonrpc.el? #2

Open
joaotavora opened this issue Jan 11, 2024 · 4 comments
Open

Interesting, but shouldn't this plug into jsonrpc.el? #2

joaotavora opened this issue Jan 11, 2024 · 4 comments
Labels
help wanted Extra attention is needed

Comments

@joaotavora
Copy link

No description provided.

@jdtsmith
Copy link
Owner

It does advise jsonrpc--json-read, but tries to be non-invasive for other users of jsonrpc who are not expecting bytecode instead of actual json.

Maybe you mean to wrap the server command itself "just in time" inside jsonrpc instead of hacking the eglot-server-programs? I agree that would be preferable, but I couldn't figure out a convenient way of doing that. Perhaps an eglot-lsp-server subclass that wraps commands before the process starts? Not sure how that would look.

Longer term if this approach pans out, it would be good to have real builtin jsonrpc support.

@joaotavora
Copy link
Author

Maybe you mean to wrap the server command itself "just in time" inside jsonrpc instead of hacking

Yes, also that.

eglot-lsp-server subclass. Not sure how that would look.

Eglot and Jsonrpc aren't set in stone. You can propose changes to them to accomodate your plugins. Maybe these new generics would help you:

diff --git a/lisp/jsonrpc.el b/lisp/jsonrpc.el
index 3f33443f321..844acfa4d79 100644
--- a/lisp/jsonrpc.el
+++ b/lisp/jsonrpc.el
@@ -562,7 +562,7 @@ jsonrpc-connection-send
                      (id 'request)
                      (method 'notification)))
          (converted (jsonrpc-convert-to-endpoint connection args kind))
-         (json (jsonrpc--json-encode converted))
+         (json (jsonrpc-encode connection converted))
          (headers
           `(("Content-Length" . ,(format "%d" (string-bytes json)))
             ;; ("Content-Type" . "application/vscode-jsonrpc; charset=utf-8")
@@ -614,35 +614,35 @@ jsonrpc-stderr-buffer
 ;;;
 (define-error 'jsonrpc-error "jsonrpc-error")
 
-(defalias 'jsonrpc--json-read
+(cl-defgeneric jsonrpc-read (conn)
+  "Read object in buffer for CONN, move point to end of buffer.")
+
+(cl-defmethod jsonrpc-read ((_conn t))
   (if (fboundp 'json-parse-buffer)
-      (lambda ()
-        (json-parse-buffer :object-type 'plist
+      (json-parse-buffer :object-type 'plist
                            :null-object nil
-                           :false-object :json-false))
+                           :false-object :json-false)
     (require 'json)
     (defvar json-object-type)
     (declare-function json-read "json" ())
-    (lambda ()
-      (let ((json-object-type 'plist))
-        (json-read))))
-  "Read JSON object in buffer, move point to end of buffer.")
+    (let ((json-object-type 'plist))
+      (json-read))))
+
+(cl-defgeneric jsonrpc-encode (conn object)
+  "Encode OBJECT for CONN.")
 
-(defalias 'jsonrpc--json-encode
+(cl-defmethod jsonrpc-encode ((_conn t) object)
   (if (fboundp 'json-serialize)
-      (lambda (object)
-        (json-serialize object
-                        :false-object :json-false
-                        :null-object nil))
+      (json-serialize object
+                      :false-object :json-false
+                      :null-object nil)
     (require 'json)
     (defvar json-false)
     (defvar json-null)
     (declare-function json-encode "json" (object))
-    (lambda (object)
-      (let ((json-false :json-false)
-            (json-null nil))
-        (json-encode object))))
-  "Encode OBJECT into a JSON string.")
+    (let ((json-false :json-false)
+          (json-null nil))
+      (json-encode object))))
 
 (cl-defun jsonrpc--reply
     (connection id method &key (result nil result-supplied-p) (error nil error-supplied-p))
@@ -737,7 +737,7 @@ jsonrpc--process-filter
                               (narrow-to-region (point) message-end)
                               (setq message
                                     (condition-case-unless-debug oops
-                                        (jsonrpc--json-read)
+                                        (jsonrpc-read conn)
                                       (error
                                        (jsonrpc--warn "Invalid JSON: %s %s"
                                                       (cdr oops) (buffer-string))
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index ba2cc72a6b4..c6f24afc05d 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1240,6 +1240,8 @@ eglot--lookup-mode
                                (cons (languages sym specs) contact))))
                       specs))))
 
+(defvar eglot-default-server-class 'eglot-lsp-server)
+
 (defun eglot--guess-contact (&optional interactive)
   "Helper for `eglot'.
 Return (MANAGED-MODES PROJECT CLASS CONTACT LANG-IDS).  If INTERACTIVE is
@@ -1273,7 +1275,7 @@ eglot--guess-contact
          (class (or (and (consp guess) (symbolp (car guess))
                          (prog1 (unless current-prefix-arg (car guess))
                            (setq guess (cdr guess))))
-                    'eglot-lsp-server))
+                    eglot-default-server-class))
          (program (and (listp guess)
                        (stringp (car guess))
                        ;; A second element might be the port of a (host, port)
@@ -1469,8 +1471,9 @@ eglot-server-initialized-hook
 
 Each function is passed the server as an argument")
 
-(defun eglot--cmd (contact)
-  "Helper for `eglot--connect'."
+(cl-defgeneric eglot--cmd (_conn contact)
+  "Helper for `eglot--connect'.
+CONN and CONTACT should probably be briefly documented."
   (if (file-remote-p default-directory)
       ;; TODO: this seems like a bug, although it’s everywhere. For
       ;; some reason, for remote connections only, over a pipe, we
@@ -1520,7 +1523,7 @@ eglot--connect
                         (more-initargs (and probe (cl-subseq contact probe)))
                         (contact (cl-subseq contact 0 probe)))
                    `(:process
-                     ,(lambda ()
+                     ,(lambda (conn)
                         (let ((default-directory default-directory)
                               ;; bug#61350: Tramp turns on a feature
                               ;; by default that can't (yet) handle
@@ -1532,7 +1535,7 @@ eglot--connect
                                "-o ControlMaster=no -o ControlPath=none"))
                           (make-process
                            :name readable-name
-                           :command (setq server-info (eglot--cmd contact))
+                           :command (setq server-info (eglot--cmd conn contact))
                            :connection-type 'pipe
                            :coding 'utf-8-emacs-unix
                            :noquery t
@@ -2635,7 +2638,7 @@ eglot-show-workspace-configuration
   (let ((conf (eglot--workspace-configuration-plist server)))
     (with-current-buffer (get-buffer-create "*EGLOT workspace configuration*")
       (erase-buffer)
-      (insert (jsonrpc--json-encode conf))
+      (insert (jsonrpc-encode (or server t) conf))
       (with-no-warnings
         (require 'json)
         (when (require 'json-mode nil t) (json-mode))

With this in place, you could simplify your plugin with something like this.

(cl-defmethod eglot--cmd ((_conn eglot-boosted-lsp-server) _contact)
  (let ((normal (cl-call-next-method)))
    ;; wrap `normal' in boosted things
    ))

(setq eglot-default-server-class 'eglot-boosted-lsp-server)

(cl-defmethod jsonrpc-read ((conn eglot-boosted-lsp-server))
  ;; do boosted things
)

As to supporting this for all jsonrpc.el clients, maybe this can be done too, but currently the client supplies the running process object to jsonrpc, or a process-making lambda when instantiating the jsonrpc-process-connection object. A future hypothetical jsonrpc-boosted-process-connection object would not allow being instantiated with the process directly, but rather with a sequence of strings. The generics I posted above would also help in this case, regardless.

@jdtsmith
Copy link
Owner

Interesting approach, I suspected something like this should be possible. Let's see how it performs in the wild a bit and think about what a more permanent/less intrusive installation might look like. Right now the underlying booster wrapper doesn't work with network socket facing servers; I suspect it would be possible to add that capability to the rust wrapper, depending on how it gets used. Probably makes more sense to wrap on the remote side since the network traffic goes down too.

@jdtsmith
Copy link
Owner

jdtsmith commented Feb 11, 2024

Hi @joaotavora, it seems people are indeed using this. A recent change to eglot on master broke it, and it's also brittle in the face of more complex entries in eglot-server-programs; see #4/#5 . Since it's hacking the alternatives list and advising jsonrpc--json-read this is actually not too surprising to me.

Can you suggest a more permanent and graceful installation based on current eglot/jsonrpc? All it needs to do at core is prepend "emacs-lsp-booster" "--json-false-value" ":json-false" "--" to the front of the process command+args list (except for network-facing servers).

@jdtsmith jdtsmith added the help wanted Extra attention is needed label Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants