PossiblyFalseReference | PossiblyFalseArgument false-positive for Collection interface

[fluffycondor]

weirdan/doctrine-psalm-plugin 0.11.3
vimeo/psalm 3.17.2

use Doctrine\Common\Collections\Collection;

function processCollection(Collection $collection)
{
        $matched = $collection->filter(function ($collectionItem) {
            return rand(0, 1) === 1;
        });

        if (!$matched->isEmpty()) {
            $matched->first()->hi(); // PossiblyFalseReference - Cannot call method hi on possibly false value (see https://psalm.dev/105)
            $collection->removeElement($matched->first()); // PossiblyFalseArgument - Argument 1 of Doctrine\Common\Collections\Collection::removeElement cannot be false, possibly false value provided (see https://psalm.dev/104)
        }
}

Neither first() nor last() can't return false on Collection that is not empty.

[VincentLanglet]

Phpstan support the isEmpty check thanks to phpstan/phpstan-doctrine#172 and phpstan/phpstan-doctrine#173.

Is it possible to do something similar for psalm @muglug @weirdan @orklah ?
I'm ok doing the PR but I don't know how to implement this

[orklah]

@VincentLanglet I think something similar had been done on this PR: vimeo/psalm#5333

The goal was to change the result of ReflectionParameter::getType() based on the result of ReflectionParameter::hasType() through a plugin.

The PR was ultimately rejected because Matt had a different idea, but I'm pretty sure the code worked and could be reproduced here

[VincentLanglet]

@VincentLanglet I think something similar had been done on this PR: vimeo/psalm#5333

The goal was to change the result of ReflectionParameter::getType() based on the result of ReflectionParameter::hasType() through a plugin.

The PR was ultimately rejected because Matt had a different idea, but I'm pretty sure the code worked and could be reproduced here

Thanks @orklah, it helps me a lot. I just have two question:

• This implementation requires to add the statement in multiple class constructor of psalm and to add a getStmt to the MethodReturnTypeProviderEvent ; is it really needed ? If so, I'll need to do a PR to psalm first.
• In his implementation, the return type was

if ($type->isTrue()) {
    return new Type\Union([
        new Type\Atomic\TNamedObject(ReflectionType::class)
     ]);
}

In my case, I assume it will be more like

return new Type\Union([
    new Type\Atomic\TGenericObject()
]);

but how do I get the value I should pass to the TGenericObject ?

[orklah]

For the statement, it is needed to get access to the variable containing the PHPParser statement here: https://github.com/vimeo/psalm/pull/5333/files#diff-ef767cc8f02262b219a73ec7be49523e0ef0c53710dda71f9be63ef2c5220602R25, so yeah, hard to do without it.

For the second question, I expect you'll have to do something similar. If I take the code snippet above, then when you are in a context when isEmpty returned false, then it means you are in the if, and you should "filter out" false from $matched->first()

[VincentLanglet]

I started vimeo/psalm#6060 then.

[VincentLanglet]

I just read vimeo/psalm#5333 (comment) ; maybe it can be solve with some psalm-assert too ?

And I think there is something different between the Collection->isEmpty() check and the ReflectionParameter::hasType() one.

ReflectionParameter is immutable, Collection is not.

Which means that in

if ($collection->isEmpty()) {
    $collection->add($foo);
    $collection->first();
}

first should not be considered as returning always false by psalm.

Is there a way to do this ?

[orklah]

The line here: https://github.com/vimeo/psalm/pull/5333/files#diff-ef767cc8f02262b219a73ec7be49523e0ef0c53710dda71f9be63ef2c5220602R45 doesn't say "if hastype was true at one point, then gettype will not return null". It says "if hastype is true for this scope then gettype will not return null.

It doesn't say anything about the return value outside of the scope you're currently looking for (outside of the if, gettype may return something else.

For hastype/gettype, immutability makes this impossible (and it may be why Matt refused the PR to use assertions), but without immutability, you can only assume the value won't change in your scope. I don't think it's an issue to assume this for doctrine plugin

I'm not certain this will work or even if it possible to make it works somehow. Just saying that's how I would try :)

[VincentLanglet]

vimeo/psalm#6060 is merged, so I just need a new release of psalm to continue :)

[VincentLanglet]

@orklah I've started #94 now 4.9.0 psalm is release.

I just don't understand how to filter false from the default return value if you have time to review

[orklah]

Fixed in #94