-
Notifications
You must be signed in to change notification settings - Fork 509
Use root expression when checking impossible types #1254
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
Use root expression when checking impossible types #1254
Conversation
Tbh, I was even expecting more errors. But it looks like it's doing too much now, not sure. E.g. it complains in https://github.com/phpstan/phpstan-doctrine/blob/1.3.3/tests/Type/Doctrine/Query/QueryResultTypeWalkerTest.php#L126 with
Not sure if this should even be looked at in the first place? |
302acc0
to
4711b3f
Compare
Some jobs didn't run because I misunderstood the documentation of GitHub Actions. Hopefully it's gonna be better now: 3bf2958 |
2aba5cc
to
f26133f
Compare
This looks clean and promising. Could you please try to simulate the situation that I outlined here? phpstan/phpstan-webmozart-assert#130 (comment)
With this improvement we'll be able to write extensions for functions like "ipv4"... |
the 10 webmozart failures are actual fixes, or respectively cases were phpstan got smarter :) |
I think it's not yet enough, I tried with #1068 here and with phpstan/phpstan-webmozart-assert#130 itself. I was running out of time, but I think such an expression would still resolve to a |
BTW I'm trying afew more commits above your PR: #1256 |
2b10c8b
to
ca6065c
Compare
Thx for the help Ondrej! I rebased on latest 1.6.x already, apparently |
OK, a first local check looks like you did it. great job! :) |
ca6065c
to
6a58237
Compare
Please let's not step on each other's toes right now, I'm very close to being green here and merging it :) |
just cherry-picking your changes xD but yeah, sorry for hogging CI, every push I thought you're done |
there's a new legit webmozart thing it found, but a couple other loosely-equal checks are now not reported anymore. is that fine? Let me know what to adapt |
Thank you :) I'm gonna wait for the issue bot and then go to bed :) And fix the integration tests in phpstan/phpstan tommorrow. |
A bigger mystery for me was that they actually were reported before. There's no logic in PHPStan to say that |
I think I thought the same while adding them. weird. ok then |
So it's fine to remove those |
than that shall be my last action for the day |
sorry, one more thing, it looks like |
Sure, should be covered here :) phpstan-src/src/Analyser/MutatingScope.php Lines 2353 to 2358 in 6bf335c
|
Wow, I'm shocked, the issue bot haven't found a single change in the issues :) But at least we haven't broken anything :) |
I hope this opens the door for furhter type specifier cleanups at least. or features |
Yeah, I think so, you can try it out in webmozart/assert extension :) Also this one should be possible now #1068 |
This is the implementation of the idea that Ondrej mentioned in phpstan/phpstan-webmozart-assert#130 (comment)
I'm not quite sure about details yet, it looks like it makes phpstan already smarter though. It also looks like phpstan now remembers more specified types as stated in https://phpstan.org/blog/remembering-and-forgetting-returned-values.
This PR is mostly to find out how much will break 👀