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.

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

Improve range multiplication and division #1961

Merged
merged 5 commits into from Jan 8, 2023

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Nov 4, 2022

if ($operand instanceof ConstantIntegerType) {
/** @var int|float|null $min */
$min = $rangeMin !== null ? $rangeMin * $operand->getValue() : null;
$min1 = ($rangeMin ?? -INF) * ($operandMin ?? -INF);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Multiplication was wrong for thing like

int<-1, 2> * int<-1, 2>

we shouldn't always multiply both min and both max.
Sometimes it's one min multiplied by the other max.

Multiplication and using min/max function wasn't easy with the null value. The easiest solution I found is to use -INF and INF instead of null.

$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.

Comment on lines 1661 to 1700
if ($result->equals(new UnionType([new IntegerType(), new FloatType()]))) {
return new BenevolentUnionType([new IntegerType(), new FloatType()]);
} else {
return $result;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I merge float|int<min, 0> and float|int<0, max> seems like before we wanted (float|int) to reproduce the behavior of division by an int.

That's why we had

if ($min === null && $max === null) {
					return new BenevolentUnionType([new IntegerType(), new FloatType()]);
				}

So I tried to reproduce the same behavior.

Comment on lines +1686 to +1731
$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;
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.

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

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)

) {
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.

@@ -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.

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.

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.


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.

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.


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.

@@ -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.

assertType('float|int<0, max>', -1 / $rMin);
assertType('float|int<min, 0>', -2 / $rMax);
assertType('-1|1|float', -1 / $rMin);
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.

@VincentLanglet VincentLanglet changed the title Improve range computation Improve range multiplication and division Nov 4, 2022
@VincentLanglet VincentLanglet marked this pull request as ready for review November 4, 2022 03:08
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

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);

@VincentLanglet
Copy link
Contributor Author

I rebased and got all the test green if you find time this is Ready to be review @ondrejmirtes

@ondrejmirtes
Copy link
Member

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants