Report maybe division by zero in InvalidBinaryOperationRule

phpstan · Sep 26, 2024 · 6dd9603 · 6dd9603
1 parent 5d4f259
commit 6dd9603
Show file tree

Hide file tree

Showing 5 changed files with 342 additions and 4 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<string, string>|bool|int|object|resource) and BinaryOpBenevolentUnion\\Foo results in an error.',
 				105,
 			],
+			[
+				'Binary operation "/" between (array<string, string>|bool|int|object|resource) and (array<string, string>|bool|int|object|resource) might result in an error.',
+				108,
+			],
+			[
+				'Binary operation "/=" between 1 and (array<string, string>|bool|int|object|resource) might result in an error.',
+				111,
+			],
 			[
 				'Binary operation "/=" between array{} and (array<string, string>|bool|int|object|resource) results in an error.',
 				114,
@@ -667,14 +683,34 @@ public function testBenevolentUnion(): void
 				'Binary operation "/=" between BinaryOpBenevolentUnion\\Foo and (array<string, string>|bool|int|object|resource) results in an error.',
 				117,
 			],
+			[
+				"Binary operation \"/=\" between '123' and (array<string, string>|bool|int|object|resource) might result in an error.",
+				120,
+			],
+			[
+				'Binary operation "/=" between 1.23 and (array<string, string>|bool|int|object|resource) might result in an error.',
+				123,
+			],
+			[
+				'Binary operation "/=" between (array<string, string>|bool|int|object|resource) and (array<string, string>|bool|int|object|resource) might result in an error.',
+				126,
+			],
 			[
 				'Binary operation "%" between (array<string, string>|bool|int|object|resource) and array{} results in an error.',
 				135,
 			],
 			[
-				'Binary operation "%" between (array<string, string>|bool|int|object|resource) and BinaryOpBenevolentUnion\\Foo results in an error.',
+				'Binary operation "%" between (array<string, string>|bool|int|object|resource) and BinaryOpBenevolentUnion\Foo results in an error.',
 				136,
 			],
+			[
+				'Binary operation "%" between (array<string, string>|bool|int|object|resource) and (array<string, string>|bool|int|object|resource) might result in an error.',
+				139,
+			],
+			[
+				'Binary operation "%=" between 1 and (array<string, string>|bool|int|object|resource) might result in an error.',
+				142,
+			],
 			[
 				'Binary operation "%=" between array{} and (array<string, string>|bool|int|object|resource) results in an error.',
 				145,
@@ -683,6 +719,18 @@ public function testBenevolentUnion(): void
 				'Binary operation "%=" between BinaryOpBenevolentUnion\\Foo and (array<string, string>|bool|int|object|resource) results in an error.',
 				148,
 			],
+			[
+				"Binary operation \"%=\" between '123' and (array<string, string>|bool|int|object|resource) might result in an error.",
+				151,
+			],
+			[
+				'Binary operation "%=" between 1.23 and (array<string, string>|bool|int|object|resource) might result in an error.',
+				154,
+			],
+			[
+				'Binary operation "%=" between (array<string, string>|bool|int|object|resource) and (array<string, string>|bool|int|object|resource) might result in an error.',
+				157,
+			],
 			[
 				'Binary operation "-" between (array<string, string>|bool|int|object|resource) and array{} results in an error.',
 				166,
@@ -810,4 +858,94 @@ public function testBug7863(): void
 		]);
 	}
 
+	/**
+	 * @dataProvider dataDivisionByMaybeZero
+	 * @param list<array{0: string, 1: int, 2?: string|null}> $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,
+				],
+			],
+		];
+	}
+
 }