Bug #45051 for Perl-Critic: "return value of eval not tested" doesn't give PBP reference

Brian Szymanski via RT wrote: Show quoted text
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.

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: Show quoted text
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.

On Thu Apr 16 21:31:09 2009, clonezone wrote: Show quoted text
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."

Brian Szymanski via RT wrote: Show quoted text
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".