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

Greater and smaller expressions combined with null in a BooleanOr are incorrectly specified #6672

Closed
herndlm opened this issue Feb 19, 2022 · 4 comments · Fixed by phpstan/phpstan-src#1028

Comments

@herndlm
Copy link
Contributor

herndlm commented Feb 19, 2022

Bug report

E.g. $b > 17 || $b === null

Trying to narrow down comparison expressions like that (where the variable is int|null) seems to be making problems if combined with a BooleanOr to add e.g. a null type.

I tried debugging this, the only thing that I found out so far is that the Greater is transformed to a Smaller expression which results in a MixedType with subtracted types. And when that left type is unionized with the right type (null) then it looses the subtracted types somewhere, basically ending up with only a mixed and therefore not adding the expected IntegerRangeType of the left part.
So, I assume this is either a bug in the TypeSpecifier in regard to the subtractable MixedType or one in TypeCombinator::union (which calls compareTypesInUnion which calls intersectWithSubtractedType). I might continue debugging this. Maybe it can "simply" add back the NullType that it substracted on the left side somehow.
Also need to check how it behaves with Smaller, that might reveal more and be easier to debug actually..

The strict comparison error that is reported is also odd, but might be fixed if the specified types are. UPDATE: those are actually fine, the < includes null of course, my fault :)

Context: Noticed while implementing phpstan/phpstan-webmozart-assert#107

Btw. I did check and my previous adaptions (phpstan/phpstan-src@7abb7c9 and phpstan/phpstan-src@c6b5430) are not related, this happened before already ;)

Code snippet that reproduces the problem

https://phpstan.org/r/658e4af2-416b-4d3e-86cf-01197edae4c5

Expected output

No errors.

Did PHPStan help you today? Did it make you happy in any way?

Not in particular today, but I'm sure soon again :)

@herndlm
Copy link
Contributor Author

herndlm commented Feb 20, 2022

Wait, is this fine maybe after all? Since it can still be null on the left side? Sorry for the noise then. Need to rethink after I had my coffee..
Update: think this is a bug. Note for later self: the right part was a null sureType IIRC, which is odd since my recent changes should preserve the left mixed subtracted type and use that in the right. It could be that the actual problem is a bug in intersecting mixed types that have subtractions or the type in the scope is wrong and missing the subtracts or so.

@jlherren
Copy link

jlherren commented Feb 21, 2022

As you correctly guessed, this seems to be a bug in the type combinator. The type specifiers completely ignore the current types of variables and specify types assuming they could be anything, that's why they produce these annoying mixed~something types. So the left side of the expression specifies $b to be mixed~(int<min, 17>|bool|null), since int<min, 17>|bool|null> are the only representable types for $b that would fail the condition. The right side specifies $b as null.

Then going through SpecifiedTypes::intersectWith() we get TypeCombinator::union(mixed~(int<min, 17>|bool|null), null), which fails to return the expected mixed~(int<min, 17>|bool) and simply returns mixed. So in the end, nothing is specified.

I guess this could be fixed by teaching the type combinator to resolve mixed~(A|B) | A to mixed~B.

@herndlm
Copy link
Contributor Author

herndlm commented Feb 21, 2022

My current approach is to not ignore the current type in the type specifier any more, which seems to be working fine. But yeah, looks like I still have to deal with the TypeCombinator after all as you mentioned. Mostly because the comparisons always work with and result in the mixed with substractions. Maybe it can just be fixed in the TypeCombinator then hmm
#6674 is also related, but I'm not a 100% sure about that yet

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants