Taking advantage of static reflection

[ondrejmirtes]

Hi, I'm reacting to #3488 (comment) which is about PHPStan 0.12.26.

Rector can take advantage of it, you should take these steps:

1. Use ReflectionProvider interface instead of Broker class in places where you're obtaining class/function/constant reflections. They have the same API.
2. I'm not sure how Rector's DI works. In PHPStan the current stable ReflectionProvider looks like this:

ChainReflectionProvider
- ClassBlacklistReflectionProvider
  - RuntimeReflectionProvider
- BetterReflectionProvider

The class blacklist is there so that some certain classes are skipped for the runtime reflection and go straight to static reflection.

If you enable the experimental flag featureToggles.disableRuntimeReflectionProvider (set to true) then the reflection provider instance is just going to be the BetterReflectionProvider.

The most important thing about BetterReflectionProvider is the source locator chain it uses. In PHPStan it's created in this class: https://github.com/phpstan/phpstan-src/blob/master/src/Reflection/BetterReflection/BetterReflectionSourceLocatorFactory.php (you might want to write your own).

It's quite difficult to construct BetterReflectionProvider by hand but fortunately there's a Nette DI-generated factory based on this interface: https://github.com/phpstan/phpstan-src/blob/master/src/Reflection/BetterReflection/BetterReflectionProviderFactory.php

Here are the services definitions: https://github.com/phpstan/phpstan-src/blob/master/conf/config.neon#L1095-L1125

Let me know if you have any questions 👍

[TomasVotruba]

Hi Ondra,

I just found out that internal errors were reported as positive in CI 🤦‍♂️

After fixin that, I've discovered that between PHPStan 0.12.25 and 0.12.26 the reflection broke.
I went through links and configs and I'm trying to fix it for last 2 hours with no success. So I ask you for help.

It breaks here:

rector/packages/node-type-resolver/src/PHPStan/Scope/PHPStanNodeScopeResolver.php

Line 162 in 5cb1947

$this->nodeScopeResolver->processNodes($nodes, $scope, $nodeCallback);

Even if I make $nodeCallback callback empty, it's still fails.

Any ideas?

[TomasVotruba]

That's weird, CI on Github Action passes: https://github.com/rectorphp/rector/pull/3628/checks?check_run_id=828250558

I'll investigate

[TomasVotruba]

Allright, deleting all PHPStan, /vendor and Rector /tmp cache for everything helped 😓

I'll try to use the latest PHPStan version and upgrade to the new static reflection

[TomasVotruba]

I've found it! It seem the robot loader cache stored files.

This fixed it:

b33ff26 (#3628)

[ondrejmirtes]

PHPStan no longer uses RobotLoader, you shouldn't need it either, it's not exactly static in nature :)

[TomasVotruba]

How do I load the stubs then?

[ondrejmirtes]

Learn how the static reflection works - especially source locators. When you set up the static reflection engine, point one of the source locators at the stubs directory. That’s all.

[TomasVotruba]

How exactly can I add such directory in phpstan.neon? Just:

scanDirectories:
        - stubs

[ondrejmirtes]

Yes but they need to be complete but it doesn't look like they are: https://github.com/rectorphp/rector/blob/master/stubs/PHPUnit/PHPUnit_Framework_TestCase.php which will lead to analysis failures (unknown methods etc).

Not sure what you need the "stubs" for - usually these classes already exist in analysed projects, why define them again but empty?

[ondrejmirtes]

BTW the fact you currently have the stubs in the current shape and form is the reason why the current dev-master of phpstan/phpstan shows these errors:

 ------ ----------------------------------------------------------------------------------------------
  Line   compiler/src/Console/Command/CompileCommand.php
 ------ ----------------------------------------------------------------------------------------------
  88     Class Symfony\Component\Process\Process does not have a constructor and must be instantiated
         without any parameters.
  98     Call to an undefined method Symfony\Component\Process\Process::mustRun().
  114    Class Symfony\Component\Process\Process does not have a constructor and must be instantiated
         without any parameters.
  116    Call to an undefined method Symfony\Component\Process\Process::mustRun().
  137    Class Symfony\Component\Process\Process does not have a constructor and must be instantiated
         without any parameters.
  138    Call to an undefined method Symfony\Component\Process\Process::mustRun().
 ------ ----------------------------------------------------------------------------------------------

 ------ ----------------------------------------------------------------------------------------------
  Line   packages/rector-generator/src/Composer/ComposerPackageAutoloadUpdater.php
 ------ ----------------------------------------------------------------------------------------------
  96     Class Symfony\Component\Process\Process does not have a constructor and must be instantiated
         without any parameters.
  97     Call to an undefined method Symfony\Component\Process\Process::run().
 ------ ----------------------------------------------------------------------------------------------

 ------ ----------------------------------------------------------------------------------------------
  Line   utils/project-validator/src/Process/ParallelTaskRunner.php
 ------ ----------------------------------------------------------------------------------------------
  132    Call to an undefined method Symfony\Component\Process\Process::start().
  144    Call to an undefined method Symfony\Component\Process\Process::isRunning().
  157    Call to an undefined method Symfony\Component\Process\Process::getErrorOutput().
  157    Call to an undefined method Symfony\Component\Process\Process::getOutput().
  185    Class Symfony\Component\Process\Process does not have a constructor and must be instantiated
         without any parameters.
  196    Call to an undefined method Symfony\Component\Process\Process::isSuccessful().
  205    Call to an undefined method Symfony\Component\Process\Process::getErrorOutput().
  205    Call to an undefined method Symfony\Component\Process\Process::getOutput().
 ------ ----------------------------------------------------------------------------------------------

You shouldn't have stubs for something you already have in require/require-dev as they can conflict like this.

[afilina]

I'll take a stab at changing Rector to leverage static reflection.

[afilina]

@TomasVotruba Since we have cinfirmation that PHPStan no longer uses RobotLoader, it would seem that the Option::AUTOLOAD_PATHS in rector.php have no effect. As long as I provide parameters.scanDirectories in phpstan.neon, all the classes in those dirs are found regardless of the Rector config.

I tried my best to create a failing integration test with the latest PHPStan, but I can't. This is either great news or I'm doing something terribly wrong.

This test will pass: https://github.com/afilina/rector/pull/1/files

[afilina]

@ondrejmirtes Will a duplicate symbol from a different file just be ignored once cached? I often analyze cade that has the same class names but are actually different, because they're loaded using different include chains.

[TomasVotruba]

As long as I provide parameters.scanDirectories in phpstan.neon, all the classes in those dirs are found regardless of the Rector config.

That sounds good. How can we make it work in cases without phpstan.neon? Rector should not depend directly on it.

[ondrejmirtes]

@TomasVotruba I described that already, you need to take advantage of source locators and set them up based on directories passed to Rector.

[ondrejmirtes]

Will a duplicate symbol from a different file just be ignored once cached?

@afilina I need to know in which context are you asking about this. Are you talking about PHPStan's analysis and PHPStan's result cache?

Multiple classes with the same name are fine as long as they stay local. PHPStan can analyse class Foo that's both in A.php and B.php independently. The problems come when you have class Bar extends Foo, or if you reference Foo some other way in a different file. That's an undefined behaviour and mostly depends on which class occurence is discovered by a source locator first.

[ondrejmirtes]

Of course that can be different when it comes to Rector doing its thing.

[afilina]

@ondrejmirtes

That's an undefined behaviour and mostly depends on which class occurence is discovered by a source locator first.

That answers my question, thanks. I'll need to come up with sensible behavior for Rector given this constraint. In the past, with runtime reflection, it would have just crashed on that use case.

[TomasVotruba]

@ondrejmirtes Hi Ondra, I'm looking into this again so we can resolve it and benefit from it.
I was able to get BetterReflectionProvider with feature toggle as you mentioned.

I was not able to find any source locator inside. How can we tell PHPStan that file some_file.php is one of sources?

I did something similar for NodeScopeResolver to make PHPStan work with traits years ago, so I need the same for BetterReflectionProvider:

$nodeScopeResolver->setAnalysedFiles(['some_file.php']);

The simpler the better :) I need working even hacky example, the quality of solution can be improved later.

[ondrejmirtes]

Source locators are inside the various reflectors here: https://github.com/phpstan/phpstan-src/blob/c1f7aafc47d007cbb82114ce1fb306a971333f26/src/Reflection/BetterReflection/BetterReflectionProvider.php#L45-L49

You can see which source locators (and the order) PHPStan itself uses here: https://github.com/phpstan/phpstan-src/blob/master/src/Reflection/BetterReflection/BetterReflectionSourceLocatorFactory.php

To locate sources in a single file, you'd want OptimizedSingleFileSourceLocator.

[TomasVotruba]

I was able to get ClassReflector, but I don't know how to set the file into it.

[ondrejmirtes]

SourceLocator is the one and only constructor argument of ClassReflector.

[ondrejmirtes]

You can also get a lot of knowledge from BetterReflection's docs: https://github.com/Roave/BetterReflection/blob/4.13.x/docs/usage.md

[TomasVotruba]

I think I have idea how to create them and bind them manually apart PHPStan. I'm lacking knowledge on how to set them to PHPStan after PHPStan container is compiled.

I've checked the docs and used this package on ApiGen, it seems my use case is not covered.

To use my own SourceLocator I have to remove the original one and replace it with AggregateSourceLocator to keep original one?

Isn't there something on the fly? Like:

$sourceLocator->addFile(....);

Each test is creating a new file that we need to parse, so I would have to create new PHPStanContainer for every single test 🤔

[ondrejmirtes]

I'm lacking knowledge on how to set them to PHPStan after container is compiled.

The best way is to create your own factory that returns SourceLocator and set it as this service in your DI overrides:

	betterReflectionSourceLocator:
		class: Roave\BetterReflection\SourceLocator\Type\SourceLocator
		factory: @PHPStan\Reflection\BetterReflection\BetterReflectionSourceLocatorFactory::create
		autowired: false

[TomasVotruba]

The best way is to create your own factory that returns SourceLocator and set it as this service in your DI overrides:

I see. I'll try to hack in my service that will be both part of Rector and PHPStan. I get it better now. Thanks 👍