RuboCop::Lint::SafeNavigationWithEmptyとは

CBcloud 2022アドベントカレンダーの5日目の記事です。

Lint/SafeNavigationWithEmptyとは、 RuboCopのルールでempty?をぼっち演算子で呼び出しているのを検知しANDで別々に書くよう修正してくれる。

「ハイ、ソウデスカ」で自動修正しちゃってもいいんですが、なんでそうするべきなのか気になったので調べてみる。

Checks to make sure safe navigation isn't used with empty? in a conditional.

While the safe navigation operator is generally a good idea, when checking foo&.empty? in a conditional, foo being nil will actually do the opposite of what the author intends.

日本語訳すると、「条件分岐の中でempty?とぼっち演算子が一緒に使っていないかチェックする」「ぼっち演算子は一般的には良い手法ではあるが、条件分岐内でfoo&.empty?をチェックするとき、 fooがnilであることは実際には書き手の意図と全く逆のことである。」

全く意味が分からないので僕の解釈を付け加える。

例えば以下のようなコードであった場合、ガード節は引数argが不正な値でないかチェックしている。

def do_something(arg)
  return if arg&.empty?

  do_something_with(arg)
end

この時、arg == nilであった場合はチェックをすり抜けdo_something_withにnilが渡されることになる。この予測しづらくかつ見落としがちな挙動が書き手の意図した挙動であるか、それを読んだ他者もしくは後で見返した書き手がすぐには判別できない。ならば少なくとも、それぞれをチェックするようにすればこの行が何を「しているか」(したいかではなく)は見落とさなくなるであろう。

以上がこのCopが作られた背景と思われる。

ついでに

ついでに導入時のPRまで遡ってみる。 https://github.com/rubocop/rubocop/pull/6568

おおよそ同じことが書かれているがこのディスクリプションの中で、更にそれより前にされた議論を見れるリンクがある。

そこでは「return if variable&.empty?が書き手の意図しない挙動をする」や「レビュー時に見落としがち」といったことが話されているので概ね間違いなさそう

更についでに

RuboCopルールの定義ファイルの読み方を解説する。

これがさっきまで話していたCopの実態。

RuboCopのルールはRuboCop::Cop::Baseを継承して定義される。(22行目)
on_ifを定義することでコードを探索して if 文が出てきたらこのon_ifが実行される。なので引数のnodeは親が必ずifノードになる。(32行目)
safe_navigation_empty_in_conditional?はCop::Baseで提供されるdef_node_matcherを使って28行目から30行目で定義されたASTパターンであり、nodeがこれに完全一致しない場合はfalseを返す。(33行目)
最後にadd_offenceでルール違反の検知を登録する。その際、MSG(25行目)が警告のメッセージとして渡される。(37行目)