vimeo / psalm

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

Psalm is complaining that a variable is a value that it can never be #9657

Open ollieread opened 1 year ago

ollieread commented 1 year ago

I've got Psalm 5.9.0 running on a project with error level 1, but I'm getting errors about things that aren't issues.

I have the following method:

/**
 * @template R of mixed
 *
 * @param \Smpl\Functional\Contracts\Func<V, R>|\Smpl\Functional\Contracts\BiFunc<K, V, R> $mapper
 *
 * @return \Smpl\Collections\Contracts\Stream<K, R, C>
 *
 * @psalm-this-out self<K, R, C>
 */
public function map(BiFunc|Func $mapper): Contracts\Stream
{
    $elements = [];

    /**
     * @var K $key
     * @var V $value
     */
    foreach ($this->collection as $key => $value) {
        if ($mapper instanceof BiFunc) {
            /** @var R $value */
            $value = $mapper->apply($key, $value);
        } else {
            $value = $mapper->apply($value);
        }

        $elements[$key] = $value;
    }

    $this->collection = $this->collection->fresh($elements);

    return $this;
}

I'm getting the following errors for the following lines.

$value = $mapper->apply($key, $value);

This produces the following error, complaining that the type of $mapper is Func, despite it being wrapped in an if statement that makes sure it isn't.

ERROR: InvalidArgument - src/Collections/Stream.php:118:41 - Argument 1 of Smpl\Functional\Contracts\Func::apply expects V:Smpl\Collections\Stream as mixed, but K:Smpl\Collections\Stream as mixed provided (see https://psalm.dev/004)
                $value = $mapper->apply($key, $value);

Amusingly, I'm also getting:

ERROR: TooFewArguments - src/Collections/Stream.php:120:35 - Too few arguments for Smpl\Functional\Contracts\BiFunc::apply - expecting arg2 to be passed (see https://psalm.dev/025)
                $value = $mapper->apply($value);

ERROR: TooFewArguments - src/Collections/Stream.php:120:35 - Too few arguments for method Smpl\Functional\Contracts\BiFunc::apply saw 1 (see https://psalm.dev/025)
                $value = $mapper->apply($value);

ERROR: InvalidArgument - src/Collections/Stream.php:120:41 - Argument 1 of Smpl\Functional\Contracts\BiFunc::apply expects K:Smpl\Collections\Stream as mixed, but V:Smpl\Collections\Stream as mixed provided (see https://psalm.dev/004)
                $value = $mapper->apply($value);

For the following line that exists in the else branch, and will literally never be touched if $mapper is in fact an instance of BiFunc:

$value = $mapper->apply($value);
orklah commented 1 year ago

Please try to reproduce on psalm.dev

It's hard to think about why it happens without knowing for example, the hierarchy between Func and BiFunc

ollieread commented 1 year ago

Please try to reproduce on psalm.dev

It's hard to think about why it happens without knowing for example, the hierarchy between Func and BiFunc

I tried, but I can't recreate it. Func and BiFunc have no hierarchy. They are Func<mixed, mixed> and BiFunc<mixed, mixed, mixed>.

ollieread commented 1 year ago

If I change the whole if statement to the following:

if ($mapper instanceof BiFunc) {
    $value = $mapper->apply($key, $value);
} else if ($mapper instanceof Func) {
    $value = $mapper->apply($value);
}

It still complains. In fact, the only way it doesn't, is if I make sure the second call ISN'T a BiFunc, which is pretty ridiculous,

if ($mapper instanceof BiFunc) {
    $value = $mapper->apply($key, $value);
} else if ($mapper instanceof Func && ! ($mapper instanceof BiFunc)) {
    $value = $mapper->apply($value);
}
ollieread commented 1 year ago

The two interfaces Func and BiFunc can be seen here: https://gist.github.com/ollieread/97f46aa77d85c5006f51244845a4d100

ygottschalk commented 1 year ago

Somewhat of a reproducer: https://psalm.dev/r/07b8322f27

What (I think what) happens here: Psalm thinks $mapper might implement Func and BiFunc at the same time. That is why psalm can not pin down the type of $mapper->apply just with the one instanceof-check, bcs psalm thinks it still might implement both function contracts.

On the other hand it should be possible to know that in the else case $mapper can not be of instance BiFunc. Possibly some bug with type reconciliation and template params I'd guess...

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/07b8322f27 ```php */ interface Collection extends \Traversable { } /** * @template K of mixed * @template V of mixed * @template C of mixed */ class Stream { /** @var Collection $collection */ private Collection $collection = null; /** * @template R of mixed * * @param (\Smpl\Functional\Contracts\Func)|(\Smpl\Functional\Contracts\BiFunc) $mapper * * @return \Smpl\Collections\Contracts\Stream * * @psalm-this-out self */ public function map(BiFunc|Func $mapper): Stream { $elements = []; /** * @var K $key * @var V $value */ foreach ($this->collection as $key => $value) { if ($mapper instanceof BiFunc) { $value = $mapper->apply($key, $value); } else { $value = $mapper->apply($value); } $elements[$key] = $value; } $this->collection = $this->collection->fresh($elements); return $this; } } ``` ``` Psalm output (using commit 4bc226e): ERROR: UnnecessaryVarAnnotation - 116:17 - The @var K annotation for $key is unnecessary ERROR: UnnecessaryVarAnnotation - 117:17 - The @var V annotation for $value is unnecessary ERROR: InvalidArgument - 121:41 - Argument 1 of Smpl\Functional\Contracts\Func::apply expects V:Smpl\Collections\Contracts\Stream as mixed, but K:Smpl\Collections\Contracts\Stream as mixed provided INFO: MixedAssignment - 121:17 - Unable to determine the type that $value is being assigned to ERROR: InvalidArgument - 123:41 - Argument 1 of Smpl\Functional\Contracts\BiFunc::apply expects K:Smpl\Collections\Contracts\Stream as mixed, but V:Smpl\Collections\Contracts\Stream as mixed provided ERROR: TooFewArguments - 123:35 - Too few arguments for Smpl\Functional\Contracts\BiFunc::apply - expecting arg2 to be passed ERROR: TooFewArguments - 123:35 - Too few arguments for method Smpl\Functional\Contracts\BiFunc::apply saw 1 INFO: MixedAssignment - 126:13 - Unable to determine the type of this assignment ERROR: UndefinedInterfaceMethod - 129:48 - Method Smpl\Collections\Contracts\Collection::fresh does not exist INFO: MixedAssignment - 129:9 - Unable to determine the type that $this->collection is being assigned to ERROR: InvalidReturnStatement - 131:16 - The inferred type 'Smpl\Collections\Contracts\Stream&static' does not match the declared return type 'Smpl\Collections\Contracts\Stream' for Smpl\Collections\Contracts\Stream::map ERROR: InvalidReturnType - 107:16 - The declared return type 'Smpl\Collections\Contracts\Stream' for Smpl\Collections\Contracts\Stream::map is incorrect, got 'Smpl\Collections\Contracts\Stream&static' ERROR: PossiblyNullPropertyAssignmentValue - 100:38 - $this->collection with non-nullable declared type 'Smpl\Collections\Contracts\Collection' cannot be assigned nullable type 'null' ```
ollieread commented 1 year ago

Somewhat of a reproducer: https://psalm.dev/r/07b8322f27

What (I think what) happens here: Psalm thinks $mapper might implement Func and BiFunc at the same time. That is why psalm can not pin down the type of $mapper->apply just with the one instanceof-check, bcs psalm thinks it still might implement both function contracts.

On the other hand it should be possible to know that in the else case $mapper can not be of instance BiFunc. Possibly some bug with type reconciliation and template params I'd guess...

I think you might be right, in terms of what is happening, because getting errors that it is simultaneously both implementations is...weird.

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/07b8322f27 ```php */ interface Collection extends \Traversable { } /** * @template K of mixed * @template V of mixed * @template C of mixed */ class Stream { /** @var Collection $collection */ private Collection $collection = null; /** * @template R of mixed * * @param (\Smpl\Functional\Contracts\Func)|(\Smpl\Functional\Contracts\BiFunc) $mapper * * @return \Smpl\Collections\Contracts\Stream * * @psalm-this-out self */ public function map(BiFunc|Func $mapper): Stream { $elements = []; /** * @var K $key * @var V $value */ foreach ($this->collection as $key => $value) { if ($mapper instanceof BiFunc) { $value = $mapper->apply($key, $value); } else { $value = $mapper->apply($value); } $elements[$key] = $value; } $this->collection = $this->collection->fresh($elements); return $this; } } ``` ``` Psalm output (using commit 4bc226e): ERROR: UnnecessaryVarAnnotation - 116:17 - The @var K annotation for $key is unnecessary ERROR: UnnecessaryVarAnnotation - 117:17 - The @var V annotation for $value is unnecessary ERROR: InvalidArgument - 121:41 - Argument 1 of Smpl\Functional\Contracts\Func::apply expects V:Smpl\Collections\Contracts\Stream as mixed, but K:Smpl\Collections\Contracts\Stream as mixed provided INFO: MixedAssignment - 121:17 - Unable to determine the type that $value is being assigned to ERROR: InvalidArgument - 123:41 - Argument 1 of Smpl\Functional\Contracts\BiFunc::apply expects K:Smpl\Collections\Contracts\Stream as mixed, but V:Smpl\Collections\Contracts\Stream as mixed provided ERROR: TooFewArguments - 123:35 - Too few arguments for Smpl\Functional\Contracts\BiFunc::apply - expecting arg2 to be passed ERROR: TooFewArguments - 123:35 - Too few arguments for method Smpl\Functional\Contracts\BiFunc::apply saw 1 INFO: MixedAssignment - 126:13 - Unable to determine the type of this assignment ERROR: UndefinedInterfaceMethod - 129:48 - Method Smpl\Collections\Contracts\Collection::fresh does not exist INFO: MixedAssignment - 129:9 - Unable to determine the type that $this->collection is being assigned to ERROR: InvalidReturnStatement - 131:16 - The inferred type 'Smpl\Collections\Contracts\Stream&static' does not match the declared return type 'Smpl\Collections\Contracts\Stream' for Smpl\Collections\Contracts\Stream::map ERROR: InvalidReturnType - 107:16 - The declared return type 'Smpl\Collections\Contracts\Stream' for Smpl\Collections\Contracts\Stream::map is incorrect, got 'Smpl\Collections\Contracts\Stream&static' ERROR: PossiblyNullPropertyAssignmentValue - 100:38 - $this->collection with non-nullable declared type 'Smpl\Collections\Contracts\Collection' cannot be assigned nullable type 'null' ```
ygottschalk commented 1 year ago

It is perfectly ok for a class to implement both interfaces. Actually tested that just moments ago. So it is the right thing that psalm takes that into account and reports to us if we forgot that...

But having a line like if( ! $mapper instanceof BiFunc) does nothing (type-wise) while if( ! $mapper instanceof Func) seem to work correctly...

orklah commented 1 year ago

If we try to display $mapper type: https://psalm.dev/r/882d7306e7

It outputs this (I cleaned up the output for easier reading): $mapper: BiFunc|(BiFunc&Func<V, R>)|(BiFunc<K, V, R>&Func<V, R>)|(Func<V, R>&Func<V, R>)

That seems a little odd to me, BiFunc seems to have lost its templates in some cases and we have an intersection with Func in both sides.

So I guess we have an issue in here. But once that is fixed, we should have something like

BiFunc<K, V, R>|Func<V, R>|(BiFunc<K, V, R>&Func<V, R>)

Checking if we have an instance of BiFunc does not allow to exclude that the variable is also a Func and I don't think Psalm allows to document that an interface is exclusive

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/882d7306e7 ```php */ interface Collection extends \Traversable { } /** * @template K of mixed * @template V of mixed * @template C of mixed */ class Stream { /** @var Collection $collection */ private Collection $collection = null; /** * @template R of mixed * * @param (\Smpl\Functional\Contracts\Func)|(\Smpl\Functional\Contracts\BiFunc) $mapper * * @return \Smpl\Collections\Contracts\Stream * * @psalm-this-out self */ public function map(BiFunc|Func $mapper): Stream { $elements = []; /** * @var K $key * @var V $value */ foreach ($this->collection as $key => $value) { /** @psalm-trace $mapper */; if ($mapper instanceof BiFunc) { $value = $mapper->apply($key, $value); } else { $value = $mapper->apply($value); } $elements[$key] = $value; } $this->collection = $this->collection->fresh($elements); return $this; } } ``` ``` Psalm output (using commit 542d627): ERROR: UnnecessaryVarAnnotation - 116:17 - The @var K annotation for $key is unnecessary ERROR: UnnecessaryVarAnnotation - 117:17 - The @var V annotation for $value is unnecessary INFO: Trace - 120:40 - $mapper: Smpl\Functional\Contracts\BiFunc|(Smpl\Functional\Contracts\BiFunc&Smpl\Functional\Contracts\Func)|(Smpl\Functional\Contracts\BiFunc&Smpl\Functional\Contracts\Func)|(Smpl\Functional\Contracts\Func&Smpl\Functional\Contracts\Func) ERROR: InvalidArgument - 122:41 - Argument 1 of Smpl\Functional\Contracts\Func::apply expects V:Smpl\Collections\Contracts\Stream as mixed, but K:Smpl\Collections\Contracts\Stream as mixed provided INFO: MixedAssignment - 122:17 - Unable to determine the type that $value is being assigned to ERROR: InvalidArgument - 124:41 - Argument 1 of Smpl\Functional\Contracts\BiFunc::apply expects K:Smpl\Collections\Contracts\Stream as mixed, but V:Smpl\Collections\Contracts\Stream as mixed provided ERROR: TooFewArguments - 124:35 - Too few arguments for Smpl\Functional\Contracts\BiFunc::apply - expecting arg2 to be passed ERROR: TooFewArguments - 124:35 - Too few arguments for method Smpl\Functional\Contracts\BiFunc::apply saw 1 INFO: MixedAssignment - 127:13 - Unable to determine the type of this assignment ERROR: UndefinedInterfaceMethod - 130:48 - Method Smpl\Collections\Contracts\Collection::fresh does not exist INFO: MixedAssignment - 130:9 - Unable to determine the type that $this->collection is being assigned to ERROR: InvalidReturnStatement - 132:16 - The inferred type 'Smpl\Collections\Contracts\Stream&static' does not match the declared return type 'Smpl\Collections\Contracts\Stream' for Smpl\Collections\Contracts\Stream::map ERROR: InvalidReturnType - 107:16 - The declared return type 'Smpl\Collections\Contracts\Stream' for Smpl\Collections\Contracts\Stream::map is incorrect, got 'Smpl\Collections\Contracts\Stream&static' ERROR: PossiblyNullPropertyAssignmentValue - 100:38 - $this->collection with non-nullable declared type 'Smpl\Collections\Contracts\Collection' cannot be assigned nullable type 'null' ```
ollieread commented 1 year ago

Thanks @orklah and @ygottschalk, this definitely seems to be a weird issue, though I have resolved my particular problem. Turns out that when I wrote the original Stream interface, I had intended to call $mapper->apply($value, $key) to make use of the shared method name, but somehow forgot that when I came to implement it.

I have encountered a bunch of other weird issues that I have suppressed for now, so I'll go through and add any more issues if I encounter them.