PHPerKaigi 2022への参加をきっかけにPHPStan本体にPRを出した

PHPerKaigi 2022の翌日、4/12にPHPerKaigi参加者用のDiscordで発生したやり取りをきっかけとしてPHPStan本体にコンストラクタ直接実行を検知するためのPRを出しました。

github.com

PHPerKaigiは実は4日目もあった！？と感じる程の盛り上がりがPRを出すまでにあったので、その一連の流れをまとめます。

#phperkaigi #a 4日目セッション

テーマ: 外部からの手動 ->__construct() コールを PHPStan 拡張を書いて滅ぼそうぜワークショップ
— wip (@mpyw) 2022年4月12日

突然 #phperkaigi のDiscordでPHPStanに機能追加提案のハッカソンが巻き起こっている
— にゃんだーすわん with kgmn (@tadsan) 2022年4月12日

2022/4/26 追記
PHPStan 1.6.0がリリースされ、Bleeding EdgeとしてPRで追加したルールも利用可能になりました。

https://github.com/phpstan/phpstan/releases/tag/1.6.0

Bleeding Edgeを有効にし、Levelを2以上にするとコンストラクタを直接呼び出した場合にエラーと見なされるようになります

2022/5/5 追記

PHPStan1.6.6にて、phpstan-strict-rulesにルールが移動されました。

github.com

phpstan-strict-rulesをインストールした上で

includes:
    - vendor/phpstan/phpstan/conf/bleedingEdge.neon
    - vendor/phpstan/phpstan-strict-rules/rules.neon
parameters:
    level: 2

のように設定頂ければ、コンストラクタの直接実行をエラーとして検知するようになります。

大まかな流れ
発端
4/12時点の静的解析ツールでの検知
PRを出すまで
さいごに

大まかな流れ

09:01 t_wadaさんから「静的解析ツールで__construct2回呼び出しを禁止出来ないか」とDiscordに問いかけ
10:18 Psalmでは検知出来る、PHPStanでは検知出来ないと返答
18:02 t_wadaさんから「__constructの明示的な呼び出しをターゲットにするルールが無いのであればコントリビュートのチャンスでは」とコメント頂く
20:15頃 tadsanさん・mpywさんが関心を持つ
20:25 焦り出す自分
mpywさんが仮実装を投稿されたり、tadsanさんが対処しなければいけないケースを整理されたり
- ここでPHPStanの拡張を書くのでは無く本体へのルール追加として提案する方向になる
21:33 phpstan/phpstan-srcをforkして作業に取り掛かり始める
テストクラス・ルールクラスの実装を進め、その間にもDiscordで実装方法が検討される
22:37 下記2パターンを検知出来るようになる
- 同クラスのコンストラクタ以外のメソッドから$this->__construct
- $生成したインスタンス->__construct
23:08 コンストラクタ内で$this->__constructを実行する再帰呼び出しのパターンはエラーにならないようにする
23:38 __constructの静的呼び出しを検知出来るようになる
23:52 tadsanさんがIssueを立てて下さる
00:13 条件式の整理などリファクタリングを行い、PRを提出
00:32 MethodCall（->で呼び出すパターン）とStaticCallをそれぞれ別のルールでチェックする形に変更
（その方が良いとIssueにコメントがあったため）
CIでLint/静的解析/テストが落ちる
01:15 Lint/静的解析/テストを通るように修正
01:25 CIの全ジョブがグリーンになり、Discord上の面々は解散
記念スクショ
03:52 PHPStan作者のOndreさんにPRが承認され、後は引き継いで頂けるとコメント頂く

発端

PHPerKaigi day1（4/10）でt_wadaさんが発表された「予防に勝る防御なし - 堅牢なコードを導く様々な設計のヒント」にて、たとえDateTimeImmutableであっても生成したインスタンスの__constructを実行する事で値を書き換えられてしまうとの話がありました。

この__constructが2回実行されるコードについて、

静的解析で禁止すれば良いと発表資料に入れたいが、そういったルールは無いか

とt_wadaさんがDiscordで質問されたのが発端となり今回のPRが生まれました。

4/12時点の静的解析ツールでの検知

t_wadaさんの質問を見て、最初は「PHPStanのphpstan-disallowed-calls拡張を使えば禁止出来るのでは？」と思いました。
しかし、phpstan-disallowed-callsで特定クラスのコンストラクタを指定した場合、new DateTimeImmutableもルールに抵触してしまいます。

To disallow naive object creation (new ClassName() or new $classname), disallow NameSpace\ClassName::__construct in disallowedMethodCalls. Works even when there's no constructor defined in that class.

PHPStan自体のレベルを9（MAX）に設定しても__construct直接実行は検出出来ませんでしたが、Psalmでレベル1（MAX）にした場合はエラー検知出来ると分かったためその旨をDiscordに投稿しました。

※Psalmも下記の理由でエラー検知出来るだけで、__construct直接実行を禁止するルールがあったり、__constructを直接実行してはいけないとエラーメッセージが出たりする訳ではありません

$dt->construct('2022-04-10')の場合
https://psalm.dev/docs/running_psalm/issues/UnusedMethodCall/
に抵触する
$hoge = $dt->construct('2022-04-10')の場合（1番を回避出来てしまわないかの確認）
__constructの戻り値はvoidのため、
https://psalm.dev/docs/running_psalm/issues/AssignmentToVoid/
に抵触する

PRを出すまで

上記の回答に対してt_wadaさんからコントリビュートチャンス！とコメント頂きましたがこの時点では腰が引けていました。

自分用のツールを「誰も見てないだろ」位の気軽さでパブリックリポジトリとして公開してはいるものの、OSSへのPRといったらPHPマニュアルに出した事が1回あるだけで、まだまだハードルを感じてしまっていました。

そういった気持ちで「とはいえ現状該当コードを禁止するルールは無い気がするし、Packagistでダウンロード数が多いPHPStan拡張をざっと見てみてそれでも無かったら作るかなあ」と考えながらDiscordを見たらtadsanさん・mpywさんが実装に関心を持たれており、速く実装しなければ、と慌てて取り掛かり始めました。

そして名前やテストの書き方どうしようなどと考えている間にもDiscordでは実装方法の検討などが進み、話がまとまった後、実装は私に任せて頂ける事になりました。
（ここで「自分がやる」ではなく「実装任せた」と言って頂けて大変有り難かったなと感じています）

過去にPHPStanの拡張を作ろうとした事があり、なんとなく「ソースコードをPHP Parserで解析した結果から良い感じにNodeを選び、__constructという文字列が含まれているかチェックすればいい」位のイメージはついていたのですが、具体的にどのように実装すれば今回のケースを満たせるかは調べてみないと分からないといった状態だったので、アドバイスのおかげでその辺り悩まず、実装に集中する事が出来ました。

また、PHPをがっつり書くのは久しぶりだったのですが、抽象クラス・インターフェースを指定してクラスを定義するだけでどのメソッドが必要か型情報から教えて貰え、PhpStormなら1クリックでそのメソッドを生やせるため「PHP/PhpStorm最高だな」と改めて感じました。

そして実装を始めてからも

__constructを呼び出すのはこういったケースもある
この静的解析のエラーはここが原因

など、色々と助けて頂きました。

焦ってローカルで全テストなどを走らずにpushしたことでPR出してからCIを通すまでに手間取ってしまったので、その点は反省点です。

（ちなみに、phpstan-srcのMakefileに定義されているタスクの内、tests-coverageを実行するとM1 ProのMacでもファンがぶん回り、M1でもファンが回る事あるんだとなりました。）

さいごに

このような流れで、PHPStanへのPRを出す事が出来ました。

自分1人ではそもそもPR出す踏ん切りがつかなかったり、実装にあたっての不明点解消に時間が掛かったりとここまでスムーズには進められなかったと思います。
背中を押して頂いたり、実装案を提示して下さったり、色々と助けて頂いたt_wadaさん、tadsan、mpywさん、yu-ichiroさん、ありがとうございました。
そして、他の人が今回の自分のような立場に立っていた時に、上手く助けになれたらと思います。

また、カンファレンス会場のアンカンファレンスでスクリーンを囲んでわいわい話しながら実装しているような熱を感じ、単純に作業していて楽しかったです。
こういった偶然の出会い・化学反応がカンファレンスの醍醐味であり、たまらない点なのかなと改めて感じました。

これをきっかけにPHPStanへの関わりを（1ユーザーとしても1開発者としても）深めていきたいです。