diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 9da565d6afb..a2f57a4871f 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -26,3 +26,4 @@ parameters: finiteTypesInHaystack: true switchConditionAlwaysFalse: true checkImportedClassNameCase: true + sortWithoutEffect: true diff --git a/conf/config.level5.neon b/conf/config.level5.neon index 534d58dde0b..aef90f4d9e5 100644 --- a/conf/config.level5.neon +++ b/conf/config.level5.neon @@ -12,6 +12,8 @@ conditionalTags: phpstan.rules.rule: %featureToggles.checkPrintfParameterTypes% PHPStan\Rules\DateIntervalInstantiationRule: phpstan.rules.rule: %featureToggles.checkDateIntervalConstructor% + PHPStan\Rules\Functions\SortWithoutEffectRule: + phpstan.rules.rule: %featureToggles.sortWithoutEffect% autowiredAttributeServices: # registers rules with #[RegisteredRule] attribute @@ -26,3 +28,8 @@ services: checkStrictPrintfPlaceholderTypes: %checkStrictPrintfPlaceholderTypes% - class: PHPStan\Rules\DateIntervalInstantiationRule + - + class: PHPStan\Rules\Functions\SortWithoutEffectRule + arguments: + treatPhpDocTypesAsCertain: %treatPhpDocTypesAsCertain% + treatPhpDocTypesAsCertainTip: %tips.treatPhpDocTypesAsCertain% diff --git a/conf/config.neon b/conf/config.neon index de093affa36..073b9b0094d 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -57,6 +57,7 @@ parameters: finiteTypesInHaystack: false switchConditionAlwaysFalse: false checkImportedClassNameCase: false + sortWithoutEffect: false fileExtensions: - php checkAdvancedIsset: false diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index ecbac473b43..ec23ecbb2d3 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -55,6 +55,7 @@ parametersSchema: finiteTypesInHaystack: bool() switchConditionAlwaysFalse: bool() checkImportedClassNameCase: bool() + sortWithoutEffect: bool() ]) fileExtensions: listOf(string()) checkAdvancedIsset: bool() diff --git a/src/Rules/Functions/SortWithoutEffectRule.php b/src/Rules/Functions/SortWithoutEffectRule.php new file mode 100644 index 00000000000..92a6dc2ff5b --- /dev/null +++ b/src/Rules/Functions/SortWithoutEffectRule.php @@ -0,0 +1,192 @@ + + */ +final class SortWithoutEffectRule implements Rule +{ + + /** + * Sort functions that keep the original keys. They cannot change an array + * with at most one element. + */ + private const KEY_PRESERVING_SORT_FUNCTIONS = ['arsort', 'asort', 'krsort', 'ksort', 'natcasesort', 'natsort', 'uasort', 'uksort']; + + /** + * Sort functions that reindex the array. They can still turn a single-element + * array with a non-zero key into a list, so they're only a no-op on lists. + */ + private const REINDEXING_SORT_FUNCTIONS = ['rsort', 'shuffle', 'sort', 'usort']; + + /** + * Sort flags under which the keys of a list are already in ascending order. + */ + private const KSORT_LIST_SAFE_FLAGS = [SORT_REGULAR, SORT_NUMERIC]; + + private const REASON_MESSAGES = [ + 'empty' => 'Parameter #1 $array (%s) of function %s is empty, call has no effect.', + 'list' => 'Parameter #1 $array (%s) of function %s is a list, call has no effect.', + 'singleElement' => 'Parameter #1 $array (%s) of function %s has at most 1 element, call has no effect.', + ]; + + public function __construct( + private ReflectionProvider $reflectionProvider, + private bool $treatPhpDocTypesAsCertain, + private bool $treatPhpDocTypesAsCertainTip, + ) + { + } + + public function getNodeType(): string + { + return FuncCall::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if (!($node->name instanceof Node\Name)) { + return []; + } + + if (!$this->reflectionProvider->hasFunction($node->name, $scope)) { + return []; + } + + $functionReflection = $this->reflectionProvider->getFunction($node->name, $scope); + $functionName = $functionReflection->getName(); + + $keyPreserving = in_array($functionName, self::KEY_PRESERVING_SORT_FUNCTIONS, true); + if (!$keyPreserving && !in_array($functionName, self::REINDEXING_SORT_FUNCTIONS, true)) { + return []; + } + + $parametersAcceptor = ParametersAcceptorSelector::selectFromArgs( + $scope, + $node->getArgs(), + $functionReflection->getVariants(), + $functionReflection->getNamedArgumentsVariants(), + ); + + $normalizedFuncCall = ArgumentsNormalizer::reorderFuncArguments($parametersAcceptor, $node); + if ($normalizedFuncCall === null) { + return []; + } + + $args = $normalizedFuncCall->getArgs(); + if (!array_key_exists(0, $args)) { + return []; + } + + if ($this->treatPhpDocTypesAsCertain) { + $arrayType = $scope->getType($args[0]->value); + } else { + $arrayType = $scope->getNativeType($args[0]->value); + } + + $keysAlreadySorted = $functionName === 'ksort' && $this->hasFlagsKeepingListOrder($scope, $parametersAcceptor, $args); + + $reason = $this->findNoEffectReason($arrayType, $keyPreserving, $keysAlreadySorted); + if ($reason === null) { + return []; + } + + $errorBuilder = RuleErrorBuilder::message(sprintf( + self::REASON_MESSAGES[$reason], + $arrayType->describe(VerbosityLevel::value()), + $functionName, + ))->identifier(sprintf('%s.%s', $functionName, $reason)); + + if ($this->treatPhpDocTypesAsCertain && $this->treatPhpDocTypesAsCertainTip) { + $nativeArrayType = $scope->getNativeType($args[0]->value); + if ($this->findNoEffectReason($nativeArrayType, $keyPreserving, $keysAlreadySorted) !== $reason) { + $errorBuilder->treatPhpDocTypesAsCertainTip(); + } + } + + return [ + $errorBuilder->build(), + ]; + } + + /** + * @return key-of|null + */ + private function findNoEffectReason(Type $arrayType, bool $keyPreserving, bool $keysAlreadySorted): ?string + { + if (!$arrayType->isArray()->yes()) { + return null; + } + + if ($arrayType->isIterableAtLeastOnce()->no()) { + return 'empty'; + } + + $isList = $arrayType->isList()->yes(); + + if ($keysAlreadySorted && $isList) { + return 'list'; + } + + if (!$keyPreserving && !$isList) { + return null; + } + + if (IntegerRangeType::fromInterval(2, null)->isSuperTypeOf($arrayType->getArraySize())->no()) { + return 'singleElement'; + } + + return null; + } + + /** + * @param Node\Arg[] $args + */ + private function hasFlagsKeepingListOrder(Scope $scope, ParametersAcceptor $parametersAcceptor, array $args): bool + { + if (array_key_exists(1, $args)) { + $flagsType = $scope->getType($args[1]->value); + } else { + $parameters = $parametersAcceptor->getParameters(); + if (!array_key_exists(1, $parameters)) { + return true; + } + + $flagsType = $parameters[1]->getDefaultValue(); + if ($flagsType === null) { + return true; + } + } + + $safeFlagsTypes = []; + foreach (self::KSORT_LIST_SAFE_FLAGS as $safeFlag) { + $safeFlagsTypes[] = new ConstantIntegerType($safeFlag); + } + + return TypeCombinator::union(...$safeFlagsTypes)->isSuperTypeOf($flagsType)->yes(); + } + +} diff --git a/tests/PHPStan/Rules/Functions/SortWithoutEffectRuleTest.php b/tests/PHPStan/Rules/Functions/SortWithoutEffectRuleTest.php new file mode 100644 index 00000000000..00061a3b2df --- /dev/null +++ b/tests/PHPStan/Rules/Functions/SortWithoutEffectRuleTest.php @@ -0,0 +1,178 @@ + + */ +class SortWithoutEffectRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new SortWithoutEffectRule( + self::createReflectionProvider(), + $this->shouldTreatPhpDocTypesAsCertain(), + true, + ); + } + + public function testRule(): void + { + $tipText = 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.'; + + $this->analyse([__DIR__ . '/data/sort-without-effect.php'], [ + [ + 'Parameter #1 $array (list) of function ksort is a list, call has no effect.', + 8, + $tipText, + ], + [ + 'Parameter #1 $array (list) of function ksort is a list, call has no effect.', + 14, + $tipText, + ], + [ + 'Parameter #1 $array (list) of function ksort is a list, call has no effect.', + 20, + $tipText, + ], + [ + 'Parameter #1 $array (array{\'a\', \'b\'}) of function ksort is a list, call has no effect.', + 70, + ], + [ + 'Parameter #1 $array (array{}) of function ksort is empty, call has no effect.', + 88, + ], + [ + 'Parameter #1 $array (array{}) of function krsort is empty, call has no effect.', + 94, + ], + [ + 'Parameter #1 $array (array{}) of function asort is empty, call has no effect.', + 100, + ], + [ + 'Parameter #1 $array (array{}) of function arsort is empty, call has no effect.', + 106, + ], + [ + 'Parameter #1 $array (array{}) of function sort is empty, call has no effect.', + 112, + ], + [ + 'Parameter #1 $array (array{}) of function rsort is empty, call has no effect.', + 118, + ], + [ + 'Parameter #1 $array (array{}) of function usort is empty, call has no effect.', + 124, + ], + [ + 'Parameter #1 $array (array{}) of function uasort is empty, call has no effect.', + 130, + ], + [ + 'Parameter #1 $array (array{}) of function uksort is empty, call has no effect.', + 136, + ], + [ + 'Parameter #1 $array (array{}) of function shuffle is empty, call has no effect.', + 142, + ], + [ + 'Parameter #1 $array (array{}) of function natsort is empty, call has no effect.', + 148, + ], + [ + 'Parameter #1 $array (array{}) of function natcasesort is empty, call has no effect.', + 154, + ], + [ + 'Parameter #1 $array (array{foo: int}) of function ksort has at most 1 element, call has no effect.', + 160, + $tipText, + ], + [ + 'Parameter #1 $array (array{foo: int}) of function krsort has at most 1 element, call has no effect.', + 166, + $tipText, + ], + [ + 'Parameter #1 $array (array{foo: int}) of function asort has at most 1 element, call has no effect.', + 172, + $tipText, + ], + [ + 'Parameter #1 $array (array{foo: int}) of function arsort has at most 1 element, call has no effect.', + 178, + $tipText, + ], + [ + 'Parameter #1 $array (array{foo: int}) of function uasort has at most 1 element, call has no effect.', + 184, + $tipText, + ], + [ + 'Parameter #1 $array (array{foo: int}) of function uksort has at most 1 element, call has no effect.', + 190, + $tipText, + ], + [ + 'Parameter #1 $array (array{foo: int}) of function natsort has at most 1 element, call has no effect.', + 196, + $tipText, + ], + [ + 'Parameter #1 $array (array{foo: int}) of function natcasesort has at most 1 element, call has no effect.', + 202, + $tipText, + ], + [ + 'Parameter #1 $array (array{int}) of function sort has at most 1 element, call has no effect.', + 232, + $tipText, + ], + [ + 'Parameter #1 $array (array{int}) of function rsort has at most 1 element, call has no effect.', + 238, + $tipText, + ], + [ + 'Parameter #1 $array (array{int}) of function usort has at most 1 element, call has no effect.', + 244, + $tipText, + ], + [ + 'Parameter #1 $array (array{int}) of function shuffle has at most 1 element, call has no effect.', + 250, + $tipText, + ], + [ + 'Parameter #1 $array (array{bar: int}|array{foo: int}) of function ksort has at most 1 element, call has no effect.', + 262, + $tipText, + ], + ]); + } + + #[RequiresPhp('>= 8.0.0')] + public function testNamedArguments(): void + { + $tipText = 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.'; + + $this->analyse([__DIR__ . '/data/sort-without-effect-named-args.php'], [ + [ + 'Parameter #1 $array (list) of function ksort is a list, call has no effect.', + 10, + $tipText, + ], + ]); + } + +} diff --git a/tests/PHPStan/Rules/Functions/data/sort-without-effect-named-args.php b/tests/PHPStan/Rules/Functions/data/sort-without-effect-named-args.php new file mode 100644 index 00000000000..f6cf20aeee5 --- /dev/null +++ b/tests/PHPStan/Rules/Functions/data/sort-without-effect-named-args.php @@ -0,0 +1,17 @@ += 8.0 + +declare(strict_types = 1); + +namespace SortWithoutEffectNamedArgs; + +/** @param list $list */ +function ksortNamedArgument(array $list): void +{ + ksort(array: $list); +} + +/** @param list $list */ +function ksortNamedArgumentWithStringFlags(array $list): void +{ + ksort(flags: SORT_STRING, array: $list); +} diff --git a/tests/PHPStan/Rules/Functions/data/sort-without-effect.php b/tests/PHPStan/Rules/Functions/data/sort-without-effect.php new file mode 100644 index 00000000000..1d59aadae31 --- /dev/null +++ b/tests/PHPStan/Rules/Functions/data/sort-without-effect.php @@ -0,0 +1,268 @@ + $list */ +function ksortList(array $list): void +{ + ksort($list); +} + +/** @param list $list */ +function ksortListRegularFlags(array $list): void +{ + ksort($list, SORT_REGULAR); +} + +/** @param list $list */ +function ksortListNumericFlags(array $list): void +{ + ksort($list, SORT_NUMERIC); +} + +/** @param list $list */ +function ksortListStringFlags(array $list): void +{ + ksort($list, SORT_STRING); +} + +/** @param list $list */ +function ksortListNaturalFlags(array $list): void +{ + ksort($list, SORT_NATURAL); +} + +/** @param list $list */ +function ksortListUnknownFlags(array $list, int $flags): void +{ + ksort($list, $flags); +} + +/** @param list $list */ +function krsortList(array $list): void +{ + krsort($list); +} + +/** @param list $list */ +function sortList(array $list): void +{ + sort($list); +} + +/** @param list $list */ +function usortList(array $list): void +{ + usort($list, fn ($a, $b) => 0); +} + +/** @param list $list */ +function asortList(array $list): void +{ + asort($list); +} + +function ksortAppendedList(): void +{ + $tips = []; + $tips[] = 'a'; + $tips[] = 'b'; + ksort($tips); +} + +/** @param array $map */ +function ksortMap(array $map): void +{ + ksort($map); +} + +/** @param non-empty-array $map */ +function ksortNonEmptyMap(array $map): void +{ + ksort($map); +} + +function ksortEmpty(): void +{ + $a = []; + ksort($a); +} + +function krsortEmpty(): void +{ + $a = []; + krsort($a); +} + +function asortEmpty(): void +{ + $a = []; + asort($a); +} + +function arsortEmpty(): void +{ + $a = []; + arsort($a); +} + +function sortEmpty(): void +{ + $a = []; + sort($a); +} + +function rsortEmpty(): void +{ + $a = []; + rsort($a); +} + +function usortEmpty(): void +{ + $a = []; + usort($a, fn ($x, $y) => 0); +} + +function uasortEmpty(): void +{ + $a = []; + uasort($a, fn ($x, $y) => 0); +} + +function uksortEmpty(): void +{ + $a = []; + uksort($a, fn ($x, $y) => 0); +} + +function shuffleEmpty(): void +{ + $a = []; + shuffle($a); +} + +function natsortEmpty(): void +{ + $a = []; + natsort($a); +} + +function natcasesortEmpty(): void +{ + $a = []; + natcasesort($a); +} + +/** @param array{foo: int} $single */ +function ksortSingle(array $single): void +{ + ksort($single); +} + +/** @param array{foo: int} $single */ +function krsortSingle(array $single): void +{ + krsort($single); +} + +/** @param array{foo: int} $single */ +function asortSingle(array $single): void +{ + asort($single); +} + +/** @param array{foo: int} $single */ +function arsortSingle(array $single): void +{ + arsort($single); +} + +/** @param array{foo: int} $single */ +function uasortSingle(array $single): void +{ + uasort($single, fn ($x, $y) => 0); +} + +/** @param array{foo: int} $single */ +function uksortSingle(array $single): void +{ + uksort($single, fn ($x, $y) => 0); +} + +/** @param array{foo: int} $single */ +function natsortSingle(array $single): void +{ + natsort($single); +} + +/** @param array{foo: int} $single */ +function natcasesortSingle(array $single): void +{ + natcasesort($single); +} + +/** @param array{foo: int} $single */ +function sortSingle(array $single): void +{ + sort($single); +} + +/** @param array{foo: int} $single */ +function rsortSingle(array $single): void +{ + rsort($single); +} + +/** @param array{foo: int} $single */ +function usortSingle(array $single): void +{ + usort($single, fn ($x, $y) => 0); +} + +/** @param array{foo: int} $single */ +function shuffleSingle(array $single): void +{ + shuffle($single); +} + +/** @param array{int} $singleList */ +function sortSingleList(array $singleList): void +{ + sort($singleList); +} + +/** @param array{int} $singleList */ +function rsortSingleList(array $singleList): void +{ + rsort($singleList); +} + +/** @param array{int} $singleList */ +function usortSingleList(array $singleList): void +{ + usort($singleList, fn ($x, $y) => 0); +} + +/** @param array{int} $singleList */ +function shuffleSingleList(array $singleList): void +{ + shuffle($singleList); +} + +/** @param array{foo: int}|array{bar: int, baz: int} $union */ +function ksortUnion(array $union): void +{ + ksort($union); +} + +/** @param array{foo: int}|array{bar: int} $unionOfSingles */ +function ksortUnionOfSingles(array $unionOfSingles): void +{ + ksort($unionOfSingles); +} + +function ksortMixed($mixed): void +{ + ksort($mixed); +}