Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH] "use warnings 'FATAL';" and related have no effect #13521

Closed
p5pRT opened this issue Jan 13, 2014 · 15 comments
Closed

[PATCH] "use warnings 'FATAL';" and related have no effect #13521

p5pRT opened this issue Jan 13, 2014 · 15 comments
Labels

Comments

@p5pRT
Copy link

p5pRT commented Jan 13, 2014

Migrated from rt.perl.org#120977 (status was 'resolved')

Searchable as RT120977$

@p5pRT
Copy link
Author

p5pRT commented Jan 13, 2014

From @haukex

Dear Perl 5 Porters,

The statements

  use warnings "FATAL";
  use warnings "NONFATAL";
  no warnings "FATAL";

have no effect on the status of warnings at all.

An unsuspecting user may (accidentally) use one of the first two and
never notice that they actually have no warnings enabled at all.

At the very least, I think the above should generate warnings or errors
about incorrect usage.

However, after thinking about it a bit, what makes the most sense to me
would be if the above statements behave as if they were

  use warnings FATAL => "all";
  use warnings NONFATAL => "all";
  no warnings FATAL => "all";

Based on this idea I created a patch which I have attached for your
consideration and discussion. The implementation may be a little naive,
but hopefully someone with more knowledge in warnings.pm might be able
to optimize it :-)

I've also included a patch that fixes a missing word in perllexwarn.pod.

Best Regards,
-- Hauke D



Flags​:
  category=core
  severity=low


Site configuration information for perl 5.19.8​:

Configured by haukex at Mon Jan 13 02​:26​:42 CET 2014.

Summary of my perl5 (revision 5 version 19 subversion 8) configuration​:
  Commit id​: 55ff7cc3477747dec7e0883473a9cf7ac1df0f30
  Platform​:
  osname=linux, osvers=3.8.0-35-generic, archname=i686-linux
  uname='linux devbox 3.8.0-35-generic #50~precise1-ubuntu smp wed
dec 4 17​:28​:45 utc 2013 i686 i686 i386 gnulinux '
  config_args='-des -Dusedevel'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=undef, usemultiplicity=undef
  use64bitint=undef, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
  optimize='-O2',
  cppflags='-fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include'
  ccversion='', gccversion='4.6.3', gccosandvers=''
  intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
  ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
  alignbytes=4, prototype=define
  Linker and Libraries​:
  ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib /lib/i386-linux-gnu /lib/../lib
/usr/lib/i386-linux-gnu /usr/lib/../lib /lib /usr/lib
  libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
  perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
  libc=, so=so, useshrplib=false, libperl=libperl.a
  gnulibc_version='2.15'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
  cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib
-fstack-protector'


@​INC for perl 5.19.8​:
  lib
  /usr/local/lib/perl5/site_perl/5.19.8/i686-linux
  /usr/local/lib/perl5/site_perl/5.19.8
  /usr/local/lib/perl5/5.19.8/i686-linux
  /usr/local/lib/perl5/5.19.8
  .


Environment for perl 5.19.8​:
  HOME=/home/haukex
  LANG=en_US.UTF-8
  LANGUAGE=en_US​:en
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)

PATH=/opt/perl5.18/bin​:/usr/local/sbin​:/usr/local/bin​:/usr/sbin​:/usr/bin​:/sbin​:/bin​:/usr/games
  PERL_BADLANG (unset)
  SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Jan 13, 2014

From @haukex

From 611d05d25a8e43e1f29118b7b29e4300d5463eef Mon Sep 17 00​:00​:00 2001
From​: Hauke D <haukex@​zero-g.net>
Date​: Mon, 13 Jan 2014 00​:46​:03 +0100
Subject​: [PATCH 1/2] add a missing word in perllexwarn
MIME-Version​: 1.0
Content-Type​: multipart/mixed; boundary="------------1.8.5.2"

This is a multi-part message in MIME format.
--------------1.8.5.2
Content-Type​: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding​: 8bit


pod/perllexwarn.pod | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

--------------1.8.5.2
Content-Type​: text/x-patch; name="0001-add-a-missing-word-in-perllexwarn.patch"
Content-Transfer-Encoding​: 8bit
Content-Disposition​: attachment; filename="0001-add-a-missing-word-in-perllexwarn.patch"

Inline Patch
diff --git a/pod/perllexwarn.pod b/pod/perllexwarn.pod
index 0c849a4..0e65cbc 100644
--- a/pod/perllexwarn.pod
+++ b/pod/perllexwarn.pod
@@ -386,7 +386,7 @@ When run it produces this output
     Useless use of length in void context at fatal line 7.  
 
 The scope where C<length> is used has escalated the C<void> warnings
