Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve range multiplication and division #1961

Merged
merged 5 commits into from Jan 8, 2023
+132 −54
Merged

Improve range multiplication and division #1961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
98afed6
Improve range computation
VincentLanglet Nov 4, 2022
de033df
Fix tests
VincentLanglet Nov 4, 2022
228217e
Avoid issue with multiplication by 0
VincentLanglet Nov 4, 2022
0d875e6
Add test
VincentLanglet Nov 4, 2022
bab5bf0
Fix cs
VincentLanglet Nov 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
125 changes: 89 additions & 36 deletions src/Reflection/InitializerExprTypeResolver.php
View file
Open in desktop
Expand Up @@ -65,14 +65,19 @@
use PHPStan\Type\UnionType;
use function array_keys;
use function array_merge;
use function assert;
use function ceil;
use function count;
use function dirname;
use function floor;
use function in_array;
use function is_float;
use function is_int;
use function max;
use function min;
use function sprintf;
use function strtolower;
use const INF;

class InitializerExprTypeResolver
{
Expand Down Expand Up @@ -1580,6 +1585,14 @@ private function integerRangeMath(Type $range, BinaryOp $node, Type $operand): T
return $union->toNumber();
}

if ($operand instanceof IntegerRangeType) {
$operandMin = $operand->getMin();
$operandMax = $operand->getMax();
} else {
$operandMin = $operand->getValue();
$operandMax = $operand->getValue();
}

if ($node instanceof BinaryOp\Plus) {
if ($operand instanceof ConstantIntegerType) {
/** @var int|float|null $min */
Expand Down Expand Up @@ -1645,63 +1658,103 @@ private function integerRangeMath(Type $range, BinaryOp $node, Type $operand): T
}
}
} elseif ($node instanceof Expr\BinaryOp\Mul) {
if ($operand instanceof ConstantIntegerType) {
/** @var int|float|null $min */
$min = $rangeMin !== null ? $rangeMin * $operand->getValue() : null;
$min1 = $rangeMin === 0 || $operandMin === 0 ? 0 : ($rangeMin ?? -INF) * ($operandMin ?? -INF);
$min2 = $rangeMin === 0 || $operandMax === 0 ? 0 : ($rangeMin ?? -INF) * ($operandMax ?? INF);
$max1 = $rangeMax === 0 || $operandMin === 0 ? 0 : ($rangeMax ?? INF) * ($operandMin ?? -INF);
$max2 = $rangeMax === 0 || $operandMax === 0 ? 0 : ($rangeMax ?? INF) * ($operandMax ?? INF);

/** @var int|float|null $max */
$max = $rangeMax !== null ? $rangeMax * $operand->getValue() : null;
} else {
/** @var int|float|null $min */
$min = $rangeMin !== null && $operand->getMin() !== null ? $rangeMin * $operand->getMin() : null;
$min = min($min1, $min2, $max1, $max2);
$max = max($min1, $min2, $max1, $max2);

/** @var int|float|null $max */
$max = $rangeMax !== null && $operand->getMax() !== null ? $rangeMax * $operand->getMax() : null;
if ($min === -INF) {
$min = null;
}

if ($min !== null && $max !== null && $min > $max) {
[$min, $max] = [$max, $min];
}

// invert maximas on multiplication with negative constants
if ((($range instanceof ConstantIntegerType && $range->getValue() < 0)
|| ($operand instanceof ConstantIntegerType && $operand->getValue() < 0))
&& ($min === null || $max === null)) {
[$min, $max] = [$max, $min];
if ($max === INF) {
$max = null;
}

} else {
if ($operand instanceof ConstantIntegerType) {
$min = $rangeMin !== null && $operand->getValue() !== 0 ? $rangeMin / $operand->getValue() : null;
$max = $rangeMax !== null && $operand->getValue() !== 0 ? $rangeMax / $operand->getValue() : null;
} else {
$min = $rangeMin !== null && $operand->getMin() !== null && $operand->getMin() !== 0 ? $rangeMin / $operand->getMin() : null;
$max = $rangeMax !== null && $operand->getMax() !== null && $operand->getMax() !== 0 ? $rangeMax / $operand->getMax() : null;
}
// Avoid division by zero when looking for the min and the max by using the closest int
$operandMin = $operandMin !== 0 ? $operandMin : 1;
$operandMax = $operandMax !== 0 ? $operandMax : -1;

if (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found really complicated to compute

int<-1, 2> / int<-1, 2>

It seemed easier to split negative and positive integers.

So

int<-1, 0> / int<-1, 0>
int<-1, 0> / int<0, 2>
int<0, 2> / int<-1, 0>
int<0, 2> / int<0, 2>

And then I merge all these types.

($operandMin < 0 || $operandMin === null)
&& ($operandMax > 0 || $operandMax === null)
) {
$negativeOperand = IntegerRangeType::fromInterval($operandMin, 0);
assert($negativeOperand instanceof IntegerRangeType);
$positiveOperand = IntegerRangeType::fromInterval(0, $operandMax);
assert($positiveOperand instanceof IntegerRangeType);

$result = TypeCombinator::union(
$this->integerRangeMath($range, $node, $negativeOperand),
$this->integerRangeMath($range, $node, $positiveOperand),
)->toNumber();

if ($result->equals(new UnionType([new IntegerType(), new FloatType()]))) {
return new BenevolentUnionType([new IntegerType(), new FloatType()]);
}

return $result;
}
if (
($rangeMin < 0 || $rangeMin === null)
&& ($rangeMax > 0 || $rangeMax === null)
) {
$negativeRange = IntegerRangeType::fromInterval($rangeMin, 0);
assert($negativeRange instanceof IntegerRangeType);
$positiveRange = IntegerRangeType::fromInterval(0, $rangeMax);
assert($positiveRange instanceof IntegerRangeType);

$result = TypeCombinator::union(
$this->integerRangeMath($negativeRange, $node, $operand),
$this->integerRangeMath($positiveRange, $node, $operand),
)->toNumber();

if ($result->equals(new UnionType([new IntegerType(), new FloatType()]))) {
return new BenevolentUnionType([new IntegerType(), new FloatType()]);
}

return $result;
}

$rangeMinSign = ($rangeMin ?? -INF) <=> 0;
$rangeMaxSign = ($rangeMax ?? INF) <=> 0;

$min1 = $operandMin !== null ? ($rangeMin ?? -INF) / $operandMin : $rangeMinSign * -0.1;
$min2 = $operandMax !== null ? ($rangeMin ?? -INF) / $operandMax : $rangeMinSign * 0.1;
$max1 = $operandMin !== null ? ($rangeMax ?? INF) / $operandMin : $rangeMaxSign * -0.1;
$max2 = $operandMax !== null ? ($rangeMax ?? INF) / $operandMax : $rangeMaxSign * 0.1;
Comment on lines +1728 to +1731
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically use the same tips than for multiplication, replacing null by INF. But a division by INF is not great, giving the float 0 or NAN.

I can still simulate this division by saying the min/max is really near from 0 with the correct sign.


$min = min($min1, $min2, $max1, $max2);
$max = max($min1, $min2, $max1, $max2);

if ($range instanceof IntegerRangeType && $operand instanceof IntegerRangeType) {
if ($rangeMax === null && $operand->getMax() === null) {
$min = 0;
} elseif ($rangeMin === null && $operand->getMin() === null) {
if ($min === -INF) {
$min = null;
}
if ($max === INF) {
$max = null;
}
}

if ($min !== null && $max !== null && $min > $max) {
[$min, $max] = [$max, $min];
}

if ($operand instanceof IntegerRangeType
&& ($operand->getMin() === null || $operand->getMax() === null)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The float type must be added as soon as I divide by a Range. For instance

int<-1, 2> / int<-1, 2>

There is 1 / 2 which is a float, even if both min and max are int.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking, this is not always true.
like

6 / int<1,3>

but I think it would get too complicated adding this logic in this PR, so no need to fix it now. It's not working correctly now anyway:)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. I didn't find a better solution.
Should still be better than https://phpstan.org/r/e2efef83-28d5-4c57-83c6-c8be5fb4d67d

Moreover 6 / int<1, 6> is possibly a float.
So the only improvement could be to check every possible values unless we find a float...
This seems to complicated.

In the same way 6 / int<1,3> is int<2, 6> but the values 4 and 5 are not possible.
But again, unless testing all the values...

Copy link
Contributor

@rajyan rajyan Nov 4, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, your solution is smart and the results are much better than before!
The number of edge cases are small because it only happens if
range multiple (like 1 * 2 * 3) * k === constant integer (like 6)

|| ($rangeMin === null || $rangeMax === null)
|| is_float($min) || is_float($max)
|| is_float($min)
|| is_float($max)
) {
if (is_float($min)) {
$min = (int) $min;
$min = (int) ceil($min);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(int) 0.5 was 0.

So, int<1, 2> / 2 was int<0, 1>|float which is wrong, 0 was never possible.

(int) ceil(0.5) is 1.
And 1|float is the correct result.

}
if (is_float($max)) {
$max = (int) $max;
}

if ($min !== null && $max !== null && $min > $max) {
[$min, $max] = [$max, $min];
$max = (int) floor($max);
}

// invert maximas on division with negative constants
Expand Down
6 changes: 3 additions & 3 deletions tests/PHPStan/Analyser/data/div-by-zero.php
View file
Open in desktop
Expand Up @@ -13,9 +13,9 @@ class Foo
*/
public function doFoo(int $range1, int $range2, int $int): void
{
assertType('(float|int)', 5 / $range1);
assertType('(float|int)', 5 / $range2);
assertType('(float|int)', $range1 / $range2);
assertType('float|int<1, 5>', 5 / $range1);
assertType('float|int<-5, -1>', 5 / $range2);
assertType('float|int<min, 0>', $range1 / $range2);
assertType('(float|int)', 5 / $int);

assertType('*ERROR*', 5 / 0);
Expand Down
55 changes: 40 additions & 15 deletions tests/PHPStan/Analyser/data/integer-range-types.php
View file
Open in desktop
Expand Up @@ -218,70 +218,70 @@ public function math($i, $j, $z, $pi, $r1, $r2, $r3, $rMin, $rMax, $x, $y) {
assertType('int<2, 13>', $r1 + $j);
assertType('int<-2, 9>', $r1 - $j);
assertType('int<1, 30>', $r1 * $j);
assertType('float|int<0, 10>', $r1 / $j);
assertType('float|int<1, 10>', $r1 / $j);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 was never possible

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please verify each expectation change on wolfram alpha like
https://www.wolframalpha.com/input?i=%28interval%5B1%2C10%5D+%2F+interval%5B1%2C3%5D%29

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staabm I checked every expectation, and I'm good with my results.
I added some link in the review, but wolfram alpha is sometimes wrong.

See https://www.wolframalpha.com/input?i=%28-1%2Finterval%5B-9999%2C+5%5D+%29
or https://www.wolframalpha.com/input?i=%28interval%5B1%2C10%5D+%2F+interval%5B-9999%2C5%5D%29

This is especially why in the code I split the / computation to not have together the positive and the negative int.

assertType('int<min, 15>', $rMin * $j);
assertType('int<5, max>', $rMax * $j);

assertType('int<2, 13>', $j + $r1);
assertType('int<-9, 2>', $j - $r1);
assertType('int<1, 30>', $j * $r1);
assertType('float|int<0, 3>', $j / $r1);
assertType('float|int<1, 3>', $j / $r1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 was never possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.wolframalpha.com/input?i=%28interval%5B1%2C3%5D+%2F+interval%5B1%2C10%5D%29

assertType('int<min, 15>', $j * $rMin);
assertType('int<5, max>', $j * $rMax);

assertType('int<-19, -10>|int<2, 13>', $r1 + $z);
assertType('int<-2, 9>|int<21, 30>', $r1 - $z);
assertType('int<-200, -20>|int<1, 30>', $r1 * $z);
assertType('float|int<0, 10>', $r1 / $z);
assertType('float|int<1, 10>', $r1 / $z);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 was never possible

assertType('int', $rMin * $z);
assertType('int<min, -100>|int<5, max>', $rMax * $z);

assertType('int<2, max>', $pi + 1);
assertType('int<-1, max>', $pi - 2);
assertType('int<2, max>', $pi * 2);
assertType('float|int<0, max>', $pi / 2);
assertType('float|int<1, max>', $pi / 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 was never possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.wolframalpha.com/input?i=%28interval%5B1%2C99999%5D+%2F+2%29

assertType('int<2, max>', 1 + $pi);
assertType('int<min, 2>', 2 - $pi);
assertType('int<2, max>', 2 * $pi);
assertType('float|int<2, max>', 2 / $pi);
assertType('float|int<1, 2>', 2 / $pi);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$pi is a positive int so the result cannot be an int bigger than 2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.wolframalpha.com/input?i=%282+%2F+interval%5B1%2C99999%5D%29


assertType('int<5, 14>', $r1 + 4);
assertType('int<-3, 6>', $r1 - 4);
assertType('int<4, 40>', $r1 * 4);
assertType('float|int<0, 2>', $r1 / 4);
assertType('float|int<1, 2>', $r1 / 4);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 was never possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.wolframalpha.com/input?i=%28interval%5B1%2C10%5D+%2F+4%29

assertType('int<9, max>', $rMax + 4);
assertType('int<1, max>', $rMax - 4);
assertType('int<20, max>', $rMax * 4);
assertType('float|int<1, max>', $rMax / 4);
assertType('float|int<2, max>', $rMax / 4);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 was never possible since $rMax >= 5

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.wolframalpha.com/input?i=%28interval%5B5%2C99999%5D+%2F+4%29


assertType('int<6, 20>', $r1 + $r2);
assertType('int<-9, 5>', $r1 - $r2);
assertType('int<5, 100>', $r1 * $r2);
assertType('float|int<0, 1>', $r1 / $r2);
assertType('float|int<1, 2>', $r1 / $r2);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int<1, 10> / int<5, 10>

0 was never possible and 10 / 5 is 2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.wolframalpha.com/input?i=%28interval%5B1%2C10%5D+%2F+interval%5B5%2C10%5D%29


assertType('int<-99, 19>', $r1 - $r3);

assertType('int<min, 15>', $r1 + $rMin);
assertType('int<-4, max>', $r1 - $rMin);
assertType('int<min, 50>', $r1 * $rMin);
assertType('float|int<min, 2>', $r1 / $rMin);
assertType('float|int<-10, -1>|int<1, 10>', $r1 / $rMin);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int<1, 10> / int<min, 5>

10 / -1 = -10 and 10 / -10 = -1
and 10 / 10 = 1 and 10 / 1 = 10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woflramalpha is not doing great on this https://www.wolframalpha.com/input?i=%28interval%5B1%2C10%5D+%2F+interval%5B-9999%2C5%5D%29

assertType('int<min, 15>', $rMin + $r1);
assertType('int<min, 4>', $rMin - $r1);
assertType('int<min, 50>', $rMin * $r1);
assertType('float|int<min, 0>', $rMin / $r1);
assertType('float|int<min, 5>', $rMin / $r1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int<min, 5> / int<1, 10> can reach 5 since $r1 can be 1


assertType('int<6, max>', $r1 + $rMax);
assertType('int', $r1 - $rMax);
assertType('int<5, max>', $r1 * $rMax);
assertType('float|int<0, max>', $r1 / $rMax);
assertType('float|int<1, 2>', $r1 / $rMax);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int<1, 10> / int<5, max>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.wolframalpha.com/input?i=%28interval%5B1%2C10%5D+%2F+interval%5B5%2C+9999%5D%29

assertType('int<6, max>', $rMax + $r1);
assertType('int<-5, max>', $rMax - $r1);
assertType('int<5, max>', $rMax * $r1);
assertType('float|int<5, max>', $rMax / $r1);
assertType('float|int<1, max>', $rMax / $r1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int<5, max> / int<1, 10>

When you do 5 / 5 you get 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.wolframalpha.com/input?i=%28interval%5B5%2C+9999%5D%2Finterval%5B1%2C10%5D+%29


assertType('5|10|15|20|30', $x / $y);

assertType('float|int<0, max>', $rMax / $rMax);
assertType('float|int<1, max>', $rMax / $rMax);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 was never possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.wolframalpha.com/input?i=%28interval%5B5%2C+9999%5D%2Finterval%5B5%2C+9999%5D+%29

assertType('(float|int)', $rMin / $rMin);
}

Expand All @@ -307,8 +307,8 @@ public function maximaInversion($rMin, $rMax) {
assertType('int<-5, max>', $rMin * -1);
assertType('int<min, -10>', $rMax * -2);

assertType('float|int<0, max>', -1 / $rMin);
assertType('float|int<min, 0>', -2 / $rMax);
assertType('-1|1|float', -1 / $rMin);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 / $rMin with $rMin an int will never be something else

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wolframalpha is not doing great on this https://www.wolframalpha.com/input?i=%28-1%2Finterval%5B-9999%2C+5%5D+%29

assertType('float', -2 / $rMax);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$rMax is int<5, max> so it's not possible to have an int

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.wolframalpha.com/input?i=%28-2%2Finterval%5B5%2C+9999%5D+%29


assertType('float|int<-5, max>', $rMin / -1);
assertType('float|int<min, -2>', $rMax / -2);
Expand All @@ -330,4 +330,29 @@ public function unaryMinus($r1, $r2, $rMin, $rMax, $rZero) {
assertType('int<-50, 0>', -$rZero);
}

/**
* @param int<-1, 2> $p
* @param int<-1, 2> $u
*/
public function sayHello($p, $u): void
{
assertType('int<-2, 4>', $p + $u);
assertType('int<-3, 3>', $p - $u);
assertType('int<-2, 4>', $p * $u);
assertType('float|int<-2, 2>', $p / $u);
}

/**
* @param int<0, max> $positive
* @param int<min, 0> $negative
*/
public function zeroIssues($positive, $negative)
{
assertType('0', 0 * $positive);
assertType('int<0, max>', $positive * $positive);
assertType('0', 0 * $negative);
assertType('int<0, max>', $negative * $negative);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another assert for $negative * $positve?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, added assertType('int<min, 0>', $negative * $positive);

assertType('int<min, 0>', $negative * $positive);
}

}