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.

Sign up for GitHub

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

Jump to bottom

Report maybe division by zero in InvalidBinaryOperationRule #3419

Open
wants to merge 1 commit into
base: 1.12.x
Choose a base branch
from
+342 −4
Open

Report maybe division by zero in InvalidBinaryOperationRule #3419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions conf/config.level2.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ services:
class: PHPStan\Rules\Operators\InvalidBinaryOperationRule
arguments:
bleedingEdge: %featureToggles.bleedingEdge%
reportMaybes: %reportMaybes%
tags:
- phpstan.rules.rule
-
Expand Down
9 changes: 8 additions & 1 deletion src/Parser/TryCatchTypeVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
}
Expand Down
68 changes: 68 additions & 0 deletions src/Rules/Operators/InvalidBinaryOperationRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -27,6 +34,7 @@ public function __construct(
private ExprPrinter $exprPrinter,
private RuleLevelHelper $ruleLevelHelper,
private bool $bleedingEdge,
private bool $reportMaybes,
)
{
}
Expand Down Expand Up @@ -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()) {
Copy link
Member

Choose a reason for hiding this comment

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

This logic should be implemented as a $callback for the RuleLevelHelper above instead. It will make sure this is correctly reported on level 2 (when it's always zero) and on level 7 (when it might be a zero).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the callback passed to RuleLevelHelper is only invoked for union types and intersection types.
in our case we pass in a expr which is resolved into a IntegerRangeType, which means the callback does not have any effect in this context.

Copy link
Member

Choose a reason for hiding this comment

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

I get it. But still I'm missing at least asking for $this->reportMaybes so that maybe-zero is reported only on level 7+.

Copy link
Member

Choose a reason for hiding this comment

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

Hey, it'd still be useful if the RuleLeveLHelper eliminated some situations. I can see that in the tests you're currently asserting an error that shouldn't be asserted because it's on a BenevolentUnionType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried testing the benevolent case, but for some reason the benevolence is lost somewhere?

https://phpstan.org/r/c0d12375-468d-46a2-8778-fcedc0d5e26f

Copy link
Member

Choose a reason for hiding this comment

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

That's not how you type a benevolent union type in PHPDoc.

It's not officially supported, there's just a semi-secret pseudotype __benevolent<...>. Check the functionMap.

Copy link
Contributor Author

@staabm staabm Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ok, hopefully I got it right now. my understanding about the expected results about the divisor are

  • we should not error on benevolent unions containing float (no matter the $checkBenevolentUnionTypes flag)
  • we should not error on float
  • we should error on unions with int|float
  • we should error on benevolent unions with non-float but number-castable types respecting $checkBenevolentUnionTypes (e.g. (int|true))

I tried endless variants of how to represent this condition via RuleLevelHelper callback, but couldn't find one which works for all this conditions

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 [];
}

Expand All @@ -124,4 +165,31 @@ public function processNode(Node $node, Scope $scope): array
];
}

private function isDivisionByZeroCaught(Node $node): bool
staabm marked this conversation as resolved.
Show resolved Hide resolved
{
$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();
}

}
144 changes: 141 additions & 3 deletions tests/PHPStan/Rules/Operators/InvalidBinaryOperationRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.',
Copy link
Member

Choose a reason for hiding this comment

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

This one. It might be int / int so this shouldn't be reported.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I get it, but int can still include zero so this should be reported.

But: We can still create an artifical example for example of (float|int) that shouldn't be reported because we don't report X / float.

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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
],
],
];
}

}
Loading
Loading