Skip to content

Commit

Permalink
Fix #4637 by duplicating the type definition for `DriverManager::getC…
Browse files Browse the repository at this point in the history
…onnection($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
<?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
  • Loading branch information
Ocramius committed May 6, 2021
1 parent 35a4071 commit af2a31e
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 2 deletions.
25 changes: 24 additions & 1 deletion lib/Doctrine/DBAL/DriverManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,30 @@ private function __construct()
* @throws Exception
*
* @phpstan-param array<string,mixed> $params
* @psalm-param Params $params
* @psalm-param array{
* charset?: string,
* dbname?: string,
* default_dbname?: string,
* driver?: key-of<self::DRIVER_MAP>,
* driverClass?: class-string<Driver>,
* driverOptions?: array<mixed>,
* 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<OverrideParams>,
* sharding?: array<string,mixed>,
* slaves?: array<OverrideParams>,
* user?: string,
* wrapperClass?: class-string<T>,
* } $params
* @psalm-return ($params is array{wrapperClass:mixed} ? T : Connection)
* @template T of Connection
*/
Expand Down
2 changes: 1 addition & 1 deletion phpcs.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

<file>bin</file>
<file>lib</file>
<file>tests</file>
<file>tests/Doctrine/Tests</file>

<rule ref="Doctrine">
<exclude name="SlevomatCodingStandard.TypeHints.DeclareStrictTypes"/>
Expand Down
15 changes: 15 additions & 0 deletions psalm-strict.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?xml version="1.0"?>
<psalm
totallyTyped="true"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns="https://getpsalm.org/schema/config"
xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
>
<projectFiles>
<directory name="tests/Doctrine/StaticAnalysis" />
<ignoreFiles>
<directory name="lib" />
<directory name="vendor" />
</ignoreFiles>
</projectFiles>
</psalm>
1 change: 1 addition & 0 deletions psalm.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
>
<projectFiles>
<directory name="lib/Doctrine/DBAL" />
<directory name="tests/Doctrine/StaticAnalysis" />
<directory name="tests/Doctrine/Tests" />
<ignoreFiles>
<directory name="vendor" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php

declare(strict_types=1);

namespace Doctrine\StaticAnalysis\DBAL;

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\DriverManager;

final class MyConnection extends Connection
{
}

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

0 comments on commit af2a31e

Please sign in to comment.