diff --git a/README.md b/README.md index 947a7516..07990b22 100644 --- a/README.md +++ b/README.md @@ -1501,7 +1501,7 @@ $this->repository = $this->someRepository; ### RequireArrayShapeReturnRule -A method that returns a packed keyed array of 2-3 named values should declare that shape in its `@return`, so the caller knows each key and its type. +A method that returns a packed array of 2-3 values - all named (`array{name: type}`) or all positional (`array{0: type, 1: type}`) - should declare that shape in its `@return`, so the caller knows each key/index and its type. ```yaml rules: diff --git a/src/Rules/RequireArrayShapeReturnRule.php b/src/Rules/RequireArrayShapeReturnRule.php index b1f291dc..79776b34 100644 --- a/src/Rules/RequireArrayShapeReturnRule.php +++ b/src/Rules/RequireArrayShapeReturnRule.php @@ -23,15 +23,16 @@ use PHPStan\Type\UnionType; /** - * A method that returns a keyed array of 2-3 named values should declare that shape in its @return, so the caller - * knows each key and its type instead of reading an opaque array. + * A method that returns a fixed array of 2-3 values should declare that shape in its @return, so the caller knows + * each key/index and its type instead of reading an opaque array. * - * Only methods whose every value-return is a literal keyed array of 2 or 3 string-keyed elements are flagged - the - * clean "packed result" case where a single shape can describe the return. Methods that also return a plain value, - * a variable, a call result or a general array are left alone, as a keyed shape cannot represent those and PHPStan - * would collapse it anyway. Single values, positional arrays and larger config/option maps are left alone, and so - * are static data maps whose values are all nested arrays or constants. A @return that already declares an array - * shape, a void or mixed return, anonymous classes and methods overriding a parent contract are skipped too. + * Only methods whose every value-return is a literal array of 2 or 3 elements with a consistent key style - all + * string keys (array{name: type}) or all positional (array{0: type, 1: type}) - are flagged, the clean "packed + * result" case where a single shape can describe the return. Methods that also return a plain value, a variable, + * a call result or a general array are left alone, as a shape cannot represent those and PHPStan would collapse it + * anyway. Single values and larger config/option maps are left alone, and so are static data maps whose values are + * all nested arrays or constants. A @return that already declares an array shape, a void or mixed return, + * anonymous classes and methods overriding a parent contract are skipped too. * * @see \Symplify\PHPStanRules\Tests\Rules\RequireArrayShapeReturnRule\RequireArrayShapeReturnRuleTest * @@ -42,7 +43,7 @@ final class RequireArrayShapeReturnRule implements Rule /** * @var string */ - public const ERROR_MESSAGE = 'Method "%s()" returns a keyed array of %d values; declare its shape in @return, e.g. array{key: type}'; + public const ERROR_MESSAGE = 'Method "%s()" returns a fixed array of %d values; declare its shape in @return, e.g. array{key: type}'; /** * @var int @@ -117,8 +118,8 @@ public function processNode(Node $node, Scope $scope): array foreach ($valueReturns as $valueReturn) { $expr = $valueReturn->expr; - // a return that is not a packed keyed array means no single shape fits, skip the method - if (! $expr instanceof Array_ || ! $this->isPackedKeyedArray($expr)) { + // a return that is not a packed array means no single shape fits, skip the method + if (! $expr instanceof Array_ || ! $this->isPackedArray($expr)) { return []; } @@ -135,14 +136,14 @@ public function processNode(Node $node, Scope $scope): array return [$ruleError]; } - private function isPackedKeyedArray(Array_ $array): bool + private function isPackedArray(Array_ $array): bool { $valueCount = count($array->items); if ($valueCount < self::MIN_VALUE_COUNT || $valueCount > self::MAX_VALUE_COUNT) { return false; } - if (! $this->hasStringKeyOnEveryItem($array)) { + if (! $this->hasConsistentKeyStyle($array)) { return false; } @@ -214,17 +215,31 @@ private function isDeclaredInParent(ClassReflection $classReflection, string $me return false; } - private function hasStringKeyOnEveryItem(Array_ $array): bool + // a clean shape needs one key style: all string keys (array{name: type}) or all positional (array{0: type}) + private function hasConsistentKeyStyle(Array_ $array): bool { - $found = true; + $stringKeyCount = 0; + $positionalCount = 0; + foreach ($array->items as $arrayItem) { - if (! $arrayItem->key instanceof String_) { - $found = false; - break; + // a spread turns the array variable-length, so no fixed shape fits + if ($arrayItem->unpack) { + return false; + } + + if ($arrayItem->key instanceof String_) { + ++$stringKeyCount; + } elseif ($arrayItem->key === null) { + ++$positionalCount; + } else { + // explicit int or computed keys are not a clean shape + return false; } } - return $found; + $itemCount = count($array->items); + + return $stringKeyCount === $itemCount || $positionalCount === $itemCount; } /** diff --git a/tests/Rules/RequireArrayShapeReturnRule/Fixture/SkipPositionalArrayReturn.php b/tests/Rules/RequireArrayShapeReturnRule/Fixture/ReportPositionalArrayReturn.php similarity index 82% rename from tests/Rules/RequireArrayShapeReturnRule/Fixture/SkipPositionalArrayReturn.php rename to tests/Rules/RequireArrayShapeReturnRule/Fixture/ReportPositionalArrayReturn.php index eed9f4fc..e3dc7673 100644 --- a/tests/Rules/RequireArrayShapeReturnRule/Fixture/SkipPositionalArrayReturn.php +++ b/tests/Rules/RequireArrayShapeReturnRule/Fixture/ReportPositionalArrayReturn.php @@ -4,7 +4,7 @@ namespace Symplify\PHPStanRules\Tests\Rules\RequireArrayShapeReturnRule\Fixture; -final class SkipPositionalArrayReturn +final class ReportPositionalArrayReturn { public function run(): array { diff --git a/tests/Rules/RequireArrayShapeReturnRule/Fixture/SkipMixedKeyArrayReturn.php b/tests/Rules/RequireArrayShapeReturnRule/Fixture/SkipMixedKeyArrayReturn.php new file mode 100644 index 00000000..cddd0120 --- /dev/null +++ b/tests/Rules/RequireArrayShapeReturnRule/Fixture/SkipMixedKeyArrayReturn.php @@ -0,0 +1,13 @@ + 'Tom', 30]; + } +} diff --git a/tests/Rules/RequireArrayShapeReturnRule/RequireArrayShapeReturnRuleTest.php b/tests/Rules/RequireArrayShapeReturnRule/RequireArrayShapeReturnRuleTest.php index 0955020e..6ba52299 100644 --- a/tests/Rules/RequireArrayShapeReturnRule/RequireArrayShapeReturnRuleTest.php +++ b/tests/Rules/RequireArrayShapeReturnRule/RequireArrayShapeReturnRuleTest.php @@ -31,10 +31,11 @@ public static function provideData(): Iterator { $errorMessage = sprintf(RequireArrayShapeReturnRule::ERROR_MESSAGE, 'run', 2); yield [__DIR__ . '/Fixture/ReportKeyedArrayReturn.php', [[$errorMessage, 11]]]; + yield [__DIR__ . '/Fixture/ReportPositionalArrayReturn.php', [[$errorMessage, 11]]]; yield [__DIR__ . '/Fixture/SkipShapedReturn.php', []]; yield [__DIR__ . '/Fixture/SkipSingleValueReturn.php', []]; - yield [__DIR__ . '/Fixture/SkipPositionalArrayReturn.php', []]; + yield [__DIR__ . '/Fixture/SkipMixedKeyArrayReturn.php', []]; } protected function getRule(): Rule