bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

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

When moving by sexp (C-M-f, C-M-u and so on) and point is already at a boundary preventing further movement, Emacs currently signals an internal error such as

 Scan error: "Containing expression ends prematurely", 5010, 5010

or

 Scan error: "Unbalanced parentheses", 5010, 1

which is unhelpful and rather looks as if something went wrong in the internal machinery.

The attached patch does away with this error when the commands are invoked interactively; programmatic use of the functions will get the scan-error just like before. There didn't seem to be much point in replacing the errors with new messages so the current version of the patch doesn't.


[-- Attachment #2: 0001-Don-t-signal-scan-error-when-moving-by-sexp-interact.patch --]
[-- Type: application/octet-stream, Size: 6173 bytes --]

From 25a03c7ec547984b2e500d4e5be7855290ae95ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 18 Sep 2020 12:49:33 +0200
Subject: [PATCH] Don't signal scan-error when moving by sexp interactively

* lisp/emacs-lisp/lisp.el (forward-sexp, backward-sexp, forward-list)
(backward-list, down-list, up-list): Remove unsightly scan-error
when running interactively and no further movement by sexp can be made.
---
 lisp/emacs-lisp/lisp.el | 84 +++++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 32 deletions(-)

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index 8c18557c79..2b0ed76633 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -55,7 +55,7 @@ forward-sexp-function
   "If non-nil, `forward-sexp' delegates to this function.
 Should take the same arguments and behave similarly to `forward-sexp'.")
 
-(defun forward-sexp (&optional arg)
+(defun forward-sexp (&optional arg noerror)
   "Move forward across one balanced expression (sexp).
 With ARG, do it that many times.  Negative arg -N means move
 backward across N balanced expressions.  This command assumes
@@ -64,23 +64,29 @@ forward-sexp
 If unable to move over a sexp, signal `scan-error' with three
 arguments: a message, the start of the obstacle (usually a
 parenthesis or list marker of some kind), and end of the
-obstacle."
-  (interactive "^p")
-  (or arg (setq arg 1))
-  (if forward-sexp-function
-      (funcall forward-sexp-function arg)
-    (goto-char (or (scan-sexps (point) arg) (buffer-end arg)))
-    (if (< arg 0) (backward-prefix-chars))))
-
-(defun backward-sexp (&optional arg)
+obstacle.  If NOERROR is non-nil, as it is interactively,
+do not signal an error."
+  (interactive "^p\nd")
+  (if noerror
+      (condition-case _
+          (forward-sexp arg nil)
+        (scan-error (ding)))
+    (or arg (setq arg 1))
+    (if forward-sexp-function
+        (funcall forward-sexp-function arg)
+      (goto-char (or (scan-sexps (point) arg) (buffer-end arg)))
+      (if (< arg 0) (backward-prefix-chars)))))
+
+(defun backward-sexp (&optional arg noerror)
   "Move backward across one balanced expression (sexp).
 With ARG, do it that many times.  Negative arg -N means
 move forward across N balanced expressions.
 This command assumes point is not in a string or comment.
-Uses `forward-sexp' to do the work."
-  (interactive "^p")
+Uses `forward-sexp' to do the work.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
   (or arg (setq arg 1))
-  (forward-sexp (- arg)))
+  (forward-sexp (- arg) noerror))
 
 (defun mark-sexp (&optional arg allow-extend)
   "Set mark ARG sexps from point.
@@ -108,41 +114,52 @@ mark-sexp
 	    (point))
 	  nil t))))
 
-(defun forward-list (&optional arg)
+(defun forward-list (&optional arg noerror)
   "Move forward across one balanced group of parentheses.
 This command will also work on other parentheses-like expressions
 defined by the current language mode.
 With ARG, do it that many times.
 Negative arg -N means move backward across N groups of parentheses.
-This command assumes point is not in a string or comment."
-  (interactive "^p")
-  (or arg (setq arg 1))
-  (goto-char (or (scan-lists (point) arg 0) (buffer-end arg))))
-
-(defun backward-list (&optional arg)
+This command assumes point is not in a string or comment.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
+  (if noerror
+      (condition-case _
+          (forward-list arg nil)
+        (scan-error (ding)))
+    (or arg (setq arg 1))
+    (goto-char (or (scan-lists (point) arg 0) (buffer-end arg)))))
+
+(defun backward-list (&optional arg noerror)
   "Move backward across one balanced group of parentheses.
 This command will also work on other parentheses-like expressions
 defined by the current language mode.
 With ARG, do it that many times.
 Negative arg -N means move forward across N groups of parentheses.
-This command assumes point is not in a string or comment."
-  (interactive "^p")
+This command assumes point is not in a string or comment.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
   (or arg (setq arg 1))
-  (forward-list (- arg)))
+  (forward-list (- arg) noerror))
 
-(defun down-list (&optional arg)
+(defun down-list (&optional arg noerror)
   "Move forward down one level of parentheses.
 This command will also work on other parentheses-like expressions
 defined by the current language mode.
 With ARG, do this that many times.
 A negative argument means move backward but still go down a level.
-This command assumes point is not in a string or comment."
-  (interactive "^p")
-  (or arg (setq arg 1))
-  (let ((inc (if (> arg 0) 1 -1)))
-    (while (/= arg 0)
-      (goto-char (or (scan-lists (point) inc -1) (buffer-end arg)))
-      (setq arg (- arg inc)))))
+This command assumes point is not in a string or comment.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
+  (if noerror
+      (condition-case _
+          (down-list arg nil)
+        (scan-error (ding)))
+    (or arg (setq arg 1))
+    (let ((inc (if (> arg 0) 1 -1)))
+      (while (/= arg 0)
+        (goto-char (or (scan-lists (point) inc -1) (buffer-end arg)))
+        (setq arg (- arg inc))))))
 
 (defun backward-up-list (&optional arg escape-strings no-syntax-crossing)
   "Move backward out of one level of parentheses.
@@ -229,7 +246,10 @@ up-list
                  (or (< inc 0)
                      (forward-comment 1))
                  (setf arg (+ arg inc)))
