vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.55k stars 661 forks source link

false is not eliminated when comparing against != "" #10547

Open bwoebi opened 8 months ago

bwoebi commented 8 months ago

A comparison if ($key != "") { ... } does still identify $key as being string|false instead of string within the if.

https://psalm.dev/r/4fb572a4fe

The goal is being able to simply compare is not empty and not false at once.

Related to #6965.

psalm-github-bot[bot] commented 8 months ago

I found these snippets:

https://psalm.dev/r/4fb572a4fe ```php prop = $env; } } } ``` ``` Psalm output (using commit a75d26a): ERROR: PossiblyFalsePropertyAssignmentValue - 8:24 - $this->prop with non-falsable declared type 'null|string' cannot be assigned possibly false type 'false|string' ```
danog commented 8 months ago

Just using a direct check fixes: https://psalm.dev/r/fb503c39ce

Now, a new issue is emitted due to the recently merged https://github.com/vimeo/psalm/pull/10502, and both in case of the != '' comparison and in this new issue, the answer I'm inclined to give is the same: loose comparisons are generally a bad idea, and should be avoided whenever possible.

psalm-github-bot[bot] commented 8 months ago

I found these snippets:

https://psalm.dev/r/fb503c39ce ```php prop = $env; } } } ``` ``` Psalm output (using commit a2140cc): ERROR: RiskyTruthyFalsyComparison - 7:13 - Operand of type false|string contains type string, which can be falsy and truthy. This can cause possibly unexpected behavior. Use strict comparison instead. ```
bwoebi commented 8 months ago

A direct check would also compare against "0" which would not be intended here. I want just "" and null/false basically.

Obviously, writing it out would work, but it's extra verbose and also less efficient code.

kkmuffme commented 8 months ago

You could just use !empty()

If you really need this, give a PR a try (since I doubt anybody of the existing contributors will fix this, since most people use/require strict comparisons bc the loose comparisons are a pain and huge source of bugs)

bwoebi commented 8 months ago

empty() sadly also compares against "0" :-/

danog commented 8 months ago

loose comparisons are a pain and huge source of bugs

+++, it along with empty and isset are banned in my codebase (and I'm very thankful for that)

I consider this issue as super low priority, I'd rather finish working on https://github.com/vimeo/psalm/pull/10577 first :)