UnsafeInstantiation when trying to dynamically construct a concrete class of an interface that guarantees the constructor

[jnvsor]

https://psalm.dev/r/d67562a544

A constructor who's signature is guaranteed by an interface or abstract parent method causes an incorrect UnsafeInstantiation when constructed dynamically

Just to make it more complicated, it is possible for this error to be correct when the calling code is filling in parameters that the class has added on to the interface definition through liskov substitution, and the child class adds different ones...

You can dynamically create a class if you verify that the class-string is an instance of the interface, but if you verify the class-string is a class that implements it psalm will complain (See dynamicWorks() and dynnamicBroke())

In the snippet psalm should allow dynamic creation by class string in dynnamicBroke() as well because the class is an implementation of an interface that guarantees the constructor signature, and the call also adheres to the interface

It should also allow dynamic creation by call to new static() in Y::instantiateWorks(), but not in Y::instantiateBroke because the call there goes beyond the interface definition, and would fail if it was called via class Z

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/d67562a544

<?php

interface X {
    public function __construct(int $a);
}

class Y implements X
{
    public function __construct(int $a, int $b = 1)
    {
    }
    
    public static function instantiateWorks(): static
    {
        return new static(1);
    }

    public static function instantiateBroke(): static
    {
        return new static(1, 2);
    }
}

class Z extends Y
{
    public function __construct(int $a, bool $f = false)
    {
    }
}

function dynamicWorks(string $var): ?X
{
    if (is_a($var, X::class, true)) {
        return new $var(1);
    }
    
    return null;
}

function dynnamicBroke(string $var): ?Y
{
    if (is_a($var, Y::class, true)) {
        return new $var(1);
    }
    
    return null;
}

Psalm output (using commit 028ac7f):

ERROR: UnsafeInstantiation - 43:16 - Cannot safely instantiate class Y with "new $class_name" as its constructor might change in child classes

ERROR: UnsafeInstantiation - 15:16 - Cannot safely instantiate class Y with "new static" as its constructor might change in child classes

ERROR: UnsafeInstantiation - 20:16 - Cannot safely instantiate class Y with "new static" as its constructor might change in child classes

[orklah]

Well, the constructor may also change when using docblocks that declare templates or things like that.

I'll keep this open because it probably could be improved but I'm not sure someone will pick this up

[jnvsor]

The main practical issue is that it's not currently possible to call new static() at all. No combination of interfaces or abstract classes will allow a new static() construction without @psalm-suppress UnsafeInstantiation

[orklah]

Did you read the doc on that issue? https://psalm.dev/docs/running_psalm/issues/UnsafeInstantiation/

Psalm provides a @psalm-consistent-constructor which will ensure that each child of your class is actually compatible with the class that uses this annotation. It disable the UnsafeInstantiation because Psalm is now sure there is no issue with your constructor

You can also declare your constructor final

[jnvsor]

I didn't see that. That actually solved the new static() problem (After I cleared the psalm cache since it was still showing up)

I tested that in the sandbox and it looks like it respects liskov substitution. Couldn't we just apply this by default to all interfaces and abstract classes with constructor definitions? Currently the annotation can't be applied to interfaces at all, but the logic behind it seems to fit perfectly

[orklah]

Well, as I said, it checks more than just the arguments, I'm not sure applying automatically this property to all __construct in interfaces would make this behaviour clear to everyone.

But maybe some cases could be skipped, when neither the parent class nor the child class has templates. This would need to be thought of a little and implemented (hence why I tagged this enhancement)