-            (signal (car err) (cdr err))))))
+            (if no-syntax-crossing
+                ;; Assume called interactively; don't signal an error.
+                (ding)
+              (signal (car err) (cdr err)))))))
       (setq arg (- arg inc)))))
 
 (defun kill-sexp (&optional arg)
-- 
2.21.1 (Apple Git-122.3)


[-- Attachment #3: Type: text/plain, Size: 2 bytes --]

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> When moving by sexp (C-M-f, C-M-u and so on) and point is already at a
> boundary preventing further movement, Emacs currently signals an
> internal error such as
>
>  Scan error: "Containing expression ends prematurely", 5010, 5010
>
> or
>
>  Scan error: "Unbalanced parentheses", 5010, 1
>
> which is unhelpful and rather looks as if something went wrong in the
> internal machinery.

Yes, those error messages are confusing in interactive usage.

> The attached patch does away with this error when the commands are
> invoked interactively; programmatic use of the functions will get the
> scan-error just like before. There didn't seem to be much point in
> replacing the errors with new messages so the current version of the
> patch doesn't.

[...]

> +  (if noerror
> +      (condition-case _
> +          (forward-list arg nil)
> +        (scan-error (ding)))

So you basically just `ding' in interactive usage?

I wonder whether this would have any negative effect when people are
using these commands in keyboard macros.  For instance, if you've
recorded a macro that does `M-C-f M-DEL' or something, previously it
would signal an error and then stop, while now it'll just continue and
delete the wrong thing?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Dmitry Gutov]

On 18.09.2020 16:13, Lars Ingebrigtsen wrote:
>> When moving by sexp (C-M-f, C-M-u and so on) and point is already at a
>> boundary preventing further movement, Emacs currently signals an
>> internal error such as
>>
>>   Scan error: "Containing expression ends prematurely", 5010, 5010
>>
>> or
>>
>>   Scan error: "Unbalanced parentheses", 5010, 1
>>
>> which is unhelpful and rather looks as if something went wrong in the
>> internal machinery.
> Yes, those error messages are confusing in interactive usage.

They might be kinda helpful for interactive debugging, though?

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

>>>   Scan error: "Containing expression ends prematurely", 5010, 5010
>>>
>>> or
>>>
>>>   Scan error: "Unbalanced parentheses", 5010, 1
>>>
>>> which is unhelpful and rather looks as if something went wrong in the
>>> internal machinery.
>> Yes, those error messages are confusing in interactive usage.
>
> They might be kinda helpful for interactive debugging, though?

They don't seem like user-level error messages (as you'd expect from a
command like `C-M-f'), and if you're debugging, you're probably calling
`M-: (forward-sexp 1)' or something?  (In which case you get these error
messages still with the patch in question.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Dmitry Gutov]

On 18.09.2020 16:42, Lars Ingebrigtsen wrote:
> They don't seem like user-level error messages (as you'd expect from a
> command like `C-M-f'), and if you're debugging, you're probably calling
> `M-: (forward-sexp 1)' or something?  (In which case you get these error
> messages still with the patch in question.)

Some translation to better-looking messages would be an improvement, for 
sure.

Up until now, M-x forward-sexp behaved the same as (forward-sexp 1), so 
I'd guess there are some Lisp developers used to that.

It's not a very strong objection, though.

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

18 sep. 2020 kl. 15.13 skrev Lars Ingebrigtsen <larsi@gnus.org>:

First of all, thanks for looking at the patch!

> So you basically just `ding' in interactive usage?

Right. I probably owe you a deeper explanation! Please bear with me for a moment.

It would be acceptable to replace the dings with (user-error "appropriate message"); it would still be an improvement. However:

I'm a firm believer in positive design. Features should be motivated by their actual value rather than habit or tradition. From the user's point of view, it is not an error when the cursor refuses to move beyond its bounds. No other editor (except one) displays a message in these cases, and many don't even beep. The only exception I've found is ed, which should delight everybody.

These messages don't make the editor easier to use in any way; it is crystal clear what the reason is when the cursor doesn't move at the edge of the {line, buffer, sexp, list, ...}. I'd say the contrary: they are nuisance messages that obscure the echo area, clutter the *Messages* buffer, and needlessly cause distractive movement in the visual periphery (a big no-no for any serious industrial UI designer).

In fact, several of the commands in question don't even beep at the boundaries in some cases: for example, C-M-f after the last sexp of the buffer jumps to end-of-buffer and silently stays there. Should we add noise messages for such cases? Surely not.

In other words: I'm not strongly against messages instead of dings if that is the condition for applying the patch, but would like to hear the benefit of those messages argued positively.

There, I'm better now. And here's a hot cuppa, lovely.

> I wonder whether this would have any negative effect when people are
> using these commands in keyboard macros.  For instance, if you've
> recorded a macro that does `M-C-f M-DEL' or something, previously it
> would signal an error and then stop, while now it'll just continue and
> delete the wrong thing?

Actually, (ding) interrupts keyboard macros, so this does work.

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> In fact, several of the commands in question don't even beep at the
> boundaries in some cases: for example, C-M-f after the last sexp of
> the buffer jumps to end-of-buffer and silently stays there. Should we
> add noise messages for such cases? Surely not.

Yeah, that is pretty inconsistent.

> In other words: I'm not strongly against messages instead of dings if
> that is the condition for applying the patch, but would like to hear
> the benefit of those messages argued positively.

Emacs does signal errors a lot more in editing than other editors, it's
true -- for instance, `left' at the beginning of the buffer.

> There, I'm better now. And here's a hot cuppa, lovely.

:-)

>> I wonder whether this would have any negative effect when people are
>> using these commands in keyboard macros.  For instance, if you've
>> recorded a macro that does `M-C-f M-DEL' or something, previously it
>> would signal an error and then stop, while now it'll just continue and
>> delete the wrong thing?
>
> Actually, (ding) interrupts keyboard macros, so this does work.

Ah, I'd forgotten that.

Still, I'm not sure whether a (ding) is more helpful than a non-cryptic
user-error message in these instances.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

18 sep. 2020 kl. 17.23 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Still, I'm not sure whether a (ding) is more helpful than a non-cryptic
> user-error message in these instances.

That's a fair view, but my point is that we shouldn't put in a message unless we can honestly argue for its usefulness. In this case it seems difficult to do so given the evidence. It's not like every other editor is more difficult to use in this particular aspect.

Or put in another way: the absence of a message isn't necessarily cryptic. When the editor explains itself by the result of the user's action, a verbal message on top of it can make matters worse.

By the way, (ding) and (user-error "") seem more or less equivalent, in case one would be preferable.

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> 18 sep. 2020 kl. 17.23 skrev Lars Ingebrigtsen <larsi@gnus.org>:
>
>> Still, I'm not sure whether a (ding) is more helpful than a non-cryptic
>> user-error message in these instances.
>
> That's a fair view, but my point is that we shouldn't put in a message
> unless we can honestly argue for its usefulness. In this case it seems
> difficult to do so given the evidence. It's not like every other
> editor is more difficult to use in this particular aspect.

The movement commands Emacs has here are more complicated than most
editors have, though.  Saying why `C-M-f' can't move seems like useful
information for the user to receive.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

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

19 sep. 2020 kl. 16.13 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> The movement commands Emacs has here are more complicated than most
> editors have, though.  Saying why `C-M-f' can't move seems like useful
> information for the user to receive.

Well, but is that true? There is no evidence for it. Emacs currently gives no useful information in such situations and never has ('scan-error' indeed). The previously posted patch can only improve matters.

Of course we can put in messages for the at-bounds cases; it would look something like the patch below. While it would still be an improvement over what Emacs currently does, it is still a statement that we value tradition over usability. Try both!

Another alternative would be the standard Emacs compromise: add a defcustom. Since it could control the message emission by other movement commands like backward-char and next-line, it's perhaps not a bad idea after all...

It wouldn't need to do anything about movement by word or sentence since they already seem to be fixed, good!


[-- Attachment #2: move-by-sexp--user-error.patch --]
[-- Type: application/octet-stream, Size: 6488 bytes --]

From ddbbaf7cdc922968ee5c2df614bc273c8d879a9a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 18 Sep 2020 12:49:33 +0200
Subject: [PATCH] Don't signal scan-error when moving by sexp interactively

* lisp/emacs-lisp/lisp.el (forward-sexp, backward-sexp, forward-list)
(backward-list, down-list, up-list): Remove unsightly scan-error
when running interactively and no further movement by sexp can be made.
---
 lisp/emacs-lisp/lisp.el | 88 ++++++++++++++++++++++++++---------------
 1 file changed, 56 insertions(+), 32 deletions(-)

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index 8c18557c79..a0b86d2fd1 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -55,7 +55,7 @@ forward-sexp-function
   "If non-nil, `forward-sexp' delegates to this function.
 Should take the same arguments and behave similarly to `forward-sexp'.")
 
-(defun forward-sexp (&optional arg)
+(defun forward-sexp (&optional arg noerror)
   "Move forward across one balanced expression (sexp).
 With ARG, do it that many times.  Negative arg -N means move
 backward across N balanced expressions.  This command assumes
@@ -64,23 +64,31 @@ forward-sexp
 If unable to move over a sexp, signal `scan-error' with three
 arguments: a message, the start of the obstacle (usually a
 parenthesis or list marker of some kind), and end of the
-obstacle."
-  (interactive "^p")
-  (or arg (setq arg 1))
-  (if forward-sexp-function
-      (funcall forward-sexp-function arg)
-    (goto-char (or (scan-sexps (point) arg) (buffer-end arg)))
-    (if (< arg 0) (backward-prefix-chars))))
-
-(defun backward-sexp (&optional arg)
+obstacle.  If NOERROR is non-nil, as it is interactively,
+do not signal an error."
+  (interactive "^p\nd")
+  (if noerror
+      (condition-case _
+          (forward-sexp arg nil)
+        (scan-error (user-error (if (> arg 0)
+                                    "No next sexp"
+                                  "No previous sexp"))))
+    (or arg (setq arg 1))
+    (if forward-sexp-function
+        (funcall forward-sexp-function arg)
+      (goto-char (or (scan-sexps (point) arg) (buffer-end arg)))
+      (if (< arg 0) (backward-prefix-chars)))))
+
+(defun backward-sexp (&optional arg noerror)
   "Move backward across one balanced expression (sexp).
 With ARG, do it that many times.  Negative arg -N means
 move forward across N balanced expressions.
 This command assumes point is not in a string or comment.
-Uses `forward-sexp' to do the work."
-  (interactive "^p")
+Uses `forward-sexp' to do the work.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
   (or arg (setq arg 1))
-  (forward-sexp (- arg)))
+  (forward-sexp (- arg) noerror))
 
 (defun mark-sexp (&optional arg allow-extend)
   "Set mark ARG sexps from point.
@@ -108,41 +116,54 @@ mark-sexp
 	    (point))
 	  nil t))))
 
-(defun forward-list (&optional arg)
+(defun forward-list (&optional arg noerror)
   "Move forward across one balanced group of parentheses.
 This command will also work on other parentheses-like expressions
 defined by the current language mode.
 With ARG, do it that many times.
 Negative arg -N means move backward across N groups of parentheses.
-This command assumes point is not in a string or comment."
-  (interactive "^p")
-  (or arg (setq arg 1))
-  (goto-char (or (scan-lists (point) arg 0) (buffer-end arg))))
-
-(defun backward-list (&optional arg)
+This command assumes point is not in a string or comment.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
+  (if noerror
+      (condition-case _
+          (forward-list arg nil)
+        (scan-error (user-error (if (> arg 0)
+                                    "No next expression"
+                                  "No previous expression"))))
+    (or arg (setq arg 1))
+    (goto-char (or (scan-lists (point) arg 0) (buffer-end arg)))))
+
+(defun backward-list (&optional arg noerror)
   "Move backward across one balanced group of parentheses.
 This command will also work on other parentheses-like expressions
 defined by the current language mode.
 With ARG, do it that many times.
 Negative arg -N means move forward across N groups of parentheses.
-This command assumes point is not in a string or comment."
-  (interactive "^p")
+This command assumes point is not in a string or comment.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
   (or arg (setq arg 1))
-  (forward-list (- arg)))
+  (forward-list (- arg) noerror))
 
