false positive on `InvalidPropertyAssignmentValue`

[p4veI]

Hello I came across this issue recently, but unfortunately I was unable to replicate with the online tool at psalm.dev

Consider this example:

final class Bag
{
    /** @var Map<Key, Provider> $providers */
    private Map $providers;

    public function __construct(ProviderA $providerA, ProviderB $providerB)
    {
        $this->providers = mapFromIterable(
            [$providerA, $providerB],
            static fn (mixed $_, Provider $provider) : Pair => new Pair(
                $provider::key(),
                $provider
            ),
        );
    }
}

I'm instantiating Map with a helper function with two instances of Provider, both implementing the Provider interface. Unfortunately psalm reports:

InvalidPropertyAssignmentValue - Bag.php:22:28 - $this->providers with declared type 'Ds\Map<Key, Provider>' cannot be assigned type 'Ds\Map<Key, ProviderA|ProviderB>'

For more details I'm including the internals of mapFromIterable:

/**
 * @param iterable<K, V> $iterable
 * @param callable(K, V): Pair<KReturn, VReturn> $mapper
 *
 * @return Map<KReturn, VReturn>
 *
 * @template K
 * @template V
 * @template KReturn
 * @template VReturn
 */
function mapFromIterable(iterable $iterable, callable $mapper) : Map
{
    /** @var Map<KReturn, VReturn> $map */
    $map = new Map();

    foreach ($iterable as $key => $value) {
        $keyValue = $mapper($key, $value);
        $map->put($keyValue->key, $keyValue->value);
    }

    return $map;
}

[orklah]

What are the types of Key and DataSource in your example? Are those interfaces? Classes? What are the relation between the two?

[p4veI]

What are the types of Key and DataSource in your example? Are those interfaces? Classes? What are the relation between the two?

ah sorry, I updated the example, DataSource was supposed to be => Key in the original example/message. I tried to simplify the error for reporting purposes.

In my code I'm using DataSource which is an enum class, so Key here refers to a class used as the Map key.

[orklah]

Unfortunately, I have zero experience with DS\Map and I have no idea how to use it. In addition to not having a reproducer, this makes resolving this quite difficult.

Could you try to create a small repo that can show the issue please?

[p4veI]

Unfortunately, I have zero experience with DS\Map and I have no idea how to use it. In addition to not having a reproducer, this makes resolving this quite difficult.

Could you try to create a small repo that can show the issue please?

Sure, prepared it here: https://github.com/p4veI/test-ds-psalm , hope it works.

[p4veI]

sidenote: feels like it's gonna be an issue with the generics on the mapFromIterable function perhaps - it's from an internal library of ours, but I added the function to the repo. While I was making the repository I tried to include the function as a public static method of a class rather than a plain function, and it didn't produce the error.

e.g Functions::mapFromIterable(...).

[klimick]

Simplified: https://psalm.dev/r/cccd83fa0f

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/cccd83fa0f

```php $items */ public function __construct(public array $items) {} } interface Foo {} interface Bar extends Foo {} interface Baz extends Foo {} function createBar(): Bar { throw new RuntimeException(); } function createBaz(): Baz { throw new RuntimeException(); } final class Ctx { /** @var Collection */ public Collection $items; public function __construct() { $this->items = new Collection([createBar(), createBaz()]); } } ``` ``` Psalm output (using commit e5a7e00): ERROR: InvalidPropertyAssignmentValue - 28:21 - $this->items with declared type 'Collection' cannot be assigned type 'Collection' ```

[klimick]

Psalm inference has limitation. During collection instantiation: new Collection([createBar(), createBaz()]) Psalm don't find most common type for Bar and Baz and infer Bar|Baz instead Foo.

Perhaps #4564 may fix it.

[klimick]

P.S. Collections from Ds are invariant. With covariant collections there is not issue: https://psalm.dev/r/91c7770c59

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/91c7770c59

```php $items */ public function __construct(public array $items) {} } interface Foo {} interface Bar extends Foo {} interface Baz extends Foo {} function createBar(): Bar { throw new RuntimeException(); } function createBaz(): Baz { throw new RuntimeException(); } final class Ctx { /** @var Collection */ public Collection $items; public function __construct() { $this->items = new Collection([createBar(), createBaz()]); } } ``` ``` Psalm output (using commit e5a7e00): No issues! ```

[AndrolGenhald]

With covariant collections there is not issue

Ah, that makes sense. Since the template is invariant Collection<Bar|Baz> can't be assigned to Collection<Foo>.

Psalm don't find most common type for Bar and Baz and infer Bar|Baz instead Foo.

I don't think we want to change how the inference works though, Collection<Bar|Baz> is the correct type there, I'd rather not have less-specific inference. I think what needs to change is something about the invariance check, maybe somehow allowing covariance only for inferred types?

This also has the issue without adding a union into the mix: https://psalm.dev/r/5ed7d169db Just trying to assign an inferred Collection<Bar> to a Collection<Foo> is the problem, and I don't think changing inference can fix it correctly anyway.

[orklah]

Psalm did find the common type but if the template is not covariant, it will prevent you to use a subtype here.

So, for me, it's not a bug

[AndrolGenhald]

I was thinking Rust might infer the type automatically, but it looks like at least for the example I was able to throw together it requires you to explicitly state the type: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=50b7009a382f2fbc3595cdba7a05c180 https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=99b51f18688e2fc1e4f37451c9dad764

See lines 45-46. The explicit Box<dyn Animal> is required, which is equivalent to us saying to do this you need to stick @var Collection<Foo> before the new Collection(...).

I think we could support this if we really wanted to: https://psalm.dev/r/9fbf6e84bc It might be possible to do something like an implicit @param-out and make it work without the argument actually being a reference: https://psalm.dev/r/dbe060609b

I don't particularly care to try it, but unless there are other issues I'm not seeing it might not be too difficult to support.

On a related note, it would be good to add an issue for this: https://psalm.dev/r/53c059464e

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/9fbf6e84bc

```php $items */ public function __construct(public array $items) {} } class Animal {} class Cat extends Animal {} class Dog extends Animal {} $animals = new Collection([new Dog()]); /** @psalm-trace $animals */; // Inferred to be Collection /** @psalm-suppress InvalidArgument */ takesAnimalCollection($animals); // After this call, $animals needs to be re-typed to Collection /** @psalm-trace $animals */; takesDogCollection($animals); // Should be InvalidArgument /** @param Collection $animals */ function takesAnimalCollection(Collection $animals): void { $animals->items[] = new Cat(); } /** @param Collection $_arg */ function takesDogCollection(Collection $_arg): void {} ``` ``` Psalm output (using commit bf22dcf): INFO: Trace - 18:29 - $animals: Collection INFO: Trace - 21:29 - $animals: Collection ```

https://psalm.dev/r/dbe060609b

```php $items */ public function __construct(public array $items) {} } class Animal {} class Cat extends Animal {} class Dog extends Animal {} $animals = new Collection([new Dog()]); /** @psalm-trace $animals */; // Inferred to be Collection /** @psalm-suppress InvalidArgument */ takesAnimalCollection($animals); // After this call, $animals needs to be re-typed to Collection /** @psalm-trace $animals */; takesDogCollection($animals); // Should be InvalidArgument /** * @param Collection $animals * @param-out Collection $animals */ function takesAnimalCollection(Collection &$animals): void { $animals->items[] = new Cat(); } /** @param Collection $_arg */ function takesDogCollection(Collection $_arg): void {} ``` ``` Psalm output (using commit bf22dcf): INFO: Trace - 18:29 - $animals: Collection INFO: Trace - 21:29 - $animals: Collection ERROR: ArgumentTypeCoercion - 22:20 - Argument 1 of takesDogCollection expects Collection, parent type Collection provided ```

https://psalm.dev/r/53c059464e

```php $items */ public function __construct(public array $items) {} } class Animal {} class Cat extends Animal {} class Dog extends Animal {} /** @param Collection $animals */ function takesAnimalCollection(Collection $animals): void { $animals->items[] = new Cat(); // Should be an error since Collection::T is covariant } ``` ``` Psalm output (using commit bf22dcf): No issues! ```

[AndrolGenhald]

Anyway, it seems like maybe templates on Ds classes should be covariant? Some of them already are, but not all of them. It looks like they're originally taken from PHPStan, it would be nice to know why they weren't covariant to begin with.