False positive TypeDoesNotContainType introduced recently (4.15 maybe)

[VincentLanglet]

An example can be found with this PR: sonata-project/SonataAdminBundle#7643

Code like

$class = \stdClass::class;
static::assertSame([$class => ['sonata.user.admin.group1']], $pool->getAdminClasses());

is reported

TypeDoesNotContainType: Cannot resolve types for $pool->getadminclasses() - array<class-string, array<array-key, string>> does not contain array{stdClass: array{"sonata.user.admin.group1"}}

But to me array<class-string, array<array-key, string>> contains array{stdClass: array{"sonata.user.admin.group1"}}.

[psalm-github-bot]

Hey @VincentLanglet, can you reproduce the issue on https://psalm.dev ?

[orklah]

Psalm seem to reduce the class-string in the offset to a simple string, and then complains that the string is not a class-string. I'm not sure what could have triggered this in recent versions.

Are you able to create a reproducer on psalm.dev?

[VincentLanglet]

Psalm seem to reduce the class-string in the offset to a simple string, and then complains that the string is not a class-string. I'm not sure what could have triggered this in recent versions.

Like 2-3 days ago there was no error, so it's pretty recent.

Are you able to create a reproducer on psalm.dev?

There is no error in https://psalm.dev/r/e62634509f
I'm not sure if it's also related to the assertSame then.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/e62634509f

<?php

/**
 * @return array<class-string, array<array-key, string>>
 */
function getArray() {
    return [];
}

function test(): bool {
    $class = \stdClass::class;
    
    /** @psalm-trace $a */
    $a = [$class => ['sonata.user.admin.group1']];
    
    /** @psalm-trace $b */
    $b = getArray();
    
    return $a === $b;
}

Psalm output (using commit 761d5f3):

INFO: Trace - 14:5 - $a: array{stdClass: array{"sonata.user.admin.group1"}}

INFO: Trace - 17:5 - $b: array<class-string, array<array-key, string>>

[VincentLanglet]

@orklah Changing the code to

static::assertTrue([$class => ['sonata.user.admin.group1']] === $pool->getAdminClasses());

solve the issue. so I don't know what's the weird interaction with assertSame.

[orklah]

Can be reproduced by https://psalm.dev/r/0482bc5bc7

Seems to be caused by #7076

@klimick mind taking a look?

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/0482bc5bc7

<?php

class A
{
    function test(): void
    {
        $a = [stdClass::class => ''];

        /** @var array<class-string, mixed> $b */
        $b = [];

        $this->assertSame($a, $b);
    }

    /**
     * @template T
     * @param T      $expected
     * @psalm-assert =T $actual
     */
    public function assertSame(mixed $expected, mixed $actual): void
    {
        return;
    }
}

Psalm output (using commit 761d5f3):

ERROR: DocblockTypeContradiction - 12:16 - Cannot resolve types for $b - docblock-defined type array<class-string, mixed> does not contain array{stdClass: ""}

[klimick]

Before #7076 next code was valid:

class A
{
    public function test(): void
    {
        // array{stdClass: ""}
        $a = [stdClass::class => ''];
        // array{Traversable: ""}
        $b = [Traversable::class => ''];

        $this->assertSame($a, $b);
    }

    /**
     * @template T
     *
     * @param T $expected
     * @param mixed $actual
     * @psalm-assert =T $actual
     */
    public function assertSame($expected, $actual): void
    {
        return;
    }
}

But is it really valid?
Psalm considered it valid because he compared array with array before.

Now Psalm tries to compare array{stdClass: ''} with array<class-string, mixed>.
Where stdClass is just string.
Not class-string.

As I know TKeyedArray can't have type different from int | string for keys.
For this reason stdClass is string.

@orklah what should we do?

[orklah]

Oh, nice so this isn't an issue with the assertion system itself.

As I know TKeyedArray can't have type different from int | string for keys.

subtypes of those two works as well, for example, you can have "hello" as an offset, which is a TLiteralString, so TClassString(or TLiteralClassStrign) should be no exception

So it seems like Psalm resolves stdClass::class to 'stdClass' but it shouldn't, so this is what has to change

[klimick]

So it seems like Psalm resolves stdClass::class to 'stdClass' but it shouldn't, so this is what has to change

Psalm already preserves info about class-string in TKeyedArray (the $class_strings property), but does not use in any way.
We could add the output of this info to the getId method but Psalm don't understand types like array{stdClass::class: mixed} or array{class-string<stdClass>: mixed} at the parser level currently.

For me, extending the parser seems like a bit of a challenge)

[weirdan]

