bug#73472: [PATCH] Substitute quotation marks in Eglot messages

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

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

[-- Attachment #1.1: Type: text/plain, Size: 251 bytes --]

Hi all,

Another quick patch from myself.  This patch makes it so that the
diagnostics and interactive prompts from Eglot (such as the prompt you
iteract with when renaming a symbol) properly respect
‘text-quoting-style’.

-- 
— Thomas

[-- Attachment #2: 0001-Substitute-quotation-marks-in-Eglot-messages.patch --]
[-- Type: text/x-patch, Size: 1788 bytes --]

From 2038b983a751a2e26c2e8a0b8d55ade29fb394fd Mon Sep 17 00:00:00 2001
From: Thomas Voss <mail@thomasvoss.com>
Date: Wed, 25 Sep 2024 15:51:36 +0200
Subject: [PATCH] Substitute quotation marks in Eglot messages

* lisp/progmodes/eglot.el (eglot--error) (eglot--message): Wrap the
message in `substitute-quotes'.
* lisp/progmodes/eglot.el (eglot-rename): Format the prompt using
`format-prompt' as opposed to `format'.

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

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index a358b82..ee52767 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1698,11 +1698,11 @@ eglot--connect
 ;;;
 (defun eglot--error (format &rest args)
   "Error out with FORMAT with ARGS."
-  (error "[eglot] %s" (apply #'format format args)))
+  (error "[eglot] %s" (substitute-quotes (apply #'format format args))))
 
 (defun eglot--message (format &rest args)
   "Message out with FORMAT with ARGS."
-  (message "[eglot] %s" (apply #'format format args)))
+  (message "[eglot] %s" (substitute-quotes (apply #'format format args))))
 
 (defun eglot--warn (format &rest args)
   "Warning message with FORMAT and ARGS."
@@ -3704,8 +3704,8 @@ eglot-rename
   "Rename the current symbol to NEWNAME."
   (interactive
    (list (read-from-minibuffer
-          (format "Rename `%s' to: " (or (thing-at-point 'symbol t)
-                                         "unknown symbol"))
+          (format-prompt "Rename `%s' to" nil (or (thing-at-point 'symbol t)
+                                                  "unknown symbol"))
           nil nil nil nil
           (symbol-name (symbol-at-point)))))
   (eglot-server-capable-or-lose :renameProvider)
-- 
2.46.2

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

[Eli Zaretskii]

> Date: Wed, 25 Sep 2024 16:03:32 +0200
> From:  "Thomas Voss" via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Another quick patch from myself.  This patch makes it so that the
> diagnostics and interactive prompts from Eglot (such as the prompt you
> iteract with when renaming a symbol) properly respect
> ‘text-quoting-style’.

Thanks.

> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -1698,11 +1698,11 @@ eglot--connect
>  ;;;
>  (defun eglot--error (format &rest args)
>    "Error out with FORMAT with ARGS."
> -  (error "[eglot] %s" (apply #'format format args)))
> +  (error "[eglot] %s" (substitute-quotes (apply #'format format args))))
>  
>  (defun eglot--message (format &rest args)
>    "Message out with FORMAT with ARGS."
> -  (message "[eglot] %s" (apply #'format format args)))
> +  (message "[eglot] %s" (substitute-quotes (apply #'format format args))))
>  
>  (defun eglot--warn (format &rest args)
>    "Warning message with FORMAT and ARGS."
> @@ -3704,8 +3704,8 @@ eglot-rename
>    "Rename the current symbol to NEWNAME."
>    (interactive
>     (list (read-from-minibuffer
> -          (format "Rename `%s' to: " (or (thing-at-point 'symbol t)
> -                                         "unknown symbol"))
> +          (format-prompt "Rename `%s' to" nil (or (thing-at-point 'symbol t)
> +                                                  "unknown symbol"))
>            nil nil nil nil
>            (symbol-name (symbol-at-point)))))
>    (eglot-server-capable-or-lose :renameProvider)

Hmm... in the first two changes, how do we know that the quote
characters are indeed quoting stuff in the context where we substitute
quotes according to quoting-style?  Those functions get strings they
know nothing about, so it looks a bit risky to blindly convert any
quoting characters there.  Or what am I missing?

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

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

Hi Eli,

On Wed Sep 25, 2024 at 6:01 PM CEST, Eli Zaretskii wrote:
> > --- a/lisp/progmodes/eglot.el
> > +++ b/lisp/progmodes/eglot.el
> > @@ -1698,11 +1698,11 @@ eglot--connect
> >  ;;;
> >  (defun eglot--error (format &rest args)
> >    "Error out with FORMAT with ARGS."
> > -  (error "[eglot] %s" (apply #'format format args)))
> > +  (error "[eglot] %s" (substitute-quotes (apply #'format format args))))
> >  
> >  (defun eglot--message (format &rest args)
> >    "Message out with FORMAT with ARGS."
> > -  (message "[eglot] %s" (apply #'format format args)))
> > +  (message "[eglot] %s" (substitute-quotes (apply #'format format args))))
> >  
> >  (defun eglot--warn (format &rest args)
> >    "Warning message with FORMAT and ARGS."
> > @@ -3704,8 +3704,8 @@ eglot-rename
> >    "Rename the current symbol to NEWNAME."
> >    (interactive
> >     (list (read-from-minibuffer
> > -          (format "Rename `%s' to: " (or (thing-at-point 'symbol t)
> > -                                         "unknown symbol"))
> > +          (format-prompt "Rename `%s' to" nil (or (thing-at-point 'symbol t)
> > +                                                  "unknown symbol"))
> >            nil nil nil nil
> >            (symbol-name (symbol-at-point)))))
> >    (eglot-server-capable-or-lose :renameProvider)
>
> Hmm... in the first two changes, how do we know that the quote
> characters are indeed quoting stuff in the context where we substitute
> quotes according to quoting-style?  Those functions get strings they
> know nothing about, so it looks a bit risky to blindly convert any
> quoting characters there.  Or what am I missing?

You’re right that it may be risky; I iterated through the usages of these
functions and nothing stood out to me as a potential location where some
unexpected behaviour might occur, although it is true that there is the
possibility.

Not sure why I didn’t think of this when creating the patch, but it’s
probably a much better idea to change this:

    (substitute-quotes (apply #'format format args))

to this:

    (apply #'format (substitute-quotes format) args)

This limits the scope of the substitution to just the format strings,
where we know no funny business is going on.  Unless you have any other
reservations I could submit a v2 of the patch to do that.

-- 
— Thomas

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

[Eli Zaretskii]

> Date: Wed, 25 Sep 2024 20:03:20 +0200
> From: "Thomas Voss" <mail@thomasvoss.com>
> Cc: <73472@debbugs.gnu.org>
> 
> Not sure why I didn’t think of this when creating the patch, but it’s
> probably a much better idea to change this:
> 
>     (substitute-quotes (apply #'format format args))
> 
> to this:
> 
>     (apply #'format (substitute-quotes format) args)
> 
> This limits the scope of the substitution to just the format strings,
> where we know no funny business is going on.

That is of course better, but do we indeed know that format will never
use quote characters for something that is not quoting?

I think the caller should itself use substitute-quotes where
appropriate.

And I'd like to hear João's opinion as well.

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

[João Távora]

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

On Thu, Sep 26, 2024, 06:52 Eli Zaretskii <eliz@gnu.org> wrote:

I think the caller should itself use substitute-quotes where
> appropriate.
>
> And I'd like to hear João's opinion as well.
>

I think I think so too. What Eglot format strings are affected by this
quote substitution? I.e. what problem is being solved here?

João

>

[-- Attachment #2: Type: text/html, Size: 958 bytes --]

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 26 Sep 2024 07:11:05 +0100
> Cc: Thomas Voss <mail@thomasvoss.com>, 73472@debbugs.gnu.org
> 
> On Thu, Sep 26, 2024, 06:52 Eli Zaretskii <eliz@gnu.org> wrote:
> 
>  I think the caller should itself use substitute-quotes where
>  appropriate.
> 
>  And I'd like to hear João's opinion as well.
> 
> I think I think so too. What Eglot format strings are affected by this quote substitution? I.e. what problem is
> being solved here? 

Ping!  Thomas, could you please answer João's questions?

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

[João Távora]

Thomas already answered, but only to me, I see:

> There are a number of them.  For example by simply enabling eglot-mode
> you’ll see a message in your minibuffer using `these quotes' which looks
> very out-of-place when the rest of the system is using ‘these quotes’.

So, if you can enumerate them, what about using quote substitution for
only those?

João

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

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

Hi,

> > There are a number of them.  For example by simply enabling eglot-mode
> > you’ll see a message in your minibuffer using `these quotes' which looks
> > very out-of-place when the rest of the system is using ‘these quotes’.
>
> So, if you can enumerate them, what about using quote substitution for
> only those?

I can do that.  I refrained from it at first because my original patch
seemed like a much simpler solution, but I do agree that this is probably
a better approach.  When I have the time I can send through a patch for
that.
-- 
— Thomas

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

[Eli Zaretskii]

> Date: Mon, 07 Oct 2024 10:12:00 +0200
> Cc: <73472@debbugs.gnu.org>
> From: "Thomas Voss" <mail@thomasvoss.com>
> 
> > > There are a number of them.  For example by simply enabling eglot-mode
> > > you’ll see a message in your minibuffer using `these quotes' which looks
> > > very out-of-place when the rest of the system is using ‘these quotes’.
> >
> > So, if you can enumerate them, what about using quote substitution for
> > only those?
> 
> I can do that.  I refrained from it at first because my original patch
> seemed like a much simpler solution, but I do agree that this is probably
> a better approach.  When I have the time I can send through a patch for
> that.

Thanks, please do.

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

[Eli Zaretskii]

> Cc: 73472@debbugs.gnu.org, joaotavora@gmail.com
> Date: Mon, 07 Oct 2024 14:32:11 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Mon, 07 Oct 2024 10:12:00 +0200
> > Cc: <73472@debbugs.gnu.org>
> > From: "Thomas Voss" <mail@thomasvoss.com>
> > 
> > > > There are a number of them.  For example by simply enabling eglot-mode
> > > > you’ll see a message in your minibuffer using `these quotes' which looks
> > > > very out-of-place when the rest of the system is using ‘these quotes’.
> > >
> > > So, if you can enumerate them, what about using quote substitution for
> > > only those?
> > 
> > I can do that.  I refrained from it at first because my original patch
> > seemed like a much simpler solution, but I do agree that this is probably
> > a better approach.  When I have the time I can send through a patch for
> > that.
> 
> Thanks, please do.

Ping!

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

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

> > > I can do that.  I refrained from it at first because my original patch
> > > seemed like a much simpler solution, but I do agree that this is probably
> > > a better approach.  When I have the time I can send through a patch for
> > > that.
> > 
> > Thanks, please do.
>
> Ping!

I’ll see if I can send through a patch this weekend.  I’ve been a bit
busy with life things (I moved countries recently) but I’ve got most of
everything sorted now.

-- 
— Thomas

bug#73472: [PATCH] Substitute quotation marks in Eglot messages

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

[-- Attachment #1.1: Type: text/plain, Size: 677 bytes --]

On Sat Oct 19, 2024 at 10:33 AM CEST, Thomas Voss wrote:
> > > > I can do that.  I refrained from it at first because my original patch
> > > > seemed like a much simpler solution, but I do agree that this is probably
> > > > a better approach.  When I have the time I can send through a patch for
> > > > that.
> > > 
> > > Thanks, please do.
> >
> > Ping!
>
> I’ll see if I can send through a patch this weekend.  I’ve been a bit
> busy with life things (I moved countries recently) but I’ve got most of
> everything sorted now.

Hi,

I had some time today to quickly put together a patch.  You can find it
attachted to this email.

-- 
— Thomas

[-- Attachment #2: v2-0001-Substitute-quotation-marks-in-Eglot-messages.patch --]
[-- Type: text/x-patch, Size: 9629 bytes --]

From 5121734428e3e3706378b1f1af49e944f2b48658 Mon Sep 17 00:00:00 2001
From: Thomas Voss <mail@thomasvoss.com>
Date: Sat, 19 Oct 2024 11:12:30 +0200
Subject: [PATCH v2] Substitute quotation marks in Eglot messages

* lisp/progmodes/eglot.el
  (eglot--check-object)
  (eglot-register-capability)
  (eglot-unregister-capability)
  (eglot-server-capable-or-lose)
  (eglot--guess-contact)
  (eglot--connect)
  (eglot-handle-notification)
  (eglot--apply-text-edits): Wrap user-facing
  messages in `substitute-quotes'.
* lisp/progmodes/eglot.el (eglot-rename): Format the prompt using
  `format-prompt'.

Copyright-paperwork-exempt: yes
---
 lisp/progmodes/eglot.el | 86 ++++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 36 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 0a14146..f02a1d8 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -713,19 +713,22 @@ eglot--check-object
     (when-let ((missing (and enforce-required
                              (cl-set-difference required-keys
                                                 (eglot--plist-keys object)))))
-      (eglot--error "A `%s' must have %s" interface-name missing))
+      (eglot--error (substitute-quotes "A `%s' must have %s")
+                    interface-name missing))
     (when-let ((excess (and disallow-non-standard
                             (cl-set-difference
                              (eglot--plist-keys object)
                              (append required-keys optional-keys)))))
-      (eglot--error "A `%s' mustn't have %s" interface-name excess))
+      (eglot--error (substitute-quotes "A `%s' mustn't have %s")
+                    interface-name excess))
     (when check-types
       (cl-loop
        for (k v) on object by #'cddr
        for type = (or (cdr (assoc k types)) t) ;; FIXME: enforce nil type?
        unless (cl-typep v type)
-       do (eglot--error "A `%s' must have a %s as %s, but has %s"
-                        interface-name)))
+       do (eglot--error
+           (substitute-quotes "A `%s' must have a %s as %s, but has %s")
+           interface-name)))
     t))
 
 (eval-and-compile
@@ -917,15 +920,17 @@ eglot-register-capability
   "Ask SERVER to register capability METHOD marked with ID."
   (:method
    (_s method _id &rest _params)
-   (eglot--warn "Server tried to register unsupported capability `%s'"
-                method)))
+   (eglot--warn
+    (substitute-quotes "Server tried to register unsupported capability `%s'")
+    method)))
 
 (cl-defgeneric eglot-unregister-capability (server method id &rest params)
   "Ask SERVER to register capability METHOD marked with ID."
   (:method
    (_s method _id &rest _params)
-   (eglot--warn "Server tried to unregister unsupported capability `%s'"
-                method)))
+   (eglot--warn
+    (substitute-quotes "Server tried to unregister unsupported capability `%s'")
+    method)))
 
 (cl-defgeneric eglot-client-capabilities (server)
   "What the Eglot LSP client supports for SERVER."
@@ -1144,8 +1149,9 @@ eglot-server-capable-or-lose
   "Like `eglot-server-capable', but maybe error out."
   (let ((retval (apply #'eglot-server-capable feats)))
     (unless retval
-      (eglot--error "Unsupported or ignored LSP capability `%s'"
-                    (mapconcat #'symbol-name feats " ")))
+      (eglot--error
+       (substitute-quotes "Unsupported or ignored LSP capability `%s'")
+       (mapconcat #'symbol-name feats " ")))
     retval))
 
 \f
@@ -1290,7 +1296,9 @@ eglot--guess-contact
               (mapcar #'symbol-name (eglot--all-major-modes)) nil t
               guessed-mode-name nil guessed-mode-name nil)))
            ((not guessed-mode)
-            (eglot--error "Can't guess mode to manage for `%s'" (current-buffer)))
+            (eglot--error
+             (substitute-quotes "Can't guess mode to manage for `%s'")
+             (current-buffer)))
            (t guessed-mode)))
          (languages-and-contact (eglot--lookup-mode main-mode))
          (managed-modes (mapcar #'car (car languages-and-contact)))
@@ -1320,24 +1328,25 @@ eglot--guess-contact
                (combine-and-quote-strings guess)))
          (prompt
           (and base-prompt
-               (cond (current-prefix-arg base-prompt)
-                     ((null guess)
-                      (format "[eglot] Couldn't guess LSP server for `%s'\n%s"
-                              main-mode base-prompt))
-                     ((and program
-                           (not (file-name-absolute-p program))
-                           (not (compat-call executable-find program t)))
-                      (if full-program-invocation
-                          (concat (format "[eglot] I guess you want to run `%s'"
-                                          full-program-invocation)
-                                  (format ", but I can't find `%s' in PATH!"
-                                          program)
-                                  "\n" base-prompt)
-                        (eglot--error
-                         (concat "`%s' not found in PATH, but can't form"
-                                 " an interactive prompt for help you fix"
-                                 " this.")
-                         program guess))))))
+               (substitute-quotes
+                (cond (current-prefix-arg base-prompt)
+                      ((null guess)
+                       (format "[eglot] Couldn't guess LSP server for `%s'\n%s"
+                               main-mode base-prompt))
+                      ((and program
+                            (not (file-name-absolute-p program))
+                            (not (compat-call executable-find program t)))
+                       (if full-program-invocation
+                           (concat (format "[eglot] I guess you want to run `%s'"
+                                           full-program-invocation)
+                                   (format ", but I can't find `%s' in PATH!"
+                                           program)
+                                   "\n" base-prompt)
+                         (eglot--error
+                          (concat "`%s' not found in PATH, but can't form"
+                                  " an interactive prompt for help you fix"
+                                  " this.")
+                          program guess)))))))
          (input (and prompt (read-shell-command prompt
                                                 full-program-invocation
                                                 'eglot-command-history)))
@@ -1661,12 +1670,13 @@ eglot--connect
                                            (null eglot-autoreconnect)))))))
                           (run-hook-with-args 'eglot-connect-hook server)
                           (eglot--message
-                           "Connected! Server `%s' now managing `%s' buffers \
+                           (substitute-quotes
+                            "Connected! Server `%s' now managing `%s' buffers \
 in project `%s'."
                            (or (plist-get serverInfo :name)
                                (jsonrpc-name server))
                            managed-modes
-                           (eglot-project-nickname server))
+                           (eglot-project-nickname server)))
                           (when tag (throw tag t))))
                       :timeout eglot-connect-timeout
                       :error-fn (eglot--lambda ((ResponseError) code message)
@@ -1689,7 +1699,8 @@ eglot--connect
                                       nil eglot-connect-timeout)))))))
               (pcase retval
                 (`(error . ,msg) (eglot--error msg))
-                (`nil (eglot--message "Waiting in background for server `%s'"
+                (`nil (eglot--message (substitute-quotes
+                                       "Waiting in background for server `%s'")
                                       (jsonrpc-name server))
                       nil)
                 (_ server)))
@@ -2332,7 +2343,9 @@ eglot-handle-notification
   "Handle unknown notification."
   (unless (or (string-prefix-p "$" (format "%s" method))
               (not (memq 'disallow-unknown-methods eglot-strict-mode)))
-    (eglot--warn "Server sent unknown notification method `%s'" method)))
+    (eglot--warn (substitute-quotes
+                  "Server sent unknown notification method `%s'")
+                 method)))
 
 (cl-defmethod eglot-handle-request
   (_server method &key &allow-other-keys)
@@ -3586,7 +3599,8 @@ eglot--apply-text-edits
            (howmany (length edits))
            (reporter (unless silent
                        (make-progress-reporter
-                        (format "[eglot] applying %s edits to `%s'..."
+                        (format (substitute-quotes
+                                 "[eglot] applying %s edits to `%s'...")
                                 howmany (current-buffer))
                         0 howmany)))
            (done 0))
@@ -3708,8 +3722,8 @@ eglot-rename
   "Rename the current symbol to NEWNAME."
   (interactive
    (list (read-from-minibuffer
-          (format "Rename `%s' to: " (or (thing-at-point 'symbol t)
-                                         "unknown symbol"))
+          (format-prompt "Rename `%s' to" nil (or (thing-at-point 'symbol t)
+                                                  "unknown symbol"))
           nil nil nil nil
           (symbol-name (symbol-at-point)))))
   (eglot-server-capable-or-lose :renameProvider)
-- 
2.47.0