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

Support question: how to use conditional return types on a complicated DBAL case #3277

Closed
greg0ire opened this issue Apr 30, 2020 · 9 comments
Labels

Comments

@greg0ire
Copy link
Contributor

I want to make SA tools (namely PHPStan, and of course Psalm) understand this method: https://github.com/doctrine/dbal/blob/6c789b30a46906c5e669158809076d2405ea4fa9/lib/Doctrine/DBAL/DriverManager.php#L124

It reads like this:

    public static function getConnection(
        array $params,
        ?Configuration $config = null,
        ?EventManager $eventManager = null
    ) : Connection {
        // lots and lots of code

        $wrapperClass = Connection::class;
        if (isset($params['wrapperClass'])) {
            if (! is_subclass_of($params['wrapperClass'], $wrapperClass)) {
                throw DBALException::invalidWrapperClass($params['wrapperClass']);
            }

            $wrapperClass = $params['wrapperClass'];
        }

        return new $wrapperClass($params, $driver, $config, $eventManager);
    }

At first, I tried using templates, like this:

    /**
     * @param array{wrapperClass?: class-string<T>} $params       The parameters.
     * @param Configuration|null                    $config       The configuration to use.
     * @param EventManager|null                     $eventManager The event manager to use.
     *
     * @return T
     *
     * @throws DBALException
     *
     * @template T of Connection
     */

but PHPStan rightfully pointed out that the output type is not necessarily T (even if it's a T of Connection).

 ------ --------------------------------------------------------------------------------------------------------------------------------------- 
  Line   Doctrine/DBAL/DriverManager.php                                                                                                        
 ------ --------------------------------------------------------------------------------------------------------------------------------------- 
  190    Method Doctrine\DBAL\DriverManager::getConnection() should return T of Doctrine\DBAL\Connection but returns Doctrine\DBAL\Connection.  
 ------ --------------------------------------------------------------------------------------------------------------------------------------- 

So I made this change:

    /**
     * @param array{wrapperClass?: class-string<T>} $params       The parameters.
     * @param Configuration|null                    $config       The configuration to use.
     * @param EventManager|null                     $eventManager The event manager to use.
     *
-    * @return T
+    * @return T|Connection
     *
     * @throws DBALException
     *
     * @template T of Connection
     */

Phpstan was happy, but I think it defeats the whole purpose, because this is just as good as @return T isn't it. I got the same errors I had at the beginning from Psalm, about methods from a subtype of connection that I can't be sure will be there.

How do I make Psalm understand that T will be returned iff wrapperClass is not null, and that Connection will be returned otherwise?

@psalm-github-bot
Copy link

Hey @greg0ire, can you reproduce the issue on https://psalm.dev ?

@greg0ire
Copy link
Contributor Author

Here you go: https://psalm.dev/r/f1d18ed5c2

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/f1d18ed5c2
<?php

class Connection
{
}

class SpecificConnection extends Connection
{
    public function foo(): void
    {
    }
}

class DriverManager
{
    
    /**
     * @param array{wrapperClass?: class-string<T>} $params       The parameters.
     *
     * @return T|Connection
     *
     * @template T of Connection
     */
    public static function getConnection(
        array $params
    ) : Connection {
        // lots and lots of code

        $wrapperClass = Connection::class;
        if (isset($params['wrapperClass'])) {
            if (! is_subclass_of($params['wrapperClass'], $wrapperClass)) {
                throw new \Exception();
            }

            $wrapperClass = $params['wrapperClass'];
        }

        return new $wrapperClass();
    }
}

DriverManager::getConnection(['wrapperClass' => SpecificConnection::class])->foo();
Psalm output (using commit 8ab5a0f):

ERROR: UndefinedMethod - 42:78 - Method Connection::foo does not exist

@muglug muglug added the bug label Apr 30, 2020
@muglug
Copy link
Collaborator

muglug commented Apr 30, 2020

This should work: https://psalm.dev/r/98267e7f6b

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/98267e7f6b
<?php

class Connection {}

class SpecificConnection extends Connection {
    public function foo(): void {}
}

class DriverManager {
    /**
     * @template T of Connection
     * @template TArray of array{wrapperClass?: class-string<T>}
     * @param TArray $params
     *
     * @return (TArray is array{wrapperClass:mixed} ? T : Connection)
     */
    public static function getConnection(array $params) {
        // lots and lots of code

        $wrapperClass = Connection::class;
        if (isset($params['wrapperClass'])) {
            if (! is_subclass_of($params['wrapperClass'], $wrapperClass)) {
                throw new \Exception();
            }

            $wrapperClass = $params['wrapperClass'];
        }

        return new $wrapperClass();
    }
}

DriverManager::getConnection(['wrapperClass' => SpecificConnection::class])->foo();
Psalm output (using commit 8ab5a0f):

INFO: MixedMethodCall - 33:78 - Cannot determine the type of the object on the left hand side of this expression

ERROR: InvalidDocblock - 17:5 - Saw : outside of object-like array in docblock for DriverManager::getConnection

INFO: MissingReturnType - 17:28 - Method DriverManager::getConnection does not have a return type, expecting Connection

@greg0ire
Copy link
Contributor Author

greg0ire commented Apr 30, 2020

This should work: https://psalm.dev/r/98267e7f6b

It does not work for me, I have the same warnings as on psalm.dev:

INFO: MixedMethodCall - 33:78 - Cannot determine the type of the object on the left hand side of this expression

ERROR: InvalidDocblock - 17:5 - Saw : outside of object-like array in docblock for DriverManager::getConnection

INFO: MissingReturnType - 17:28 - Method DriverManager::getConnection does not have a return type, expecting Connection
 

EDIT: I realise that might have been what you wanted to say since you added the "bug" label 😅

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/98267e7f6b
<?php

class Connection {}

class SpecificConnection extends Connection {
    public function foo(): void {}
}

class DriverManager {
    /**
     * @template T of Connection
     * @template TArray of array{wrapperClass?: class-string<T>}
     * @param TArray $params
     *
     * @return (TArray is array{wrapperClass:mixed} ? T : Connection)
     */
    public static function getConnection(array $params) {
        // lots and lots of code

        $wrapperClass = Connection::class;
        if (isset($params['wrapperClass'])) {
            if (! is_subclass_of($params['wrapperClass'], $wrapperClass)) {
                throw new \Exception();
            }

            $wrapperClass = $params['wrapperClass'];
        }

        return new $wrapperClass();
    }
}

DriverManager::getConnection(['wrapperClass' => SpecificConnection::class])->foo();
Psalm output (using commit 8ab5a0f):

INFO: MixedMethodCall - 33:78 - Cannot determine the type of the object on the left hand side of this expression

ERROR: InvalidDocblock - 17:5 - Saw : outside of object-like array in docblock for DriverManager::getConnection

INFO: MissingReturnType - 17:28 - Method DriverManager::getConnection does not have a return type, expecting Connection

@muglug
Copy link
Collaborator

muglug commented Apr 30, 2020

I realise that might have been what you wanted to say since you added the bug label

Yup!

@muglug
Copy link
Collaborator

muglug commented May 1, 2020

This works now!

@muglug muglug closed this as completed May 1, 2020
@greg0ire greg0ire mentioned this issue May 1, 2020
16 tasks
greg0ire added a commit to greg0ire/dbal that referenced this issue May 1, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
greg0ire added a commit to greg0ire/dbal that referenced this issue May 3, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
greg0ire added a commit to greg0ire/dbal that referenced this issue May 3, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
greg0ire added a commit to greg0ire/dbal that referenced this issue May 4, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
greg0ire added a commit to greg0ire/dbal that referenced this issue May 8, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
greg0ire added a commit to greg0ire/dbal that referenced this issue May 11, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
greg0ire added a commit to greg0ire/dbal that referenced this issue May 18, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
greg0ire added a commit to greg0ire/dbal that referenced this issue May 24, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
greg0ire added a commit to greg0ire/dbal that referenced this issue May 26, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
greg0ire added a commit to greg0ire/dbal that referenced this issue May 27, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
greg0ire added a commit to greg0ire/dbal that referenced this issue May 27, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
greg0ire added a commit to greg0ire/dbal that referenced this issue May 27, 2020
This return type is conditional to $params['wrapperClass']. Luckily,
recent versions of Psalm allow documenting it properly.
See vimeo/psalm#3277

Note that phpstan is not able to understand this yet, but still attempts
to, hence the extra ignore rules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants