Skip to content

Fix non-integer range comparisons #120

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

Closed

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Mar 3, 2022

Closes #118
Closes #119

The problem is that range types are only supported for ints. See also https://phpstan.org/r/4c476dc8-d156-4f32-a733-207ba74133c5

If those assertions would be called with anything else, e.g. a float, the extensions would not add any additional type information and therefore lead to the errors described with strict-rules enabled.

@herndlm
Copy link
Contributor Author

herndlm commented Mar 3, 2022

Wait, I'm gonna take a second look if there's not something to improve in the TypeSpecifier instead that would benefit PHPStan itself..

@herndlm herndlm marked this pull request as draft March 3, 2022 14:37
@dragosprotung
Copy link

I see that there are specific exceptions for integer types.

What about cases like:

function test(DateTimeInterface $value): void
{
    Assert::greaterThan($value, new DateTimeImmutable('now'));
}

@herndlm herndlm force-pushed the fix-non-integer-range-comparisons branch from 3354ab2 to f3636db Compare March 3, 2022 22:32
@herndlm
Copy link
Contributor Author

herndlm commented Mar 3, 2022

I see that there are specific exceptions for integer types.

What about cases like:

function test(DateTimeInterface $value): void
{
    Assert::greaterThan($value, new DateTimeImmutable('now'));
}

It's fine, because what I basically do is disabe the type specifying extension for everything that is not an int by not generating an expression that PHPStan would use to determine types. Which in term is used to determine the impossible-check-type-calls which would lead to those errors you reported.

I did not find a better fix, I think PHPStan is specifying types as good as it can and there is just no more type info for non-ints. And the only range-types supported here are ints as fare as I know. But I added DateTime test cases too, just in case :)

Still not a 100% happy though, I'd like to debug the impossible-check-helper too before keeping it like this.

@herndlm herndlm marked this pull request as ready for review March 3, 2022 22:35
@herndlm herndlm marked this pull request as draft March 3, 2022 23:07
@herndlm
Copy link
Contributor Author

herndlm commented Mar 4, 2022

@ondrejmirtes sorry to bother you, wanted to ask you about your opinion.

The problem here is that a MixedType with subtracts is considered a super type of the given FloatType in https://github.com/phpstan/phpstan-src/blob/1.4.7/src/Rules/Comparison/ImpossibleCheckTypeHelper.php#L215

I'm just wondering if this is correct or if you think there's a bug in PHPStan. The answer seems to be coming from the subtracted UnionType that is giving a no which get's negated in the MixedType.

More details about the Union:
IntegerRange is not (trinary -1) considered to be a super type of FloatType (should it be maybe maybe via https://github.com/phpstan/phpstan-src/blob/1.4.7/src/Type/IntegerRangeType.php#L237 as would be an IntegerType?)
ConstantBooleanType is not (trinary -1) considered to be super of FloatType
NullType is not (trinary -1) considered to be a super type of FloatType

image

UPDATE: Yeah I think the IntegerRangeType is not playing nice with the FloatType on super type detection. I'll create a PR in phpstan

@herndlm
Copy link
Contributor Author

herndlm commented Mar 4, 2022

As mentioned in the other PR I was wrong xD I think PHPStan is behaving correctly then and as long as Ondrej doesn't have a better idea I'll go with the changes here :)

@herndlm herndlm marked this pull request as ready for review March 4, 2022 12:09
'range' => static function (Scope $scope, Arg $value, Arg $min, Arg $max): Expr {
'range' => static function (Scope $scope, Arg $value, Arg $min, Arg $max): ?Expr {
$valueType = $scope->getType($value->value);
if ((new IntegerType())->isSuperTypeOf($valueType)->no()) {
Copy link
Member

Choose a reason for hiding this comment

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

What about mixed or float|int? Do they demonstrate this bug or not?

Copy link
Contributor Author

@herndlm herndlm Mar 5, 2022

Choose a reason for hiding this comment

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

I assume you're trying to find out if I should use !(new IntegerType())->isSuperTypeOf($valueType)->yes() instead? :)
Apparently they are not affected, but I need to be able to use it with int|null to not break e.g. nullOrRange. If that makes sense. I'm not sure what the best solution for that is. I'm not sure if I fully understood already why it doesn't work, I need to take a closer look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aaah, I understand why. At that point, when the expression is built, it is still the int|null union of course..

@herndlm herndlm marked this pull request as draft March 8, 2022 06:31
@herndlm herndlm mentioned this pull request Mar 10, 2022
@herndlm
Copy link
Contributor Author

herndlm commented Mar 10, 2022

We're tryin to fix this in PHPStan itself, see #127 (comment)

@herndlm herndlm closed this Mar 10, 2022
@herndlm herndlm deleted the fix-non-integer-range-comparisons branch March 10, 2022 10:48
@ondrejmirtes
Copy link
Member

The tests here would still be useful.

@herndlm
Copy link
Contributor Author

herndlm commented Mar 10, 2022

For sure, but they are currently failing?

@ondrejmirtes
Copy link
Member

...to merge them once this is fixed 😊

@herndlm
Copy link
Contributor Author

herndlm commented Mar 10, 2022

of course! sorry, misunderstood 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