phpstan-src icon indicating copy to clipboard operation
phpstan-src copied to clipboard

Narrow types based on non-strict equality with constant types

Open staabm opened this issue 1 year ago • 11 comments

closes https://github.com/phpstan/phpstan/issues/10481

staabm avatar Jan 25 '24 13:01 staabm

You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x.

phpstan-bot avatar Jan 25 '24 13:01 phpstan-bot

Hi, I'd like you to know that I welcome this effort, but I'm unlikely to merge it, because this area is more complex than this PR admits or implements. For example $mixed == 'backend' should not result in 'backend' but $mixed can also be true. And there are different strings that would be equal to false. This PR does not address that at all.

You've already tried a similar problem once (https://github.com/phpstan/phpstan-src/pull/2216) but didn't finish it.

ondrejmirtes avatar Jan 25 '24 13:01 ondrejmirtes

Also the behaviour is PHP version-dependent: https://3v4l.org/rij8A

ondrejmirtes avatar Jan 25 '24 16:01 ondrejmirtes

thanks for the input.

what do you think about turning the assertions into (for all truethy-constant comparisons) ?

if ($mixed == 'backend') {
			assertType("mixed~0|0.0|''|'0'|array{}|false|null", $mixed);
}

I feel we can't really get much better then do some blacklisting in this conditions for now?

staabm avatar Jan 25 '24 16:01 staabm

I'm for doing as precise types as possible, but it'd be a substantial effort.

ondrejmirtes avatar Jan 25 '24 16:01 ondrejmirtes

and would you be open to do it in small PRs, like e.g. doing "mixed vs. constant-type equal compare narrowing" in a single PR? (or any other small step you are "ok" with would work for me - so I don't need to build up a multi month effort in a single PR)

staabm avatar Jan 25 '24 16:01 staabm

We need to design a new method on Type for this. For example if you compare non-falsy-string against a different type, it tells us something about both left and right type if the comparison is true and something else about both types if the comparison is false.

The information from this method can be also used to figure out "is this always true or always false" comparison, similarly to what ImpossibleCheckTypeHelper figures out from the TypeSpecifier output.

Once we get this design right and have a proof of concept for a small number of Types, we can merge that and then implement the method in other types in smaller chunks, making the analysis more precise gradually.

ondrejmirtes avatar Jan 25 '24 16:01 ondrejmirtes

my thinking is

example: non-falsey-string == int

-> take all candidate values out of the comparison table for the involved types

non-falsey-string -> "1", "-1" , "php" int -> 1, 0, -1

-> based on the comparison table .. the results for these candidates are

php7:

"1" x 1 => true, "-1" x 1 => false , "php" x 1 => false

"1" x 0 => false, "-1" x 0 => false , "php" x 0 => true

"1" x -1 => false, "-1" x -1 => true , "php" x -1 => false

php8+:

"1" x 1 => true, "-1" x 1 => false , "php" x 1 => false

"1" x 0 => false, "-1" x 0 => false , "php" x 0 => false

"1" x -1 => false, "-1" x -1 => true , "php" x -1 => false

-> this means for the IF branch

php7: non-falsey-string stays non-falsey-string php8: non-falsey-string gets narrowed to numeric-string

php7: stays int php8: gets narrowed to int~0

-> this means for the ELSE branch

php7: non-falsey-string stays non-falsey-string php8: non-falsey-string stays non-falsey-string

php7: staysint php8: stays int


I think this means we need a type-method like Type->looseEqual(Type $other): Type - meaning the looseEqual method returns the resulting type of the comparison.

staabm avatar Jan 25 '24 19:01 staabm

Yeah, something like that, the method also needs to have PhpVersion passed in.

ondrejmirtes avatar Jan 25 '24 20:01 ondrejmirtes

Looking longer at the comparison table, I think it would be helpful to add a new non-numeric-string type. Do you agree?

staabm avatar Jan 27 '24 07:01 staabm

Ahh just found https://github.com/phpstan/phpstan/issues/10239#issuecomment-1837571316

staabm avatar Jan 27 '24 07:01 staabm