PHPstanのカスタムルールの書き方を紹介します

はじめに

こんにちは。シニアエンジニアのプログラミングをするパンダ（@Panda_Program）です。本記事は BASE アドベントカレンダー 2023 の11日目の記事です。

BASE のバックエンド開発では巨大なモノリスからモジュラーモノリスへの移行が進んでいます。この記事では、モジュラーモノリスの中で自分のチームが担当しているモジュールに導入した PHPStan のカスタムルールの導入とその効果について紹介します。

PHPStan は BASE のモジュラーモノリスなバックエンドシステムに既に導入されていました。モジュラーモノリスの中で PHPStan のカスタムルールは2種類あります。各モジュールが守るべき共通のルールと、それぞれのモジュール内で特有のルールです。

PHP のコード品質を担保する PHPStan は多くの開発現場で採用されていますが、具体的なカスタムルールの事例はあまり公開されていないように思います。そこで、自分たちが実際に現場で使用しているカスタムルールの一部を共有します。PHPStan のカスタムルールに関しては、どのようなルールを設けているか、また具体的なコードにどう落とし込んでいるかの2つの側面があります。特に後者の面で一定の価値があるのではないかと考えて記事にしました。

自分が所属しているチームでは、 CRM モジュールという BASE の CRM 領域の機能を担う部分を開発しています。本記事では CRM モジュールで適用するカスタムルールに限定して紹介します。

課題: レビューで同様の指摘をしていること

ツールは何らかの課題を解決するための手段です。カスタムルールを作成するに至った理由も、コードレビュー時にどうも似たような指摘を複数回、いろんな人にしているかもしれないという課題があったからです。

ストレートに考えると同様の指摘を複数回することの解決策は、コーディング規約を作成したりモブプロ・ペアプロで担保することです。そこで開発チームの前提を共有すると、現在2つのスクラムチームがCRM モジュールの担当になっています。メンバー構成について、Aチームはバックエンドエンジニアが6名、Bチームはフロントエンドエンジニア2名+バックエンドエンジニア3名であり、自分もその中の一人です。

LeSS（Large-Scale Scrum）というフレームワークに基づき、2チーム体制で開発をし始めて1年以上が経ちます。しかし、当たり前のことですが、チームは均質ではありません。時間が経って互いのチームが LeSS に慣れたとしても、一人一人のスキルにバラツキがあったり、他チームからサポートとして来てくれている方が2名いたりと、コミュニケーションコストが減ることはありません。

以前はコーディング規約や細かい書き方のドキュメントは整備されていませんでした。このため、チーム歴が長い人がペアプロをして CRM モジュールではこう書くんだよと口頭で伝えるか、コードレビューで誰かがコメントすることで知識の平準化が図られていました。しかし、ペア・モブプロにも限界はあります。設計レベルの議論はチームを横断して行われるものの、「今はこう書くようになった」という細かい実装の知識はチーム内に閉じてしまいます。

自分は両チームのコードレビューをしているのですが、レビューコメントでその中で細かいコーディングレベルの話を共有することを心がけていました。同一モジュール内でも UseCase ごとに書き方がバラけてしまうを防ぐためです。

AチームのXさんのコードレビューで、プロパティが readonly ではない DTO を見つけては、コメントで「最近Bチームでは DTO は必ず readonly class にすることになったよ」と伝えたり、BチームのYさんのコードに switch 文を見つけては「過去に swtich 文は使わず match 式にしようという指摘があって、あの時はチーム内で特に議論はなかったけどやっぱり match 式使う？どうする？」と決定を促す質問をするうちに、両チームの過去のプルリクエストに遡り、当該コメントのURLを貼るのも流石に大変だなと思うようになってきました。

そこで、非同期で誰でも参照できるコーディング規約を作ることと、静的解析で実装レベルの表記揺れを防ぐことを目指しました。そのために、まずは意を決して（というほど大変でもなかったですが）直近3ヶ月の約180件のPRを見返しました。結果的に、期間は1週間ほどで開発のスキマ時間で過去のPRを辿っていきました。これは良い指摘だと思えるレビューコメントをPRの中からピックアップし、その内容を以下の3つに分類しました。

静的解析で検出できるもの
必ず守る方針
どちらでもいいもの

解決方針: コーディング規約を作る一方で、静的解析で表記揺れを検出する

それぞれの解決方針は以下の通りです。

分類	対策
静的解析で検出できるもの	PHPStanのカスタムルールを作成して CI で検査する
必ず守る方針	ドキュメント化されていなかった暗黙のルールだったのでコーディング規約という形で文書化
どちらでもいいもの	本当にどちらでも良いので特にルール化はしていない、ということを明示するために文書化

「必ず守る方針」も「どちらでもいいもの」については、マークダウンで文書化してモジュラーモノリスがあるレポジトリにコミットしました。そうすると、S3 にデプロイした VitePress 上で誰でも（もちろん社内の人限定ではありますが）閲覧することができます。この仕組みもちょっと面白いので、その話はどこかで書くかもしれません。

「静的解析で検出できるもの」については PHPStan で対応することにしました。なお、カスタムルール作成を検討するにあたり、ルールにするほどか迷うものは両チームにアンケートを取っていました。全て過去の PR でコメントがあったものです。

1つ目の自身をインスタンス化する書き方については、本当にどっちでもいいので特に文書にも記載していません。2つ目の「テストコードで if 文を書けないようにする」ものは、カスタムルールを作成しました（下に実コードを載せています）。3つ目の PHP 組み込みの enum の生成テストについては、あって困ることはないが特に書かなくても問題ないということを「どちらでもいいもの」の文書に掲載しています。

（3つ目は、例えば String の Backed Enum SomeEnum::Foo がある時に、SomeEnum::Foo->name === 'Foo', SomeEnum::Foo->value === 'foo'であることをテストするかしないかというものです）

カスタムルールの紹介

CRM モジュール内で実際に使用しているPHPStanのカスタムルールをいくつか紹介します。

DTO は Readonly Class にするというルール

名前の最後がDtoであるクラスは、必ずreadonly classにするというルールです。readonly class は PHP 8.2 からの機能で、コンストラクタ以外でプロパティに代入ができないので DTO に最適です。

<?php

declare(strict_types=1);

namespace BaseInc\Modules\Crm\Tools\PhpStan\Rules;

use PhpParser\Node;
use PhpParser\Node\Stmt\Class_;
use PHPStan\Analyser\Scope;
use PHPStan\Rules\Rule;

/**
 * DTO は Readonly Class にするというルール
 */
final class ReadonlyDtoRule implements Rule
{
    public function getNodeType(): string
    {
        return Class_::class;
    }

    public function processNode(Node $node, Scope $scope): array
    {
        if (!($node instanceof Class_)) {
            return [];
        }

        $isInApplicationDirectory = str_starts_with($node->namespacedName->toString(), 'BaseInc\Modules\Crm\Application');
        $isRequestClass = str_ends_with($node->namespacedName->toString(), 'Dto');

        if (!$isInApplicationDirectory || !$isRequestClass) {
            return [];
        }

        return $node->isReadonly() ? [] : ['DTO が Readonly Class ではありません'];
    }
}

DTO には setter が不要なので、PHP 8.2 だと以下のようにシンプルに書くことができます。

readonly class MembershipMemberSummaryDto
{
    public function __construct(
        public MembershipId $membershipId,
        public UserId $userId,
        public MembershipName $membershipName,
        public MembershipMemberActionType $actionType,
        public int $count,
    ) {
    }
}

switch 文を禁止するというルール

switch 文を使ってはいけないというルールです。CRMモジュール内では swtich はそもそも1箇所でしか使われていなかったので簡単に書き換えができること、switch でできることは match 式で簡単にできることから採用されました。

<?php

declare(strict_types=1);

namespace BaseInc\Modules\Crm\Tools\PhpStan\Rules;

use PhpParser\Node;
use PhpParser\NodeFinder;
use PHPStan\Analyser\Scope;
use PHPStan\Rules\Rule;

final class NoSwitchRule implements Rule
{
    public function getNodeType(): string
    {
        return Node\Stmt\ClassMethod::class;
    }

    /**
     * @param Node\Stmt\ClassMethod $node
     */
    public function processNode(Node $node, Scope $scope): array
    {
        if (!$scope->isInClass()) {
            return [];
        }

        $nodeFinder = new NodeFinder();
        $switchNodes = $nodeFinder->find(
            $node->stmts,
            fn (Node $node) => $node instanceof Node\Stmt\Switch_
        );

        if (count($switchNodes) === 0) {
            return [];
        }

        return ['switch 文は使用せず、match 式に置き換えてください'];
    }
}

アンケートの投票中に「このケースとあのケースは match 式で実現できないのでは？」という質問がありました。そこで「それは全部 match 式に置き換え可能ですよ」と返事をしたところ、全員一致で賛成になりました。ルール化の検討がチーム内で match 式の理解を深めるきっかけになってよかったです。

if 文のネストを禁止する

過去のPRを遡ると、「ここは早期リターンで書けるのでは？」というコメントがちらほらあったのでif 文のネストを禁止するルールを追加しました。

<?php

declare(strict_types=1);

namespace BaseInc\Modules\Crm\Tools\PhpStan\Rules;

use PhpParser\Node;
use PhpParser\Node\Stmt\If_;
use PHPStan\Analyser\Scope;
use PHPStan\Rules\Rule;

final class NoNestedIfRule implements Rule
{
    public function getNodeType(): string
    {
        return If_::class;
    }

    public function processNode(Node $node, Scope $scope): array
    {
        if (!$node instanceof If_) {
            return [];
        }

        $parent = $node->getAttribute('parent');
        while ($parent !== null) {
            if ($parent instanceof If_) {
                return ['if 文のネストは禁止されています'];
            }
            $parent = $parent->getAttribute('parent');
        }

        return [];
    }
}

またテスト界隈では「テストで if 文を使うのは良くない、それならそもそもテストケースを分けるべきだ」という意見があり、実際に過去の PR で同様の指摘があったため、テストメソッドの中で if 文があれば検出するルールも書いています。

<?php

declare(strict_types=1);

namespace BaseInc\Modules\Crm\Tools\PhpStan\Rules;

use PhpParser\Node;
use PhpParser\Node\Stmt\If_;
use PHPStan\Analyser\Scope;
use PHPStan\Rules\Rule;

final class NoIfInTestRule implements Rule
{
    public function getNodeType(): string
    {
        return If_::class;
    }

    public function processNode(Node $node, Scope $scope): array
    {
        if (!$node instanceof If_) {
            return [];
        }

        $method = $scope->getFunction();
        if ($method === null) {
            return [];
        }

        $isTestMethod = str_starts_with($method->getName(), 'test')
            || str_contains($method->getDocComment() ?? '', '@test');

        return $isTestMethod ? ['テストメソッド内で if 文は使用できません'] : [];
    }
}

Repository の参照系のメソッドの命名の OrFail を末尾に固定する

とても細かいことなのですが、Repository の参照系のメソッドの命名が「findByIdOrFail」や「findOrFailById」のように、OrFail の位置が固定されていませんでした。命名が Repository ごとに異なることは、レビューする側として気になったため、こちらもアンケートを取りました。投票の結果、末尾以外で書けないようにするルールを作りました。

<?php

declare(strict_types=1);

namespace BaseInc\Modules\Crm\Tools\PhpStan\Rules;

use PhpParser\Node;
use PhpParser\Node\Stmt\ClassMethod;
use PhpParser\Node\Stmt\Interface_;
use PHPStan\Analyser\Scope;
use PHPStan\Rules\Rule;

final class FindOrFailRule implements Rule
{
    public function getNodeType(): string
    {
        return Interface_::class;
    }

    public function processNode(Node $node, Scope $scope): array
    {
        if (!$node instanceof Interface_) {
            return [];
        }

        $target = 'OrFail';
        foreach ($node->stmts as $stmt) {
            if ($stmt instanceof ClassMethod) {
                $methodName = $stmt->name->toString();
                if (str_contains($methodName, $target) && !str_ends_with($methodName, $target)) {
                    return ['メソッド名は findOrFailByFooBar ではなく、findByFooBarOrFail のように、OrFail を末尾に記述してください'];
                }
            }
        }

        return [];
    }
}

このルールを適用した結果、なんと同一 Repository の中でも findOrFailByXxx とfindByYyyOrFail のように混在しているインターフェースを一つ見つけました。それを見た瞬間に「細かいルールだけど、これはこれでやる意味があったな」と思いました。

繰り返される些細な指摘がなくなった

もともと上記のような細かい指摘の数が多いわけではなかったので、たくさんのPRを遡ったりチームでアンケートを取ったりカスタムルールを記述してルール化するコストより、スポットで指摘するコストの方が低いと判断してルール化してはいませんでした。しかし、最近同じような指摘が増えてきてるかもと思い、今回整理することにしました。

これらのカスタムルールの導入から1ヶ月が経ち、レビューでの些細な指摘事項はなくなりました。表記揺れや細かい書き方を PHPStan の解析で検出できるようにしてから、コードレビュー時に本質的なところに意識を向けるだけで良くなり、「ここは細かいけど見逃せない！」と思う回数が減りました。細かい指摘というものはレビューされる側も小さいといえどもストレスを感じるものだと思います。また、コメントをする自分自身も気が楽になったので導入してよかったと思いました。他の人のレビューコメントを読んでいても、些細な指摘事項から生まれる些細な議論は無くなったと思います。

ただ、過去の PR をたくさん遡ることの副作用にも注意が必要です。悲しいことに、PHPStan のルールを作成する前後の期間、無意識のうちに自分のレビューコメントの指摘が細かく、厳しくなっていました。

何か最近自分のレビューコメントが過剰かもなと自分でなんとか気づけたこと、またフォローしてくれる同僚がいてよかったです。

明日は、TanaamiYukiさんの記事です。お楽しみに！

BASEプロダクトチームブログ

ネットショップ作成サービス「BASE ( https://thebase.in )」、ショッピングアプリ「BASE ( https://thebase.in/sp )」のプロダクトチームによるブログです。

180件のPRを遡って、良いレビューコメントをLintのルールに組み込んだ