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

implemented math on IntegerRangeType and ConstantIntegerType #637

Merged
merged 21 commits into from Aug 27, 2021

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Aug 23, 2021

initial pass on implementing simple math operations on the IntegerRangeType.
closes phpstan/phpstan#5457
closes phpstan/phpstan#5515

for the time beeing I have only implemented + and - between ConstantIntegerType and IntegerRangeType to gather initial feedback on the implemation details.

I am aware that this is missing lot of stuff, e.g.

  • multiple/divide operation
  • left type is ConstantIntegerType and right type is IntegerRangeType
  • union types containing integer-range (like we do in the modulo operator)

possible future improvements (separate PR)

@staabm
Copy link
Contributor Author

staabm commented Aug 24, 2021

the remaining out-of-memory error is unrelated. I think this is good to go.

in the PR description I have separated the possible future improvements from the todo list.

@staabm staabm marked this pull request as ready for review August 24, 2021 07:26
@staabm staabm changed the title implemented math on IntegerRangeType implemented math on IntegerRangeType and ConstantIntegerType Aug 24, 2021
Copy link
Contributor Author

@staabm staabm left a comment

Choose a reason for hiding this comment

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

@ondrejmirtes I think this one is now good to go.

assertType('int<-19, 13>', $r1 + $z);
assertType('int<-2, 30>', $r1 - $z);
assertType('int<-20, 30>', $r1 * $z);
assertType('0', $r1 / $z);
Copy link
Contributor Author

@staabm staabm Aug 26, 2021

Choose a reason for hiding this comment

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

the only case, for which I am kind of suprised/not sure is this assertion. everything else feels solid to me

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is definitely a bug, it's never a zero.

Also, there are more divison bugs - in some cases where a float can be produced, the correct type should be int<X, Y>|float.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh yes.. obviously we can get a float here :-).

fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Did you also cover a case where only int can be produced? And maybe don't do a range but a union of constant integers?

Like:

// $x 20|40|60
// $y 2|4

\PHPStan\dumpType($x / $y); // 5|10|15|20|30

Copy link
Member

Choose a reason for hiding this comment

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

Might already be covered by MutatingScope::calculateFromScalars actually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just added your testcase which succeeds

@@ -54,10 +54,6 @@ public function getFilenameParts(string $filename): array
}

$dotsCount = $parentPartsCount - $i;
if ($dotsCount < 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

with the new knowledge phpstan gets by this PR, it now knows that this case can never happen.
removed the code, because we get:

 ------ --------------------------------------------------------------------- 
  Line   src/File/ParentDirectoryRelativePathHelper.php                       
 ------ --------------------------------------------------------------------- 
  58     Comparison operation "<" between int<0, max> and 0 is always false.  
 ------ --------------------------------------------------------------------- 

Copy link
Member

Choose a reason for hiding this comment

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

Wow ❤️

assertType('int<-19, 13>', $r1 + $z);
assertType('int<-2, 30>', $r1 - $z);
assertType('int<-20, 30>', $r1 * $z);
assertType('0', $r1 / $z);
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is definitely a bug, it's never a zero.

Also, there are more divison bugs - in some cases where a float can be produced, the correct type should be int<X, Y>|float.

assertType('int<-19, 13>', $r1 + $z);
assertType('int<-2, 30>', $r1 - $z);
assertType('int<-20, 30>', $r1 * $z);
assertType('0|float', $r1 / $z);
Copy link
Member

Choose a reason for hiding this comment

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

Still don't get this case. It can be a non-zero integer as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a line to handle this case explicitly. the test was adjusted to float only, without the 0.

@ondrejmirtes ondrejmirtes merged commit 65b91aa into phpstan:master Aug 27, 2021
@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
3 participants