-(defun down-list (&optional arg)
+(defun down-list (&optional arg noerror)
   "Move forward down one level of parentheses.
 This command will also work on other parentheses-like expressions
 defined by the current language mode.
 With ARG, do this that many times.
 A negative argument means move backward but still go down a level.
-This command assumes point is not in a string or comment."
-  (interactive "^p")
-  (or arg (setq arg 1))
-  (let ((inc (if (> arg 0) 1 -1)))
-    (while (/= arg 0)
-      (goto-char (or (scan-lists (point) inc -1) (buffer-end arg)))
-      (setq arg (- arg inc)))))
+This command assumes point is not in a string or comment.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
+  (if noerror
+      (condition-case _
+          (down-list arg nil)
+        (scan-error (user-error "At bottom level")))
+    (or arg (setq arg 1))
+    (let ((inc (if (> arg 0) 1 -1)))
+      (while (/= arg 0)
+        (goto-char (or (scan-lists (point) inc -1) (buffer-end arg)))
+        (setq arg (- arg inc))))))
 
 (defun backward-up-list (&optional arg escape-strings no-syntax-crossing)
   "Move backward out of one level of parentheses.
@@ -229,7 +250,10 @@ up-list
                  (or (< inc 0)
                      (forward-comment 1))
                  (setf arg (+ arg inc)))
-            (signal (car err) (cdr err))))))
+            (if no-syntax-crossing
+                ;; Assume called interactively; don't signal an error.
+                (user-error "At top level")
+              (signal (car err) (cdr err)))))))
       (setq arg (- arg inc)))))
 
 (defun kill-sexp (&optional arg)
-- 
2.21.1 (Apple Git-122.3)

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Well, but is that true? There is no evidence for it. Emacs currently
> gives no useful information in such situations and never has
> ('scan-error' indeed). The previously posted patch can only improve
> matters.

Even if the error looked like an internal error, the bit in the middle
is informative:

forward-sexp: Scan error: "Containing expression ends prematurely", 2076, 2077

> Of course we can put in messages for the at-bounds cases; it would
> look something like the patch below. While it would still be an
> improvement over what Emacs currently does, it is still a statement
> that we value tradition over usability. Try both!

OK, tried it now.  It's less confusing on the whole, except this bit:

> +  (if noerror
> +      (condition-case _
> +          (forward-sexp arg nil)
> +        (scan-error (user-error (if (> arg 0)
> +                                    "No next sexp"
> +                                  "No previous sexp"))))

Hitting `C-M-f' at the start of this line

)()

gives you that error, but there is indeed more sexps in the buffer.  The
error is that the we can't progress because we reached the ")" without
having a "(".

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Dmitry Gutov]

On 20.09.2020 20:33, Mattias Engdegård wrote:
> While it would still be an improvement over what Emacs currently does, it is still a statement that we value tradition over usability. Try both!

I think that is certainly the case, as a multitude of discussions on 
emacs-devel has shown.

It's kinda weird that you chose this particular bit of functionality as 
an example, though. There are no counterparts to this in other editors, 
so there's no real usability research, and we don't really know which 
approach is the most usable.

I'd really prefer we examine both and make a considered choice, rather 
than add a user option.

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[João Távora]

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

On Fri, Sep 18, 2020 at 2:14 PM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Mattias Engdegård <mattiase@acm.org> writes:
>
> > When moving by sexp (C-M-f, C-M-u and so on) and point is already at a
> > boundary preventing further movement, Emacs currently signals an
> > internal error such as
> >
> >  Scan error: "Containing expression ends prematurely", 5010, 5010
> >
> > or
> >
> >  Scan error: "Unbalanced parentheses", 5010, 1
> >
> > which is unhelpful and rather looks as if something went wrong in the
> > internal machinery.
>
> Yes, those error messages are confusing in interactive usage.
>

I disagree strongly.  Maybe the integers following them and
containing character positions are confusing, but the messages
themselves are fine.

Here's my use case.  When selecting a certain amount of forms
inside another one, but not an outermost one, I type a lot of M-PC
until I see the "Containing expression..." message.  I actively look
out for it.  When it pops up, the region persists. I can then C-w
or do whatever, keeping that part of the code balanced. If, however,
the message is "Unbalanced parenthesis", I know something's up
and have to fix it.

Please don't remove the message and/or conflate the two cases.

João

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

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

20 sep. 2020 kl. 21.54 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Even if the error looked like an internal error, the bit in the middle
> is informative:
> 
> forward-sexp: Scan error: "Containing expression ends prematurely", 2076, 2077

That message reads either as an error in Emacs itself or in the user's code. There is typically no expression that ends prematurely when the command is invoked (there may be, but Emacs cannot know that). It seems that the message is not so much informative as outright misinforming.
The message from C-M-u, "Unbalanced parentheses", is an equally blatant lie.

> OK, tried it now.  It's less confusing on the whole, except this bit:

Thank you for testing the patch. (Less confusing than Emacs master, I presume? Or if you mean compared with the no-message patch, what was confusing about it?)

> )()
> 
> gives you that error, but there is indeed more sexps in the buffer.  The
> error is that the we can't progress because we reached the ")" without
> having a "(".

If I understand you properly, you say that "No next sexp" is an inappropriate message for C-M-f at

 ((A B_) C D)

where the underscore indicates point, since the expressions C and D follow? Editors differ in how they handle this case: some just continue up one level (in this case, past C) instead of stopping. There are merits to either behaviour and I'm not suggesting a change to Emacs in this respect.

What would a good message be then, if we insist on one? "No next sexp" is correct within the semantics of forward-sexp and to the point; a more detailed message might be something like

 "Past last element in expression"
 "No next subexpression"
 "Past last subexpression"

which may or may not be better. 'Sexp' is an Emacs-specific term which may still be appropriate in these messages since it figures prominently in the manual and the commands are defined around it. Suggestions are welcome!

Let's not forget that the empty string is also an alternative and should be compared with the verbal messages. Assuming that the user understands what C-M-f does, it seems clear enough.

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

20 sep. 2020 kl. 23.39 skrev Dmitry Gutov <dgutov@yandex.ru>:

> I think that is certainly the case, as a multitude of discussions on emacs-devel has shown.

Unfortunately yes, but what else can we do than trying to improve matters little by little? We will probably fail, but at least we have tried.

> There are no counterparts to this in other editors, so there's no real usability research, and we don't really know which approach is the most usable.

Other editors have structural movement commands although they may or may not behave identically. However, it is a common pattern almost everywhere else that verbal messages aren't displayed unless needed to convey useful information; Emacs is the clear outlier here. There is no reason to believe why one special kind of movement commands in Emacs would warrant error messages (but only sometimes).

> I'd really prefer we examine both and make a considered choice, rather than add a user option.

Very much agreed!

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Dmitry Gutov]

On 21.09.2020 14:21, Mattias Engdegård wrote:
> Other editors have structural movement commands although they may or may not behave identically.

Do they try to preserve the pairing of parentheses (or similar 
structures)? What do they say if the user tries to delete a closing 
delimiter without deleting the opening one?

> However, it is a common pattern almost everywhere else that verbal messages aren't displayed unless needed to convey useful information;

"We don't move point because of XXX" might be useful info, might it not?

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

>  >  Scan error: "Containing expression ends prematurely", 5010, 5010
>  >
>  > or
>  >
>  >  Scan error: "Unbalanced parentheses", 5010, 1
>  >
>  > which is unhelpful and rather looks as if something went wrong in the
>  > internal machinery.
>
>  Yes, those error messages are confusing in interactive usage.
>
> I disagree strongly.  Maybe the integers following them and 
> containing character positions are confusing, but the messages 
> themselves are fine.  

Yes, those are the confusing bits.  :-)  "Scan error" and ", 5010, 1"
sounds like something went wrong deep inside Emacs.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Thank you for testing the patch. (Less confusing than Emacs master, I
> presume?

Yes.

> Or if you mean compared with the no-message patch, what was confusing
> about it?)

Beeping at me without telling me why is confusing, I think.

> If I understand you properly, you say that "No next sexp" is an
> inappropriate message for C-M-f at
>
>  ((A B_) C D)
>
> where the underscore indicates point, since the expressions C and D
> follow? Editors differ in how they handle this case: some just
> continue up one level (in this case, past C) instead of
> stopping. There are merits to either behaviour and I'm not suggesting
> a change to Emacs in this respect.
>
> What would a good message be then, if we insist on one? "No next sexp"
> is correct within the semantics of forward-sexp and to the point; a
> more detailed message might be something like
>
>  "Past last element in expression"
>  "No next subexpression"
>  "Past last subexpression"

The current message is "Containing expression ends prematurely", which
isn't...  perfect...  but it tells us that it tried to read an
expression but failed, because it reached something that ended the
expression before ... it should?  

Uhm...  No, I have no good suggestions, but these new messages aren't
really any clearer than the old one.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

21 sep. 2020 kl. 10.49 skrev João Távora <joaotavora@gmail.com>:

> If, however, 
> the message is "Unbalanced parenthesis", I know something's up 
> and have to fix it.

Thanks for mentioning mark-sexp; it was accidentally omitted in my patch. I agree that this particular condition in mark-sexp merits special treatment; the reason for not being able to extend the mark may be far from the cursor and even off-screen.
It is probably worthwhile to communicate the "unbalanced parenthesis" case differently.

For those wondering what this is about: the condition arises when C-M-SPC is pressed with point before an unbalanced left bracket.

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

21 sep. 2020 kl. 16.47 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Beeping at me without telling me why is confusing, I think.

Well, better than beeping while lying to you I'd say...

> The current message is "Containing expression ends prematurely", which
> isn't...  perfect...  but it tells us that it tried to read an
> expression but failed, because it reached something that ended the
> expression before ... it should?  

Well put -- you and I, and everyone else friendly with the internal workings, have to make the effort to stop thinking like Emacs and put ourselves in the shoes of the User (who doesn't give two hoots for what happens inside the machine).

> Uhm...  No, I have no good suggestions, but these new messages aren't
> really any clearer than the old one.

Here is a long shot. If we think that "End of buffer" is a good message (I don't really) then what about

 "End of expression"
 "End of subexpressions"

The standard procedure for a designer who has trouble formulating a response to the user is to rework the operation so that the response isn't needed, but we can't really do that. Another strategy is considering whether a response is necessary at all.

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

21 sep. 2020 kl. 14.36 skrev Dmitry Gutov <dgutov@yandex.ru>:

>> Other editors have structural movement commands although they may or may not behave identically.
> 
> Do they try to preserve the pairing of parentheses (or similar structures)? What do they say if the user tries to delete a closing delimiter without deleting the opening one?

Structural editing has long history and there is great variety, all from rigid syntax-enforcing editors to plain text editors with some assisting electricity on top; it's impossible to generalise.

> "We don't move point because of XXX" might be useful info, might it not?

So one might think, but there's an opportunity cost. If the user understands what's going on (from context, behaviour etc) without a textual message, then that message is of negative value. In other words, the user would be better off without it.

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[João Távora]

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

Hi Mattias,

I haven't looked at your patch, but I hope you are taking care with
the C-M- family of commands.  It is not only mark-sexp that has
that property that it warns about something potentially off screen.
Many other C-M- commands also have that.  Think  M-7 C-M-k for
example.  I hope you understand you are touching a cornerstone
of Lisp-related (actually, not only Lisp) editing here.

So I think I can get used to differently worded messages, but the
errors themselves are pretty useful.

João



On Mon, Sep 21, 2020 at 6:12 PM Mattias Engdegård <mattiase@acm.org> wrote:

> 21 sep. 2020 kl. 10.49 skrev João Távora <joaotavora@gmail.com>:
>
> > If, however,
> > the message is "Unbalanced parenthesis", I know something's up
> > and have to fix it.
>
> Thanks for mentioning mark-sexp; it was accidentally omitted in my patch.
> I agree that this particular condition in mark-sexp merits special
> treatment; the reason for not being able to extend the mark may be far from
> the cursor and even off-screen.
> It is probably worthwhile to communicate the "unbalanced parenthesis" case
> differently.
>
> For those wondering what this is about: the condition arises when C-M-SPC
> is pressed with point before an unbalanced left bracket.
>
>

-- 
João Távora

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

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Dmitry Gutov]

On 21.09.2020 20:12, Mattias Engdegård wrote:
>> "We don't move point because of XXX" might be useful info, might it not?
> So one might think, but there's an opportunity cost. If the user understands what's going on (from context, behaviour etc) without a textual message, then that message is of negative value. In other words, the user would be better off without it.

Could be.

Although if we managed to come up with more reasonable messages, and 
keep the buffer position numbers in them, the result might be both 
friendlier and more helpful.

Though admittedly most of the benefit will come in, again, debugging 
situations.

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Here is a long shot. If we think that "End of buffer" is a good
> message (I don't really) then what about
>
>  "End of expression"
>  "End of subexpressions"

Yeah, that makes sense to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

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

22 sep. 2020 kl. 16.32 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Yeah, that makes sense to me.

Apologies for my indecision, but I'm not sure if they make sense to me.

But my personal flaws should not impede progress. How about I push what I've got (latest patch attached for reference) since we seem to agree that it's an improvement over what's in master, and if then you or anyone else want to further adjust the messages then I have no objections.


[-- Attachment #2: 0001-Don-t-signal-scan-error-when-moving-by-sexp-interact.patch --]
[-- Type: application/octet-stream, Size: 8920 bytes --]

From 3249aeba659d636d6344a7ab0e3b1ecbb83da160 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 18 Sep 2020 12:49:33 +0200
Subject: [PATCH] Don't signal scan-error when moving by sexp interactively

* lisp/emacs-lisp/lisp.el (forward-sexp, backward-sexp, forward-list)
(backward-list, down-list, up-list, mark-sexp, kill-sexp)
(backward-kill-sexp): Remove unsightly scan-error when running
interactively and no further movement by sexp can be made.
---
 lisp/emacs-lisp/lisp.el | 134 ++++++++++++++++++++++++++--------------
 1 file changed, 89 insertions(+), 45 deletions(-)

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index ac4ba78897..f74243c890 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -55,7 +55,7 @@ forward-sexp-function
   "If non-nil, `forward-sexp' delegates to this function.
 Should take the same arguments and behave similarly to `forward-sexp'.")
 
-(defun forward-sexp (&optional arg)
+(defun forward-sexp (&optional arg noerror)
   "Move forward across one balanced expression (sexp).
 With ARG, do it that many times.  Negative arg -N means move
 backward across N balanced expressions.  This command assumes
@@ -64,23 +64,31 @@ forward-sexp
 If unable to move over a sexp, signal `scan-error' with three
 arguments: a message, the start of the obstacle (usually a
 parenthesis or list marker of some kind), and end of the
-obstacle."
-  (interactive "^p")
-  (or arg (setq arg 1))
-  (if forward-sexp-function
-      (funcall forward-sexp-function arg)
-    (goto-char (or (scan-sexps (point) arg) (buffer-end arg)))
-    (if (< arg 0) (backward-prefix-chars))))
-
-(defun backward-sexp (&optional arg)
+obstacle.  If NOERROR is non-nil, as it is interactively,
+do not signal an error."
+  (interactive "^p\nd")
+  (if noerror
+      (condition-case _
+          (forward-sexp arg nil)
+        (scan-error (user-error (if (> arg 0)
+                                    "No next sexp"
+                                  "No previous sexp"))))
+    (or arg (setq arg 1))
+    (if forward-sexp-function
+        (funcall forward-sexp-function arg)
+      (goto-char (or (scan-sexps (point) arg) (buffer-end arg)))
+      (if (< arg 0) (backward-prefix-chars)))))
+
+(defun backward-sexp (&optional arg noerror)
   "Move backward across one balanced expression (sexp).
 With ARG, do it that many times.  Negative arg -N means
 move forward across N balanced expressions.
 This command assumes point is not in a string or comment.
-Uses `forward-sexp' to do the work."
-  (interactive "^p")
+Uses `forward-sexp' to do the work.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
   (or arg (setq arg 1))
-  (forward-sexp (- arg)))
+  (forward-sexp (- arg) noerror))
 
 (defun mark-sexp (&optional arg allow-extend)
   "Set mark ARG sexps from point.
@@ -99,50 +107,75 @@ mark-sexp
 	 (set-mark
 	  (save-excursion
 	    (goto-char (mark))
-	    (forward-sexp arg)
+            (condition-case error
+	        (forward-sexp arg)
+              (scan-error
+               (user-error (if (equal (cadr error)
+                                      "Containing expression ends prematurely")
+                               "No more sexp to select"
+                             (cadr error)))))
 	    (point))))
 	(t
 	 (push-mark
 	  (save-excursion
-	    (forward-sexp (prefix-numeric-value arg))
+            (condition-case error
+	        (forward-sexp (prefix-numeric-value arg))
+              (scan-error
+               (user-error (if (equal (cadr error)
+                                      "Containing expression ends prematurely")
+                               "No sexp to select"
+                             (cadr error)))))
 	    (point))
 	  nil t))))
 
