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

Fixed InterRangeType multiplication/division of maximas by a negative constant #670

Merged
merged 13 commits into from
Sep 13, 2021

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Sep 12, 2021

As discussed in #669 (comment) maxima need to be inverse when multiplying with a negative constant operand

refs phpstan/phpstan#5614

see https://3v4l.org/AUHH7

@staabm staabm changed the title Added failling test Fixed InterRangeType multiplication by a negative constant value Sep 12, 2021
@@ -232,8 +232,8 @@ public function math($i, $j, $z, $pi, $r1, $r2, $rMin, $rMax, $x, $y) {
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('int<min, 15>', $rMin * $z);
assertType('int<-100, max>', $rMax * $z);
assertType('int', $rMin * $z);
Copy link
Contributor Author

@staabm staabm Sep 12, 2021

Choose a reason for hiding this comment

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

Multiplication by $z leads to a integer range with no upper bound unioned with a range without a lower bound. the previous expectation was wrong.

@staabm staabm changed the title Fixed InterRangeType multiplication by a negative constant value Fixed InterRangeType multiplication by a negative constant Sep 12, 2021
@staabm
Copy link
Contributor Author

staabm commented Sep 12, 2021

Remaining failures seem unrelated - otherwise good to go

@staabm staabm marked this pull request as ready for review September 12, 2021 17:57
@@ -1021,12 +1021,7 @@ private function resolveType(Expr $node): Type
}

if ($type instanceof IntegerRangeType) {
$negativeRange = $this->resolveType(new Node\Expr\BinaryOp\Mul($node->expr, new LNumber(-1)));

if ( $negativeRange instanceof IntegerRangeType && ($negativeRange->getMin() === null || $negativeRange->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.

This check was moved into the integerRangeMath method and therefore fixes multiplication with a negative number for the general case

@staabm staabm marked this pull request as draft September 13, 2021 07:19
@staabm staabm changed the title Fixed InterRangeType multiplication by a negative constant Fixed InterRangeType multiplication/division of maximas by a negative constant Sep 13, 2021
@staabm
Copy link
Contributor Author

staabm commented Sep 13, 2021

I just added the same patch for division, which initially only was made for multiplication.

we could make this even more complete by supporting multiplication/division with negative intervals, e.g. int<min, 5> * int<-2, -5> but atm this looks like a rare edge case.

the PR as is already fixes a few glitches, therefore I would like to land it as is.

build failures seem unrelated. ready to go.

//cc @orklah

@staabm staabm marked this pull request as ready for review September 13, 2021 08:24
@ondrejmirtes ondrejmirtes merged commit 386b5bc into phpstan:master Sep 13, 2021
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the patch-2 branch September 13, 2021 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants