[Bug 7120] New: "return" statement with explicit "undef" - Perl::Critic : Subroutines::ProhibitExplicitReturnUndef

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7120

            Bug ID: 7120
           Summary: "return" statement with explicit "undef" -
                    Perl::Critic :
                    Subroutines::ProhibitExplicitReturnUndef
           Product: Spamassassin
           Version: SVN Trunk (Latest Devel Version)
          Hardware: All
                OS: All
            Status: NEW
          Severity: minor
          Priority: P2
         Component: Libraries
          Assignee: dev@spamassassin.apache.org
          Reporter: Mark.Martinec@ijs.si

The Perl::Critic (as run by xt/60_perlcritic.t) may issue the following
warning:

# Perl::Critic found these violations in "...":
# "return" statement with explicit "undef" at line xxx, column x.
#
#   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.
#   [... example omitted ...]
#     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).

... which deserves careful consideration before deciding to follow
that advise.  The problematic part is "Now if that list is subsequently
put in a boolean context to test for failure". If this assumption is
not true (i.e. not in a boolean context), then the advise MUST NOT BE
blindly followed.

Consider the following function to compute a square root of its argument
(which we want to return undef for negative or zero values of its argument):

  sub sq {
    my($x) = @_;
    return undef if $x <= 0;
    sqrt($x);
  }
  my($a, $b, $c) = (sq(4), sq(-1), sq(16));
  print "a=$a, b=$b, c=$c\n";

which works fine:

  a=2, b=, c=4

but Perl::Critic complains:

  "return" statement with explicit "undef" at line 7, column 3.
   See page 199 of PBP.  (Severity: 5)


Obeying that ill-advice yields:

  sub sq {
    my($x) = @_;
    return if $x <= 0;
    sqrt($x);
  }
  my($a, $b, $c) = (sq(4), sq(-1), sq(16));
  print "a=$a, b=$b, c=$c\n";

yields misplaced results:

  a=2, b=4, c=


The same happens when collecting arguments to some subroutine call.
The context is a list context, and the list will *not* be interpreted
as a boolean:

  foo("bla", sq(0), 999)

The subroutine foo will receive two(!) arguments, the "bla" as its
first argument, and 999 as its second arguments. This is clearly
wrong, one would expect that the second argument receives undef
and 999 ends up in the third argument.


I have been bitten more than once by following that Perl::Critic
advise without considering in what context a function will be called
and what is it expected to return.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7120] "return" statement with explicit "undef" - Perl::Critic : Subroutines::ProhibitExplicitReturnUndef

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7120

Kevin A. McGrail <km...@pccc.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |kmcgrail@pccc.com
         Resolution|---                         |FIXED

--- Comment #1 from Kevin A. McGrail <km...@pccc.com> ---
Resolved Util.pm return undef by vetting issue with Mark and deciding to flag
the code to ignore the warning.

## no critic (ProhibitExplicitReturnUndef)  - See Bug 7120  added and passes xt
test.

svn commit -m 'Perl Critic exemption per bug 7120' lib/
Sending        lib/Mail/SpamAssassin/Util.pm
Transmitting file data .
Committed revision 1652429.

-- 
You are receiving this mail because:
You are the assignee for the bug.