Skip Menu |
Report information
Id: 122966
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: mauke- <l.mai [at] web.de>
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type: unknown
Perl Version: (no value)
Fixed In: (no value)



Subject: [PATCH] bogus fatal warnings can hide syntax errors
Download (untitled) / with headers
text/plain 2.1k
The Problem ----------- Consider this code: #!perl use strict; use warnings; if (1 { my $wtf = 42; print "$wtf\n"; } __END__ If you run that, you get: "my" variable $wtf masks earlier declaration in same statement at foo.pl line 7. syntax error at foo.pl line 5, near "1 {" syntax error at foo.pl line 8, near "}" Execution of foo.pl aborted due to compilation errors. I.e. there's a bogus warning on line 7, then the actual error on line 5. Now the same code with fatal warnings: #!perl use strict; use warnings FATAL => 'all'; if (1 { my $wtf = 42; print "$wtf\n"; } __END__ This time the output is just: "my" variable $wtf masks earlier declaration in same statement at bar.pl line 7. The actual error is completely hidden because the bogus warning throws an exception and aborts everything. My Solution ----------- I've attached a patch that solves this problem by simply defatalizing warnings if we're currently parsing and there are pending parse errors. I think this is a good way to handle this situation because we don't know whether the warning we're about to emit is useful or caused by misinterpretation of the code after a syntax error. So we warn non-fatally because the pending parse errors will throw an exception later on anyway. Thoughts? (Patch copied inline here because I don't know whether attaching or inline code is the preferred method.) From f3aa466b4080470e3a14071cb476674e9bfdf796 Mon Sep 17 00:00:00 2001 From: Lukas Mai <l.mai@web.de> Date: Sun, 12 Oct 2014 19:01:09 +0200 Subject: [PATCH] don't fatalize warnings after syntax errors --- util.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/util.c b/util.c index ae3b833..0689d7c 100644 --- a/util.c +++ b/util.c @@ -1911,7 +1911,8 @@ Perl_vwarner(pTHX_ U32 err, const char* pat, va_list* args) { dVAR; PERL_ARGS_ASSERT_VWARNER; - if (PL_warnhook == PERL_WARNHOOK_FATAL || ckDEAD(err)) { + if ((PL_warnhook == PERL_WARNHOOK_FATAL || ckDEAD(err)) + && !(PL_parser && PL_parser->error_count)) { SV * const msv = vmess(pat, args); invoke_exception_hook(msv, FALSE); -- 2.1.2
Subject: 0001-don-t-fatalize-warnings-after-syntax-errors.patch
From f3aa466b4080470e3a14071cb476674e9bfdf796 Mon Sep 17 00:00:00 2001 From: Lukas Mai <l.mai@web.de> Date: Sun, 12 Oct 2014 19:01:09 +0200 Subject: [PATCH] don't fatalize warnings after syntax errors --- util.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/util.c b/util.c index ae3b833..0689d7c 100644 --- a/util.c +++ b/util.c @@ -1911,7 +1911,8 @@ Perl_vwarner(pTHX_ U32 err, const char* pat, va_list* args) { dVAR; PERL_ARGS_ASSERT_VWARNER; - if (PL_warnhook == PERL_WARNHOOK_FATAL || ckDEAD(err)) { + if ((PL_warnhook == PERL_WARNHOOK_FATAL || ckDEAD(err)) + && !(PL_parser && PL_parser->error_count)) { SV * const msv = vmess(pat, args); invoke_exception_hook(msv, FALSE); -- 2.1.2
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.5k
On Mon Oct 13 01:35:03 2014, mauke- wrote: Show quoted text
> The Problem > ----------- > > Consider this code: > > #!perl > use strict; > use warnings; > > if (1 { > my $wtf = 42; > print "$wtf\n"; > } > __END__ > > If you run that, you get: > > "my" variable $wtf masks earlier declaration in same statement at > foo.pl line 7. > syntax error at foo.pl line 5, near "1 {" > syntax error at foo.pl line 8, near "}" > Execution of foo.pl aborted due to compilation errors. > > I.e. there's a bogus warning on line 7, then the actual error on line > 5. > > Now the same code with fatal warnings: > > #!perl > use strict; > use warnings FATAL => 'all'; > > if (1 { > my $wtf = 42; > print "$wtf\n"; > } > __END__ > > This time the output is just: > > "my" variable $wtf masks earlier declaration in same statement at > bar.pl line 7. > > The actual error is completely hidden because the bogus warning throws > an exception and aborts everything. > > > My Solution > ----------- > > I've attached a patch that solves this problem by simply defatalizing > warnings if we're currently parsing and there are pending parse > errors. I think this is a good way to handle this situation because we > don't know whether the warning we're about to emit is useful or caused > by misinterpretation of the code after a syntax error. So we warn non- > fatally because the pending parse errors will throw an exception later > on anyway. > > Thoughts?
Since the programmer has requested fatal warnings, why not send it to qerror instead of simply turning off the fatality? -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1000b
On Mon Oct 13 06:12:04 2014, sprout wrote: Show quoted text
> > Since the programmer has requested fatal warnings, why not send it to > qerror instead of simply turning off the fatality?
Because I wasn't aware qerror() exists. :-) This works: if (PL_warnhook == PERL_WARNHOOK_FATAL || ckDEAD(err)) { SV * const msv = vmess(pat, args); if (PL_parser && PL_parser->error_count) { qerror(msv); } else { invoke_exception_hook(msv, FALSE); die_unwind(msv); } } else { Perl_vwarn(aTHX_ pat, args); } but it changes the order of the output to: syntax error at foo.pl line 5, near "1 {" "my" variable $wtf masks earlier declaration in same statement at foo.pl line 7. syntax error at foo.pl line 8, near "}" Execution of foo.pl aborted due to compilation errors. Personally I think this is an improvement (messages are ordered by line number) but it's different from non-fatal warnings now. (Revised patch attached.)
Subject: 0001-treat-fatal-warnings-after-syntax-errors-as-syntax-e.patch
From 23ec748a59335698a67aed4968486f22541a9854 Mon Sep 17 00:00:00 2001 From: Lukas Mai <l.mai@web.de> Date: Sun, 12 Oct 2014 19:01:09 +0200 Subject: [PATCH] treat fatal warnings after syntax errors as syntax errors --- util.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/util.c b/util.c index ae3b833..add8f1d 100644 --- a/util.c +++ b/util.c @@ -1914,8 +1914,13 @@ Perl_vwarner(pTHX_ U32 err, const char* pat, va_list* args) if (PL_warnhook == PERL_WARNHOOK_FATAL || ckDEAD(err)) { SV * const msv = vmess(pat, args); - invoke_exception_hook(msv, FALSE); - die_unwind(msv); + if (PL_parser && PL_parser->error_count) { + qerror(msv); + } + else { + invoke_exception_hook(msv, FALSE); + die_unwind(msv); + } } else { Perl_vwarn(aTHX_ pat, args); -- 2.1.2
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.3k
On Mon Oct 13 13:26:45 2014, mauke- wrote: Show quoted text
> On Mon Oct 13 06:12:04 2014, sprout wrote:
> > > > Since the programmer has requested fatal warnings, why not send it to > > qerror instead of simply turning off the fatality?
> > Because I wasn't aware qerror() exists. :-) > > This works: > > if (PL_warnhook == PERL_WARNHOOK_FATAL || ckDEAD(err)) { > SV * const msv = vmess(pat, args); > > if (PL_parser && PL_parser->error_count) { > qerror(msv); > } > else { > invoke_exception_hook(msv, FALSE); > die_unwind(msv); > } > } > else { > Perl_vwarn(aTHX_ pat, args); > } > > but it changes the order of the output to: > > syntax error at foo.pl line 5, near "1 {" > "my" variable $wtf masks earlier declaration in same statement at > foo.pl line 7. > syntax error at foo.pl line 8, near "}" > Execution of foo.pl aborted due to compilation errors. > > Personally I think this is an improvement (messages are ordered by > line number) but it's different from non-fatal warnings now.
I’m not too worried about the difference. I think your patch is good. Could you supply tests as well? (We could use qerror for non-fatal warnings after syntax errors as long as there is no $SIG{__WARN__} or $SIG{__DIE__} registered and we are not inside an eval. That way, things will come out in order most of the time, but code that catches warnings or errors will see no difference.) -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 323b
On Mon Oct 13 14:16:03 2014, sprout wrote: Show quoted text
> > I’m not too worried about the difference. I think your patch is good. > Could you supply tests as well?
I'm not sure what the best way to test this is, but I've attached a revised patch that adds another test case to 7fatal. It's based on the example code in this ticket.
Subject: 0001-treat-fatal-warnings-after-syntax-errors-as-syntax-e.patch
From e974871a15eb4ab46ceb0f1faa3d68d09bf97117 Mon Sep 17 00:00:00 2001 From: Lukas Mai <l.mai@web.de> Date: Sun, 12 Oct 2014 19:01:09 +0200 Subject: [PATCH] treat fatal warnings after syntax errors as syntax errors --- t/lib/warnings/7fatal | 13 +++++++++++++ util.c | 9 +++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/t/lib/warnings/7fatal b/t/lib/warnings/7fatal index aab7fd1..87f3fd0 100644 --- a/t/lib/warnings/7fatal +++ b/t/lib/warnings/7fatal @@ -535,3 +535,16 @@ print STDERR "The End.\n" ; EXPECT Reversed += operator at - line 10. The End. +######## + +# fatal warnings shouldn't hide parse errors [perl #122966] +use warnings FATAL => 'all'; +if (1 { + my $x = "hello"; + print $x, "\n"; +} +EXPECT +syntax error at - line 4, near "1 {" +"my" variable $x masks earlier declaration in same statement at - line 6. +syntax error at - line 7, near "}" +Execution of - aborted due to compilation errors. diff --git a/util.c b/util.c index ae3b833..add8f1d 100644 --- a/util.c +++ b/util.c @@ -1914,8 +1914,13 @@ Perl_vwarner(pTHX_ U32 err, const char* pat, va_list* args) if (PL_warnhook == PERL_WARNHOOK_FATAL || ckDEAD(err)) { SV * const msv = vmess(pat, args); - invoke_exception_hook(msv, FALSE); - die_unwind(msv); + if (PL_parser && PL_parser->error_count) { + qerror(msv); + } + else { + invoke_exception_hook(msv, FALSE); + die_unwind(msv); + } } else { Perl_vwarn(aTHX_ pat, args); -- 2.1.2
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 454b
On Tue Oct 14 14:51:01 2014, mauke- wrote: Show quoted text
> On Mon Oct 13 14:16:03 2014, sprout wrote:
> > > > I’m not too worried about the difference. I think your patch is > > good. > > Could you supply tests as well?
> > I'm not sure what the best way to test this is, but I've attached a > revised patch that adds another test case to 7fatal. It's based on the > example code in this ticket.
Thanks, applied as 594b6face91a40f19765cb90d794c3b9a877497a. Tony


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org