Report in-place sort calls that cannot change the array behind the sortWithoutEffect bleeding-edge toggle - #6296
Conversation
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
I think the same is true for arrays which are known to have a size of 0 or 1
Should be checked with IntegerRangeType of 0-1 isSuperTypeOf $arg->getArraySize.
|
Addressed the review and pushed as Change ( - if (IntegerRangeType::fromInterval(2, null)->isSuperTypeOf($arrayType->getArraySize())->no()) {
+ if (IntegerRangeType::fromInterval(0, 1)->isSuperTypeOf($arrayType->getArraySize())->yes()) {The two forms select the same calls — an unknown I also added two data cases that exercise the range form on a size that is genuinely
|
|
worth testing: there are cases where sorting a array with a size of 1 still has a effect, because e.g. keys get re-numbered. <?php
$array = ['key' => 42];
sort($array);
var_dump($array);
// [0 => 42]see https://3v4l.org/XHf45#veol -> we should not emit a PHPStan error for this cases |
|
Pushed What I changed — only the rule's test data, since the behavior staabm raised is already correct:
function sortSingleElementMapLiteral(): void
{
$array = ['key' => 42];
sort($array);
}
/** @param array{foo?: int} $optional */
function sortOptionalKeyMap(array $optional): void { sort($optional); }
/** @param array{foo?: int} $optional */
function shuffleOptionalKeyMap(array $optional): void { shuffle($optional); }The first is the 3v4l reproducer verbatim: I verified these are meaningful and not vacuously passing: replacing the
|
…ortWithoutEffect` bleeding-edge toggle
- New rule `PHPStan\Rules\Functions\SortWithoutEffectRule` (level 5), gated behind the
`sortWithoutEffect` feature toggle in `conf/bleedingEdge.neon`.
- `ksort()` on a `list` is reported as `ksort.list`: the keys are already `0..n-1` ascending.
Only reported when the flags are known to be `SORT_REGULAR` or `SORT_NUMERIC` (or absent);
`SORT_STRING`, `SORT_NATURAL` and non-constant flags bail out.
- Swept the whole in-place sort family for the same class of dead call:
- definitely empty array + any of `sort`, `rsort`, `usort`, `shuffle`, `asort`, `arsort`,
`ksort`, `krsort`, `uasort`, `uksort`, `natsort`, `natcasesort` → `<fn>.empty`.
- array with at most one element + a key-preserving sort (`asort`, `arsort`, `ksort`,
`krsort`, `uasort`, `uksort`, `natsort`, `natcasesort`) → `<fn>.singleElement`.
- array with at most one element that is also a list + a reindexing sort (`sort`, `rsort`,
`usort`, `shuffle`) → `<fn>.singleElement`. Reindexing sorts are deliberately not reported
on single-element non-lists, because they renumber the key.
- `krsort()` on a multi-element list is deliberately not reported - it reverses the array.
- Registers the toggle in `conf/config.neon`, `conf/parametersSchema.neon` and wires the rule
up in `conf/config.level5.neon`.
Instead of asserting that int<2, max> is not a supertype of the array size, ask directly whether int<0, 1> is a supertype of it. Covers the same cases and states the condition the way it is meant to be read. Co-Authored-By: Claude Opus 5 <[email protected]>
sort()/shuffle() on a one-element array with a non-zero key is not a
no-op: the key is renumbered to 0. The rule already bails out on
non-lists for the reindexing sorts; these cases pin that behaviour down,
including the maybe-a-list array{foo?: int} shape whose size is int<0, 1>.
Co-Authored-By: Claude Opus 5 <[email protected]>
c67ac56 to
b9bf231
Compare
3 participants
Summary
ksort()on alistis always a no-op: a list's keys are0..n-1in ascending order already, so with the defaultSORT_REGULARflags there is nothing to reorder. This is the same class of dead callArrayValuesRulealready reports asarrayValues.list/arrayValues.empty, and it silently hides real bugs (the issue reports a third-party extension whose output was non-deterministic because aksort()was meant to be asort()).This PR adds
PHPStan\Rules\Functions\SortWithoutEffectRule, behind thesortWithoutEffectbleeding-edge toggle, and covers the whole family of in-place sort functions rather thanksort()alone.Changes
src/Rules/Functions/SortWithoutEffectRule.php— new rule listening onFuncCall. It normalizes the arguments (so named arguments work), takes the PHPDoc or native type of#1 $arraydepending ontreatPhpDocTypesAsCertain, and reports:ksort.list— the array is a list and the sort flags are known to beSORT_REGULARorSORT_NUMERIC(or omitted). The flags check is aisSuperTypeOf()against0|1, soSORT_STRING,SORT_NATURAL,SORT_REGULAR|SORT_FLAG_CASEand non-constantintflags all bail out.<fn>.empty— the array is definitely empty. Applies to every in-place sort:sort,rsort,usort,shuffle,asort,arsort,ksort,krsort,uasort,uksort,natsort,natcasesort.<fn>.singleElement— the array has at most one element (int<2, max>is not a supertype ofgetArraySize()). Reported for the key-preserving sorts unconditionally, and for the reindexing sorts (sort,rsort,usort,shuffle) only when the array is also a list.conf/bleedingEdge.neon,conf/config.neon,conf/parametersSchema.neon— newfeatureToggles.sortWithoutEffect.conf/config.level5.neon— registers the rule (mirroringArrayValuesRule's level) with aconditionalTagsentry on the toggle.tests/PHPStan/Rules/Functions/SortWithoutEffectRuleTest.php+ two data files.Analogous cases probed and deliberately not reported, because they are not no-ops:
krsort()on a list of two or more elements — it reverses the array.sort()/rsort()/usort()/shuffle()on a single-element array that is not a list (e.g.array{foo: int}) — they renumber the key to0.ksort()onnon-empty-array<string, int>, onarray{foo: int}|array{bar: int, baz: int}(size1|2), and onmixed.ksort($list, SORT_STRING)/SORT_NATURAL/ a non-constantint— string comparison orders"10"before"2", and unknown flags are skipped conservatively.Root cause
This is a missing check rather than a regression. The existing "call has no effect" rules only covered
array_values()andarray_filter(); the in-place sort family had no equivalent. The pattern behind all of them is the same: an argument type whose shape (empty, at most one element, already a list) makes the operation provably identity.Rather than special-casing
ksort(), the rule splits the sort functions along the one axis that decides the answer — whether the function preserves keys or reindexes the array — and derives the three no-effect conditions from the argument'sisIterableAtLeastOnce(),isList()andgetArraySize(). That way every sibling function is covered by construction and adding a new one is a single array entry.Test
tests/PHPStan/Rules/Functions/SortWithoutEffectRuleTest.php:testRule()analysesdata/sort-without-effect.php, which contains both reproducers from the issue (ksort()on alist<string>parameter, andksort()on an array built with$tips[] = ...), plus one isolated function per sort function for the empty, single-element, single-element-list, non-empty-map, union andmixedcases, and one per interestingksort()flags value. Each call lives in its own function so the by-reference type of the previous call cannot leak into the next assertion.testNamedArguments()(#[RequiresPhp('>= 8.0.0')]) analysesdata/sort-without-effect-named-args.phpand coversksort(array: $list)andksort(flags: SORT_STRING, array: $list), exercisingArgumentsNormalizer.Both the rule test and an end-to-end
bin/phpstan analyse -l 8run of the issue's snippet were checked to report nothing with the toggle off and the two expectedksort.listerrors withconf/bleedingEdge.neon.make tests,make phpstanandmake csare green. (make name-collisionfails ontests/PHPStan/Reflection/data/attribute-const-reflection.php, which reproduces on a clean checkout and is unrelated to this change.)Fixes phpstan/phpstan#15126