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

support integer-ranges in pow() #1904

Merged
merged 12 commits into from Jan 12, 2023
Merged

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Oct 24, 2022

closes the last remaining todo of #5614

fixes phpstan/phpstan#5614

@staabm
Copy link
Contributor Author

staabm commented Oct 24, 2022

//cc @orklah

@staabm staabm marked this pull request as ready for review October 24, 2022 10:41
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

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.

This doesn't handle unions of integer ranges on both sides. Instead of instanceof *Type, it'd be better to handle this with a new method on Type. But of course that needs to be designed in order to be useful, ideally to replace other instances of instancoef IntegerRangeType and even maybe TypeUtils::getConstantIntegers().

@staabm
Copy link
Contributor Author

staabm commented Oct 26, 2022

it'd be better to handle this with a new method on Type

if I get you right we are talking about methods such as the following, right?

diff --git a/src/Type/Type.php b/src/Type/Type.php
index f02971559..5a00d7f1c 100644
--- a/src/Type/Type.php
+++ b/src/Type/Type.php
@@ -112,6 +112,16 @@ interface Type

        public function shuffleArray(): Type;

+       public function subtract(Type $subtrahend): Type;
+
+       public function sum(Type $summand): Type;
+
+       public function multiply(Type $multiplicand): Type;
+
+       public function divide(Type $divisor): Type;
+
+       public function pow(Type $exponent): Type;
+

or do you have somthing different in mind?

@ondrejmirtes
Copy link
Member

Yeah, that would be great, although "pow" isn't a word unlike the others, should be "exponentiate" probably.

@ondrejmirtes
Copy link
Member

We'll be able to get rid of a lot of code in InitializerExprTypeResolver :)

@clxmstaab clxmstaab force-pushed the pow-interval branch 2 times, most recently from 1b6b658 to 47d98bf Compare October 27, 2022 09:24
@staabm staabm marked this pull request as ready for review October 27, 2022 09:26
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@staabm
Copy link
Contributor Author

staabm commented Oct 27, 2022

implemented Type->exponentiate() for now and will do the others in separate PRs

return TypeCombinator::union(...$resultTypes);
$exponentiatedTyped = $leftType->exponentiate($rightType);
if (!$exponentiatedTyped instanceof ErrorType) {
return $exponentiatedTyped;
}

return $this->resolveCommonMath(new BinaryOp\Pow($left, $right), $leftType, $rightType);
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 kept this pass thru resolveCommonMath because otherwise we would loose support for operator-type specifiying extensions (even though atm, there is no test-coverage for using it with pow).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test added with #1933

src/Reflection/InitializerExprTypeResolver.php Outdated Show resolved Hide resolved
src/Type/Accessory/AccessoryNumericStringType.php Outdated Show resolved Hide resolved
src/Type/Accessory/AccessoryNonFalsyStringType.php Outdated Show resolved Hide resolved
src/Type/Accessory/AccessoryNonEmptyStringType.php Outdated Show resolved Hide resolved
src/Type/Accessory/AccessoryLiteralStringType.php Outdated Show resolved Hide resolved
@@ -685,6 +685,14 @@ public function tryRemove(Type $typeToRemove): ?Type
return null;
}

public function exponentiate(Type $exponent): Type
{
return new BenevolentUnionType([
Copy link
Member

Choose a reason for hiding this comment

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

What if all numeric types are subtracted?

Copy link
Contributor Author

@staabm staabm Oct 30, 2022

Choose a reason for hiding this comment

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

pow() supports a lot of types as exponent, see e.g. https://3v4l.org/ZtZVc

we would need a big union, which might not be really useful?

per https://www.php.net/manual/en/function.pow.php it supports mixed args, while phpstan only accepts int|float per functionMap.

src/Type/NullType.php Outdated Show resolved Hide resolved
public function exponentiate(Type $exponent): Type
{
$object = new ObjectWithoutClassType();
if (!$exponent instanceof NeverType && !$object->isSuperTypeOf($this)->no() && !$object->isSuperTypeOf($exponent)->no()) {
Copy link
Member

Choose a reason for hiding this comment

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

Objects can be exponentiated? Huh?

Copy link
Contributor Author

@staabm staabm Oct 30, 2022

Choose a reason for hiding this comment

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

yep, see

function (\GMP $a, \GMP $b): void {
assertType('GMP', pow($a, $b));
assertType('GMP', $a ** $b);
};
function (\stdClass $a, \GMP $b): void {
assertType('GMP|stdClass', pow($a, $b));
assertType('GMP|stdClass', $a ** $b);
};

I moved this pre-exisiting logic from InitializerExprTypeResolver

src/Type/StaticType.php Outdated Show resolved Hide resolved
src/Type/StringType.php Outdated Show resolved Hide resolved
}

if ($exponent instanceof ConstantScalarType) {
if ($exponent->getValue() == 0) {
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 cs-fixer will turn these into strict comparisons, which in turn breaks a lot of tests.
I think we should ignore these from the cs fixer

@staabm staabm force-pushed the pow-interval branch 3 times, most recently from 7ca9ef5 to 8cdf131 Compare October 30, 2022 21:28
@ondrejmirtes
Copy link
Member

There are some conflicts.

@staabm
Copy link
Contributor Author

staabm commented Jan 8, 2023

resolved

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.

Otherwise 👍

public function exponentiate(Type $exponent): Type
{
$helper = new ExponentiateHelper();
return $helper->exponentiate($this, $exponent);
Copy link
Member

Choose a reason for hiding this comment

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

It's weird to new this. Typically we inject these services through DI. But I understand that we're in a value object where DI is not accessible. In these cases I'd write this as a static method: ExponentiateHelper::exponentiate. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, fixed.

@staabm staabm force-pushed the pow-interval branch 2 times, most recently from afe0b90 to 33c011a Compare January 11, 2023 18:50
@ondrejmirtes ondrejmirtes merged commit ed14b2a into phpstan:1.9.x Jan 12, 2023
@ondrejmirtes
Copy link
Member

Thank you.

@staabm staabm deleted the pow-interval branch January 12, 2023 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants