[Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Robert Cochran]

Hello emacs-devel!

Now that the byte-compile-error-on-warn patch was merged (thanks by the
way!), this is the next step: I've marked most every file in lisp/
that currently byte-compiles without warnings with
'byte-compile-error-on-warn: t' in its file local variables.

I did an absolutely from-scratch build and it completed succesfully, as
well as running the full test suite with no significant errors. I may
have missed some files that should have been marked, but I took care to
*not* mark any files that had errors in them. The intended goal is to
not break the build, but have files that are already warning-free not
grow warnings. Files that still have warnings can be fixed as we go, ala
lexical-binding.

So, it's my understanding that there are some portions of what's in
emacs.git - org-mode being the prominent example in my mind - are simply
merges of a separate repository into emacs.git. Will it be necessary to
submit patches to the 'child' repositories rather than here? If so, how
can I find out which files are from said repositories?

This patch is rather large - the email patch form is 600k. The reason
for this is that I've touched quite a number of files - over 1,000
according to the Gitlab interface.

Thusly, I'm opting to link to the commit from my copy of the repository
rather than attach directly:

https://gitlab.com/RobertCochran/emacs/commit/f8446de4525910cf24dab7635124c3227c1ed1be

Gitlab also has the email form for convenience:

https://gitlab.com/RobertCochran/emacs/commit/f8446de4525910cf24dab7635124c3227c1ed1be.patch

Comments, suggestions, concerns, all that good stuff warmly welcomed.

Thanks,
-- 
~Robert Cochran

GPG Fingerprint - BD0C 5F8B 381C 64F0 F3CE  E7B9 EC9A 872C 41B2 77C2

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Eli Zaretskii]

> From: Robert Cochran <robert+Emacs@cochranmail.com>
> Date: Wed, 21 Feb 2018 10:30:24 -0800
> 
> Now that the byte-compile-error-on-warn patch was merged (thanks by the
> way!), this is the next step: I've marked most every file in lisp/
> that currently byte-compiles without warnings with
> 'byte-compile-error-on-warn: t' in its file local variables.

I'm not sure why we'd want this by default, but in any case please
don't do this with platform-specific files, like w32-fns.el or
term/x-win.el, because these almost always cause legitimate warnings
on platforms other than those for which they are written.

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Robert Cochran]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Cochran <robert+Emacs@cochranmail.com>
>> Date: Wed, 21 Feb 2018 10:30:24 -0800
>> 
>> Now that the byte-compile-error-on-warn patch was merged (thanks by the
>> way!), this is the next step: I've marked most every file in lisp/
>> that currently byte-compiles without warnings with
>> 'byte-compile-error-on-warn: t' in its file local variables.
>
> I'm not sure why we'd want this by default,

Because that extra bit of correctness is always nice to have. Some
classes of warning could very well turn out to be the root of errors. I
especially think that files in the core ought to be subject to slightly
stricter quality control anyways. Honestly, the warnings that annoy me
the most are the deprecation warnings. IMO, *nothing* in core should be
using deprecated functions/variables so far as it can be helped. If it
were up to me, I'd refuse patches that didn't fix other files when
deprecating functions. I'm of the belief that it looks sloppy.

I personally do not think it is unreasonable to expect that files in the
core compile 100% clean.

> but in any case please don't do this with platform-specific files,
> like w32-fns.el or term/x-win.el, because these almost always cause
> legitimate warnings on platforms other than those for which they are
> written.

OK, no problem. Those can go into the pile of files that still need
b-c-e-o-w off for the time being. How can I determine which files are
platform specific (because I have no idea on how to determine this)?

Thanks,
-- 
~Robert Cochran

GPG Fingerprint - BD0C 5F8B 381C 64F0 F3CE  E7B9 EC9A 872C 41B2 77C2

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Eli Zaretskii]

> From: Robert Cochran <robert+Emacs@cochranmail.com>
> Cc: Robert Cochran <robert+Emacs@cochranmail.com>,  emacs-devel@gnu.org
> Date: Wed, 21 Feb 2018 18:51:46 -0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Robert Cochran <robert+Emacs@cochranmail.com>
> >> Date: Wed, 21 Feb 2018 10:30:24 -0800
> >> 
> >> Now that the byte-compile-error-on-warn patch was merged (thanks by the
> >> way!), this is the next step: I've marked most every file in lisp/
> >> that currently byte-compiles without warnings with
> >> 'byte-compile-error-on-warn: t' in its file local variables.
> >
> > I'm not sure why we'd want this by default,
> 
> Because that extra bit of correctness is always nice to have. Some
> classes of warning could very well turn out to be the root of errors. I
> especially think that files in the core ought to be subject to slightly
> stricter quality control anyways.

I didn't say such a facility won't be useful, I just think that having
it on by default might not be the best idea, for exactly the same
reasons we don't have -Werror in the default compiler flags.  So this
feature should IMO definitely be turned off in the release tarballs.

We should also keep in mind that the development branches are tracked
by many people who have no interest or knowledge to fix these problems
when they pop up.  They just want to use "the latest and the
greatest".  For them, stopping the build cold in the tracks when some
recent change emits a warning is a terrible nuisance.  Which is why I
don't think this should be on by default even in repository builds.

> Honestly, the warnings that annoy me the most are the deprecation
> warnings. IMO, *nothing* in core should be using deprecated
> functions/variables so far as it can be helped. If it were up to me,
> I'd refuse patches that didn't fix other files when deprecating
> functions. I'm of the belief that it looks sloppy.

Given the wide-spread opinion that contribution to Emacs development
already presents too many high-bar obstacles, I very much doubt that
such a requirement, if enforced, will have any positive effect.  It
might have negative effects, like making people less willing to
contribute their code, or requiring the core team to fix these
problems when admitting changes.  Don't forget that we are still not
done requiring documentation changes to go with each change, and
that's even sloppier, at least in my eyes.

> I personally do not think it is unreasonable to expect that files in the
> core compile 100% clean.

We should be practical.  I agree with the goal, but the reality, and
the fact that the problem exists for such a long time, should teach us
something.  I don't expect such a policy to hold, given our lose
practices for installing patches.  (And I don't believe any
significantly different practices would work for Emacs.)

IME, nothing significant in Emacs gets done without a motivated
individual who is willing to work on that.  So I don't expect
byte-compilation warnings to disappear without a significant effort to
eliminate them.  Preventing a build from continuing is IMO a step in
the wrong direction, because its chances of encouraging people to fix
warnings for whose introduction they are not responsible are IME nil.
All this will bring us is more complaints and bug reports.

> How can I determine which files are platform specific (because I
> have no idea on how to determine this)?

Their names usually tell.  Look for files whose names start with "x"
or "w32" or "ns".  There are also Lisp files that support certain
optional features, and might emit warnings if compiled with Emacs that
was built without those features; there's no rule I can think of for
discovering those, but we can come up with a list.

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Stefan Monnier]

> Now that the byte-compile-error-on-warn patch was merged (thanks by the
> way!), this is the next step: I've marked most every file in lisp/
> that currently byte-compiles without warnings with
> 'byte-compile-error-on-warn: t' in its file local variables.

