RichParser: Remove additional node-traversal pass - #6612

//cc @SanderMuller

This pull request has been marked as ready for review.

I ran this against 2.3.x 3bc4b93d6. Parsing gets about 6% cheaper. But with the pipe transform in the main pass, the other visitors see a different tree around a pipe, and that changes analysis results.

The first pass used to rewrite every Pipe into a call before any other visitor ran. Now PipeTransformerVisitor rewrites a pipe only when the traverser reaches it. A visitor that looks at a child from the parent's enterNode() sees the child before that happens. ReversePipeTransformerVisitor now runs in enterNode() too, so the traverser then goes into the children of the rebuilt Pipe instead of the call.

Three results that change, all with the snippets below at level 8:

<?php declare(strict_types = 1);
namespace PipeArrow\Sub;
class Helper { /** @return non-empty-string */ public static function run(string $s): string { return $s . 'x'; } }
/** @return list<int> */
function listy(string $s): array { return [1]; }
namespace PipeArrow;
use PipeArrow\Sub\Helper;
use function PipeArrow\Sub\listy;
function g(string $s): void
{
	$a = $s |> Helper::run(...) |> (fn ($v) => \PHPStan\dumpType($v));
	$b = $s |> listy(...) |> (fn ($v) => \PHPStan\dumpType($v));
	$c = $s |> Helper::run(...) |> (function ($v) { \PHPStan\dumpType($v); });
}

1. The parameter of a closure or arrow function at the end of a pipe chain loses its type. 2.3.x dumps non-empty-string, list<int> and non-empty-string. This PR dumps mixed three times. ArrowFunctionArgVisitor and ClosureArgVisitor store the call's arguments in enterNode(), and the inner pipe in those arguments is not rewritten yet. The traverser never visits it after that, so its names stay unresolved.

<?php declare(strict_types = 1);
namespace PipeLast;
function f(int $x): void
{
	if ($x > 0) {
	} elseif ($x |> is_int(...)) {
	}
	echo match (true) {
		$x > 0 => 1,
		$x |> is_int(...) => 2,
	};
}

2. LastConditionVisitor marks $elseif->cond and the last match arm from the parent's enterNode(), so now it marks the Pipe and not the call. 2.3.x reports Elseif condition is always true. on the elseif line. This PR reports Call to function is_int() with int<min, 0> will always evaluate to true. on the elseif line and on the match arm.

3. With bleedingEdge (checkImportedClassNameCase), $s |> helper::run(...) after use PipeCase\Sub\Helper; no longer reports Class PipeCase\Sub\Helper referenced with incorrect case. The plain call helper::run($s) still does. The traverser visits the rebuilt helper::run(...), so NameResolver resolves it twice, and originalName becomes the already resolved name.

All three give the same result with the turbo extension loaded (71cb8ca, diagnose says enabled).

The reverse pass alone can join the main pass without these effects: reverse in leaveNode() and register the visitor first. php-parser calls leaveNode() in reverse order, so it then runs after every other visitor, and a node replaced in leaveNode() is not traversed again. The forward pass has to stay separate, because every visitor that tags a child from the parent's enterNode() needs the rewritten tree. On top of 2.3.x:

--- a/src/Parser/ReversePipeTransformerVisitor.php
+++ b/src/Parser/ReversePipeTransformerVisitor.php
 	#[Override]
