This post is about how to fix a bug in PMD - the project mess detector. PMD is a source code analyzer tool for Java and many other languages. It checks your source for common programming mistakes so you don’t repeat them. However, as with all software, it is not perfect. PMD itself has bugs which lead to either false positives, e.g. when PMD reports a mistake which is not really a problem. Or to false negatives, which means, that PMD doesn’t detect the potential bug.

The different problems PMD can detect are implemented as so called rules. Each rule in PMD is responsible to detect a potential problem. The rules are language dependent. For java, there are many rules. They are organized in rulesets. The default rulesets for Java cover quite a big area. No wonder there are bugs.

There are different categories of bugs. In this post, I’ll talk about bugs that are affecting a specific rule and do not touch the infrastructure itself that PMD provides. These bugs would be affecting how PMD parses the source code, leading to parsing errors or not detecting usages of variables which points to the symbol table algorithm. But for now: Simple bugs that only affect one specific rule. The bug - arbitrarily chosen - is bug #1449. It says:

False positive when casting variable to short

The affected rule is AvoidUsingShortType. It seems, that this rule should not trigger, if you cast explicitly a variable to short, because then you as a programmer expressed the intention, to actually use short. The rule is in the controversial ruleset and the full documentation can be found online:

Java uses the ‘short’ type to reduce memory usage, not to optimize calculation. In fact, the JVM does not have any arithmetic capabilities for the short type: the JVM must convert the short into an int, do the proper calculation and convert the int back to a short. Thus any storage gains found through use of the ‘short’ type may be offset by adverse impacts on performance.

While this rule is in the controversial ruleset - which means, it your opinion about the usefulness of this rule might be different - the bug still makes sense.

Step by step

In order to fix a bug, several steps need to be taken. First, we are going to reproduce the bug with a new unit test. Second, we are going to fix it. Third, either we create a pull request or - in my case, I’ll merge it into the different branches so that the bugfix will be in the next release.

Before we begin, we need the source. Therefore we are going to create a new branch based on version 5.3.x - at the moment, that the oldest version I’m going to provide bugfixes for. And this rule is old, so the bug probably already exists in the 5.3.x-branch.

$ git checkout -b bug-1449 pmd/5.3.x
Switched to a new branch 'bug-1449'

Reproducing with a unit test

The next step in fixing the bug is to reproduce it. We already know, that the rule is AvoidUsingShortType. Each rule has a XML file associated with it, which contains test cases specifically for this rule. Search for it:

$ find -name "AvoidUsingShortType.xml"
./pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/controversial/xml/AvoidUsingShortType.xml

So, the file is found under src/test/resources within a subfolder of controversial - this means, we found the right file. This file needs another test case. The file is a XML file with the root element being test-data and multiple child elements test-code. Each test-code represents a separate test case. Let’s add the following new test case:

    <test-code>
        <description>#1449 false positive when casting a variable to short</description>
        <expected-problems>0</expected-problems>
        <code><![CDATA[
public class CastToShort {
    public void testcase() {
        ByteBuffer buffer = ByteBuffer.allocate(2);
        buffer.putShort((short) "1111".getBytes().length);
    }
}
        ]]></code>
    </test-code>

As description I usually use the bug title. As this is a test for a false positive bug, the expected-problems are none. The code tag contains a small java code snippet, that can reproduce the problem. Please note, that the code needs to be parseable but not necessary compile - like here, it would not compile, because you would need to import the ByteBuffer class.

Now let’s run the tests. In order to speed up things a bit, we’ll only run the test class, that starts the test cases for our rule and not all tests. The test classes are organized by ruleset, so each ruleset has its own test class. In our case, the test class is called ControversialRulesTest (full name “net.sourceforge.pmd.lang.java.rule.controversial.ControversialRulesTest”). With maven, you can run test test as follows:

$ mvn -f pmd-java/pom.xml test -Dtest=ControversialRulesTest

You should see as a result this line:

Failed tests: 
  ControversialRulesTest>RuleTst.runTest:106 "#1449 false positive when casting a variable to short" resulted in wrong number of failures, expected:<0> but was:<1>

Tests run: 115, Failures: 1, Errors: 0, Skipped: 0

This means, we succesfully reproduced the problem and can now start fixing it.

Bugfixing the problem

Now it depends what kind of rule AvoidUsingShortType is. In PMD there are two kinds of rules: XPath-based rules and Java-based rules. See also How to write a Rule.

