Issue when upcasting to generic param

[aidantwoods]

When assigning to a generic typed variable where the templated param is a parent type of the provided param, psalm should permit the provided param to be upcasted (i.e. become less specific) during assignment. Instead an error occurs because types do not exactly match.

---

Input:

<?php

/** @template T */
class Foo
{
	/** @param \Closure():T $closure */
    public function __construct($closure) {}
}

class Bar
{
  /** @var Foo<array> */
  private $FooArray;
  
  public function __construct()
  {
    $this->FooArray = new Foo(function(): array { return []; });
  }
}

Result:

Psalm output (using commit 0e8fa0e): 

ERROR: InvalidPropertyAssignmentValue - 17:23 - $this->FooArray with declared type 'Foo<array<array-key, mixed>>' cannot be assigned type 'Foo<array<empty, empty>>'

Expected:

No issues!

---

https://psalm.dev/r/597622e845

[aidantwoods]

I probably should have also mentioned this occurs also when psalm infers a struct-like array: https://psalm.dev/r/64caa24031, which I don't think b4f03ab considers?

[aidantwoods]

Also occurs when upcasting to parent types: https://psalm.dev/r/659bd4454f, and to interfaces: https://psalm.dev/r/9b4782b2fe.

[muglug]

I realise this is by design - change @template T to @template-covariant T to allow this behaviour - see #1603

[aidantwoods]

The upcast actually happens prior to being assigned (note the return type of the closure matches what is being assigned to) so variance of the generic param isn't (and probably shouldn't be) a concern here. The issue happens because psalm tracks the more specific type through the closure (even though the return type is explicitly the parent type).

[aidantwoods]

Another way to look at it, I shouldn't be forced to use @template-covariant when it isn't actually required?

[muglug]

I'm not an expert in this stuff, but this shows how the error given is functionally similar to the one expected in the above-referenced ticket: https://psalm.dev/r/a77e9eeaea

[aidantwoods]

No variance of generic param is required for the case I'm presenting here though :) this is because the "upcast" happens inside the closure body (i.e. we return a specific type but explicitly state a parent type that is less specific).

The type of the closure is \Closure():Base (and this is assigned to \Closure():Base), but because Psalm tracks the more specific type in the return statement and falsely sees the closure as being declared as \Closure():Derived it then worries about variance when it doesn't need to.

Here's an example of why this is annoying, here I want a function that takes a string and returns a ?T. I specialise to T = array and provide such a function:
https://psalm.dev/r/15610c0460

However, now my implementation changes and I need to enforce that a specific key is set to a value as part of validation (but the user of the function doesn't care about this) so I teach Psalm a bit more about the array, but keep the function signature the same... but now this errors because Psalm has too much information (even though I'm asking it to lose some via the return type being deliberately less specific):

https://psalm.dev/r/b8bb89bdce

[iluuu1994]

I agree with @aidantwoods, this is similar to other issues that have been reported before (#1815). The inferred type is too specific. On second thought, not really the same. Either way, the desired type Foo<Base> should be considered when inferring the type of a new expression.

Right now, the simplest way to fix this is using @param annotations. This is suboptimal because some type-safety is lost.

[aidantwoods]

This is suboptimal because some type-safety is lost.

Very much so, you can put nonsense in a @var :)

[iluuu1994]

@muglug To clarify: This issue is not really about covariance, it's about type inference.

In C# you'd instantiate Foo like so:

FooBase = new Foo<Base>(() => new Derived());

In PHP you don't have the option to specify the generic type in the expression which is why it would be great if the type would be inferred correctly. I don't know how this works in Psalm but a simple solution could be to pass the desired type to the type inferrer.

In all these statements:

$this->property = new Foo(fn() => new Derived());
func(new Foo(fn() => new Derived()));
return new Foo(fn() => new Derived());

Psalm would pass the desired type (type of the property, parameter type, return type of the function, respectively) to the type inferrer. Psalm would check if the types are compatible and if they are the desired type is chosen.

Good type inference is incredibly complex and I'm not experienced in it. Not sure if this is a reasonable approach.

[muglug]

What about an annotation that would allow you to specify a wider type to be checked by Psalm that would also be verified e.g.

Allowed:

/** @assign Foo<Bar> $this->foo */
$this->foo = new Foo(new BarChild());

Causes a Psalm issue:

/** @assign Foo<int> $this->foo */
$this->foo = new Foo(new BarChild());

[iluuu1994]

What about function arguments and return values?

func(new Foo(fn() => new Derived()));
return new Foo(fn() => new Derived());

Note that this should only be allowed with new expressions. For a variables this would be type unsafe.

[muglug]

In PHP you don't have the option to specify the generic type in the expression

I'm mainly interested in solutions that involve decorating existing code with docblocks (that might one day translate to syntax if generics support is added) rather than forcing people to write PHP code a certain way.

[iluuu1994]

rather than forcing people to write PHP code a certain way.

I'm not suggesting that. I think Psalm could infer the type without too much hassle. I'll see if I can get it working when I have some free time. 🙂

[muglug]

I think Psalm could infer the type without too much hassle.

I'm still unclear about the mechanism - we don't have enough information here to be certain of whether the issue is a bug or intended behaviour.

We don't know if the developer would have instead written new Foo<Base>(fn() => new Derived()) or whether they would have written new Foo<Derived>(fn() => new Derived()), and I think it's unsafe to assume.

[iluuu1994]

Lots of languages do that 🤷‍♂️

For example: Swift

class Base {}
class Derived: Base {}

class Foo<T> {
    init(closure: () -> T) {}
}

func returnFooBase() -> Foo<Base> {
    return Foo { Derived() }
}

print(returnFooBase())

[iluuu1994]

@muglug That's just Swift syntactical sugar.

Foo { Derived() }

is the same as

Foo({ Derived() })

Swift closure syntax takes some getting used to.

You can try it out here: http://online.swiftplayground.run/

[muglug]

Swift doesn't have a notion of covariant/contravariant generic params:

class Base {}
class Derived: Base {}

class Foo<T> {
    init(t: T) {}
}

func returnFooBase() -> Foo<Base> {
    return Foo ( t: Derived() ) // should not work
}

print(returnFooBase())

This ticket suggests that it should be supported: https://forums.swift.org/t/generic-types-covariance-contravariance/4680

[iluuu1994]

As mentioned, IMO this doesn't have anything to do with covariance/contravariance. This is about type inference.

[muglug]

Yeah you're right - in Hack, this is allowed:

class Base {}
class Derived extends Base {}

class Foo<T> {
    public function __construct (T $t) {}
}

function returnFooBase() : Foo<Base> {
    return new Foo(new Derived());
}

but this is not:

class Base {}
class Derived extends Base {}

class FooParent<T as Base> {}
class Foo<T as Derived> extends FooParent<T> {
    public function __construct (T $t) {}
}

function returnFooBase() : FooParent<Base> {
    return new Foo(new Derived());
}

[muglug]

This should pass: https://psalm.dev/r/f5bae4baf2

[aidantwoods]

Just to add the the discussion on type annotations v.s. inference, IMO this need not be a dichotomy—Swift is quite happy to have both, and IMO they have merit even when not strictly necessary from an inference perspective (to help us humans out when reading things :) ). Adding annotations now to help Psalm arrive at the intended type doesn't prevent anyone building better inference in later anyway :)

[muglug]

Implementation note: Swift doesn't allow this:

class Base {}
class Derived: Base {}

class Foo<T>
{
  init(t: T) {}
}

func returnFooBase() -> Foo<Base> {
    let f = Foo(t: Derived());
    return f;
}

but Hack allows this:

class Base {}
class Derived extends Base {}

class Foo<T as Base> {
    public function __construct (T $t) {}
}

function returnFooBase() : Foo<Base> {
    $f = new Foo(new Derived());
    return $f;
}

That is, Hack remembers that the type was derived, but Swift forgets that it was derived.

[iluuu1994]

but Swift forgets that it was derived.

I think that's sufficient and covers 95% of the cases. Otherwise you'd have to consider cases like this:

$collection = new Collection();
$collection->add(1);
$collection->add('Foo');
$collection->add(new Bar());

// $collection is inferred as Collection<1|'Foo'|Bar>

There are languages that do this but it's pretty complex and largely unnecessary IMHO.

[muglug]

There are languages that do this but it's pretty complex and largely unnecessary IMHO.

Where possible I hew close to Hack's implementation, so I'll likely attempt to replicate it. I think it involves marking a type as derived, and storing that type's upper and lower bound.

[iluuu1994]

Ok, just be mindful of cases like this:

class Base {}
class Derived extends Base {}

class Foo<T as Base> {
    public function __construct (T $t) {}
}

function returnFooBase(): Foo<Base> {
    $f = new Foo(new Derived());
    takesFooDerived($f);
    return $f; // Invalid return type
}

function takesFooDerived(Foo<Derived> $foo): void {}

<<__Entrypoint>>
function main(): void {
  var_dump(returnFooBase());
}

So the call to takesFooDerived fixes the type of $f to Foo<Derived>.

[iluuu1994]

https://psalm.dev/r/57663a1f67

As mentioned, this one should error. I'll see if I can create a realistic example that makes it more obvious.

[iluuu1994]

I'm bad at coming up with good examples but hopefully you get the gist.

https://psalm.dev/r/434d861ccf

<?php

class Base {}
class Derived extends Base {}
class Derived2 extends Base {}

/**
 * @template T
 */
class Box {
    /** @var T */
    public $value;
  
    /**
     * @param T $value
     */
    public function __construct($value) {
        $this->value = $value;
    }
  
    /**
     * @param T $value
     */
    public function set($value) {
        $this->value = $value;
    }
}

class Foo {
  /**
   * @var Box<Derived> $box
   */
  public $box;
  
  /**
   * @return Box<Base>
   */
  public function bar(): Box {
    $box = new Box(new Derived());
    $this->baz($box);
    return $box;
  }
  
  /**
   * @param Box<Derived> $box
   */
  public function baz($box) {
    $this->box = $box;
  }
}

$foo = new Foo();
$baseBox = $foo->bar();
$baseBox->set(new Derived2());

var_dump($foo->box); // Type says `Box<Derived>` but value is `Box<Derived2>`

[muglug]

Would you mind ticketing that? The first example is good enough (as Hack prohibits it and Psalm currently allows it).

[iluuu1994]

Done: #1927