Migrate to nikic/php-parser 5.0

[sebastianbergmann]

The test suite passes with v5.0.0alpha3, no changes were required.

We currently use ParserFactory::create(ParserFactory::PREFER_PHP7, new Lexer) to create a parser instance, this is now deprecated. We should use ParserFactory::createForNewestSupportedVersion() instead.

[buismaarten]

Are you open to Pull requests for this?

[sebastianbergmann]

Are you open to Pull requests for this?

When the time comes, I will do this myself. Thanks!

[buismaarten]

Composer conflicts if you have PHPUnit installed and want to use version 5 of nikic/php-parser. Is it possible to add support both nikic/php-parser version 4 and 5?

[sebastianbergmann]

Not before there is a final release of nikic/php-parser v5.

[ondrejmirtes]

Hi, I'd like to persuade you to support nikic/php-parser ^4 || ^5 in require of this package. Because this package is a dependency of phpunit/phpunit it means the entire PHP ecosystem currently depends on nikic/php-parser v4.

If php-code-coverage switched to ^5 at once, it'd mean that:

• Projects stuck on an older PHPUnit version wouldn't be able to upgrade nikic/php-parser independently.
• Projects stuck on an older PHPUnit wouldn't be able to use any dependencies that require nikic/php-parser ^5.
• Projects that would like to upgrade to the latest PHPUnit version would have to update nikic/php-parser at the same time.
• Projects that would like to upgrade to the latest PHPUnit would also have to wait for all other dependencies to also move to nikic/php-parser ^5.

You can see how this ripples out throughout the entire ecosystem.

I think the compatibility with both versions can be done with very little work on your side, otherwise I wouldn't bother you with this.

I tried to bring support to ^5 and this is all it takes here. Also, all of the changes are essentially forward compatible with ^4.18 so they could be commited today:

diff --git a/src/StaticAnalysis/ExecutableLinesFindingVisitor.php b/src/StaticAnalysis/ExecutableLinesFindingVisitor.php
index 54824314..253132c6 100644
--- a/src/StaticAnalysis/ExecutableLinesFindingVisitor.php
+++ b/src/StaticAnalysis/ExecutableLinesFindingVisitor.php
@@ -122,6 +122,12 @@ public function enterNode(Node $node): void
             return;
         }
 
+		if ($node instanceof Node\Stmt\Expression && $node->expr instanceof Node\Expr\Throw_) {
+			$this->setLineBranch($node->expr->expr->getEndLine(), $node->expr->expr->getEndLine(), ++$this->nextBranch);
+
+			return;
+		}
+
         if ($node instanceof Node\Stmt\Enum_ ||
             $node instanceof Node\Stmt\Function_ ||
             $node instanceof Node\Stmt\Class_ ||
diff --git a/src/StaticAnalysis/ParsingFileAnalyser.php b/src/StaticAnalysis/ParsingFileAnalyser.php
index 3d1b5c88..3190bb4e 100644
--- a/src/StaticAnalysis/ParsingFileAnalyser.php
+++ b/src/StaticAnalysis/ParsingFileAnalyser.php
@@ -22,7 +22,6 @@
 use function token_get_all;
 use function trim;
 use PhpParser\Error;
-use PhpParser\Lexer;
 use PhpParser\NodeTraverser;
 use PhpParser\NodeVisitor\NameResolver;
 use PhpParser\NodeVisitor\ParentConnectingVisitor;
@@ -140,10 +139,7 @@ private function analyse(string $filename): void
 
         assert($linesOfCode > 0);
 
-        $parser = (new ParserFactory)->create(
-            ParserFactory::PREFER_PHP7,
-            new Lexer
-        );
+        $parser = (new ParserFactory)->createForHostVersion();
 
         try {
             $nodes = $parser->parse($source);
diff --git a/tests/tests/StaticAnalysis/CodeUnitFindingVisitorTest.php b/tests/tests/StaticAnalysis/CodeUnitFindingVisitorTest.php
index 5042b222..0f1a8c77 100644
--- a/tests/tests/StaticAnalysis/CodeUnitFindingVisitorTest.php
+++ b/tests/tests/StaticAnalysis/CodeUnitFindingVisitorTest.php
@@ -144,7 +144,7 @@ public function testHandlesFunctionOrMethodWithDisjunctiveNormalFormTypes(): voi
 
     private function findCodeUnits(string $filename): CodeUnitFindingVisitor
     {
-        $nodes = (new ParserFactory)->create(ParserFactory::PREFER_PHP7)->parse(
+        $nodes = (new ParserFactory)->createForHostVersion()->parse(
             file_get_contents($filename)
         );
 
diff --git a/tests/tests/StaticAnalysis/ExecutableLinesFindingVisitorTest.php b/tests/tests/StaticAnalysis/ExecutableLinesFindingVisitorTest.php
index 95491fe9..72e02b8c 100644
--- a/tests/tests/StaticAnalysis/ExecutableLinesFindingVisitorTest.php
+++ b/tests/tests/StaticAnalysis/ExecutableLinesFindingVisitorTest.php
@@ -13,7 +13,6 @@
 use function file_get_contents;
 use function preg_match;
 use function str_contains;
-use PhpParser\Lexer;
 use PhpParser\NodeTraverser;
 use PhpParser\ParserFactory;
 use PHPUnit\Framework\Attributes\CoversClass;
@@ -43,10 +42,7 @@ public function testExecutableLinesAreGroupedByBranchPhp82(): void
     private function doTestSelfDescribingAsset(string $filename): void
     {
         $source = file_get_contents($filename);
-        $parser = (new ParserFactory)->create(
-            ParserFactory::PREFER_PHP7,
-            new Lexer
-        );
+        $parser = (new ParserFactory)->createForHostVersion();
         $nodes                         = $parser->parse($source);
         $executableLinesFindingVisitor = new ExecutableLinesFindingVisitor($source);

It's also fair to mention that:

• With the changes above I have one failing test - ExecutableLinesFindingVisitorTest::testExecutableLinesAreGroupedByBranch shows numbers off-by-one from line 322 onward and I couldn't figure out why.
• We'd probably need to test both "lowest" and "highest" dependencies in CI which isn't currently done here.
• We'd also need to do the same thing in sebastian/complexity and sebastian/lines-of-code.

Please let me know if you're on board with this. Thank you.

[ondrejmirtes]

Nikita shares the same hope: nikic/PHP-Parser#929 (comment)

Yeah, I do hope that a handful of core ecosystem dependencies like PHP-Unit can support version 4.x and 5.x at the same time. I think that for "simple" users of PHP-Parser this should be possible with little effort. In the last 4.x version I pushed some forward-compatibility APIs to make it easier to support both.

[sebastianbergmann]

You can see how this ripples out throughout the entire ecosystem.

Only because the ecosystem insists on mixing runtime dependencies such as PHP-Parser with development-time dependencies such as PHPUnit. In a perfect world, where the entire ecosystem would not install PHPUnit using Composer and instead use the PHAR distribution of PHPUnit, this would not be a problem.

</rant> (sorry)

My comment back in September ("Not before there is a final release of nikic/php-parser v5.") was phrased poorly. What I meant was: not before there is a release candidate of PHP-Parser 5.x.

I plan to support PHP-Parser 5.x alongside PHP-Parser 4.x and will more or less do that as you outlined. I cannot (and will not) promise to get this done before the holidays, but I will do my best.

[ondrejmirtes]

and instead use the PHAR distribution of PHPUnit, this would not be a problem

Yes, a few years ago I realized that using PHPStan as a PHAR is a better experience so it should be the only experience. So people can only get PHPStan as a PHAR and it's a Composer package so that they can install PHPStan extensions with proper version constraints alongside it too.

There are some downsides and gotchas and I could list and describe all of them somewhere if you'd be interested, but generally it mostly works and once the initial bugs were exterminated the problems are very rare.

I cannot (and will not) promise to get this done before the holidays, but I will do my best.

I've only wanted to ask you to support 5 together with 4 once you start supporting 5, I didn't mean you need to start supporting 5 as quickly as possible. There's no rush.

Thank you for your answer, I really appreciate it!

[sebastianbergmann]

sebastian/complexity 3.2.0 and sebastian/lines-of-code 2.0.2 with support for nikic/php-parser 5.x have been released.

[sebastianbergmann]

I have pushed changes to make this component compatible with PHP-Parser ^4.18 and PHP-Parser ^5.0.

I can confirm that a test for the ExecutableLinesFindingVisitor fails with PHP-Parser 5.0. This is related to how throw statements are processed. Maybe there are additional changes (related to line numbers) than just Stmt\Throw_ versus Stmt\Expression and Expr\Throw_?

Maybe @nikic (or @dvdoug) can have a look?

I am travelling all day and may not be able to look into this before I get home.

[ondrejmirtes]

@sebastianbergmann Awesome! I'll throw in asking for one more favour - any chance you could do this for PHPUnit ^9? It has support until February 2, 2024 and it would allow everyone on PHP >= 7.3 to benefit from this too.

In practice it'd mean to add support for PHP-Parser 5 in phpunit/php-code-coverage ^9.2.28, sebastian/complexity ^2.0 and sebastian/lines-of-code ^1.0.3. Thank you for considering that, although it's already awesome that PHPUnit 10 will support ^4 || ^5!

[sebastianbergmann]

PHPUnit 9.6 (and its dependencies) support PHP 7.3 which is not supported by PHP-Parser 5.x.

[ondrejmirtes]

Yeah but there are still people on 7.4 and 8.0 and it would help them.

[sebastianbergmann]

I will look into it.

Would it be okay if PHPUnit 11, due in February, would only support PHP-Parser 5.x? I do not want to support two major versions of the same dependency forever.

[ondrejmirtes]

Would it be okay if PHPUnit 11, due in February, would only support PHP-Parser 5.x?

Most likely yes. Thank you!

[ondrejmirtes]

I'm looking into the ExecutableLinesFindingVisitorTest failure. And I think I figured it out. The problem is that throw expression inside Statement expresion (the new way such code is represented) is counted twice by the visitor.

The first time it gets counted thanks to if ($node instanceof Node\Stmt\Expression && $node->expr instanceof Node\Expr\Throw_) { but as the traverser descends into this it also gets counted again with the last "fallback" line in enterNode:

$this->setLineBranch($node->getStartLine(), $node->getEndLine(), ++$this->nextBranch);

I was able to make the test pass with:

diff --git a/src/StaticAnalysis/ExecutableLinesFindingVisitor.php b/src/StaticAnalysis/ExecutableLinesFindingVisitor.php
index a13ad3eb..fedc6215 100644
--- a/src/StaticAnalysis/ExecutableLinesFindingVisitor.php
+++ b/src/StaticAnalysis/ExecutableLinesFindingVisitor.php
@@ -107,6 +107,7 @@ public function enterNode(Node $node): void
             $node instanceof Node\Expr\ConstFetch ||
             $node instanceof Node\Expr\Match_ ||
             $node instanceof Node\Expr\Variable ||
+            $node instanceof Node\Expr\Throw_ ||
             $node instanceof Node\ComplexType ||
             $node instanceof Node\Const_ ||
             $node instanceof Node\Identifier ||

But I'm not sure if it's the right solution and how do you want to count throw expressions that aren't on separate lines, for example in $foo ?? throw $e.

[sebastianbergmann]

I have released 1.10.11 with this fix. Thanks!

[sebastianbergmann]

@sebastianbergmann Awesome! I'll throw in asking for one more favour - any chance you could do this for PHPUnit ^9? It has support until February 2, 2024 and it would allow everyone on PHP >= 7.3 to benefit from this too.

sebastian/complexity 2.0.3, sebastian/lines-of-code 1.0.4, and phpunit/php-code-coverage 9.2.30 have been released with support for nikic/php-parser 5.x. These are the versions used by PHPUnit 9.6.

[ondrejmirtes]

Thank you! Working on support for PHP-Parser 5 in real-world projects is now really easy.