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

[Eli Zaretskii <eliz@gnu.org>]

> 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.

> 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?

> > 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.

> --- 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.

> +(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.

> +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".

> +(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."

> +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.

>  (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