Prevent unnecessary scope merging

[staabm]

In the profiles of #4628 (comment) we can see MutatingScope->createConditionalExpressions as one of the most expensive calls (which gets called via MutatingScope->merge()):

After this PR, we can see PHPSTAN_FNSR=1 php bin/phpstan analyse src/Analyser/NodeScopeResolver.php -vvv --debug taking the return $this; fast path ~180 times.

It seems make test is 0.5-1 second faster

[ondrejmirtes]

WDYT?

[ondrejmirtes]

Suggested change

	if ($otherScope === null || $this === $otherScope) {
	if ($otherScope === null || $this === $otherScope || $this->equals($otherScope)) {

[staabm]

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

[ondrejmirtes]

Sounds like something to fix. The equals method should probably also check conditionalExpressions property.

[staabm]

Sounds like something to fix. The equals method should probably also check conditionalExpressions property.

adding $this->equals($otherScope) in this already hot path will not save us much I think. it will add additional work in the common case, as we most likely will not have equal scopes. perf wise I think it will be counter productive.

if we add checks for conditionalExpressions property in equals(), I think we also need to add it to generalization?

I think we should merge this PR as is.

[ondrejmirtes]

Thank you!

[ondrejmirtes]

Yeah, it's unlikely that two Scopes will be the same if they're not the same object.