Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DriverManager::getConnection with wrapperClass static analysis problem #4637

Closed
DavideBicego opened this issue May 4, 2021 · 3 comments · Fixed by #4638
Closed

DriverManager::getConnection with wrapperClass static analysis problem #4637

DavideBicego opened this issue May 4, 2021 · 3 comments · Fixed by #4638

Comments

@DavideBicego
Copy link

After updating to doctrine/orm 2.8.4 from 2.8.1 I have a static analysis issue with the function DriverManager::getConnection().

The specific Psalm error is:

ERROR: MixedInferredReturnType - ... - Could not verify return type 'KanbanBOX\Core\Infrastructure\DataAccess\DatabaseConnection\ServiceDatabaseConnection' for ... :-:closure (see https://psalm.dev/047)
ServiceDatabaseConnection::class => static function (): ServiceDatabaseConnection {

ServiceDatabaseConnection extends Connection so the function call is:

DriverManager::getConnection(
    [
        'driver' => ...
        'wrapperClass' => ServiceDatabaseConnection::class,
    ]
);

Any suggestion?

@Ocramius
Copy link
Member

Ocramius commented May 6, 2021

This is an issue for doctrine/dbal, not doctrine/orm.

The change is from doctrine/dbal:2.12.1 to doctrine/dbal:2.13.1: 2.12.1...2.13.1

Relevant diff:

     *
-     * @param array{wrapperClass?: class-string<T>} $params
-     * @param Configuration|null                    $config       The configuration to use.
-     * @param EventManager|null                     $eventManager The event manager to use.
+     * @param array<string,mixed> $params
+     * @param Configuration|null  $config       The configuration to use.
+     * @param EventManager|null   $eventManager The event manager to use.
     *
     * @throws Exception
     *
+     * @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,
        ?Configuration $config = null,
        ?EventManager $eventManager = null
    ): Connection {

@Ocramius Ocramius transferred this issue from doctrine/orm May 6, 2021
@Ocramius
Copy link
Member

Ocramius commented May 6, 2021

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!

I'll send a patch with a fix for this.

Ocramius added a commit to Ocramius/dbal that referenced this issue May 6, 2021
…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 Ocramius added this to the 2.13.2 milestone May 6, 2021
Ocramius added a commit to Ocramius/dbal that referenced this issue May 6, 2021
…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
morozov added a commit that referenced this issue May 9, 2021
…ce-for-wrapped-connection-class

Fix #4637 by duplicating the type definition for `DriverManager::getConnection($args)` params
@morozov morozov closed this as completed May 9, 2021
derrabus added a commit to derrabus/dbal that referenced this issue Jun 5, 2021
* 2.13.x:
  Bump doctrine/coding-standard
  [doctrineGH-4643] Fix SQLServerPlatform::quoteIdentifier()
  [doctrineGH-4645] Use error suppression instead of an error handler in MySQLi Connection
  Rewrote `DriverManagerTest` replica/shards test scenarios to verify whole data structure equality
  Add type-inference tests to github actions, so that we test also that types match up
  Add warnings in docblocks
  Rework the caching documentation
  Fix doctrine#4637 by duplicating the type definition for `DriverManager::getConnection($args)` params
derrabus added a commit to derrabus/dbal that referenced this issue Jun 5, 2021
* 2.13.x:
  Bump doctrine/coding-standard
  [doctrineGH-4643] Fix SQLServerPlatform::quoteIdentifier()
  [doctrineGH-4645] Use error suppression instead of an error handler in MySQLi Connection
  Rewrote `DriverManagerTest` replica/shards test scenarios to verify whole data structure equality
  Add type-inference tests to github actions, so that we test also that types match up
  Add warnings in docblocks
  Rework the caching documentation
  Fix doctrine#4637 by duplicating the type definition for `DriverManager::getConnection($args)` params
derrabus added a commit to derrabus/dbal that referenced this issue Jun 5, 2021
* 2.13.x:
  Bump doctrine/coding-standard
  [doctrineGH-4643] Fix SQLServerPlatform::quoteIdentifier()
  [doctrineGH-4645] Use error suppression instead of an error handler in MySQLi Connection
  Rewrote `DriverManagerTest` replica/shards test scenarios to verify whole data structure equality
  Add type-inference tests to github actions, so that we test also that types match up
  Add warnings in docblocks
  Rework the caching documentation
  Fix doctrine#4637 by duplicating the type definition for `DriverManager::getConnection($args)` params
derrabus added a commit to derrabus/dbal that referenced this issue Jun 5, 2021
* 2.13.x:
  Bump doctrine/coding-standard
  [doctrineGH-4643] Fix SQLServerPlatform::quoteIdentifier()
  [doctrineGH-4645] Use error suppression instead of an error handler in MySQLi Connection
  Rewrote `DriverManagerTest` replica/shards test scenarios to verify whole data structure equality
  Add type-inference tests to github actions, so that we test also that types match up
  Add warnings in docblocks
  Rework the caching documentation
  Fix doctrine#4637 by duplicating the type definition for `DriverManager::getConnection($args)` params
derrabus added a commit to derrabus/dbal that referenced this issue Jun 6, 2021
* 2.13.x:
  Bump doctrine/coding-standard
  [doctrineGH-4643] Fix SQLServerPlatform::quoteIdentifier()
  [doctrineGH-4645] Use error suppression instead of an error handler in MySQLi Connection
  Rewrote `DriverManagerTest` replica/shards test scenarios to verify whole data structure equality
  Add type-inference tests to github actions, so that we test also that types match up
  Add warnings in docblocks
  Rework the caching documentation
  Fix doctrine#4637 by duplicating the type definition for `DriverManager::getConnection($args)` params
derrabus added a commit to derrabus/dbal that referenced this issue Jun 7, 2021
* 2.13.x:
  Bump doctrine/coding-standard
  [doctrineGH-4643] Fix SQLServerPlatform::quoteIdentifier()
  [doctrineGH-4645] Use error suppression instead of an error handler in MySQLi Connection
  Rewrote `DriverManagerTest` replica/shards test scenarios to verify whole data structure equality
  Add type-inference tests to github actions, so that we test also that types match up
  Add warnings in docblocks
  Rework the caching documentation
  Fix doctrine#4637 by duplicating the type definition for `DriverManager::getConnection($args)` params

Signed-off-by: Alexander M. Turek <me@derrabus.de>
derrabus added a commit to derrabus/dbal that referenced this issue Jun 7, 2021
* 2.13.x:
  Bump doctrine/coding-standard
  [doctrineGH-4643] Fix SQLServerPlatform::quoteIdentifier()
  [doctrineGH-4645] Use error suppression instead of an error handler in MySQLi Connection
  Rewrote `DriverManagerTest` replica/shards test scenarios to verify whole data structure equality
  Add type-inference tests to github actions, so that we test also that types match up
  Add warnings in docblocks
  Rework the caching documentation
  Fix doctrine#4637 by duplicating the type definition for `DriverManager::getConnection($args)` params

Signed-off-by: Alexander M. Turek <me@derrabus.de>
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants