ハッピーな開発ライフを過ごすために静的解析ツールを本格導入した

case.1 サーバサイド編

自己紹介

アイキューブドシステムズでCLOMOのサーバーサイドを担当している Kakuno です。

生まれも育ちも東京で、約９年間ほど東京でエンターテイメント系の企業でサーバーサイドを担当してきました。福岡に移住したいという夢があり、2019年12月から福岡に本拠地があるアイキューブドシステムズへご縁があり入社しました。

コーディング規約

弊社のサーバーサイド開発ではRuby on Railsを主に使用しています。

さて、入社して最初にコードを書くときに気になることといえば、「コーディング規約」はどうなっているのか、だと思います。

Ruby on Rails自体には、インデントにタブではなくスペース2個を使用する等の基本的なルールはありますが、それに加えて「うちの会社ではこうしよう！」みたいな独自ルールがどの会社にも存在するかと思います。

弊社もコーディング規約は2016年に定義されていました。

しかしながら、私が入社した頃はコーディング規約に沿っているかどうかのチェックを、プルリクエストでのレビュー時に人力で行っており、レビュワーによって指摘内容が異なる、指摘を見落としてしまう、という事が起こっていました。そこで、コーディング規約が正しく守られているか自動的にチェック出来ないのか？ということが課題の一つになっていました。

RuboCopの導入

その解決に選んだのが静的解析ツールです。Ruby on RailsではRuboCopが静的解析の一つとして有名なツールになります。

github.com

RuboCopの導入はとてもシンプルです。プロダクトのGemfileに gem 'rubocop' を追加するだけです。あとは、プロダクトのルートディレクトリに.rubocop.ymlというファイルを用意して、そこにルール、すなわちコーディング規約を書いていくだけです。

Rails:
  Enabled: true
AllCops:
  TargetRubyVersion: 2.6
Include:
    - ‘**/**.rb'
    
...このあとにルールを追加していく...

RuboCopによる指摘

例えば、引数valueが0より大きい場合元の数を返す、0以下であれば２倍の数を返すといったcalcがあったとします。こちらのコードを下記のように書いた場合、RuboCopでは下記のように指摘してくれます。

[修正前]

def calc(value)
  if value > 0
    return value
  end
  return value*2
end

[RuboCop指摘]

def calc(value)
  if value > 0 # ←0以上と0を超えるで間違いやすいので positive?を使う
    return value
  end # ←ガード節を使ってネストを浅くする
  return value*2 # ←無駄なreturnを消す / 演算子の間はスペースをあけて読みやすく
end

RuboCopはこのように見やすい形にする指摘をしてくれたり、間違いやすいコードに対する指摘をしてくれます。これを修正すると下記のようなコードになります。

[修正後]

def calc(value)
  return value if value.positive?

  value * 2
end

RuboCopの指摘を修正したことによりシンプルになり、可読性もあがりました。

エディタでの使用

RuboCopでは「エディタ」での指摘が可能です。 例として、先程の無駄なreturnの指摘がある場合の挙動を見ていきましょう。 エディタでは下記のように無駄なreturnがあるよ(Redundant 'return' keyword)という指摘コメントがされています。 ※ Visual Studio Code、Atom、RubyMine、Sublime Text等ほとんどのIDEで導入が出来ます。

Bitbucketとの連携

ローカルのエディタでRuboCopの指摘に気づくことはできますが、それだけではRuboCopの指摘を直さないままgit pushしてしまっても他の人が気づくことができません。そこで、git pushされた際にも自動でRuboCopを実行するようにCIを設定をします。

弊社では、つい最近までBitbucketを使ってソース管理をしていました。Bitbucketでは標準でBitbucket PipelinesというCIを利用することができ、pull_requestトリガーでRuboCopによる静的解析をかけていました。

指摘範囲を修正した場所のみに限定する

RuboCopはファイル単位で静的解析を実行するため、例えば1000行あるソースコードのうち3行しか触っていなくても、残りの997行に違反コードが含まれると指摘されてしまいます。

「なるべく違反コードの多いソースコードは触らないようにしよう」という変な発想になってしまうのを防ぐためにも、指摘対象は変更を加えた部分に限定したいです。

RuboCopの指摘対象をフィルタリングするためのOSSとしては

github.com

github.com

この2つが有名です。Bitbucketへのインラインコメントが付けられる（とREADMEに書いてある）prontoを採用しました。

BitbucketのPull RequestにインラインコメントでRubocopの指摘がされるようにする

RuboCopのCIチェック導入当初は、指摘をインラインコメントするのではなく、指摘をビルドエラー扱いにしてCIを落として、CIのログから指摘箇所を確認して直してもらうフローとしていました。

今から思えば、いちいちPull Request作成者がBitbucket Pipelinesの実行ログを見に行かないといけないなんて、明らかに面倒なフローです。

しかし、当時はprontoが壊れていて、READMEのとおりに設定をしてもBitbucketへのインラインコメント機能がエラーになって動きませんでした。

Comment for Bitbucket stopped working · Issue #346 · prontolabs/pronto · GitHub

その後、prontoのソースをよくよく見ると、実はこのバグはmasterブランチではすでに修正されていて、下記のようにGemfileを修正して formatterにbitbucket_prを指定することでインラインコメントが付くようになりました。(ちなみに、この記事を書いている2020/8/6現在も最新版の0.10.0には修正が取り込まれていません。masterブランチを利用する必要があります。)

• Gemfileの書き方を下記のようにする

gem 'pronto', github: 'prontolabs/pronto'

• bitbucket pipelineの書き方を下記のようにする

- bundle exec pronto run [:ProjectDir] --formatters bitbucket_pr --commit origin/$BITBUCKET_PR_DESTINATION_BRANCH --runner rubocop

この方法でインラインコメントがつくようになりました。

GitHubとの連携

現在、弊社ではGitHubを使用しています。

Bitbucketでは苦労したRuboCopのCI構築が、GitHubではreviewdogのGitHub Actionの設定がマーケットプレイスで公開されており、一瞬でCIを構築することができます。 https://github.com/marketplace/actions/run-eslint-with-reviewdog

設定は以下のようにYAMLを書くだけで完了です。

name: reviewdog
on: [pull_request]
jobs:
  rubocop:
    name: runner / rubocop
    runs-on: ubuntu-latest
    steps:
      - name: Check out code
        uses: actions/checkout@v2
      - name: rubocop (Panel)
        uses: reviewdog/action-rubocop@v1
        with:
          github_token: ${{ secrets.github_token }}
          reporter: github-pr-review
          rubocop_version: 0.86.0
          rubocop_flags: --config Panel/.rubocop.yml Panel/

これだけで、pull_requestをトリガーとしてRuboCopからの指摘があれば該当のソースにコメントをしてくれます。prontoと同様、静的解析の対象はPull Requestで修正したソースコードのみです。

RuboCopの適用ルールについて

RuboCopには便利なルールセットが多数定義されていますが、デフォルトで有効なルールが厳しすぎたり、社内のコーディング規約とはマッチしない部分があったりします。

そのため、導入に際してはデフォルトを無効に( DisabledByDefault: true を指定)して、必要なルールを追加していくようにしました。

いきなりすべてのソースコードに対してRuboCopを適用しようとすると、リファクタリングが大変になってしまいますが、前述の通りPull Requestで修正したソースコードのみを対象としたことで、多少きつめのルール設定でもスムーズな導入が出来ています。

既存のコーディング規約に加え、役に立ちそうなものは今後も積極的に採用していきたいと考えています。

まとめ

弊社では、コーディング規約にもあるように、主に下記のような目的のためにRuboCopを導入しています。

• 問題を内包しやすいコードや、可読性を下げるコードが生産されることを防ぐ
• 些細な記述方法の不統一による無用なコードレビュー指摘/論争を減らす
• スタイルガイドを元にこれまで蓄積されてきたグッド/バッドノウハウを学ぶ

しかしながら、RuboCopに指摘されたから直す≒なぜこう直すのか理解しないということが起こりえます。RuboCopは全知全能の神ではなく、なぜこういうコーディング規約になっているのかを理解し、必要であれば議論をし、時には既存のルールに変更を加えることも必要です。弊社でも隔週でRuboCopにアップデートをチェックし、ルールについて議論をしています。メンバーみんなでRuboCopを育てていくことがとても大事です。

case.2 モバイルアプリ編

自己紹介

2019年11月に入社したiida-i3です。SECURED APPsのXamarin開発を主に担当しています。

以前は創業約40年の独立系中小SIer(東京)で働いていましたが、福岡に移住してきました。出身は奈良県なのですが、妻の地元であることがきっかけです。これも何かの縁かもしれませんね。

StyleCop.Analyzersの導入

モバイルアプリ編では、Xamarinで作られたプロダクトにStyleCop.Analyzersという静的解析ツールを導入した話をします。

ツール導入の背景

弊社のXamarinアプリの開発現場では、保守対象のアプリが多いため「今まで担当してきたメンバーが今後も担当できるとは限らないという」状況がよく発生しています。

人によって書き方がバラバラだと品質を保つのが難しく、「コードの品質は保証したいが、手数をかけずに継続的に検査したい」ということを考えるようになりました。そこで、まずは静的解析ツールを適用するのが妥当だろうという流れで、ツールの導入をすることになりました。

StyleCop.Analyzersとは

github.com

C#の静的解析ツールで、簡単に言うと、さまざまなルールに対して違反コードを書いていた時、設定に応じて警告やエラーを出してくれるツールです。

StyleCop.Analyzersの適用例

実例を2つほど挙げます。

• SA1008:開き括弧の空白文字不正

• SA1027:タブ/空白文字の使われ方が設定と一致しない

※SA1008,SA1027といった文言は、警告コードです。先頭に「SA」と付くものが、StyleCop.Analyzersによって出されている警告です。

コードレビューで指摘するのも面倒なレベルなので、開発者がビルドした時点で警告を出してくれるのはありがたいものです。SA1027の例のように、注意深く見ないと見分けが付かないものを指摘してくれるのは特に素晴らしいですね。

プロジェクトに適用

今回は、まずXamarinで作られたiOSアプリ3つに対して適用しました。

例として挙げたような警告が、3アプリで合計約20000件存在していました。ちょっと気が遠くなる分量ですね。…ということで、適用プロジェクトとして外部の力をお借りして対応を進めました。

具体的には以下の通りです。

基本方針をお伝えした上で警告対応作業自体は外部の会社様に依頼
　→対応内容をピックアップして確認、および動作確認
　　→検証チームによる最低限のテスト実施

概ね計画通り、約1ヶ月強で完了しました。概ね問題なかったのですが、いくつかうまくいかなかったこともありました。参考として共有いたします。

うまくいかなかったこと

対応内容をどうチェックするか

ビルドして警告が消えていれば何らかの対応を行ったことは分かるのですが、どのような対応を行ったかについては教えてくれません。以下について地道に確認しました。愚直ですね。

• 修正前のコードに対してVisualStudioが提案する内容
• 実際の修正内容

全件確認していたらキリがないので、警告コード1つにつき1度ずつとしました。それでも今数えたらちょうど100件ありました。最早多いのか少ないのかよく分からないですね。勉強になりました。

不要な差分の混入

「いうて提示されるがままに警告直すだけやし、問題なんてそんな起こらへんやろ」と油断していました。実際には、設定ファイルをテスト用に書き換えたものがコミットされたりしたことはありました。チェック大事ですね。

そもそもアプリの動かし方が分からない、からのクラッシュ

3アプリ中、自分が開発に携わってきたのは1つだけでした。他2つについては、嵌ったら有識者にTeams上で相談しつつ進めました。製品固有の知識が無いと、なぜかさっぱり動かないといった事態に遭遇したことがある方も多いかと想像します。嵌ったのはそのような類のものです。

盛大に嵌った箇所については全社的に利用しているConfluenceに書き留めたので、今後入ってくる方は嵌らないはず、です。情報共有大事ですね。

そんなこんなで動かし方を理解しまして「よっしゃ軽く動かして検証チームに引き渡して問題なしで終わりじゃーい…だったらいいな」と少し油断していました。実際には、軽く動かすと(内部でクラッシュして)軽く固まったりしました。チェック大事ですね。

適用例に記載したような、VisualStudioから修正方法が提示されるものばかりではなかったのです。連絡し、素早く対応していただけました。20000件も対応したら、少しぐらいは失敗することもあるでしょう。ミスは起こりうるものと心構えしておくことが大事ですね。

クラッシュと言えば、弊社のXamarinアプリ開発においてはAppCenterを利用し、設定に応じてクラッシュログを収集するようにしています。もしクラッシュした場合、スムーズにログを入手することができます。原因調査時に便利ですね。

また、クラッシュに限らず課題管理ツールとしてJIRAを利用しています。特に変わった使い方をしている訳ではないですが、チームを跨ぐと特に忘れ去ってしまいがちなので、課題管理は大切ですね。

今後

今後も品質向上とレビュー効率向上のために静的解析を活用していく所存です。

現状は、上記で紹介したようにVisualStudio上で警告を出すに留まっているので、「CI/CDと連携したい」などの課題もまだまだあります。

また、実際にコードを書いていると「このルール、要る？」「それはごもっともだけど、俺の考えは違った」という風な疑問や意見も挙がってくるはずです。

今のルールを絶対とせずに適宜見直し、改善しつつ進めていきたいと考えています。