Skip to content

Fix 130 #134

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

Merged
merged 2 commits into from
May 9, 2022
Merged

Fix 130 #134

merged 2 commits into from
May 9, 2022

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Apr 28, 2022

Closes #118
Closes #119
Closes #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
Copy link
Member

Looks like the rootExpr approach is potentially making problems

Can you show the false positive here?

@herndlm
Copy link
Contributor Author

herndlm commented Apr 28, 2022

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
Copy link
Contributor Author

herndlm commented May 2, 2022

@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
Copy link
Contributor Author

herndlm commented May 2, 2022

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 herndlm force-pushed the fix-130 branch 4 times, most recently from 8eaed3f to 7c8b85c Compare May 3, 2022 20:38
@herndlm
Copy link
Contributor Author

herndlm commented May 3, 2022

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 herndlm marked this pull request as ready for review May 3, 2022 20:40
@herndlm
Copy link
Contributor Author

herndlm commented May 8, 2022

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

@ondrejmirtes ondrejmirtes merged commit 45ee104 into phpstan:1.2.x May 9, 2022
@ondrejmirtes
Copy link
Member

Thank you.

@herndlm herndlm deleted the fix-130 branch May 9, 2022 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants