Prevent unnecessary scope merging#4640
Conversation
| public function mergeWith(?self $otherScope): self | ||
| { | ||
| if ($otherScope === null) { | ||
| if ($otherScope === null || $this === $otherScope) { |
There was a problem hiding this comment.
| if ($otherScope === null || $this === $otherScope) { | |
| if ($otherScope === null || $this === $otherScope || $this->equals($otherScope)) { |
There was a problem hiding this comment.
the suggested change will make the test-suite fail with
There was 1 failure:
1) PHPStan\Analyser\NodeScopeResolverTest::testFile with data set "tests/PHPStan/Analyser/nsrt/unset-conditional-expressions.php" ('/Users/staabm/workspace/phpst...ns.php')
Failed assertions in /Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/nsrt/unset-conditional-expressions.php:
Line 27:
Expected: array<string, mixed>
Actual: array{}
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:298
|
Sounds like something to fix. The equals method should probably also check |
adding if we add checks for I think we should merge this PR as is. |
|
Thank you! |
|
Yeah, it's unlikely that two Scopes will be the same if they're not the same object. |
In the profiles of #4628 (comment) we can see
MutatingScope->createConditionalExpressionsas one of the most expensive calls (which gets called viaMutatingScope->merge()):After this PR, we can see
PHPSTAN_FNSR=1 php bin/phpstan analyse src/Analyser/NodeScopeResolver.php -vvv --debugtaking thereturn $this;fast path ~180 times.It seems
make testis 0.5-1 second faster