PHPStanのエラーを2000個弱解消した際のTips

こんにちは、スターフェスティバル株式会社でバックエンドエンジニアをやっている吉田あひるです。

先日、僕が関わっているプロダクトで発生していた 2000 個弱の PHPStan のエラーを一部を除きほぼ全て解消したので、その際に何に気を付けて作業をしていたのか、そして一連の作業の中でどういった恩恵を受けることができたのかを共有できればと思います。

PHPStan とは

詳しい説明は割愛しますが、PHP の静的解析ツールで型レベルの整合性のチェックなどを行うことができるツールです。
https://phpstan.org/user-guide/getting-started

なぜ PHPStan のエラーを解消したかったのか

バグをリリースする確率を減らせる

僕の経験上、ある程度の量のコードを書いたあとに PHPStan を実行するとほぼ確実に何かしらのエラーが報告されるため、少なくとも僕にとっては静的解析なしで型レベルの整合性を担保し続けることは難しいということがわかっています。
PHPStan を使うことで型レベルで辻褄があっていない、あるいは合わない可能性のあるコードを自動で検知することができるため、バグをリリースする可能性を減らすことができます。

レビューの負担を下げることができる

PHP の組み込み関数は int|false だったり string|string[]|null だったり思わぬ型が返ってくることがよくあるため「この関数は false が返ってくるパターンもあるので、その場合の条件分岐も追加してください〜」といったレビューが発生したりします。
こういったやりとりを減らすことが出来るのはもちろんメリットですし、レビュワーがそういった細かいところを気にせずにもっと価値の高いところにフォーカスしてレビューすることができるようになります。

PHP や依存ライブラリのバージョンアップを少しでも楽にしたい

僕が関わっているプロダクトはしっかりと全体を自動テストできているわけではないため、ライブラリや PHP のバージョンを上げる際にデグレを検知するためには手作業でテストをするしかない部分があります。
しかし、PHPStan を用いてプロダクト全体に静的解析をかけることが出来れば少なくとも型レベルのデグレは自動的に検知することができるため、手作業のみで検証するしかない現状に比べるとだいぶマシになります。

Baseline について

PHPStan のような静的解析ツールを途中から導入した場合、大量のエラーが発生するため CI に組み込むことができないという状況になりますが、そういった場合でも PHPStan を導入できるように Baseline という機能が用意されています。
Baseline は現状発生しているエラーを全て無視するための機能なので、現状のエラーを無視することで途中からでも無理なく CI に PHPStan を組み込むことができ、新規の変更にのみ静的解析をかけることが可能になります。

なので、無理に既存のエラーを減らさなくても新規コードに対して静的解析をかけることが出来るのであればそれでいいのではという意見もありそうですが、途中にエラーが紛れているロジックに対する静的解析はそこまで信用できるものではないと個人的には考えています。
例えばこのような例だと最後の substr() も本来は PHPStan に怒られるべきコードなのですが、その手前でエラーが発生した結果スルーされてしまっています。

class Foo
{
    public function foo(): self
    {
        return $this;
    }
}

class Bar extends Foo
{
    public function bar(): ?string
    {
        return null;
    }
}

$foo = new Bar();
$stringOrNull = $foo->foo()->bar();

echo substr($stringOrNull, 0, 10);

こういった理由により、既存のエラーについても極力減らすことに決めました。

進めていく上で気をつけていたこと

主に以下の 2 つをテーマにしていました。

• レビュワーの負荷を下げる
• エラーを解消する過程で極力バグを出さない

レビュワーの負荷を下げる

エラーの数が 2000 弱なので、1 つの PR で 50 個のエラーを解消したとしても 40 個の PR をレビューしてもらう必要があります。
また修正範囲はアプリケーション全体なので普段触れない箇所の修正をレビューする必要があったり、1 つ 1 つの修正に関連性がないことも多いため頭の切り替えが多く必要だったりなど、レビュワーの負担がかなり大きいことが予想されます。
レビュワーの負担を無視した PR を出し続けてしまうとレビューの滞りが発生してしまったり、あるいは殆ど何も見ずに Approve されてしまうなどのリスクが考えられるので、PHPStan 対応を進めるにあたってレビュワーの負担軽減はかなり重要であるように思いました。

エラーを解消する過程で極力バグを出さない

いわずもがなですがバグというのは極力リリースしたくありません。
が、(繰り返しになりますが)PHPStan の対応ではアプリケーション全体を対応する必要があるため普段触れない箇所を修正する必要があります。
その上、大量の修正をこなす必要があるため、慣れが生じた際にうっかり考慮の足りない修正を行ってしまう可能性が高いと思われます。
そのため、バグをリリースしにくいルールを作る必要があるように思いました。

進める上でのルール

レビュワーの負担を下げ、極力エラーを出さずに対応を進めるために実際にいくつかのルールを決めてやっていたので紹介させていただきます。

あるべき姿に直さない

既存のコードを読んでいると「もっとこうなっていたら読みやすいのに」などいろいろ思ってしまうものだと思いますが、そういった場合も極力手を加えないように気をつけていました。
なぜかというと、PHPStan の対応で重要なのは「エラーの数を減らすことで型レベルで整合性が担保される範囲を広げる」ことであって、リファクタリングそのものではありません。
あるべき姿に直そうという欲を出すと、ただでさえ多い工数がさらに増えゴールがより遠くなりますし、リファクタリングは修正範囲も広がりがちなため、これはレビュワーの負担が増えることにもつながります。

そして型レベルの安全性を担保できていない状態から大胆なリファクタリングまでしてしまうと想定していないバグを産む可能性も高いと考えています。
そのため、「エラーを潰す過程で極力バグを出さない」というテーマの観点からも型レベルの安全性を保証するという段階で一区切りをつけ、次のステップとしてしっかりリファクタリングをするという進め方が望ましいと考え、PHPStan 対応の中ではリファクタリングは極力行わないというルールにしました。

なるべく細かく PR を出していく

やはり大きな PR というのはレビューの負荷が高いため、最大でも 5 分程度でレビューが済むような量を意識していました。
他には、考慮が必要になりそうな修正はそれだけ別の PR に分けたりなどしてレビューの負荷を調整していました。

挙動が変わる場合は問題がない根拠を示す

あまり知らないコードの挙動が変わる場合はレビュワーの負担がかなり高くなってしまうと思われるので、メソッドのシグネチャに変更がある場合はそのメソッドの呼び出し箇所を提示したり、元の実装が間違っていると思われる場合は過去の commit ログをもとに「本当はこうしたかったはず」とわかる根拠を示すようにしてレビュワーの負担を下げるようにしていました。
逆にこれができない場合はデグレさせてしまったり、バグをリリースしてしまう危険が高いと思われるため手をつけずエラーのまま無視するようにしていました。

エラーを無視することを恐れない

全てのエラーを完全に解消することを重要視してしまうと、「そもそも設計から直さないとエラーの解消が難しい」といった場合などにエラーの解消にかなりの時間がとられてしまうため、全体の進捗に悪影響が発生してしまいます。
また、そういったエラーは修正が大きくなりがちなのでバグをリリースする確率も高いと考えています。
そのため、そういったものは次のフェーズで対応すると決めてエラーを無視する方がメリットが大きいと判断して、難しいと感じたものは躊躇なく無視することにしていました。

ローカル変数に対して安易に var タグを使わない

/** @var string $foo */ のような PHPDoc による補助は慎重に行うようにしていました。
PHPStan 対応の目的の一つに依存ライブラリのアップデート時などのデグレを検知することがありますが、こういった型の補助はデグレを見逃す可能性があるためです。

例えば「使いたい関数の型が緩く実際には string が返ってくるものの mixed 扱いになっていたため、var タグを使って PHPStan のエラーを抑制した」といった場合に、もしその関数の返り値が string|null に変更されるとおそらくその var タグを使用している箇所は PHPStan の検査を抜けてしまいます。
このように PHPStan のエラーを抑制した結果、本来の目的が達成できない危険性があるため、主観になってしまいますが明らかに問題がないと思われるもののみ var タグをつかうようにしていました。

実際にやってみてどうだったのか

こういったルールの元、計 50 個ちょっとの PR を出し先日ようやく対応を終えることができました。
レビューはチームメンバーの多大なる協力のおかげで速やかに進み、バグに関しては軽微なものを 1 度リリースしてしまうに留めることができました。

対応を進める中で最初に挙げたメリットを享受できるようになったほか、未発見のバグを複数個見つけることができたり、デッドコードを複数個発見することができたり、ドメイン知識が思いの外増えたりなど、当初想定していないメリットもあったため空き時間にやるタスクとしてはかなりコスパの良い作業なように思われます。

みなさんも是非空いてる時間に PHPStan のエラーの解消をしてみてはいかがでしょうか。