Blog post: Bugs!? - static analysis to the rescue

#perl #bug #staticanalysis

Dave Cross wrote an insightful blog post "A Subtle Bug" describing a anti-pattern in Perl resulting in a bug, which is quite subtle (hence the title of the original post).

Yes it is possible to do a scan/search of your code base and spot this bug. But a one-off analysis poses some issues, like keeping the same bugs out of our code base - permanently.

And this is exactly what static program analysis (or just static analysis) tools are for, especially when integrated with your source control system or CI/CD system.

The dominant static-analysis tool for Perl is named Perl::Critic. I have written several policies for Perl::Critic to enforce my own coding policies and I am quite keen on the concept of static analysis. The reason for my enthusiasm is that I really really like code reviews, but I have often observed that we tend to discuss the obvious, so if we want to get the most out of a human review, which takes time and effort, we leave the boring stuff to the computers, meaning static analysis of your code base and the humans can do what human are quite good at and that is spotting the other stuff using creativity, pattern matching and experience.

Based on Dave's post, which immediately triggered something with me, I have developed a first shot at a policy for discovering the anti-pattern and possible bug in your Perl source code.

Introducing: Perl::Critic::Policy::InputOutput::ProhibitHighPrecedentLogicalOperatorErrorHandling

sub _is_logical_operator {
    my ( $self, $sibling ) = @_;


    if ($sibling) {
        if ($sibling->class eq 'PPI::Token::Operator') {
            if ($sibling->content eq q{||}) {
                return $TRUE;
            }
        }
        return $self->_is_logical_operator($sibling->snext_sibling());
    }


    return $FALSE;
}

I do not like the name myself and it is somewhat wrong since the remedy of using or over || is also a logical operator. And secondly you can still use the || operator in combination with parentheses.

First implementation was very elegant relying only on the recursive function _is_logical_operator, but this did not handle the presence of parentheses.

So I had to introduce a regular expression :-/

    return if $elem->snext_sibling()->content() =~ m/^[\s]*[(]/xism;

Under the banner of clean code, I would love to at least isolate the regular expression - handling it differently would be AWESOME, so I am still pondering a solution.

I have notified Dave of my implementation and tweeted about it and now I am blogging, so hopefully I will get some reviews or feedback - did I mention I love code reviews? :-)

When I have stabilized the code and finalized the documentation, I am thinking about making a pull request to the Perl::Critic distribution, since I am not really interested in maintaining yet another stand-alone policy, but more on this in another blog post.

Please check it out at let me know what you think: