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

[Eli Zaretskii]

> From: sbaugh@catern.com
> Date: Tue, 03 Oct 2023 19:16:09 +0000 (UTC)
> Cc: Spencer Baugh <sbaugh@janestreet.com>, 66326@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Spencer Baugh <sbaugh@janestreet.com>
> >> Date: Tue, 03 Oct 2023 14:39:02 -0400
> >> 
> >> +(defcustom warning-to-error-types nil
> >> +  "List of warning types to signal as an error instead.
> >> +If any element of this list matches the TYPE argument to 
> >> `display-warning',
> >> +an error is signaled instead of logging a warning.
> >    ^^^^^^^^^^^^^^^^^^^^
> > Passive voice alert!
> >
> >>  (defun display-warning (type message &optional level buffer-name)
> >>    "Display a warning message, MESSAGE.
> >> @@ -263,105 +278,109 @@ 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."
> >> -  (if (not (or after-init-time noninteractive (daemonp)))
> >> -      ;; Ensure warnings that happen early in the startup sequence
> >> -      ;; are visible when startup completes (bug#20792).
> >> -      (delay-warning type message level buffer-name)
> >> -    (unless level
> >> -      (setq level :warning))
> >> -    (unless buffer-name
> >> -      (setq buffer-name "*Warnings*"))
> >> +  (unless level
> >> +    (setq level :warning))
> >> +  (unless buffer-name
> >> +    (setq buffer-name "*Warnings*"))
> >> +  (cond
> >> +   ((< (warning-numeric-level level)
> >> +       (warning-numeric-level warning-minimum-log-level)))
> >> +   ((warning-suppress-p type warning-suppress-log-types))
> >> +   ((warning-suppress-p type warning-to-error-types)
> >> +    (warning-to-error type message level))
> >> +   ((not (or after-init-time noninteractive (daemonp)))
> >> +    ;; Ensure warnings that happen early in the startup sequence
> >> +    ;; are visible when startup completes (bug#20792).
> >> +    (delay-warning type message level buffer-name))
> >> +   (t
> >
> > AFAICT, this reorders parts of the evaluation, and thus changes the
> > semantics/behavior.  Please try to keep the order of the evaluation
> > the same as it was, to avoid unintended consequences.  (It will also
> > make the patch review easier.)
> 
> Unfortuantely it's not possible to avoid either reordering the
> evaluation or duplicating some parts of it.  Because, of course we want
> a warning to not become an error if it's listed in
> warning-suppress-log-types, and we do want it to become an error even if
> it occurs early in the startup sequence.  So the
> warning-suppress-log-types check has to happen before the to-error
> check, and the to-error check has to happen before the early-startup
> check.

Even if the above is true, I see no justification to calling
delay-warning with overridden values of level and buffer, and after
the other 'cond' cases, where it originally was called right away.

And in this case, duplication is a lesser evil than reordering of
logic, since the chances of unintended consequences would be lower in
the former case.

> Reordering them doesn't actually change behavior, because the
> early-startup check just delays the warning, so it should be fine.

Famous last words.  When will we learn that Emacs is so much complex
that we should humbly realize we never have the full picture to make
such cavalier assumptions?

> I can separate out the reordering change into a separate patch if you
> like, then you can see that it should not change behavior.

No, that won't help me to be sure we are not introducing some
incompatible changes in this long-standing behavior.

Please realize: this is a very minor feature, useful in a small number
of situations, so the risk of it causing us trouble in the much more
important cases of issuing and delaying warnings is unacceptable.  So
unless I'm reasonably sure this risk is very low, I will simply not
agree to installing this feature.

TIA