Perl::Critic "Don't use this method"-type rule

Question

We have been using Perl::Critic here at work to enforce our code conventions. Recently we ran into issues with /tmp directory getting filled up due to the Temp::File::tempdir function. tempdir cleans up when the Perl process terminates, but since our entire backend is a Perl process, this only occurs when the server itself gets restarted (not very often). We want to encourage developers to use the newdir object method in the future, which cleans up after itself as soon as the object goes out of scope.

Basically, we're trying to mark Temp::File::tempdir as a code convention violation, but I can't seem to find any rule that would be similar on CPAN. I understand that this is hard to enforce in a dynamically-typed language without introducing false positives, but I would expect someone has ran into similar issue in the past with another deprecated function. We're not expecting to catch all the tricky cases either, only the most obvious uses of Temp::File::tempdir. The idea is to discourage accidental use of tempdir when newdir could do the job, not to catch all attempts to fool the critic (developer could always just use ## no critic). It would probably be enough to complain when tempdir is used if use Temp::File is defined (preferably checking that nothing else redefines tempdir) and when Temp::File::tempdir is used.

Is there already something similar, or should I start from scratch? Thanks

Curious. There are configurable `ProhibitEvilModules` and `ProhibitEvilVariables` policies, but no `ProhibitEvilMethods` ? — mob, Dec 03 '12 at 21:25

Borodin · Accepted Answer · 2012-12-05T01:32:34.040

There isn't anything in Perl::Critic at present to provide what you need, but it's quite possible add a policy to do something like it. Unfortunately PPI isn't comprehensive enought to identify properly what each token in the program is doing, so it takes more coding than it might.

This program checks for a use File::Temp statement that tries to import tempdir using any of

use File::Temp 'tempdir';
use File::Temp q(tempdir);
use File::Temp "tempdir";
use File::Temp qq(tempdir);
use File::Temp qw/ tempdir /;

(with any delimiter for the q, qq, and qw forms). It also checks for a PPI::Token::Word node that looks like a function call and is equal to File::Temp::tempdir.

package Perl::Critic::Policy::Prohibit_tempdir;

use strict;
use warnings;

use Perl::Critic::Utils qw{ is_function_call :severities };
use Scalar::Util 'blessed';

use base 'Perl::Critic::Policy';

my $DESC = 'Temp::File::tempdir function';
my $EXPL = 'The tempdir function from Temp::File is deprecated. Use newdir method instead';

sub default_severity { $SEVERITY_HIGH };

sub applies_to{ qw/ PPI::Statement::Include PPI::Token::Word / }

sub violates {

  my ($self, $elem) = @_;

  if ($elem->isa('PPI::Statement::Include')) {

    return unless $elem->type eq 'use';

    my $module = $elem->module;
    return unless $module and $module eq 'File::Temp';

    for my $kid ($elem->children) {

      next unless blessed($kid) =~ /^PPI::Token::Quote/;

      if ($kid->can('string') and $kid->string eq 'tempdir'
          or $kid->can('literal') and grep $_ eq 'tempdir', $kid->literal) {
        return $self->violation($DESC, $EXPL, $elem);
      }
    }
  }
  else {
    if (is_function_call($elem) and $elem eq 'File::Temp::tempdir') {
      return $self->violation($DESC, $EXPL, $elem);
    }
  }

  return;
}

1;

with this code

use strict;
use warnings;

use File::Temp 'tempdir';
use File::Temp "tempdir";
use File::Temp qw/ tempdir /;

my $dir = tempdir();
$dir = tempdir;
$dir = File::Temp::tempdir;

my $ft = File::Temp->new;
$dir = $ft->newdir;

generates this output from perlcritic -4 test.pl

Code not contained in explicit package at line 1, column 1.  Violates encapsulation.  (Severity: 4)
Temp::File::tempdir function at line 4, column 1.  The tempdir function from Temp::File is deprecated. Use newdir method instead.  (Severity: 4)
Temp::File::tempdir function at line 5, column 1.  The tempdir function from Temp::File is deprecated. Use newdir method instead.  (Severity: 4)
Temp::File::tempdir function at line 6, column 1.  The tempdir function from Temp::File is deprecated. Use newdir method instead.  (Severity: 4)
Temp::File::tempdir function at line 10, column 8.  The tempdir function from Temp::File is deprecated. Use newdir method instead.  (Severity: 4)
Module does not end with "1;" at line 13, column 1.  Must end with a recognizable true value.  (Severity: 4)

Thanks, this does almost exactly what we need, it doesn't catch lines 8 and 9 yet, but I will tweak it to do so, and probably make it more generic so the methods can be specified per-module to make it a bit more generic. — Alexander Tsepkov, Dec 04 '12 at 23:16
I used the approach that you described in your question. Calls such as those in lines 8 and 9 will compile only if there is a preceding instance of lines 4, 5 or 6. i.e. the subroutine must be imported before it can be called. The problem with identifying calls to `tempdir` that aren't fully-qualified with the module name is that `PPI` isn't very helpful about the context of the bareword's appearance. The `is_function_call` classifier supplied by `Perl::Critic::Utils` is very vague, and simply means that the bareword doesn't appear to be any of the other nine possibilities that it checks for. — Borodin, Dec 05 '12 at 01:31

Perl::Critic "Don't use this method"-type rule

1 Answers1