From af2a31e6262a4b25e61530c7ccc891f75c88fc5c Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Thu, 6 May 2021 15:25:01 +0200 Subject: [PATCH] Fix #4637 by duplicating the type definition for `DriverManager::getConnection($args)` params 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 , * } */ final class DriverManager { /** * @param array $params * @param Configuration|null $config The configuration to use. * @param EventManager|null $eventManager The event manager to use. * * @phpstan-param array $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 , * } */ final class DriverManager { /** * @psalm-param array{ * charset?: string, * wrapperClass?: class-string, * } $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 --- lib/Doctrine/DBAL/DriverManager.php | 25 ++++++++++++++++++- phpcs.xml.dist | 2 +- psalm-strict.xml | 15 +++++++++++ psalm.xml.dist | 1 + ...ager-retrieves-correct-connection-type.php | 19 ++++++++++++++ 5 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 psalm-strict.xml create mode 100644 tests/Doctrine/StaticAnalysis/DBAL/driver-manager-retrieves-correct-connection-type.php diff --git a/lib/Doctrine/DBAL/DriverManager.php b/lib/Doctrine/DBAL/DriverManager.php index 3822bd420bf..2a4b6539250 100644 --- a/lib/Doctrine/DBAL/DriverManager.php +++ b/lib/Doctrine/DBAL/DriverManager.php @@ -160,7 +160,30 @@ private function __construct() * @throws Exception * * @phpstan-param array $params - * @psalm-param Params $params + * @psalm-param array{ + * charset?: string, + * dbname?: string, + * default_dbname?: string, + * driver?: key-of, + * driverClass?: class-string, + * driverOptions?: array, + * host?: string, + * keepSlave?: bool, + * keepReplica?: bool, + * master?: OverrideParams, + * memory?: bool, + * password?: string, + * path?: string, + * pdo?: \PDO, + * platform?: Platforms\AbstractPlatform, + * port?: int, + * primary?: OverrideParams, + * replica?: array, + * sharding?: array, + * slaves?: array, + * user?: string, + * wrapperClass?: class-string, + * } $params * @psalm-return ($params is array{wrapperClass:mixed} ? T : Connection) * @template T of Connection */ diff --git a/phpcs.xml.dist b/phpcs.xml.dist index 33fda9b921a..b7d41ac233a 100644 --- a/phpcs.xml.dist +++ b/phpcs.xml.dist @@ -14,7 +14,7 @@ bin lib - tests + tests/Doctrine/Tests diff --git a/psalm-strict.xml b/psalm-strict.xml new file mode 100644 index 00000000000..9c80c9e93de --- /dev/null +++ b/psalm-strict.xml @@ -0,0 +1,15 @@ + + + + + + + + + + diff --git a/psalm.xml.dist b/psalm.xml.dist index b30b4f1a3f3..1282a41f0e4 100644 --- a/psalm.xml.dist +++ b/psalm.xml.dist @@ -7,6 +7,7 @@ > + diff --git a/tests/Doctrine/StaticAnalysis/DBAL/driver-manager-retrieves-correct-connection-type.php b/tests/Doctrine/StaticAnalysis/DBAL/driver-manager-retrieves-correct-connection-type.php new file mode 100644 index 00000000000..566db2474dc --- /dev/null +++ b/tests/Doctrine/StaticAnalysis/DBAL/driver-manager-retrieves-correct-connection-type.php @@ -0,0 +1,19 @@ + MyConnection::class, + ]); +}