RuboCop RSpecに新しいルールを作った話

こんにちは、プロダクトエンジニアのkitazawaです。

みなさんRuboCop使っていますか？静的コード解析でさまざまなチェックをしてくれて便利ですよね。私が担当しているプロダクトでももちろん利用しており、とても活躍しています。

そんな便利なRuboCopですが、既存のルールだけではなく自分でルールを作ることもできます。今回はルールを自作した話と、その作ったルールrubocop-rspecに取り込んでもらった話を紹介します。

間違ったテストコードの発見

ある日、コードを修正しているときにRSpecで以下のようなテストが書いてあることに気付きました。

it { expect(do_something).kind_of? Foo }

このテストはうまく動いていなかったのですが、理由がわかるでしょうか？

すぐにわかった方は流石です！実は .to be_kind_of であるべきところが .kind_of? になっています。これではテストは実行されず、 kind_of? が呼び出されて false を返すだけのコードになっていました。私は最初見たときには気付かず、失敗すると思っていたテストが成功してやっと気付きました。よく間違えそうなコードに思えたので既存のルールがないか探してみました。

RuboCopには標準のルール以外にextensionという、外部ライブラリに対応したルールを持つ拡張ライブラリがあります。そして、RSpecに対応したextensionとして、rubocop-rspecがあります。ここにルールがないか調べてみました。しかし見つかりません。社内のメンバーにも聞いてみましたが誰も知らず、どうやら既存のルールは存在しないようでした。

ところで、このコードはなんだか機械的に判断できそうに見えませんか？ルールがないなら自分で作ってしまえば良さそうです！

ちなみにRuboCopのルールはCopと呼ばれているため、以降はルールをCopと表現します。

Copの自作方法

実はRuboCopのCopを自作するのは意外と簡単で、以下のような決まったインターフェースを持つクラスを作成するだけです。

RuboCop::Cop::Base を継承する
MSG 定数でエラーメッセージを定義する
on_*** コールバックをメソッド定義し、ルールに違反していたら add_offense を呼び出す

これらの条件を満たすサンプルコードを書いてみます。例えば lib/custom_cops というディレクトリに sugoi_cop.rb というファイルを以下のように作成します。コールバックのメソッドはいくつかありますが、ここでは on_send を定義します。

module CustomCops
  class SugoiCop < Rubocop::Cop::Base
    MSG = "違反時のメッセージ"

    def on_send(node)
      return unless check_something

      add_offense(node)
    end
  end
end

そして .rubocop.yml で require します。

require:
  - ./lib/custom_cops/sugoi_cop

たったこれだけです！肝心のチェックをどう書くかですが、詳しいことはRuboCopのドキュメントを読んでみてください。

間違ったテストコードを検知するCopの作成

さて、ここまででざっくりCopの作り方がわかったので、今回の問題になったコードについてのCopを考えてみたいと思います。

RSpecの場合テストを実行するには expect, is_expected の後に .to, .not_to, .to_not メソッドを呼び出し、引数にマッチャーを指定します。

expect(foo).to eq 42

このような形式です。そのため、 .to, .not_to, .to_not を呼び出していないコードをチェックすればよさそうです。上記のコードをパースすると、次のようなASTになっています。このパース結果が重要なので覚えておきます。

$ ruby-parse -e 'expect(foo).to eq 42'
(send
  (send nil :expect
    (send nil :foo)) :to
  (send nil :eq
    (int 42)))

on_send コールバックは多く呼ばれるため、定数 RESTRICT_ON_SEND で使用するメソッド名を制限することで最適化できます。 def_node_matcher はRubyをパースした結果のNodePatternとマッチするか確認するメソッドを定義できます。ここで先程パースした結果を元に、違反しているコードを見つけるためのパターンを定義します。 NodePatternの詳細はrubocop-astのドキュメントを参照してください。

これらを組み合わせると以下のようなコードができあがります。

RESTRICT_ON_SEND = [:expect].freeze

# @!method expectation_without_runner?(node)
def_node_matcher :expectation_without_runner?, <<~PATTERN
  (send (send nil? :expect ...) !{:to :not_to :to_not} ...)
PATTERN

def on_send(node)
  expectation_without_runner?(node.parent) do
    add_offense(node.parent.loc.selector)
  end
end

この他にも is_expected を使った場合や、ブロックを渡したパターンを考慮する必要がありました。自作したCopを早速プロダクトで実行してみると、いくつか問題を見つけることができました。また、RuboCopはCIで実行しているため、今後同じ間違いをしなくなる点もメリットです。

rubocop-rspecへのコントリビュート

さて、私のプロダクトではチェックできるようになりましたが、今回のCopは特にプロダクト固有の事情はないので他のプロダクトでも利用できそうです。さらに言うと、全RSpec利用者にとって価値がありそうです！つまり rubocop-rspec に追加できるとよさそうに思いました。

ということで出したPRが以下になります。
github.com

大量のレビューをもらっていて少し恥ずかしいですが、無事マージしてもらえました。レビューを受けて感じたのは、変更内容が必要最小限であることの大切さでした。日頃気を付けているつもりではありますが、変更内容が多いとレビューの負担が上がってしまいます。また、コードを読んだ時の認知負荷も高くなります。これはOSSかどうかに限らず、日々の開発の中でも意識していけると良いと思いました。

ちなみに、マージされたのがちょうどメジャーバージョンアップであるrubocop-rspec 3.0を出すタイミングだったため、デフォルト有効な状態で入っています。