bug#44746: 28.0.50; [feature/native-comp] Noisy "*Warnings*" buffer shown on start

bug#44746: 28.0.50; [feature/native-comp] Noisy "*Warnings*" buffer shown on start

[Stefan Kangas]

Every time I start Emacs after recompilation or clearing cache, I see a
"*Warnings*" buffer popup in a new window, filled with a large amount of
warnings like these:

Warning (comp): debian-ispell.el:229:17: Warning: assignment to free
variable Disable showing Disable logging
Warning (comp): debian-ispell.el:228:28: Warning: reference to free
variable ‘really-hunspell’ Disable showing Disable logging
Warning (comp): debian-ispell.el:386:16: Warning: reference to free
variable Disable showing Disable logging
Warning (comp): debian-ispell.el:392:16: Warning: reference to free
variable Disable showing Disable logging
Warning (comp): debian-ispell.el:393:16: Warning: reference to free
variable Disable showing Disable logging
Warning (comp): debian-ispell.el:403:24: Warning: assignment to free
variable Disable showing Disable logging
Warning (comp): debian-ispell.el:403:20: Warning: reference to free
variable Disable showing Disable logging
[...]
Warning (comp): init-general.el:44:7: Warning: assignment to free
variable Disable showing Disable logging
Warning (comp): init-general.el:45:7: Warning: assignment to free
variable Disable showing Disable logging
Warning (comp): init-general.el:47:7: Warning: assignment to free
variable ‘Man-width’ Disable showing Disable logging
[...]

(The file init-general.el is required from my init file.)

To reproduce this, I would assume it is sufficient to either run Debian,
where the debian-ispell.el file is part of the site-files, or having an
init file requiring a file with, for example, the line:

    (setq display-time-24hr-format t) ; my line 47 above


I'm not exactly sure what the best course of action is here.  But
wouldn't it be better to not show this at all to users, unless they
explicitly ask for it?  As it stands, it is a bit too noisy and
in-your-face, I think.

(I also don't understand why the byte-compiler does not complain about
these variables.)

bug#44746: 28.0.50; [feature/native-comp] Noisy "*Warnings*" buffer shown on start

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefan@marxist.se> writes:

> Every time I start Emacs after recompilation or clearing cache, I see a
> "*Warnings*" buffer popup in a new window, filled with a large amount of
> warnings like these:
>
> Warning (comp): debian-ispell.el:229:17: Warning: assignment to free
> variable Disable showing Disable logging
> Warning (comp): debian-ispell.el:228:28: Warning: reference to free
> variable ‘really-hunspell’ Disable showing Disable logging
> Warning (comp): debian-ispell.el:386:16: Warning: reference to free
> variable Disable showing Disable logging
> Warning (comp): debian-ispell.el:392:16: Warning: reference to free
> variable Disable showing Disable logging
> Warning (comp): debian-ispell.el:393:16: Warning: reference to free
> variable Disable showing Disable logging
> Warning (comp): debian-ispell.el:403:24: Warning: assignment to free
> variable Disable showing Disable logging
> Warning (comp): debian-ispell.el:403:20: Warning: reference to free
> variable Disable showing Disable logging
> [...]
> Warning (comp): init-general.el:44:7: Warning: assignment to free
> variable Disable showing Disable logging
> Warning (comp): init-general.el:45:7: Warning: assignment to free
> variable Disable showing Disable logging
> Warning (comp): init-general.el:47:7: Warning: assignment to free
> variable ‘Man-width’ Disable showing Disable logging
> [...]
>
> (The file init-general.el is required from my init file.)
>
> To reproduce this, I would assume it is sufficient to either run Debian,
> where the debian-ispell.el file is part of the site-files, or having an
> init file requiring a file with, for example, the line:
>
>     (setq display-time-24hr-format t) ; my line 47 above
>
>
> I'm not exactly sure what the best course of action is here.  But
> wouldn't it be better to not show this at all to users, unless they
> explicitly ask for it?  As it stands, it is a bit too noisy and
> in-your-face, I think.
>
> (I also don't understand why the byte-compiler does not complain about
> these variables.)

The byte compiler does not complain because when it compiles these
definitions happen to be present in the compilation environment (read
the file defining these variables was by chance already loaded).  Each
file should be consistent and compilable regardless the load history of
the Emacs used to compile (read specify the correct requires).

Given async compilation start from a fresh Emacs it's more sensitive
into spotting these errors or warnings.

Reporting these messages to the user as warnings was the result of
#44168.  Before #44168 these messages where only reported into the
*Async-native-compile-log*.

This was requested by a number of people because more than one package
missed requiring the feature containing some macro definition.  This
indeed resulted in miscompilations with the diagnostic message being
"hidden" in *Async-native-compile-log*.

Compilation units should *not* rely on the compiler environment for
their definitions.

So yeah these are real warnings or errors and package developers need to
get them somehow.  They could manifest also on non nativecomp builds if
the compilation order or something else chance.  So this is not only a
nice clean-up but also a real fix IMO.

I'm not sure what's the best action we can take to reduce the verbosity
but keep developers informed at the same time.

  Andrea

bug#44746: 28.0.50; [feature/native-comp] Noisy "*Warnings*" buffer shown on start

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> Every time I start Emacs after recompilation or clearing cache, I see a
>> "*Warnings*" buffer popup in a new window, filled with a large amount of
>> warnings like these:
>>
>> Warning (comp): debian-ispell.el:229:17: Warning: assignment to free
>> variable Disable showing Disable logging
>> Warning (comp): debian-ispell.el:228:28: Warning: reference to free
>> variable ‘really-hunspell’ Disable showing Disable logging
>> Warning (comp): debian-ispell.el:386:16: Warning: reference to free
>> variable Disable showing Disable logging
>> Warning (comp): debian-ispell.el:392:16: Warning: reference to free
>> variable Disable showing Disable logging
>> Warning (comp): debian-ispell.el:393:16: Warning: reference to free
>> variable Disable showing Disable logging
>> Warning (comp): debian-ispell.el:403:24: Warning: assignment to free
>> variable Disable showing Disable logging
>> Warning (comp): debian-ispell.el:403:20: Warning: reference to free
>> variable Disable showing Disable logging
>> [...]
>> Warning (comp): init-general.el:44:7: Warning: assignment to free
>> variable Disable showing Disable logging
>> Warning (comp): init-general.el:45:7: Warning: assignment to free
>> variable Disable showing Disable logging
>> Warning (comp): init-general.el:47:7: Warning: assignment to free
>> variable ‘Man-width’ Disable showing Disable logging
>> [...]
>>
>> (The file init-general.el is required from my init file.)
>>
>> To reproduce this, I would assume it is sufficient to either run Debian,
>> where the debian-ispell.el file is part of the site-files, or having an
>> init file requiring a file with, for example, the line:
>>
>>     (setq display-time-24hr-format t) ; my line 47 above
>>
>>
>> I'm not exactly sure what the best course of action is here.  But
>> wouldn't it be better to not show this at all to users, unless they
>> explicitly ask for it?  As it stands, it is a bit too noisy and
>> in-your-face, I think.
>>
>> (I also don't understand why the byte-compiler does not complain about
>> these variables.)
>
> The byte compiler does not complain because when it compiles these
> definitions happen to be present in the compilation environment (read
> the file defining these variables was by chance already loaded).  Each
> file should be consistent and compilable regardless the load history of
> the Emacs used to compile (read specify the correct requires).
>
> Given async compilation start from a fresh Emacs it's more sensitive
> into spotting these errors or warnings.
>
> Reporting these messages to the user as warnings was the result of
> #44168.  Before #44168 these messages where only reported into the
> *Async-native-compile-log*.
>
> This was requested by a number of people because more than one package
> missed requiring the feature containing some macro definition.  This
> indeed resulted in miscompilations with the diagnostic message being
> "hidden" in *Async-native-compile-log*.
>
> Compilation units should *not* rely on the compiler environment for
> their definitions.
>
> So yeah these are real warnings or errors and package developers need to
> get them somehow.  They could manifest also on non nativecomp builds if
> the compilation order or something else chance.  So this is not only a
> nice clean-up but also a real fix IMO.
>
> I'm not sure what's the best action we can take to reduce the verbosity
> but keep developers informed at the same time.
>
>   Andrea

To complete this answer, ATM is possible to gate all warnings reported
by async native compilations leveraging the
`comp-async-report-warnings-errors' customize.

Is this sufficient to close this bug or do we like to discuss this
default?

My opinion (got it from the discussion on emacs-devel) is that at least
for now would be good to keep `comp-async-report-warnings-errors' to t
for the reason I've explained in the mail I'm quoting.  Perhaps we could
think about revisiting this default but probably in the future.

Thanks

  Andrea

bug#44746: 28.0.50; [feature/native-comp] Noisy "*Warnings*" buffer shown on start

[Stefan Kangas]

Andrea Corallo <akrl@sdf.org> writes:

>> The byte compiler does not complain because when it compiles these
>> definitions happen to be present in the compilation environment (read
>> the file defining these variables was by chance already loaded).  Each
>> file should be consistent and compilable regardless the load history of
>> the Emacs used to compile (read specify the correct requires).
>>
>> Given async compilation start from a fresh Emacs it's more sensitive
>> into spotting these errors or warnings.
>>
>> Reporting these messages to the user as warnings was the result of
>> #44168.  Before #44168 these messages where only reported into the
>> *Async-native-compile-log*.
>>
>> This was requested by a number of people because more than one package
>> missed requiring the feature containing some macro definition.  This
>> indeed resulted in miscompilations with the diagnostic message being
>> "hidden" in *Async-native-compile-log*.
>>
>> Compilation units should *not* rely on the compiler environment for
>> their definitions.
>>
>> So yeah these are real warnings or errors and package developers need to
>> get them somehow.  They could manifest also on non nativecomp builds if
>> the compilation order or something else chance.  So this is not only a
>> nice clean-up but also a real fix IMO.

(Sorry I didn't reply properly the last time around.)

The above reasoning is correct and valid for any ELisp package, I think.

But most of the warnings I see are actually from my init files.  There,
I specifically do not want to `require' everything I need as that would
slow down startup.  It also places undue burden on mere users that want
to customize this or that option from their init file.

And users can and do customize pretty much anything.  This is Emacs
after all.  So we can't really auto load stuff either...

Perhaps we need some special casing to detect what is an init file and
what is a package?  I have no idea how to do that reliably though, so it
might be a pipe dream.

Or maybe this will all be fixed, for some definition of fixed, when we
flip the default?

>> I'm not sure what's the best action we can take to reduce the verbosity
>> but keep developers informed at the same time.
>>
>>   Andrea
>
> To complete this answer, ATM is possible to gate all warnings reported
> by async native compilations leveraging the
> `comp-async-report-warnings-errors' customize.
>
> Is this sufficient to close this bug or do we like to discuss this
> default?
>
> My opinion (got it from the discussion on emacs-devel) is that at least
> for now would be good to keep `comp-async-report-warnings-errors' to t
> for the reason I've explained in the mail I'm quoting.  Perhaps we could
> think about revisiting this default but probably in the future.

Yes, I agree with the plan to leave the default for now and think about
changing it closer to release.

It is your decision to close this bug or not.  As far as I'm concerned,
I think the necessary clarifications have been made and it is clear that
you are on top of things.  The other option is to mark it as blocking
Emacs 28.1 (Bug#39202) as a reminder.

Thanks again for your hard work on this branch!  I've been using it
daily since August and it is running just fine here.

bug#44746: 28.0.50; [feature/native-comp] Noisy "*Warnings*" buffer shown on start

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Thu, 25 Feb 2021 17:59:18 -0600
> 
> The above reasoning is correct and valid for any ELisp package, I think.
> 
> But most of the warnings I see are actually from my init files.  There,
> I specifically do not want to `require' everything I need as that would
> slow down startup.  It also places undue burden on mere users that want
> to customize this or that option from their init file.

But you could bind that variable to nil when loading those init files,
right?

bug#44746: 28.0.50; [feature/native-comp] Noisy "*Warnings*" buffer shown on start

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> But you could bind that variable to nil when loading those init files,
> right?

I would assume it would work, but I haven't tried it.  But that should
solve the issue locally, yes.

bug#44746: 28.0.50; [feature/native-comp] Noisy "*Warnings*" buffer shown on start

[Eli Zaretskii]

> Cc: stefan@marxist.se
> Date: Thu, 25 Feb 2021 22:58:54 +0000
> From:  Andrea Corallo via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> To complete this answer, ATM is possible to gate all warnings reported
> by async native compilations leveraging the
> `comp-async-report-warnings-errors' customize.
> 
> Is this sufficient to close this bug or do we like to discuss this
> default?

I think we can close this, yes.  Basically, you are saying that most
of these warnings will go away once we get our act together and fix
all the missing 'require's.

Btw, I think the doc string of comp-async-report-warnings-errors could
use some love: as it stands now, I needed to read the code where it is
used to understand what it does, and why it is a defcustom.  Some of
the explanation you, Andrea, wrote in this thread could find its way
into the doc string and make it much more clear.

> My opinion (got it from the discussion on emacs-devel) is that at least
> for now would be good to keep `comp-async-report-warnings-errors' to t
> for the reason I've explained in the mail I'm quoting.

Agreed.

Thanks.

bug#44746: 28.0.50; [feature/native-comp] Noisy "*Warnings*" buffer shown on start

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: stefan@marxist.se
>> Date: Thu, 25 Feb 2021 22:58:54 +0000
>> From:  Andrea Corallo via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> To complete this answer, ATM is possible to gate all warnings reported
>> by async native compilations leveraging the
>> `comp-async-report-warnings-errors' customize.
>> 
>> Is this sufficient to close this bug or do we like to discuss this
>> default?
>
> I think we can close this, yes.  Basically, you are saying that most
> of these warnings will go away once we get our act together and fix
> all the missing 'require's.

Exactly, consider that (I think) most of these warnings are coming from
packages non in emacs.git.  Code in emacs.git being regularly compiled
during the build process in a fresh enviroment is typically cleaner from
this prespective IME.

> Btw, I think the doc string of comp-async-report-warnings-errors could
> use some love: as it stands now, I needed to read the code where it is
> used to understand what it does, and why it is a defcustom.  Some of
> the explanation you, Andrea, wrote in this thread could find its way
> into the doc string and make it much more clear.

Right I tried with ad74b1b2b6, please have a look an let me know in
case.

>> My opinion (got it from the discussion on emacs-devel) is that at least
>> for now would be good to keep `comp-async-report-warnings-errors' to t
>> for the reason I've explained in the mail I'm quoting.
>
> Agreed.
>
> Thanks.

Closing this then.

Thanks

  Andrea

bug#44746: 28.0.50; [feature/native-comp] Noisy "*Warnings*" buffer shown on start

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: 44746-done@debbugs.gnu.org, stefan@marxist.se
> Date: Fri, 26 Feb 2021 14:27:26 +0000
> 
> > Btw, I think the doc string of comp-async-report-warnings-errors could
> > use some love: as it stands now, I needed to read the code where it is
> > used to understand what it does, and why it is a defcustom.  Some of
> > the explanation you, Andrea, wrote in this thread could find its way
> > into the doc string and make it much more clear.
> 
> Right I tried with ad74b1b2b6, please have a look an let me know in
> case.

Thanks, it was very good.  I made some minor improvements (the main of
which is to tell that people who are annoyed by the warnings should
set this to a nil value).

bug#44746: 28.0.50; [feature/native-comp] Noisy "*Warnings*" buffer shown on start

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: 44746-done@debbugs.gnu.org, stefan@marxist.se
>> Date: Fri, 26 Feb 2021 14:27:26 +0000
>> 
>> > Btw, I think the doc string of comp-async-report-warnings-errors could
>> > use some love: as it stands now, I needed to read the code where it is
>> > used to understand what it does, and why it is a defcustom.  Some of
>> > the explanation you, Andrea, wrote in this thread could find its way
>> > into the doc string and make it much more clear.
>> 
>> Right I tried with ad74b1b2b6, please have a look an let me know in
>> case.
>
> Thanks, it was very good.  I made some minor improvements (the main of
> which is to tell that people who are annoyed by the warnings should
> set this to a nil value).

Thanks

  Andrea