Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Perl-Critic CPAN distribution.

Report information
The Basics
Id: 45051
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: brianski [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: 1.098
Fixed in: (no value)



Subject: "return value of eval not tested" doesn't give PBP reference
Download (untitled) / with headers
text/plain 377b
Some code I have generates warnings of this form: # Return value of eval not tested at line 6, column 1. You can't depend upon the value of $@/$EVAL_ERROR to tell whether an eval failed. (Severity: 3) It took me awhile to hunt down the relevant section of PBP. I'd say there should be a pointer to PBP page 297, as in most other diagnostics. Cheers, Brian Szymanski
Subject: Re: [rt.cpan.org #45051] "return value of eval not tested" doesn't give PBP reference
Date: Wed, 15 Apr 2009 21:52:34 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Download (untitled) / with headers
text/plain 542b
Brian Szymanski via RT wrote: Show quoted text
> Some code I have generates warnings of this form: > # Return value of eval not tested at line 6, column 1. You can't > depend upon the value of $@/$EVAL_ERROR to tell whether an eval > failed. (Severity: 3) > > It took me awhile to hunt down the relevant section of PBP. I'd say > there should be a pointer to PBP page 297, as in most other diagnostics.
I find nothing on this page relevant to the policy. This isn't a PBP policy. It was inspired by the discussion linked to in the documentation.
Download (untitled) / with headers
text/plain 1.4k
As I recall (I don't have the book in front of me right now), page 297 pointed out that you can have nested evals hidden in other functions, etc. which overwrite $@/$EVAL_ERROR, which is global. Which seems pretty similar if not identical to the point raised in the discussion on http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2008-06/msg00537.html Incidentally, the diagnostic is a bit misleading in my case, as I never check $@ nor the return code - in this particular case I need no error handling. Would it be possible to omit the warning if $@ is never used after the eval? ski@ganiodayo:~$ perlcritic --harsh - package foo; use strict; use warnings; eval {}; #dummy code can be inserted in that eval if you want 1; Return value of eval not tested at line 4, column 1. You can't depend upon the value of $@/$EVAL_ERROR to tell whether an eval failed. (Severity: 3) On Wed Apr 15 22:52:58 2009, clonezone wrote: Show quoted text
> Brian Szymanski via RT wrote:
> > Some code I have generates warnings of this form: > > # Return value of eval not tested at line 6, column 1. You can't > > depend upon the value of $@/$EVAL_ERROR to tell whether an eval > > failed. (Severity: 3) > > > > It took me awhile to hunt down the relevant section of PBP. I'd say > > there should be a pointer to PBP page 297, as in most other
> diagnostics. > > I find nothing on this page relevant to the policy. This isn't a PBP > policy. It was inspired by the discussion linked to in the > documentation.
Download (untitled) / with headers
text/plain 898b
On Thu Apr 16 10:01:17 2009, BRIANSKI wrote: Show quoted text
> As I recall (I don't have the book in front of me right now), page 297 > pointed out that you can have nested evals hidden in other functions, > etc. which overwrite $@/$EVAL_ERROR, which is global. Which seems pretty > similar if not identical to the point raised in the discussion on >
http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2008-06/msg00537.html Ok, got the book in front of me now. The relevant (penultimate) paragraph on page 297 reads: Refactoring in this way is usually a highly recommended practice, but in this case it has the potential to introduce a subtle bug. The problem is that the $EVAL_ERROR exception variable (a.k.a. $@) is a global variable. So if fail_if_incorrigible() happens to throw -- and then internally catch -- some other exception during its execution, that nested exception will overwrite $EVAL_ERROR.
Subject: Re: [rt.cpan.org #45051] "return value of eval not tested" doesn't give PBP reference
Date: Thu, 16 Apr 2009 20:12:18 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Download (untitled) / with headers
text/plain 1.6k
Brian Szymanski via RT wrote: Show quoted text
> Ok, got the book in front of me now. The relevant (penultimate) > paragraph on page 297 reads: > Refactoring in this way is usually a highly recommended practice, but in > this case it has the potential to introduce a subtle bug. The problem is > that the $EVAL_ERROR exception variable (a.k.a. $@) is a global > variable. So if fail_if_incorrigible() happens to throw -- and then > internally catch -- some other exception during its execution, that > nested exception will overwrite $EVAL_ERROR.
Looking at the example given directly above, I can see the possibility of making an exception for an eval block that ends with a return statement. However, let us pretend that the contents of the eval block do not contain a return statement. Then, the test of the Exception::Class object is not valid due the the reasons that this policy was created; whatever fail_if_incorrigible() does is not relevant to the discussion here. What he discusses here is losing the value of $EVAL_ERROR /after/ the eval has finished. What this policy is concerned about is what happens to the value of $EVAL_ERROR /during/ the execution of the eval. The else part of if statement in his example is not correct, because it assumes that $EVAL_ERROR contains an object. Given the type of thing that this policy is looking for, the value of $EVAL_ERROR might be undef or a string. The documentation could include a pointer to this part of the book, but it's not what this policy is looking for. Really what needs to happen is for there to be a new policy that requires you to save the value of $EVAL_ERROR prior to using it, which would be a reference to this part of the book.
Subject: Re: [rt.cpan.org #45051] "return value of eval not tested" doesn't give PBP reference
Date: Thu, 16 Apr 2009 19:53:06 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Download (untitled) / with headers
text/plain 831b
Brian Szymanski via RT wrote: Show quoted text
> Incidentally, the diagnostic is a bit misleading in my case, as I never > check $@ nor the return code - in this particular case I need no error > handling. Would it be possible to omit the warning if $@ is never used > after the eval? > > ski@ganiodayo:~$ perlcritic --harsh - > package foo; > use strict; > use warnings; > eval {}; #dummy code can be inserted in that eval if you want > 1; > Return value of eval not tested at line 4, column 1. You can't depend > upon the value of $@/$EVAL_ERROR to tell whether an eval failed. > (Severity: 3)
I think this is perfectly clear: you're not testing the eval. This policy doesn't look at whether you're looking at $@/$EVAL_ERROR. It only cares that you're doing something with the eval itself. Thus, in your example, it is working correctly.
On Thu Apr 16 21:31:09 2009, clonezone wrote: Show quoted text
> I think this is perfectly clear: you're not testing the eval. This > policy doesn't look at whether you're looking at $@/$EVAL_ERROR. > It only cares that you're doing something with the eval itself. > Thus, in your example, it is working correctly.
What's confusing is that you get the exact same error message whether you check $@ or not, and the error message assumes you ARE checking $@. Whereas not checking $@ (ie, no effort is made to handle any exceptions thrown) is a different case altogether and should be worded as such. If, as I suspect, it's not practical to see whether $@ is used later or not, perhaps instead of: "Return value of eval not tested... You can't depend upon the value of $@/$EVAL_ERROR to tell whether an eval failed." (which assumes that you ARE checking $@/$EVAL_ERROR), you could try to word the error more like: "Return value of eval not tested... If you are checking $@/$EVAL_ERROR, you should check the return value of the eval instead."
Download (untitled) / with headers
text/plain 249b
On Thu Apr 16 21:12:49 2009, clonezone wrote: Show quoted text
> Really what needs to happen is for there to be a new policy that > requires you to save the value of $EVAL_ERROR prior to using it, > which would be a reference to this part of the book.
Agreed.
Subject: Re: [rt.cpan.org #45051] "return value of eval not tested" doesn't give PBP reference
Date: Thu, 16 Apr 2009 22:26:21 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Download (untitled) / with headers
text/plain 513b
Brian Szymanski via RT wrote: Show quoted text
> you could try to word the error more like: > "Return value of eval not tested... If you are checking > $@/$EVAL_ERROR, you should check the return value of the eval instead."
Hrrmm... this doesn't exactly fit the way that the rest of the policies' explanations work. The explanation is intended to describe why the policy exists, with a single sentence, not what you're supposed to do about it. I don't know. Also, it shouldn't be "instead", it would be "also" or "as well".


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

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.