-
Notifications
You must be signed in to change notification settings - Fork 545
Implement RequireExtendsPropertiesClassReflectionExtension #2856
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
Conversation
src/Reflection/RequireExtension/RequireExtendsPropertiesClassReflectionExtension.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some regression tests that use these tags to make sure this actually solves the problems:
(just pushed a intermediate state to allow me switching to a different computer over the weekend)
7295e4a to
e5c4136
Compare
src/Reflection/RequireExtension/RequireExtendsOrImplementsMethodsClassReflectionExtension.php
Outdated
Show resolved
Hide resolved
src/Reflection/RequireExtension/RequireExtendsMethodsClassReflectionExtension.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this recursion guard tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried thinking of a way this could get recursive, but wasn't able to come up with a example.
do you think its impossible and the recursion-guard should be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect something like this to end up in infinite recursion https://phpstan.org/r/aa768c71-4596-4edf-a900-60e4db58582b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for the inspiration. I tried similar things before and I can't force them to endless recurse.
I think this might not happen, because the new extension early exits for non-interfaces and the phpstan-require-extends requires a class as argument.
since I was still not able to force a recursion, I have removed the recursion guards for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a point for the rules - @...-require-extends can only be above an interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so do you mean we should still try to infer the property across the type hierarchy, even though the require-extends is placed on a invalid location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't get it. This comment was just a reminder for you to check whether require-extends is always above an interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rules will be topic for a followup PR. thanks for noticing.
src/Reflection/RequireExtension/RequireExtendsPropertiesClassReflectionExtension.php
Outdated
Show resolved
Hide resolved
8d326eb to
0b4bf73
Compare
18a9fe7 to
b69346e
Compare
This pull request has been marked as ready for review.
0f88b00 to
029b523
Compare
I merged part of this pull request: 53a61dc
You should be able to rebase this PR cleanly (maybe after you squash all commits).
The reason why I did this is that you can start working on the rules PR in parallel to this PR.
I want to keep 1.10.x releasable and I don't want to merge this PR and then wait several releases for the rules PR to be finished, because I see them as an atomic feature.
You should be able to work on rules in a separate branch/PR without any conflicts.
d02503b to
fcfacb3
Compare
fcfacb3 to
1b92082
Compare
Thank you!
FYI this was a missing part: 9ac6fd1
About generics - I'd like to support things like:
/** * @template T * @phpstan-require-extends Model<T> */
-
Which would inform about what
Tis in case theModeltypehints it in the accessed property/called method. I think it might already work with the existing code. -
Also when doing
class Foo implements Bar extends Model, it should enforce that the same type in@implements Bar<X>is also used in@extends Model<X>. -
Also if
Modelis generic but the user does not declare whatTis in@phpstan-require-extends, PHPStan should complain. -
It should for example also complain when
Modelhas two@templatebut the number of type variables isn't 2.
For 3) and 4) there are already existing checks which we should just call.
Do you feel up to the task? If you start working on this and hit a roadblock don't worry, open a draft PR and we'll figure out why it doesn't work. Thanks!
FYI this was a missing part: 9ac6fd1
great catch, thank you.
@ondrejmirtes should we add the require-extends and require-implements types to $dependenciesReflections similar how its done for MixinTypes?
phpstan-src/src/Dependency/DependencyResolver.php
Lines 489 to 496 in 9c4508d
Yes, definitely! Ideally try to simulate in e2e-tests.yml a situation where the result cache gets stale. There arr several examples already.
No description provided.