-
Notifications
You must be signed in to change notification settings - Fork 504
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
Remove inefficient caching from PhpMethodReflection #3534
Conversation
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 |
af3a0c8
to
0b51b89
Compare
This pull request has been marked as ready for review. |
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
21e164b
to
2496e8a
Compare
btw: On my mac with this PR we are ~1-2 seconds slower when running |
} | ||
|
||
$this->classStack[] = $className; | ||
$this->inClassLike = $this->inNamespace !== null ? $this->inNamespace . '\\' . implode('\\', $this->classStack) : implode('\\', $this->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.
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
.
|| $node instanceof Node\Stmt\ClassLike | ||
) { | ||
if (!$node->name instanceof Node\Identifier) { | ||
$className = 'class@anonymous:' . $node->getStartLine(); |
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.
return $this->functionCallStatementFinder->findFunctionCallInStatements(ParametersAcceptor::VARIADIC_FUNCTIONS, $node->getStmts()) !== null; | ||
if ( | ||
is_array($variadicMethods) | ||
&& array_key_exists($declaringClass->getName(), $variadicMethods) |
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.
$this->classStack[] = $className; | ||
$this->inClassLike = $className; // anonymous classes are in global namespace |
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
.
with the latest push we are now able to properly handle |
This reverts commit 083aaf3b64b2a539ac11db1bb5ebbc0574af9967.
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. |
@@ -296,19 +252,4 @@ public function acceptsNamedArguments(): TrinaryLogic | |||
return TrinaryLogic::createFromBoolean($this->acceptsNamedArguments); | |||
} | |||
|
|||
private function isFunctionNodeVariadic(Function_ $node): bool |
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.
It's a bit slower but maybe we'll find opportunities elsewhere :) Thank you. |
thank you! |
Oh yeah, I did it: 19cd999 Previously |
In my measurements it's now fast as before without writing anything to disk. |
as described in phpstan/phpstan#11748 (comment)