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] bogus fatal warnings can hide syntax errors #14155

Closed
p5pRT opened this issue Oct 13, 2014 · 11 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Oct 13, 2014

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

Searchable as RT122966$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 13, 2014

From @mauke

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(-)

Inline Patch
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
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 13, 2014

From @mauke

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 13, 2014

From @cpansprout

On Mon Oct 13 01​:35​:03 2014, mauke- wrote​:

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 13, 2014

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 13, 2014

From @mauke

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.

(Revised patch attached.)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 13, 2014

From @mauke

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 13, 2014

From @cpansprout

On Mon Oct 13 13​:26​:45 2014, mauke- wrote​:

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 14, 2014

From @mauke

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.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 14, 2014

From @mauke

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 15, 2014

From @tonycoz

On Tue Oct 14 14​:51​:01 2014, mauke- wrote​:

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 594b6fa.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 15, 2014

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

@p5pRT p5pRT closed this Oct 15, 2014
@p5pRT p5pRT added the Severity Low label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.