I sometimes consider it preferable to keep a warning than to silence it
(typically because I can't fix it (yet)).  This said, I'm fine with
adding such annotations for those files which currently don't have warnings.

> So, it's my understanding that there are some portions of what's in
> emacs.git - org-mode being the prominent example in my mind - are simply
> merges of a separate repository into emacs.git.

You'll want to be super extra careful with files that are also
distributed outside of Emacs, since they may emit warnings when compiled
by a different version of Emacs (typically one that doesn't exist yet).


        Stefan

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Robert Cochran]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Now that the byte-compile-error-on-warn patch was merged (thanks by the
>> way!), this is the next step: I've marked most every file in lisp/
>> that currently byte-compiles without warnings with
>> 'byte-compile-error-on-warn: t' in its file local variables.
>
> I sometimes consider it preferable to keep a warning than to silence it
> (typically because I can't fix it (yet)).  This said, I'm fine with
> adding such annotations for those files which currently don't have warnings.

Yeah, right now it is only marking files that emit no warnings
whatsoever. I intend for this to be a situation similar to
lexical-binding, where it is enabled on a per file as they get cleaned
up and become warning-free.

I am trying to keep warnings from lingering. The
string-{to,as}-{multi,uni}byte family of functions have been deprecated
since June 2016, which are some of the functions I see the most in the
deprecation warnings. Stuff like that.

>> So, it's my understanding that there are some portions of what's in
>> emacs.git - org-mode being the prominent example in my mind - are simply
>> merges of a separate repository into emacs.git.
>
> You'll want to be super extra careful with files that are also
> distributed outside of Emacs, since they may emit warnings when compiled
> by a different version of Emacs (typically one that doesn't exist yet).

Hmm, sounds like its better to just not do that with files distributed
in parallel. I don't have a problem with that (most files is better than
no files after all), but I have no idea which ones are and are not 'core
only' files. Guidance requested.

Thanks,
-- 
~Robert Cochran

GPG Fingerprint - BD0C 5F8B 381C 64F0 F3CE  E7B9 EC9A 872C 41B2 77C2

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Stefan Monnier]

> I am trying to keep warnings from lingering. The
> string-{to,as}-{multi,uni}byte family of functions have been deprecated
> since June 2016, which are some of the functions I see the most in the
> deprecation warnings. Stuff like that.

Indeed, these turn out to be difficult to fix:
- the potential replacement code is subtly different and it usually
  requires a detailed understanding of the code to know what's the right fix.
- in many places those function calls were added when Emacs's handling of
  `eight-bit` chars was different and it's difficult to know how that's
  affected by subsequent changes.
- there are various situations where there's no really good replacement.
  I guess that means that while they are marked as obsolete we really
  won't be getting rid of them any time soon.  And we can probably just
  silence the remaining warnings with `with-no-warnings` but only after
  we clearly document the need to use this function and what it would
  need to do otherwise.

> Hmm, sounds like its better to just not do that with files distributed
> in parallel. I don't have a problem with that (most files is better than
> no files after all), but I have no idea which ones are and are not 'core
> only' files. Guidance requested.

Good question.  After contributing to Emacs for almost 20 years
I generally "just know", but it's not 100% and it's not written down
anywhere for those who don't have that 20 year head start.

I agree it would be useful to document it.  Eli, John, any idea what
would be the best way to document those things?  Should we just add
a "Unbundled: yes" to the pseudo-headers, or maybe just (ab)use the
typical "URL: ..." pseudo-header for that?

As for which files are affected, I can't remember offhand, but a first
approximation is: org/*, mh-e/*, cc-*, vhdl and verilog, antlr and
those files mentioned in elpa.git/externals-list?


        Stefan

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Thu, 22 Feb 2018 00:24:37 -0500
> Cc: emacs-devel@gnu.org
> 
> - there are various situations where there's no really good replacement.
>   I guess that means that while they are marked as obsolete we really
>   won't be getting rid of them any time soon.  And we can probably just
>   silence the remaining warnings with `with-no-warnings` but only after
>   we clearly document the need to use this function and what it would
>   need to do otherwise.

I think we need a new set of specialized primitives and/or optional
arguments to existing primitives, in order to cover the cases where
there are no good replacements.

We declared those functions obsolete because they were used by Lisp
packages without a good understanding of what they do and how to do
the same "correctly". Most, if not all, of the remaining warnings in
our own sources are of the kind where the proposed alternatives won't
do.  So I think we want to keep the obsolescence, but we need
additional features to solve the problems without using these
functions.

> > Hmm, sounds like its better to just not do that with files distributed
> > in parallel. I don't have a problem with that (most files is better than
> > no files after all), but I have no idea which ones are and are not 'core
> > only' files. Guidance requested.
> 
> Good question.  After contributing to Emacs for almost 20 years
> I generally "just know", but it's not 100% and it's not written down
> anywhere for those who don't have that 20 year head start.
> 
> I agree it would be useful to document it.  Eli, John, any idea what
> would be the best way to document those things?  Should we just add
> a "Unbundled: yes" to the pseudo-headers, or maybe just (ab)use the
> typical "URL: ..." pseudo-header for that?

A new keyword sounds good.

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > - there are various situations where there's no really good replacement.

Maybe that means we shouldn't declare them obsolete.

Can you describe some of these situations and what the problem is?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Date: Fri, 23 Feb 2018 04:47:24 -0500
> Cc: emacs-devel@gnu.org
> 
>   > - there are various situations where there's no really good replacement.
> 
> Maybe that means we shouldn't declare them obsolete.

No, I think we should continue declaring them obsolete, because in
many cases they are used incorrectly, and produce subtle bugs.  For
example, in some locales, string-make-unibyte produces the same
results as encode-coding-string, so people could mistakenly use the
former instead of the latter, if that happens to work in their locale.

> Can you describe some of these situations and what the problem is?

One such case is in ruler.el; search for "unibyte".  This use case
could be fixed by adding an optional argument to make-string, which
would force it to always produce a multibyte string -- that's the kind
of alternatives I had in mind, which should help us get rid of these
obsolete functions in the few legitimate uses we still have.

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > One such case is in ruler.el; search for "unibyte".  This use case
  > could be fixed by adding an optional argument to make-string, which
  > would force it to always produce a multibyte string -- that's the kind
  > of alternatives I had in mind, which should help us get rid of these
  > obsolete functions in the few legitimate uses we still have.

How about rewriting that code?  That's the way to verify these
constructs really are not necessary.
-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> CC: monnier@IRO.UMontreal.CA, emacs-devel@gnu.org
> Date: Sat, 24 Feb 2018 18:33:16 -0500
> 
>   > One such case is in ruler.el; search for "unibyte".  This use case
>   > could be fixed by adding an optional argument to make-string, which
>   > would force it to always produce a multibyte string -- that's the kind
>   > of alternatives I had in mind, which should help us get rid of these
>   > obsolete functions in the few legitimate uses we still have.
> 
> How about rewriting that code?

If you mean rewriting to eliminate the need to use aref, then that's
more than I can afford to invest in this particular package.  Patches
to that effect will be welcome, though.

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Stefan Monnier]

>> Can you describe some of these situations and what the problem is?
> One such case is in ruler.el; search for "unibyte".  This use case
                ^^        ^^                ^^^
                was?     -mode             multi
IIUC.


        Stefan

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: rms@gnu.org, emacs-devel@gnu.org
> Date: Sun, 25 Feb 2018 08:13:40 -0500
> 
> >> Can you describe some of these situations and what the problem is?
> > One such case is in ruler.el; search for "unibyte".  This use case
>                 ^^        ^^                ^^^
>                 was?     -mode             multi
> IIUC.

ruler-mode.el, yes.  As for the rest: I was looking at emacs-26,
whereas you are looking at master, where I already fixed that (and
forgot about it).  Still, the way I fixed that is an example of what I
meant by "alternatives".

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Eli Zaretskii]

> From: Robert Cochran <robert+Emacs@cochranmail.com>
> Date: Wed, 21 Feb 2018 19:05:11 -0800
> Cc: emacs-devel@gnu.org
> 
> > I sometimes consider it preferable to keep a warning than to silence it
> > (typically because I can't fix it (yet)).  This said, I'm fine with
> > adding such annotations for those files which currently don't have warnings.
> 
> Yeah, right now it is only marking files that emit no warnings
> whatsoever.

I don't see why a file that today emits warnings should be exempted
from the policy (if we accept it).  I think the current state of
affairs is entirely accidental, in the sense that it is the result of
the fact that no one has seriously worked yet on the files that do
emit warnings.

So I find this aspect of the proposed change puzzling.  Either we care
about the warnings or we don't.  If we do, we shouldn't tolerate them
anywhere.

> I intend for this to be a situation similar to lexical-binding,
> where it is enabled on a per file as they get cleaned up and become
> warning-free.

Lack of lexical-binding was not an evidence of something being wrong
with the file.  So I don't see the analogy there.

> I am trying to keep warnings from lingering. The
> string-{to,as}-{multi,uni}byte family of functions have been deprecated
> since June 2016, which are some of the functions I see the most in the
> deprecation warnings. Stuff like that.

I don't see how stopping the build will miraculously help us fix these
warnings in a reasonably correct way.  These warnings are hard to fix
(see my other message about that).  Moreover, there are warnings
triggered by bugs or misfeatures in the byte compiler -- e.g., try
byte-compiling gud.el.  They can be shut up by kludgey workarounds,
but do we really want our code to use such kludges, just to avoid the
warnings?

I predict that the result of making warnings stop the build will be
abundance of kludges in the code, which will not be adequately
explained in the sources, and so 10 years from now we will have the
same situation as with string-to-unibyte, where no one really knows
why we do that, and how to test whether the original reason is gone.

IMO, the only way to eliminate the warnings is to start the work on
fixing them one by one.  This requires motivated individuals, and this
requires time.  Making warnings break the build is not the right way,
IMO, it will just harm development by diverting part of our scarce
resources to inventing kludges.

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Noam Postavsky]

On Wed, Feb 21, 2018 at 1:30 PM, Robert Cochran
<robert+Emacs@cochranmail.com> wrote:

> This patch is rather large - the email patch form is 600k. The reason
> for this is that I've touched quite a number of files - over 1,000
> according to the Gitlab interface.

If you instead set byte-compile-error-on-warn to t by default (e.g.,
on the command line) and just set it to nil in files which trigger a
warning, would the patch be bigger or larger?

By the way, I think it would be better to set it in the local
variables list at the end of the file, rather than in the first line.
For lexical-binding it's required to put it there for technical
reasons, but I think those don't apply to byte-compile-error-on-warn,
and that first line can get a bit crowded.

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Robert Cochran]

Noam Postavsky <npostavs@gmail.com> writes:

> On Wed, Feb 21, 2018 at 1:30 PM, Robert Cochran
> <robert+Emacs@cochranmail.com> wrote:
>
>> This patch is rather large - the email patch form is 600k. The reason
>> for this is that I've touched quite a number of files - over 1,000
>> according to the Gitlab interface.
>
> If you instead set byte-compile-error-on-warn to t by default (e.g.,
> on the command line) and just set it to nil in files which trigger a
> warning, would the patch be bigger or larger?

According to my blacklist and whitelist files, 179 warning files vs 1319
non-warning files, so significantly smaller it seems. I'll look into
modifying the Makefile.in to accommodate this.

> By the way, I think it would be better to set it in the local
> variables list at the end of the file, rather than in the first line.
> For lexical-binding it's required to put it there for technical
> reasons, but I think those don't apply to byte-compile-error-on-warn,
> and that first line can get a bit crowded.

I generated this mechanically via add-file-local-variable-prop-line in
an Emacs Lisp batch script. Is there a mechanism by which I can use
a-f-l-v-p-l or some other function to put it at the bottom?

Thanks,
-- 
~Robert Cochran

GPG Fingerprint - BD0C 5F8B 381C 64F0 F3CE  E7B9 EC9A 872C 41B2 77C2

Re: [Patch] Enable byte-compile-error-on-wran for error-free files in lisp/

[Noam Postavsky]

On Wed, Feb 21, 2018 at 9:29 PM, Robert Cochran
<robert+Emacs@cochranmail.com> wrote:

>> If you instead set byte-compile-error-on-warn to t by default (e.g.,
>> on the command line) and just set it to nil in files which trigger a
>> warning, would the patch be bigger or [smaller]?
>
> According to my blacklist and whitelist files, 179 warning files vs 1319
> non-warning files, so significantly smaller it seems. I'll look into
> modifying the Makefile.in to accommodate this.

Marking warning files also has the advantage of avoiding problems for
files which are distributed outside of Emacs.
For lisp/cedet/ files we should probably just set it to nil in a
.dir-locals file, since there's not much point in fixing up warnings
while the cedet merge problem remains unresolved.

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28878

>> By the way, I think it would be better to set it in the local
>> variables list at the end of the file, rather than in the first line.
>> For lexical-binding it's required to put it there for technical
>> reasons, but I think those don't apply to byte-compile-error-on-warn,
>> and that first line can get a bit crowded.
>
> I generated this mechanically via add-file-local-variable-prop-line in
> an Emacs Lisp batch script. Is there a mechanism by which I can use
> a-f-l-v-p-l or some other function to put it at the bottom?

add-file-local-variable adds to the bottom, I believe.