-(defun forward-list (&optional arg)
+(defun forward-list (&optional arg noerror)
   "Move forward across one balanced group of parentheses.
 This command will also work on other parentheses-like expressions
 defined by the current language mode.
 With ARG, do it that many times.
 Negative arg -N means move backward across N groups of parentheses.
-This command assumes point is not in a string or comment."
-  (interactive "^p")
-  (or arg (setq arg 1))
-  (goto-char (or (scan-lists (point) arg 0) (buffer-end arg))))
-
-(defun backward-list (&optional arg)
+This command assumes point is not in a string or comment.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
+  (if noerror
+      (condition-case _
+          (forward-list arg nil)
+        (scan-error (user-error (if (> arg 0)
+                                    "No next group"
+                                  "No previous group"))))
+    (or arg (setq arg 1))
+    (goto-char (or (scan-lists (point) arg 0) (buffer-end arg)))))
+
+(defun backward-list (&optional arg noerror)
   "Move backward across one balanced group of parentheses.
 This command will also work on other parentheses-like expressions
 defined by the current language mode.
 With ARG, do it that many times.
 Negative arg -N means move forward across N groups of parentheses.
-This command assumes point is not in a string or comment."
-  (interactive "^p")
+This command assumes point is not in a string or comment.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
   (or arg (setq arg 1))
-  (forward-list (- arg)))
+  (forward-list (- arg) noerror))
 
-(defun down-list (&optional arg)
+(defun down-list (&optional arg noerror)
   "Move forward down one level of parentheses.
 This command will also work on other parentheses-like expressions
 defined by the current language mode.
 With ARG, do this that many times.
 A negative argument means move backward but still go down a level.
-This command assumes point is not in a string or comment."
-  (interactive "^p")
-  (or arg (setq arg 1))
-  (let ((inc (if (> arg 0) 1 -1)))
-    (while (/= arg 0)
-      (goto-char (or (scan-lists (point) inc -1) (buffer-end arg)))
-      (setq arg (- arg inc)))))
+This command assumes point is not in a string or comment.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "^p\nd")
+  (if noerror
+      (condition-case _
+          (down-list arg nil)
+        (scan-error (user-error "At bottom level")))
+    (or arg (setq arg 1))
+    (let ((inc (if (> arg 0) 1 -1)))
+      (while (/= arg 0)
+        (goto-char (or (scan-lists (point) inc -1) (buffer-end arg)))
+        (setq arg (- arg inc))))))
 
 (defun backward-up-list (&optional arg escape-strings no-syntax-crossing)
   "Move backward out of one level of parentheses.
@@ -229,26 +262,37 @@ up-list
                  (or (< inc 0)
                      (forward-comment 1))
                  (setf arg (+ arg inc)))
-            (signal (car err) (cdr err))))))
+            (if no-syntax-crossing
+                ;; Assume called interactively; don't signal an error.
+                (user-error "At top level")
+              (signal (car err) (cdr err)))))))
       (setq arg (- arg inc)))))
 
-(defun kill-sexp (&optional arg)
+(defun kill-sexp (&optional arg noerror)
   "Kill the sexp (balanced expression) following point.
 With ARG, kill that many sexps after point.
 Negative arg -N means kill N sexps before point.
-This command assumes point is not in a string or comment."
-  (interactive "p")
-  (let ((opoint (point)))
-    (forward-sexp (or arg 1))
-    (kill-region opoint (point))))
-
-(defun backward-kill-sexp (&optional arg)
+This command assumes point is not in a string or comment.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "p\nd")
+  (if noerror
+      (condition-case _
+          (kill-sexp arg nil)
+        (scan-error (user-error (if (> arg 0)
+                                    "No next sexp"
+                                  "No previous sexp"))))
+    (let ((opoint (point)))
+      (forward-sexp (or arg 1))
+      (kill-region opoint (point)))))
+
+(defun backward-kill-sexp (&optional arg noerror)
   "Kill the sexp (balanced expression) preceding point.
 With ARG, kill that many sexps before point.
 Negative arg -N means kill N sexps after point.
-This command assumes point is not in a string or comment."
-  (interactive "p")
-  (kill-sexp (- (or arg 1))))
+This command assumes point is not in a string or comment.
+If NOERROR is non-nil, as it is interactively, do not signal an error."
+  (interactive "p\nd")
+  (kill-sexp (- (or arg 1)) noerror))
 
 ;; After Zmacs:
 (defun kill-backward-up-list (&optional arg)
-- 
2.21.1 (Apple Git-122.3)

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> But my personal flaws should not impede progress. How about I push
> what I've got (latest patch attached for reference) since we seem to
> agree that it's an improvement over what's in master, and if then you
> or anyone else want to further adjust the messages then I have no
> objections.

Sure.  The only thing is this:

> -(defun backward-sexp (&optional arg)
> +obstacle.  If NOERROR is non-nil, as it is interactively,
> +do not signal an error."
> +  (interactive "^p\nd")
> +  (if noerror
> +      (condition-case _
> +          (forward-sexp arg nil)
> +        (scan-error (user-error (if (> arg 0)
> +                                    "No next sexp"
> +                                  "No previous sexp"))))

The parameter is NOERROR, but now it does signal an error.  :-)

So perhaps the parameter should be USER-ERROR/TERSE-ERROR or something
and the doc strings adjusted?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

23 sep. 2020 kl. 15.40 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> The parameter is NOERROR, but now it does signal an error.  :-)
> 
> So perhaps the parameter should be USER-ERROR/TERSE-ERROR or something
> and the doc strings adjusted?

Good catch, thank you! The pushed change uses USER-ERROR.

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[João Távora]

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

I must say that while this makes some sense at the UI level
to me, resignalling errors is a bad practice all around.  Not only
does it convolute the code, but errors should have, in all situations,
I guess even in interactive ones, as much stack as they can.

I think the correct fix here is to change the way that the error
is *printed*.  In the end that was the original motivation behind
the bug report!  In CL this is easy enough to do with the
PRINT-OBJECTgeneric function.  That print function would  print
the alternate message in this particular interactive context. I
think Emacs has recently gained the means to do something
similar to PRINT-OBJECT, has it not?  I seem to remember
Stefan Monnier telling me so.

If it has not, and we do decide to go this route, I'd just nitpick
that the parameter name should be INTERACTIVE or
RESIGNAL-ERROR.  So that unsuspecting programs calles
won't expect the error to have the properties of the scan-error
object if they pass non-nil in the parameter.

João





On Wed, Sep 23, 2020 at 3:34 PM Mattias Engdegård <mattiase@acm.org> wrote:

> 23 sep. 2020 kl. 15.40 skrev Lars Ingebrigtsen <larsi@gnus.org>:
>
> > The parameter is NOERROR, but now it does signal an error.  :-)
> >
> > So perhaps the parameter should be USER-ERROR/TERSE-ERROR or something
> > and the doc strings adjusted?
>
> Good catch, thank you! The pushed change uses USER-ERROR.
>
>
>
>
>

