-
Notifications
You must be signed in to change notification settings - Fork 449
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
Add basic type narrowing in the loose equality with the empty string #3304
base: 1.11.x
Are you sure you want to change the base?
Conversation
You've opened the pull request against the latest branch 1.12.x. If your code is relevant on 1.11.x and you want it to be released sooner, please rebase your pull request and change its target to 1.11.x. |
We need to solve two things in regard to
I'm in favour of adding a new method (or methods) on I'd like the implementations to easily follow the loose comparison table from https://www.php.net/manual/en/types.comparisons.php. The new method on Type could return a union of all types it loosely compares to with
To resolve the result of For type narrowing, we can run I'm not saying this will work for all cases, maybe we'll have to adjust, but it's a start. |
I'm a bit lost on this approach, but I'd be happy to investigate it. Should I open a dedicated issue for it? This PR is about getting one subset of the problem handled. |
I'm sorry, I don't want a narrow partial solution if we can do a proper one. |
I still don't understand the approach of returning the types, are you sure it shouldn't be an looseIntersectWith($type, $version)? How would you tackle things that don't involve basic constants, like Anyway I managed to convert it to a plugin by leveraging the nette DI, I had to override also the TypeSpecifierFactory, as there is an explicit reference to the class name there. |
Another example that comes to mind is: if ($a == 15) ...
In this case So a proper solution is really not easy. I think the best solution is to follow the approach in this PR and just handle all the cases in the table there, I basically handlded the empty string case but it can easily be duplicated for the other cases. Would you accept the PR if I complete the table in the docs? |
I'm working on an updated PR to handle more cases. This also fixes a problem I found, see this: https://phpstan.org/r/62751f1c-596f-4d7c-8a82-ff027b0515d5 Note that the evaluation of the expression actually gets it right, it's the type narrowing that gets it wrong.. is there no possibility to get this done in one method without duplicating the logic? |
181bcf0
to
d222675
Compare
As promised a long time ago, I submit my first real attempt to narrow the type after
if ($a != "") ...
This is by no means a complete analysis of the loose equality operator, but it should be sound. The discrepancy between php 7.x and 8.x are handled by preserving the two offending types (integer and float zero) in the type for both the true and false context.
I have the feeling that the definitive solution would be to replace
Type::looseCompare()
withType::looseIntersectWith()
, which should solve all the loose comparison problems. Do you think it would be something worth pursuing?If in the meanwhile you think this PR is acceptable, it would solve a lot of problems in my codebase.