Remove inefficient caching from PhpMethodReflection #3534

staabm · 2024-10-03T17:37:01Z

as described in phpstan/phpstan#11748 (comment)

phpstan-bot · 2024-10-03T17:37:13Z

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.

staabm · 2024-10-03T17:37:40Z

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.

phpstan-bot · 2024-10-03T17:58:12Z

This pull request has been marked as ready for review.

ondrejmirtes · 2024-10-03T17:59:56Z

PhpFunctionReflection too please.

src/Reflection/Php/PhpFunctionReflection.php

staabm · 2024-10-03T19:05:50Z

PhpFunctionReflection too please.

here we go

ondrejmirtes

Also, having SimpleParserTest unit test to check for the attributes existence would also be nice.

conf/config.neon

src/Parser/VariadicFunctionsVisitor.php

src/Parser/VariadicMethodsVisitor.php

staabm · 2024-10-06T19:21:24Z

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

src/Parser/VariadicMethodsVisitor.php

ondrejmirtes · 2024-10-07T15:41:52Z

src/Parser/VariadicMethodsVisitor.php

+			}
+
+			$this->classStack[] = $className;
+			$this->inClassLike = $this->inNamespace !== null ? $this->inNamespace . '\\' . implode('\\', $this->classStack) : implode('\\', $this->classStack);


We don't need this property. We should always read the last entry in classStack

AnonymousClassNode is not contained in ClassLike

Err, what? final class AnonymousClassNode extends Class_, class Class_ extends ClassLike.

src/Parser/VariadicMethodsVisitor.php

tests/PHPStan/Parser/ParserTest.php

ondrejmirtes · 2024-10-07T15:46:07Z

src/Parser/VariadicMethodsVisitor.php

+			|| $node instanceof Node\Stmt\ClassLike
+		) {
+			if (!$node->name instanceof Node\Identifier) {
+				$className = 'class@anonymous:' . $node->getStartLine();


This isn't sufficiently unique. This was recently fixed elsewhere by #3328.

ondrejmirtes · 2024-10-07T16:24:21Z

src/Reflection/Php/PhpMethodReflection.php

-					return $this->functionCallStatementFinder->findFunctionCallInStatements(ParametersAcceptor::VARIADIC_FUNCTIONS, $node->getStmts()) !== null;
+				if (
+					is_array($variadicMethods)
+					&& array_key_exists($declaringClass->getName(), $variadicMethods)


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.

I did not find column information in the reflection api.
instead I used start/end-of line combination of the class definition for now.

staabm · 2024-10-08T07:23:47Z

src/Parser/VariadicMethodsVisitor.php

+				$this->classStack[] = $className;
+				$this->inClassLike = $className; // anonymous classes are in global namespace


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.

staabm · 2024-10-08T07:26:06Z

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:

https://phpstan.org/r/5c9c464b-5bef-40c3-b902-c9891308a2f7

This reverts commit 083aaf3.

phpstan-bot · 2024-10-08T08:22:23Z

This pull request has been marked as ready for review.

ondrejmirtes · 2024-10-08T09:23:46Z

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!

staabm · 2024-10-08T09:32:48Z

thank you. thats way simpler - awesome.

staabm · 2024-10-08T09:41:28Z

src/Reflection/Php/PhpFunctionReflection.php

@@ -296,19 +252,4 @@ public function acceptsNamedArguments(): TrinaryLogic
 		return TrinaryLogic::createFromBoolean($this->acceptsNamedArguments);
 	}

-	private function isFunctionNodeVariadic(Function_ $node): bool


I wonder why this $node->params logic is no longer reflected in the patch. it seems it was dead code before

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.

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.

ondrejmirtes · 2024-10-08T11:51:50Z

It's a bit slower but maybe we'll find opportunities elsewhere :) Thank you.

staabm · 2024-10-08T11:52:03Z

thank you!

ondrejmirtes · 2024-10-08T12:42:27Z

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.

ondrejmirtes · 2024-10-08T12:42:43Z

In my measurements it's now fast as before without writing anything to disk.

staabm force-pushed the less-caching branch from 545e3d6 to 594336e Compare October 3, 2024 17:37

staabm force-pushed the less-caching branch 3 times, most recently from af3a0c8 to 0b51b89 Compare October 3, 2024 17:51

staabm marked this pull request as ready for review October 3, 2024 17:58

staabm commented Oct 3, 2024

View reviewed changes

src/Reflection/Php/PhpFunctionReflection.php Outdated Show resolved Hide resolved

staabm force-pushed the less-caching branch from 0475b39 to 0f0cc65 Compare October 3, 2024 18:58

ondrejmirtes requested changes Oct 4, 2024

View reviewed changes

conf/config.neon Show resolved Hide resolved

src/Parser/VariadicFunctionsVisitor.php Outdated Show resolved Hide resolved

clxmstaab force-pushed the less-caching branch 2 times, most recently from c91049b to 15fc96d Compare October 4, 2024 12:31

staabm force-pushed the less-caching branch from 15fc96d to 4fc5d31 Compare October 5, 2024 12:10

ondrejmirtes requested changes Oct 6, 2024

View reviewed changes

src/Parser/VariadicMethodsVisitor.php Outdated Show resolved Hide resolved

staabm force-pushed the less-caching branch 3 times, most recently from 21e164b to 2496e8a Compare October 6, 2024 18:57

staabm mentioned this pull request Oct 7, 2024

Remove $isFinal dead-code in PhpFunctionReflection #3545

Merged

staabm force-pushed the less-caching branch from 2496e8a to 183ef1f Compare October 7, 2024 09:10

ondrejmirtes requested changes Oct 7, 2024

View reviewed changes

staabm commented Oct 8, 2024

View reviewed changes

staabm force-pushed the less-caching branch from a775e37 to 287f74f Compare October 8, 2024 07:24

staabm marked this pull request as draft October 8, 2024 07:45

Remove inefficient caching from PhpMethodReflection

ea07f8c

staabm and others added 11 commits October 8, 2024 10:00

Remove inefficient caching from PhpFunctionReflection

da0aac6

Remove FunctionCallStatementFinder

9a02775

Added *Parser unit tests

b8ee0bc

test nested classes

0e03ad5

test functions

c3cdb0b

unique index for same-line anonymous classes

687c50d

Drop inClassLike property

fc7e0ca

Revert "Drop inClassLike property"

13b1067

This reverts commit 083aaf3.

fix anonymous class detection

c958367

put test out of global namspace

f3fb80a

fix wrong inClassLike after stack pop

676d440

staabm force-pushed the less-caching branch from 9d57607 to 676d440 Compare October 8, 2024 08:00

staabm marked this pull request as ready for review October 8, 2024 08:22

My vision

d4d5c1a

staabm commented Oct 8, 2024

View reviewed changes

ondrejmirtes added 2 commits October 8, 2024 12:05

Optimization

d1a8cbd

Another optimization

cfbadfd

ondrejmirtes merged commit dddf984 into phpstan:2.0.x Oct 8, 2024
392 of 449 checks passed

staabm deleted the less-caching branch October 8, 2024 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove inefficient caching from PhpMethodReflection #3534

Remove inefficient caching from PhpMethodReflection #3534

staabm commented Oct 3, 2024

phpstan-bot commented Oct 3, 2024

staabm commented Oct 3, 2024 •

edited

Loading

phpstan-bot commented Oct 3, 2024

ondrejmirtes commented Oct 3, 2024

staabm commented Oct 3, 2024

ondrejmirtes left a comment

staabm commented Oct 6, 2024 •

edited

Loading

ondrejmirtes Oct 7, 2024

staabm Oct 7, 2024

ondrejmirtes Oct 7, 2024

ondrejmirtes Oct 7, 2024

ondrejmirtes Oct 7, 2024

staabm Oct 8, 2024

staabm Oct 8, 2024

staabm commented Oct 8, 2024 •

edited

Loading

phpstan-bot commented Oct 8, 2024

ondrejmirtes commented Oct 8, 2024

staabm commented Oct 8, 2024

staabm Oct 8, 2024 •

edited

Loading

ondrejmirtes Oct 8, 2024

staabm Oct 8, 2024

ondrejmirtes commented Oct 8, 2024

staabm commented Oct 8, 2024

ondrejmirtes commented Oct 8, 2024

ondrejmirtes commented Oct 8, 2024

		$this->classStack[] = $className;
		$this->inClassLike = $className; // anonymous classes are in global namespace

Remove inefficient caching from PhpMethodReflection #3534

Remove inefficient caching from PhpMethodReflection #3534

Conversation

staabm commented Oct 3, 2024

phpstan-bot commented Oct 3, 2024

staabm commented Oct 3, 2024 • edited Loading

phpstan-bot commented Oct 3, 2024

ondrejmirtes commented Oct 3, 2024

staabm commented Oct 3, 2024

ondrejmirtes left a comment

Choose a reason for hiding this comment

staabm commented Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm commented Oct 8, 2024 • edited Loading

phpstan-bot commented Oct 8, 2024

ondrejmirtes commented Oct 8, 2024

staabm commented Oct 8, 2024

staabm Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes commented Oct 8, 2024

staabm commented Oct 8, 2024

ondrejmirtes commented Oct 8, 2024

ondrejmirtes commented Oct 8, 2024

staabm commented Oct 3, 2024 •

edited

Loading

staabm commented Oct 6, 2024 •

edited

Loading

staabm commented Oct 8, 2024 •

edited

Loading

staabm Oct 8, 2024 •

edited

Loading