Avoiding stringy eval in Perl

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 value¹. 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:²

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::Criticdocumentation 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.³

The simple fix

The fix is very simple:⁴

  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 the eval and the if.

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::Loadmodule. 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.⁵ 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:

• Test::Requires
• Test2::Require::Module

Hope these are useful additions for readers wishing to avoid stringy evals! Happy hacking :)

1. If the eval was successful $@ contains the empty string which evaluates as false. ↩

2. It turns out that avoiding postfix controls is also something one should do. ↩

3. Finding silly bugs at runtime can be rather frustrating. ↩

4. It’s unfortunate that some modules still recommend the outdated eval form for module-guard code. Oh well. ↩

5. Things are nuanced. Who would have thought? 😉 ↩