Javaの静的解析ツール「PMD」を導入してみた件

※KDDIコマースフォワード㈱ 、略称「KCF」は2019年4月1日、同グループ会社の㈱ルクサと合併し「auコマース＆ライフ株式会社」として再設立いたしました。 　本記事は2019年3月31日以前に書かれた記事のアーカイブとなります。予めご了承ください。

導入した背景について

こんにちは、KCFのエンジニアの坂本です。

今回は、静的解析ツールの導入の話です。
静的解析ツールといえば「どうでもいい細かいコードスタイルとか怒っていてウザい」と感じていませんか？

少なくとも、私はそうでした。

しかし、私たちのプロジェクト・チームの色いろな事情もあり、
「人間の眼によるレビューだけでなく、機械による自動的なチェックにも助けてもらう、必要がある。」
ということを痛感しました。
そこで、静的解析ツールを導入してみようということになったわけです。
今回はそんな話です。

静的解析ツールも色いろと種類があるわけで、
候補になりそうなものをざっとひと通り調べて見たのですが、
Javaの静的解析ツール「PMD」を採用することにしました。

pmd.github.io

導入の理由ですが、カスタムの設定が比較的シンプル・簡単に行えて、
自分たちのプロジェクトに合った設定を実現できそうだったからです。

例えば、極端な話、以下のようなカスタム設定XMLを用意すると、
「自分たちのコード・ベースに不要なimport文がないかどうか」
という事だけをチェックすることが出来ます。

<?xml version="1.0"?>
<ruleset name="customruleset">
    <rule ref="category/java/bestpractices.xml/UnusedImports" />
</ruleset>

そういうわけで、
「これなら自分たちのプロジェクトにも導入してつかえるんじゃないか？！」
と思ったわけです。

導入方法の簡単な紹介

さて、実際にPMDを動作させる方法ですが、非常にシンプルです。

先ほどのPMD本家サイトの「QuickStart」の部分にあるように、
例えば「MacOS」であれば、
以下のようにインストールしてみてください。

$ cd $HOME
$ curl -OL https://github.com/pmd/pmd/releases/download/pmd_releases%2F6.12.0/pmd-bin-6.12.0.zip
$ unzip pmd-bin-6.12.0.zip

そして、まず試しに実行してみる場合は、こんな感じですね。

$ alias pmd="$HOME/pmd-bin-6.12.0/bin/run.sh pmd"
$ pmd -d <プロジェクトのソースフォルダ> -R rulesets/java/quickstart.xml -f text

ここで例えば、さっきの
「不要なimport文がないかどうかだけをチェックする」
というカスタム設定XMLを
以下のようなパス＆ファイル名で保存したとします。

■<プロジェクトのソースフォルダ>
/static-code-analysis/pmd/custom_analysis.xml

その場合の実行はこんな感じになります。

$ alias pmd="$HOME/pmd-bin-6.12.0/bin/run.sh pmd"
$ pmd -d <プロジェクトのソースフォルダ> -R <プロジェクトのソースフォルダ>/static-code-analysis/pmd/custom_analysis.xml -f text

ちょっとこれは「追記」になるのですが、
開発IDE（EclipseやIntelliJなど）のPMDプラグインを試してみたのですが、 「カスタム設定XMLを指定してそれを読み込む」というような機能が見あたらず、 それだとチームのメンバーがそれぞれ「手動でチェックして設定を合わせる」 というような形になってしまうので、あまりしっくり来ないというかそれだと意味ないな、 と感じられ、
私たちのプロジェクトでは現在でも上記のようなコマンド・ベースで使っています。

と、ここまでは、
自分のPCの内部でローカル環境で実行しているイメージですが、
ほぼそのまま、インストールから実行まで、
CIサーバーのLinuxで同じことが出来ると思います。
（先ほどの、PMD本家サイトの「QuickStart」の「Linux」のタブの内容も参考にしてください。）

CIサーバーへのインストールはこんな感じ。

cd <PMDインストールフォルダ>
$ wget https://github.com/pmd/pmd/releases/download/pmd_releases%2F6.12.0/pmd-bin-6.12.0.zip
$ unzip pmd-bin-6.12.0.zip

