phpstan-symfony icon indicating copy to clipboard operation
phpstan-symfony copied to clipboard

Analyzing services with 'get' methods breaks following files

Open nreynis opened this issue 5 years ago • 5 comments

Bug report

Analyzing nested services which have get methods produces weird side effect on other files.
It seems to break something container/autoloader related.
It then causes classes to not be loaded properly and not be processed by dg/bypass-finals.
And breaks (in this example) intersection types.

Having one service with a get method doesn't seems to be enough, I can only reproduce it with two.
This is really hard to pinpoint because it is order dependent, so it may work on some machines because the analysis order is not deterministic. Also parallel processing makes that even more flaky.

The bug may seems to be really obscure edge case, but it's really not that hard to stumble on it: if you have a service/repository architecture (which symfony project usually have) you really likely to have nested get calls.

Minimal reproducible setup

https://github.com/yannickroger/phpstan-troubleshoot

In the neon file if you swap the order of the two analyzed files you can trigger the error on and off.

Actual result

Depending on the analysis order you may have this error:

------ -------------------------------------------------------------
 Line   test/MyClassTest.php
------ -------------------------------------------------------------
 10     PHPDoc tag @var for property MyClassTest::$myClass contains
        unresolvable type.
------ -------------------------------------------------------------

Expected result

Type should be resolved and the analysis shouldn't yield any error, whatever the analysis order is.

Related to

https://github.com/phpstan/phpstan/issues/3179

Likely culprit

Considering it's really dependant on the get method name (renaming methods fix the bug) it is very likely related to this part of the code:
https://github.com/phpstan/phpstan-symfony/blob/master/src/Type/Symfony/ServiceDynamicReturnTypeExtension.php#L41

nreynis avatar Sep 03 '20 11:09 nreynis

So I figured out what's the root problem and it has nothing to do with phpstan-symfony.

bypass-finals registers its own stream wrapper for file://. AutoloadSourceLocator also registers its own stream wrapper for file:// but it seems like it doesn't restore back to bypass-finals but to the PHP one that just reads the file. I don't know how to restore the previous stream wrapper...

ondrejmirtes avatar Sep 03 '20 19:09 ondrejmirtes

Both bypass-finals and AutoloadSourceLocator are horrible hacks... I'd suggest if you want to mock something, don't make it actually final. Create an interface and mock that instead...

ondrejmirtes avatar Sep 03 '20 20:09 ondrejmirtes

You are right, it does not seems to be related to the get method name... I don't know why my tests succeeded yesterday. I probably misinterpreted cached results and jumped on the wrong conclusion.

😞 Altering architecture to accommodate tooling is not a very satisfying solution.

I've found related issues, seems like it clash with infection too:

  • https://github.com/dg/bypass-finals/issues/9
  • https://github.com/infection/infection/issues/1275

If there's no practical solution to the problem, maybe you should do what the creator of infection suggested:

Another idea is to add dg/bypass-finals to composer.json's conflict section so that users can't even install Infection if dg/bypass-finals is used, with a clear explanation in the Docs (yeah, we will still have to deal with PHAR distribution, but that's doable).

nreynis avatar Sep 03 '20 21:09 nreynis

Maybe you could do like phpunit and provide a hook. This way we could re-bind bypass final after each stream wrapper restoration.

That's a bit of plumbing, but the other solutions:

  • stopping using final
  • doubling codebase by adding interface systematically
  • don't do any test that require mock (no unit or integration)

... are not acceptable.

PHPStan is a quality tool, if I have to write worse code to be able to use it, it defeat the purpose.

nreynis avatar Sep 04 '20 01:09 nreynis

Seems like the only thing you need to register stream wrappers is their FQCN, so you could just use a config parameter for that:

parameters:
    streamWrappers:
        # FQCN of the stream wrappers you wan't the analyzer to use
        # see https://www.php.net/manual/en/class.streamwrapper.php
        file: 'DG\BypassFinals'
        # phar: 'streamWrapper\FQCN'

At bootstrap you register them:

foreach ($parameters->streamWrappers as $protocol => $streamWrapperFQCN) {
    spl_autoload_call($streamWrapperFQCN);
    stream_wrapper_unregister($protocol);
    stream_wrapper_register($protocol, $streamWrapperFQCN);
}

In FileReadStreamWrapper finally clause:

try {
    // ...
} finally {
    foreach ($streamWrapperProtocols as $protocol) {
        if (array_key_exists($protocol, $parameters->streamWrappers)) {
            stream_wrapper_unregister($protocol);
            stream_wrapper_register($protocol, $parameters->streamWrappers[$protocol]);
        } else {
            stream_wrapper_restore($protocol);
        }
    }
}

nreynis avatar Sep 04 '20 08:09 nreynis