カスタムCopでリファクタリング
RuboCopのカスタムCopを書いてリファクタリングを行う話として、丁度良い事例があったので紹介します。
改善したいコード
仕事先のRailsアプリを眺めてみると、昔から慣習的に次のようなコードが書かれていることが分かりました。
module A
extend ::ActiveSupport::Concern
included do
def foo
end
def bar
end
end
end
本来は、特別な理由が無い限り次のように書かれるべきコードです。
module A
def foo
end
def bar
end
end
これは後から分かったことですが、このようなコードはファイル数で言うと数百件、メソッド定義数で言うと千件弱あるようでした。
用意したカスタムCop
そこで、RuboCopのカスタムCopを書いて、このコードを自動修正することにしました。詳しい書き方についてはここでは深入りしませんが、カスタムCopは、別にGem等にパッケージングしなくとも、適当なファイルを置いて読み込めば使えるし、適当に設定すればテストも書けるので、一般的に想像されるよりも気軽に書けるものです。
完成したものを、後になって雑Cop集であるSevencopに追加して公開したものがこちらです。
さて、このCopは冒頭のコードをおおよそ次のような形に変換します。
module A
extend ::ActiveSupport::Concern
public def foo
end
public def bar
end
included do
end
end
具体的な実装としては、included の中にメソッド定義を見つけるたびに、included の直前にそれを移動するようになっています。直前ではなく直後に入れる実装だと、上から処理していった場合、結果的に定義順序が逆順になってしまうので、直前を選んでいます。また、可視性が変わらないような配慮を手短に実現するために、インラインなスタイルで記載することで元々の可視性を維持するようにしました。
このCopによる自動修正だけでは、冒頭に紹介した理想的なコードと異なり、可視性に関する部分が残ってしまいますが、これは Style/AccessModifierDeclarations というCopで別途適切に自動修正されるので、最終的にはおおよそ理想的なコードになります。Style/AccessModifierDeclarations は最近まで自動修正に対応していませんでしたが、こういう用途等で便利に使えるかと思い、夏頃になんとなく対応しておいたのが功を奏しました。
また、元々メソッド定義しか含まれていなかった included については、上のように変換後に空っぽのまま残ってしまいます。これは別途、空の included を削除するカスタムCopを適当に書いて消し込みました。
自動修正してはいけないパターン
さて、これで全てが万事解決したわけではありません。冒頭で説明した二つのコードは、include してみると明らかに挙動が異なります。
module A
extend ::ActiveSupport::Concern
included do
def foo
end
def bar
end
end
end
class B
include A
end
上のコードの場合、B#foo と B#bar が定義されます。
module A
def foo
end
def bar
end
end
class B
include A
end
一方、上のコードの場合、A#foo と A#bar が定義されます。
そのため、クラス側にメソッドを定義されることを（書いた本人がそれを意図しているかどうかはさておき）利用しているパターンは自動修正せず、利用していないパターンだけを自動修正する必要があります。
この挙動の利用方法には、二つのパターンがあります。
パターン1: クラスに定義されたメソッドを上書きするもの
module A
extend ::ActiveSupport::Concern
included do
def foo
end
end
end
class B
def foo
end
include ::A
end
上のパターンは、既にクラスに定義されているメソッドを上書きする、という使い方です。クラス側に定義されているメソッドについても何らかの方法で動的に定義されている可能性があるので、これを静的検査で検知するのは難しいです。
パターン2: 親モジュールのメソッドをOverrideするもの
module A
extend ::ActiveSupport::Concern
included do
def foo
end
end
end
module B
def foo
end
end
class C
include ::A
include ::B
end
上のパターンは、C < B < A という継承関係であるのに反して、先に include されているはずの A の #foo の定義が優先されて使われてしまう、というものです。include を内部的に呼び出す宣言的なメソッドや、モジュールの側で更に include されている場合など、複雑な実現方法もあるので、これも静的検査で検知するのは難しいです。
無視すべきパターンの検知方法
今回は、実際に実行してみて、つまり動的に検査することで、上記のようなパターンに一致するものが無いか調べることにしました。一致するメソッド定義全てに # rubocop:disable Sevencop/MethodDefinitionInIncluded というコメントを付けてCopを無効化すれば、安全に自動修正できるようになるはずです。
Railsでは、Rails.application.eager_load! を呼び出すと、モデルやコントローラーなどの定義が一通り読み込まれます。これを活用して、上述のパターンに一致するものがあればその箇所を報告してもらうことにしましょう。
メソッド上書きの検知
1つ目の、既にクラスに定義されているメソッドを上書きするパターンについて。これは、included の中でいまからメソッドを定義する際に、自身にそのインスタンスメソッドが既に定義されていないかを調べれば判断できます。Rubyには Module#method_added という便利なフックがありますが、これはメソッドが定義された後に呼び出されるので、今回は使えません。そこで今回は、前述のカスタムCopを少し編集して、事前にコードを次のような形に変換してみることにしました。
module A
extend ::ActiveSupport::Concern
included do
method_will_be_defined_in_included(:foo)
def foo
end
end
end
method_will_be_defined_in_included を適当に定義して、Rails.application.eager_load! を呼び出せば、このパターンに一致するメソッド定義を洗い出せます。
class Module
# @param method_name [Symbol]
def method_will_be_defined_in_included(method_name)
if method_defined?(method_name, false) || private_method_define?(method_name, false)
# ここで報告したりする
# (今回は `caller` を見て呼び出し元のソースコードにコメントを挿入する自己破壊的なコードにしました)
end
end
end
Rails.application.eager_load!
親モジュールに対するOverrideの検知
次に、included によって後に include されるモジュールよりも優先されるメソッドが定義されるパターンの検知方法について。これは、後に定義されるメソッドに影響を及ぼしているか検知したいので、そのメソッドが定義されたタイミングではまだ検知できません。そこで、すべての定義が終わった後で、結果的にそういう構造になったメソッド定義を探し出すことにしました。
Rails.application.eager_load!
# この慣習で `included` が使われているのはアプリ内のモデルとコントローラーだけなので、
# これらのすべての子孫クラスから探索する
result = [
ApplicationController,
ApplicationRecord
].flat_map(&:descendants).flat_map do |klass|
# 1. そのクラスから、`included` 内の `def` で定義されたメソッドを探索する
methods_maybe_defined_in_included = klass.instance_methods(false).map do |method_name|
klass.instance_method(method_name)
end.select do |method|
path = method.source_location[0]
File.read(path).include?('included do')
end
# 2. そのクラスの継承ツリーから、モジュール上に定義されたメソッドを探索する
methods_defined_in_module = klass.ancestors[1..].grep_v(Class).flat_map do |mod|
mod.instance_methods(false).map do |method_name|
mod.instance_method(method_name)
end
end
# 1と2から得られた二つの集合を積算すると、ほしいメソッドの集合を得られる
method_names = methods_maybe_defined_in_included.map(&:name) & methods_defined_in_module.map(&:name).uniq
method_names.map do |method_name|
"#{klass}##{method_name}"
end
end
puts result.sort
上記のコードでこのパターンに一致するメソッド定義を洗い出せるので、同様に該当箇所に # rubocop:disable ... なコメントを挿入し、Copを無効化します。
結果
これでCopを無効化すべき箇所を無効化できたので、改めてRuboCopで自動修正すれば、リファクタリング完了です。実際には +62,932 −62,469 という差分になりました。
おわり
以上です。RuboCopのカスタムCopを書いてリファクタリングを行う話として、丁度良い事例があったので紹介しました。