Skip to content

feat: chunk_by に Mode を追加して assoc 対応にする - #90

Open
Javakky wants to merge 1 commit into
masterfrom
javakky/chunk-by-mode
Open

feat: chunk_by に Mode を追加して assoc 対応にする#90
Javakky wants to merge 1 commit into
masterfrom
javakky/chunk-by-mode

Conversation

@Javakky

@Javakky Javakky commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

概要(What / Why)

chunk_byMode を追加し、assoc 入力に対応させます。あわせて $callback にキーを第 2 引数で渡します。

chunk_by#75 でマージしたときに「associative array は順序保証が無いため非対応」と書きましたが、
この理由は誤りでした。PHP の配列は挿入順を保持するので、assoc でも「隣接する要素」は明確に定義できます。撤回します。

変更点

  • src/chunk_by.php
    • Mode $mode = Mode::MODE_AUTO を追加。各 chunk の内部が mode に従う (外側は常に list)
    • 入力を list<V>|array<K, V> に拡張
    • $callbackcallable(V, K): R にしてキーを第 2 引数で渡す (他の関数と揃える)
    • phpdoc の「順序保証が無いため非対応」を撤回し、Mode の説明と assoc の例に差し替え
  • tests/ChunkByTest.php — 既存 7 ケースは無変更のまま 6 ケース追加

動作確認

  • 手動:
    • chunk_by(['a' => 1, 'b' => 1, 'c' => 2], fn ($v) => $v)[['a' => 1, 'b' => 1], ['c' => 2]] (AUTO で assoc)
    • 同じ入力に Mode::MODE_LIST[[1, 1], [2]]
  • 自動:
    • vendor/bin/php-cs-fixer fix --dry-run --diff / vendor/bin/phpstan analyse -c phpstan.neon / vendor/bin/phpunit tests
    • 結果:OK (110 tests, 120 assertions) / PHPStan level 10 [OK] No errors / CS Fixer Fixed 0 of 31 files

補足(任意)

互換性

$callback に第 2 引数が増えますが、PHP は宣言より多い引数を無視するので fn ($v) => $v 形の既存呼び出しはそのまま動きます。$mode は既定値ありなので既存呼び出しは無変更で通り、既存の 7 ケースは 1 件も落ちていません

assoc 側の戻り値に non-empty を付けていない理由

空の chunk は作らないので list<non-empty-array<K, V>> と書きたいところですが、これを入れると
空配列リテラルを直接渡す呼び出しが PHPStan level 10 で落ちます (実測)。

chunk_by([], fn ($v) => $v);   // function.unresolvableReturnType

non-empty-array<never, never> が unresolvable になるためです (non-empty-list<never> は問題なし — list は値型しか持たないため)。list 側の non-empty-list<V> は維持し、assoc 側だけ array<K, V> に緩めました。理由は phpdoc にも書いてあります。

型注釈なしの chunk_by([], ...) を直接書く回帰テストを入れてあります。

Closes なし (対応 issue はありません。#75 の後追い改善です)

🤖 Generated with Claude Code

chunk_by の入力を list 限定から associative array まで広げ、 chunk 内部のキーの扱いを Mode で選べるようにした。 外側 (chunk の並び) は従来どおり常に list。

PR #75 では "associative array は順序保証が無いため非対応" と書いていたが、 PHP の配列は挿入順を保持するので assoc でも隣接関係は定義できる。 この理由は誤りだったので撤回する。

あわせて $callback に第 2 引数でキーを渡すようにし、 他の関数と揃えた。 PHP は宣言より多い引数を無視するため既存の呼び出しはそのまま動く。

戻り値の assoc 側は non-empty-array を付けていない。 non-empty-array<never, never> が chunk_by([], ...) の呼び出しで unresolvable になり、 利用者が空配列リテラルを渡せなくなるため。 list 側の non-empty-list は維持している。
@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown

Claude finished @Javakky's task in 4m 32s —— View job


コードレビュー: feat: chunk_by に Mode を追加して assoc 対応にする

  • PR 差分の取得と確認
  • src/Mode.php / 既存 Mode 対応関数 (filter, map, unique, group_by) との規約比較
  • 型注釈・ロジック・テストの検証
  • PHPStan / PHPUnit / CS Fixer のローカル実行実行できませんでした(下記「検証できなかったこと」参照)
  • レビュー結果の投稿

総評

方針・実装ともに妥当です。Mode::check_mode() を使った list/assoc の分岐、条件付き戻り値型のネスト($mode$input is list の順)、@param list<V>|array<K, V> という形は filter.php / map.php / unique.php と完全に揃っており、リポジトリの既存規約から逸脱していません。

#75 の「順序保証が無いため非対応」という記述の撤回も正しい判断です。PHP の配列は挿入順を保持するので「隣接」は明確に定義できます。

ロジックも追いました。$prevChunkKey を毎回代入する形に変えていますが、旧実装(差分がある時だけ代入)と結果は同値です(一致したときは既に同じ値が入っているため)。むしろ「直前の chunk キー」という変数名の意味と実装が一致して読みやすくなっています。$callbacknull を返すケースでも $chunks !== [] ガードがあるので初回に誤って merge されることはありません。

non-empty を assoc 側だけ外した判断と、その理由を phpdoc に残したうえで回帰テストまで書いているのは良いです。

以下、気になった点を重要度順に。Blocking はありません。


1. phpdoc の継続行が @param にぶら下がっている(中)

src/chunk_by.php:31-34

 * @param callable(V, K): R $callback  各要素を chunk キーに変換する。 戻り値は `===` で比較される。
 * 空の chunk は作らないが、 assoc 側は `non-empty-array<never, never>` が
 * `chunk_by([], ...)` の呼び出しで unresolvable になるため `non-empty` を付けていない。
 *
 * @return list<non-empty-list<V>>|list<array<K, V>>

この 3 行は内容としては戻り値型の説明ですが、phpdoc のパース規則上は直前の @param $callback の説明の続きとして解釈されます。IDE や phpDocumentor では「$callback とは何か」の欄にこの文章が表示されてしまいます。

@return の直前か、冒頭の説明ブロックの末尾へ移動するのが良いです。

Fix this →


2. 互換性の説明が「内部関数の文字列コールバック」で成立しない(中・PR 本文の記述の話)

PR 本文の

PHP は宣言より多い引数を無視するので fn ($v) => $v 形の既存呼び出しはそのまま動きます

これはユーザー定義関数・クロージャに限った話です。PHP の内部関数は余分な引数を無視せず ArgumentCountError を投げます。

chunk_by($words, 'strlen');       // 変更前: 動く
                                   // 変更後: strlen(): expects exactly 1 argument, 2 given
chunk_by($nums, 'abs');            // 同上
chunk_by($items, strlen(...));     // first-class callable でも同じ

filter / map / group_by が既に callable(V, K) を採用しているので、リポジトリ全体の方針としては chunk_by を揃えるのが正しいと思います。実害も、まだタグが 1 つも切られていない(git tag が空)ので実質ゼロです。

ただ PR 本文の主張としては不正確なので、「クロージャ/ユーザー定義関数は無変更で通るが、内部関数を文字列コールバックで渡していた場合は引数エラーになる」と書き換えておくと後から履歴を読む人が混乱しません。phpdoc 側に一言注意書きを足すのもありです。

Fix this →


3. テストギャップ: list 入力での第 2 引数(小)

testChunkByPassesKeyAsSecondArgumenttests/ChunkByTest.php:89)は assoc の文字列キーしか検証していません。list 入力のときに int の添字が第 2 引数へ渡ることを固定するケースがあると、今回の変更の主眼(キーを渡す)が両方の入力形状でカバーされます。

