Carp.pm useless warning "Use of uninitialized value $error[1] in join or string" #16815
Comments
From wagnerc@plebeian.comMessage RFC822: This is a bug report for perl from wagnerc@plebeian.com, The Carp 1.50 main functions will print a useless and confusing warning about the use of an uninitialized value if one of the arguments to the given function is undef. This breaks the encapsulation principle of Carp to "report the caller's errors, not the ones it might happen to tickle while doing so." Examples: $ perl -MCarp -e 'sub{Carp::confess "foo", undef, "bar"}->();' $ perl -MCarp -e 'package X; sub{Carp::croak "foo", undef, "bar"}->();' $ perl -MCarp -e 'package X; sub{Carp::cluck "foo", undef, "bar"}->();' Fix: sub ret_backtrace {
sub ret_summary {
It might still be desirable to report about the undef argument but it should report from the caller's location about the caller's use of an undef to a function that "shouldn't" have undef arguments. Thanks. [Please do not change anything below this line]Flags:
|
From wagnerc@plebeian.comCreated by wagnerc@plebeian.comThis is a bug report for perl from wagnerc@plebeian.com, ----------------------------------------------------------------- Examples: $ perl -MCarp -e 'sub{Carp::confess "foo", undef, "bar"}->();' $ perl -MCarp -e 'package X; sub{Carp::croak "foo", undef, "bar"}->();' $ perl -MCarp -e 'package X; sub{Carp::cluck "foo", undef, "bar"}->();' Fix: sub ret_backtrace { sub ret_summary { It might still be desirable to report about the undef argument but it should report from the caller's location about the caller's use of an undef to a function that "shouldn't" have undef arguments. Thanks. Perl Info |
From @jkeenanOn 1/18/19 10:31 PM, Chris Wagner (via RT) wrote:
Are you quoting "report the caller's errors ..." from some I ask because I don't find that phrasing in Carp's documentation, even
Would we be better off simply suppressing the uninitialized value
Thank you very much. |
|
The RT System itself - Status changed from 'new' to 'open' |
From @jkeenanOn Sat, 19 Jan 2019 15:34:22 GMT, jkeenan@pobox.com wrote:
OTOH, filtering out the arguments which are undefined values would be consistent with the 'warn' and 'die' built-ins which 'carp' and 'croak' emulate. ##### $ perl -e 'die("alpha", undef, "beta") unless 1 == 0;'
-- |
From wagnerc@plebeian.comOn Sat, 19 Jan 2019 07:34:22 -0800, jkeenan@pobox.com wrote:
That comes from caller_info() in Carp.pm # poke around inside the stack. Inside of Carp.pm it makes little |
From wagnerc@plebeian.comOn Sat, 19 Jan 2019 08:03:37 -0800, jkeenan wrote:
Eh, warn() and die() are not that nice. :) $ perl -w I say we just add the defined check because undef()'s just smash down to blank anyway. We "could": Thanks. |
From @tonycozOn Sun, 20 Jan 2019 14:48:35 -0800, wagnerc@plebeian.com wrote:
This makes sense to me. Tony |
From @jkeenanOn Mon, 21 Jan 2019 00:52:46 GMT, tonyc wrote:
Please evaluate the patch attached, which is available for smoke-testing in the smoke-me/jkeenan/133776-carp-uninit-value-warning branch. Thank you very much. -- |
From @jkeenan0001-Suppress-uninitialized-value-warning-if-one-argument.patchFrom 78baa69d418e2a5fc200846cd48a1618dec29913 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Sun, 20 Jan 2019 21:29:02 -0500
Subject: [PATCH] Suppress uninitialized value warning if one argument is
'undef'.
As suggested by Chris Wagner. For: RT #133776
---
dist/Carp/lib/Carp.pm | 10 +++-
dist/Carp/t/rt133776-no-warn-on-undef-args.t | 49 ++++++++++++++++++++
2 files changed, 57 insertions(+), 2 deletions(-)
create mode 100644 dist/Carp/t/rt133776-no-warn-on-undef-args.t
diff --git a/dist/Carp/lib/Carp.pm b/dist/Carp/lib/Carp.pm
index 109b7fec77..d53308ac3e 100644
--- a/dist/Carp/lib/Carp.pm
+++ b/dist/Carp/lib/Carp.pm
@@ -585,7 +585,10 @@ BEGIN {
sub ret_backtrace {
my ( $i, @error ) = @_;
my $mess;
- my $err = join '', @error;
+ if (warnings::enabled("uninitialized") && scalar grep {!defined} @error) {
+ carp "Use of uninitialized value in error message";
+ }
+ my $err = join '', grep defined, @error;
$i++;
my $tid_msg = '';
@@ -627,7 +630,10 @@ sub ret_backtrace {
sub ret_summary {
my ( $i, @error ) = @_;
- my $err = join '', @error;
+ if (warnings::enabled("uninitialized") && scalar grep {!defined} @error) {
+ carp "Use of uninitialized value in error message";
+ }
+ my $err = join '', grep defined, @error;
$i++;
my $tid_msg = '';
diff --git a/dist/Carp/t/rt133776-no-warn-on-undef-args.t b/dist/Carp/t/rt133776-no-warn-on-undef-args.t
new file mode 100644
index 0000000000..6a3da81309
--- /dev/null
+++ b/dist/Carp/t/rt133776-no-warn-on-undef-args.t
@@ -0,0 +1,49 @@
+use strict;
+use warnings;
+use Carp;
+use Test::More tests => 4;
+use Data::Dumper; $Data::Dumper::Indent=1;
+our $Level;
+
+sub __capture {
+ push @::__capture, join "", @_;
+}
+
+sub capture_warnings {
+ my $code = shift;
+
+ local @::__capture;
+ local $SIG {__WARN__} = \&__capture;
+ local $Level = 1;
+ &$code;
+ return @::__capture;
+}
+
+{
+ my @warnings = capture_warnings( sub { eval { Carp::carp("foo", undef, "bar"); }; 1; } );
+ my $str = join("\n" => @warnings);
+ unlike($str, qr/Use of uninitialized value \$error\[1\] in join or string/s,
+ "No uninitialized value warning for 'undef' arg to carp");
+}
+
+{
+ my @warnings = capture_warnings( sub { eval { Carp::croak("foo", undef, "bar"); }; 1; } );
+ my $str = join("\n" => @warnings);
+ unlike($str, qr/Use of uninitialized value \$error\[1\] in join or string/s,
+ "No uninitialized value warning for 'undef' arg to croak");
+}
+
+{
+ my @warnings = capture_warnings( sub { eval { Carp::cluck("foo", undef, "bar"); }; 1; } );
+ my $str = join("\n" => @warnings);
+ unlike($str, qr/Use of uninitialized value \$error\[1\] in join or string/s,
+ "No uninitialized value warning for 'undef' arg to cluck");
+}
+
+{
+ my @warnings = capture_warnings( sub { eval { Carp::confess("foo", undef, "bar"); }; 1; } );
+ my $str = join("\n" => @warnings);
+ unlike($str, qr/Use of uninitialized value \$error\[1\] in join or string/s,
+ "No uninitialized value warning for 'undef' arg to confess");
+}
+
--
2.17.1
|
From wagnerc@plebeian.comAlthough, if we do rewarn about the undef if might be better to do it as a warn(shortmess(@_)) in case someone replaced carp() in such a way as to prevent the main backtrace from completing. e.g. In this case the user informative croak message would never print because the carp() about the undef was changed to a die. Sure, it's a stretch... The question is of course how far to go with preempting edge cases. Chris |
From wagnerc@plebeian.comI should have put the expression form of grep in the warnings check since that is faster than the block form. scalar grep !defined, @error Chris |
From @jkeenanOn Mon, 21 Jan 2019 04:08:12 GMT, wagnerc@plebeian.com wrote:
Chris, before we go down that rabbit hole, can you (and anyone else) please review the smoke-me/jkeenan/133776-carp-uninit-value-warning branch as of commit f7528e1? The changes so far are causing warnings to be thrown during the running of dist/Carp/t/stash_deletion.t that I don't see in blead. ##### ok 1 Thank you very much. |
From @tonycozOn Mon, Jan 21, 2019 at 12:43:34PM -0800, James E Keenan via RT wrote:
You test that the warnings that started the ticket are no longer There might be some problems with warnings::enabled() and carp() Tony |
From @jkeenanOn Mon, 21 Jan 2019 20:43:34 GMT, jkeenan wrote:
The warnings are all coming from this block in dist/Carp/t/stash_deletion.t: ##### -- |
From @jkeenanOn Mon, 21 Jan 2019 22:44:41 GMT, tonyc wrote:
Acknowledged, but no one else had suggested any tests for the changes in code and it took me a long time to even write those tests. The problem with new warnings in dist/Carp/t/stash_deletion.t makes me wonder whether the effort we're going through is worth it.
-- |
From wagnerc@plebeian.comSo I've been analyzing these spurious warnings. I've narrowed it down to this one line in stash_deletion.t. 40: delete ${'::'}{'Confess::'}; Commenting it out eliminates the warnings. Also, removing the warnings::enabled("uninitialized") call also eliminates the warnings. It's strange because none of the other package deletes trigger the warnings. I also tried a different package name besides Confess::. No difference. Bascially warnings::enabled() causes a call to short_error_loc() in order to know where to grab the caller's warning vector. In the test file, that happens after the test has deleted the package and called confess(). So in the course of one exception sequence, there are competing calls to long_error_loc() by longmess_heavy and short_error_loc() by warnings::enabled(). My best guess now is that caller() itself is confused by this and $called is left undefined in short_error_loc; There are no defined checks on $called like there is on $caller. It's quite possible that this use case has exposed a new bug in Carp itself. Totally narrowing it down would probably take some debugger tracing or other instrumentation. But I'll see if I can come up with some code to deal with $called being undefined. Thanks, |
From wagnerc@plebeian.comI created RT#133800 (Carp croak() fails on anon sub and package deletion) to cover the problems in short_error_loc(). I think that needs to be fixed before we can fix the uninitialized warnings. Thanks. |
RT#133800 now appears as #16827 |
Migrated from rt.perl.org#133776 (status was 'open')
Searchable as RT133776$
The text was updated successfully, but these errors were encountered: