bug#28280: Maximally unhelpful diagnostic message.

bug#28280: Maximally unhelpful diagnostic message.

[Alan Mackenzie]

Hello, Emacs.

In GNU Emacs 25.2.3 (x86_64-unknown-linux-gnu, GTK+ Version 3.22.16)
of 2017-08-15 built on ACM
Configured using:
  'configure --with-gif=no --with-tiff=no --with-gpm'

Configured features:
XPM JPEG PNG RSVG SOUND GPM DBUS GSETTINGS NOTIFY ACL GNUTLS LIBXML2
FREETYPE XFT ZLIB TOOLKIT_SCROLL_BARS GTK3 X11


I was just debugging some code when I was presented with the diagnostic:

    Assertion failed: (eq 10 (char-before))

.  Just that.  Nothing else.  No backtrace, no context, no nothing.
This is not helpful.

I request that consideration be given to providing one or more of these
details when assertion code triggers.

Thanks!

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#28280: Maximally unhelpful diagnostic message.

[Glenn Morris]

Alan Mackenzie wrote:

>     Assertion failed: (eq 10 (char-before))

A few seconds grep suggests this is due to the code in newline, added ~
3 years ago in 62ee8b1 ("Add assertions to try and help catch bug#18913").
The complete lack of progress in that report since then suggests it may
be time to remove/downgrade those assertions.

bug#28280: Maximally unhelpful diagnostic message.

[John Wiegley]

>>>>> "GM" == Glenn Morris <rgm@gnu.org> writes:

GM> A few seconds grep suggests this is due to the code in newline, added ~ 3
GM> years ago in 62ee8b1 ("Add assertions to try and help catch bug#18913").
GM> The complete lack of progress in that report since then suggests it may be
GM> time to remove/downgrade those assertions.

Agreed.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

bug#28280: Maximally unhelpful diagnostic message.

[Alan Mackenzie]

Hello, Glenn.

On Tue, Aug 29, 2017 at 15:20:23 -0400, Glenn Morris wrote:
> Alan Mackenzie wrote:

> >     Assertion failed: (eq 10 (char-before))

> A few seconds grep suggests this is due to the code in newline, added ~
> 3 years ago in 62ee8b1 ("Add assertions to try and help catch bug#18913").
> The complete lack of progress in that report since then suggests it may
> be time to remove/downgrade those assertions.

Yes, but that's a separate problem from the essence of the bug report.
Regardless of the lack of progress on bug #18913, the information
provided by an assert being triggered is unhelpful and inadequate.

That aside, I can't see how the two assertions suggested in #18913 could
trigger the message I saw.  I'll maybe have a look at the actual patch
sometime when I'm less tired.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#28280: Maximally unhelpful diagnostic message.

[npostavs]

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

Alan Mackenzie <acm@muc.de> writes:

> Yes, but that's a separate problem from the essence of the bug report.
> Regardless of the lack of progress on bug #18913, the information
> provided by an assert being triggered is unhelpful and inadequate.

Here is a possible M-x cl-show-assert command:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 2695 bytes --]

