Skip to content
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

Strict comparison for functions that return false on failure. #43

Closed
rquadling opened this issue Dec 22, 2016 · 11 comments
Closed

Strict comparison for functions that return false on failure. #43

rquadling opened this issue Dec 22, 2016 · 11 comments

Comments

@rquadling
Copy link
Contributor

For example:

if (false === realpath('nope')){}

Results in Strict comparison using === between bool and string will always evaluate to false.

Whilst the statement about the comparison is true, realpath() returns false for a non existent file.

I suspect realpath(string $path): ?string would solve this issue.

Which is PHP7.1. Not sure how many of the functions and methods in core/extensions have been updated to reflect this. Hopefully all of them!!

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Dec 22, 2016

Unfortunately reflection data of internal PHP functions and methods are neglected and often out of sync. During development of PHPStan I encountered and reported three bugs: 70960, 71077, 71416 (was not able to convince the devs in the last case). Each one of them is "fixed" in PHPStan by a special condition.

But your issue is weird. PHP does not report any return type for realpath: https://3v4l.org/QBXVf That means that PHPStan thinks it's mixed and should not report any errors on the result. My theory is that you define your own function realpath in the same namespace as your sample code and PHPStan reads the return type or PHPDoc from that.

Let me know if we're on the right path.

@rquadling
Copy link
Contributor Author

I've added further info on 71416 and re-opened it.

@rquadling
Copy link
Contributor Author

Made the test is_bool($path) && false === $path and all is well.

I think we need to have a mechanism to easily add broken PHP reflection to PHPStan so that PHPStan can "just do it's job" and the devs don't need to jump through hoops to get PHPStan happy.

@ondrejmirtes
Copy link
Member

It's still a mystery to me why PHPStan thought in your case that realpath returns boolean since PHP reflection does not report any return type. I suspect you have your own realpath implementation in the same namespace (for mocking purposes in your tests) and PHPStan looked at that.

As for broken PHP reflection - I have a few PHP bugfixes in the codebase (look for // PHP bug) so I prefer that these inconsistencies get reported to me and I can fix them for everyone. You can also write your own class reflection extension if the bug can be solved that way (nonexistent properties/methods).

@rquadling
Copy link
Contributor Author

\PHPExcel_Shared_File::realpath() ... But that's a static function and not called by the code in question. It is tucked away in a vendor directory and not supposed to be parsed by PHPStan (we don't check other devs code).

I'm pretty positive we haven't our own implementation of realpath.

REALLY positive!

@ondrejmirtes
Copy link
Member

OK, so try to come up with a reproducing testcase of this issue. The only possible explanation I have for this is the one I described.

@rquadling
Copy link
Contributor Author

Ha ha ha!

The issue was that the type of the property declared in its docblock to which I had assigned the result of realpath() was just string and not string|false.

@ondrejmirtes
Copy link
Member

You should use string|bool, because false is not a type but a concrete value :) Nice catch!

@JanTvrdik
Copy link
Contributor

because false is not a type but a concrete value

Since PHP 7, false is internally a type. Boolean is a union of true and false. For example typescript considers false also as specific type.

Supporting true and false as types allows better static analysis. It's also explicitly mentioned in phpDoc documentation.

@ondrejmirtes
Copy link
Member

OK, shouldn't be a problem to add it.

@ondrejmirtes
Copy link
Member

@JanTvrdik @hrach Implemented in 3f4435c. Thanks for the suggestion.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants