Fix #4637 by duplicating the type definition for `DriverManager::getConnection($args)` params #4638

Ocramius · 2021-05-06T13:26:48Z

Q	A
Type	bug
BC Break	no
Fixed issues	#4637

Fixes #4637

As per #4637, vimeo/psalm:4.6.4 cannot introduce templated parameters within a re-used imported
type: this is understandable, since imported types are copy-paste aided by the type checker, but do
not carry any template information with them (and it would be too complex to do so anyway).

Quoting the initial issue:

Minimal reproducer: https://psalm.dev/r/62a0a5854d

<?php

class Connection {}
class MyConnection extends Connection{}

/**
 * @psalm-type Params = array{
 *     charset?: string,
 *     wrapperClass?: class-string<Connection>,
 * }
 */
final class DriverManager
{
    /**
     * @param array<string,mixed> $params
     * @param Configuration|null  $config       The configuration to use.
     * @param EventManager|null   $eventManager The event manager to use.
     *
     * @phpstan-param array<string,mixed> $params
     * @psalm-param Params $params
     * @psalm-return ($params is array{wrapperClass:mixed} ? T : Connection)
     * @template T of Connection
     */
    public static function getConnection(
        array $params
    ): Connection {
        throw new \Exception('irrelevant');
    }
}

function mkConnection(): MyConnection {
    return DriverManager::getConnection(['wrapperClass' => MyConnection::class]);
}

Which produces:

Psalm output (using commit aa854ae):

INFO: MixedInferredReturnType - 31:26 - Could not verify return type 'MyConnection' for mkConnection

The issue is that the Param alias type is not templated: this is potentially a psalm limitation, but also an understandable one, since there is no syntax to specify template arguments for an imported type (AFAIK).

Here is a potential fix:

https://psalm.dev/r/8ef9909bb9

<?php

class Connection {}
class MyConnection extends Connection{}

/**
 * @psalm-type Params = array{
 *     charset?: string,
 *     wrapperClass?: class-string<Connection>,
 * }
 */
final class DriverManager
{
    /**
     * @psalm-param array{
     *     charset?: string,
     *     wrapperClass?: class-string<T>,
     * } $params
     * @psalm-return ($params is array{wrapperClass: mixed} ? T : Connection)
     * @template T of Connection
     */
    public static function getConnection(
        array $params
    ): Connection {
        throw new \Exception('irrelevant');
    }
}

function mkConnection(): MyConnection {
    return DriverManager::getConnection(['wrapperClass' => MyConnection::class]);
}

It's a bit ugly, but the idea is that we replace Params with the concrete definition again, but this time including T, so that the type inference can correctly fill the type variable with the given input:

Psalm output (using commit aa854ae):

No issues!

The idea therefore is to re-introduce some duplication at the advantage of better type inference later on.

In order to verify that the change had an effect, we introduced a new tests/Doctrine/StaticAnalysis directory,
to be checked via vendor/bin/psalm -c psalm-strict.xml, which will tell us exactly whether there are obvious
type-checker issues around our code. Since psalm.xml.dist uses errorLevel="2", it is not sufficient to integrate
this directory within the normal workflow.

Because this project uses a custom github action for running vimeo/psalm, it is not really possible to run this
in CI, and therefore that part of the patch has been omitted.

It is endorsed that maintainers of doctrine/dbal in future:

commit composer.lock
set up dependabot to track dependency upgrades
use the standard vimeo/psalm via require-dev, and run the CI static analysis checks from the same
environment that runs normal unit/integration tests

lib/Doctrine/DBAL/DriverManager.php

@param

…er::getConnection($args)` params As per doctrine#4637, `vimeo/psalm:4.6.4` cannot introduce templated parameters within a re-used imported type: this is understandable, since imported types are copy-paste aided by the type checker, but do not carry any template information with them (and it would be too complex to do so anyway). Quoting the initial issue: Minimal reproducer: https://psalm.dev/r/62a0a5854d ```php <?php class Connection {} class MyConnection extends Connection{} /** * @psalm-type Params = array{ * charset?: string, * wrapperClass?: class-string<Connection>, * } */ final class DriverManager { /** * @param array<string,mixed> $params * @param Configuration|null $config The configuration to use. * @param EventManager|null $eventManager The event manager to use. * * @phpstan-param array<string,mixed> $params * @psalm-param Params $params * @psalm-return ($params is array{wrapperClass:mixed} ? T : Connection) * @template T of Connection */ public static function getConnection( array $params ): Connection { throw new \Exception('irrelevant'); } } function mkConnection(): MyConnection { return DriverManager::getConnection(['wrapperClass' => MyConnection::class]); } ``` Which produces: ``` Psalm output (using commit aa854ae): INFO: MixedInferredReturnType - 31:26 - Could not verify return type 'MyConnection' for mkConnection ``` The issue is that the `Param` alias type is not templated: this is potentially a psalm limitation, but also an understandable one, since there is no syntax to specify template arguments for an imported type (AFAIK). Here is a potential fix: https://psalm.dev/r/8ef9909bb9 ```php <?php class Connection {} class MyConnection extends Connection{} /** * @psalm-type Params = array{ * charset?: string, * wrapperClass?: class-string<Connection>, * } */ final class DriverManager { /** * @psalm-param array{ * charset?: string, * wrapperClass?: class-string<T>, * } $params * @psalm-return ($params is array{wrapperClass: mixed} ? T : Connection) * @template T of Connection */ public static function getConnection( array $params ): Connection { throw new \Exception('irrelevant'); } } function mkConnection(): MyConnection { return DriverManager::getConnection(['wrapperClass' => MyConnection::class]); } ``` It's a bit ugly, but the idea is that we replace `Params` with the concrete definition again, but this time including `T`, so that the type inference can correctly fill the type variable with the given input: ``` Psalm output (using commit aa854ae): No issues! ``` The idea therefore is to re-introduce some duplication at the advantage of better type inference later on. In order to verify that the change had an effect, we introduced a new `tests/Doctrine/StaticAnalysis` directory, to be checked via `vendor/bin/psalm -c psalm-strict.xml`, which will tell us exactly whether there are obvious type-checker issues around our code. Since `psalm.xml.dist` uses `errorLevel="2"`, it is not sufficient to integrate this directory within the normal workflow. Because this project uses a custom github action for running `vimeo/psalm`, it is not really possible to run this in CI, and therefore that part of the patch has been omitted. It is endorsed that maintainers of `doctrine/dbal` in future: 1. commit `composer.lock` 2. set up dependabot to track dependency upgrades 3. use the standard `vimeo/psalm` via `require-dev`, and run the CI static analysis checks from the same environment that runs normal unit/integration tests

Ocramius · 2021-05-06T13:33:30Z

These issues seem to be pre-existing: unsure whether/how they should be adjusted. Should I document these keys?

ERROR: PossiblyUndefinedArrayOffset - tests/Doctrine/Tests/DBAL/DriverManagerTest.php:165:43 - Possibly undefined array key $params['primary'] on array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, keepReplica?: bool, keepSlave?: bool, master?: array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string}, memory?: bool, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, primary?: array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string}, replica?: array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string}>, sharding?: array<string, mixed>, slaves?: array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string}>, user?: string, wrapperClass?: class-string<Doctrine\DBAL\Connection>} (see https://psalm.dev/167)
            self::assertArrayHasKey($key, $params['primary']);


ERROR: PossiblyUndefinedArrayOffset - tests/Doctrine/Tests/DBAL/DriverManagerTest.php:166:40 - Possibly undefined array key $params['primary'][$key] on array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string} (see https://psalm.dev/167)
            self::assertEquals($value, $params['primary'][$key]);


ERROR: PossiblyUndefinedArrayOffset - tests/Doctrine/Tests/DBAL/DriverManagerTest.php:168:43 - Possibly undefined array key $params['replica'] on array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, keepReplica?: bool, keepSlave?: bool, master?: array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string}, memory?: bool, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, primary?: array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string}, replica?: array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string}>, sharding?: array<string, mixed>, slaves?: array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string}>, user?: string, wrapperClass?: class-string<Doctrine\DBAL\Connection>} (see https://psalm.dev/167)
            self::assertArrayHasKey($key, $params['replica']['replica1']);


ERROR: PossiblyUndefinedArrayOffset - tests/Doctrine/Tests/DBAL/DriverManagerTest.php:169:40 - Possibly undefined array key $params['replica']['replica1'][$key] on array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string} (see https://psalm.dev/167)
            self::assertEquals($value, $params['replica']['replica1'][$key]);


ERROR: PossiblyUndefinedArrayOffset - tests/Doctrine/Tests/DBAL/DriverManagerTest.php:172:35 - Possibly undefined array key $params['primary']['dbname'] on array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string} (see https://psalm.dev/167)
        self::assertEquals('baz', $params['primary']['dbname']);


ERROR: PossiblyUndefinedArrayOffset - tests/Doctrine/Tests/DBAL/DriverManagerTest.php:173:43 - Possibly undefined array key $params['replica']['replica1']['dbname'] on array{charset?: string, dbname?: string, default_dbname?: string, driver?: "drizzle_pdo_mysql"|"ibm_db2"|"mysqli"|"oci8"|"pdo_mysql"|"pdo_oci"|"pdo_pgsql"|"pdo_sqlite"|"pdo_sqlsrv"|"sqlanywhere"|"sqlsrv", driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, user?: string} (see https://psalm.dev/167)
        self::assertEquals('baz_replica', $params['replica']['replica1']['dbname']);


ERROR: InvalidArrayOffset - tests/Doctrine/Tests/DBAL/DriverManagerTest.php:204:40 - Cannot access value on variable $params using offset value of global, expecting 'charset', 'dbname', 'default_dbname', 'driver', 'driverClass', 'driverOptions', 'host', 'keepSlave', 'keepReplica', 'master', 'memory', 'password', 'path', 'pdo', 'platform', 'port', 'primary', 'replica', 'sharding', 'slaves', 'user' or 'wrapperClass' (see https://psalm.dev/115)
            self::assertEquals($value, $params['global'][$key]);


ERROR: InvalidArrayOffset - tests/Doctrine/Tests/DBAL/DriverManagerTest.php:205:40 - Cannot access value on variable $params using offset value of shards, expecting 'charset', 'dbname', 'default_dbname', 'driver', 'driverClass', 'driverOptions', 'host', 'keepSlave', 'keepReplica', 'master', 'memory', 'password', 'path', 'pdo', 'platform', 'port', 'primary', 'replica', 'sharding', 'slaves', 'user' or 'wrapperClass' (see https://psalm.dev/115)
            self::assertEquals($value, $params['shards'][0][$key]);

greg0ire · 2021-05-06T22:05:54Z

Should I document these keys?

The issue does not seem to be a lack of documentation

ERROR: PossiblyUndefinedArrayOffset - tests/Doctrine/Tests/DBAL/DriverManagerTest.php:165:43 - Possibly undefined array key $params['primary'] …

For instance here, although it's documented that primary is a key that might exist, it might also not exist, and that depends on what $options were passed to DriverManager::getConnection. To make these go away, we would have to write some sort of extension I suppose. I don't think it would be simple in any case. What you could do here is add extra assertions, probably with assert since the existence of this key is not really what we are testing here.

It is endorsed that maintainers of doctrine/dbal in future:
commit composer.lock
set up dependabot to track dependency upgrades
use the standard vimeo/psalm via require-dev, and run the CI static analysis checks from the same
environment that runs normal unit/integration tests

We know, and you know we know :P

Personally I'm not certain that would be best for us given the high level of notifications we get from dependabot for a repository alone (doctrine/coding-standard, although it might be misconfigured?), also, I have spear-headed the removal of composer.lock and applied it on all repositories, so even if you convince me, I'm not going to be the one rolling that back, I don't see myself finding the motivation to do it.

Because this project uses a custom github action for running vimeo/psalm, it is not really possible to run this
in CI, and therefore that part of the patch has been omitted.

Maybe duplicate the job, and add a step that overwrites the normal psalm config with the one introduced in this PR? Ugly, but I think it's better if all the tests are run.

Anyway, thanks for this PR, it looks really great, I wish they were all this clear!

psalm-strict.xml

greg0ire · 2021-05-06T22:18:29Z

These issues seem to be pre-existing

It does not look that way, see this other very recent PR: #4640

I don't have an explanation though, sorry.

Ocramius · 2021-05-07T16:08:36Z

Maybe duplicate the job, and add a step that overwrites the normal psalm config with the one introduced in this PR? Ugly, but I think it's better if all the tests are run.

Feasible, will try.

Personally I'm not certain that would be best for us given the high level of notifications we get from dependabot for a repository

Meh, notifications can be silenced, but that's none of my biz :P

Meanwhile, I'll re-adjust the failing test so that assertions no longer fail type-checking there.

… types match up The normal psalm checks run in CI are not sufficient, since they run with level 2 checks (too lax), and therefore we need a dedicated action to be run when verifying the contents of `tests/Doctrine/StaticAnalysis`, which is designed to verify type inference and type-checking details provided by the library.

…hole data structure equality While it is true that such tests are accessing unknown/undocumented array keys that are not part of the psalm type definitions, we still need to verify equality on the retrieved data structure, in order to avoid accidentally introducing a BC break while messing with them. This change therefore: * makes the type-checker a bit happier (was failing before, due to undocumented keys) * makes the tests a bit more strict around potential undocumented keys in these arrays

Ocramius · 2021-05-07T16:57:58Z

All green, ready to go! /cc @greg0ire

morozov · 2021-05-09T18:21:15Z

Thanks, @Ocramius!

Ocramius added Bug Strict Typing Static Analysis labels May 6, 2021

Ocramius added this to the 2.13.2 milestone May 6, 2021

Ocramius linked an issue May 6, 2021 that may be closed by this pull request

DriverManager::getConnection with wrapperClass static analysis problem #4637

Closed

andrew-demb reviewed May 6, 2021

View reviewed changes

lib/Doctrine/DBAL/DriverManager.php Outdated Show resolved Hide resolved

Ocramius force-pushed the fix/#4637-restore-type-inference-for-wrapped-connection-class branch from 0a0b035 to af2a31e Compare May 6, 2021 13:32

greg0ire reviewed May 6, 2021

View reviewed changes

psalm-strict.xml Show resolved Hide resolved

Ocramius added 2 commits May 7, 2021 18:16

greg0ire approved these changes May 8, 2021

View reviewed changes

morozov approved these changes May 9, 2021

View reviewed changes

morozov merged commit b728198 into doctrine:2.13.x May 9, 2021

Ocramius deleted the fix/#4637-restore-type-inference-for-wrapped-connection-class branch May 13, 2021 18:48

This was referenced May 15, 2021

Check for use of deprecated API with Psalm doctrine/orm#8691

Merged

Analyze test directory doctrine/orm#8633

Closed

greg0ire mentioned this pull request May 24, 2021

The class ClassMetada is not correctly inferring the constructor of ClassMetadaInfo doctrine/orm#8709

Closed

derrabus mentioned this pull request Jun 5, 2021

Merge 2.13.x into 3.1.x #4666

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4637 by duplicating the type definition for `DriverManager::getConnection($args)` params #4638

Fix #4637 by duplicating the type definition for `DriverManager::getConnection($args)` params #4638

Ocramius commented May 6, 2021 •

edited

Ocramius commented May 6, 2021

greg0ire commented May 6, 2021 •

edited

greg0ire commented May 6, 2021

Ocramius commented May 7, 2021

Ocramius commented May 7, 2021

morozov commented May 9, 2021

Fix #4637 by duplicating the type definition for DriverManager::getConnection($args) params #4638

Fix #4637 by duplicating the type definition for DriverManager::getConnection($args) params #4638

Conversation

Ocramius commented May 6, 2021 • edited

Ocramius commented May 6, 2021

greg0ire commented May 6, 2021 • edited

greg0ire commented May 6, 2021

Ocramius commented May 7, 2021

Ocramius commented May 7, 2021

morozov commented May 9, 2021

Fix #4637 by duplicating the type definition for `DriverManager::getConnection($args)` params #4638

Fix #4637 by duplicating the type definition for `DriverManager::getConnection($args)` params #4638

Ocramius commented May 6, 2021 •

edited

greg0ire commented May 6, 2021 •

edited