Once upon a time, I used to take an active part in the CPAN Pull Request Challenge (and later in the Pull Request Club). One task I tackled as part of each monthly assignment was to try to remove as many Perl::Critic
violations as possible. One violation which tended to crop up often as part of module-loading guards in tests was stringy eval
. Generally, this is something to be avoided. But how? Here I discuss recommended best practices and some other options.
Stringy eval
is bad, mkay?
Unlike chunky bacon, stringy eval
is something one usually wants to avoid in Perl code. Itâs hopefully obvious that one shouldnât wantonly use eval
in oneâs code (mainly because it could potentially run any code and that would be bad). Even so, it does have its uses in the appropriate context.
Often one uses eval
to check if a module is available before continuing with further processing. For instance, if you want to run Pod checking tests but only if Test::Pod
has already been installed. In this case, youâd try to load Test::Pod
within an eval
and then skip the tests if the $EVAL_ERROR
variable $@
is a truthy value1. It turns out that one canât always rely on this behaviour either and that there is a more secure technique; more on this later.
Module-loading guard code tends to look like this:
use strict;
use warnings;
use Test::More;
# check for Test::Pod existence
eval "use Test::Pod";
plan skip_all => "Test::Pod required to test Pod" if $@;
# check all Pod files if Test::Pod is available
all_pod_files_ok();
where we try to use
a module with an eval
and if that fails, skip the remaining tests and print an appropriate warning message. Here we check the eval
status as part of a postfix if
statement. Of course, one can run the eval
status check earlier, in which case the code takes this shape:2
use strict;
use warnings;
use Test::More;
# check for Test::Pod existence
eval "use Test::Pod";
if ($@) {
plan skip_all => "Test::Pod required to test Pod";
}
# check all Pod files if Test::Pod is available
all_pod_files_ok();
Either way, both of these code snippets are discouraged because we pass a string to eval
. The ProhibitStringyEval
Perl::Critic
documentation explains why:
The Perl interpreter recompiles the string form of
eval
for each execution, whereas the block form is only compiled once. Also, the string form doesnât give compile-time warnings.
In other words, one should use the block form of the eval
statement. Not only is the block form more performant, but the compiler checks our code at compile time rather than at runtime. Thus, it finds any syntax errors and other silly bugs earlier.3
The simple fix
The fix is very simple:4
use strict;
use warnings;
use Test::More;
# check for Test::Pod existence
- eval "use Test::Pod";
+ eval { use Test::Pod };
if ($@) {
plan skip_all => "Test::Pod required to test Pod";
}
# check all Pod files if Test::Pod is available
all_pod_files_ok();
So thatâs all, right? Just change stringy eval
to use the block form and everythingâs fixed and you can carry on with your day? Not so fastâŠ
Itâs never as simple as you first thinkâŠ
The problem is that, in general, you canât depend upon the value of $@
/$EVAL_ERROR
to tell whether an eval
failed.
The reasoning behind the issue is a bit involved but the short explanation is that
[itâs possible for] the value of
$@
to change between the end of theeval
and theif
.
This is unfortunate because it is precisely this behaviour weâre relying upon for the module-loading guard code to work.
Help is at hand: use the try_load_class
function from the Class::Load
module. If you want to be absolutely sure that youâre testing that the module loads correctly before running tests, you can change the guard code to this:
use strict;
use warnings;
use Test::More;
use Class::Load qw( try_load_class );
# check for Test::Pod existence
try_load_class('Test::Pod') or plan skip_all => "Test::Pod required to test Pod";
# check all Pod files if Test::Pod is available
all_pod_files_ok();
which makes the code shorter and a bit more declarative. The downside is that your project needs to rely on another module. If the extra dependency isnât a problem for you, then using Class::Load
to safely check that your optional modules have loaded is a viable solution.
The nice thing with this solution is that try_load_class()
returns true if the class loaded and false otherwise. This is a positive formulation. By this, I mean that the function returns true if the thing that we wanted to happen succeeded. In contrast, with the $@
check, the formulation is negative: did the thing we want to happen fail? If yes, barf with an appropriate message. Both formulations do their job, but I find the âyup, everything worked, letâs continue doing stuffâ formulation nicer.
Sometimes you need to be eval
Of course, eval
isnât always bad.5 Sometimes, it can be exactly what you need. Consider, for instance, this code:
# $a, $b, $o, and $orig set earlier from other code
my $code = "\$str =~ s/$b/$a/$o";
try {
eval "$code";
}
catch {
die "Bad rule: $orig ($_)";
}
Here one constructs the code to be run from other information that is only available at runtime, thus requiring a stringy eval
. In other words, sometimes one needs to construct code dynamically and evaluate it. In that case, creating the code to run in a string and then using stringy eval
is perfectly acceptable. Of course, in such situations, you need to know what youâre doing. But if you know the risks and take relevant precautions, this is still valid code. The only issue here is that perlcritic
will complain, this time unnecessarily. How to get around this? Simply annotate the code with a no critic
exception:
# $a, $b, $o, and $orig set earlier from other code
my $code = "\$str =~ s/$b/$a/$o";
try {
+ ## no critic (ProhibitStringyEval)
eval "$code";
}
catch {
die "Bad rule: $orig ($_)";
}
where weâre being explicit about which Perl::Critic
warning weâre keeping quiet to signal to future readers of the code that we genuinely do want to do this.
Best practices are best
Best practices are just that: best. Theyâre not hard-and-fast rules. Yet theyâre called best practices for a reason: hard-won experience has shown them to be the safer bet in most situations.
In our case here: itâs usually best to replace stringy eval
with its equivalent block form. If you want to be a bit more careful and donât mind an extra dependency, use Class::Load
. But if you really, really need a stringy eval
, thatâs also ok, Perl will still let you. Make sure you know what youâre doing though.
Update
Since I published this post, FErki pinged me on Mastodon to let me know that there are core modules available which do a similar job
to that performed by Class::Load
. Many thanks for the heads up!
Here's what FErki had to say:
If one would like to use a core module instead of try_load_class()
from Class::Load
, they might be interested in can_load()
from Module::Load::Conditional
.
Particulary in tests, one may also be interested in either of:
Hope these are useful additions for readers wishing to avoid stringy evals! Happy hacking :)
If the
eval
was successful$@
contains the empty string which evaluates as false. â©It turns out that avoiding postfix controls is also something one should do. â©
Finding silly bugs at runtime can be rather frustrating. â©
Itâs unfortunate that some modules still recommend the outdated
eval
form for module-guard code. Oh well. â©Things are nuanced. Who would have thought? đ â©
Top comments (0)