-
Notifications
You must be signed in to change notification settings - Fork 428
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-range type in min/max #604
Conversation
build errors are unrelated |
) { | ||
$rangeType = $firstType; | ||
$intType = $secondType; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have this implemented with isSuperTypeOf
logic instead of doing instanceof *Type
. You're cover more ground with that. Learn more: https://phpstan.org/developing-extensions/type-system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried several variants, but didn't find one with the same coverage this currently implements.
isSuperTypeOf
does not work on IntegerRangeType
as I cannot instantiate a IntegerRangeType.
the logic I use here only works for ConstantIntegerType
in combination with IntegerRangeType
.
I have no idea how I can express this properly with isSuperTypeOf
for the code at hand, without additonal instanceof *Type
checks
&& $secondType instanceof IntegerRangeType | ||
) { | ||
$rangeType = $secondType; | ||
$intType = $firstType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an idea for complete reimplementation of this - you can rewrite min($x, $y)
as $x < $y ? $x : $y
. What if we did this instead (pseudocode)?
$scope->getType(new Ternary(
new Smaller($args[0], $args[1]),
$args[0],
$args[1]
));
And handled it in TypeSpecifier around this area?
phpstan-src/src/Analyser/TypeSpecifier.php
Lines 465 to 555 in f7250db
if ($leftType instanceof ConstantIntegerType) { | |
if ($expr->right instanceof Expr\PostInc) { | |
$result = $result->unionWith($this->createRangeTypes( | |
$expr->right->var, | |
IntegerRangeType::fromInterval($leftType->getValue(), null, $offset + 1), | |
$context | |
)); | |
} elseif ($expr->right instanceof Expr\PostDec) { | |
$result = $result->unionWith($this->createRangeTypes( | |
$expr->right->var, | |
IntegerRangeType::fromInterval($leftType->getValue(), null, $offset - 1), | |
$context | |
)); | |
} elseif ($expr->right instanceof Expr\PreInc || $expr->right instanceof Expr\PreDec) { | |
$result = $result->unionWith($this->createRangeTypes( | |
$expr->right->var, | |
IntegerRangeType::fromInterval($leftType->getValue(), null, $offset), | |
$context | |
)); | |
} | |
} | |
if ($rightType instanceof ConstantIntegerType) { | |
if ($expr->left instanceof Expr\PostInc) { | |
$result = $result->unionWith($this->createRangeTypes( | |
$expr->left->var, | |
IntegerRangeType::fromInterval(null, $rightType->getValue(), -$offset + 1), | |
$context | |
)); | |
} elseif ($expr->left instanceof Expr\PostDec) { | |
$result = $result->unionWith($this->createRangeTypes( | |
$expr->left->var, | |
IntegerRangeType::fromInterval(null, $rightType->getValue(), -$offset - 1), | |
$context | |
)); | |
} elseif ($expr->left instanceof Expr\PreInc || $expr->left instanceof Expr\PreDec) { | |
$result = $result->unionWith($this->createRangeTypes( | |
$expr->left->var, | |
IntegerRangeType::fromInterval(null, $rightType->getValue(), -$offset), | |
$context | |
)); | |
} | |
} | |
if ($context->true()) { | |
if (!$expr->left instanceof Node\Scalar) { | |
$result = $result->unionWith( | |
$this->create( | |
$expr->left, | |
$orEqual ? $rightType->getSmallerOrEqualType() : $rightType->getSmallerType(), | |
TypeSpecifierContext::createTruthy(), | |
false, | |
$scope | |
) | |
); | |
} | |
if (!$expr->right instanceof Node\Scalar) { | |
$result = $result->unionWith( | |
$this->create( | |
$expr->right, | |
$orEqual ? $leftType->getGreaterOrEqualType() : $leftType->getGreaterType(), | |
TypeSpecifierContext::createTruthy(), | |
false, | |
$scope | |
) | |
); | |
} | |
} elseif ($context->false()) { | |
if (!$expr->left instanceof Node\Scalar) { | |
$result = $result->unionWith( | |
$this->create( | |
$expr->left, | |
$orEqual ? $rightType->getGreaterType() : $rightType->getGreaterOrEqualType(), | |
TypeSpecifierContext::createTruthy(), | |
false, | |
$scope | |
) | |
); | |
} | |
if (!$expr->right instanceof Node\Scalar) { | |
$result = $result->unionWith( | |
$this->create( | |
$expr->right, | |
$orEqual ? $leftType->getSmallerType() : $leftType->getSmallerOrEqualType(), | |
TypeSpecifierContext::createTruthy(), | |
false, | |
$scope | |
) | |
); | |
} | |
} |
You might even find out it already works. Additionally, fixing some additional cases to make these tests pass would improve engine not just for min
/max
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Do you mean we would achieve this by rewriting the AST? If so, would this rewriting happen within a extension somewhere or as part of the phpstan-core logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No rewriting necessary. The idea is that the entire code of the extension for min()
should be:
return $scope->getType(new Ternary(
new Smaller($args[0], $args[1]),
$args[0],
$args[1]
));
And the whole logic should be handled in the linked code in TypeSpecifier.
Of course it gets a little complicated for multiple arguments and $arg–>unpack
but it might be achievable with BooleanAnd and BooleanOr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to keep this PR as is in case of problems with the different approach, I created a new PR based on your suggestion:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I love the new PR, thanks :)
closes phpstan/phpstan#5398
this fixes the reported issue, but I can think of more complex scenarios which are not yet covered, e.g.
but I think these should be covered in a separate PR, as they are rare and this PR - as is - already covers a lot of real world code