bug#66326: 29.1.50; There should be a way to promote warnings to errors

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: sbaugh@catern.com,  66326@debbugs.gnu.org
>> Date: Mon, 16 Oct 2023 15:26:51 -0400
>> 
>> > If it's deliberate, it will have to come with an additional option to
>> > enable it.  I don't want users to have their startup aborted just
>> > because they want some warning later on converted to an error.
>> > Startup is a delicate process where signaling errors is not always a
>> > good idea, and we delay warnings there for a good reason.  Changing
>> > this unconditionally is not acceptable.
>> 
>> What is the good reason that we delay warnings?
>
> Because Emacs is not yet able to display stuff reliably, for example.
> Or the display environment was not yet set up completely.

Right.  But that is not a concern for errors.  We already go to great
lengths to make sure that errors signaled during startup can be
displayed to the user one way or another.  So there's no need to do
extra work to delay errors in warnings.el.

>> But OK, attached is a new patch which adds an option to immediately
>> raise the warnings converted into errors during startup, instead of
>> delaying them.  So the default is to delay the error until after
>> startup.  Does this work?
>
> Did you test that?  If you did, what happens with delayed warnings
> when the new option is nil?

They continue to be delayed.  If a warning is turned into an error, the
error is signaled by delayed-warnings-hook, run by after-init-hook.

>> > There's no difficulty in debugging a warning whatsoever, IME.  It is a
>> > serious exaggeration to claim that there's a significant problem here
>> > that needs a solution.
>> 
>> I'm curious: if you have some piece of code which warns during startup,
>> how would you find out why that code is running?
>
> By searching for the warning text and analyzing the code which invokes
> the warning, of course.  And that's only if the warning message itself
> doesn't explain itself (which a good warning should).
>
>> e.g., how would you figure out what function is calling some other
>> code which internally warns?  This seems pretty difficult to me.
>
> I normally find this unnecessary.  In very rare and extreme
> situations, I run Emacs under GDB.
>
> Like I said: this is a niche feature, which seems to have grown out of
> some very specific experience you had at some point.  It is not at all
> common enough to consider it important.  So the last thing we should
> allow is for it to introduce regressions, and IME messing with startup
> code runs a very high risk of introducing regressions.

It's an experience I've had several times, but yes.  The very concrete
motivation is: I have some user running code which is emitting warnings.
They find this annoying.  I can't practically tell a user to run Emacs
under GDB.  I'd like to have some easy way to get more information about
the warning.  So, if I have the user do (setq warning-to-error-types t),
or:

./src/emacs --eval '(setq warning-to-error-types t
warning-signal-errors-during-startup t)' --debug-init

then I can just tell them to send me the contents of *Backtrace* and
I can figure out what's warning from that.

That's also useful to me personally when debugging, and I also think
it's useful for running automated tests where warnings should cause
failures.

(All these use cases would be improved if I didn't have to remember to
also set warning-signal-errors-during-startup, BTW.  That variable
doesn't seem to ever help...)

>> --- a/lisp/emacs-lisp/warnings.el
>> +++ b/lisp/emacs-lisp/warnings.el
>> @@ -114,11 +114,37 @@ warning-suppress-types
>>  The element must match an initial segment of the list TYPE.
>>  Thus, (foo bar) as an element matches (foo bar)
>>  or (foo bar ANYTHING...) as TYPE.
>> +An empty list as an element matches any TYPE.
>
> Why was this change necessary?  An empty list is the same as nil, and
> treating nil as matching everything is unusual and unintuitive, IMO.
> If this option is really needed and important, the value of t is much
> more natural.

Yes, good point, I'll do that.

The reason I made nil match everything is because currently nil *does*
match any list of warning types; but if the warning type is just a
single symbol, it doesn't match.

But yes, just having t match anything is much more sensible.

>> +(defcustom warning-to-error-types nil
>> +  "List of warning types to signal as an error instead.
>                                                   ^^^^^^^
> "instead of a warning".  Or maybe even
>
>   List of types of warnings that will be converted to errors.

Fixed.

>> +If any element of this list matches the TYPE argument to `display-warning',
>> +`display-warning' signals an error instead of logging a warning.
>
> "Logging" is not a good terminology here, as warning are "displayed",
> not "logged".

Fixed.

>
>> +(defcustom warning-signal-errors-during-startup nil
>> +  "If non-nil, warnings converted into errors are signaled during startup.
>
>  "Whether to signal errors converted from warnings during startup."

Fixed.

>
>> +Normally, warnings generated during startup are delayed until
>> +after startup.  This includes warnings converted into errors by
>> +`warning-to-error-types': they will be signaled after startup
>> +completes, outside the context of the code which caused the
>> +warning.  If you'd prefer that these errors be signaled
>> +immediately so that the context is present during debugging, set
>> +this variable to nil."
>
> I think this should explain what does "after startup" mean, and also
> mention the special case of daemon.

Fixed.

>
>>  (defun display-warning (type message &optional level buffer-name)
>>    "Display a warning message, MESSAGE.
>> @@ -263,6 +299,14 @@ display-warning
>>  disable automatic display of the warning or disable the warning
>>  entirely by setting `warning-suppress-types' or
>>  `warning-suppress-log-types' on their behalf."
>> +  (when (and (>= (warning-numeric-level (or level :warning))
>> +             (warning-numeric-level warning-minimum-log-level))
>> +         (not (warning-suppress-p type warning-suppress-log-types))
>> +             (warning-suppress-p type warning-to-error-types)
>> +             (or warning-signal-errors-during-startup
>> +                 after-init-time noninteractive (daemonp))
>> +             )
>
> I'd suggest to move the last condition to be the first one, or
> thereabouts, as the default values will then make this code a tad
> faster

Fixed.

>From 4b342c2a36aad90a34ce21e31514245eb81cf04d Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Thu, 19 Oct 2023 10:49:29 -0400
Subject: [PATCH] Support turning warnings into errors

Support turning warnings into errors in a user-configurable way.  This
is especially useful in combination with (setq debug-on-error t) to
drop to the debugger when a warning happens.

* lisp/emacs-lisp/warnings.el (warning-suppress-types,
warning-suppress-log-types): Document the effect of t.
(warning-to-error-types, warning-to-error): Add. (bug#66326)
(display-warning): Check warning-to-error-types.
(warning-suppress-p): Allow t to match any type.
---
 lisp/emacs-lisp/warnings.el | 108 ++++++++++++++++++++++++++----------
 1 file changed, 79 insertions(+), 29 deletions(-)

diff --git a/lisp/emacs-lisp/warnings.el b/lisp/emacs-lisp/warnings.el
index 31b840d6c83..e7d1a50f72a 100644
--- a/lisp/emacs-lisp/warnings.el
+++ b/lisp/emacs-lisp/warnings.el
@@ -102,8 +102,10 @@ warning-suppress-log-types
 Thus, (foo bar) as an element matches (foo bar)
 or (foo bar ANYTHING...) as TYPE.
 If TYPE is a symbol FOO, that is equivalent to the list (FOO),
-so only the element (FOO) will match it."
-  :type '(repeat (repeat symbol))
+so only the element (FOO) will match it.
+If this is t, warnings are never logged."
+  :type '(choice (repeat (repeat symbol))
+                 (const :tag "Completely ignore all warnings" t))
   :version "22.1")
 
 (defcustom warning-suppress-types nil
@@ -111,14 +113,43 @@ warning-suppress-types
 If any element of this list matches the TYPE argument to `display-warning',
 the warning is logged nonetheless, but the warnings buffer is
 not immediately displayed.
+If this is t, the warnings buffer is never displayed.
 The element must match an initial segment of the list TYPE.
 Thus, (foo bar) as an element matches (foo bar)
 or (foo bar ANYTHING...) as TYPE.
 If TYPE is a symbol FOO, that is equivalent to the list (FOO),
 so only the element (FOO) will match it.
 See also `warning-suppress-log-types'."
-  :type '(repeat (repeat symbol))
+  :type '(choice (repeat (repeat symbol))
+                 (const :tag "Never display the warnings buffer" t))
   :version "22.1")
+
+(defcustom warning-to-error-types nil
+  "List of warning types that will be converted to errors.
+If any element of this list matches the TYPE argument to `display-warning',
+`display-warning' signals an error instead of displaying a warning.
+See `warning-suppress-types' for the format of elements in this list.
+If this is t, all warnings are converted to errors.
+See also `warning-signal-errors-during-startup'."
+  :type '(choice (repeat (repeat symbol))
+                 (const :tag "Convert all warnings into errors"))
+  :version "30.1")
+
+(defcustom warning-signal-errors-during-startup nil
+  "Whether to signal errors converted from warnings during startup.
+
+Normally, if Emacs is interactive and not a daemon, then warnings
+generated before init finishes loading are delayed until
+`after-init-hook' runs.  This includes warnings converted into
+errors by `warning-to-error-types': they will be signaled after
+startup completes, outside the context of the code which caused
+the warning.  If you'd prefer that these errors be signaled
+immediately so that the context is present during debugging, set
+this variable to nil."
+  :type '(choice
+          (const :tag "Warnings converted into errors raise after startup" nil)
+          (const :tag "Warnings converted into errors raise immediately" t))
+  :version "30.1")

 ;; The autoload cookie is so that programs can bind this variable
 ;; safely, testing the existing value, before they call one of the
@@ -176,32 +207,35 @@ warning-numeric-level
 
 (defun warning-suppress-p (type suppress-list)
   "Non-nil if a warning with type TYPE should be suppressed.
-SUPPRESS-LIST is the list of kinds of warnings to suppress."
-  (let (some-match)
-    (dolist (elt suppress-list)
-      (if (symbolp type)
-         ;; If TYPE is a symbol, the ELT must be (TYPE).
-         (if (and (consp elt)
-                  (eq (car elt) type)
-                  (null (cdr elt)))
-             (setq some-match t))
-       ;; If TYPE is a list, ELT must match it or some initial segment of it.
-       (let ((tem1 type)
-             (tem2 elt)
-             (match t))
-         ;; Check elements of ELT until we run out of them.
-         (while tem2
-           (if (not (equal (car tem1) (car tem2)))
-               (setq match nil))
-           (setq tem1 (cdr tem1)
-                 tem2 (cdr tem2)))
-         ;; If ELT is an initial segment of TYPE, MATCH is t now.
-         ;; So set SOME-MATCH.
-         (if match
-             (setq some-match t)))))
-    ;; If some element of SUPPRESS-LIST matched,
-    ;; we return t.
-    some-match))
+SUPPRESS-LIST is the list of kinds of warnings to suppress,
+or t if all warnings should be suppressed."
+  (or (eq suppress-list t)
+      (let (some-match)
+        (dolist (elt suppress-list)
+          (if (symbolp type)
+             ;; If TYPE is a symbol, the ELT must be (TYPE) or ().
+             (if (or (null elt)
+                      (and (consp elt)
+                          (eq (car elt) type)
+                          (null (cdr elt))))
+                 (setq some-match t))
+           ;; If TYPE is a list, ELT must match it or some initial segment of 
it.
+           (let ((tem1 type)
+                 (tem2 elt)
+                 (match t))
+             ;; Check elements of ELT until we run out of them.
+             (while tem2
+               (if (not (equal (car tem1) (car tem2)))
+                   (setq match nil))
+               (setq tem1 (cdr tem1)
+                     tem2 (cdr tem2)))
+             ;; If ELT is an initial segment of TYPE, MATCH is t now.
+             ;; So set SOME-MATCH.
+             (if match
+                 (setq some-match t)))))
+        ;; If some element of SUPPRESS-LIST matched,
+        ;; we return t.
+        some-match)))

 (define-icon warnings-suppress button
   `((emoji "⛔")
@@ -230,6 +264,15 @@ warnings-suppress
                               (cons (list type) warning-suppress-types)))
     (_ (message "Exiting"))))
 
+(defun warning-to-error (type message level)
+  (unless level
+    (setq level :warning))
+  (let* ((typename (if (consp type) (car type) type))
+         (level-info (assq level warning-levels)))
+    (error (concat (nth 1 level-info) "%s")
+           (format warning-type-format typename)
+           message)))
+
 ;;;###autoload
 (defun display-warning (type message &optional level buffer-name)
   "Display a warning message, MESSAGE.
@@ -263,6 +306,13 @@ display-warning
 disable automatic display of the warning or disable the warning
 entirely by setting `warning-suppress-types' or
 `warning-suppress-log-types' on their behalf."
+  (when (and (or warning-signal-errors-during-startup
+                 after-init-time noninteractive (daemonp))
+             (>= (warning-numeric-level (or level :warning))
+                (warning-numeric-level warning-minimum-log-level))
+            (not (warning-suppress-p type warning-suppress-log-types))
+             (warning-suppress-p type warning-to-error-types))
+    (warning-to-error type message level))
   (if (not (or after-init-time noninteractive (daemonp)))
       ;; Ensure warnings that happen early in the startup sequence
       ;; are visible when startup completes (bug#20792).
-- 
2.39.3