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

Phpdoc inheritance with generics #2909

Merged

Conversation

RobertMe
Copy link
Contributor

@RobertMe RobertMe commented Feb 9, 2024

This fixes the issue with inherited template types not being resolved in the context of @phpstan-assert* usage.
For @phpstan-self-out I added some checks to the existing test to validate the (template) type

Fixes phpstan/phpstan#10037

@phpstan-bot
Copy link
Collaborator

You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x.

@RobertMe
Copy link
Contributor Author

RobertMe commented Feb 9, 2024

I struggled to get something to work for @throws, so maybe you can help out with that @ondrejmirtes :) I'm unsure where to add the test (file or specific method) and following that how to properly implement it.

@RobertMe RobertMe changed the base branch from 1.11.x to 1.10.x February 9, 2024 21:31
Currently when an inherited method has an assert tag with a template
type this template type isn't resolved which means you must override /
copy the tag to the inherited method as well to make it work. This
properly resolves the template type so the asserts properly work
without copying.
@RobertMe RobertMe force-pushed the phpdoc-inheritance-with-generics branch from 606e689 to 8fa9eab Compare February 9, 2024 21:37
@ondrejmirtes
Copy link
Member

Looks like phpstan/phpstan#9123 is also fixed, can you please add a regression test? Thanks!

@RobertMe
Copy link
Contributor Author

Looks like phpstan/phpstan#9123 is also fixed, can you please add a regression test? Thanks!

I'm not sure I'm seeing what should be tested here which isn't the case already? The only difference in that example is the explicit /** @phpstan-assert-if-true MyEvent $event */ on the second implementation. But that already worked because it was the work-a-round you gave me as well :) And for the mentioned expected result, of both being Event, I think that's incorrect and should be MyEvent?

Bu obviously I could just copy/paste the linked example into a bug-... test file if you want me to. But personally I don't see how that ticket differs from the test I already wrote (but I might be missing something).

@ondrejmirtes
Copy link
Member

I'm not sure I'm seeing what should be tested here

Every fixed issue gets a regression test, regardless of whether the issue was exactly the same or not. These code snippets are very valuable and we should not throw them away.

Please use this snippet https://phpstan.org/r/a9d9027b-da35-44f2-a757-de78dab88386 and do assertType instead of dumpType. Thank you!

@RobertMe
Copy link
Contributor Author

Every fixed issue gets a regression test, regardless of whether the issue was exactly the same or not. These code snippets are very valuable and we should not throw them away.

Okay, just adding an explicit test case for every fixed ticket 👍🏻

Please use this snippet https://phpstan.org/r/a9d9027b-da35-44f2-a757-de78dab88386 and do assertType instead of dumpType. Thank you!

Done, and added both of your examples from phpstan/phpstan#10037 as well

@ondrejmirtes ondrejmirtes merged commit 813d15e into phpstan:1.10.x Feb 10, 2024
425 of 430 checks passed
@ondrejmirtes
Copy link
Member

Perfect, thank you!

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