Upgrade to Pro — share decks privately, control downloads, hide ads and more …

PHPerKaigi 2022をきっかけにPHPStanにコントリビュートした話 / PHPerKaigi makes me PHPStan's Contributor

PHPerKaigi 2022をきっかけにPHPStanにコントリビュートした話 / PHPerKaigi makes me PHPStan's Contributor

muno92
PRO

May 27, 2022
Tweet

More Decks by muno92

Other Decks in Programming

Transcript

  1. PHPerKaigi 2022をきっかけにPHPStanに
    コントリビュートした話
    2022/5/27
    PHPerのための「静的解析」を語り合うPHP TechCafe
    muno92
    1

    View Slide

  2. 自己紹介
    twitter: @muno_92
    PHP(≒
    エンジニア)歴5年くらい
    型や静的解析は好き
    https://github.com/muno92/resharper_inspectcode
    GitHub ActionsでC#のコードを静的解析するアクション
    JetBrainsのReSharper command line toolsを使用
    2

    View Slide

  3. こんなルールを作りました

    $dt = new DateTimeImmutable();

    $dt->__construct('2022/1/23');


    > ./vendor/bin/phpstan analyse -c phpstan.neon

    ------ -------------------------------------------------------------

    Line sample.php

    ------ -------------------------------------------------------------

    4 Call to __construct() on an existing object is not allowed.

    ------ -------------------------------------------------------------


    デフォルトでは有効になっていないため、次ページ記載の設定が必要
    3

    View Slide

  4. 今回作ったルールを有効にするための設定
    phpstan.neon
    includes:

    - vendor/phpstan/phpstan/conf/bleedingEdge.neon

    - vendor/phpstan/phpstan-strict-rules/rules.neon

    parameters:

    #
    レベルは2
    以上

    level: 2

    paths:

    -
    解析対象のパス

    4

    View Slide

  5. 伝えたいこと
    PHPStanにPull Requestを投げたときの盛り上がり
    PHPStanのルール作成は意外とハードル高くない
    5

    View Slide

  6. 話さないこと
    PHPStanのルールの詳しい作り方
    6

    View Slide

  7. 静的解析ツールはソースコードをパーツに分解・解析している
    https://www.php.net/manual/ja/function.token-get-all.php
    ライブラリを使っていても基本的な考え方は同じ
    (PHPStanはPHP Parserを使用) 7

    View Slide

  8. 今日はこれだけ分かっていればOK
    8

    View Slide

  9. PHPStanのルールの詳しい作
    り方はこちら
    https://speakerdeck.com/nazonoh
    ito51/make-phpstan-customrule-
    d596e237-6692-4e6b-b83b-
    f5fac3618797
    https://youtu.be/xemXGgx0w0k
    9

    View Slide

  10. 話の発端 (PHPerKaigi 2022 day1)
    予防に勝る防御なし - 堅牢なコードを導く様々な設計のヒント (twadaさん) 10

    View Slide

  11. 既存の静的解析ツールで検知出来る?
    PHPerKaigi翌日、4/12(火)の9時頃
    PHPerKaigi参加者用のDiscordにtwadaさんが問いかけ
    確認したところ、PHPStanやPsalmに明示的にコンストラクタの直接実行を禁止するル
    ールは無かった
    (詳しくはブログ参照)
    11

    View Slide

  12. コントリビュートチャンス?
    twadaさん「コントリビュートのチャンス!!」
    (ぼんやりした実装イメージはあるけど、OSSへのコントリビュートはハードルを感じ
    てしまう・・・)
    12

    View Slide

  13. 実装イメージ
    PHPStanは1つ1つのチェック項目をルールクラスとして実装していたので、
    1. ソースコード内でメソッド呼び出しを行っている箇所を解析し
    2. 呼び出しているメソッドが__constructで
    3. 正常な使い方をしていない場合
    にエラーを返す良い感じのルールクラスを実装すれば良さそう
    (1〜3番の具体的な実装方法は思いついていない)
    13

    View Slide

  14. 良い感じのルールクラス

    class MyRule implements Rule

    {

    public function getNodeType(): string

    {

    //
    一致するコードがprocessNode
    でチェックされる

    return Node\Expr\
    メソッド呼び出しを行っているパーツ::class;

    }

    public function processNode(Node $node, Scope $scope): array

    {

    // __construct
    実行をチェック

    if (
    エラーでない場合) {

    return [];

    }

    return ['
    エラーです'];

    }

    }

    14

    View Slide

  15. そうこうしている間に興味を持つ人が増える
    20時過ぎ
    (ハードルは感じるけど無さそうだし作るかなあ・・・)Discordを見る
    tadsanさん、mpywさんが興味を持つ
    自分「スピード勝負になりつつある・・・!?」
    15

    View Slide

  16. PHPerKaigi (day3?) ハッカソン爆誕
    加速するDiscord
    名前どうする?
    「1. ソースコード内でメソッド呼び出しを行っている箇所を解析し」  
    どのノードタイプで引っ掛ける?
    「3. 正常な使い方をしていない場合」  
    弾いてはいけない正常なパターンはどれ?
    どこに実装する?
    PHPStan本体
    phpstan-strict-rules
    こんな感じのコードでいけそう (from mpywさん、yu-ichiroさん)
    16

    View Slide

  17. これはOK
    class MyDateTimeImmutable extends DateTimeImmutable

    {

    public function __construct(string $datetime = "now", ?\DateTimeZone $timezone = null))

    {

    parent::__construct($datetime, $timezone);

    }

    }

    17

    View Slide

  18. これはNG
    class MyDateTimeImmutable extends DateTimeImmutable

    {

    public function mutate(string $datetime = "now", ?\DateTimeZone $timezone = null))

    {

    parent::__construct($datetime, $timezone);

    }

    }

    $dt = new DateTimeImmutable();

    $dt->__construct('2022/1/23');

    18

    View Slide

  19. こういったパターンもある (OK?)
    from mpywさん
    class MyDateTimeImmutable extends DateTimeImmutable

    {

    public function __construct(string $datetime = "now", ?\DateTimeZone $timezone = null))

    {

    self::__construct($datetime, $timezone);

    }

    }


    許容する形で実装したが、最終的にマージされたコードではNGに
    19

    View Slide

  20. 話がまとまり、実装を任して頂ける事に
    OK/NGのパターンが整理出来た
    PHPStan本体組み込みのルールとして提案する
    DateTimeImmutableインスタンスのコンストラクタを直接呼び出す事により、安全
    でないコードを再現出来てしまうため
    20

    View Slide

  21. 実装(PHPStan作者の方による調整が入った最終形)

    namespace PHPStan\Rules\Methods;

    // use
    は行数の都合で省略

    class IllegalConstructorMethodCallRule implements Rule

    {

    public function getNodeType(): string

    {

    //
    戻り値のNodeType
    (今回で言えばメソッド呼び出し)が解析対象のコードに含まれていた場合、processNode
    で解析する

    return Node\Expr\MethodCall::class;

    }

    public function processNode(Node $node, Scope $scope): array

    {

    if (!$node->name instanceof Node\Identifier || $node->name->toLowerString() !== '__construct') {

    //
    エラー無し

    return [];

    }

    return [

    RuleErrorBuilder::message('Call to __construct() on an existing object is not allowed.')

    ->build(),

    ];

    }

    }

    どうやってここに行き着く? 21

    View Slide

  22. とりあえずvar_dump
    3ページ目のコードを解析した場合のvar_dump($node)を抜粋
    object(PhpParser\Node\Expr\MethodCall)#3146 (4) {

    ["attributes":protected]=> array(9) {}

    // 4
    行目にあるdt
    という名前の変数がメソッドを呼び出している

    ["var"]=>

    object(PhpParser\Node\Expr\Variable)#3142 (2) {

    ["attributes":protected]=>

    array(9) {

    ["startLine"]=>

    int(4)

    }

    ["name"]=>

    string(2) "dt"

    }

    //
    呼び出しているメソッドの名前は__construct

    ["name"]=>

    object(PhpParser\Node\Identifier)#3143 (2) {

    ["attributes":protected]=> array(6) {}

    ["name"]=>

    string(11) "__construct"

    }

    //
    今回は引数の情報は要らない

    ["args"]=> array(1) {}

    }

    22

    View Slide

  23. もりもり実装
    MethodCall( ->__construct
    )に対応
    StaticCall( ::__construct
    )も同様にvar_dumpで型情報を確認し、実装
    23

    View Slide

  24. シュッとIssueを立てて下さるtadsanさん
    実装している間に、PSR-7にIssue作成
    https://github.com/Nyholm/psr7/issues/198
    Pull Request提出用にコードを整えている間にPHPStanにもIssueを作成し、PHPStan作
    者のOndreさんの意見も確認して下さる
    https://github.com/phpstan/phpstan/issues/7022
    24

    View Slide

  25. Pull Request出した、ヨシ!
    25

    View Slide

  26. 焦ってCIに引っかかり、修正
    この時点で0:32。
    焦ってしまい、ローカルで静的解析やLintを走らせていなかった。
    テスト
    Nullsafe Operator ?->
    の使用がPHP7系でのテストで引っ掛かる
    ローカルではPHP8.1でテストを走らせていたが、CIではPHP7.2〜8.1でテスト
    テスト対象のコードに記載した再帰コンストラクタが原因で一部のテストでメモリ
    エラーに
    テスト対象のコードで再帰を1回に抑える
    静的解析
    tadsanさんが助けて下さる
    Lint
    phpcbfで自動修正
    26

    View Slide

  27. CIの完了を固唾を飲んで待つ面々
    なんと驚きのジョブ数244
    27

    View Slide

  28. CI (テスト)
    28

    View Slide

  29. CI (E2Eテスト)
    29

    View Slide

  30. CI (Lint)
    30

    View Slide

  31. CI (静的解析)
    31

    View Slide

  32. CI (Pharコンパイルなど)
    (phpstan-phpunitなど拡張のテストも実行) 32

    View Slide

  33. All グリーン!
    33

    View Slide

  34. その後
    4/13
    承認され、Ondreさんから後は引き取るとコメント頂く
    4/17
    調整が入った上でマージされた
    4/26
    PHPStan 1.6.0でBleeding Edgeとして利用可能に
    5/5
    PHPStan 1.6.6でphpstan-strict-rulesに移動
    34

    View Slide

  35. まとめ
    PHPerKaigiをきっかけに、ハッカソン的にPHPStanにコントリビュートした
    意外とPHPStanのルールを作成するハードルは高くない
    PHPStanへのコントリビュートが全てでは無い
    「こういったコードを検知したい」をルールに落とし込んでみる
    自社・自チームのコーディング規約をルールとして実装するなど
    手を動かしてみるとなんとかなるかもしれない
    35

    View Slide

  36. 良い静的解析ライフを
    36

    View Slide