RubocopでRubyコードのスタイルを強制せず、バグだけ検出

今回はRubyのRubocopの使い方のお話です。

Summary

RubocopのLayout, Lint, Metrics, Styleらのcopのほとんどすべてをdisable
潜在的バグ発見に役立つ系のSecurity, Lintのcopの一部をenable
rubocop:disable や .rubocop_todo.yml だけゆるく禁止

というルールで運用で一定の成功を上げているという報告です。

このルールは、Quipper (Tokyo) 社内のRubyアプリケーションのうち、複数のチームにまたがってメンテナンスされている、いわゆる境界的な位置に属するものだけを対象にしています。新しく書かれるコードの多くは個別チームがownershipをもってメンテナンスをするため、そちらではチームの合意で自由にRubocopの設定を導入したりしなかったりしますが、特定チームに寵愛を受けていないいわゆる孤児なコードベースには、一括してこの特殊ルールを適用しています。(コードサイズ的にはむしろいまだに孤児の方がずっとでかいはずです)

Background

Quipperにはもともと社内で作って一貫してみんなで使ってみんなで育てる、みんなに愛されていたrubocop configがありました。いわゆる大統一rubocopです。

https://rubygems.org/gems/quipper-rubocop-config/

ここでは最初からenabledなcopはだいたいenabledで、ちょくちょくスタイルの指定がありました。そのあたりを変更するときはチームで議論して、たまにちょっともめつつ、合意してどれかに決めて採用してenforceしていく、みたいな感じでした。

しかし月日は流れ、開発のメンバーも徐々に入れ替わっていくうちに、quipper-rubocop-configを積極的にメンテナンスする人は減っていき、手元には膨れ上がったrubocop_todo.ymlやrubocop:disableがありました。不定期に一括でrubocopのルールをapplyさせたりはしつつも。

意思決定

一年ちょっと前、この大統一rubocopが宙ぶらりんになっているのに気づいた mtsmfmさんが問題提起してくれました。

主張としては、quipper-rubocop-configをやめたいというより、誰もメンテしていないのに導入されたままになっている状況をやめたい、というものです。というわけで名乗り出ました

※ 当時rubocopに対して良い想いが全然なかったので↑のような治安の悪い書き方をしていますが、その後触っていくうちに、こいつかわいいな、やるじゃん、と思い、今ではrubocop自体は手放しで絶賛しています。rubocop最高。

以下のルールを設定してみました。正確にはルールというか、誰でも破ることができるけど、破るのがちょっと面倒なのでわざわざ破ろうとしないし、破る必要もあんまりない、みたいな。

.rubocop_todo.ymlに加筆しない。というかこのファイル自体をそもそも消したので、やりたかったら新規作成する必要がある
rubocopのviolationがあるとCIが通らない (のでpull requestのmergeができない)
rubocop:disabledコメントを書かない。これはあってもmergeできるけど、ujihisaが不定期にコード検索して見つけたらpull requestを投げる
- 実際には現状そもそもこれの必要があったことがなく、一度もrubocop:disabledコメントが増えなかった

ちなみに、今回↑で僕が挙げた方針以外に、以下のような方針をとる選択肢もありました。

testdouble/standard (非標準) の使用
Rubucopのデフォルトの使用
そもそもRubocopを使わない

しかし、いずれの方針にせよ、「持続的にそれを積極的に推進し、周囲を説得し、先導する人」が必要で、特にこうしたいという強い情熱を持っている人は少なくともその時点ではいませんでした。

今回挙げた前述の手法は、僕自身が求めている形であるのと同時に、他の人から反対もなく自然に導入できた感じです。まあおいしいとこどりをしている形なので必然的に反論が出にくい的なあれだったりします。

現状

もともとquipper-rubocop-configという大統一rubocop configがあったのとは別で管理したかったので、別の名前のライブラリにしました。名前はquipper-rubocop-config-for-monolithです。みんなから共有されているモノリスなコードベースに対してのものなので直接的です。

中の構成は、 rubocop --show-copsによると

Available cops (567)
Enabled: true: 39個
つまり6.9%使用

みたいな形です (2021-07現在)。内訳としては、Security系はすべてenable, Lint系は Lint/DuplicateCaseCondition や Lint/UselessMethodDefinition や Lint/IneffectiveAccessModifier などの明確な潜在的バグのみをピンポイントでenableしています。詳しくはこの記事の後半で全文公開しているので参照ください。

他にも良い潜在的バグ発見系copがあればどんどん導入していく予定です。ただしちょっとでもノイズがあるようなら導入しない、みたいな感じです。

このルールにするより前は、こんな感じでした

Available cops (446) <- 結構昔のrubocop versionだったため
Enabled: true: 405個
つまり 90.8%使用

効果については、正直、静的にparseしてわかる程度の自明すぎる潜在的バグ検出系なんてそんなにないだろう、と思いきや、意外にちょくちょく見つかったりしました。歴史的にいろんな人が編集してきた、いわゆる古くからある複雑なファイルほどそういうのが多い印象です。お互いに他所の土地と認識していじっているような感じの。

また、rubocopのcopがうるさくないかなど、意見をいろんな人に聞いてみたところ

ここ一年何も言われなくなったので存在を忘れていた

などの絶賛の声をいただきました。まさにこのような、空気のような存在を目指していました。うるさくないのにバグだけ見つけてくれるみたいな (まあ検出率は低そうだけど...)

おまけ1

以下quipper-rubocop-config-for-monolithの config/rubocop.yml 全文です。今後どんどんアップデートしていく予定ですが、この記事はあくまで2021-07時点でのスナップショットです。

require:
  - rubocop-rails

Bundler:
  Enabled: true

Bundler/OrderedGems:
  Enabled: false

Gemspec:
  Enabled: true

Gemspec/OrderedDependencies:
  Enabled: false

Gemspec/RequiredRubyVersion:
  Enabled: false

Layout:
  Enabled: false

Lint:
  Enabled: false

Lint/AmbiguousAssignment:
  Enabled: true
Lint/BooleanSymbol:
  Enabled: true
Lint/CircularArgumentReference:
  Enabled: true
Lint/DuplicateCaseCondition:
  Enabled: true
Lint/DuplicateElsifCondition:
  Enabled: true
Lint/DuplicateHashKey:
  Enabled: true
Lint/DuplicateMethods:
  Enabled: true
Lint/DuplicateRegexpCharacterClassElement:
  Enabled: true
Lint/DuplicateRequire:
  Enabled: true
Lint/DuplicateRescueException:
  Enabled: true
Lint/EmptyExpression:
  Enabled: true
Lint/EmptyInterpolation:
  Enabled: true
Lint/FormatParameterMismatch:
  Enabled: true
Lint/FloatOutOfRange:
  Enabled: true
Lint/SafeNavigationChain:
  Enabled: true
Lint/IneffectiveAccessModifier:
  Enabled: true
Lint/MissingCopEnableDirective:
  Enabled: true
Lint/NestedMethodDefinition:
  Enabled: true
Lint/NumberedParameterAssignment:
  Enabled: true
Lint/OrAssignmentToConstant:
  Enabled: true
Lint/PercentSymbolArray:
  Enabled: true
Lint/StructNewOverride:
  Enabled: true
Lint/TripleQuotes:
  Enabled: true
Lint/UselessElseWithoutRescue:
  Enabled: true
Lint/UselessMethodDefinition:
  Enabled: true
Lint/UselessSetterCall:
  Enabled: true
Lint/Void:
  Enabled: true

Metrics:
  Enabled: false

Migration:
  Enabled: true

Naming:
  Enabled: false

Security:
  Enabled: true

Style:
  Enabled: false

Rails:
  Enabled: false

Rails/BulkChangeTable:
  Enabled: true

いまのところそもそも社内プロジェクトの一部向けなので特に公開することの第三者へのメリットがなさそうなのでfreesoftwareとして公開していませんが、デメリットもないので公開しちゃっても良い気がします。まあ上に書いたのが全部なのですでに公開しているといえばしていますね。

おまけ2

最後にRubyの書き方のいわゆるスタイルについて統一するかしないか的な議論への主観も軽く述べます。以下はチームや社内全体で同意してるものではなく筆者ujihisaの主観もりもりで、あんまり客観的なものではないです。

Rubyは読み手のために書き手の意図をコードに残すことができる言語で、スタイルを統一することの一部がそれをたまに損なう
文字幅がここになったら強制的に改行するなどのルールはコードに無駄にdiffを多く発生させ、gitなどの履歴が読みにくくなるし、なによりも不必要に他人の修正とconflictしがち
ロジックの修正と見た目の修正を分離したくても、rubocopのviolationをCIで防いでいたら、両者の混在が要求され、GitHub Pull Requestならびにdeployの粒度が不必要に大きくなる
かといってCIせずに放置してもokになるとどんどんviolationが増えてrubocopがあることがノイズでしかならなくなる
あるいはrubocop_todo.ymlやrubocop:disableを単に増やすだけになり、それが消化されることはない
要するにRubyにおけるソフトウェア開発をcomplectする
というわけで本記事におけるスタイル「RubocopでRubyコードのスタイルを強制せず、バグだけ検出」は、孤児コードベースでなく、ふつうに任意のコードベースで便利なんじゃないかなあ
- Layoutとか一切強要しなくても、みんなコード書くとき、既存のコードが2タブならわざわざそこに3タブでパッチ投げたりしないので