CIサーバーでのPMDの実行はこんな感じ。

sh <PMDインストールフォルダ>/pmd-bin-6.12.0/bin/run.sh pmd -no-cache -d <プロジェクトのソースフォルダ> -R <プロジェクトのソースフォルダ>/static-code-analysis/pmd/custom_analysis.xml -f text || exit 0

このシェルを、例えば、Jenkins（やそれに類するもの）などでキックすることで、 テキストとしての解析結果を出力することになるかと思います。

PMDの「カテゴリーと優先度」について

さて、少しだけ話が変わって、
PMDのよい点として、
それぞれのチェック項目が、
Javaの１個のルール・クラスに対応していることです。

また、PMDはオープンソースであり、
オープンソース・コミュニティによって、
そのチェック項目（Javaクラス）がメンテナンスされています。

pmd.github.io

上に掲げたドキュメントを見てもらうと、分かるのですが、
それぞれのチェック項目には
■カテゴリーと優先度
という分類があります。

例えば、さっきの
「不要なimport文があるかどうか」のチェックは、
UnusedImportsRulesというJavaのクラスに対応しており、
このチェック項目については、
「カテゴリー」は「bestpractices」であり、
「優先度」は「Medium Low (4)」となります。

Best Practices | PMD Source Code Analyzer

pmd/UnusedImportsRule.java at master · pmd/pmd · GitHub

そこで、
このドキュメントを読みながら、
試しにチェック項目を追加してみて、
実際に解析結果がどのようになるかを見てみることで、
どのようなチェック項目を最終的に入れるべきかを検討して行きました。


ここからは、私たちのプロジェクトで、
■PMD導入にあたっての考え方・ポリシー
みたいなものをまとめてみたので、以下に紹介しておきます。


●方針１
まず、各カテゴリーに関して、
Priority: High (1)
のものを重視します。

もし、Priority: High (1) のチェックを無視する場合には、
明示的にコメントアウトの形で残しておき、なぜ無視しているのかの理由も合わせて書いておきます。

一方で、Priority: High (1) 以外でも、
Priority: Medium High (2) Priority: Medium (3) などで、これは有用だと思ったものは加えています。

●方針２
次に、以下のカテゴリーを重視します。
errorprone
multithreading
security

「errorprone」は辞書で引くと「エラーを引き起こしやすい」という意味です。

errorproneについては Priority: High (1) の項目に加えて、rulesets/java/basic.xml の errorprone にあるもの全てを加えてあります。
multithreadingについては、UseConcurrentHashMapを除いて、全項目を加えてあります。
securityについては、親のsecurity.xmlだけを指定しているので、いつも全項目がチェックされるようにしてあります。（現状では2項目しかありませんが。）

●方針３
次に、bestpracticesのカテゴリーを重視します。
ここには、Priority: High (1) の項目はないものの、良いチェック項目があると思います。

例えば、「Effective Java」に昔からある、
「戻りの型がListやMapだったらnullを返すのではなく空のListやMapを返しましょう」
のようなことをチェックしてくれる項目もあります。

●方針４
designとperformanceのカテゴリーは、Priority: High (1) の項目だけに留めています。

●方針５
最後の順番で、codestyleのカテゴリーです。
ここに、Priority: High (1) の項目があるものの、自分たちのプロジェクトに合わないと感じたものはコメントアウトしてあります。
※例としては、MyBatisGeneratorの自動生成コードがチェックされて怒られるなど。

★方針の補足
documentationのカテゴリーについては、Priority: High (1) の項目がないものの、必要とあれば後で追加して行きます。


以上までが、
今回のPMD導入にあたっての、
考え方・ポリシー
となります。

カスタム設定XMLの例

最後になりますが、
■カスタム設定XMLの例
という意味合いで、私たちのプロジェクトのカスタム設定XMLを載せておきます。

（※先ほどのポリシー・方針の内容も、
プロジェクトのソース・コードの一部分として引き継いで行けるように、
XMLファイルにそのままコメントとして入れてあります。）

<?xml version="1.0"?>
<ruleset name="customruleset">

    <description>
    static code analysis for XXXX Java source code by PMD
    </description>

    <!-- https://pmd.github.io/ -->
    <!-- https://pmd.github.io/pmd-6.10.0/pmd_rules_java.html -->

    <!--
    以下、基本的な方針を書いておきます。
    まず、各カテゴリーに関して、
    Priority: High (1)
    のものを重視します。
    もし、Priority: High (1) のチェックを無視する場合には、
    明示的にコメントアウトの形で残しておき、なぜ無視しているのかの理由も合わせて書いておきます。
    一方で、Priority: High (1) 以外でも、
    Priority: Medium High (2) Priority: Medium (3) などで、これは有用だと思ったものは加えています。
    次に、以下のカテゴリーを重視します。
    errorprone
    multithreading
    security
    errorproneについては Priority: High (1) の項目に加えて、rulesets/java/basic.xml の errorprone にあるもの全てを加えてあります。
    multithreadingについては、UseConcurrentHashMapを除いて、全項目を加えてあります。
    securityについては、親のsecurity.xmlだけを指定しているので、いつも全項目がチェックされるようにしてあります。（現状では2項目しかありませんが。）
    次に、bestpracticesのカテゴリーを重視します。
    ここには、Priority: High (1) の項目はないものの、良いチェック項目があると思います。
    designとperformanceのカテゴリーは、Priority: High (1) の項目だけに留めています。
    最後の順番で、codestyleのカテゴリーです。
    ここに、Priority: High (1) の項目があるものの、自分たちのプロジェクトに合わないと感じたものはコメントアウトしてあります。
    ※例としては、MyBatisGeneratorの自動生成コードがチェックされて怒られるなど。
    補足：documentationのカテゴリーについては、Priority: High (1) の項目がないものの、必要とあれば後で追加して行きます。
    -->

    <!-- bestpractices ここから -->

    <!-- このカテゴリーは Priority: High (1) の項目がないので気に入ったものを少しづつ追加して行く -->
    <!-- ↓これはMyBatisGeneratorの自動生成コードが怒られる・・・ -->
    <!-- <rule ref="category/java/bestpractices.xml/AbstractClassWithoutAbstractMethod" /> -->
    <rule ref="category/java/bestpractices.xml/AccessorClassGeneration" />
    <rule ref="category/java/bestpractices.xml/AccessorMethodGeneration" />
    <rule ref="category/java/bestpractices.xml/ArrayIsStoredDirectly" />
    <rule ref="category/java/bestpractices.xml/AvoidPrintStackTrace" />
    <rule ref="category/java/bestpractices.xml/AvoidReassigningParameters" />
    <rule ref="category/java/bestpractices.xml/AvoidStringBufferField" />
    <!-- ↓これは使わないと思うが念のため -->
    <rule ref="category/java/bestpractices.xml/CheckResultSet" />
    <rule ref="category/java/bestpractices.xml/ConstantsInInterface" />
    <rule ref="category/java/bestpractices.xml/DefaultLabelNotLastInSwitchStmt" />
    <rule ref="category/java/bestpractices.xml/ForLoopCanBeForeach" />
    <rule ref="category/java/bestpractices.xml/UnusedFormalParameter" />
    <!-- ↓これはやり過ぎかな -->
    <!-- <rule ref="category/java/bestpractices.xml/GuardLogStatement" /> -->
    <!-- ↓これもいいけどちょっとやり過ぎかな -->
    <!-- <rule ref="category/java/bestpractices.xml/LooseCoupling" /> -->
    <rule ref="category/java/bestpractices.xml/MethodReturnsInternalArray" />
    <rule ref="category/java/bestpractices.xml/MissingOverride" />
    <!-- ↓これもいいけどちょっとやり過ぎかな -->
    <!-- <rule ref="category/java/bestpractices.xml/OneDeclarationPerLine" /> -->
    <rule ref="category/java/bestpractices.xml/PositionLiteralsFirstInCaseInsensitiveComparisons" />
    <rule ref="category/java/bestpractices.xml/PositionLiteralsFirstInComparisons" />
    <rule ref="category/java/bestpractices.xml/PreserveStackTrace" />
    <rule ref="category/java/bestpractices.xml/ReplaceEnumerationWithIterator" />
    <rule ref="category/java/bestpractices.xml/ReplaceHashtableWithMap" />
    <rule ref="category/java/bestpractices.xml/ReplaceVectorWithList" />
    <rule ref="category/java/bestpractices.xml/SwitchStmtsShouldHaveDefault" />
    <rule ref="category/java/bestpractices.xml/SystemPrintln" />
    <rule ref="category/java/bestpractices.xml/UnusedFormalParameter" />
    <rule ref="category/java/bestpractices.xml/UnusedImports" />
    <rule ref="category/java/bestpractices.xml/UnusedLocalVariable" />
    <!-- ↓以下の2つはSpringと相性が良くないので -->
    <!-- <rule ref="category/java/bestpractices.xml/UnusedPrivateField" /> -->
    <!-- <rule ref="category/java/bestpractices.xml/UnusedPrivateMethod" /> -->
    <!-- ↓これはMyBatisGeneratorの自動生成コードが怒られる・・・ -->
    <!-- <rule ref="category/java/bestpractices.xml/UseCollectionIsEmpty" /> -->
    <!-- ↓言っていることは分かるが、チェックまでは要らないかな、それぞれの場面で判断すればいいと思う。 -->
    <!-- <rule ref="category/java/bestpractices.xml/UseVarargs" /> -->

    <!-- bestpractices ここまで -->

    <!-- codestyle ここから -->

    <!-- 以下が Priority: High (1) のもの全て -->
    <!-- ↓これは厳し過ぎる！人間によるレビュー・チェックでいいと思う -->
    <!-- <rule ref="category/java/codestyle.xml/ClassNamingConventions" /> -->
    <rule ref="category/java/codestyle.xml/EmptyMethodInAbstractClassShouldBeAbstract" />
    <!-- ↓これも厳し過ぎる！人間によるレビュー・チェックでいいと思う -->
    <!-- <rule ref="category/java/codestyle.xml/FieldNamingConventions" /> -->
    <!-- ↓これも厳し過ぎる！人間によるレビュー・チェックでいいと思う -->
    <!-- <rule ref="category/java/codestyle.xml/FormalParameterNamingConventions" /> -->
    <!-- ↓これも厳し過ぎる！人間によるレビュー・チェックでいいと思う -->
    <!-- <rule ref="category/java/codestyle.xml/LocalVariableNamingConventions" /> -->
    <!-- ↓単体テスト系のメソッド名は全て出力される、逆に言うと出るのはそれだけ、これはtest以外で走らせる？ -->
    <!-- <rule ref="category/java/codestyle.xml/MethodNamingConventions" /> -->
    <!-- ↓これも厳し過ぎる！人間によるレビュー・チェックでいいと思う -->
    <!-- <rule ref="category/java/codestyle.xml/VariableNamingConventions" /> -->

    <!-- codestyle ここまで -->

    <!-- design ここから -->

    <!-- 以下が Priority: High (1) のもの全て -->
    <rule ref="category/java/design.xml/AbstractClassWithoutAnyMethod" />
    <rule ref="category/java/design.xml/AvoidThrowingNullPointerException" />
    <!-- ↓これはどうかな！？MyBatisGeneratorの自動生成コードも怒られるし・・・ -->
    <!-- <rule ref="category/java/design.xml/AvoidThrowingRawExceptionTypes" /> -->
    <!-- ↓なるほど言っていることは分かる、しかしこれはどうかな？ -->
    <!-- <rule ref="category/java/design.xml/ClassWithOnlyPrivateConstructorsShouldBeFinal" /> -->

    <!-- design ここまで -->

    <!-- documentation -->
    <!-- このカテゴリーは Priority: High (1) の項目がないものの必要とあれば後で追加します -->

    <!-- errorprone ここから -->

    <!-- まずは Priority: High (1) のもの全て -->
    <rule ref="category/java/errorprone.xml/ConstructorCallsOverridableMethod" />
    <rule ref="category/java/errorprone.xml/EqualsNull" />
    <rule ref="category/java/errorprone.xml/ReturnEmptyArrayRatherThanNull" />

    <!-- 以下は rulesets/java/basic.xml の中の errorprone カテゴリーにあるものを全て追加しました -->
    <!-- https://github.com/pmd/pmd/blob/master/pmd-java/src/main/resources/rulesets/java/basic.xml -->
    <rule ref="category/java/errorprone.xml/AvoidBranchingStatementAsLastInLoop" />
    <rule ref="category/java/errorprone.xml/AvoidDecimalLiteralsInBigDecimalConstructor" />
    <rule ref="category/java/errorprone.xml/AvoidMultipleUnaryOperators" />
    <rule ref="category/java/errorprone.xml/AvoidUsingOctalValues" />
    <rule ref="category/java/errorprone.xml/BrokenNullCheck" />
    <rule ref="category/java/errorprone.xml/CheckSkipResult" />
    <rule ref="category/java/errorprone.xml/ClassCastExceptionWithToArray" />
    <rule ref="category/java/errorprone.xml/DontUseFloatTypeForLoopIndices" />
    <rule ref="category/java/errorprone.xml/JumbledIncrementer" />
    <rule ref="category/java/errorprone.xml/MisplacedNullCheck" />
    <rule ref="category/java/errorprone.xml/OverrideBothEqualsAndHashcode" />
    <rule ref="category/java/errorprone.xml/ReturnFromFinallyBlock" />
    <rule ref="category/java/errorprone.xml/UnconditionalIfStatement" />

    <!-- errorprone ここまで -->

    <!-- multithreading ここから -->

    <!-- このカテゴリーはほぼ全ての項目を追加するようにします -->
    <rule ref="category/java/multithreading.xml/AvoidSynchronizedAtMethodLevel" />
    <rule ref="category/java/multithreading.xml/AvoidThreadGroup" />
    <rule ref="category/java/multithreading.xml/AvoidUsingVolatile" />
    <rule ref="category/java/multithreading.xml/DoNotUseThreads" />
    <rule ref="category/java/multithreading.xml/DontCallThreadRun" />
    <rule ref="category/java/multithreading.xml/DoubleCheckedLocking" />
    <rule ref="category/java/multithreading.xml/NonThreadSafeSingleton" />
    <rule ref="category/java/multithreading.xml/UnsynchronizedStaticDateFormatter" />
    <!-- クラスのフィールドなどでないローカル・スコープのHashMapまで全てConcurrentHashMapに置き換えろ、というのはやり過ぎのように思う。 -->
    <!-- <rule ref="category/java/multithreading.xml/UseConcurrentHashMap" /> -->
    <rule ref="category/java/multithreading.xml/UseNotifyAllInsteadOfNotify" />

    <!-- multithreading ここまで -->

    <!-- performance ここから -->

    <!-- 以下が Priority: High (1) のもの全て -->
    <rule ref="category/java/performance.xml/AvoidFileStream" />
    <!-- ↓これもやり過ぎかなと思う。 -->
    <!-- この設定を有効にすると、coreのtestのTestValueクラスのshort型のフィールドに警告を出すが、その1個所だけである。 -->
    <!-- <rule ref="category/java/performance.xml/AvoidUsingShortType" /> -->

    <!-- performance ここまで -->

    <!-- security -->
    <!-- このカテゴリーはsecurity.xmlだけを指定していつも全ての項目がチェックされるようにしておきます -->
    <rule ref="category/java/security.xml" />

</ruleset>

このカスタム設定XMLですが、
「これが最高だ！」という意味合いではまったくなく、
私たちのプロジェクトとはぜんぜん違う性質を持ったプロジェクトの場合には、 また違った設定が合っているでしょう。

これからも、このカスタム設定XMLは、時と共に、育てて行こうと思っています。

導入してよかったと思う点

静的解析ツールを導入したからといって、
「だからすぐに品質が上がる」というものではないと思います。

「自分たちのプロジェクトの基準を、自分たちのコード・ベースで管理して、それを次に続く人たちに引き継いで行ける。」

「自分たちのプロジェクトのその基準に合致しているかどうかは、人間の目によるチェックだけではなく、機械のチェックに助けてもらう。」

という部分に価値があるのかなと思っています。