(Posting a mail on behalf of David Godfrey who seems to be having trouble posting to the list)
Hi Nick,
k-man (irc) mentioned that the dancer team think some of these policies are bad.
mst (not a dancer dev per-se, but acrive in #dancer especially in respect to security and quality from what I've noted) suggested another list of recommended policies https://metacpan.org/pod/Perl::Critic::Freenode
mst also had this to say about Explicit returns.
ProhibitExplicitReturnUndef will mean people use just 'return;' which is context sensitive which will result in the same sort of security bugs seen from CGI.pm
So while I personally think the policy is a good one, it probably falls short in it's critique, and we probably need to ensure that any returns we have are correctly scoped to do what we want/expect
In short, mst has this to say about Perl::Critic Policies....
1) most of the builtin ones are crap and don't reflect how perl is written in the real world
2) the Freenode preset projibits actual mistakes we see regularly
Further to ProhibitExplicitReturnUndef
subroutines without an explicit return will explicitly return whatever their last _expression_ is
but
return;
is 'return wantarray ? () : undef;'
and generally, wantarray is a code smell
for example
sub lookup_row { my ($id) = @_; if (my @row = $dbh->selectrow_array(...) { my %row; @row{qw(id name foo bar)} = @row; return \@row } return undef }
yes, I know that's not quite what you'd really write but can we accept it as an example of a pattern?
now, consider
my %info = (row => lookup_row($id), id => $id);
if you replace 'return undef' with 'return', now in the case where the row doesn't exist, you get
my $info = (row => 'id', $id => undef);
i.e. you just handed control of a hash key to whatever code you got $id from, maybe a user
so now I sent your code ?id=is_admin
and SURPRISE bug/security hole
basically ProhibitExplicitUndef will make you add 'The Perl Jam' CGI.pm-style security holes *back in to your code*
this does not, to me, seem like a good idea.
so this is what bugs me about policies - people blindly 'fix' what it tells them to and can very easily introduce bugs in the process
Reading https://perlmaven.com/how-to-return-undef-from-a-function, it would appear it's a complicated issue that has no easy solutions. :-((
Once again mst has this to say about parts of that article....
the example where 'return undef' is wrong is ... list context
the whole point is 'return undef;' is correct for a *scalar* function
for a list context function, you use 'return;' or 'return ();' which both do the same thing, but I tend towards 'return ();' to be explicit when I remember
(I don't claim my code 100% follows these rules ;)
all that article points out against 'return undef' is that if you use a scalar return in a lst-returning function you will confuse everybody
and, well, yes, you will
but then my original complaint is that 'return;' turns a scalar returning fuynction into a maybe-scalar-maybe-list-returning function
which EW
baaasically, the world is a better place if you pretend that all perl routines are *either* scalar returning *or* list-returning, only ever use a given subroutine one way, and define your subroutines to be used one of the two ways
some of my greatest sadnesses over my career have involved maintaing code where I decided 'wantarray' was a good idea
and then for scalar-returning functions, 'return;' introduces an implicit wantarray which argh
in fact, many years ago somebody tested DBIC-using code for basically exactly the lookup_row bug while trying to exploit it
and DBIx::Class containing an explicit 'return undef' was why we passed the security audit
(I just went and found said undef just to check I remembered right ;)
Some other notes from mst (not exhaustive by any means) ....
* ProhibitBooleanGrep is a pessimisation
* ProhibitStringySplit is silly and unidiomatic
* ProhibitExplicitISA is probably fine because it's rare you actually want to do that
* RequireCarping is at best pointless and at worst screws up your error handling
* ProhibitInteractiveTest misses the point
* RequireCheckedClose # no. really. just no.
So 3 out of the first 10 on the list he'd avoid, which suggests to me we should be very careful about what we choose to include in our list.
mst is going to have a closer look at that policy list, and will get back to me over the next few days.
I'll report back in this thread.
Regards
David G