http://www.perlmonks.org?node_id=596691

After more than 18 months of development, Perl-Critic has reached it's first major release! Version 1.01 is now available on a CPAN mirror near you.

For those of you who aren't familiar with it, Perl-Critic is a highly flexible static source code analyzer (like lint, but for Perl code). It automaticaly detects potential bugs, awkward constructs and over-complicated code. Most of the rules are based on TheDamian's book "Perl Best Practices".

No matter what your personal preferences are, Perl-Critic can help you and your team write more consistent, robust, and maintainable code. Numerous open-source projects and commercial development shops have adopted Perl-Critic as an integral part of their development process. And at my $work, we use Perl-Critic as a tool for coaching junior developers.

So I invite all of you to give Perl-Critic a shot. If you want to see what it does without installing anything, try using the Perl-Critic web service at http://perlcritic.com.

I sincerely thank all the PerlMonks who have contributed code, ideas, and valuable feedback throughout the Perl-Critic project.

-Jeff

Replies are listed 'Best First'.
Re: Module Announcement: Perl-Critic-1.01
by stvn (Monsignor) on Jan 26, 2007 at 13:13 UTC

    Leaving the whole thing about RCS keywords (my grandfather used RCS, I use subversion ;) and specifc POD sections (maybe I don't want to name them the way TheDamian does) aside, I am surpised at a couple of things in the output:

    • Magic punctuation variable used at line ...
    • The magic punctuatinon it is talking about is $@ without which I could not really catch exceptions. I think that is a little severe.

    • Useless interpolation of literal string at line ...
    • I don't even get that one because the line in question is basically something like this:

      confess 'Some error happened because ' . $obj->foo . ' blew up';
      I would have to say that this string was far from useless.

    • Subroutine does not end with "return" at line ...
    • Now, I can understand this for large subroutines, but the sub in queston was basically this:

      sub mumble_path { join ", " => split '/' => (shift)->get_path }
      I would argue that adding return to something like that would be just adding noise.

    Of course I know you are supposed to be able to customize Perl::Critic to your own tastes, but I wonder if the default settings are maybe not too severe? My first thoughts when I saw all the output was similar to BrowserUK, I felt like I was looking at some overpriced Java middleware exception stack trace.

    -stvn
      The magic punctuatinon it is talking about is $@ without which I could not really catch exceptions
      use English qw(-no_match_vars); eval { die 'A horrible death' }; print "Something died\n" if $EVAL_ERROR;
      Useless interpolation of literal string at line ...
      I don't even get that one because the line in question is basically something like this:
      confess 'Some error happened because ' . $obj->foo . ' blew up';
      I would have to say that this string was far from useless.

      If those were double-quotes instead of single-quotes (which they probably were) then the warning was precisely correct.

      sub mumble_path { join ", " => split '/' => (shift)>get_path }
      I would argue that adding return to something like that would be just adding noise.

      You certainly can argue! Perl-Critic-Lax is a suite of slightly more forgiving versions of several Policies. Perhaps you could write a new module for it.

      Perl::Critic is lenient by default. Normally, it only reports the most severe issues (the relative severities are configurable). However, the web-service is currently configured for maximum strictness. I suppose I should loosen that up a bit. One of these days, I'd like to expose all the Perl::Critic configurations through the web-service, but that's a ways off.

      -Jeff
        However, the web-service is currently configured for maximum strictness. I suppose I should loosen that up a bit.

        I suspect you'll get more positive reactions if you tune the level of strictness for what you think "typical" (hah!) programmers would find helpful. Not everyone takes PBP as gospel.

        Alternatively, even if you don't expose all the configuration via the service, perhaps either having two options "Analyze my code aggressively" or "Analyze my code mildly" would help. Or perhaps just grouping the output in descending order of severity -- so people see the highest severity issues first and then can decide where to stop paying attention.

        -xdg

        Code written by xdg and posted on PerlMonks is public domain. It is provided as is with no warranties, express or implied, of any kind. Posted code may not have been tested. Use of posted code is at your own risk.

        use English qw(-no_match_vars); eval { die 'A horrible death' }; print "Something died\n" if $EVAL_ERROR;

        Hmm, well that is one way to do it, but I am not sure it is a better way. IMO English.pm ends up just obscuring the more common "special" punctuation variables which any decent Perl hacker will already know by heart. Not to mention the fact that I REALLY DONT LIKE $SOURCE_CODE THAT @SHOUTS AT $ME ALL THE *TIME.

        If those were double-quotes instead of single-quotes (which they probably were) then the warning was precisely correct.

        Nope, they were not double quotes. My personal policy is to only use double quotes when interpolating, and single quotes otherwise (so that accidental interpolation is just not possible). This is why this one threw me, cause it just seems wrong.

        Perl::Critic is lenient by default. Normally, it only reports the most severe issues (the relative severities are configurable). However, the web-service is currently configured for maximum strictness. I suppose I should loosen that up a bit. One of these days, I'd like to expose all the Perl::Critic configurations through the web-service, but that's a ways off.

        I agree with xdg, a more lax policy on the webservice and/or the ability to select that more lax policy would certainly be a nice addition. If for no other reason then to demonstrate the power and flexibility of Perl::Critic.

        Now please do not misinterpret me, I think Perl::Critic is a very important tool, and one which the community desperately needs. However nothing will turn people off more than 40-50 lines of pretty severe warnings only maybe 2-3 of which are actually valuable advice. A more lax policy (this is Perl after all) would likely help adoption even if it lets a few of TheDamian's edicts slip by.

        -stvn
        confess 'Some error happened because ' . $obj->foo . ' blew up';
        If those were double-quotes instead of single-quotes (which they probably were) then the warning was precisely correct.

        No, that can't be it, because the strings in question contain none of the things Critic cares about, namely unescaped $. or @. and escaped metachars. I suspect that your parser is not detecting the fact that the confess argument is actually three strings concantenated together, and it's thinking the $obj is inside the string.

        Anyway, I can't verify any of the above, since I can't get the confess line to trigger a warning from the web-based Critic now.

        A word spoken in Mind will reach its own level, in the objective world, by its own weight

        update: I overlooked the -no_match_vars (ETOOMUCHNOISE? :-), the impact of use English isn't that nefarious in the advised usage. But even so, I stand to my statement.

        jthalhammer,

        The magic punctuatinon it is talking about is $@ without which I could not really catch exceptions
        use English qw(-no_match_vars); eval { die 'A horrible death' }; print "Something died\n" if $EVAL_ERROR;

        Please go read perlvar. I include the relevant bits here:

        BUGS
        Due to an unfortunate accident of Perl's implementation, "use English" imposes a considerable performance penalty on all regular expression matches in a program, regardless of whether they occur in the scope of "use English". For that reason, saying "use English" in libraries is strongly discouraged. See the Devel::SawAmpersand module documentation from CPAN ( http://www.cpan.org/modules/by-module/Devel/ ) for more information.

        Suggesting use English to avoid the use of $@ is just

        plain silly

        --shmem

        _($_=" "x(1<<5)."?\n".q·/)Oo.  G°\        /
                                      /\_¯/(q    /
        ----------------------------  \__(m.====·.(_("always off the crowd"))."·
        ");sub _{s./.($e="'Itrs `mnsgdq Gdbj O`qkdq")=~y/"-y/#-z/;$e.e && print}
      It's better to be strict by default. If people only turned on the warnings that they thought were useful, they wouldn't know about the other warnings that are helpful that they discovered by mistake.

      People use lint and it's hardcore stringent by default, too.

      xoxo,
      Andy

        It's better to be strict by default.

        As a coding practice, I agree. As a marketing device, I don't. First impressions count and missing RCS variables probably shouldn't be the first impression people get from the Perl::Critic website.

        -xdg

        Code written by xdg and posted on PerlMonks is public domain. It is provided as is with no warranties, express or implied, of any kind. Posted code may not have been tested. Use of posted code is at your own risk.

Re: Module Announcement: Perl-Critic-1.01
by BrowserUk (Patriarch) on Jan 26, 2007 at 11:37 UTC

    Hmm. I selected a file from my test dir at random:

    #! perl -slw use strict; use Devel::Timer; sub firstNprimes { my $n = shift; open my $primes, '<:raw', 'data\primes.all' or die $!; my @primes = split ' ', do{ local $/ = \( $n * 10 ); <$primes> }; close $primes; return \@primes; } BEGIN{ <STDIN>; } my $T = new Devel::Timer; my $ref = firstNprimes( $ARGV[ 0 ] ); $T->mark( $ARGV[ 0 ] ); { local $\; $T->report; } printf 'Check mem'; <STDIN>; __END__ C:\Perl\test\data>..\junk4 Devel::Timer Report -- Total time: 0.3176 secs Interval Time Percent ---------------------------------------------- 02 -> 03 0.3169 99.78% 100,000 -> 1,000,000 01 -> 02 0.0007 0.21% 201 -> 100,000 00 -> 01 0.0000 0.01% INIT -> 201

    And and gave it to your demo page. (It'd be nice if the results where prefixd with a MIME/text header so that my browser wouldn't wrap everything.) I got:

    Code before strictures are enabled at line 1, column 1. See page 429 o +f PBP. RCS keywords $Id$ not found at line 1, column 1. See page 441 of PBP. RCS keywords $Revision$, $HeadURL$, $Date$ not found at line 1, column + 1. See page 441 of PBP. RCS keywords $Revision$, $Source$, $Date$ not found at line 1, column +1. See page 441 of PBP. No "VERSION" variable found at line 1, column 1. See page 404 of PBP. Code before warnings are enabled at line 1, column 1. See page 431 of +PBP. perltidy had errors!! at line 1, column 1. See page 33 of PBP. Code not contained in explicit package at line 1, column 1. Violates e +ncapsulation. Code not contained in explicit package at line 2, column 33. Violates +encapsulation. Useless interpolation of literal string at line 2, column 38. See page + 51 of PBP. Code not contained in explicit package at line 2, column 51. Violates +encapsulation. Useless interpolation of literal string at line 2, column 60. See page + 51 of PBP. Code not contained in explicit package at line 7, column 1. Violates e +ncapsulation. Mixed-case subroutine name at line 9, column 1. See page 44 of PBP. Code not contained in explicit package at line 9, column 1. Violates e +ncapsulation. Code not contained in explicit package at line 10, column 5. Violates +encapsulation. Code not contained in explicit package at line 11, column 5. Violates +encapsulation. "die" used instead of "croak" at line 11, column 52. See page 283 of P +BP. Magic punctuation variable used at line 11, column 56. See page 79 of +PBP. Code not contained in explicit package at line 12, column 5. Violates +encapsulation. Quotes used with an empty string at line 12, column 24. See page 53 of + PBP. Code not contained in explicit package at line 12, column 33. Violates + encapsulation. Magic punctuation variable used at line 12, column 39. See page 79 of +PBP. Code not contained in explicit package at line 12, column 47. Violates + encapsulation. Code not contained in explicit package at line 12, column 58. Violates + encapsulation. Code not contained in explicit package at line 13, column 5. Violates +encapsulation. Code not contained in explicit package at line 14, column 5. Violates +encapsulation. Code not contained in explicit package at line 17, column 1. Violates +encapsulation. Code not contained in explicit package at line 17, column 8. Violates +encapsulation. Code not contained in explicit package at line 18, column 1. Violates +encapsulation. Code not contained in explicit package at line 20, column 1. Violates +encapsulation. Code not contained in explicit package at line 20, column 25. Violates + encapsulation. Code not contained in explicit package at line 20, column 32. Violates + encapsulation. Code not contained in explicit package at line 21, column 1. Violates +encapsulation. Code not contained in explicit package at line 21, column 11. Violates + encapsulation. Code not contained in explicit package at line 21, column 18. Violates + encapsulation. Code not contained in explicit package at line 23, column 2. Violates +encapsulation. "local" variable not initialized at line 23, column 3. See page 78 of +PBP. Code not contained in explicit package at line 23, column 3. Violates +encapsulation. Magic punctuation variable used at line 23, column 9. See page 79 of P +BP. Code not contained in explicit package at line 23, column 13. Violates + encapsulation. Code not contained in explicit package at line 25, column 1. Violates +encapsulation. Module does not end with "1;" at line 25, column 21. Must end with a r +ecognizable true value. Code not contained in explicit package at line 25, column 21. Violates + encapsulation. Code not contained in explicit package at line 27, column 1. Violates +encapsulation.

    Have you ever heard the parable of the Boy That Cried Wolf? There may be one or two useful messages in there somwhere, but they are so drowned by useless and incorrect noise, that I cannot see them. I cannot even make the line numbers tally?


    Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
    "Science is about questioning the status quo. Questioning authority".
    In the absence of evidence, opinion is indistinguishable from prejudice.

      How odd. It appears that the report you got was created on Jan 19 and pertains to some other file that was previously uploaded. Somehow, the web-service dished up the wrong analysis. I'll have to dig into it. Thanks for exposing that!

      When I analyze your code with 'perlcritic' directly, the results seem much more sensible, and a lot less noisy.

      -Jeff

        I just tried again, and got

        I knew I wouldn't like it, but I at least expected to have to justify my arguments. But this?

        This is just another example of the PC madness, like http://news.bbc.co.uk/1/hi/england/north_yorkshire/6296637.stm and many, many other examples that is gripping our world and turning it into a mirthless, freedomless place of bland, grey uniformity.

        Thanks theDamian. You've gone from being the wild child of Perl, envied and awed by the many; to being the pillar of salt that the PHBs will forever use to enforce mediocrity, uniformity and the lowest common denominator upon the once hip and innovative and freedom loving world of Perl. Like the reformed smoker, or the born again religious, when you converted, you made sure that the PHBs will enforce your will upon the rest of us too.


        Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
        "Science is about questioning the status quo. Questioning authority".
        In the absence of evidence, opinion is indistinguishable from prejudice.
      Yeah. Looks like 18 months of development for the trashcan. But that's just me.

      Update: Me too used the web interface which is apparently broken. Using the version from CPAN gives better results. Apologies.


      holli, /regexed monk/

        I just cleared out the cache and restarted apache. Not sure what was wrong. Seems to work better now. I'm glad you guys spotted that.

        -Jeff
Re: Module Announcement: Perl-Critic-1.01
by jettero (Monsignor) on Jan 26, 2007 at 15:01 UTC
    The one that gets me presently is "return" statement with explicit "undef" at line 942, column 5. See page 199 of PBP. I'd like to know when being explicit has ever been a problem. I'm almost tempted to buy that book just to see what the argument is on page 199. Almost.

    Update: Ahh, I see... I hadn't thought about list context since the function only ever returns scalars and the results are all checked with if defined $scalar_name. I'm going to go ahead and stick with return undef if $something for functions that only return scalars and consider the advice given by Perl::Critic to be educated guesses or tips.

    -Paul

      Maybe it's trying to encourage you to specify the return value explicitly, as in

      return undef;
      or
      return ();

      Update: Here's a table of the differences:

      return undef; return (); return;
      scalar $sv = foo(); undef undef undef
      boolean if (foo()) { } false false false
      string '' . foo() '' with warning '' with warning '' with warning
      number 0 + foo() 0 with warning 0 with warning 0 with warning
      list my @a = foo(); ( undef ) ( ) ( )
      boolean list assignment if (my @a = foo())
      if (my ($sv) = foo())
      if (() = foo())
      true false false
      scalar list assignment my $sv = () = foo() 1 0 0

      Note: How list assignments are treated in scalar context is what allows
      while (my ($key) = each(%hash))
      to work even if there's a key named 0.

        Maybe it's trying to encourage you to specify the return value explicitly...

        Actually, exactly the opposite. It says you shouldn't explicitly return undef because you might actually mean return (), and that return; dwims.

        -Paul

        "return;" is identical to "return();"
      Returning an explicit undef can be a problem if the sub gets called in list context. Because then the return value will be a one element list containing an undef, which would evaluate to "true" and that is in most cases not what you want. A naked return; dwims in scalar and list contexts.


      holli, /regexed monk/
        the return value will be a one element list containing an undef, which would evaluate to "true" and that is in most cases not what you want.
        Boolean list context? I'm baffled as to what case you are concerned about. As to "most cases", I quibble. Some, but not most.

      If your function normally returns scalars, then doing return; is likely the wrong thing to do. If your function at least sometimes returns lists, then return undef; is likely the wrong thing to do. The latter is covered by other replies.

      The reason that the former is wrong is because it can break code like:

      my %hash= ( key1 => genValue1(), key2 => genValue2() }; # %hash= ( key1 => 'key2' ); is never what you want to get

      And the problems with return undef; don't really apply to functions that only ever return a scalar because you don't write code like:

      if( @array= getScalar() ) { ... } else { die "Couldn't get scalar"; }

      So, unless Perl::Critic checks whether your function ever returns other than 1 scalar elsewhere, I consider this warning to be encouraging a bad practice (rather than just pushing a reasonable practice too hard as many people will find many of the possible warnings, surely).

      - tye        

        The reason that the former is wrong is because it can break code like:

        Why do you not consider the hash assignment code broken? If you need to call a function in scalar context, make it explicit.

      Well, whenever being explicit breaks things. return; and return undef; is NOT the same thing! It does return the same thing in a scalar context, but a very different thing in a list one. So for example

      sub foo { return undef; } if (@results = foo()) { print "Helo jettero :-P\n"; }
      does print the greeting. Because the function returns a list containing one element, the undef. And once you assign the list to an array and evaluate that assignment in scalar context you get the number of elements in the list and thus a true value. Probably not what you wanted, right?

        You are correct. I can't argue that that is what happens when you return undef.

        Now - the larger question is, who is still designing Perl module interfaces that return non-scalar results.

        I can't remember the last time I designed a function to return a list of values - much less variably return a list or a single scalar depending upon context. To me it is a code smell to not return a single scalar value (the single value could be arrayref or hashref - but it is still a single value). I know that may not sound Perlish - but as a user of the modules I produce I prefer not have to guess what context I need to call items in. It also saves memory to return arrayrefs or hashrefs.

        Yes I still use map and grep and caller and times and stat, but they are some of the last things I use that return lists. Perl 6 will fix some of these issues.

        my @a=qw(random brilliant braindead); print $a[rand(@a)];
      I'd like to know when being explicit has ever been a problem.

      It's a problem when return happens in array context -- you get a literal undef in the array. Contrived example:

      use Data::Dump::Streamer; sub filter_true { if ( my $v = shift ) { return $v; } else { return undef; } } my @values = (1, 0, 0); my @true_values = map { filter_true($_) } @values; Dump \@true_values

      Gives:

      $ARRAY1 = [ 1, ( undef ) x 2 ];

      -xdg

      Code written by xdg and posted on PerlMonks is public domain. It is provided as is with no warranties, express or implied, of any kind. Posted code may not have been tested. Use of posted code is at your own risk.

      Well, I guess it really depends on what you are being explict about :)

      The other replys to this node all have valid points. However, what if I really did want to return undef? This is valid use case too.

      -stvn

        Perl::Critic has a section about "Bending the Rules" -- you can bypass criticism if you really intended something. It's good if you usually follow the rules, but don't want to be hassled when you're knowingly breaking them.

        return undef; ## no critic

        -xdg

        Code written by xdg and posted on PerlMonks is public domain. It is provided as is with no warranties, express or implied, of any kind. Posted code may not have been tested. Use of posted code is at your own risk.

Re: Module Announcement: Perl-Critic-1.01
by webfiend (Vicar) on Jan 26, 2007 at 20:16 UTC

    This is very nifty, but I think it would be an even better demo with a couple of tweaks:

    • Make it possible to see what policies are being applied.
    • Provide a more lax policy set that just incorporates perlstyle.

    Perl-Critic itself with default policies looks impressive, and I will be poking at it. But its reconfigurability is even cooler, and I think the demo should show that off in some way.

Re: Module Announcement: Perl-Critic-1.01
by Raster Burn (Beadle) on Jan 26, 2007 at 16:49 UTC
    Wonderful! I love Perl::Critic, and have written many of my own policies (that are internal to my own company and not really useful to the CPANosphere)
      I'd love to know what those policies are. I think you might be surprised at how useful they might be.

      xoxo,
      Andy

        They're mainly to ensure cross-project portable code. I enforce the usage of certain environment variables instead of hard-coded values.