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

Fix types after ctype_digit() with casted parameter #2189

Merged
merged 2 commits into from Jan 19, 2023

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jan 19, 2023

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.

Can you please explain the changes and your findings since the last PR?

} else {
assertType('mixed', $mixed);
}
assertType('mixed', $mixed);

if (ctype_digit((int) $mixed)) {
assertType('int<48, 57>|int<256, max>|numeric-string', $mixed);
assertType('mixed', $mixed); // could be *NEVER*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as can be seen in https://3v4l.org/k3Qce casting the parameter for ctype-digit to int beforehand does not yield a usefull result at all.

therefore I adjusted the extension to only narrow on (string) call.

I think its not worth to add logic into the extension which turns all "bad uses" of casted parameters into a NeverType.

Copy link
Member

Choose a reason for hiding this comment

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

phpstan/phpstan#8736 is about (string) cast, not (int) cast, so please explain what changes for (string) that makes 1.9.13 not work.

Copy link
Contributor Author

@staabm staabm Jan 19, 2023

Choose a reason for hiding this comment

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

in 1.9.13 we assumed a parameter type before casting would be the same as the one the ctype_digit function narrows.

this is not entirely correct. you can actually pass in e.g. a 1 which casted to string is a numeric-string, but is not contained within the

$types = [
IntegerRangeType::fromInterval(48, 57), // ASCII-codes for 0-9
IntegerRangeType::createAllGreaterThanOrEqualTo(256), // Starting from 256 ints are interpreted as strings
];
if ($context->true()) {
$types[] = new IntersectionType([
new StringType(),
new AccessoryNumericStringType(),
]);
}

type.

what we actually need is a type which represents all values which after casted with (string) will lead in a numeric-string. thats also the reason why e.g. bool is narrowed to true

@@ -59,9 +61,17 @@ public function specifyTypes(FunctionReflection $functionReflection, FuncCall $n
$unionType = TypeCombinator::union(...$types);
$specifiedTypes = $this->typeSpecifier->create($exprArg, $unionType, $context, false, $scope);

if ($context->true() && $exprArg instanceof Cast) {
if ($context->true() && $exprArg instanceof Cast\String_) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

with this change, I just turn the narrowing for non-string casts back to what it was before the recent PR #2186

@staabm staabm marked this pull request as ready for review January 19, 2023 09:24
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@staabm staabm marked this pull request as draft January 19, 2023 09:45
@staabm staabm marked this pull request as ready for review January 19, 2023 09:48
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes ondrejmirtes merged commit 39c12da into phpstan:1.9.x Jan 19, 2023
@ondrejmirtes
Copy link
Member

Thank you.

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