public function testChunkByPassesIntIndexAsSecondArgumentOnList(): void
{
    $this->assertSame(
        [[10, 20], [30, 40]],
        chunk_by([10, 20, 30, 40], fn (int $v, int $k): bool => $k < 2),
    );
}

Fix this →


4. mode 判定がループ内で 2 箇所に分散している(nit)

src/chunk_by.php:52-66 は「chunk を切るか」と「キーを保つか」という直交する 2 条件が if / elseif / else に畳み込まれていて、elseif ($mode === Mode::MODE_ASSOC) が「新規 chunk かつ assoc」を意味する形になっています。切れ目判定を先に済ませると重複が消えます。

foreach ($input as $key => $value) {
    $chunkKey = $callback($value, $key);

    if ($chunks === [] || $prevChunkKey !== $chunkKey) {
        $chunks[] = [];
    }

    $last = \count($chunks) - 1;

    if ($mode === Mode::MODE_ASSOC) {
        $chunks[$last][$key] = $value;
    } else {
        $chunks[$last][] = $value;
    }

    $prevChunkKey = $chunkKey;
}

ただし注意点があります。この形は一瞬 [](空 chunk)を作るため、PHPStan level 10 が list<non-empty-list<V>> を証明できなくなる可能性があります。現状の実装は level 10 を通っているとのことなので、通ることを確認できない限り現状維持で問題ありません。あくまで任意です。

Fix this →


5. @param Mode $mode の説明がない(nit)

src/unique.php:14@param Mode $mode MODE_LIST のときは… と各 case を明示していますが、chunk_by は冒頭の説明ブロックで書いています(filter / map@param なし)。リポジトリ内で既に割れている慣習なので必須ではありませんが、unique 側に揃えると @param だけ見れば分かる状態になります。


6. group_by との非対称(本 PR の対象外・別 PR 提案)

src/group_by.php:22 は同じ「list なら詰める / assoc ならキー維持」の判定を array_is_list() で内部固定していて、Mode を受け取りません。今回 chunk_by が Mode 化されたことで、Mode を持たない関数が group_by だけになりました。本 PR のスコープ外ですが、追随の別 PR があると全関数が揃います。


7. MODE_ASSOC + list リテラル時の K 推論(確認推奨・既存関数と同じ形)

testChunkByOnListWithAssocModeKeepsOriginalIndexes のように list リテラルへ MODE_ASSOC を渡すと、list<V>|array<K, V> の union に対する推論で Knever へ潰れて list<array<never, V>> になっていないか、一度 \PHPStan\dumpType() で確認しておくと安心です。assertSame()mixed を受けるためテストでは検出できません。

これは unique / map / filter と同じ注釈の形なので本 PR で悪化したものではなく、既に踏んでいるなら既存の問題です。今回 non-empty-array<never, never> の unresolvable を踏んでいることからも、同系統の推論が絡んでいそうなので念のため。


セキュリティ

問題なしです。外部入力・I/O・動的実行・secrets の扱いはありません。$callback はユーザー提供の callable をそのまま呼ぶだけで、chunk_by 側が新たな信頼境界を作っていません。入力配列を破壊しないことは testChunkByDoesNotMutateInput で担保されています。


検証できなかったこと

この実行環境では composer install および vendor/bin/* の実行が権限で拒否されたため、PHPStan / PHPUnit / CS Fixer をこちらで走らせて確認できていません。PR 本文に記載の

  • OK (110 tests, 120 assertions)
  • PHPStan level 10 [OK] No errors
  • CS Fixer Fixed 0 of 31 files

は作者の報告値として受け取っており、私が再現したものではありません。上記の型推論に関する指摘(4・7)も静的な読みに基づく推測です。こちらで実行させたい場合は --allowedToolsBash(composer install)Bash(vendor/bin/*) を追加してください。


ブランチ: javakky/chunk-by-mode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant