report error for non-strict or empty comparison on truthy+falsy union

[kkmuffme]

• Fix Internal: non-strict checking leads to inconsistent behavior #10501 => when a type can contain types that can be truthy AND falsy, it will require strict comparison
• fix it for empty()
• fix empty always returned bool even in cases where it can only be true/false

[kkmuffme]

@orklah looks like this PR strikes a chord, as this issue is extremely widespread.

There's no way I can fix all these existing errors - can we update the baseline file please?

The shepherd ones should be fixed by someone eventually, as I'm sure there are at least a couple more bugs in psalm, similar to what I fixed in #10500 - just fixing those shepherd will fix them. But this needs to be on a separate issue/PR (those are all good first issues), as otherwise this PR will be ready come Psalm version 17...

[orklah]

I have no issue with baselining real bugs in Psalm itself.

Though it seems tests are failing a lot too and this is more an issue...

[kkmuffme]

Though it seems tests are failing a lot too and this is more an issue...

Which tests specifically? I only see tests that fail because they have that issue and it's reporting them as it should. Or what do you mean? (or is there no baseline for the tests themselves?)

[orklah]

there is no baseline for tests, the only baseline we have is for Psalm analyzing its own code

[kkmuffme]

Ok, this was a pain :( I fixed them all now

3 points I realized when fixing the issues:

• TypeDoesNotContainType might be confusing for this issue, so I created a new error type (RiskyTruthyFalsyComparison)
• the same issue also exists in empty() => added this check there too*
• lots of tests that reported that issue had exactly this issue, where it was unclear whether it meant to compare for the falsy/truthy type or also for the falsy/truthy possible type in the otherwise truthy type. I just made assumptions where it was possible and ignored the issue in all other ones to not risk breaking the test/intention of the test
• fixed some spelling mistakes and bugs in unrelated tests
• fixed empty() return type to be more specific
• fixed keyed arrays possibly undefined types to actually include null when not verified (generic arrays still have the previous behavior)

*because of this, people should ideally use your plugin https://github.com/orklah/psalm-not-empty, as that will get rid of lots of errors. Maybe we should include this in psalm core by default?

Please update the baseline and merge this :-) @orklah

[orklah]

You can update the baseline on your side by just running psalm with --update-baseline. Other than that and my comment above, seems good to merge :)

[kkmuffme]

Thanks, baseline updating worked using composer psalm-set-baseline

Also test-with-real-projects still fails - I guess there's a baseline too? How do I update that one?

[kkmuffme]

Yes. Afaik there were some tests that used the elvis which I changed to a normal ternary, bc I wasn't aware of this.
I'm not available for the next 2 weeks, anybody feel free to PR this.

[veewee]

Psalm is smart enough to narrow down types in situations like these, so I'm using it as a shortcut to do so in a lot of places. It's giving me quite some errors which I wouldn't consider harmfull but rather expected behaviour.
I'm not sure if dealing with elvis is enough, cause it's also valid when doing ternary checks or even in if-based validations.

But mostly I'm not sure if doing strict comparison over taking the educated shortcut is adding a lot of value.
So for now I'm gonna opt-out on this feature cause it feels a bit too restrictive.

I might be missing the context in which scenarios this is actually risky though.

[theodorejb]

I'm surprised to see a change like this introduced in a minor release. It is a very common pattern to use checks like:

if (!$possiblyEmptyStringOrNull) {
    throw new Exception("Missing required value");
}

// or
if ($intFlagOrNull) {
    doThingSinceOptionIsEnabled();
}

// or
$date = $row['dateStrOrNull'] ? new DateTime($row['dateStrOrNull']) : null;

I also don't see these as risky - it's using the language as designed to easily handle null values the same way as an empty string or 0 integer.

[kkmuffme]

I'm surprised to see a change like this introduced in a minor release

@theodorejb could you clarify why? As that would allow us to improve the readme perhaps.
Anyway, you can just suppress it in your config. When we released PHP 8.2 or 8.3 we included changes that had way more "breaking" character than what you have here.

---

The reason why I added this rule is bc this risky - while you might be aware when you create the code, refactoring might introduce unwanted behavior - but even when you write code you might not be aware. If you're in an organization with loads of developers, and I'm happy that psalm will catch those rather unlikely things too, since it's something that happens/get forgotten when you also have less experienced people write code.

The impetus is a bug in psalm itself that was caused by one of these unstrict comparisons: #10500

[kkmuffme]

Btw. the feedback is exactly what I expected from this rule, since it reports tons of errors for lots of cases where it might not be an issue at all in the end - but that's what happens with ambiguous code.
Especially since stricter types and PHP 8, you rarely ever see someone do $foo == false as most codebases disallow that. But for whatever reason they're fine with !$foo which is just the same thing.

[kkmuffme]

@veewee

Does it make sense to be very strict about Risky checks by default or are there acceptable scenarios ?

Yes there are, it's just impossible to know them from static analysis, since we cannot analyze the intent easily.
Just like there are scenarios where you can $foo == false, most codebases require you to use a strict comparison

[theodorejb]

could you clarify why?

Because these code patterns are very common and typically used intentionally to simplify code.

Now instead of if (!$possiblyEmptyStringOrNull) I have to write if ($possiblyEmptyStringOrNull === null || $possiblyEmptyStringOrNull === '' || $possiblyEmptyStringOrNull === '0') for the equivalent check. It makes the code far more verbose.

Having to write out all the extra conditions is itself risky since it introduces the possibility for bugs (e.g. accidentally using && instead of || or !== instead of ===).

[kkmuffme]

Yeah, that's the purpose. You can just suppress it via the config.

Does your codebase allow if ($possiblyEmptyStringOrNull == false) ? If not, then it shouldn't allow if (!$possiblyEmptyStringOrNull) either

[danog]

All in all, this new check is quite strict and is bound to affect a lot of codebases, but IMO, it's something that can catch potential bugs, which is always a good thing, and the reason why I merged this.

I did have some doubts on whether it should be on level 1 instead of level 2, maybe it's better to lower its level...

[theodorejb]

The impetus is a bug in psalm itself that was caused by one of these unstrict comparisons: #10500

It looks like in this case the bug was with !$arrayOrNull checks, which incorrectly handled an empty array the same as null.

Perhaps the new error could be limited to array comparisons like this? That does seem far more risky than checking for falsy strings/integers.

[kkmuffme]

Totally an option to split it into separate errors - the one we have now as fallback (e.g. where you have multiple) and for more specific cases, e.g. arrays (RiskyTruthyFalsyArrayComparison) or strings (RiskyTruthyFalsyStringComparison) you create a separate errors.
This is simple to add to psalm and you could give it a PR a try? @theodorejb

[bwoebi]

I would love to see separate checks for if (something-which-can-be-string) and if(something-never-string).
From my experience I've never had issues with array|null or bool|null or such, but only with strings, where "0" was also included, and if ("" != $str) instead if ($str) would be the right choice. For explicit checking for "0", "" and false/null there's then if ($str == true).

[aguerre]

I'm confused with this new report on empty() function, as it is usually used to check if it's null or "empty scalar value" (like 0 or ''). There is no risky comparison in it.
It's too sad that i had to deactivate this rule because of empty() checks warnings even though it is useful for other cases.
Can you imagine to split this rule in two ?

[kkmuffme]

Could you please create a small code snippet on psalm.dev to give an example of your use case, where you think it's not risky, as that might help identify cases where this rule shouldn't report an error.

[aguerre]

Yes, check this : https://psalm.dev/r/f95fd49c39

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/f95fd49c39

<?php

/**
 * Typical Doctrine Entity used in form
 */
class Address
{
    private ?string $streetName = null;
    private ?string $city = null;
    private ?int $zipCode = null;
    private ?string $country = null;

    public function isEmpty(): bool
    {
        return empty($this->streetName)
            && empty($this->city)
            && empty($this->zipCode)
            && empty($this->country)
        ;
    }
}

Psalm output (using commit 4b2c698):

ERROR: RiskyTruthyFalsyComparison - 15:16 - Operand of type null|string contains type string, which can be falsy and truthy. This can cause possibly unexpected behavior. Use strict comparison instead.

ERROR: RiskyTruthyFalsyComparison - 16:16 - Operand of type null|string contains type string, which can be falsy and truthy. This can cause possibly unexpected behavior. Use strict comparison instead.

ERROR: RiskyTruthyFalsyComparison - 17:16 - Operand of type int|null contains type int, which can be falsy and truthy. This can cause possibly unexpected behavior. Use strict comparison instead.

ERROR: RiskyTruthyFalsyComparison - 18:16 - Operand of type null|string contains type string, which can be falsy and truthy. This can cause possibly unexpected behavior. Use strict comparison instead.

[kkmuffme]

This is an example of where this rule makes sense: not all countries use zipCodes or people input an invalid value (0). How would you differentiate between a '' value and a not-set (null) value if you just check with empty?

[aguerre]

This is a french example where all fields are required, sorry for the lack of precision.
Here we WANT to check if null or empty string. The user shall not pass if there is no address set (untouched field or empty string post). I don't want to differentiate at this point.

[kkmuffme]

If it doesn't matter for you in the first place, then why use null at all? Could just initialized it with '' instead of null.
Then use === '' instead of empty

[stronk7]

I'm afraid, but this is the first time that I'm going to need to disable something because the solution doesn't make any sense to me.

Sure that it has been introduced for a good reason, but it destroys any attempt to use elvis, empty() and simple conditions when you're sure that the behaviour is the same for null/false/empty string (aka any falsy).

I honestly cannot see any advantage having to switch to the long alternative in a lot of cases. Said with all respects!

https://psalm.dev/r/78ca579efd

Ciao :-)

PS: In fact, this very same PR, where the feature was implemented is a great representation of the impact that it's going to cause on any project. The changeset is massive. Again, just IMO.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/78ca579efd

<?php

// Just a silly example with getenv() but can be other thousands of things.
$reallyRiskyOhMy = getenv('SOME') ?: 'default';
$soIsThisTooOhMy = empty(getenv('SOME')) ?: 'default';
if (getenv('SOME')) {
    $notSureHereOhMy = getenv('SOME');
} else {
    $notSureHereOhMy = 'default';
}

// Why do I need this when I want the same behaviour for any empty/falsy.
$doINeedThisOhMy = (getenv('SOME') !== false && getenv('SOME') !== '') ? getenv('SOME') : 'default';

echo $reallyRiskyOhMy;
echo $soIsThisTooOhMy;
echo $notSureHereOhMy;
echo $doINeedThisOhMy;

Psalm output (using commit 639bed0):

ERROR: RiskyTruthyFalsyComparison - 4:20 - Operand of type false|string contains type string, which can be falsy and truthy. This can cause possibly unexpected behavior. Use strict comparison instead.

ERROR: RiskyTruthyFalsyComparison - 5:20 - Operand of type false|string contains type string, which can be falsy and truthy. This can cause possibly unexpected behavior. Use strict comparison instead.

ERROR: RiskyTruthyFalsyComparison - 6:5 - Operand of type false|string contains type string, which can be falsy and truthy. This can cause possibly unexpected behavior. Use strict comparison instead.