Make types compatible with ORM/ODM drivers

[greg0ire]

It does not look like the implementations for these methods every return ClassMetadata objects.

[greg0ire]

Here is the output of Psalm on ORM with this:

ERROR: PossiblyNullReference - src/Mapping/Driver/XmlDriver.php:137:94 - Cannot call method children on possibly null value (see https://psalm.dev/083)
                        $columnDef['options'] = $this->parseOptions($discrColumn['options']->children());


ERROR: PossiblyNullIterator - src/Mapping/Driver/XmlDriver.php:148:30 - Cannot iterate over nullable var SimpleXMLElement|null (see https://psalm.dev/097)
                    foreach ($xmlRoot->{'discriminator-map'}->{'discriminator-mapping'} as $discrMapElement) {


ERROR: UnusedBaselineEntry - src/Mapping/Driver/XmlDriver.php:0:0 - Baseline for issue "InvalidPropertyFetch" has 2 extra entries. (see https://psalm.dev/316)



ERROR: UnusedBaselineEntry - src/Mapping/Driver/XmlDriver.php:0:0 - Baseline for issue "InvalidReturnStatement" has 1 extra entry. (see https://psalm.dev/316)



ERROR: UnusedBaselineEntry - src/Mapping/Driver/XmlDriver.php:0:0 - Baseline for issue "InvalidReturnType" has 1 extra entry. (see https://psalm.dev/316)



ERROR: UnusedBaselineEntry - src/Mapping/Driver/XmlDriver.php:0:0 - Baseline for issue "NoInterfaceProperties" has 2 extra entries. (see https://psalm.dev/316)



ERROR: UnusedBaselineEntry - src/Mapping/Driver/XmlDriver.php:0:0 - Baseline for issue "TypeDoesNotContainType" has 3 extra entries. (see https://psalm.dev/316)

[stof]

Why not implementing the generic type instead of mixed as I suggested on Slack ?

[greg0ire]

@stof I plan on doing it, but on 3.4.x since it is no bugfix (which this is from an SA point of view; the types are plain wrong here).

[stof]

Well, as the types are plain wrong, I would consider that the fix might be to put directly the right type as a replacement (which btw might generate less new SA errors in the ORM than your patch)

[greg0ire]

Ok, let me give it a try.

[greg0ire]

@stof I gave it a try, and I need more changes on ORM to obtain the exact same output as above. On top of that, there are quite a few hurdles to overcome to get it to work on persistence itself, so I think this should be contributed on 3.4.x. Here is a preview of the issues:

ERROR: InvalidReturnType - tests/Persistence/Mapping/FileDriverTest.php:215:55 - The declared return type 'array<class-string, Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<object>>' for Doctrine\Tests\Persistence\Mapping\TestFileDriver::loadMappingFile is incorrect, got 'array{'Doctrine\\Tests\\Persistence\\Mapping\\Fixtures\\AnotherGlobalClass'::class?: Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<Doctrine\Tests\Persistence\Mapping\Fixtures\AnotherGlobalClass>, 'Doctrine\\Tests\\Persistence\\Mapping\\Fixtures\\GlobalClass'::class?: Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<Doctrine\Tests\Persistence\Mapping\Fixtures\GlobalClass>, stdClass::class?: Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<stdClass>}' (see https://psalm.dev/011)
    protected function loadMappingFile(string $file): array


ERROR: InvalidReturnStatement - tests/Persistence/Mapping/FileDriverTest.php:218:20 - The inferred type 'array{'Doctrine\\Tests\\Persistence\\Mapping\\Fixtures\\AnotherGlobalClass'::class: Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<Doctrine\Tests\Persistence\Mapping\Fixtures\AnotherGlobalClass>, 'Doctrine\\Tests\\Persistence\\Mapping\\Fixtures\\GlobalClass'::class: Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<Doctrine\Tests\Persistence\Mapping\Fixtures\GlobalClass>}' does not match the declared return type 'array<class-string, Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<object>>' for Doctrine\Tests\Persistence\Mapping\TestFileDriver::loadMappingFile (see https://psalm.dev/128)
            return [
                GlobalClass::class => new TestClassMetadata(GlobalClass::class),
                AnotherGlobalClass::class => new TestClassMetadata(AnotherGlobalClass::class),
            ];


ERROR: InvalidReturnStatement - tests/Persistence/Mapping/FileDriverTest.php:224:16 - The inferred type 'array{stdClass::class: Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<stdClass>}' does not match the declared return type 'array<class-string, Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<object>>' for Doctrine\Tests\Persistence\Mapping\TestFileDriver::loadMappingFile (see https://psalm.dev/128)
        return [stdClass::class => new TestClassMetadata(stdClass::class)];


------------------------------
3 errors found
------------------------------

Note: Using configuration file /home/greg/dev/doctrine-persistence/phpstan.neon.
 67/67 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   tests/Persistence/Mapping/FileDriverTest.php
 ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  218    Method Doctrine\Tests\Persistence\Mapping\TestFileDriver::loadMappingFile() should return array<class-string, Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<object>> but returns array{Doctrine\Tests\Persistence\Mapping\Fixtures\GlobalClass:
         Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<Doctrine\Tests\Persistence\Mapping\Fixtures\GlobalClass>, Doctrine\Tests\Persistence\Mapping\Fixtures\AnotherGlobalClass:
         Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<Doctrine\Tests\Persistence\Mapping\Fixtures\AnotherGlobalClass>}.
         💡 Template type T on class Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata is not covariant. Learn more: https://phpstan.org/blog/whats-up-with-template-covariant
         💡 Template type T on class Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata is not covariant. Learn more: https://phpstan.org/blog/whats-up-with-template-covariant

  224    Method Doctrine\Tests\Persistence\Mapping\TestFileDriver::loadMappingFile() should return array<class-string, Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<object>> but returns array{stdClass:
         Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata<stdClass>}.
         💡 Template type T on class Doctrine\Tests\Persistence\Mapping\Fixtures\TestClassMetadata is not covariant. Learn more: https://phpstan.org/blog/whats-up-with-template-covariant
 ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

[stof]

I think the fact that TestFileDriver actually returns ClassMetadata objects (for which SA tools infer new TestClassMetadata(GlobalClass::class) and new TestClassMetadata(AnotherGlobalClass::class) as different types) is the cause of all your issues for the analysis of the test class. Making it use a non-generic type as its template type will be simpler, while still allowing to test the getElement API.

[greg0ire]

@stof that and using @template-covariant on TestClassMetadata. Now it works, thanks!