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

Report maybe division by zero in InvalidBinaryOperationRule #3419

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

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Sep 8, 2024

alternative implementation to #3417

closes phpstan/phpstan#11090

@staabm staabm changed the base branch from 2.0.x to 1.12.x September 8, 2024 08:26
@phpstan-bot
Copy link
Collaborator

You've opened the pull request against the latest branch 2.0.x. PHPStan 2.0 is not going to be released for months. If your code is relevant on 1.12.x and you want it to be released sooner, please rebase your pull request and change its target to 1.12.x.

@staabm
Copy link
Contributor Author

staabm commented Sep 8, 2024

(submitted, so we can discuss the 2 different approaches)

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

We should do it only for ConstantIntegerType. FloatType might always be a zero because we don't have float ranges yet.

|| $node instanceof Node\Expr\BinaryOp\Mod
) {
$zeroType = new UnionType([new ConstantIntegerType(0), new ConstantFloatType(0.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

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

@VincentLanglet
Copy link
Contributor

submitted a alternative impl as a base for discussion: #3419

So I assume you prefer to discuss about it here.

thanks for pointing this out.

You have to enable "missingCheckedExceptionInThrows", "DivisionByZeroError"

I don't think coverage for a division-by-zero error should be a opt-in thing and "hidden" in the config file.

I agree it could make sens to have such checks (at level 7 for int ranges).

Also, if you go in that path @staabm, currently you're implementing the rule for the / and %.
cf the throw point:

if (
($expr instanceof Expr\AssignOp\Div || $expr instanceof Expr\AssignOp\Mod) &&
!$scope->getType($expr->expr)->toNumber()->isSuperTypeOf(new ConstantIntegerType(0))->no()
) {
$throwPoints[] = ThrowPoint::createExplicit($scope, new ObjectType(DivisionByZeroError::class), $expr, false);
}

But there is also:

We should be consistent about the PHPStan checks.

There is already InvalidBinaryOperationRule

@ondrejmirtes wdyt about adding the logic of this PR into InvalidBinaryOperationRule (or the type system) while checking the config whether DivisionByZeroError was configured as missingCheckedExceptionInThrows to prevent duplicate errors?

For the "What to do about the DivisionByZeroError", I'm not sure looking for DivisionByZeroError in missingCheckedExceptionInThrows might be the right thing to do:

  • If I add DivisionByZeroError in the list of uncheckedException, it's maybe because I don't want to check for 0, so I won't want this rule too.
  • If I add Error in the list of uncheckedException, we dunno if I was thinking about DivisionByZeroError.

If we look at something similar, like the TypeError, we can see in https://phpstan.org/r/06610d2b-47e5-48c5-8e25-fe4eabc331c9 that

  • PHPStan reports an error about the type passed to the method
  • PHPStan consider the catch is useless (but it is https://3v4l.org/46LqD)

That's a (good ?) strategy, and we could do the same for DivisionByZeroError by considering that since there is a rule checking this error, we dunno care about the error anymore. But I'm not sure it's the wanted path or Ondrej wouldn't have open the following issue phpstan/phpstan#9146 (with the code https://phpstan.org/r/9fbbf072-4da5-4b79-9fda-4559405cb0f5).

So I dunno what's the best to avoid duplicated errors...

@staabm
Copy link
Contributor Author

staabm commented Sep 9, 2024

  • currently there is no ThrowTypeExtension

I have patch prepared, which will be submitted after other currently open PRs are merged

@staabm
Copy link
Contributor Author

staabm commented Sep 9, 2024

unrelated CI error will be fixed in #3424

@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@@ -659,6 +665,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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing "possible division by zero" error
4 participants