Request for Comments on Perl::Critic

[jthalhammer]

Dear Monks:

Perl::Critic is a static source code analyzer that I've been working on for several months now. There is still much work to do, but I think the project is starting to reach a critical mass and I'm hoping to get some feedback from the Perl community. Some of the issues I'm concerned about are:

• Interface and Configuration. I imagine that most folks will use Perl::Critic via the perlcritic command-line interface and use the .perlcriticrc file to configure Perl::Critic. Are these sufficiently robust and user-friendly?
• Extensibility. I wrote Perl::Critic with the hope that other developers would create their own Policy modules to suit their own tastes. Doing so requires a working knowledge of PPI, which is the Critic's underlying mechanism for parsing the source code. Are the PPI and Perl::Critic APIs simple enough to encourage other developers to contribute to the project?
• Editor Integration. Right now, Perl::Critic offers some degree of integration for savvy emacs and vim users. How can I get tighter integration with a broader range of editors and make the setup easier for non-guru types of people?
• New Policies. As of version 0.13, Perl::Critic has Policies for about 50 of Conway's "best practices." Over time, I hope to implement as many of them as possible. Of the remaining practices, which ones would people like to see first?
• Politics. Coding practices are always a sensitive area. Although I have used Conway's book as a reference, Perl::Critic is not limited to his rules and will even support contradicting rules. How can we design and represent Perl::Critic to serve the greater-good without alienating people or starting flame wars?
• Next Steps. In the near future, I hope to enhance Perl::Critic to actually correct the code instead of just complaining about it. But I have no idea how that feature should be implemented or how it will affect the current architecture. What are your thoughts on this?

If you're not already familiar with Perl::Critic, I invite you to download it from CPAN and give it a try. Even if you don't agree with it's Policies, I think you'll find that it has the potential to be a very useful tool. I look forward to getting your feedback.

Thanks!

-Jeff

[hossman]

I've only spent about 5 minutes looking at it, but so far it looks good.

Politics. Coding practices are always a sensitive area. Although I have used Conway's book as a reference, Perl::Critic is not limited to his rules and will even support contradicting rules. How can we design and represent Perl::Critic to serve the greater-good without alienating people or starting flame wars?

I think the key to not alienating people, is to provide an extremely large set of policies in the main distribution, but only enable a small subset of them by default -- a subset that nearly everyone on the planet can agree on, policies that identify truely risky patterns. This way, when people first try your tool, they don't feel "picked on" the first time they run it against their code base. Let them get comfortable with the system, and discover the plethora of policies they can choose to turn on, and gradually realize "hmmm ... yeah, that is a bad idea.

I haven't delved into your docs enough to know if this is already supported: but having a very easy way to turn on "all policies except the following list: ..." would be another good way to encourage people to use this tool without a lot of work

Examples of policies I think are good, but probably don't need to be on by default...

1. ControlStructures::ProhibitCStyleForLoops
2. CodeLayout::ProhibitParensWithBuiltins
3. CodeLayout::RequireTidyCode
4. ControlStructures::ProhibitCascadingIfElse

...these are all good ideas, but I don't know that I'd consider them dangerous -- let people turn them on manually once they clean up all their bigger problems.

PS: ValuesAndExpressions::ProhibitInterpolationOfLiterals seems to be overly critical. It doesn't seem to much of a stretch to use double-quotes to quote single quotes...

my $t = "can't please anybody";

[Roy Johnson]

Having some thematic groupings of policies, e.g., "Minimal" (what you propose), "PBP" (for as many of those policies as it handles), etc., might be handy for people who don't want to wade through a universe of settings.

[jthalhammer]

You can select/deselect Policies that match an arbitrary regular expression using the -include and -exclude options with perlcritic. See the POD for more details.
Based on your suggestions, I have some new ideas to make Perl::Critic a little kinder and gentler. Thanks for the great feedback!
-Jeff

[cchampion]

I used Perl::Critic 0.12 to clean some legacy code and I was impressed. Now I see that the latest version has many enhancements, and such a task should be much easier.

I like the interface, although the policy names are a little scary. If you could find a way of making them shorter (or even identifiable by some acronym, for instance ppwb as a shortcut to ProhibitParensWithBuiltins) perhaps they could become a habit for command line enthusiasts.

As for your plan to enhancements, I see very favourably your idea of covering most of Conway's book, but I don't think I would use a feature that changes my code automatically based on some rule. Sure it looks interesting, but I produce just enough bugs manually, that I don't need the ones introduced in my code automatically.

;)

Kudos for this great job!

[pernod]

I have performed static code analysis at $firm on C code with Logiscope, so I'll chime in with some general experiences with static analysis. The software in question was a business critical component (NDA, sorry), and we used Logiscope's built-in rules in addition to MISRA's ruleset as a basis for the analysis.

We found that the first runs drowned us in information on violated rules, many of which were of the type "Put your brackets at the end of the line." As a result of this we ended up evaluating all the 200 or so rules and only using around 20.

What's my point in this rambling intro? Static analysis is never better than your ruleset, and choosing a good ruleset is time consuming. I therefore second the suggestions of making a small default ruleset, and making it very clear that users should think hard about what they are looking for when performing the analysis. Static analysis is somewhat similar to test code coverage, in that it tells you how well you have done in a certain subset of your problem area, while completely ignoring any frolicking dragons in neighbouring areas.

Don't get me wrong, though. Both static analysis and code coverage are excellent ways of getting metrics for software, but they must be used with care. Know what they can help you with, and don't assume your software is perfect just because your metrics say so. I therefore think that using the rules from Conway's book is a bad idea, simply because the large number of rules will tell you your code is bad no matter how good you are. Using some of his rules, on the other hand, is a good idea. We just have to figure out which ones...

And by the way, my bosses will love you for making Perl::Critic. Great work, now we can take our own medicine :)

[Perl Mouse]

I looked at the first subroutine of Perl::Critic (sub new), and I noticed that it violates several of PBP's recommendations. For instance, while the API of sub new uses named arguments, it doesn't say all of them need to be passed in a hash ref. It also uses hash based objects, instead of using Class::Std. I also note regexes without a /s modifier, and even at least one without any modifier.

I started installing Perl::Critic, but it has an army of dependencies - when it way up in the dependence tree had dependencies on ExtUtils::AutoInstall and CPANPLUS, I aborted the process.

[friedo]

PBP is a great book and while I have nothing but immense respect for TheDamian, I think he goes a little off the deep end in parts of it. When reading it, I found that about 80% of it consisted of things I already do, 15% consisted of things that might be a good idea, and 5% consisted of things that made me say, "What the hell is this nutcase thinking?!"

But that's OK. Nobody agrees on coding standards, and in Perl especially, because there's a million ways to do everything. I like to think my code is clean, abstract, modular and well-organized, but I'm sure Damian would be horrified by some of my decisions. ("Oh no! friedo uses unless all over the place! He must be a drooling mental patient!) You get the idea.

That said, I don't think PBP is a good baseline to use for a code critic. A lot of the reccomendations are somewhat superficial, whereas a code critic should look primarily at dangerous constructs. (Non-use of strict, indirect method invocation, accidental autovivification, symrefs, overly complicated conditionals, etc.)

[tirwhan]

Ooh, interesting! Does not following PBP constitute a failing that should be flagged in and of itself? (I'm not being facetious here, I'm genuinely interested in what people think about this.) If so, to what degree? While I think PBP is a fantastic book, there are (to my mind) certain recommendations in it which can/should be ignored more readily than others and regarding the whole book as a "must-follow" sort of bible would pose more restrictions on module authors than I personally think sensible.

[jthalhammer]

