phpstan / phpstan-webmozart-assert

PHPStan extension for webmozart/assert
160 stars 27 forks source link

Fix 130 #134

Closed herndlm closed 2 years ago

herndlm commented 2 years ago

Closes https://github.com/phpstan/phpstan-webmozart-assert/issues/118 Closes https://github.com/phpstan/phpstan-webmozart-assert/issues/119 Closes https://github.com/phpstan/phpstan-webmozart-assert/issues/130

Looks like the rootExpr approach is potentially making problems if the same thing is asserted twice in a row, where previously it would detect the second one as always evaluating to true/false. But this could be related to the all* implementation here too.

ondrejmirtes commented 2 years ago

Looks like the rootExpr approach is potentially making problems

Can you show the false positive here?

herndlm commented 2 years ago

It's just that https://github.com/phpstan/phpstan-webmozart-assert/blob/1.1.2/tests/Type/WebMozartAssert/data/impossible-check.php#L26 is not reported anymore, but maybe I need to adapt something else here

herndlm commented 2 years ago

@ondrejmirtes I took another look to find out why that assertion is not reported anymore. The step debugging revealed the following for the second assertion:

image

What baffles me is the scope state. $b is already correct, the array_filter(... is correct (the same as $b), but the $b === array_filter(.. expression resolves still to a BooleanType instead of a ConstantBooleanType. Any idea where I could/should look maybe? In MutatingScope I guess? :) UPDATE: ah nevermind, looks like I got it :)

herndlm commented 2 years ago

It's coming from https://github.com/phpstan/phpstan-src/blob/1.6.4/src/Analyser/MutatingScope.php#L2345 but that makes sense. Since phpstan only knows that both left and right are array<mixed, Bar> it cannot know if they are equal or identical. Hmm, stuck again :/ Maybe we have to take a look at the specified types after all in some cases with $rootExpr given

herndlm commented 2 years ago

Came up with something new for the all* handling. Instead of the typewise-complex array_filter solution I switched to a more simple check of the first array element. Combined with the already existing arrayOrIterable helper that seems to be working fine. And helps solving the annoying impossible type checks. What do you think?

Also added 2 other regression tests, but the bugs behind them were most likely fixed by the recent typespecifier adaptions in phpstan itself 🎉

herndlm commented 2 years ago

I think this one's good enough, did not came up with anything better yet and it's not too weird either IMO.

ondrejmirtes commented 2 years ago

Thank you.