mje has asked for the wisdom of the Perl Monks concerning the following question:

perlcritic complains about the following code:

use strict;
use warnings;
use Data::Dumper;
sub one {
    return undef;
}
sub two {
    my @a = @_;
    print Dumper(\@a);
    return;
}
two(1,2,one(), 3);

[download]

The complaint is "return" statement with explicit "undef" at line 5, near 'return undef;'. However, if you follow perlcritic advise and remove the "undef" to make the return just "return;", the program fails to work as can been seen from the following output, the first with the undef and the second without

$VAR1 = [1,2,undef,3];
$VAR1 = [1,2,3];

[download]

Is the perlcritic advice questionable? Is this worth reporting for Perl::Critic?

The full perlcritic description of the problem is:

"return" statement with explicit "undef" at line 5, near 'return undef;'. Subroutines::ProhibitExplicitReturnUndef (Severity: 5) Returning `undef' upon failure from a subroutine is pretty common. But if the subroutine is called in list context, an explicit `return undef;' statement will return a one-element list containing `(undef)'. Now if that list is subsequently put in a boolean context to test for failure, then it evaluates to true. But you probably wanted it to be false.

      sub read_file {
          my $file = shift;
          -f $file || return undef;  #file doesn't exist!

          #Continue reading file...
      }

      #and later...

      if ( my @data = read_file($filename) ){

          # if $filename doesn't exist,
          # @data will be (undef),
          # but I'll still be in here!

          process(@data);
      }
      else{

          # This is my error handling code.
          # I probably want to be in here
          # if $filname doesn't exist.

          die "$filename not found";
      }

[download]

The solution is to just use a bare `return' statement whenever you want to return failure. In list context, Perl will then give you an empty list (which is false), and `undef' in scalar context (which is also false).

      sub read_file {
          my $file = shift;
          -f $file || return;  #DWIM!

          #Continue reading file...
      }

[download]

Replies are listed 'Best First'.
Re: Question perlcritic ProhibitExplicitReturnUndef
by JavaFan (Canon) on Feb 06, 2009 at 10:30 UTC
    PerlCritic is like a lint program. It detects constructs that may be buggy (it also detects many stylistic issues that have nothing to do with right or wrong). In that sense, it's just like use warnings. Use warnings will also flag things that may be wrong. Most of the time, it is. Sometimes, it isn't. You have a case where it isn't. PerlCritic has a way to silence a warning. In your case, you should use that.
[reply]
Re: Question perlcritic ProhibitExplicitReturnUndef
by moritz (Cardinal) on Feb 06, 2009 at 10:29 UTC
    Perlcritic tests for common errors; the cases where a return undef; would better be written as a bare return; are probably more common than the other way round, so I wouldn't call it bogus.

    You have to decide for yourself if you call it "questionable", but what's wrong with just ignoring the warnings, or turning them off?

[reply]
[d/l]
[select]

      There is no problem with turning this warning off - that is easily achieved. I was less complaining about the warning and more the explanation. I was slightly concerned that the critic explanatory text makes it sound like if you use "return undef" you should stop and replace it with a straight return without pointing out that you could easily break your code if you use the function in list context.

[reply]
Re: Question perlcritic ProhibitExplicitReturnUndef
by dsheroh (Monsignor) on Feb 06, 2009 at 15:59 UTC
    Perl::Critic, by default, attempts to enforce the content of Perl Best Practices. PBP is intended as a set of guidelines and suggestions to make you think about how you write your code and whether it can be improved upon, not as a set of ironclad commandments.

    Therefore, Perl::Critic's warnings should also not be taken as strict rules that you must comply with.

    Perl::Critic gives you an explanation of why it objects to return undef;. Use that explanation to determine whether it is appropriate for you to return undef; in this case or not.

    Also note that the Perl::Critic explanation says, "The solution is to just use a bare `return' statement whenever you want to return failure." If your intent is to return an undefined value rather than to indicate failure, then there's no problem with your return undef;, is there? (In that case, I would advise adding a comment stating that this is the intent so that a future maintainer who subscribes to PBP doesn't break your code in an attempt to "fix" it. Mentioning in the sub's documentation that a return value of undef is not intended to indicate failure would also be wise.)

[reply]
[d/l]
[select]

      I know what perlcritic is and that it is not a set of ironclad commandments or a set of strict rules that you must comply with.

      My point was more to do with the perlcritic description of the problem than how to work around it, or document the way the real sub I was using did. I had felt it was too strongly pointing you in one direction that may not be right for you but on reading it again noting your bolded emphasis I suppose it is targeted to using undef for error reporting which my example was not doing.

[reply]
Re: Question perlcritic ProhibitExplicitReturnUndef
by Herkum (Parson) on Feb 06, 2009 at 14:58 UTC

    A blank return is not intended to return anything. It is doing what is is supposed to do which is error checking.

    The explanation of a blank return vs. return undef is that if it is used as an error checking mechanism, the undef screws things up when used in a list context.

    The coding example you used is not in an error checking context, you wanted to return a value. Why you want to return an undef to set a value, I don't know, but you could always return an empty string which would make more sense anyways.

    You can approach undef in this way, if I have a scalar value which is undef it means 'noone has attempted to assign a value to this variable.' If you have an empty string, 'someone has assigned a value which is nothing.'

[reply]
Re: Question perlcritic ProhibitExplicitReturnUndef
by Anonymous Monk on Feb 06, 2009 at 11:16 UTC
    It is also a common oversight to forget scalar context :)
    two(1,2,one(), 3);
two(1,2,scalar one(), 3);

    [download]
[reply]
[d/l]

Back to Seekers of Perl Wisdom