vimeo / psalm

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

False-positive `RedundantCondition` #7776

Closed gharlan closed 2 years ago

gharlan commented 2 years ago

https://psalm.dev/r/1b1ca51578

<?php

/**
 * @param DateTime[] $bar
 */
function foo(?DateTime $foo, array $bar): void {
    if (
        $foo && !in_array($foo, $bar, true)
        || !$foo && $bar
    ) {
        return;
    }

    if ($foo) {
    }
}
psalm-github-bot[bot] commented 2 years ago

I found these snippets:

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

~It looks like the issue is with the negation of strict in_array, this and this should be identical, but Psalm seems to think that if $int is not in $ints then it must not be an int, which is incorrect. It's probably just comparing the types somewhere without accounting for the fact that they can have the same type but still be different.~

Scratch that, not sure what I was thinking there. Here's something that's actually useful though, doing a strict comparison against null fixes it.

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

I found these snippets:

https://psalm.dev/r/39b01f458c ```php $ints */ function foobar(?int $int, array $ints): bool { if ($int === null) { return true; } return in_array($int, $ints, true); } ``` ``` Psalm output (using commit f0b2142): No issues! ```
https://psalm.dev/r/37e040f7fe ```php $ints */ function foobar(?int $int, array $ints): bool { if (!in_array($int, $ints, true)) { return false; } return $int === null; } ``` ``` Psalm output (using commit f0b2142): ERROR: TypeDoesNotContainNull - 10:12 - int does not contain null ```
AndrolGenhald commented 2 years ago

Well I git bisected it to #6233, but I'm not actually sure that's at fault, it seems more like a boolean algebra problem. I think this is the same problem without in_array (proof it's wrong since I keep messing it up). git bisecting that points to a832d77d73aab8fcb498b3c7c8c82bfd50df30ec, which seems more relevant. Maybe @muglug can help with this?

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

I found these snippets:

https://psalm.dev/r/3395485445 ```php
muglug commented 2 years ago

The conjunctive normal form version (which Psalm uses internally) also shows the bug: https://psalm.dev/r/edc2283627

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

I found these snippets:

https://psalm.dev/r/edc2283627 ```php
muglug commented 2 years ago

Fixed in the master branch

muglug commented 2 years ago

The culrpit was bad logic here.

It was simplifying (A || B || !B) to (A), whereas it should have simplified it to true

gharlan commented 2 years ago

@muglug Thanks for the quick fix. But shouldn't the fix target the 4.x branch?

orklah commented 2 years ago

We're starting to have big differences between 4.x and master.

Fixing this in 4.x means basically fixing an old version then rebasing that on master and getting conflicts, so basically fixing it twice.

Recent events kinda disrupted availability and motivation of the team, I was hoping Psalm 5 would be available by now. In the meantime, it means some bugs are getting fixed on master unfortunately.

However, feel free to switch to our master branch and report us any bug that could appear!