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

RedundantCondition when asserting exception sub-class #4473

Closed
BenMorel opened this issue Nov 3, 2020 · 7 comments
Closed

RedundantCondition when asserting exception sub-class #4473

BenMorel opened this issue Nov 3, 2020 · 7 comments

Comments

@BenMorel
Copy link
Contributor

BenMorel commented Nov 3, 2020

While creating doctrine/dbal#4401, I noticed that Psalm complains in the following (simplified) case:

class DriverException {}

class ConnectionException extends DriverException {}

interface ExceptionConverter
{
    public function convert(): DriverException;
}

class Test
{
    private ExceptionConverter $converter;

    /**
     * @param class-string<DriverException> $expectedClass
     *
     * @dataProvider exceptionConversionProvider
     */
    public function testConvertsException(string $expectedClass): void
    {
        $dbalException = $this->converter->convert();

        self::assertInstanceOf($expectedClass, $dbalException);
    }

    public function exceptionConversionProvider(): array
    {
        return [
            [ConnectionException::class],
        ];
    }
}

ERROR: RedundantCondition - tests/Driver/API/ExceptionConverterTest.php:54:15 - Type Doctrine\DBAL\Exception\DriverException for $dbalException is always Doctrine\DBAL\Exception\DriverException (see https://psalm.dev/122)
self::assertInstanceOf($expectedClass, $dbalException);

As far as I can see, the condition is not redundant: while $dbalException is a DriverException, there is no guarantee that it passes the instanceof check for the subclass of DriverException provided.

Full reproducer:

git clone https://github.com/BenMorel/dbal.git
cd dbal
git checkout unused-var
composer install
vendor/bin/psalm
@BenMorel
Copy link
Contributor Author

BenMorel commented Nov 3, 2020

Cannot reproduce it on psalm.dev I'm afraid.

@muglug
Copy link
Collaborator

muglug commented Nov 3, 2020

Running the above doesn't reproduce the stated issue

@muglug muglug closed this as completed Nov 3, 2020
@muglug
Copy link
Collaborator

muglug commented Nov 3, 2020

Actually here's a simple reproducer: https://psalm.dev/r/6d85680f44

@muglug muglug reopened this Nov 3, 2020
@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/6d85680f44
<?php declare(strict_types = 1);

/**
 * @param mixed $value
 * @param class-string<T> $type
 * @template T
 * @psalm-assert T $value
 */
function assertInstanceOf($value, string $type): void {
    // some code
}

class E {}

/** @param class-string<E> $e_class */
function foo(string $e_class, E $some_e) : void {
    assertInstanceOf($some_e, $e_class);
}
Psalm output (using commit 5abde20):

ERROR: RedundantCondition - 17:5 - Type E for $some_e is always E

@muglug
Copy link
Collaborator

muglug commented Nov 3, 2020

Actually this is an issue with PHPUnit's assertion – it should be @psalm-assert =T: https://psalm.dev/r/c02c5ac41f

Please open a ticket in PHPUnit's repo!

@muglug muglug closed this as completed Nov 3, 2020
@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/c02c5ac41f
<?php declare(strict_types = 1);

/**
 * @param mixed $value
 * @param class-string<T> $type
 * @template T
 * @psalm-assert =T $value
 */
function assertInstanceOf($value, string $type): void {
    // some code
}

class E {}

/** @param class-string<E> $e_class */
function foo(string $e_class, E $some_e) : void {
    assertInstanceOf($some_e, $e_class);
}
Psalm output (using commit 5abde20):

No issues!

@BenMorel
Copy link
Contributor Author

BenMorel commented Nov 3, 2020

Thanks a lot, @muglug!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants