Eli Zaretskii writes: >> From: Spencer Baugh >> 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.