Allow BcMath\Number increment/decrement#305
Conversation
VincentLanglet
left a comment
There was a problem hiding this comment.
I prefer @ondrejmirtes opinion on such change since by definition this repository is about opinionated rules.
We might arg that rather than writing ++$number or $number + $number you could call the native method $number->add(...)
| @@ -68,7 +73,13 @@ public function isValidForDecrement(Scope $scope, Expr $expr): bool | |||
|
|
|||
| private function isSubtypeOfNumber(Scope $scope, Expr $expr): bool | |||
There was a problem hiding this comment.
This method is also used by isValidForArithmeticOperation so you have more impact than expected for instance it changes the behavior of the rule with
// Addition or Substraction
$a = BcMath\Number + BcMath\Number
$a = int - BcMath\Number
// Unary
$a = - BcMath\Number
// Division, Exponentiation, Modulo, Multiplication
...
I recommend you to add test for those case, and I'm not sure they should stop reporting error.
There was a problem hiding this comment.
You're right, I added more tests based on the existing operators.php test file. Looks like there were no issues with BcMath\Number before... I'm not sure why
There was a problem hiding this comment.
What I meant is that this is maybe safer to handle the special case directly inside isValidForIncrement and isValidForDecrement rather than inside isSubtypeOfNumber.
As an exemple isValidForIncrement has a special case
if ($type->isString()->yes()) {
// Because `$a = 'a'; $a++;` is valid
return true;
}
01b67d1 to
c54e639
Compare
c54e639 to
4e6eb9d
Compare
| $number % $mixed; | ||
| $mixed % $number; | ||
|
|
||
| $number += $number; |
There was a problem hiding this comment.
All the next operation should be done in dedicated function to avoid modiying the variabel.
As an example in https://phpstan.org/r/1577da80-3b14-4e29-8019-1fa57b3f155a
As an example the line 148 say
Binary operation "+=" between BcMath\Number and 123.456 results in an error.
and then
line 149 Binary operation "+=" between 123.456 and mixed results in an error.
meaning that all the $number variable are now *ERROR* and test nothing.
| $acceptedTypes[] = new ObjectType('BcMath\Number'); | ||
| } | ||
|
|
||
| $acceptedType = new UnionType($acceptedTypes); |
There was a problem hiding this comment.
Before supporting an union here, you might want to support an union on PHPStan side, cf
https://phpstan.org/r/3e185620-55d1-470c-ba3a-c7ae8dc603fd
| @@ -68,7 +73,13 @@ public function isValidForDecrement(Scope $scope, Expr $expr): bool | |||
|
|
|||
| private function isSubtypeOfNumber(Scope $scope, Expr $expr): bool | |||
There was a problem hiding this comment.
What I meant is that this is maybe safer to handle the special case directly inside isValidForIncrement and isValidForDecrement rather than inside isSubtypeOfNumber.
As an exemple isValidForIncrement has a special case
if ($type->isString()->yes()) {
// Because `$a = 'a'; $a++;` is valid
return true;
}
This allows
++/--on BcMath\Number objects when using strict rules.See https://phpstan.org/r/f3cd9326-5435-4000-b173-9e143d745acb
Related to phpstan/phpstan#13965
Would be nice to include tests for PHP < 8.4, but the tests don't emit any errors because type
BcMath\Numberis not found. If there's a way to inject a dummyBcMath\Numberclass then I think it could be done?