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
Implement RequireExtendsPropertiesClassReflectionExtension #2856
Conversation
src/Reflection/RequireExtension/RequireExtendsPropertiesClassReflectionExtension.php
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
Show resolved
Hide resolved
$this->inProcess[$typeDescription][$methodName] = true; | ||
|
||
if (!$type->hasMethod($methodName)->yes()) { | ||
unset($this->inProcess[$typeDescription][$methodName]); |
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
private function findMethod(ClassReflection $classReflection, string $methodName): ?MethodReflection | ||
{ | ||
if (!$classReflection->isInterface()) { | ||
return null; |
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>
*/
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! |
great catch, thank you. |
@ondrejmirtes should we add the require-extends and require-implements types to 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.