-category into a fatal error, so the program terminates immediately it
+category into a fatal error, so the program terminates immediately when it
 encounters the warning.
 
 To explicitly turn off a "FATAL" warning you just disable the warning

--------------1.8.5.2--



From 55ff7cc3477747dec7e0883473a9cf7ac1df0f30 Mon Sep 17 00:00:00 2001 From​: Hauke D \ Date​: Mon\, 13 Jan 2014 00​:50​:51 \+0100 Subject​: \[PATCH 2/2\] assume "all" in "use warnings 'FATAL';" and related MIME\-Version​: 1\.0 Content\-Type​: multipart/mixed; boundary="\-\-\-\-\-\-\-\-\-\-\-\-1\.8\.5\.2"

This is a multi-part message in MIME format.
--------------1.8.5.2
Content-Type​: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding​: 8bit

Until now, the statements

use warnings "FATAL";
use warnings "NONFATAL";
no warnings "FATAL";

had no effect on the status of warnings at all. This change causes them
to be handled with an implied "all" at the end of the import list.


AUTHORS | 1 +
lib/warnings.pm | 8 ++++++--
pod/perllexwarn.pod | 14 +++++++++++++
regen/warnings.pl | 8 ++++++--
t/lib/warnings/7fatal | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 83 insertions(+), 4 deletions(-)

--------------1.8.5.2
Content-Type​: text/x-patch; name="0002-assume-all-in-use-warnings-FATAL-and-related.patch"
Content-Transfer-Encoding​: 8bit
Content-Disposition​: attachment; filename="0002-assume-all-in-use-warnings-FATAL-and-related.patch"

Inline Patch
diff --git a/AUTHORS b/AUTHORS
index 2d6c583..65eae39 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -459,6 +459,7 @@ Harmen				<[email protected]>
 Harmon S. Nine			<[email protected]>
 Harri Pasanen			<[email protected]>
 Harry Edmon			<[email protected]>
+Hauke D			<[email protected]>
 Helmut Jarausch			<[email protected]>
 Henrik Tougaard			<[email protected]>
 Herbert Breunung		<[email protected]>
diff --git a/lib/warnings.pm b/lib/warnings.pm
index 440bfdd..119ca3e 100644
--- a/lib/warnings.pm
+++ b/lib/warnings.pm
@@ -5,7 +5,7 @@
 
 package warnings;
 
-our $VERSION = '1.20';
+our $VERSION = '1.21';
 
 # Verify that we're called correctly so that warnings will work.
 # see also strict.pm.
@@ -421,6 +421,9 @@ sub import
         $mask |= $Bits{'all'} ;
         $mask |= $DeadBits{'all'} if vec($mask, $Offsets{'all'}+1, 1);
     }
+
+    # append 'all' when implied (after a lone "FATAL" or "NONFATAL")
+    push @_, 'all' if @_==1 && ( $_[0] eq 'FATAL' || $_[0] eq 'NONFATAL' );
     
     # Empty @_ is equivalent to @_ = 'all' ;
     ${^WARNING_BITS} = @_ ? _bits($mask, @_) : $mask | $Bits{all} ;
@@ -438,7 +441,8 @@ sub unimport
         $mask |= $DeadBits{'all'} if vec($mask, $Offsets{'all'}+1, 1);
     }
 
-    push @_, 'all' unless @_;
+    # append 'all' when implied (empty import list or after a lone "FATAL")
+    push @_, 'all' if !@_ || @_==1 && $_[0] eq 'FATAL';
 
     foreach my $word ( @_ ) {
 	if ($word eq 'FATAL') {
diff --git a/pod/perllexwarn.pod b/pod/perllexwarn.pod
index 0e65cbc..b00db82 100644
--- a/pod/perllexwarn.pod
+++ b/pod/perllexwarn.pod
@@ -403,6 +403,20 @@ except for those in the "syntax" category.
 
     use warnings FATAL => 'all', NONFATAL => 'syntax';
 
+As of Perl 5.20, you can shorten C<< use warnings FATAL => 'all'; >> like
+this:
+
+    use v5.20;   # this is *required* for the following line to work
+    use warnings 'FATAL';   # normally: use warnings FATAL => 'all';
+
+Because this requires Perl 5.20, you should not use it if you want your
+scripts to remain backwards compatible, and spell it out instead.
+(In previous versions of Perl, the statements
+C<< use warnings 'FATAL'; >>, C<< use warnings 'NONFATAL'; >>
+and C<< no warnings 'FATAL'; >> actually had no effect on the status of
+warnings at all. As of Perl 5.20 they behave as if they included the
+C<< => 'all' >> portion.)
+
 B<NOTE:> Users of FATAL warnings, especially
 those using C<< FATAL => 'all' >>
 should be fully aware that they are risking future portability of their
diff --git a/regen/warnings.pl b/regen/warnings.pl
index e8dcf4a..72774ca 100644
--- a/regen/warnings.pl
+++ b/regen/warnings.pl
@@ -476,7 +476,7 @@ close_and_rename($lexwarn);
 __END__
 package warnings;
 
-our $VERSION = '1.20';
+our $VERSION = '1.21';
 
 # Verify that we're called correctly so that warnings will work.
 # see also strict.pm.
@@ -691,6 +691,9 @@ sub import
         $mask |= $Bits{'all'} ;
         $mask |= $DeadBits{'all'} if vec($mask, $Offsets{'all'}+1, 1);
     }
+
+    # append 'all' when implied (after a lone "FATAL" or "NONFATAL")
+    push @_, 'all' if @_==1 && ( $_[0] eq 'FATAL' || $_[0] eq 'NONFATAL' );
     
     # Empty @_ is equivalent to @_ = 'all' ;
     ${^WARNING_BITS} = @_ ? _bits($mask, @_) : $mask | $Bits{all} ;
@@ -708,7 +711,8 @@ sub unimport
         $mask |= $DeadBits{'all'} if vec($mask, $Offsets{'all'}+1, 1);
     }
 
-    push @_, 'all' unless @_;
+    # append 'all' when implied (empty import list or after a lone "FATAL")
+    push @_, 'all' if !@_ || @_==1 && $_[0] eq 'FATAL';
 
     foreach my $word ( @_ ) {
 	if ($word eq 'FATAL') {
diff --git a/t/lib/warnings/7fatal b/t/lib/warnings/7fatal
index 6eeac74..add39b1 100644
--- a/t/lib/warnings/7fatal
+++ b/t/lib/warnings/7fatal
@@ -433,3 +433,59 @@ print STDERR "The End.\n" ;
 EXPECT
 Unsuccessful open on filename containing newline at - line 5.
 close() on unopened filehandle fred at - line 6.
+########
+
+# 'use warnings NONFATAL=>"all"' should be the same as 'use warnings'
+use warnings NONFATAL=>"all" ;
+my $a = oct "7777777777777777777777777777777777778" ;
+EXPECT
+Integer overflow in octal number at - line 4.
+Illegal octal digit '8' ignored at - line 4.
+Octal number > 037777777777 non-portable at - line 4.
+########
+
+# 'use warnings "NONFATAL"' should be the same as 'use warnings'
+use warnings "NONFATAL";
+my $a = oct "7777777777777777777777777777777777778" ;
+EXPECT
+Integer overflow in octal number at - line 4.
+Illegal octal digit '8' ignored at - line 4.
+Octal number > 037777777777 non-portable at - line 4.
+########
+
+# 'use warnings "FATAL"' should be the same as 'use warnings FATAL=>"all"'
+use warnings "FATAL" ;
+{
+    no warnings ;
+    my $a =+ 1 ;
+}
+my $a =+ 1 ;
+print STDERR "The End.\n" ;
+EXPECT
+Reversed += operator at - line 8.
+########
+
+# 'no warnings FATAL=>"all"' should be the same as 'no warnings'
+use warnings ;
+{
+    no warnings FATAL=>"all" ;
+    my $a =+ 1 ;
+}
+my $a =+ 1 ;
+print STDERR "The End.\n" ;
+EXPECT
+Reversed += operator at - line 8.
+The End.
+########
+
+# 'no warnings "FATAL"' should be the same as 'no warnings'
+use warnings ;
+{
+    no warnings "FATAL" ;
+    my $a =+ 1 ;
+}
+my $a =+ 1 ;
+print STDERR "The End.\n" ;
+EXPECT
+Reversed += operator at - line 8.
+The End.

--------------1.8.5.2--

@p5pRT
Copy link
Author

p5pRT commented Jan 14, 2014

From @tonycoz

On Sun Jan 12 18​:23​:01 2014, haukex@​zero-g.net wrote​:

The statements

use warnings "FATAL";
use warnings "NONFATAL";
no warnings "FATAL";

have no effect on the status of warnings at all.

An unsuspecting user may (accidentally) use one of the first two and
never notice that they actually have no warnings enabled at all.

At the very least, I think the above should generate warnings or
errors
about incorrect usage.

However, after thinking about it a bit, what makes the most sense to
me
would be if the above statements behave as if they were

use warnings FATAL => "all";
use warnings NONFATAL => "all";
no warnings FATAL => "all";

Based on this idea I created a patch which I have attached for your
consideration and discussion. The implementation may be a little
naive,
but hopefully someone with more knowledge in warnings.pm might be able
to optimize it :-)

This change makes sense to me.

The only possible problem I can see is if some module is calling
warnings​::import("FATAL", @​foo) and expecting an empty @​foo to be a no-op.

I plan to apply this unless someone responds with an argument against it.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jan 14, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Jan 14, 2014

From @haukex

On Mon Jan 13 16​:22​:34 2014, tonyc wrote​:

On Sun Jan 12 18​:23​:01 2014, haukex@​zero-g.net wrote​:

The statements

use warnings "FATAL";
use warnings "NONFATAL";
no warnings "FATAL";

have no effect on the status of warnings at all.

This change makes sense to me.

The only possible problem I can see is if some module is calling
warnings​::import("FATAL", @​foo) and expecting an empty @​foo to be a no-op.

I plan to apply this unless someone responds with an argument against it.

Tony

Hi,

Thanks for taking this!

I investigated a bit more and found that while the central point of the issue is still the same, it wasn't 100% accurate to say there is currently no effect on warnings at all.

I've attached a quick test script to look at how the warning bits change. On Perl v5.10.1, the statements really do have no effect on the warnings status at all. On Perl v5.18.2, it looks like only the $DEFAULT warnings are turned on, but not made fatal by both "use warnings 'NONFATAL';" and "use warnings 'FATAL';". That behavior still doesn't really make sense and I doubt if anyone is using it in this undocumented way on purpose, so I'm sticking with my suggestion.

So I could make the perllexwarn wording more accurate and provide an updated patch tomorrow or the day after, if that's not too late.

Regards,
-- Hauke D

P.S. I sent a similar comment to perlbug-followup@​perl.org yesterday but it hasn't shown up here yet, sorry if this turns out to be a dupe.

@p5pRT
Copy link
Author

p5pRT commented Jan 14, 2014

From @haukex

#!/usr/bin/env perl
use strict;

print "This is Perl $^V\n";

sub show { print sprintf("%-24s​: ",shift),unpack("b*",${^WARNING_BITS}),"\n" }

BEGIN { show "before" }
use warnings "FATAL";
BEGIN { show "use warn FATAL" }
use warnings "NONFATAL";
BEGIN { show "use warn NONFATAL" }
use warnings;
BEGIN { show "use warn" }
use warnings NONFATAL=>"all";
BEGIN { show "use warn NONFATAL all" }
use warnings FATAL=>"all";
BEGIN { show "use warn FATAL all" }

no warnings "FATAL";
BEGIN { show "no warn FATAL" }
no warnings FATAL=>"all";
BEGIN { show "no warn FATAL all" }

use warnings;
BEGIN { show "use warn" }
no warnings;
BEGIN { show "no warn" }

@p5pRT
Copy link
Author

p5pRT commented Jan 14, 2014

From @haukex

Hi,

Investigating this a little further shows that while the spirit of the
bug is still the same, it wasn't 100% accurate to say there is no effect
on warnings at all.

See the attached test script​: on Perl v5.18.2, it looks like the default
warning categories are turned on, but not made fatal by "use warnings
'FATAL';". On Perl v5.10.1, the statement does have no effect on the
warnings status.

Best Regards,
-- Hauke D

@p5pRT
Copy link
Author

p5pRT commented Jan 14, 2014

From @haukex

#!/usr/bin/env perl
use strict;

print "This is Perl $^V\n";

sub show { print sprintf("%-24s​: ",shift),unpack("b*",${^WARNING_BITS}),"\n" }

BEGIN { show "before" }
use warnings "FATAL";
BEGIN { show "use warn FATAL" }
use warnings "NONFATAL";
BEGIN { show "use warn NONFATAL" }
use warnings;
BEGIN { show "use warn" }
use warnings NONFATAL=>"all";
BEGIN { show "use warn NONFATAL all" }
use warnings FATAL=>"all";
BEGIN { show "use warn FATAL all" }

no warnings "FATAL";
BEGIN { show "no warn FATAL" }
no warnings FATAL=>"all";
BEGIN { show "no warn FATAL all" }

use warnings;
BEGIN { show "use warn" }
no warnings;
BEGIN { show "no warn" }

@p5pRT
Copy link
Author

p5pRT commented Jan 14, 2014

From @khwilliamson

On 01/13/2014 09​:14 PM, Hauke D via RT wrote​:

So I could make the perllexwarn wording more accurate and provide an updated patch tomorrow or the day after, if that's not too late.

It's never too late; although at times it may be too late for the
current release. Doc patches are generally accepted quite late in the
cycle.

@p5pRT
Copy link
Author

p5pRT commented Jan 15, 2014

From @tonycoz

On Mon Jan 13 20​:14​:59 2014, haukex@​zero-g.net wrote​:

On Mon Jan 13 16​:22​:34 2014, tonyc wrote​:

On Sun Jan 12 18​:23​:01 2014, haukex@​zero-g.net wrote​:

The statements

use warnings "FATAL";
use warnings "NONFATAL";
no warnings "FATAL";

have no effect on the status of warnings at all.

This change makes sense to me.

The only possible problem I can see is if some module is calling
warnings​::import("FATAL", @​foo) and expecting an empty @​foo to be a
no-op.

I plan to apply this unless someone responds with an argument against
it.

Tony

Hi,

Thanks for taking this!

I investigated a bit more and found that while the central point of
the issue is still the same, it wasn't 100% accurate to say there is
currently no effect on warnings at all.

I've attached a quick test script to look at how the warning bits
change. On Perl v5.10.1, the statements really do have no effect on
the warnings status at all. On Perl v5.18.2, it looks like only the
$DEFAULT warnings are turned on, but not made fatal by both "use
warnings 'NONFATAL';" and "use warnings 'FATAL';". That behavior still
doesn't really make sense and I doubt if anyone is using it in this
undocumented way on purpose, so I'm sticking with my suggestion.

So I could make the perllexwarn wording more accurate and provide an
updated patch tomorrow or the day after, if that's not too late.

An updated patch is fine.

I don't think this is so super-critical that is has to go in 5.20 - and
2014-01-20 is the contentious changes freeze - this change doesn't appear to be
(or no-one is reading the list.)

Tony

@p5pRT
Copy link
Author

p5pRT commented Jan 16, 2014

From @haukex

Hi,

Attached is a new version of the patch. I've updated the documentation and expanded the tests a little bit. Also, this patch doesn't contain the warnings.pm version number update from 1.20 to 1.21 like it did before because the recent commit 2fe4abd did the same thing and I wasn't sure if the version number needs to be upped again?

Unless anyone notices anything else I think this patch should be all set.

Regards,
-- Hauke D

@p5pRT
Copy link
Author

p5pRT commented Jan 16, 2014

From @haukex

patches120977-v2

@p5pRT
Copy link
Author

p5pRT commented Jan 22, 2014

From @tonycoz

On Wed Jan 15 16​:45​:25 2014, haukex@​zero-g.net wrote​:

Hi,

Attached is a new version of the patch. I've updated the documentation
and expanded the tests a little bit. Also, this patch doesn't contain
the warnings.pm version number update from 1.20 to 1.21 like it did
before because the recent commit 2fe4abd did the same thing and I
wasn't sure if the version number needs to be upped again?

Unless anyone notices anything else I think this patch should be all
set.

Applied as e214621 and c91312d (with a minor AUTHORS alignment fix), with a version bump in 76ff28b, since there's been a release.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jan 22, 2014

@tonycoz - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this as completed Jan 22, 2014
@p5pRT
Copy link
Author

p5pRT commented Jan 22, 2014

From @haukex

Hi,

Thanks!

Regards,
-- Hauke D

Am 22.01.2014 06​:37, schrieb Tony Cook via RT​:

On Wed Jan 15 16​:45​:25 2014, haukex@​zero-g.net wrote​:

Hi,

Attached is a new version of the patch. I've updated the documentation
and expanded the tests a little bit. Also, this patch doesn't contain
the warnings.pm version number update from 1.20 to 1.21 like it did
before because the recent commit 2fe4abd did the same thing and I
wasn't sure if the version number needs to be upped again?

Unless anyone notices anything else I think this patch should be all
set.

Applied as e214621 and c91312d (with a minor AUTHORS alignment fix), with a version bump in 76ff28b, since there's been a release.

Tony

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=120977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant