はじめに
こんにちは。スマサテで開発を担当しているキノシタです。
今回は、RuboCopの自動修正機能を使ってコード品質を改善しようとした際に、2度のデグレードを起こしてしまった経験を共有します。静的解析ツールは非常に便利ですが、機械的に適用すると思わぬ落とし穴があることを痛感しました。
この記事が、同じような問題に直面する方の参考になれば幸いです。
RuboCopとは
RuboCopは、Rubyの静的コード解析ツールです。コードの品質、スタイル、バグの可能性を自動的にチェックし、多くの問題を自動修正できます。Rails開発では事実上の標準ツールとして広く使われています。
自動修正の2つのモード
RuboCopには2種類の自動修正モードがあります：
rubocop -a (安全な自動修正)
Safe: true のルールのみを修正します。インデント、スペース、命名規則など、動作を変えない修正が対象です。
# 修正前
def bad_method_name
x=1+2
end
# 修正後
def bad_method_name
x = 1 + 2
end
基本的にはこちらを使うことが推奨されます。
rubocop -A (安全でない自動修正)
Safe: false のルールも含めて修正します。動作を変える可能性があるため、実行後は必ず動作確認が必要です。
# 修正前
users.each do |user|
user.update(status: 'active')
end
# 修正後
users.find_each do |user| # ← 動作が変わる可能性
user.update(status: 'active')
end
なぜ -A を使ったのか
スマサテでは、.rubocop_todo.yml に蓄積された技術的負債を減らす取り組みを進めていました。大量の指摘を効率的に修正するため、rubocop -A を使用することにしました。
しかし、動作確認が不十分だったため、2つのデグレードを起こしてしまいました。
デグレ事例①: flash vs flash.now
問題の発見
あるレポート機能で、ポップアップメッセージが表示されないという不具合が報告されました。
原因となったコード
# デグレ版（RuboCopが自動修正した後）
def add_popup(message)
flash.now[:popup_detail] = message # ← ここが問題
end
# 正常版（元のコード）
def add_popup(message)
flash[:popup_detail] = message
end
何が起きたのか
このメソッドは、ensure ブロック内で redirect_to と一緒に呼ばれていました：
def some_action
begin
# 何か処理
dangerous_operation
rescue => e
# エラー処理
logger.error(e)
ensure
# 成功・失敗に関わらず実行される
add_popup("処理が完了しました")
redirect_to root_path # ← リダイレクトしている
end
end
flash と flash.now の違い
-
flash: 次のリクエストで表示される（リダイレクト先で表示される） -
flash.now: 現在のリクエストのrenderでのみ表示される
今回のケースでは redirect_to しているため、flash.now で設定したメッセージは次のリクエストに引き継がれず、消えてしまいました。
RuboCopの指摘内容
RuboCopのルール Rails/ActionControllerFlashBeforeRender は、「render の前なら flash.now を使うべき」と推奨します。この指摘自体は正しいのですが、今回のケースには適用できませんでした。
なぜ誤検知が起きたのか
RuboCopは静的解析ツールなので、以下のような限界があります：
- ✅ 「このメソッド内に
renderがある」は検知できる - ❌ 「
ensureブロック内で実際にredirect_toが実行される」は検知できない
実行時の制御フローまでは追跡できないため、ensure ブロック内の redirect_to を考慮せず、機械的に flash.now に変更してしまいました。
対応方法
# .rubocop_todo.yml
Rails/ActionControllerFlashBeforeRender:
Exclude:
- 'app/controllers/owned_room_excels_controller.rb'
このファイルだけルールを除外し、元のコードに戻しました。
デグレ事例②: each vs find_each
問題の発見
あるレポートのプレビュー機能で、エラーが発生するようになりました。
原因となったコード
# デグレ版（RuboCopが自動修正した後）
MasterHintItem
.where(type: item.type)
.find_each do |hint_master| # ← ここが問題
# 処理
end
# 正常版（元のコード）
MasterHintItem
.where(type: item.type)
.each do |hint_master|
# 処理
end
何が起きたのか
find_each とは
find_each は、ActiveRecordの大量データ処理用メソッドです：
- 内部で自動的にバッチ処理（デフォルト1000件ずつ）
- メモリ効率が良い
- ただし、ActiveRecord::Relationにのみ使える
なぜエラーになったのか
MasterHintItem クラスは、ActiveRecordではなく ActiveYaml を継承していました：
class MasterHintItem < ActiveYaml::Base
include ActiveHash::Enum
set_root_path "config/active_hash"
set_filename 'master_hint_item'
end
ActiveYamlは、YAMLファイルからデータを読み込んでActiveRecordライクなインターフェースを提供するgemですが、内部的にはActiveRecordではありません。
そのため：
-
whereメソッドは使えるが、返されるのはActiveYaml::Relationまたは配列 -
find_eachはActiveRecord専用のメソッドなので存在しない - 結果として
NoMethodErrorが発生
ActiveYamlのようなActiveRecord風のライブラリは、すべてのActiveRecordメソッドを実装しているわけではないため、find_each のようなバッチ処理用メソッドは使えません。このケースでは、通常の each を使う必要がありました。
RuboCopの指摘内容
RuboCopのルール Rails/FindEach は、「each より find_each を使うべき」と推奨します。
ルールの意図:
- 大量データでメモリを節約
- パフォーマンス向上
しかし今回は:
- そもそもActiveRecordではないオブジェクト
なぜ誤検知が起きたのか
RuboCopは静的解析ツールなので、以下のような限界があります：
- ✅ 「
whereメソッドが呼ばれている」は検知できる - ❌ 「実行時に返されるオブジェクトの実際の型」は検知できない
型情報を完全に把握できないため、機械的に find_each に変更してしまいました。
対応方法
# .rubocop_todo.yml
Rails/FindEach:
Exclude:
- 'app/controllers/assessments/reports/full_occupancy_controller.rb'
このファイルだけルールを除外し、元のコードに戻しました。
2つのデグレの共通点
| 項目 | flash.nowの件 | find_eachの件 |
|---|---|---|
| 問題の本質 | 実行時の制御フローを理解できない | オブジェクトの実際の型を理解できない |
| RuboCopのルール | 一般論として正しい | 一般論として正しい |
| 今回の適用結果 | このコードには不適切 | このコードには不適切 |
| 対応方法 |
.rubocop_todo.ymlで除外 |
.rubocop_todo.ymlで除外 |
どちらも静的解析ツールの限界が原因でした。
静的解析ツールの限界
RuboCopのような静的解析ツールは、ソースコードのテキストを解析するため、以下のような制約があります：
| 検知できること | 検知できないこと |
|---|---|
| 構文エラー | 実行時の制御フロー |
| スタイル違反 | オブジェクトの実際の型 |
| 一般的なアンチパターン | ドメイン固有の文脈 |
| 命名規則の違反 | ビジネスロジックの妥当性 |
ツールはコードの意図や文脈を理解していません。あくまで機械的にパターンマッチングを行っているだけです。
学んだこと
ツールとの正しい付き合い方
-
-aを基本とする: まずは安全な修正から始める -
-Aは慎重に: 修正後は必ず動作確認とテストを実行 - 盲信しない: 自動修正の内容を1つずつレビューする
-
.rubocop_todo.ymlを恐れない: 除外すべきケースは積極的に除外する
再発防止に向けて
スマサテでは、以前から以下のプロセスを実施していました（今回もそのプロセスの中でデグレを発見できました）が、今回の事例を通じて、その重要性を改めて認識しました：
- 自動修正後は必ず手動でコードレビューを実施
- 修正範囲が大きい場合はプルリクエストを分割
- 自動テストを必ず実行し、通過を確認
- 検証環境での動作確認を徹底
- 検証環境でのE2Eテストの実施
おわりに
RuboCopは素晴らしいツールですが、万能ではありません。ツールの特性と限界を理解し、最終的な判断は人間が下す。それが健全な開発プロセスだと考えています。
今回の2度のデグレは痛い経験でしたが、チーム全体で「ツールとの付き合い方」を学ぶ良い機会になりました。静的解析ツールを使う際は、その限界を理解した上で、適切に活用していくことが大切です。
スマサテでは、このような技術的課題に一緒に取り組んでくれるエンジニアを募集しています。
Discussion