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

Template bound type throws invalid bound type error when referencing another type of the same templated class #3922

Closed
ste93cry opened this issue Oct 7, 2020 · 11 comments

Comments

@ste93cry
Copy link
Contributor

ste93cry commented Oct 7, 2020

Bug report

When a type of the template is bound to a generic type that references another template type, the following (false-positive) errors are thrown:

PHPDoc tag @template ... for class ... has invalid bound type ....
PHPDoc tag @template ... for interface ... has invalid bound type ....

I see at least two issues here: the first one is that the same error is thrown twice (for the same line) like if the analysis was done twice, not sure why (one time for the class and another for the interface, even though the line it's referring to is just the latter). The second and most severe issue is that PHPStan is not recognizing that the bound type is referencing another type of the same templated class.

Code snippet that reproduces the problem

https://phpstan.org/r/175d8c1a-3159-4d28-b761-7d1bb8d80bb2

Expected output

No error expected

@dakur
Copy link

dakur commented Feb 4, 2021

It looks like our query bus implementation actually. Just getting rid of baselined errors and this one remains. 🙂

@ste93cry
Copy link
Contributor Author

ste93cry commented Feb 4, 2021

That's exactly my use-case, I also tried implementing the feature into PHPStan but could not get it working

@MartinMystikJonas
Copy link
Contributor

I too stumbled upon this. My use case is passing Entity and EntityCollection types to generic ORM mapper.

@ondrejmirtes
Copy link
Member

Hi, I'm trying to wrap my head around what are the expectations here. In which case this alternative doesn't work for you? https://phpstan.org/r/7fbb37b7-54bd-4578-97ba-f90272a6719b

@ste93cry
Copy link
Contributor Author

In your example the TResult param of QueryHandlerInterface is not bound anymore to the TResult param of QueryInterface, therefore you lose the security that the value returned by the handler is of the type expected when using its query. For example, assuming that to the execution of the FooQuery query gets you back a FooQueryResult object, with my use-case you get an error if you try to use an handler that returns BarQueryResult while with yours you don't. Psalm supports this, but it looks like it's not easy to implement according to its internals.

@ondrejmirtes
Copy link
Member

@ste93cry A code sample is worth a 1000 words, so I'd like a phpstan.org example with expected output so that I'm sure we're on the same page :)

@ste93cry
Copy link
Contributor Author

ste93cry commented Feb 25, 2021

I don't know how I could express this better than the following example with a few comments added. Since the generics are a PHPStan's thing there is no way to assert that the returned value is of the expected type using plain PHP, but ideally the tool should warn me when something is wrong

https://phpstan.org/r/0aa9a099-3d52-4e6d-8875-b510bd8ef66d

Below you can find also the same example with Psalm which works as expected, maybe that's better than any broken example I can get with the PHPStan playground

https://psalm.dev/r/bd6495a1e2

Thank you for trying to understand the issue, I know you said you're busy and this is not a priority on your list but having it work or trying to come up with the solution together would be nice

@ondrejmirtes
Copy link
Member

Implemented: phpstan/phpstan-src@e671cc0

I still have some failing tests around this (phpstan/phpstan-src@f5cb727) that I'll resolve before the release is out.

Please test the hell out of this :)

@ste93cry
Copy link
Contributor Author

Wow, thank you very much for taking the time to implement this, I appreciate this so much!

@ondrejmirtes
Copy link
Member

You're welcome :) Feel free to check out the implementation to see what was 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 Apr 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants