-
Notifications
You must be signed in to change notification settings - Fork 545
Remove inefficient caching from PhpMethodReflection #3534
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
545e3d6 to
594336e
Compare
You've opened the pull request against the latest branch 2.0.x. PHPStan 2.0 is not going to be released for months. If your code is relevant on 1.12.x and you want it to be released sooner, please rebase your pull request and change its target to 1.12.x.
as of now this PR only implements the PhpMethodReflection part. we would need the same thing for PhpFunctionReflection. Will do the 2nd part after a first feedback round.
af3a0c8 to
0b51b89
Compare
This pull request has been marked as ready for review.
PhpFunctionReflection too please.
0475b39 to
0f0cc65
Compare
PhpFunctionReflection too please.
here we go
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.
Also, having SimpleParserTest unit test to check for the attributes existence would also be nice.
c91049b to
15fc96d
Compare
15fc96d to
4fc5d31
Compare
21e164b to
2496e8a
Compare
btw: On my mac with this PR we are ~1-2 seconds slower when running time make phpstan in comparison to before this PR - I will have another look why this is the case
2496e8a to
183ef1f
Compare
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.
We don't need this property. We should always read the last entry in classStack
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.
AnonymousClassNode is not contained in ClassLike
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.
Err, what? final class AnonymousClassNode extends Class_, class Class_ extends ClassLike.
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 isn't sufficiently unique. This was recently fixed elsewhere by #3328.
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 should do the class@anonymous thing that's done by the visitor too. I don't think we'll be able to replicate the anonymous class index here and we should do more something like a column instead.
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 did not find column information in the reflection api.
instead I used start/end-of line combination of the class definition 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.
because of this special case I had to re-introduce the inClassLike property to make sure the keys of a anonymous class don't contain a namespace. thats required so I am able to build a matching array-offset in PhpMethodReflection.
a775e37 to
287f74f
Compare
with the latest push we are now able to properly handle func_get_args based variadics in anonymous classes, which we did not got right before this PR:
This reverts commit 083aaf3b64b2a539ac11db1bb5ebbc0574af9967.
9d57607 to
676d440
Compare
This pull request has been marked as ready for review.
Hey, I still saw too many things I'd do differently so I rolled up my sleeves and did them: d4d5c1a
No tests are failing for me. Feel free to write more tests if you think your code covered a scenario that mine doesn't. Thanks!
thank you. thats way simpler - awesome.
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 wonder why this $node->params logic is no longer reflected in the patch. it seems it was dead code before
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.
It's really old code, maybe there just wasn't a test for it. Since we're in 2.0.x, we can risk it, and maybe introduce it back if it breaks someone's scenario.
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 think it was unnecessary logic I implemented with 0ee67d5#diff-3ac4e5d4a520618a490963b70e3fcd428d56aea61683641020393167fff2ca5d
deleting the lines on 2.0.x before this PR, also doesn't break tests :).
all good.
dddf984
into
phpstan:2.0.x
It's a bit slower but maybe we'll find opportunities elsewhere :) Thank you.
thank you!
Oh yeah, I did it: 19cd999
Previously VariadicMethodsVisitor::$cache was not really useful, it only stored true values so the files were parsed over and over again.
In my measurements it's now fast as before without writing anything to disk.
as described in phpstan/phpstan#11748 (comment)