vimeo / psalm

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

Bitwise assignment doesn't change type #1340

Open bugreportuser opened 5 years ago

bugreportuser commented 5 years ago

If $x is false, using |= true should make it 1.

Example (https://getpsalm.org/r/11805c0b24):

<?php
$x = false;
$x |= rand(0, 1) !== 2;
$x |= true;
$x |= 1;
if ($x) {
    echo $x;
}

Example output:

Psalm output (using commit a428b34):

ERROR: TypeDoesNotContainType - 6:5 - Found a contradiction when evaluating $x and trying to reconcile type 'false' to !falsy
muglug commented 5 years ago

I've fixed this for ints - not sure about bools

bugreportuser commented 5 years ago

Thanks. Are you willing to add support for bools later or at least a warning?

muglug commented 5 years ago

Yeah, going to keep this open because it's a valid use case

SignpostMarv commented 5 years ago

Just checking in; is the following example a separate issue to this one? https://psalm.dev/r/222baddf7b

bugreportuser commented 5 years ago

@SignpostMarv It's a separate issue but similar to #2139.

orklah commented 3 years ago

This has been reworked since and now boolean operations with bitwise operators emits an error. Not sure we can still consider this a valid use case

bugreportuser commented 3 years ago

You're right. I've had to avoid this for other tools too, so this issue isn't a priority. It's surprising php hasn't started supporting bitwise operators for booleans since it would be very useful

tvdijen commented 3 years ago

This issue still exists, yet the error messages have changed over time; https://psalm.dev/r/aa2d948c03

psalm-github-bot[bot] commented 3 years ago

I found these snippets:

https://psalm.dev/r/aa2d948c03 ```php
AndrolGenhald commented 2 years ago

I went and implemented this with the strictBooleanOperands config, but then I decided it's a really bad idea and scrapped it.

I figured for consistency we should support all the boolean operators:

$bool = random_int(0, 1) ? true : false;

/** @psalm-check-type-exact $var = 1|2 */
$var = $bool + 1;
/** @psalm-check-type-exact $var = -1|-2 */
$var = $bool - 2;
/** @psalm-check-type-exact $var = 0|1 */
$var = $bool % 3;
/** @psalm-check-type-exact $var = 0|2 */
$var = $bool * 2;
/** @psalm-check-type-exact $var = 0|1 */
$var = $bool ** 3;
/** @psalm-check-type-exact $var = 4|5 */
$var = $bool | 4;
/** @psalm-check-type-exact $var = 2|3 */
$var = $bool ^ 3;
/** @psalm-check-type-exact $var = 0|1 */
$var = $bool & 3;
/** @psalm-check-type-exact $var = 0|4 */
$var = $bool << 2;
/** @psalm-check-type-exact $var = 0|1 */
$var = $bool >> 0;

But if we do that, then this doesn't show any issues:

/** @var string */
$str = "some string";
$pos = strpos($str, "z") + 1;

And I think catching that is more important than allowing |= on booleans. I think the real solution here is for PHP to add logical assignment operators &&= and ||= (ie someone should pick up php/php-src#4535), but until then I think casting is the way to go.

Should we go ahead and close this @orklah?

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/daa21fb9b5 ```php ```
https://psalm.dev/r/bdb02a1b35 ```php
orklah commented 2 years ago

I'm not sure why solving boolean operations make the strpos operation go without Issue, can you explain that?

AndrolGenhald commented 2 years ago

I pushed my attempt here so you can look at it. The problem is that the bool to int cast happens before the PossiblyFalseOperand is emitted. I hadn't considered to try both allowing the cast and emitting the issue, but I thought the point was more to make Psalm allow this pattern without any issues rather than to still show issues but have the correct types.

Especially after the casting improvements in #7696 I think the best solution for now is just for users to cast when doing this, it will result in correct types and no issues from Psalm.