From 6dd9603e090ded457f9b30533abe874068b72d55 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Sun, 8 Sep 2024 10:26:22 +0200 Subject: [PATCH] Report maybe division by zero in InvalidBinaryOperationRule --- conf/config.level2.neon | 1 + src/Parser/TryCatchTypeVisitor.php | 9 +- .../Operators/InvalidBinaryOperationRule.php | 68 +++++++++ .../InvalidBinaryOperationRuleTest.php | 144 +++++++++++++++++- .../Rules/Operators/data/invalid-division.php | 124 +++++++++++++++ 5 files changed, 342 insertions(+), 4 deletions(-) create mode 100644 tests/PHPStan/Rules/Operators/data/invalid-division.php diff --git a/conf/config.level2.neon b/conf/config.level2.neon index 13993940325..bff9405438b 100644 --- a/conf/config.level2.neon +++ b/conf/config.level2.neon @@ -184,6 +184,7 @@ services: class: PHPStan\Rules\Operators\InvalidBinaryOperationRule arguments: bleedingEdge: %featureToggles.bleedingEdge% + reportMaybes: %reportMaybes% tags: - phpstan.rules.rule - diff --git a/src/Parser/TryCatchTypeVisitor.php b/src/Parser/TryCatchTypeVisitor.php index cca8bf4e3a3..47cdd5ddde0 100644 --- a/src/Parser/TryCatchTypeVisitor.php +++ b/src/Parser/TryCatchTypeVisitor.php @@ -24,7 +24,14 @@ public function beforeTraverse(array $nodes): ?array public function enterNode(Node $node): ?Node { - if ($node instanceof Node\Stmt || $node instanceof Node\Expr\Match_) { + if ( + $node instanceof Node\Stmt + || $node instanceof Node\Expr\Match_ + || $node instanceof Node\Expr\AssignOp\Div + || $node instanceof Node\Expr\AssignOp\Mod + || $node instanceof Node\Expr\BinaryOp\Div + || $node instanceof Node\Expr\BinaryOp\Mod + ) { if (count($this->typeStack) > 0) { $node->setAttribute(self::ATTRIBUTE_NAME, $this->typeStack[count($this->typeStack) - 1]); } diff --git a/src/Rules/Operators/InvalidBinaryOperationRule.php b/src/Rules/Operators/InvalidBinaryOperationRule.php index 517c41b9092..56b76aca7f6 100644 --- a/src/Rules/Operators/InvalidBinaryOperationRule.php +++ b/src/Rules/Operators/InvalidBinaryOperationRule.php @@ -2,17 +2,24 @@ namespace PHPStan\Rules\Operators; +use DivisionByZeroError; use PhpParser\Node; use PHPStan\Analyser\MutatingScope; use PHPStan\Analyser\Scope; use PHPStan\Node\Printer\ExprPrinter; +use PHPStan\Parser\TryCatchTypeVisitor; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Rules\RuleLevelHelper; use PHPStan\ShouldNotHappenException; +use PHPStan\Type\BenevolentUnionType; +use PHPStan\Type\Constant\ConstantIntegerType; use PHPStan\Type\ErrorType; +use PHPStan\Type\ObjectType; use PHPStan\Type\Type; +use PHPStan\Type\TypeCombinator; use PHPStan\Type\VerbosityLevel; +use function array_map; use function sprintf; use function strlen; use function substr; @@ -27,6 +34,7 @@ public function __construct( private ExprPrinter $exprPrinter, private RuleLevelHelper $ruleLevelHelper, private bool $bleedingEdge, + private bool $reportMaybes, ) { } @@ -108,6 +116,39 @@ public function processNode(Node $node, Scope $scope): array ->assignVariable($rightName, $rightType, $rightType); if (!$scope->getType($newNode) instanceof ErrorType) { + if ( + $this->reportMaybes + && ( + $node instanceof Node\Expr\AssignOp\Div + || $node instanceof Node\Expr\AssignOp\Mod + || $node instanceof Node\Expr\BinaryOp\Div + || $node instanceof Node\Expr\BinaryOp\Mod + ) + && !$this->isDivisionByZeroCaught($node) + && !$this->hasDivisionByZeroThrowsTag($scope) + ) { + $rightType = $scope->getType($right); + // as long as we don't support float-ranges, we prevent false positives for maybe floats + if ($rightType instanceof BenevolentUnionType && $rightType->isFloat()->maybe()) { + return []; + } + + $zeroType = new ConstantIntegerType(0); + if ($zeroType->isSuperTypeOf($rightType)->maybe()) { + return [ + RuleErrorBuilder::message(sprintf( + 'Binary operation "%s" between %s and %s might result in an error.', + substr(substr($this->exprPrinter->printExpr($newNode), strlen($leftName) + 2), 0, -(strlen($rightName) + 2)), + $scope->getType($left)->describe(VerbosityLevel::value()), + $scope->getType($right)->describe(VerbosityLevel::value()), + )) + ->line($left->getStartLine()) + ->identifier(sprintf('%s.invalid', $identifier)) + ->build(), + ]; + } + } + return []; } @@ -124,4 +165,31 @@ public function processNode(Node $node, Scope $scope): array ]; } + private function isDivisionByZeroCaught(Node $node): bool + { + $tryCatchTypes = $node->getAttribute(TryCatchTypeVisitor::ATTRIBUTE_NAME); + if ($tryCatchTypes === null) { + return false; + } + + $tryCatchType = TypeCombinator::union(...array_map(static fn (string $class) => new ObjectType($class), $tryCatchTypes)); + + return $tryCatchType->isSuperTypeOf(new ObjectType(DivisionByZeroError::class))->yes(); + } + + private function hasDivisionByZeroThrowsTag(Scope $scope): bool + { + $function = $scope->getFunction(); + if ($function === null) { + return false; + } + + $throwsType = $function->getThrowType(); + if ($throwsType === null) { + return false; + } + + return $throwsType->isSuperTypeOf(new ObjectType(DivisionByZeroError::class))->yes(); + } + } diff --git a/tests/PHPStan/Rules/Operators/InvalidBinaryOperationRuleTest.php b/tests/PHPStan/Rules/Operators/InvalidBinaryOperationRuleTest.php index a656898b2a8..657669d4670 100644 --- a/tests/PHPStan/Rules/Operators/InvalidBinaryOperationRuleTest.php +++ b/tests/PHPStan/Rules/Operators/InvalidBinaryOperationRuleTest.php @@ -19,11 +19,14 @@ class InvalidBinaryOperationRuleTest extends RuleTestCase private bool $checkImplicitMixed = false; + private bool $checkBenevolentUnionTypes = false; + protected function getRule(): Rule { return new InvalidBinaryOperationRule( new ExprPrinter(new Printer()), - new RuleLevelHelper($this->createReflectionProvider(), true, false, true, $this->checkExplicitMixed, $this->checkImplicitMixed, true, false), + new RuleLevelHelper($this->createReflectionProvider(), true, false, true, $this->checkExplicitMixed, $this->checkImplicitMixed, true, $this->checkBenevolentUnionTypes), + true, true, ); } @@ -282,7 +285,12 @@ public function testBug3515(): void public function testBug8827(): void { - $this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-8827.php'], []); + $this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-8827.php'], [ + [ + 'Binary operation "/" between int<0, max> and int<0, max> might result in an error.', + 25, + ], + ]); } public function testRuleWithNullsafeVariant(): void @@ -659,6 +667,14 @@ public function testBenevolentUnion(): void 'Binary operation "/" between (array|bool|int|object|resource) and BinaryOpBenevolentUnion\\Foo results in an error.', 105, ], + [ + 'Binary operation "/" between (array|bool|int|object|resource) and (array|bool|int|object|resource) might result in an error.', + 108, + ], + [ + 'Binary operation "/=" between 1 and (array|bool|int|object|resource) might result in an error.', + 111, + ], [ 'Binary operation "/=" between array{} and (array|bool|int|object|resource) results in an error.', 114, @@ -667,14 +683,34 @@ public function testBenevolentUnion(): void 'Binary operation "/=" between BinaryOpBenevolentUnion\\Foo and (array|bool|int|object|resource) results in an error.', 117, ], + [ + "Binary operation \"/=\" between '123' and (array|bool|int|object|resource) might result in an error.", + 120, + ], + [ + 'Binary operation "/=" between 1.23 and (array|bool|int|object|resource) might result in an error.', + 123, + ], + [ + 'Binary operation "/=" between (array|bool|int|object|resource) and (array|bool|int|object|resource) might result in an error.', + 126, + ], [ 'Binary operation "%" between (array|bool|int|object|resource) and array{} results in an error.', 135, ], [ - 'Binary operation "%" between (array|bool|int|object|resource) and BinaryOpBenevolentUnion\\Foo results in an error.', + 'Binary operation "%" between (array|bool|int|object|resource) and BinaryOpBenevolentUnion\Foo results in an error.', 136, ], + [ + 'Binary operation "%" between (array|bool|int|object|resource) and (array|bool|int|object|resource) might result in an error.', + 139, + ], + [ + 'Binary operation "%=" between 1 and (array|bool|int|object|resource) might result in an error.', + 142, + ], [ 'Binary operation "%=" between array{} and (array|bool|int|object|resource) results in an error.', 145, @@ -683,6 +719,18 @@ public function testBenevolentUnion(): void 'Binary operation "%=" between BinaryOpBenevolentUnion\\Foo and (array|bool|int|object|resource) results in an error.', 148, ], + [ + "Binary operation \"%=\" between '123' and (array|bool|int|object|resource) might result in an error.", + 151, + ], + [ + 'Binary operation "%=" between 1.23 and (array|bool|int|object|resource) might result in an error.', + 154, + ], + [ + 'Binary operation "%=" between (array|bool|int|object|resource) and (array|bool|int|object|resource) might result in an error.', + 157, + ], [ 'Binary operation "-" between (array|bool|int|object|resource) and array{} results in an error.', 166, @@ -810,4 +858,94 @@ public function testBug7863(): void ]); } + /** + * @dataProvider dataDivisionByMaybeZero + * @param list $expectedErrors + */ + public function testDivisionByMaybeZero(bool $checkBenevolentUnionTypes, array $expectedErrors): void + { + $this->checkBenevolentUnionTypes = $checkBenevolentUnionTypes; + + $this->analyse([__DIR__ . '/data/invalid-division.php'], $expectedErrors); + } + + public function dataDivisionByMaybeZero(): iterable + { + yield [ + false, + [ + [ + 'Binary operation "/" between int and int<0, max> might result in an error.', + 12, + ], + [ + 'Binary operation "/=" between int and int<0, max> might result in an error.', + 13, + ], + [ + 'Binary operation "%" between int and int<0, max> might result in an error.', + 21, + ], + [ + 'Binary operation "%=" between int and int<0, max> might result in an error.', + 22, + ], + [ + 'Binary operation "/" between int and int<0, max> might result in an error.', + 61, + ], + [ + 'Binary operation "/" between int and float|int might result in an error.', + 90, + ], + [ + 'Binary operation "/" between int and (int|true) might result in an error.', + 106, + ], + [ + 'Binary operation "/" between int and -5|int<0, max> might result in an error.', + 123, + ], + ], + ]; + + yield [ + true, + [ + [ + 'Binary operation "/" between int and int<0, max> might result in an error.', + 12, + ], + [ + 'Binary operation "/=" between int and int<0, max> might result in an error.', + 13, + ], + [ + 'Binary operation "%" between int and int<0, max> might result in an error.', + 21, + ], + [ + 'Binary operation "%=" between int and int<0, max> might result in an error.', + 22, + ], + [ + 'Binary operation "/" between int and int<0, max> might result in an error.', + 61, + ], + [ + 'Binary operation "/" between int and float|int might result in an error.', + 90, + ], + [ + 'Binary operation "/" between int and (int|true) might result in an error.', + 106, + ], + [ + 'Binary operation "/" between int and -5|int<0, max> might result in an error.', + 123, + ], + ], + ]; + } + } diff --git a/tests/PHPStan/Rules/Operators/data/invalid-division.php b/tests/PHPStan/Rules/Operators/data/invalid-division.php new file mode 100644 index 00000000000..ce444b7bbbb --- /dev/null +++ b/tests/PHPStan/Rules/Operators/data/invalid-division.php @@ -0,0 +1,124 @@ + $x + */ + public function doDiv(int $y, $x): void + { + echo $y / $x; + $y /= $x; + } + + /** + * @param int<0, max> $x + */ + public function doMod(int $y, $x): void + { + echo $y % $x; + $y %= $x; + } + + public function doMixed(int $y, $mixed): void + { + echo $y % $mixed; + $y %= $mixed; + } + + /** + * @param int $negative + * @param int<1, max> $positive + */ + public function doRanges(int $y, int $negative, int $positive): void + { + echo $y / $negative; + $y /= $negative; + + echo $y / $positive; + $y /= $positive; + } +} + +function (int $i, int $x): void { + $a = range(1, 3/2); + + if ($x !== 0) { + echo $i / $x; + } + + if ($x != 0) { + echo $i / $x; + } + + if ($x > 0) { + echo $i / $x; + } + + if ($x >= 0) { + echo $i / $x; + } + + try { + $i / $x; + } catch (\DivisionByZeroError $e) { + echo 'Division by zero'; + } +}; + +/** + * @throws \DivisionByZeroError + */ +function throws(int $i, int $x) { + return $i / $x; +}; + +class Divide +{ + public function doDiv(int $y, float $x): void + { + echo $y / $x; + } + + /** + * @param int|float $maybeFloat + */ + public function doDiv1(int $y, $maybeFloat): void + { + echo $y / $maybeFloat; + } + + /** + * @param __benevolent $benevolentMaybeFloat + */ + public function doDiv2(int $y, $benevolentMaybeFloat): void + { + echo $y / $benevolentMaybeFloat; + } + + /** + * @param __benevolent $benevolent + */ + public function doDiv3(int $y, $benevolent): void + { + echo $y / $benevolent; + } + + /** + * @param __benevolent $benevolent + */ + public function doDiv4(int $y, $benevolent): void + { + echo $y / $benevolent; + } +} + +/** + * @param int<0, max>|-5 $x + */ +function divUnion(int $y, $x): void +{ + echo $y / $x; +}