search

phpstan / phpdoc-parser

Next-gen phpDoc parser with support for intersection types and generics
MIT License
1.35k stars 61 forks source link

Custom Phpstan rule tests broken after update to 1.20.0 #188

Closed keulinho closed 1 year ago

keulinho commented 1 year ago

After running composer update and updating to v1.20.0 of this lib our custom test case for a phpstan rule fails, because it seems to not find the new TokenIterator::currentTokenLine() method :thinking:

Running the test a second time and everything is fine, but after downgrading this lib and upgrading again to 1.20.0 the first test run fails again with the same message. This is a issue for our CI where we always install a fresh setup and run the tests there, so that is currently broken.

The full error message is:

Error: Call to undefined method PHPStan\PhpDocParser\Parser\TokenIterator::currentTokenLine()

/home/platform/vendor/phpstan/phpdoc-parser/src/Parser/PhpDocParser.php:107
/home/platform/vendor/phpstan/phpdoc-parser/src/Parser/PhpDocParser.php:68
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/PhpDoc/PhpDocStringResolver.php:28
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Type/FileTypeMapper.php:442
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Type/FileTypeMapper.php:245
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Type/FileTypeMapper.php:461
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Type/FileTypeMapper.php:472
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Type/FileTypeMapper.php:428
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Type/FileTypeMapper.php:179
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Type/FileTypeMapper.php:164
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Type/FileTypeMapper.php:113
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Reflection/ClassReflection.php:1155
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Reflection/ClassReflection.php:1185
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Reflection/ClassReflection.php:1098
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Reflection/ClassReflection.php:789
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Reflection/ClassReflection.php:718
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Reflection/ClassReflection.php:1205
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Reflection/ClassReflection.php:1230
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Reflection/Php/PhpClassReflectionExtension.php:398
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Reflection/Php/PhpClassReflectionExtension.php:372
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Reflection/ClassReflection.php:474
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Reflection/ClassReflection.php:486
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/MutatingScope.php:3200
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/MutatingScope.php:1118
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/MutatingScope.php:557
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php:1446
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php:557
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php:327
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/FileAnalyser.php:175
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/Analyser.php:72
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Testing/RuleTestCase.php:113
phar:///home/platform/vendor/phpstan/phpstan/phpstan.phar/src/Testing/RuleTestCase.php:86

Any ideas how and why it seems that for the first execution an old version of the TokenIterator class seems to be autoloaded?

ondrejmirtes commented 1 year ago

What’s your use-case? Why do you install phpstan/phpdoc-parser separately while you also depend on PHPStan?

keulinho commented 1 year ago

we don't install phpstan/phpdoc-parser it is only there as a transient dependency, with the latest release suddenly our CI pipelines failed.

To debug it i tried to manually require phpstan/phpdoc-parser to downgrade it to 1.19.1 and everything worked fine then, removing the separate require again (and thus updating to 1.20.0 again) and the error popped up again.

ondrejmirtes commented 1 year ago

Transient dependency of what?

I’ll probably need a small reproduction repo to see what’s going on and whos fault it is.

keulinho commented 1 year ago

i thought it was a transient dependency from phpstan itself, but turned out that the phpdocumentor/type-resolver package brings along the phpstan/phpdoc-parser.

I'll investigate deeper what's going on, could not reproduce it now.

ondrejmirtes commented 1 year ago

My guess is that the things get mixed up, because phpstan/phpdoc-parser is in phpstan.phar (and that's going to be 1.19.1), and if you install phpstan/phpdoc-parser separately, it's somehow possible the autoloading is gonna mix it up. It's not an easy problem to solve.

keulinho commented 1 year ago

That's a good insight!

I stumpled upon this line of code in the PharAutoloader of phpstan

if (strpos($class, 'PHPStan\\') !== 0 || strpos($class, 'PHPStan\\PhpDocParser\\') === 0) {
      return;
}

So there seems to be already custom handling for the case when PhpDocParser is installed directly as well.

I'm not sure if there is already an issue there, but looking at the stacktrace again it seems to me like the PhpDocParser is wrongly loaded from the vendor folder instead of from the phar, probably because that class is autoloaded before the .phar file is loaded.

And i'm also not sure on how that could be fixed, but i found that there is some kind of prefixing going on in the phpstan phar file for some other dependencies, eg. Nette or Psr stuff and so on, shouldn't the phpdoc-parser also be prefixed to prevent issues likes this?

ondrejmirtes commented 1 year ago

So it’s the opposite - we actually want to load it from the PHAR.

keulinho commented 1 year ago

I mean by prefixing the dependency in phpstan we would make sure that from inside phpstan we always use the version of the doc parser shipped with phpstan and we prevent auto load collisions when the doc parser is installed directly as well

ondrejmirtes commented 1 year ago

I mean by prefixing the dependency in phpstan

Can't do that, it's in PHPStan\ namespace and people rely on it in custom PHPStan extensions.

keulinho commented 1 year ago

Any idea for how to fix this? The only solution i see currently is conflicting the newer doc parser version in our setup, based on what version is included in the packaged phpstan phar :thinking:

ondrejmirtes commented 1 year ago

The easiest fix is to release a new PHPStan version with the new phpdoc-parser included, which will make this problem go away. I'll do that in a matter of days.

In the long term, we should probably do something like including the PHAR preload.php script when the PHPStanTestCase is loaded which would ensure that all the phpdoc-parser classes come from the same version.

keulinho commented 1 year ago

Can you explain the idea with the preload.php in a little more detail? I'd love to give it a try fixing it.

I already tried some approaches there but in the end it either did not fix the issue or make it worse (fatal errors because of trying to redeclare classes).

ondrejmirtes commented 1 year ago

We could put require_once __DIR__ . '/../../preload.php'; at the top of PHPStanTestCase here: https://github.com/phpstan/phpstan-src/blob/1.10.x/src/Testing/PHPStanTestCase.php

In the compiled phpstan.phar file it preloads unprefixed classes: https://github.com/phpstan/phpstan-src/blob/840a64f113c7ca10d5111719a0338914b0051ceb/compiler/src/Console/PrepareCommand.php#L157-L192

I'd rather do it in a major version because it could break someone's use case.

keulinho commented 1 year ago

Ok i tried to patch that into our system, but it did not work that easily because it lead to fatal errors that it cannot redeclare classes.

I debugged a little more and found out that in our case the issue was not that phpstan loads the wrong class, but the root issue is that the PhpDocParser class actually is autoloaded before phpstan itself is loaded (but only on first test execution after running composer update).

After debugging the composer class loading i found that symfony when you use the framework-bundle will load the PhpDocParser during container compilation: https://github.com/symfony/symfony/blob/6.3/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L1853

So after running composer update the symfony container needs to be recompiled once before the tests actually runs, which leads to the newer version of the PhpDocParser being auto loaded for that test execution run.

In that case i actually am not quite sure at how to fix it, because i'm not sure what is expected in that cases :thinking:

keulinho commented 1 year ago

So for now we are running the test case with @runInSeparateProcess which resolves the symfony container compile issue which seems to work around the issue.

github-actions[bot] commented 1 year ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.