From 77201baf45cd837f084d74392b456b1c815d6ece Mon Sep 17 00:00:00 2001 From: AlexandruGG Date: Tue, 22 Jun 2021 19:44:47 +0100 Subject: [PATCH] Fixes & improvements --- docs/pages/api.rst | 2 +- docs/pages/code/operations/distinct.php | 35 +++++++++- spec/loophp/collection/CollectionSpec.php | 12 ++-- src/Collection.php | 6 +- src/Contract/Operation/Distinctable.php | 6 +- src/Operation/Distinct.php | 53 ++++++++------ tests/static-analysis/distinct.php | 85 ++++++++++++++--------- 7 files changed, 132 insertions(+), 67 deletions(-) diff --git a/docs/pages/api.rst b/docs/pages/api.rst index 4e1c4bb68..3710cb7be 100644 --- a/docs/pages/api.rst +++ b/docs/pages/api.rst @@ -560,7 +560,7 @@ how values are accessed and compared to each other. The first parameter is the comparator. This is a curried function which takes first the left part, then the right part and then returns a boolean. -The second parameter is the accessor. This binary function take the value and +The second parameter is the accessor. This binary function takes the value and the key of the current iterated value and then return the value to compare. This is useful when you want to compare objects. diff --git a/docs/pages/code/operations/distinct.php b/docs/pages/code/operations/distinct.php index f8d7995c4..947ec0022 100644 --- a/docs/pages/code/operations/distinct.php +++ b/docs/pages/code/operations/distinct.php @@ -14,11 +14,11 @@ include __DIR__ . '/../../../../vendor/autoload.php'; -// Example 1 -> Using the default callback, with scalar values +// Example 1 -> Using the default callbacks, with scalar values $collection = Collection::fromIterable(['a', 'b', 'a', 'c']) ->distinct(); // [0 => 'a', 1 => 'b', 3 => 'c'] -// Example 2 -> Using one custom callback, with object values +// Example 2 -> Using a custom comparator callback, with object values final class User { private string $name; @@ -46,7 +46,36 @@ public function name(): string static fn (User $left): Closure => static fn (User $right): bool => $left->name() === $right->name() ); // [0 => User, 1 => User, 3 => User] -// Example 3 -> Using two custom callbacks, with object values +// Example 3 -> Using a custom accessor callback, with object values +final class Person +{ + private string $name; + + public function __construct(string $name) + { + $this->name = $name; + } + + public function name(): string + { + return $this->name; + } +} + +$users = [ + new Person('foo'), + new Person('bar'), + new Person('foo'), + new Person('a'), +]; + +$collection = Collection::fromIterable($users) + ->distinct( + null, + static fn (Person $person): string => $person->name() + ); // [0 => Person, 1 => Person, 3 => Person] + +// Example 4 -> Using both accessor and comparator callbacks, with object values final class Cat { private string $name; diff --git a/spec/loophp/collection/CollectionSpec.php b/spec/loophp/collection/CollectionSpec.php index 194857602..6236a94bd 100644 --- a/spec/loophp/collection/CollectionSpec.php +++ b/spec/loophp/collection/CollectionSpec.php @@ -765,6 +765,10 @@ public function it_can_distinct(): void ->distinct() ->shouldIterateAs([0 => 1, 2 => 2, 4 => 3, 6 => $stdclass]); + $this::fromIterable(['foo' => 'f', 'bar' => 'b', 'baz' => 'f']) + ->distinct() + ->shouldIterateAs(['foo' => 'f', 'bar' => 'b']); + $cat = static fn (string $name) => new class($name) { private string $name; @@ -792,21 +796,21 @@ public function name(): string $this::fromIterable($cats) ->distinct( - static fn ($left) => static fn ($right) => $left->name() === $right->name() + static fn (object $left) => static fn (object $right) => $left->name() === $right->name() ) ->shouldIterateAs([$cat1, $cat2, $cat3]); $this::fromIterable($cats) ->distinct( - static fn ($left) => static fn ($right) => $left === $right, - static fn ($cat): string => $cat->name() + static fn (string $left) => static fn (string $right) => $left === $right, + static fn (object $cat): string => $cat->name() ) ->shouldIterateAs([$cat1, $cat2, $cat3]); $this::fromIterable($cats) ->distinct( null, - static fn ($cat): string => $cat->name() + static fn (object $cat): string => $cat->name() ) ->shouldIterateAs([$cat1, $cat2, $cat3]); } diff --git a/src/Collection.php b/src/Collection.php index 460194aae..bac8c05ef 100644 --- a/src/Collection.php +++ b/src/Collection.php @@ -277,13 +277,13 @@ public function distinct(?callable $comparatorCallback = null, ?callable $access $comparatorCallback ??= /** - * @param mixed $left + * @param T $left * - * @return Closure(mixed): bool + * @return Closure(T): bool */ static fn ($left): Closure => /** - * @param mixed $right + * @param T $right */ static fn ($right): bool => $left === $right; diff --git a/src/Contract/Operation/Distinctable.php b/src/Contract/Operation/Distinctable.php index 69a2d440c..eec516e3e 100644 --- a/src/Contract/Operation/Distinctable.php +++ b/src/Contract/Operation/Distinctable.php @@ -19,8 +19,10 @@ interface Distinctable { /** - * @param null|callable(mixed): (Closure(mixed): bool) $comparatorCallback - * @param null|callable(T, TKey): mixed $accessorCallback + * @template U + * + * @param null|callable(U): (Closure(U): bool) $comparatorCallback + * @param null|callable(T, TKey): U $accessorCallback * * @return Collection */ diff --git a/src/Operation/Distinct.php b/src/Operation/Distinct.php index 0339c9f0e..2d15fa7aa 100644 --- a/src/Operation/Distinct.php +++ b/src/Operation/Distinct.php @@ -22,47 +22,60 @@ final class Distinct extends AbstractOperation { /** - * @return Closure(callable(mixed): (Closure(mixed): bool)): Closure(callable(T, TKey): mixed): Closure(Iterator): Generator + * @template U + * + * @return Closure(callable(U): Closure(U): bool): Closure(callable(T, TKey): U): Closure(Iterator): Generator */ public function __invoke(): Closure { return /** - * @param callable(mixed): (Closure(mixed): bool) $comparatorCallback + * @param callable(U): (Closure(U): bool) $comparatorCallback * - * @return Closure(callable(T, TKey): mixed): Closure(Iterator): Generator + * @return Closure(callable(T, TKey): U): Closure(Iterator): Generator */ static fn (callable $comparatorCallback): Closure => /** - * @param callable(T, TKey): mixed $accessorCallback + * @param callable(T, TKey): U $accessorCallback * * @return Closure(Iterator): Generator */ static function (callable $accessorCallback) use ($comparatorCallback): Closure { + /** + * @param callable(T, TKey): U $accessorCallback + * + * @return Closure(callable(U): Closure(U): bool): Closure(list, array{0: TKey, 1: T}): list + */ $foldLeftCallbackBuilder = - static fn (callable $accessorCallback): Closure => static fn (callable $comparatorCallback): Closure => + static fn (callable $accessorCallback): Closure => /** - * @param list $seen - * @param array{0: TKey, 1: T} $value + * @param callable(U): (Closure(U): bool) $comparatorCallback + * + * @return Closure(list, array{0: TKey, 1: T}): list */ - static function (array $seen, array $value) use ($accessorCallback, $comparatorCallback): array { - $isSeen = false; - $comparator = $comparatorCallback($accessorCallback($value[1], $value[0])); + static fn (callable $comparatorCallback): Closure => + /** + * @param list $seen + * @param array{0: TKey, 1: T} $value + */ + static function (array $seen, array $value) use ($accessorCallback, $comparatorCallback): array { + $isSeen = false; + $comparator = $comparatorCallback($accessorCallback($value[1], $value[0])); - foreach ($seen as $item) { - if (true === $comparator($accessorCallback($item[1], $item[0]))) { - $isSeen = true; + foreach ($seen as $item) { + if (true === $comparator($accessorCallback($item[1], $item[0]))) { + $isSeen = true; - break; + break; + } } - } - if (false === $isSeen) { - $seen[] = $value; - } + if (false === $isSeen) { + $seen[] = $value; + } - return $seen; - }; + return $seen; + }; /** @var Closure(Iterator): Generator $pipe */ $pipe = Pipe::of()( diff --git a/tests/static-analysis/distinct.php b/tests/static-analysis/distinct.php index 5c953f84b..d53ce304e 100644 --- a/tests/static-analysis/distinct.php +++ b/tests/static-analysis/distinct.php @@ -15,51 +15,68 @@ /** * @param CollectionInterface $collection */ -function distinct_checkList(CollectionInterface $collection): void +function distinct_checkIntList(CollectionInterface $collection): void { } /** - * @param CollectionInterface $collection + * @param CollectionInterface $collection */ -function distinct_checkMap(CollectionInterface $collection): void +function distinct_checkObjectList(CollectionInterface $collection): void { } -function distinct_checkIntElement(int $value): void -{ -} -function distinct_checkNullableInt(?int $value): void -{ -} -function distinct_checkStringElement(string $value): void -{ -} -function distinct_checkNullableString(?string $value): void +/** + * @param CollectionInterface $collection + */ +function distinct_checkMap(CollectionInterface $collection): void { } -distinct_checkList(Collection::fromIterable([1, 2, 3, 1])->distinct()); -distinct_checkMap(Collection::fromIterable(['a' => 'foo', 'b' => 'bar', 'c' => 'baz', 'd' => 'bar'])->distinct()); +$cat = static function (string $name): stdClass { + $instance = new stdClass(); + $instance->name = $name; + + return $instance; +}; + +$cats = [ + $cat1 = $cat('izumi'), + $cat2 = $cat('nakano'), + $cat3 = $cat('booba'), + $cat3, +]; + +$accessor = static fn (stdClass $object): string => $object->name; +$stringComparator = static fn (string $left): Closure => static fn (string $right): bool => $left === $right; +$objectComparator = static fn (stdClass $left): Closure => static fn (stdClass $right): bool => $left->name === $right->name; + +distinct_checkIntList(Collection::fromIterable([11, 12, 11, 13])->distinct()); +distinct_checkMap(Collection::fromIterable(['foo' => 'f', 'bar' => 'b', 'baz' => 'f'])->distinct()); + +distinct_checkObjectList(Collection::fromIterable($cats)->distinct()); +distinct_checkObjectList(Collection::fromIterable($cats)->distinct($objectComparator)); +distinct_checkObjectList(Collection::fromIterable($cats)->distinct($stringComparator, $accessor)); +distinct_checkObjectList(Collection::fromIterable($cats)->distinct(null, $accessor)); -distinct_checkList(Collection::empty()->distinct()); -distinct_checkMap(Collection::empty()->distinct()); +// VALID failures -distinct_checkNullableInt(Collection::fromIterable([1, 2, 3, 1])->distinct()->current()); -distinct_checkNullableString(Collection::fromIterable(['foo' => 'bar', 'baz' => 'bar'])->head()->current()); +// `distinct` does not change the collection types TKey, T +/** @psalm-suppress InvalidScalarArgument @phpstan-ignore-next-line */ +distinct_checkIntList(Collection::fromIterable(['a', 'b', 'c'])->distinct()); +/** @psalm-suppress InvalidScalarArgument @phpstan-ignore-next-line */ +distinct_checkMap(Collection::fromIterable(['foo' => 1, 'bar' => 2, 'baz' => 'f'])->distinct()); -// This retrieval method doesn't cause static analysis complaints -// but is not always reliable because of that. -distinct_checkIntElement(Collection::fromIterable([1, 2, 3, 1])->distinct()->all()[0]); -distinct_checkStringElement(Collection::fromIterable(['a' => 'foo', 'b' => 'bar', 'c' => 'baz', 'd' => 'bar'])->distinct()->all()['a']); -distinct_checkStringElement(Collection::fromIterable(['a' => 'foo', 'b' => 'bar', 'c' => 'baz', 'd' => 'bar'])->distinct()->all()['b']); +// mixing object comparator parameter types +$objectComparator = static fn (stdClass $left): Closure => static fn (string $right): bool => $left->name === $right; +/** @psalm-suppress InvalidArgument @phpstan-ignore-next-line */ +distinct_checkObjectList(Collection::fromIterable($cats)->distinct($objectComparator)); -// VALID failures - `current` returns T|null -/** @psalm-suppress PossiblyNullArgument @phpstan-ignore-next-line */ -distinct_checkIntElement(Collection::fromIterable([1, 2, 3, 1])->distinct()->current()); -/** @psalm-suppress PossiblyNullArgument @phpstan-ignore-next-line */ -distinct_checkStringElement(Collection::fromIterable(['foo' => 'bar', 'bar'])->distinct()->current()); +// using wrong type parameter for accessor callback +$accessor = static fn (string $object): string => $object; +/** @psalm-suppress InvalidArgument */ +distinct_checkObjectList(Collection::fromIterable($cats)->distinct(null, $accessor)); -// VALID failures - these keys don't exist -/** @psalm-suppress InvalidArrayOffset */ -distinct_checkIntElement(Collection::fromIterable([1, 2, 3, 1])->distinct()->all()[4]); -/** @psalm-suppress InvalidArrayOffset @phpstan-ignore-next-line */ -distinct_checkStringElement(Collection::fromIterable(['foo' => 'bar', 'baz' => 'bar'])->distinct()->all()['plop']); +// comparator parameter types need to match accessor return type +$accessor = static fn (stdClass $object): string => $object->name; +$objectComparator = static fn (stdClass $left): Closure => static fn (stdClass $right): bool => $left->name === $right->name; +/** @psalm-suppress InvalidArgument @phpstan-ignore-next-line */ +distinct_checkObjectList(Collection::fromIterable($cats)->distinct($objectComparator, $accessor));