Psalm: Dog/Cat/Animal generic snafu should not be allowed

[muglug]

class Animal {}
class Dog extends Animal {}
class Cat extends Animal {}

/**
 * @template T
 */
class Collection {
    /**
     * @param T $t
     */
    public function add($t) : void {}
}

/**
 * @param Collection<Animal> $list
 */
function addAnimal(Collection $list) : void {
    $list->add(new Cat());
}

/**
 * @param Collection<Dog> $list
 */
function takesDogList(Collection $list) : void {
    addAnimal($list); // this should be an error
}

Expected: InvalidArgument when passing Collection<Dog> to Collection<Animal>
Actual: No issue

[iluuu1994]

Something like C#s in and out keywords would be necessary to understand if upcasting or downcasting is allowed:

• https://docs.microsoft.com/en-us/dotnet/standard/generics/covariance-and-contravariance
• https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/in-generic-modifier
• https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/out-generic-modifier

[muglug]

That's great info, thanks!

This is made a little more complicated/interesting with union types.

Currently Psalm treats MyCollection<A>|MyCollection<B> as identical to MyCollection<A|B>, but that's not right.

[iluuu1994]

Since a mutable collection uses the generic type T as both input and output (set(int index, T value) and get(int index): T) I think strictly speaking T is neither covariant nor contravariant.

class ReadOnlyCollection<out T> {
    get(int index): T
}

class WriteOnlyCollection<in T> {
    set(int index, T value)
}

class Collection<T> {
    set(int index, T value)
    get(int index): T
}

class Animal {}
class Dog extends Animal {}
class Cat extends Animal {}

let _: ReadOnlyCollection<Animal> = new ReadOnlyCollection<Dog>() // OK
let _: ReadOnlyCollection<Dog> = new ReadOnlyCollection<Animal>() // NOK
let _: ReadOnlyCollection<Dog|Cat> = new ReadOnlyCollection<Dog>() // OK
let _: ReadOnlyCollection<Dog> = new ReadOnlyCollection<Dog|Cat>() // NOK

let _: WriteOnlyCollection<Dog> = new WriteOnlyCollection<Animal>() // OK
let _: WriteOnlyCollection<Animal> = new WriteOnlyCollection<Dog>() // NOK
let _: WriteOnlyCollection<Dog> = new WriteOnlyCollection<Dog|Cat>() // OK
let _: WriteOnlyCollection<Dog|Cat> = new WriteOnlyCollection<Dog>() // NOK

let _: Collection<Animal> = new Collection<Dog>() // NOK
let _: Collection<Dog> = new Collection<Animal>() // NOK
let _: Collection<Dog|Cat> = new Collection<Dog>() // NOK
let _: Collection<Dog> = new Collection<Dog|Cat>() // NOK

Hope this helps.

[muglug]

Hmm but I also don't want an error with https://psalm.dev/r/2e0c889319

[iluuu1994]

Unfortunately, this should be an error since getSound could alter the collection and add an instance of any other class extending Animal. There are languages that ignore this for convenience but if you want to be safe you'd have to clone the collection or prove that the collection is not altered.

[iluuu1994]

You could allow casting Collection<Dog> to iterable<Animal> though since iterable isn't mutable. This would also make getSound more portable.

[muglug]

Maybe this check could be bypassed when a @psalm-immutable annotation exists on the class?

[muglug]

Where Traversable and iterable would be @psalm-immutable by default

[iluuu1994]

Maybe this check could be bypassed when a @psalm-immutable annotation exists on the class?

Sounds a little inflexible. It's not just about mutability. in and out restrict where you can use the generic parameters. Specifying out allows upcasting T but it restricts you from using it as a parameter type. in is the opposite. Traversible would be specified internally as Traversible<out T> and iterable as iterable<out T>. That's the approach taken by C#.

[muglug]

Yeah, I guess I'd just rather have the in/out not just on the params, but on the whole templated class.

So @psalm-template-allow-subtyping or similar, which would apply to all template params.

Sidenote: Hack has SomeContainer<T super Foo> which relates to something different - a constraint on what can be assigned to T

[muglug]

I have the implementation ready - it's just a matter of the circumstances under which this elseif branch is entered: https://github.com/vimeo/psalm/pull/1604/files#diff-6f471193a38ac3da0731e37bc4724239R1608

[iluuu1994]

Have you considered this case?

/**
 * @template TKey
 * @template KVal
 * @psalm-template-allow-subtyping
 */
class Collection
{
    /**
     * @param TKey $key
     * @return TVal
     */
    public function get($key)
    {
        // ...
    }
}

/** @var Collection<int, Dog> */
$dogCol = new Collection();
/** @var Collection<mixed, Animal> */
$animalCol = $dogCol;
$animalCol->get(new \stdClass());

How do you know which parameter is covariant/contravariant?

On the other hand, this:

/**
 * @template TKey
 * @template out KVal
 */
class Collection
{
    /**
     * @param TKey $key
     * @return TVal
     */
    public function get($key)
    {
        // ...
    }
}

/** @var Collection<int, Dog> */
$dogCol = new Collection();
/** @var Collection<mixed, Animal> */ // <-- This is invalid
$animalCol = $dogCol;
$animalCol->get(new \stdClass());

would allow Psalm to know that this isn't allowed.

[muglug]

Ok, you've convinced me it should be per-param - is out the best word?

[iluuu1994]

@muglug Can't think of anything better. covariant would be another option but I think out would be more intuitive for most people.

Are you also planning to error when T is used incorrectly?

https://dotnetfiddle.net/Jdi9mA

One note: C# restricts in and out to interfaces. I don't think there's a technical reason for that other than that it's simply not supported (or it has something to do with the runtime with is irrelevant for Psalm). Also, they do allow using T for parameters of type callable(): T while marking it as out. This seems to be an edge case though.

[muglug]

I'm hesitant about out because there's already the concept of out parameters, and Psalm has a @param-out annotation that roughly mirrors that functionality.

Covariant sounds better - maybe something like

@psalm-template TKey as Foo
@psalm-template-covariant TValue as Bar

[iluuu1994]

I see. Yeah that makes sense 👍

[iluuu1994]

Something I forgot: There's an RFC for generics in PHP in the works. It looks like they do plan on using the in and out keywords. Just something to keep in mind.

[muglug]

Oh yeah, if/when that RFC's accepted I'll happily update the syntax!

[iluuu1994]

This is dope 👍

Only thing left is to warn on line 13

[muglug]

Only thing left is to warn on line 13

What should the message be, roughly?

[iluuu1994]

Can't use a covariant template as an input

Something like that?

[iluuu1994]

This should also error.

/**
 * @template T
 */
class Invariant
{
}

/**
 * @template-covariant T
 * @extends Invariant<T>
 */
class Covariant extends Invariant
{
}

[muglug]

Input should be fine for a constructor, though?

https://psalm.dev/r/82914a1e16

[muglug]

/**
 * @template-covariant T
 * @extends Invariant<T>
 */

Why is that bad? Could you provide an example?

ArrayObject and others currently have non-covariant parameters, and it might be annoying to prevent people downstream from extending them with covariant params.

[iluuu1994]

Yeah, constructors should be fine. Only public non-static methods methods should be relevant I think. I'll have to think more about casting in private methods.

[iluuu1994]

Why is that bad? Could you provide an example?

Hope this is understandable:

https://psalm.dev/r/6d69fd7baf

<?php

class Animal {}
class Dog extends Animal {}
class Cat extends Animal {}

/**
 * @template T
 */
class InvariantBox
{
    /**
     * @var T|null
     */
    protected $animal;

    /**
     * @param T $value
     */
    public function set($value): void
    {
        $this->animal = $value;
    }
}

/**
 * @template-covariant T
 * @extends InvariantBox<T>
 */
class CovariantBox extends InvariantBox
{
}

/** @var CovariantBox<Dog> $dogBox */
$dogBox = new CovariantBox();
/** @var CovariantBox<Animal> $animalBox */
$animalBox = $dogBox;
$animalBox->set(new Cat());

Basically, we need to make sure that the inherited API also doesn't use T as an parameter type.

[muglug]

Ah that makes sense - mind creating tickets for both, so I don't lose track?

[iluuu1994]

Done

[muglug]

Thanks!