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

Change GenericObjectType generics via TypeSpecifyingExtension #6620

Closed
staabm opened this issue Feb 9, 2022 · 3 comments · Fixed by phpstan/phpstan-src#1011 or staabm/phpstan-dba#258

Comments

@staabm
Copy link
Contributor

staabm commented Feb 9, 2022

Feature request

In my use case, I have partly successfull already implemented in staabm/phpstan-dba#258, I have the following scenario.

A PDOStatement class is specifed via a return type extension of PdoStatement->query() like

        $query = 'SELECT email, adaid FROM ada';
        $stmt = $pdo->query($query, PDO::FETCH_NUM);
        assertType('PDOStatement<array{string, int<0, 4294967295>}>', $stmt);

to implement support for PDO native PDOStatement->setFetchType() I am using a MethodTypeSpecifyingExtension to support

        $query = 'SELECT email, adaid FROM ada';
        $stmt = $pdo->query($query, PDO::FETCH_NUM);
        assertType('PDOStatement<array{string, int<0, 4294967295>}>', $stmt);

        $stmt->setFetchMode(PDO::FETCH_ASSOC);
        assertType('PDOStatement<array{email: string, adaid: int<0, 4294967295>}>', $stmt);

.. so the idea is to turn a PDOStatement<array{string, int<0, 4294967295>}> into a PDOStatement<array{email: string, adaid: int<0, 4294967295>}> - and suprisingly this worked - from a phpunit point of view regarding assertType.

it has a negative side-effect though, that the ImpossibleCheckTypeHelper realizes a incompatibility between the generic-type before/after the specifying extension does what it is supposed to do.

https://github.com/phpstan/phpstan-src/blob/c5a9c2c50b9b639715ca85541fcfd7f877f5df06/src/Rules/Comparison/ImpossibleCheckTypeHelper.php#L195-L200

in the above scenario $isSuperType->no() is true and therefore we run into the return false case.
this return false triggers the following case

https://github.com/phpstan/phpstan-src/blob/c5a9c2c50b9b639715ca85541fcfd7f877f5df06/src/Rules/Comparison/ImpossibleCheckTypeMethodCallRule.php#L57-L66

which makes phpstan error like

Call to method PDOStatement<array<int|string, int<0, 4294967295>|string>,array<int|string,
         int<0, 4294967295>|string>>::setFetchMode() with 3 will always evaluate to false.

I create this issue as a "feature request" as I can see, that the behaviour as is makes sense.
Still I hope in scenarios where a MethodTypeSpecifyingExtension uses $overwrite=true like I do with
return $this->typeSpecifier->create($methodCall->var, $reducedType, TypeSpecifierContext::createTrue(), true);
the ImpossibleCheckTypeHelper could be made a bit tolerant regarding the types involved.

as it seems, in the end phpstan and the type-system work as expected with my MethodTypeSpecifyingExtension but the "verification" after type changes is not happy with me changing the generic type involved.

I wanted to discuss my scenario before submitting failing test cases, to see whether we agree, that this is something usefull to support.

@staabm staabm changed the title Change GenericObjectType generics via TypeSpecifyingExtension Change GenericObjectType generics via TypeSpecifyingExtension Feb 9, 2022
@staabm
Copy link
Contributor Author

staabm commented Feb 9, 2022

After a 2 hour bike ride and a lot of fresh air I had an idea:

What about taking

https://github.com/phpstan/phpstan-src/blob/c5a9c2c50b9b639715ca85541fcfd7f877f5df06/conf/config.neon#L25

Into account in the possible type checker and be more forgiving for classes listed there?

——

Alternatively we could create a new feature toggle for this specific problem..

——

another idea: having something like a „BenevolentGenericObject“ which is not as strict regarding is superTypeOf

@ondrejmirtes
Copy link
Member

Please contribute a change that makes ImpossibleCheckTypeHelper not report anything for $overwrite=true cases. No additional configuration option is needed.

@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 Mar 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants