refactor!: filter をカリー化して ShapeOp を導入する - #91
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
f6cab67 to
be95fbb
Compare
適用対象の配列だけを保留した操作を表す invokable クラス。 $input を受け取る callable なので そのまま呼び出すか PHP 8.5 のパイプ演算子の右辺に置ける。 issue #76 の土台。 Closure を返す形は PHPStan level 10 を通せない。 原因は 3 つとも PHPStan 側の制限で、 Closure の 変性 (引数=反変・戻り=共変) により Closure(array): array は Closure(list<V>): list<V> を満たさず (#11707)、 戻り型を絞るには Closure リテラルに phpdoc を書くしかないがそれは読まれず (#3770)、 外側の @template を返り値 Closure 内で使えない (#3075)。 __invoke はメソッドなので phpdoc が効き、 @PARAM で union を保ちつつ @phpstan-return で条件分岐でき、 メソッドテンプレート (@template TK of K / @template TV of V) で $input から型を推論できる。 番兵に入力の型パラメータを使う設計 (I is K で「入力キーを保存する」を表す) は誤りだった。 条件型の is は subsumption (部分型判定) であって同一性判定ではないため、 出力キーが入力のキーを 保存するかどうかを見分けられず型が嘘をつく。 PR #91 のレビュー指摘のうち 4 件がこれを原因とする 実際のバグだった。 専用マーカー Preserve を導入し I is Preserve / E is Preserve で判定する形へ 置き換えた。 Preserve は他の array-key に subsume されないので同じ壊れ方をしない。 条件型はキーの位置に置けない (array<(I is Preserve ? TK : I), X> は黙って array に落ちる) ため、 I is Preserve を条件型の最上位に出して TMode の 3 枝を 2 回書いている。 値の位置にはネストできる (括弧が必須で、 無いとパースエラー) ので値側は (E is Preserve ? TV : E) に畳めており、 重複して いるのは外側だけ。 @phpstan-type はテンプレート引数を取れないため、 外側の重複を型エイリアスへ 切り出すことはできない。 クラス名は戻り値の代数形を表す。 ShapeOp は list<E> | array<I, E> を返す 16 関数 (filter map map_keys collect unique take drop slice tail init sort_by take_while drop_while flat_map associate count_by) を収容する。 器の形が違う操作は別クラスにする: array<I, Shape<E>> は KeyedOp、 list<Shape<E>> は ListOp、 array{Shape<E>, Shape<E>} は PairOp、 スカラーは ValueOp (いずれも 後続 PR)。 器の形は関数を呼ぶ時点で決まるのに中身は $input を渡す時点で決まるため、 器を型 パラメータで受け取れない (higher-kinded types が無い)。 この分け方は Scala (HKT を持ちながら groupBy / partition は具体型) と Kotlin (全操作で具体型) と一致する。 $apply の @var 再アサーションは消せない。 返すべきなのは $input から推論した TK を使った型だが、 TK は __invoke のメソッドテンプレートなのでコンストラクタの @PARAM には書けず、 PHPStan は array<TK, TV> と array<K, V> を (TK of K でも) 繋げない。 理由をコード内コメントに残した。 前回の履歴では CollectionOp として追加していたが、 origin/master には出ていないので履歴を作り 直してこのクラス 1 つに畳んだ。 Refs #76
filter($input, $cb, $mode) を filter($cb, $mode)($input) に変える。 戻り値は $input を受け取る callable なので PHP 8.5 のパイプ演算子の右辺に直接置ける。 破壊的変更だが packagist 未公開 (タグ 0 本) なので BC を破棄できる段階。 移行は引数の順序を 入れ替えて $input を戻り値の呼び出しへ移すだけで済む。 // 変更前 $adults = filter($users, fn (User $u): bool => $u->age >= 20); // 変更後 $adults = filter(fn (User $u): bool => $u->age >= 20)($users); // PHP 8.5 のパイプ演算子と組み合わせる場合 $adults = $users |> filter(fn (User $u): bool => $u->age >= 20); $mode の位置と意味は変わらない。 native 戻り型は callable にして ShapeOp を phpdoc の奥に隠す。 利用者から実装クラスは見えない。 @phpstan-return ShapeOp<K, V, Preserve, Preserve, TMode> で入力のキーと値の両方を保存することを 表す。 MODE_ASSOC で array_filter への委譲をやめ foreach に統一した。 $cb の型で PHPStan が落ちるため。 挙動は同一で、 既存 5 件の期待値を無変更のまま通ることで担保している。 Closure 内の $cb の @var は callable(mixed, array-key): bool にした。 $value は $input 由来の TV、 キーも TK だが PHPStan は callable の引数を不変扱いするため証明できない。 キー側を K のままに すると __invoke 側の TK と繋がらず、 宣言が実態と食い違う。 テストは既存 5 件を curried 呼び出しに書き換え、 5 件追加した。 - 戻り値が callable であること (call_user_func 経由で実呼び出し) - 同じ操作を list と assoc に再利用できること (MODE_AUTO の判定が呼び出しごとに効いている証明) - パイプ演算子での動作 (#[RequiresPhp('>= 8.5')] + eval。 8.4 ではファイル全体のパースで 構文エラーになるため eval で包む。 8.4 では skip される) - MODE_LIST に assoc を渡すと 0 始まり連番になること - MODE_ASSOC に list を渡すと元の整数添字を保持し詰めないこと 後ろの 2 件は型の証拠として挙げている組合せなのに実行経路のテストが無かったもの。 composer.json の require.php は >=8.4 のまま。 Closure を返すだけなら 8.4 で動き、 パイプを使うかは 利用者の PHP バージョン次第。 issue #76 の第 1 弾 (残り 27 関数は形の承認後に展開)。 Refs #76
型の正しさの根拠が dumpType の目視だけだったので、 PHPStan に検査させるファイルを置く。
PHPStan 公式の \PHPStan\Testing\assertType() で期待値を文字列で明示する。
検出力があることを確認した。 期待値を壊すと落ちる:
assertType('list<int>', filter($cb)($assoc));
-> Expected type list<int>, actual: array<string, int> at tests/types/filter.php:28
受け取り側の関数に厳しい型を宣言してそこへ渡す形も検討したが、 それでは「型が広がった」ときだけ
しか落ちない。 assertType は狭まった場合も検出するのでこちらを採った。
カバーする組合せは list×AUTO / assoc×AUTO / assoc×MODE_LIST / list×MODE_ASSOC / 引数を 1 つしか
宣言しないクロージャの list と assoc の 6 本。 CollectionOp から ShapeOp へ載せ替えても推論結果は
変わらない。
callback が K を string に固定した状態で list を渡すケースは置かない。 静的型は通るが実行すると
TypeError: Argument #2 ($k) must be of type string, int given で落ちる。 実行できないコードを型
テストに置かない。 同じ理由で「キーを受け取らない callback でも値型が保たれる」というコメントも
過剰主張なので、 引数を 1 つしか宣言しないクロージャに限る旨へ直した。 内部関数の first-class
callable (is_int(...) 等) は引数の数が厳密で ArgumentCountError になり渡せない。
phpstan.neon の paths は既に tests を含むので設定変更は不要。 PHPUnit は既定 suffix Test.php で
収集するため tests/types/filter.php は対象外。
Refs #76
be95fbb to
0549691
Compare
callback を先に受け取る形と、 PHP 8.5 のパイプ演算子の右辺にそのまま置ける ことを使い方の節に足した。 CHANGELOG は置かない。 packagist 未公開でタグが 0 本なので移行対象の 利用者が存在せず、 破壊的変更の内容は refactor! コミットの本文と PR に 書けば足りる。 Refs #76 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0549691 to
7f44bda
Compare
@template TK of K の bound は @PARAM の照合では効かないため、 K に string を要求する callback に list を渡す呼び出しが静的に通り、 実行時に TypeError になっていた。 filter(fn (int $v, string $k), MODE_ASSOC)([1, 2, 3]) → Argument #2 ($key) must be of type string, int given キー型を array<TK&K, TV> と交差させて K 側の適合も検査させる。 拒否されることを @PHPStan-Ignore で固定したので、 退行すると未使用 ignore として level 10 が落ちる (&K を外すと No error with identifier argument.type is reported で落ちることを確認済み)。 list を渡す経路は塞いでいない。 array<K, V> は list を含む (list は array<int, V> の特殊形) ので、 K が int を許容する限り list リテラルも空配列も渡せる。 拒否されるのは callback が string キーしか受け取れない場合だけ。 空配列を渡す経路は non-empty 系の型を入れると壊れやすいので実行時テストで固定した。 filter のプレーンな @return も list を union に並べる形をやめ array<K, V> に揃えた。 Refs #76 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
assert([2, 4], $result) は第 2 引数が説明文 (Throwable|string|null) なので、 assertion が有効な環境では配列を渡した時点で TypeError になる。 既存の例も同じ形だったので併せて直した。 Refs #76 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1f6a090 to
d28833e
Compare
|
@claude [review] |
This comment was marked as resolved.
This comment was marked as resolved.
phantom type として型パラメータの位置に置くためだけのクラスなのに new Preserve() が通っていた。 27 関数へ展開すると型引数の位置に大量に 現れるので、 誤用の芽を private constructor で塞ぐ。 Refs #76 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docblock は「解決済み mode」と説明しているのに型が Mode で MODE_AUTO を 許していた。 Mode::check_mode() の戻り型は MODE_LIST|MODE_ASSOC なので そのまま書ける。 絞ると filter の else が「MODE_ASSOC 確定」だと PHPStan にも読めるようになり、 MODE_AUTO が漏れた場合に暗黙で assoc 扱いされる形より契約が明確になる。 Refs #76 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
カリー化のついでに array_filter($input, $cb, ARRAY_FILTER_USE_BOTH) を 手書き foreach に置き換えていたが、 カリー化と無関係な変更だった。 C レベルのループを PHP レベルに落とすため assoc の大きい配列で遅くなる。 非カリー化版と同じ「MODE_ASSOC は array_filter で早期 return」の構造に戻した。 挙動の等価性は実測で確認 (list/assoc x AUTO/LIST/ASSOC の 4 経路 + 空配列)。 Refs #76 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
キーを捨てるのは出力側だけという契約が未固定だった。 既存の MODE_LIST x assoc テストは値のみの callback、 キーを見るテストは list 入力で、 この組み合わせが抜けていた。 Refs #76 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
autoload-dev の PSR-4 は Oyashiro846\Phollection\ -> tests/ なので、 名前空間 ...\Types に対応するディレクトリは tests/Types (大文字 T)。 小文字のままだと将来ここにクラスを置いたとき case-sensitive な FS で オートロードが壊れる。 Refs #76 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @Javakky's task in 5m 27s —— View job レビュー結果(3 巡目)
前回指摘のうち 5 件が対応済みであることを確認しました。特に 以下、新規に見つけたものを重要度順に挙げます。 🔴 High:
|
|
PHPStanのbound照合が効いていないことについては、Keyと同じくValue側も交差型で強制照合して、テストで拒否できるかどうかを確認しましょう。
|
概要
issue #76 (PHP 8.5 のパイプ演算子対応) の第 1 弾。
filter1 関数だけをカリー化し、型表現の器ShapeOpを導入します。1 関数に絞ったのは型表現を実物でレビューしていただきたいからです。承認後に残り 27 関数へ展開します。packagist 未公開 (タグ 0 本) なので BC を破棄できる段階です。
破壊的変更
$modeの意味は変わりません。引き続き$callbackの直後に指定しますが、引数番号は第 3 引数から第 2 引数に変わります。CHANGELOG.mdは置いていません。packagist 未公開でタグが 0 本なので移行対象の利用者が存在せず、破壊的変更の内容はrefactor!:コミットの本文とこの PR に書けば足ります。動作確認
vendor/bin/phpunit tests(PHP 8.4)Tests: 111, Assertions: 119, Skipped: 1(skip はパイプテストのみ)vendor/bin/phpstan analyse(level 10)[OK] No errors(@phpstan-ignoreは負のテスト 1 箇所のみ)vendor/bin/php-cs-fixer fixFixed 0 of 34 filesphp:8.5-cli)[1,2,3,4] |> filter(fn ($v) => $v % 2 === 0)→[2, 4]型は
tests/types/filter.phpで\PHPStan\Testing\assertType()により固定しています。このテストが検出力を持つことを確認済みです。 28 行目の期待値を
array<string, int>→list<int>に壊すとExpected type list<int>, actual: array<string, int>で落ち、戻すと[OK]に復帰します。設計
なぜ Closure でなく invokable オブジェクトなのか
@return Closure(...)は 9 パターン試して level 10 を通せませんでした。Closure の変性は戻りが共変なのでClosure(array): arrayがClosure(list<V>): list<V>を満たせず (#11707 仕様)、戻り型を絞るための Closure リテラルの phpdoc は読まれず (#3770 open)、外側の@templateも返り値 Closure 内で使えません (#3075 open)。__invokeはメソッドなので phpdoc が効き、@paramで union を保ちつつ@phpstan-returnで条件分岐でき、メソッドテンプレートで$inputから型を推論できます。利用者からは隠れます — native 戻り型は
callable、プレーンな@returnもcallable(...)で、@phpstan-returnだけがShapeOpを知り、クラスには@internalが付いています。クラスは戻り値の「代数形」ごとに 5 個
操作の意味 (値を保存する / 変換する) で分けてはいけません。 最初はそう分けて 6 クラスになりましたが、
filterとmapは代数形で見ると両方Shape<E>で同じ器です。Shape= 入力と同じ器 (Mode と入力に応じてlistかarray<K, ·>) と置くと 5 形に尽きます。list<E>|array<I, E>ShapeOp(この PR)array<I, Shape<E>>KeyedOplist<Shape<E>>ListOparray{Shape<E>, Shape<E>}PairOpRValueOp全 28 関数ぶんの型を実測して確定しました。 表を書き直す原因になったのは 3 つです。
flat_mapはキーを保存せずarray<R, E>を返す (Mode 分岐あり) →ShapeOpのIで賄うchunk/windowedは内側がnon-empty-listreduceは既に(R is null ? (?V) : R)の番兵を持ち、find_optionの?Vとany/all/noneのboolも同じ式から出るKeyedOp(array<I, ·>) とListOp(list<·>) は外側の器しか違いませんが統合できません。器を型パラメータで受けるには higher-kinded types が必要で、器の形は関数を呼ぶ時点で決まるのに中身 (TK/TV) は$inputを渡す時点で決まるため「あとで埋める枠」を渡せません。Preserveマーカーで「入力の型をそのまま返す」を表すPreserveは実行時に現れない phantom type で、(E is Preserve ? TV : E)の分岐スイッチです。「入力の値型」はTV(メソッドテンプレート) でしか書けずE(クラステンプレート) に渡せないため、値ではなく「保存せよ」という指示を型で渡します。nullを番兵にできないのは、Eが値型なのでmap(fn () => null)やcollectの?Eでnullが正当に来るためです。マーカー型の慣習としては C++ の tag type (std::in_place_t/std::nullopt_t— 意味をそのまま名詞化した空の型) に倣っています。実測で確定した PHPStan の境界
list<(E is Preserve ? TV : E)>array<(I is Preserve ? TK : I), X>arrayに落ちる@phpstan-type ShapeAlias<TK2, TV2> = …Unexpected token "<")。条件型の共通部分を切り出せない@template TK of Kの bound は@paramの照合で効くかarray<TK, TV>と書いてもK = stringの callback に list を渡せてしまう。array<TK&K, TV>と交差させるとK側の適合も検査されるキーの位置に置けないため
ShapeOpの条件型は 8 枝あります (I is Preserveを最上位に出してTModeの 3 枝を 2 回書く)。値側は(E is Preserve ? TV : E)で畳めているので、重複しているのは外側だけです。レビュー指摘への対応
3 巡目 (High 1 / Medium 3 / Low 6)
🔴 High: PHP 8.5 の CI job がない → #92 で対応しました (別ブランチなのでこの PR の差分には現れません)。
lint/checks (8.4)/checks (8.5)の 3 job 構成で、パイプのテストが 8.5 で実際に走ります。🟡 Medium:
$applyの契約をShapeOpに集約する提案 → 採りませんでした。array_values()を一律で通すか foreach で$result[] =するかは関数ごとに最適が違う (array_sliceを使う関数は前者、foreach で組む関数は後者が速い) ため、実装者の判断に委ねます。ご指摘の「型は嘘をつくのに CI は緑」という問題はテスト側で担保します —Modeを取る関数は 4 経路 (AUTO×list /AUTO×assoc /LIST×assoc /ASSOC×list) すべてをassertSameで固定することを共通規約にしました。assertSameはキーの順序・型まで見るので、MODE_LISTの期待値を list リテラルで書けばキーが残った時点で落ちます。なお
assertTrue(array_is_list($result))を足す案は実測で捨てました。PHPStan が宣言された型からarray{2, 3}と narrowing してfunction.alreadyNarrowedTypeで level 10 が落ちます。型が嘘をついていれば narrowing も嘘になるので、静的解析下では検出器になりません。🟡 Medium:
\Closureを満たさない → 実害はほぼありませんでした。「回避策はどちらも条件型を落として
arrayに退化する」というご指摘は実測では否定されました。private \Closure $opに入れたい場合はfilter($cb)(...)と書くだけで、型精度の代償なく解決します。private callable $opが書けないのは PHP の仕様 (Property cannot have type callable) でShapeOpとは無関係です。README への追記は次のコミットで入れます。🟢 Low (5 件対応、1 件は本文で回答)
new Preserve()が通るprivate function __construct()で塞ぎました$applyの第 2 引数をMode::MODE_LIST|MODE_ASSOCに絞れるfilterのelseが「ASSOC 確定」と PHPStan に読めるようになりましたarray_filter→ foreach はカリー化と無関係array_filterに戻しました。 挙動の等価性は 4 経路 + 空配列で実測済みtests/types/の置き場tests/Types/にリネームしました (case-insensitive FS ではgit mvを 2 段階でやる必要がありました)head_option/last_option/intersectがないdiff/zip/zip_withはオーナー判断でカリー化対象外です —head_option/last_optionは引数が$inputだけなのでhead_option(...)で既にパイプに置けます。intersect/diff/zip/zip_withは配列を 2 つ取り、どちらを残すかが非自明なため現行の形を維持します。表は「カリー化する 28 関数」の内訳なので 3 関数が現れないのが正しい状態です。tail()/init()/unique()の空括弧呼び出し ($x |> tail()) は意図した形です (追加引数がModeだけの関数もカリー化する、というオーナー決定)「8 枝は許容範囲」への同意に加えていただいた提案 — 「キー位置の phpdoc は失敗しても沈黙で
arrayに退化するので、ShapeOpに載せる関数はすべてtests/types/<fn>.phpのassertTypeを必須にする」は、展開 PR の完了条件として共通規約に入れました。期待値をわざと壊して落ちることの確認まで含めています。2 巡目 (3 件すべて対応)
list<TV>がKの制約を迂回しているKは$callbackから正しく推論されています (dumpTypeはShapeOp<string, int, Preserve, Preserve, MODE_ASSOC>)。@template TK of Kの bound が@paramの照合で効いていないのが原因で、@param array<TK, TV>に変えても拒否されませんでした (実測)。array<TK&K, TV>と交差させてK側の適合も検査させることでParameter #1 $input expects array<string, int>, list<int> given.が出るようになりました。filter()のプレーンな@returnもcallable(array<K, V>)に揃えました。list を渡す経路は塞いでいません —array<K, V>は list を含むので、Kがintを許容する限り list リテラル (filter($cb)([1, 2, 3])→list<1|2|3>) も空配列 (filter($cb)([])→list<*NEVER*>) も通ります (実測)。拒否されるのは callback がstringキーしか受け取れない場合だけです。空配列を渡す経路はnon-empty系の型を入れると壊れやすいので実行時テストで固定しましたassert()が壊れているassert($result === [2, 4])に修正。既存の例も同じ形だったので併せて直しました$modeの位置の記述が不正確負のテストを
@phpstan-ignoreで固定しました。 拒否できなくなると未使用 ignore として level 10 が落ちるので、これが退行の検出器になります。&Kを外すとで落ち、戻すと
[OK]に復帰することを確認しました。1 巡目 (6 件すべて対応)
うち 4 件は実測で本物のバグと確認しました。
I is Kでキー保存を表すのは危険isは同一性判定でなく subsumption なのでK = array-keyにI = stringを渡すとstring is array-keyが真になり、dumpTypeはarray<int<0, max>, int>と言うのに実行時は文字列キーが返っていました。専用マーカーPreserveに置換filter(fn (object $v) => true)($users)がlist<object>に広がっていました。(E is Preserve ? TV : E)で解決filter(fn (int $v, string $k), MODE_ASSOC)([1,2,3])は静的には通りますが実行するとTypeError。削除しました@varがarray<I, E>を落としているarray<I&array-key, mixed>を追加。callable(mixed, K)もTKと繋がらないのでarray-keyに修正is_int(...)のような内部関数の first-class callable はArgumentCountErrorになるため、「引数を 1 つしか宣言しないクロージャ」に限定して明記CHANGELOG.mdはリリース前で移行対象の利用者がいないため置かないcomposer.jsonは触っていません (ShapeOp/Preserveは PSR-4 で解決されます)。見てほしい点
arrayまたはnull」は__invokeの戻りがarrayなので守られていますが、filter()自身の戻りはcallableになります。「オブジェクトを生成してチェーンしていくスタイルは採らない」は->map()->filter()を指していて$x |> map($f) |> filter($g)は演算子による接続なので別物だと解釈していますが、この解釈が妥当かの判断をお願いします。思想を優先して見送る選択もあります (その場合パイプは$x |> (fn ($a) => filter($a, $cb))と書けます — RFC 自身がこの形を例示しています)Iを捨ててmap_keys/flat_map/associate/count_by用に別クラスを立てれば 3 枝になりますが、クラスが 1 つ増えます$applyの契約が型で守れていない —$applyに「キーを変えてしまう実装」を渡しても型エラーになりません。Closure の中身が宣言型と照合されない (#3770 と同根) ためで、契約はテストで担保していますtests/FilterTest.phpの既存分は snake_case のままです。「無関係なリネームをしない」を優先しましたが、揃えるなら別コミットで切れますマージ順のお願い
カリー化は全関数のシグネチャに触るので、open な PR すべてと衝突します。この順序だと手戻りがありません。
composer.jsonの衝突が消えます3 と 4 は逆にできません。 公開後のシグネチャ変更は BC break になります。
Refs #76
🤖 Generated with Claude Code