It doesn't have to touch the parser part, if we accept the ambiguity of $a = [stdClass::class => 1] vs $a = ['stdClass' => 1]. Both of those could be represented by array{stdClass: 1}, but they may differ internally.

Wouldn't be the first time we do this either - e.g. there are a quite a few flags on Union that do not affect textual type representation.

[klimick]

It doesn't have to touch the parser part, if we accept the ambiguity of $a = [stdClass::class => 1] vs $a = ['stdClass' => 1]. Both of those could be represented by array{stdClass: 1}, but they may differ internally.

Sorry. I don't understand how it can help.
$a = [stdClass::class => 1] and $a = ['stdClass' => 1] can be compared textually, but array<class-string, int> and array{stdClass: 0} not.

[orklah]

If the class-string is actually preserved in the TKeyedArray, then the fault must be in the comparison of the two types

[klimick]

If the class-string is actually preserved in the TKeyedArray, then the fault must be in the comparison of the two types

Before the actual type is compared, it will be serialized to a string with the getId:

psalm/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php

Line 789 in dee3fc4

$ored_type_assertions[] = $prefix . $replacement_atomic_type->getId();

Information about class-string will be lost later in the AssertionReconciler. During the type parsing:

psalm/src/Psalm/Internal/Type/AssertionReconciler.php

Line 341 in 39402c2

$new_type_union = Type::parseString($assertion);

Why problem not with TKeyedArray serialization/deserialization?)

[orklah]

Oh, yeah, you're right, I missed the fact that it was serialized here.

I'd go for changing the way we display class-strings in keyed array then. One of those two syntaxes: https://psalm.dev/r/f780ccaa84

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/f780ccaa84

<?php

class A
{
    function test(): void
    {
        /** @var array{stdClass::class: mixed} $a */
        $a = [];

        /** @var array{class-string<stdClass>: mixed} $a */
        $a = [];
        
        /** @var array<class-string, mixed> $b */
        $b = [];

        $this->assertSame($a, $b);
    }

    /**
     * @template T
     * @param T      $expected
     * @psalm-assert =T $actual
     */
    public function assertSame(mixed $expected, mixed $actual): void
    {
        return;
    }
}

Psalm output (using commit 761d5f3):

ERROR: InvalidDocblock - 8:9 - array{stdClass::class: mixed} is not a valid type (from /var/www/vhosts/psalm.dev/httpdocs/src/somefile.php:7)

ERROR: InvalidDocblock - 11:9 - array{class-string<stdClass>: mixed} is not a valid type (from /var/www/vhosts/psalm.dev/httpdocs/src/somefile.php:10)

INFO: UnusedVariable - 8:9 - $a is never referenced or the value is not used

[weirdan]

It would be interesting to explore what it would take to avoid type -> text -> type roundtrip though.

[orklah]

Yeah, I thought about that too, but going full serialize($union) is a bit much in my taste. Also, that's a hole in the syntax not to allow class-strings in a keyed array. Some users may want to do that (as a param type for example)

[weirdan]

Re proposed syntax options: both are used in Psalm type system, so they not necessarily alternatives: https://psalm.dev/r/6e690bd75f

[psalm-github-bot]

I found these snippets:

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

<?php

/** @param stdClass::class $_c */
function a(string $_c): void {}

/** @param class-string<stdClass> $_c */
function b(string $_c): void {}

class z extends stdClass {}

a(stdClass::class);
a(z::class);

// c.f.
b(stdClass::class);
b(z::class); // ok for class-string<stdClass>, but not for stdClass::class

Psalm output (using commit 761d5f3):

ERROR: InvalidArgument - 12:3 - Argument 1 of a expects stdClass::class, z::class provided

[weirdan]

So for keyed array, where key names have to be known, stdClass::class syntax is the only one that makes sense.

[orklah]

yeah, you're right, ::class syntax is actually a TDependentGetClass if I'm not mistaken. It's used to say it could actually be a class-string of a child of shown class I believe

EDIT: no, I'm actually wrong on that, I mistook this for something else

[weirdan]

::class syntax is actually a TDependentGetClass if I'm not mistaken

It's TLiteralClassString

[VincentLanglet]

I'm not sure to understand. Is there a "quick" solution or should #7076 be reverted ?

It's better to have false negative like #7109 (comment) than having false positive.

[orklah]

I tried to look at it yesterday, it seems the class-string doesn't make it to the TKeyedArray. I'll have to check what can be done about that.

Ideally, I'd like to avoid reverting because the change is legit. We'll try to get it fixed for the next release, in the meantime, feel free to go back to the previous version

[orklah]

Ok, the class-string do make it to the TKeyedArray, I was not expecting it to be stored like that. I'm able to change the output, I'm now trying to check the parser