Improve count() narrowing of constant arrays

[herndlm]

Generalizes the already existing solution (wow, this is way more complicated than I thought!) and uses it outside of union types as well. I kind of had to make the "isNormalCount" determination a bit more dry, which unfortunately made the diff worse..

I did do a performance comparison with hyperfine running make phpstan and didn't notice any changes, but my system is not the best.

Closes phpstan/phpstan#12190
Closes phpstan/phpstan#3631

[herndlm]

I know, I changed that narrow* method to return Type instead of SpecifiesTypes. Maybe I'll change this back again tomorrow.

[herndlm]

I know, I changed that narrow* method to return Type instead of SpecifiesTypes. Maybe I'll change this back again tomorrow.

adapted. looks better now IMO. The master plan is to move more common things for both count() == xx and count > xx specification into the now renamed specifyTypesForCountFuncCall(). Would also be great if turnListIntoConstantArray() goes away completely or is only used internally once... But for here this is more than enough IMO.

@staabm what do you think about this?

[staabm]

what do you think about this?

this PR looks great, thanks for cleaning up after me :)

[herndlm]

Should stuff like this be based on 1.12.x or latest dev branch? I sometimes have issues deciding 😅

[ondrejmirtes]

Big changes to TypeSpecifier I prefer on 2.1.x, to avoid conflicts.

[herndlm]

Thx, makes sense. And good call, there was a conflict.. Rebased this and also other of my PRs on 2.1.x and dealt with conflicts 😊

[herndlm]

I think this is ready, only that first issue bot response is confusing me slightly: https://github.com/phpstan/phpstan-src/actions/runs/12635127480. but at the same time it looks like those errors are already there now (https://phpstan.org/r/3cb22712-7994-4b1c-9946-3946083ddb27) or am I missing something?

UPDATE: ah wait, it's line 109 of PHP 7.1 - 7.4 which seems to be loosing some generic info I think. I'll check again then..

[herndlm]

ok done, this was even simplified further, is now behaving as I expect it to and should be save :)

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

The levels test JSON needs updating: https://github.com/phpstan/phpstan-src/actions/runs/13762263415/job/38480859499?pr=3709

Just run it locally, it will autoupdate the JSON file, and commit that.

Also it might be pointing out an actual bug, it might be worth reviewing what changed.

[herndlm]

@ondrejmirtes thx for checking this. it looks like though those levels test differences are not coming from here but were recently introduced in deb0911

UPDATE: but let me update those levels tests in a dedicated PR anyway, you can review the changes there.

--> #3869

[VincentLanglet]

What about adding assertType('array{string}', $arr); ?

[herndlm]

good point, I already forgot. but quickly looked again and apparently it's currently

assertType('array{0: string, 1?: string}', $arr);

it ends up using a falsey scope here, because count($arr) <= 1 is transformed to 2 < count($arr) and then something in the loop still skips creating a result type because it can't fully deal with falsey scopes unfortunately. If I remove that continue, it would basically come up with "cannot be array{string, string}" which should make it then array{string} by some falsey scope filtering or so irrelevant of the new code. but that does also not work :/ and many tests for (list) counting start to fail :/ would be great to further improve this, but I was kind of stuck there..

[ondrejmirtes]

Thank you!

[ondrejmirtes]

Hi, I found out about a regression because of this PR: https://phpstan.org/r/38051a80-e8ef-4f26-abaa-dac27791b444

The keys shouldn't be optional :)

[herndlm]

It looks like this was already like that before but that case is not supported by the changes here? but I agree, they should be non-optional 🤔

[herndlm]

aha, that one is not handled because the $arrayType->isList() returns maybe. interesting

[ondrejmirtes]

Yeah but even if something is not a list, if the total number of keys is <= than the verified count, we know we can make all of them required.

[herndlm]

I'll look into this. @staabm initially created all kind of list count tests (luckily!) and some start to fail, so those cases are dangerous. e.g. smth like where the keys have gaps I suppose.

if (count($row) === 2) {
	assertType('array{0: int, 3?: string|null}', $row); // is array{int, *ERROR*} instead
} else {
	assertType('array{0: int, 3?: string|null}|array{string}', $row);
}

or, in general, some new feature for this needs to be added or the existing one changed :) should be doable

[ondrejmirtes]

FYI this PR broke this snippet https://phpstan.org/r/8b318eac-e881-46c4-a72d-8ae55f2953e5

There's nothing we can deduce for $this->importedDaySummaryRows so the type shouldn't be narrowed to Never.

I'm looking into this myself but I'm not sure how successful I'm going to be, so please look into it too. The problem goes away when I change the type $this->totalExpectedRows from int to positive-int so it's going to be something about that.

[ondrejmirtes]

This diff fixes the problem but makes some tests fail:

diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php
index c016b2869..e54de7d5e 100644
--- a/src/Analyser/TypeSpecifier.php
+++ b/src/Analyser/TypeSpecifier.php
@@ -1136,8 +1136,10 @@ final class TypeSpecifier
 				$resultTypes[] = $valueTypesBuilder->getArray();
 				continue;
 			}
+		}
 
-			$resultTypes[] = $arrayType;
+		if (count($resultTypes) === 0) {
+			return null;
 		}
 
 		return $this->create($countFuncCall->getArgs()[0]->value, TypeCombinator::union(...$resultTypes), $context, $scope)->setRootExpr($rootExpr);

Please look into this as this is currently blocking a release. Thanks.

[staabm]

I think I have a working fix. PR incoming

[ondrejmirtes]

I have one more blocking regression caused by this PR: https://phpstan.org/r/1fe15afb-62c0-4c3d-8df5-b240ccb36399

The type of $otherMinPrices should not degrade to mixed.

[herndlm]

preparing a fix, but currently it "unfixes" phpstan/phpstan#1311 again unfortunately.
UPDATE: looks like it was not properly fixed really anyway, I'll open the PR soon with explanations