Limit int ranges when narrowing arrays via count()

[herndlm]

Fixes phpstan/phpstan#12787

for const ints a limit existed already.

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

This isn't what I'd expect. You should first calculate the expected array size and then compare it with ConstantArrayTypeBuilder::ARRAY_COUNT_LIMIT.

What you're doing is that you're first comparing min with the limit, and then (max - min) with the limit. Theoretically you can end up with a much larger array than the limit.

Thanks.

[herndlm]

the code here that creates the arrays, is a bit complex and dynamic. and it was like that already before I did my changes. but I'll try to see what I can do

[ondrejmirtes]

I just want to count how many time exactly is setOffsetValueType being called, and that number has to be under the limit.

[herndlm]

sure, I understand. I checked again and slightly changed things. the max - min was not useful, I agree. I think the current state is OK, but let me know please.

what the code for int ranges basically does is 3 things

1. use the min value to create non-optional offsets in a constant array. this is what makes it create a huuge constant array in the reproducer. and min decides how often setOffsetValueType() is being called.
2. use the max value to then continue starting from min up until max to create optional offsets. here we get then up until max setOffsetValueType() calls in summary. so this should also be under the limit.
3. use an existing constant array, start with min, building a list until it finds an offset that does not exist. this is a bit weird for me still, but at least not relevant to limits I'd say, since it does not create new offsets

what do you think?

[herndlm]

summary: min and max should be under the limit IMO. checking max would be enough if it is set, but the if is already complex enough, so currently both are checked

[herndlm]

writing this down made me realize it can be a bit simpler. pushed again 🙈

[ondrejmirtes]

I don't think that ($sizeType->getMax() ?? $sizeType->getMin()) is the final size of the array created here.

Please refactor this section. You can prepare the arguments for setOffsetValueType in a single array beforehand, count the number of elements, and then proceed with creating the constant array only if it's below ConstantArrayTypeBuilder::ARRAY_COUNT_LIMIT.

[herndlm]

I still think it does, I tried to describe the logic here #3902 (comment)

but you can also check

phpstan-src/src/Analyser/TypeSpecifier.php

Lines 1108 to 1133 in dfd423f

	// turn optional offsets non-optional
	$valueTypesBuilder = ConstantArrayTypeBuilder::createEmpty();
	for ($i = 0; $i < $sizeType->getMin(); $i++) {
	$offsetType = new ConstantIntegerType($i);
	$valueTypesBuilder->setOffsetValueType($offsetType, $arrayType->getOffsetValueType($offsetType));
	}
	if ($sizeType->getMax() !== null) {
	for ($i = $sizeType->getMin(); $i < $sizeType->getMax(); $i++) {
	$offsetType = new ConstantIntegerType($i);
	$valueTypesBuilder->setOffsetValueType($offsetType, $arrayType->getOffsetValueType($offsetType), true);
	}
	} elseif ($arrayType->isConstantArray()->yes()) {
	for ($i = $sizeType->getMin();; $i++) {
	$offsetType = new ConstantIntegerType($i);
	$hasOffset = $arrayType->hasOffsetValueType($offsetType);
	if ($hasOffset->no()) {
	break;
	}
	$valueTypesBuilder->setOffsetValueType($offsetType, $arrayType->getOffsetValueType($offsetType), !$hasOffset->yes());
	}
	} else {
	$resultTypes[] = TypeCombinator::intersect($arrayType, new NonEmptyArrayType());
	continue;
	}
	$resultTypes[] = $valueTypesBuilder->getArray();

case (for loop) 1 and 2 combined call setOffsetValueType() up to max times. case 3 is special but limited by an existing constant array which should make it safe.

do you disagree? I don't even know how to refactor this right now tbh, I think the "max if set, otherwise min" is the simplest way of dealing with this.

[ondrejmirtes]

Check the 2nd commit - this is what I meant.

[ondrejmirtes]

Thank you.

[herndlm]

Check the 2nd commit - this is what I meant.

Oh. Sorry for not getting you and thx for adapting it yourself. Will check it later in detail.

[herndlm]

Tbh, I still think my solution is enough and was simpler of course. But yes, yours is slightly safer I'd say 😊