The Perl::Critic distribution is fully self-compliant. If you have Test::Perl::Critic installed, then it automatically critiques itself during the build.
But I'm just like everyone else who has read PBP: there are some practices that I'm still working to adopt and there are some that I completely reject. Does this make Perl::Critic a heretic? I don't think so.
Regardless of how it is actually written, the idea behind Perl::Critic is to help developers follow clear and consistent coding practices. It doesn't actually matter what those practices are. I just chose to start with PBP becuase they are well known and come from a reputable source. Any developer is free to create Perl::Critic::Policy modules that enforce their style and practices.
-Jeff

[dave0]

First off, great job on Perl::Critic. The first thing I thought after reading PBP is "Hey, I wonder if there's a tool that would check my code for this stuff". And, as usual, it was on CPAN.

Here are my thoughts on some possible additions and modifications that might be useful:

• A HOWTO document on writing custom policies. This would make it much easier for people who don't yet grok PPI (like myself) to just dive in and write something, whether it's implementing the remaining things from PBP that can be submitted back to you, or a set of policy modules to match their own internal coding standards.
• Package configuration files for better editor integration. Being able to just type:

  :compiler perlcritic
  :make
  

  within Vim is very nice, and is easier to configure than the :grep hack I suggested earlier. I think I've already sent you my perlcritic.vim with instructions on setting it up, but if you didn't get it, let me know.
• An extension to the ## no critic syntax that would allow (or optionally require?) an explanation of the reason for overriding the Perl::Critic check in that case. Maybe something like:

  ## no critic: All the bad stuff below is necessary because [...]
  

  I could see this being possibly implemented as a Perl::Critic::Policy::Pragma::NoCriticRequiresComments, except that the 'no critic' blocks get stripped out before the policies are applied. Not sure if there's an easy way around that.
• A way to allow sets of policies to reference a URL, or page numbers in a different book. This would give you a mechanism to refer to the Safari version of PBP, or for new policies, allow them to refer to completely different documents altogether (ie: internal coding standards on company website, other books like Code Complete or AntiPatterns, or even just a 2nd edition of PBP with different page numbers).

  You could do it with a Perl::Critic::Reference class that would provide a lookup table and remove page references entirely from the Policy modules. You could look up the appropriate reference with something like:

  use Perl::Critic::Reference;
  my $ref = Perl::Critic::Reference->new();
  
  # Load reference data for policies from three different files
  $ref->load( qw( PBP FooCorpCodingStyle CodeComplete ) );
  
  # and later...
  my $explanation = $ref->lookup($violation->policy());
  

  Data files containing the explanation reference could be subclasses of Perl::Critic::Reference (with the parent delegating the ->lookup() to each of the load()'ed subclasses in order until a hit is found), or they could just be YAML files merged into a single hash for lookup.

Hmm... that Reference idea sounds useful, and not too difficult. If you'd take the patch, I think I might try coding it up.

One last comment before I head off to sleep. I'm not sure I agree with the idea that Perl::Critic could or should be enhanced to correct the code. I use Perl::Critic to warn me when I'm doing something silly, so that I can fix it, and by fixing it, learn to not do it again. If it tried to correct those things for me, there's less of an opportunity to learn. Plus, some of the things detected by the policies would be downright nasty, if not impossible, to auto-correct without potentially breaking the code.

[EvanCarroll]

I have never used Perl::Critic, but congrats on a very impressive and abstract module, along with its awe inspiring documentation. Looks interesting.

[wolfger]

I don't know about Perl::Critic, but you did just inspire me with a desire to write ACME::PerlCritic. Similar, but of a much less-serious nature. :-)

[sfink]

Minor point: I'm not crazy about the command-line name perlcritic. perlcc is in my path, so I would have to type perlcr before I can hit tab for command completion. If it were, say, critique, I could get away with cri then tab. I notice that wtf is also available on my machine as a complete command name. Might make sense for much of my code...

[Perl Mouse]

    $ alias critique='/usr/local/bin/perlcritic'

    $ (echo '#!/bin/sh'; echo '/usr/local/bin/perlcritic $*') > ~/bin/
+critique
    $ chmod +x ~/bin/critique
    $ PATH="$PATH:~/bin"