-	public function enterNode(Node $node): ?Node
+	public function leaveNode(Node $node): ?Node
 	{
--- a/src/Parser/RichParser.php
+++ b/src/Parser/RichParser.php
 		$nodeTraverser = new NodeTraverser();
+		// first, so that its leaveNode() runs after every other visitor's: leave order is the reverse of enter order
+		$nodeTraverser->addVisitor(new ReversePipeTransformerVisitor());
 		$nodeTraverser->addVisitor($this->nameResolver);
@@
-		$reversePipeTransformer = new NodeTraverser(new ReversePipeTransformerVisitor());
-		/** @var array<Node\Stmt> */
-		$nodes = $reversePipeTransformer->traverse($nodes);
-

That gives the 2.3.x result for all three snippets. It saves much less, though: see the numbers below. Whether about 2% of parse time is worth the change is your call.

What I checked, with 2.3.x 3bc4b93d6 as base, this PR at 82a6978e9, and the change above:

• I dumped the RichParser AST with every attribute and sub-node. That covers the 19 files under tests/ that contain |>, plus 5 files of my own. Two dumps of base are identical. This PR differs from base in 18 of the 24 files. The change above differs in 2. There, the argument list that ArrowFunctionArgVisitor and ClosureArgVisitor store holds the reversed Pipe, as the final tree does. I found no output difference from that.
• make tests passes on this PR and with the change (22366 tests), so no existing test covers the three results above. make phpstan and phpcs are clean with the change.
• $t |> TypeTraverser::map(...) also leaks the depth of TypeTraverserInstanceofVisitor in this PR, because leaveNode() gets the Pipe. That call is not valid code, so this one is theoretical.

Performance: RichParser::parseString() over the 1519 files PHPStan analyses in Tempest, user CPU, 5 interleaved rounds, turbo not loaded. Base takes 1198 ms (median, range 1190-1201), this PR 1130 ms (1122-1135) and the change above 1177 ms (1160-1199). So most of the saving comes from merging the forward pass, which is the part that changes the tree.

CI: each of the 12 red checks also fails on #6611 or #6608 today. The two Test (PHP …) jobs are phpbench. Locally, the five variants that failed on both PHP versions are flat over 3 rounds (base, then this PR):

• bug-7581: 287.0-288.6 ms, 285.2-287.5 ms
• bug-12671: 458.2-461.3 ms, 458.9-464.7 ms
• bug-14462: 188.4-189.6 ms, 190.2-190.3 ms
• bug-14972-concat: 1.139-1.142 s, 1.136-1.140 s
• bug-15061: 1.172-1.177 s, 1.169-1.172 s

yeah, that's why the source works like it works today

The new version keeps the tree as it is on 2.3.x. The reverse pass only acts on calls that carry originalPipeAttrs, and only PipeTransformerVisitor sets that. So skipping the reverse pass when nothing was rewritten cannot change the result.

I checked it against 2.3.x 3bc4b93d6, with this PR at 53df5b72e:

• A dump of the RichParser AST with every attribute and sub-node is identical to 2.3.x for all 325 files I tried. They are the 19 files under tests/ that contain |>, my 5 files from above, 300 Tempest files without a pipe, and the one Tempest file with a pipe (packages/mapper/src/Mappers/ArrayToObjectMapper.php).
• The three snippets from my comment above give the 2.3.x result again, with and without the turbo extension.
• make tests (22366 tests), make phpstan and phpcs pass. Neither pipe visitor is shadowed by turbo.

Whole-parse timings were too noisy here today, at a load of 20-50. So I timed the passes themselves over the 1519 Tempest files, as the minimum of 15 runs, twice. The forward pass costs 48.6-49.4 ms and the reverse pass 56.6-59.2 ms, against 1220-1239 ms for all of parseString(). This version therefore saves about 57 ms, or 4.6% of parse time, on files without a pipe.

The forward pass can be skipped the same way. A Pipe node needs the |> token, so a file without the text |> has no pipe to rewrite. A |> in a string or a comment only means that the passes run as today. str_contains() over all 1519 files costs 0.11 ms, and 1 of them contains |>. On top of this PR:

+		// a Pipe node needs the |> token, so without it neither pipe pass can change anything
+		$hasPipe = str_contains($sourceCode, '|>');
 		$pipeTransformerVisitor = new PipeTransformerVisitor();
-		$pipeTransformer = new NodeTraverser($pipeTransformerVisitor);
-		/** @var array<Node\Stmt> */
-		$nodes = $pipeTransformer->traverse($nodes);
+		if ($hasPipe) {
+			$pipeTransformer = new NodeTraverser($pipeTransformerVisitor);
+			/** @var array<Node\Stmt> */
+			$nodes = $pipeTransformer->traverse($nodes);
+		}

With that change, the same 325 files still give an identical AST, and make tests and make phpstan still pass. It skips about 105 ms instead of 57 ms.

Each of the 13 red checks also fails on #6609, #6604 or #6611 today.

in my measures the e2e time does not get faster :-(