[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Perl::Critic Tests

Hi Guys,

This question tread lead me to ask about using Perl::Critic on Dancer2
code on #dancer on IRC. mst had some things to say about it and I
thought you might find it relevant.

Later David G joined the discussion:

11:09:13     k-man   do we run perl critic on the d2 code base?
11:09:28     k-man   also, would it be relevant to use this on d2?
11:09:37     @mst    k-man: do you have a set of perl critic settings
that make sense?
11:09:47     @mst    because the standard ones are useless for
production code
11:10:16     k-man   mst, none at all. I'm not an expert in this
11:10:45     @mst    a bunch of the poliicies recommended in that gist
are actively unsafe
11:11:22     k-man   there is a xt/perlcritic.rc file i see
11:13:46     k-man   mst, could you point me to one of the policies that
is unsafe in there and maybe point me to why its unsafe?
11:14:00     @mst
11:14:00             {yaURLs}~> http://tinyurl.com/mynmr79 (gist.github.com)
11:14:05     @mst    ProhibitExplicitReturnUndef
11:14:30     @mst    will mean people use just 'return;' which is
context sensitive which will result in the same sort of security bugs
seen from CGI.pm
11:14:53     k-man   ah i see
11:15:27     k-man   have you looked into that specific list before?
11:16:13     @mst    nope
11:16:26     @mst    I'm pretty good at spotting problems with perl
critic policies
11:16:30     @mst    due to them all being terrible ;)
11:16:48     k-man   mst, I was wondering all this because i was reading
the LedgerSMB mailing list, and they were discussing using that list for
perlcritic checking their code
11:17:07     @mst    that's a shame
11:17:16     @mst    the ledgersmb project used to come to me for perl
advuce quite regularly
11:17:27     k-man   oh
11:17:34     k-man   they don't any more?
11:17:49     @mst    not for a while, I think the person who mostly did
that may have moved on
11:17:57     k-man   i see
11:18:14     k-man   do you care to comment on their plan if I point you
to the post in question?
11:25:28     fuzzix  PBP and critic policies are great if you want to
get roasted for your Perl in ##c
11:38:04     k-man   so, perlcritic on lib/Dancer2.pm suggest our
$runner is a bad idea
11:39:37     @mst    based on which policy?
11:40:17     k-man   Variables::ProhibitPackageVars
11:40:38     @mst    well, that's bullshit
11:40:49     k-man   ok! glad I raised it then
11:40:51     @mst    it's a global variable. that's the correct way to
implement it.
11:41:14     k-man   it goes on to say it suggests using all caps for
global vars if they are needed
11:41:27     k-man   and then it won't complain about it
11:42:20     @mst    that's actually wrong
11:42:25     @mst    perlstyle says all caps are for constants
11:42:29     @mst    globals should be Foo_Bar
11:42:40     @mst    so, yeah, theoretically, it should be 'our $Runner'
11:42:49     @mst    but this is the sort of stupid shit that makes me
not want to use perlcritic
11:42:53     k-man   heh
11:44:40     k-man   the percritic advice comes from Conway,  is that a
style difference between Conway and perlstyle? (what is perlstyle)?
11:44:59     k-man   ok, ignore that last question, found it
12:17:05     k-man   mst, so is that list even worth looking at and
culling the bad rules?
12:17:17     @mst    maybe
12:17:17     k-man   mst, or is there a recommended list to use as a
starting point?
12:17:56     k-man   mst, they asked me if you'd review it ot suggest a
better list
12:18:14     @mst    the freenode policies are a start
12:29:57     -->     dcg (~..hidden..)
has joined #dancer
12:38:49     dcg     mst: hi
12:38:55     @mst    dcg: ay up
12:39:34     dcg     I haven't seen the backlog here (Client wasn't
connected) but have a couple of questions about Perl::Critic policies
12:39:55     @mst    short version
12:40:04     dcg     k-man: mentioned that you (in particular) don't
like ProhibitExplicitReturnUndef
12:40:06     @mst    1) most of the builtin ones are crap and don't
reflect how perl is written in the real world
12:40:16     @mst    2) the Freenode preset projibits actual mistakes we
see regularl;y
12:41:23     @mst    well, yes, that one introduces bugs and security holes
12:41:49     @mst    which suggests that whoever created the list with
it on isn't any good at perl
12:42:19     dcg     Ok, I'm interested in how, I thought all subs have
an implicit return that is equivalent to "return;"
12:43:11     k-man   now that I have sufficiently stired the pot, I have
to go now
12:43:17     k-man   :)
12:43:18     @mst    subroutines without an explicit return will
explicitly return whatever their last expression is
12:43:18     k-man   ttyl
12:43:22     @mst    but
12:43:24     @mst    return;
12:43:26     dcg     (note: I code in many languages, and freely admit
my indepth perl knowledge is sadly lacking, and sometimes clouded by
other languages)
12:43:28     @mst    is 'return wantarray ? () : undef;'
12:43:40     @mst    generally, wantarray is a code smell
12:44:04     dcg     k-man: thanks for that <eyeroll>
12:44:16     @mst    for example
12:44:37     k-man   dcg, i'll be back later and read the backlog, I
12:45:14     @mst    sub lookup_row { my ($id) = @_; if (my @row =
$dbh->selectrow_array(...) { my %row; @row{qw(id name foo bar)} = @row;
return \@row } return undef }
12:45:34     @mst    dcg: yes, I know that's not quite what you'd really
write but can we accept it as an example of a pattern?
12:46:00     @mst    now, consider
12:46:24     @mst    my %info = (row => lookup_row($id), id => $id);
12:46:45     @mst    if you replace 'return undef' with 'return', now in
the case where the row doesn't exist, you get
12:46:52     @mst    my $info = (row => 'id', $id => undef);
12:47:03     @mst    i.e. you just handed control of a hash key to
whatever code you got $id from, maybe a user
12:47:10     @mst    so now I sent your code ?id=is_admin
12:47:19     @mst    and SURPRISE bug/security hole
12:47:47     @mst    basically ProhibitExplicitUndef will make you add
'The Perl Jam' CGI.pm-style security holes *back in to your code*
12:47:56     @mst    this does not, to me, seem like a good idea.
12:48:56     @mst    (brb)
12:49:17     @mst    dcg: ^^
12:55:51     dcg     mst: Ok, I sortof see the problem.
12:55:51     dcg     I wasn't aware that perl returned the kitchen sink
like that.
12:55:51     dcg     Would a reasonable requirement be that ALL subs
have an explicit return, with an explicit value returned?
12:55:52     dcg     ie: 'return ($retval);'
12:55:52     dcg     Even if $retval is UNDEF if that's what it really
should be?
12:56:19     @mst    'returned the kitchen sink' ?
12:57:12     dcg     Well, it seems from your example, that "too much"
is returned if using a simple "return;" hence the kitchen sink comment
12:57:17     @mst    no
12:57:29     @mst    *nothing* is returned
12:57:51     @mst    again: return; is return wantarray ? () : undef;
12:58:19     dcg     Ah, sorry, Need to re-read your example.
12:59:52     dcg     Yep, on re-reading I caught what I missed the first
scan around :-(
13:00:22     @mst    right
13:00:38     @mst    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
13:04:23     dcg     mst:
https://perlmaven.com/how-to-return-undef-from-a-function suggests that
both "return;" and "return undef;" have their own issues/side-affects
and that there really is no easy way out.
13:04:23     dcg     Do you have any thoughts on that article?
13:04:23             {yaURLs}~> http://tinyurl.com/lao5e9s (perlmaven.com)
13:06:00     @mst    oh gods perlmaven
13:06:11     dcg     Yeah, Yeah, I know......
13:06:46     @mst    yeah, no, because the example where 'return undef'
is wrong is ... list context
13:06:54     dcg     Don't worry, I take EVERYTHING on the web with a
tablespoon of salt, hence the questions I'm asking now ;-)
13:06:58     @mst    the whole point is 'return undef;' is correct for a
*scalar* function
13:07:26     @mst    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
13:07:35     @mst    (I don't claim my code 100% follows these rules ;)
13:08:40     @mst    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
13:08:43     @mst    and, well, yes, you will
13:09:10     @mst    but then my original complaint is that 'return;'
turns a scalar returning fuynction into a
maybe-scalar-maybe-list-returning function
13:09:14     @mst    which EW
13:09:39     dcg     mst: Ok, that makes sense. And I totally agree.
13:11:06     @mst    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
13:11:35     @mst    some of my greatest sadnesses over my career have
involved maintaing code where I decided 'wantarray' was a good idea
13:11:46     dcg     My first language was TurboPascal 1.x and most
other languages I use are strongly typed (or in the cast of assembly
everything is binary anyway)
13:11:46     dcg     This means I can tend to overlook and oversimplify
the vagaries of typing in perl etc
13:11:50     @mst    and then for scalar-returning functions, 'return;'
introduces an implicit wantarray which argh
13:12:29     @mst    perl is weird. it is, once you reach a certain
point, actually mostly the same sort of weird uniformly
13:12:44     @mst    (but you need to have a level of fluency to see
*how* it's mostly the same weird0
13:12:48     @mst    but, still, weird :)
13:13:31     dcg     Ok, Thanks very much for your input there, It helps
me (and hopefully #ledgersmb) significantly.
13:15:07     @mst    in fact, many years ago somebody tested DBIC-using
code for basically exactly the lookup_row bug while trying to exploit it
13:15:19     @mst    and DBIx::Class containing an explicit 'return
undef' was why we passed the security audit
13:15:32     @mst    (I just went and found said undef just to check I
remembered right ;)
13:16:08     dcg     Before I go spend some easter time with SWMBO were
there other policies in this link https://gist.github.com/briandfoy/4525877
13:16:08             {yaURLs}~> http://tinyurl.com/moaajj7 (gist.github.com)
13:16:47     dcg     Ah, cool, that's a good datapoint, the fact that
"return undef;" helped you pass an audit
13:17:29     @mst    I think about half of those make some vague sort of
13:17:34     @mst    and the other half are either pointless or harmful
13:18:07     dcg     mst: Sorry, incomplete request there.
13:18:07     dcg     Other Policies at that link you consider to be "Bad"
13:19:03     @mst    yes, probably
13:19:14     @mst    going through each one in detail is quite a bit of
effort though
13:20:45     dcg     Fair enough, If any jump out at you we'd be
interested to know, but I'm not asking you to spend any significant time
(unless your at a loss for something to do :-o)
13:20:58     @mst    well ProhibitBooleanGrep is a pessimisation
13:21:14     @mst    ProhibitStringySplit is silly and unidiomatic
13:21:33     @mst    ProhibitExplicitISA is probably fine because it's
rare you actually want to do that
13:22:00     @mst    RequireCarping is at best pointless and at worst
screws up your error handling
13:24:02     @mst    so ... three I'd avoid out of the first ten
13:24:14     @mst    which is why I'm not convinced of the list in general
13:24:41     @mst    though 4,5,7,8,10 all seem pretty reasonable to me
for most code
13:25:24     @mst    ProhibitInteractiveTest misses the point
13:25:35     @mst    RequireCheckedClose # no. really. just no.
13:26:14     @mst    meh, this is just depressing
13:28:30     dcg     mst: Once again thanks for your knowledge, It's
invaluable IMHO
13:28:52     dcg     for RequireCheckedClose I'm struggling to
understand that one as well.
13:30:31     dcg     for a webapp in particular does it really matter in
most cases (reading a file) if you could close it without an error?
13:30:31     dcg     What conditions would cause an error on close
anyway? The file no-longer being accessible?
13:31:14      *      mst cries
13:31:25     @mst    you're making me want to try and bikeshed a decent
set of policies
13:31:27     @mst    and oh gods
13:31:50     @mst    dcg: btw, this is the sort of stuff metatrontech
used to poke me for and I was always happy to
13:31:54     @mst    however
13:32:09     dcg     Oh, dear, I'm sooooo sorry. I didn't mean to ruin
easter for you ;-)
13:34:35     @mst    hmm
13:34:46     @mst    actually, the rest of the list is holding up
better, roughly
13:34:54     @mst    poke me tomorrow and I'll try and go through the
rest of it
13:35:12     @mst    I'm starting to think that about 2/3 of this list
would genuinely be a good start
13:35:46     dcg     That would be awesome, if you have the time.
13:37:20     dcg     Feel free to drop in at #ledgersmb or hit me up in
a PM on freenode (nick is dcg_mx)
13:37:29     @mst    aight, shall do
13:37:42     dcg     once again, thanks for your expertise.

Jason Lewis

Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Ledger-smb-devel mailing list