From 835486ccfaaf58476cbb0e097bed3b81adbcfebe Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 30 Aug 2017 08:12:18 -0400
Subject: [PATCH v1] New command to backtrace from assertion failure
 (Bug#28280)

* lisp/emacs-lisp/cl-lib.el (cl-show-assert): New command.
(cl-assert-last-backtrace): New variable.
* lisp/emacs-lisp/cl-preloaded.el (cl--assertion-failed): Set it.

TODO: keybinding? NEWS.
---
 lisp/emacs-lisp/cl-lib.el       | 15 +++++++++++++++
 lisp/emacs-lisp/cl-preloaded.el | 10 ++++++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/cl-lib.el b/lisp/emacs-lisp/cl-lib.el
index 243622a301..59bbdbca51 100644
--- a/lisp/emacs-lisp/cl-lib.el
+++ b/lisp/emacs-lisp/cl-lib.el
@@ -614,6 +614,21 @@ cl-pairlis
 
 ;;; Miscellaneous.
 
+(defvar cl-assert-last-backtrace nil
+  "Holds a list of frames from the last assertion failure.")
+
+(declare-function debugger-insert-backtrace "debug" (backtrace doxrefs))
+
+(defun cl-show-assert ()
+  "Show backtrace from last assertion failure."
+  (interactive)
+  (unless cl-assert-last-backtrace
+    (user-error "There hasn't been an assertion failure yet"))
+  (require 'debug)
+  (with-current-buffer (get-buffer-create "*Assertion Failure*")
+    (debugger-insert-backtrace cl-assert-last-backtrace t)
+    (display-buffer (current-buffer))))
+
 (provide 'cl-lib)
 (unless (load "cl-loaddefs" 'noerror 'quiet)
   ;; When bootstrapping, cl-loaddefs hasn't been built yet!
diff --git a/lisp/emacs-lisp/cl-preloaded.el b/lisp/emacs-lisp/cl-preloaded.el
index ab6354de7c..d66a4b9726 100644
--- a/lisp/emacs-lisp/cl-preloaded.el
+++ b/lisp/emacs-lisp/cl-preloaded.el
@@ -41,13 +41,19 @@
 
 ;; The `assert' macro from the cl package signals
 ;; `cl-assertion-failed' at runtime so always define it.
-(define-error 'cl-assertion-failed (purecopy "Assertion failed"))
+(define-error 'cl-assertion-failed (purecopy "Assertion failed (\\[cl-show-assert] for backtrace)"))
+
+(defvar cl-assert-last-backtrace)
 
 (defun cl--assertion-failed (form &optional string sargs args)
   (if debug-on-error
       (funcall debugger 'error `(cl-assertion-failed (,form ,string ,@sargs)))
+    (setq cl-assert-last-backtrace (backtrace-frames 'cl--assertion-failed))
+    (autoload 'cl-show-assert "cl-lib"
+      "Show backtrace from last assertion failure." t)
     (if string
-        (apply #'error string (append sargs args))
+        (apply #'error (concat string " (\\[cl-show-assert] for backtrace)")
+               (append sargs args))
       (signal 'cl-assertion-failed `(,form ,@sargs)))))
 
 ;; When we load this (compiled) file during pre-loading, the cl--struct-class
-- 
2.14.1


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


> That aside, I can't see how the two assertions suggested in #18913 could
> trigger the message I saw.  I'll maybe have a look at the actual patch
> sometime when I'm less tired.

There is a third assertion added in [1: 62ee8b1a59] which is the one you
hit.

[1: 62ee8b1a59]: 2014-10-31 16:57:59 -0400
  * lisp/simple.el (newline): Add assertions to try and help catch bug#18913.
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=62ee8b1a59d5343197cb59d9f0a52e07a23af1d8

bug#28280: Maximally unhelpful diagnostic message.

[Lars Ingebrigtsen]

npostavs@users.sourceforge.net writes:

>> Yes, but that's a separate problem from the essence of the bug report.
>> Regardless of the lack of progress on bug #18913, the information
>> provided by an assert being triggered is unhelpful and inadequate.
>
> Here is a possible M-x cl-show-assert command:

[...]

> * lisp/emacs-lisp/cl-lib.el (cl-show-assert): New command.
> (cl-assert-last-backtrace): New variable.
> * lisp/emacs-lisp/cl-preloaded.el (cl--assertion-failed): Set it.

[...]

> +    (debugger-insert-backtrace cl-assert-last-backtrace t)

This patch still applies, but this function was removed in 2018, so if
we want something like this, we have to refactor out that function
again.

But...  I'm not sure a command like this makes sense.  As others noted
in this thread, if you want a backtrace (from an error or assertion),
then you set debug-on-error.

So if we want this, I think it should be even more general -- a command
to give you a backtrace of the previous error signalled, no matter
whether it was an error or an assertion.

Which I think would be a really nice thing to have -- I run Emacs with
debug-on-error on by default because of this, but if it were possible to
say `M-x debug-last-error', then I wouldn't have to.

Would this be possible to implement in an easy way, I wonder?

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

bug#28280: Maximally unhelpful diagnostic message.

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> So if we want this, I think it should be even more general -- a command
> to give you a backtrace of the previous error signalled, no matter
> whether it was an error or an assertion.

In any case, the reported bug here was fixed by Paul at the time, so I'm
closing this bug report.

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

bug#28280: Maximally unhelpful diagnostic message.

[Glenn Morris]

Alan Mackenzie wrote:

> Yes, but that's a separate problem from the essence of the bug report.

So M-x toggle-debug-on-error, like with any error.

bug#28280: Maximally unhelpful diagnostic message.

[Alan Mackenzie]

Hello, Glenn.

On Wed, Aug 30, 2017 at 15:51:38 -0400, Glenn Morris wrote:
> Alan Mackenzie wrote:

> > Yes, but that's a separate problem from the essence of the bug report.

> So M-x toggle-debug-on-error, like with any error.

Thanks, I'll try that when the error comes up again.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#28280: Maximally unhelpful diagnostic message.

[Paul Eggert]

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

> GM> A few seconds grep suggests this is due to the code in newline, added ~ 3
> GM> years ago in 62ee8b1 ("Add assertions to try and help catch bug#18913").
> GM> The complete lack of progress in that report since then suggests it may be
> GM> time to remove/downgrade those assertions.
> 
> Agreed.

Make sense, so I installed the attached into emacs-26.

[-- Attachment #2: 0001-Remove-old-cl-assert-calls-in-newline.patch --]
[-- Type: text/x-patch, Size: 3075 bytes --]

From 81b3261b0324137b7c8c05353227673f18264849 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 17 Sep 2017 22:32:31 -0700
Subject: [PATCH] Remove old cl-assert calls in 'newline'

* lisp/simple.el (newline): Remove cl-assert calls
that didn't seem to be helping us debug Bug#18913,
and that caused problems as reported in Bug#28280.
Suggested by Glenn Morris (Bug#28280#8).
---
 lisp/simple.el | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 1ffe181..4e42fd5 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -434,10 +434,6 @@ newline
           ;; Do the rest in post-self-insert-hook, because we want to do it
           ;; *before* other functions on that hook.
           (lambda ()
-            ;; We are not going to insert any newlines if arg is
-            ;; non-positive.
-            (or (and (numberp arg) (<= arg 0))
-                (cl-assert (eq ?\n (char-before))))
             ;; Mark the newline(s) `hard'.
             (if use-hard-newlines
                 (set-hard-newline-properties
@@ -456,25 +452,22 @@ newline
             ;; starts a page.
             (or was-page-start
                 (move-to-left-margin nil t)))))
-    (unwind-protect
-        (if (not interactive)
-            ;; FIXME: For non-interactive uses, many calls actually
-            ;; just want (insert "\n"), so maybe we should do just
-            ;; that, so as to avoid the risk of filling or running
-            ;; abbrevs unexpectedly.
-            (let ((post-self-insert-hook (list postproc)))
-              (self-insert-command arg))
-          (unwind-protect
-              (progn
-                (add-hook 'post-self-insert-hook postproc nil t)
-                (self-insert-command arg))
-            ;; We first used let-binding to protect the hook, but that
-            ;; was naive since add-hook affects the symbol-default
-            ;; value of the variable, whereas the let-binding might
-            ;; only protect the buffer-local value.
-            (remove-hook 'post-self-insert-hook postproc t)))
-      (cl-assert (not (member postproc post-self-insert-hook)))
-      (cl-assert (not (member postproc (default-value 'post-self-insert-hook))))))
+    (if (not interactive)
+	;; FIXME: For non-interactive uses, many calls actually
+	;; just want (insert "\n"), so maybe we should do just
+	;; that, so as to avoid the risk of filling or running
+	;; abbrevs unexpectedly.
+	(let ((post-self-insert-hook (list postproc)))
+	  (self-insert-command arg))
+      (unwind-protect
+	  (progn
+	    (add-hook 'post-self-insert-hook postproc nil t)
+	    (self-insert-command arg))
+	;; We first used let-binding to protect the hook, but that
+	;; was naive since add-hook affects the symbol-default
+	;; value of the variable, whereas the let-binding might
+	;; only protect the buffer-local value.
+	(remove-hook 'post-self-insert-hook postproc t))))
   nil)
 
 (defun set-hard-newline-properties (from to)
-- 
2.7.4