-- 
João Távora

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

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

23 sep. 2020 kl. 16.45 skrev João Távora <joaotavora@gmail.com>:

> If it has not, and we do decide to go this route, I'd just nitpick
> that the parameter name should be INTERACTIVE or 
> RESIGNAL-ERROR.

Thank you! INTERACTIVE is indeed a better parameter name; now renamed.

I'm not very interested in the exact mechanism used for actually displaying the messages to the user. As you have no doubt grown weary hearing by now, I would have preferred there not being any to show! The employed method (user-error) seemed to be simple and workable.  That it uses the error-signalling mechanism is coincidental.

This bug can probably be closed now. Further improvements will of course still be possible.

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[João Távora]

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

> That it uses the error-signalling mechanism is coincidental.

Yes, but it is also unfortunate, as it makes debugging e.g.
forward-sexp-function or scan-sexps harder.  That is
a side effect of this bug fix that I assert shouldn't have
come along in the bargain.

João

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

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Mattias Engdegård]

23 sep. 2020 kl. 18.37 skrev João Távora <joaotavora@gmail.com>:

> Yes, but it is also unfortunate, as it makes debugging e.g.
> forward-sexp-function or scan-sexps harder.  That is 
> a side effect of this bug fix that I assert shouldn't have 
> come along in the bargain.

Sorry to hear that, but perhaps it's not quite that grave? After all, there's nothing unusual about commands that use lower-level functions in their implementation and need to catch errors raised by those functions.
Let's hope the person debugging isn't too much bothered by typing M-: (forward-sexp) instead of C-M-f, don't you think?

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[João Távora]

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

On Thu, Sep 24, 2020 at 4:50 PM Mattias Engdegård <mattiase@acm.org> wrote:

> Let's hope the person debugging isn't too much bothered by typing M-:
(forward-sexp) instead of C-M-f, don't you think

Sure, just a modest +600% increase in keys typed :-)

> Tthere's nothing unusual about commands that use
> lower-level functions in their implementation and need to
> catch errors raised by those functions.

I disagree Mattias.  I think swallowing errors is bad, generally.
Swallowing errors because you wanted to change the way
they're textually represented is worse. It's fixing the problem
in the wrong place, simple as that. Simply because by
fixing the problem it creates a new one, however small
you deem it. These solutions never satisfy me.

But I personally hope _not_ to be terribly bitten by this. After all,
most of elec-pair.el is already done, and quite stable, so I haven't
been messing with this stuff for a while now.

Which reminds me...  Maybe you could just use
condition-case-unless-debug.  Then it will be
just textually ugly but not functionally so.

João

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

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Stefan Monnier]

> I disagree Mattias.  I think swallowing errors is bad, generally.

Agreed, BUT the reason Matthias did it this way is that the error we get
currently from `scan-sexp` can't be turned into a good error message
without knowing that it was triggered during `forward-sexp`.

I think the right way to fix it is indeed not to catch&reraise the
error, but instead to:
A) improve the way errors are printed.
B) change scan-sexp so its errors can be used to generate a good error
   message without having to know whether it occurred while running
   forward-sexp or up-list.

I hoped this case would be a good opportunity to do (A), but sadly this
needs (B) to happen first and this one seemed less straightforward.
Help very welcome on this one.


        Stefan

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I disagree Mattias.  I think swallowing errors is bad, generally.
>
> Agreed, BUT the reason Matthias did it this way is that the error we get
> currently from `scan-sexp` can't be turned into a good error message
> without knowing that it was triggered during `forward-sexp`.

I don't understand.  The point about not swallowing errors is that we
don't want to turn them into anything else.  We don't want to change the
nature of the thing, merely dress it up so it looks less crude in
certain contexts, the reponse to an interactive request by the user
being one of them.

In fact, I think this falls right into the use-case for
(called-interactively-p 'interactive)

   The only known proper use of `interactive' for KIND is in deciding
   whether to display a helpful message, or how to display it.  

So my idea is would be to find out where errors are printed, use
cl-prin1 in that spot and then if write a cl-print-object method
specificallly for scan-error that consults called-interactively-p in the
advised manner.

Or something equivalent if cl-print-object is loaded too late or
something

> I think the right way to fix it is indeed not to catch&reraise the
> error, but instead to:
> A) improve the way errors are printed.
> B) change scan-sexp so its errors can be used to generate a good error
>    message without having to know whether it occurred while running
>    forward-sexp or up-list.
>
> I hoped this case would be a good opportunity to do (A), but sadly this
> needs (B) to happen first and this one seemed less straightforward.
> Help very welcome on this one.

I don't see why scan-sexp needs to be changed.  I think we just need to
know where errors are printed to messages when debug-on-error is nil and
dispatch on the error type there.

João

bug#43489: [PATCH] Don't signal scan-error when moving by sexp interactively

[Stefan Monnier]

>>> I disagree Mattias.  I think swallowing errors is bad, generally.
>>
>> Agreed, BUT the reason Matthias did it this way is that the error we get
>> currently from `scan-sexp` can't be turned into a good error message
>> without knowing that it was triggered during `forward-sexp`.
>
> I don't understand.  The point about not swallowing errors is that we
> don't want to turn them into anything else.

By "turning into" I'm referring to the job of `print-object` done in
the command-loop when it catches an unhandled error.

> I don't see why scan-sexp needs to be changed.

Because the error objects it throws give information that is
difficult/impossible to turn into good error messages.  For example,
when doing an up-list, `scan-sexp` may signal the error object:

    (scan-error "Unbalanced parentheses" 5010 1)

even tho there is no unbalanced paren in the buffer.
Instead, the error is that we were not within a set of parens when
we started.


        Stefan