Skip to content

Remember resolved types after pushInFunctionCall#4648

Merged
ondrejmirtes merged 5 commits intophpstan:2.1.xfrom
staabm:push-resolve
Dec 22, 2025
Merged

Remember resolved types after pushInFunctionCall#4648
ondrejmirtes merged 5 commits intophpstan:2.1.xfrom
staabm:push-resolve

Conversation

@staabm
Copy link
Contributor

@staabm staabm commented Dec 21, 2025 •
edited
Loading

prevents duplicate work, as MethodCalls will re-calculated one less time.

analysing

<?php

class Foo
{

	public function doFoo(DateTimeImmutable $d): void
	{
		$a = $d->format('j. n. Y');
	}

}

before this PR reaches “if ($node instanceof MethodCall) {“ in MutatingScope.php:2161 4 times.
after the PR only 3 times.

with PHPSTAN_FNSR=1 php bin/phpstan analyze foo.php --debug after PR its only 2 times.

➜  phpstan-src git:(push-resolve) ✗ hyperfine -i 'php bin/phpstan analyze foo.php --debug'
Benchmark 1: php bin/phpstan analyze foo.php --debug
  Time (mean ± σ):     743.8 ms ±   1.8 ms    [User: 578.3 ms, System: 161.6 ms]
  Range (min … max):   741.5 ms … 747.1 ms    10 runs
 
  Warning: Ignoring non-zero exit code.
 
➜  phpstan-src git:(push-resolve) ✗ hyperfine -i 'php bin/phpstan analyze foo.php'        
Benchmark 1: php bin/phpstan analyze foo.php
  Time (mean ± σ):      1.047 s ±  0.005 s    [User: 0.777 s, System: 0.255 s]
  Range (min … max):    1.039 s …  1.054 s    10 runs
 
  Warning: Ignoring non-zero exit code.
  
➜  phpstan-src git:(2.1.x) ✗ hyperfine -i 'php bin/phpstan analyze foo.php --debug'
Benchmark 1: php bin/phpstan analyze foo.php --debug
  Time (mean ± σ):     754.9 ms ±   3.3 ms    [User: 587.2 ms, System: 164.1 ms]
  Range (min … max):   752.6 ms … 761.7 ms    10 runs
 
  Warning: Ignoring non-zero exit code.
 
➜  phpstan-src git:(2.1.x) ✗ hyperfine -i 'php bin/phpstan analyze foo.php'        
Benchmark 1: php bin/phpstan analyze foo.php
  Time (mean ± σ):      1.119 s ±  0.054 s    [User: 0.792 s, System: 0.260 s]
  Range (min … max):    1.055 s …  1.166 s    10 runs
 
  Warning: Ignoring non-zero exit code.

ondrejmirtes
ondrejmirtes requested changes Dec 21, 2025
View reviewed changes
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Could be in popInFunctionCall too
  2. BUT: $this->inFunctionCallsStack is used in resolveType() in } elseif ($node instanceof Expr\Closure || $node instanceof Expr\ArrowFunction) { so that could be a problem (the problem should be reproduced in a test first).

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Dec 21, 2025

Issue bot points out an example where this breaks https://phpstan.org/r/dfc26675-234f-47b3-8d0f-dc64b66ef5aa

@staabm
Copy link
Contributor Author

staabm commented Dec 22, 2025

2. BUT: $this->inFunctionCallsStack is used in resolveType() in } elseif ($node instanceof Expr\Closure || $node instanceof Expr\ArrowFunction) { so that could be a problem (the problem should be reproduced in a test first).

I think this problem was caught by existing tests, as the suite failed with the change until I got it right.

@staabm staabm marked this pull request as ready for review December 22, 2025 08:56
@phpstan-bot
Copy link
Collaborator

phpstan-bot commented Dec 22, 2025

This pull request has been marked as ready for review.

@ondrejmirtes ondrejmirtes merged commit 5c67ca8 into phpstan:2.1.x Dec 22, 2025
632 of 640 checks passed
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Dec 22, 2025

Perfect! Thank you. My idea for preserving resolvedTypes in often-called methods like assignVariable, mergeWith, specifyExpressionType is to have something like $scope->resolvedTypes = $this->preserveResolvedTypes($newExpressionTypes) before return $scope;.

This logic doesn't have to be called in methods like enterDeclareStrictTypes, enterClass, enterNamespace because there are unlikely to be any expressions.

And the implementation of preserveResolvedTypes would look at changes between $this->expressionTypes and $newExpressionTypes. For example - if the new scope doesn't change anything about $a, we can preserve resolved types about $a.

This logic is very similar to what invalidateExpression does. But of course the risk is that the logic to figure out what cached types to preserve would be more expensive than to just empty the whole cache every time. So even if we have a proper implementation of the idea and all tests pass, we have to measure whether it's any faster.

Do you think you want to look into this?

@staabm staabm deleted the push-resolve branch December 22, 2025 14:22
@staabm
Copy link
Contributor Author

staabm commented Dec 23, 2025

I can have a look

@staabm
Copy link
Contributor Author

staabm commented Dec 23, 2025 •
edited
Loading

My idea for preserving resolvedTypes in often-called methods like assignVariable, mergeWith, specifyExpressionType is to have something like $scope->resolvedTypes = $this->preserveResolvedTypes($newExpressionTypes) before return $scope;.

I am not sure the memory/perf tradeoff will be good in this direction. a different direction to explore could be trying to implement TypeTraverser natively.

it seems bootleneck enough and way less complex than a TypeCombinator re-implementation in e.g. rust

see blackfire run php vendor/bin/phpunit tests/PHPStan/Analyser/NodeScopeResolverTest.php
grafik

@staabm staabm mentioned this pull request Dec 28, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@staabm @ondrejmirtes @phpstan-bot