Let’s open the ruleset file controversial.xml which can be found in the folder pmd-java/src/main/resources/rulesets/java and search for our rule entry:

  <rule
        name="AvoidUsingShortType"
       language="java"
        since="4.1"
        message="Do not use the short type"
        class="net.sourceforge.pmd.lang.rule.XPathRule"
      externalInfoUrl="${pmd.website.baseurl}/rules/java/controversial.html#AvoidUsingShortType">
        <description>
            <![CDATA[
Java uses the 'short' type to reduce memory usage, not to optimize calculation. In fact, the JVM does not have any
arithmetic capabilities for the short type: the JVM must convert the short into an int, do the proper calculation
and convert the int back to a short. Thus any storage gains found through use of the 'short' type may be offset by
adverse impacts on performance.
            ]]>
        </description>
        <priority>1</priority>
        <properties>
            <property name="xpath">
                <value>
                    <![CDATA[
            //PrimitiveType[@Image = 'short']
                    ]]>
                </value>
            </property>
        </properties>
        <example>
            <![CDATA[
public class UsingShort {
   private short doNotUseShort = 0;

   public UsingShort() {
    short shouldNotBeUsed = 1;
    doNotUseShort += shouldNotBeUsed;
  }
}
       ]]>
     </example>
   </rule>

The class attribute defines the type of rule. In this case it is a XPath rule, as the class is XPathRule. If it would be a Java-based rule, the class attribute would point to the implementing class of the rule. But XPath rules, the implementation is a xpath expression, which can be found in the property xpath below. In our case, it is a very simple expression:

//PrimitiveType[@Image = 'short']

Which means, whenever the primitive type “short” is used in the source code, this rule triggers and reports a violation. Now we need to add an exception in case, the type is used within a cast expression. An easy way to see the structure of the source code, on which the xpath expressions are executed, is using the Designer. This is a small GUI application included in PMD that helps in creating new (XPath) rules. Start it with this maven command:

mvn -f pmd-java/pom.xml exec:java -Dexec.mainClass=net.sourceforge.pmd.util.designer.Designer

Now you can copy-paste the code from the test case into the upper-left text area and copy-pase the XPath expression into the upper-right textarea. Click the button “Go”. You’ll see the source code structure in the lower-left area as a tree and any resuls from the XPath expression in the lower-right area. See also the screenshot.

PMD Designer

So, we can see, that the structure looks like this:

CompilationUnit
    ...
    PrimarySuffix
        Arguments
            ArgumentList
                Expression
                    CastExpression
                        Type
                            PrimitiveType: short

Obviously the grandparent of the “PrimitiveType” node is a “CastExpression”. Simplest solution is to add another and expression with square brackets to make sure, that the grand parent is not a CastExpression:

//PrimitiveType[@Image = 'short'][name(../..) != 'CastExpression']

If you click again on “Go”, you should see, that there a “No matching nodes” found anymore. Great!

Let’s adjust the ruleset file controversial.xml with the fix and rerun all the tests of the Controversial ruleset, to make sure, we didn’t break anything:

$ mvn -f pmd-java/pom.xml test -Dtest=ControversialRulesTest

This time, we see that there a no failing tests anymore:

Results :

Tests run: 115, Failures: 0, Errors: 0, Skipped: 0

Contributing

Let’s update the changelog file, so that the bugfix will appear in the release notes of the next release: ./src/site/markdown/overview/changelog.md. Add the following entry under “Bugfixes”:

*   java-controversial/AvoidUsingShortType:
    *   [#1449](https://sourceforge.net/p/pmd/bugs/1449/): false positive when casting a variable to short

Commit your changes:

$ git add pmd-java/src/main/resources/rulesets/java/controversial.xml
$ git add pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/controversial/xml/AvoidUsingShortType.xml
$ git add src/site/markdown/overview/changelog.md
$ git commit -m "fixes #1449 false positive when casting a variable to short"
[bug-1449 44a8c0d] fixes #1449 false positive when casting a variable to short
 3 files changed, 16 insertions(+), 1 deletion(-)

Now, push it to your repo on github and create a pull request for the public pmd repo. I’ll merge the pull request as soon as possible. (Of course, this bug is now fixed, so you’ll need to pick another one).

Going further

More posts will follow that describe the XML format used for the test cases in more detail. Also, the Designer itself is worth a post. Another interesting question is: How to extend PMD with an own rule?

Stay tuned…