Call leaveNode() on visitors in reverse order

Node visitation is now properly nested. The call sequence will now be $visitor1->enterNode($n); $visitor2->enterNode($n); $visitor2->leaveNode($n); $visitor1->leaveNode($n); rather than $visitor1->enterNode($n); $visitor2->enterNode($n); $visitor1->leaveNode($n); $visitor2->leaveNode($n); Fixes #899.
nikic · May 21, 2023 · 8490c0e · 8490c0e
1 parent 8bc6982
commit 8490c0e
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 23 deletions.
diff --git a/lib/PhpParser/NodeTraverser.php b/lib/PhpParser/NodeTraverser.php
@@ -72,7 +72,8 @@ public function traverse(array $nodes): array {
 
         $nodes = $this->traverseArray($nodes);
 
-        foreach ($this->visitors as $visitor) {
+        for ($i = \count($this->visitors) - 1; $i >= 0; --$i) {
+            $visitor = $this->visitors[$i];
             if (null !== $return = $visitor->afterTraverse($nodes)) {
                 $nodes = $return;
             }
@@ -99,7 +100,7 @@ protected function traverseNode(Node $node): Node {
                 }
             } elseif ($subNode instanceof Node) {
                 $traverseChildren = true;
-                $breakVisitorIndex = null;
+                $visitorIndex = -1;
 
                 foreach ($this->visitors as $visitorIndex => $visitor) {
                     $return = $visitor->enterNode($subNode);
@@ -111,7 +112,6 @@ protected function traverseNode(Node $node): Node {
                             $traverseChildren = false;
                         } elseif (NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN === $return) {
                             $traverseChildren = false;
-                            $breakVisitorIndex = $visitorIndex;
                             break;
                         } elseif (NodeVisitor::STOP_TRAVERSAL === $return) {
                             $this->stopTraversal = true;
@@ -131,7 +131,8 @@ protected function traverseNode(Node $node): Node {
                     }
                 }
 
-                foreach ($this->visitors as $visitorIndex => $visitor) {
+                for (; $visitorIndex >= 0; --$visitorIndex) {
+                    $visitor = $this->visitors[$visitorIndex];
                     $return = $visitor->leaveNode($subNode);
 
                     if (null !== $return) {
@@ -152,10 +153,6 @@ protected function traverseNode(Node $node): Node {
                             );
                         }
                     }
-
-                    if ($breakVisitorIndex === $visitorIndex) {
-                        break;
-                    }
                 }
             }
         }
@@ -176,7 +173,7 @@ protected function traverseArray(array $nodes): array {
         foreach ($nodes as $i => &$node) {
             if ($node instanceof Node) {
                 $traverseChildren = true;
-                $breakVisitorIndex = null;
+                $visitorIndex = -1;
 
                 foreach ($this->visitors as $visitorIndex => $visitor) {
                     $return = $visitor->enterNode($node);
@@ -194,7 +191,6 @@ protected function traverseArray(array $nodes): array {
                             $traverseChildren = false;
                         } elseif (NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN === $return) {
                             $traverseChildren = false;
-                            $breakVisitorIndex = $visitorIndex;
                             break;
                         } elseif (NodeVisitor::STOP_TRAVERSAL === $return) {
                             $this->stopTraversal = true;
@@ -214,7 +210,8 @@ protected function traverseArray(array $nodes): array {
                     }
                 }
 
-                foreach ($this->visitors as $visitorIndex => $visitor) {
+                for (; $visitorIndex >= 0; --$visitorIndex) {
+                    $visitor = $this->visitors[$visitorIndex];
                     $return = $visitor->leaveNode($node);
 
                     if (null !== $return) {
@@ -236,10 +233,6 @@ protected function traverseArray(array $nodes): array {
                             );
                         }
                     }
-
-                    if ($breakVisitorIndex === $visitorIndex) {
-                        break;
-                    }
                 }
             } elseif (\is_array($node)) {
                 throw new \LogicException('Invalid node structure: Contains nested arrays');

diff --git a/test/PhpParser/NodeTraverserTest.php b/test/PhpParser/NodeTraverserTest.php
@@ -34,31 +34,45 @@ public function testModifying() {
         $str2Node  = new String_('Bar');
         $printNode = new Expr\Print_($str1Node);
 
-        // first visitor changes the node, second verifies the change
-        $visitor1 = new NodeVisitorForTesting([
+        // Visitor 2 performs changes, visitors 1 and 3 observe the changes.
+        $visitor1 = new NodeVisitorForTesting();
+        $visitor2 = new NodeVisitorForTesting([
             ['beforeTraverse', [], [$str1Node]],
             ['enterNode', $str1Node, $printNode],
             ['enterNode', $str1Node, $str2Node],
             ['leaveNode', $str2Node, $str1Node],
             ['leaveNode', $printNode, $str1Node],
             ['afterTraverse', [$str1Node], []],
         ]);
-        $visitor2 = new NodeVisitorForTesting();
+        $visitor3 = new NodeVisitorForTesting();
 
         $traverser = new NodeTraverser();
         $traverser->addVisitor($visitor1);
         $traverser->addVisitor($visitor2);
+        $traverser->addVisitor($visitor3);
 
         // as all operations are reversed we end where we start
         $this->assertEquals([], $traverser->traverse([]));
         $this->assertEquals([
-            ['beforeTraverse', [$str1Node]],
-            ['enterNode', $printNode],
-            ['enterNode', $str2Node],
+            // Sees nodes before changes on entry.
+            ['beforeTraverse', []],
+            ['enterNode', $str1Node],
+            ['enterNode', $str1Node],
+            // Sees nodes after changes on leave.
             ['leaveNode', $str1Node],
             ['leaveNode', $str1Node],
             ['afterTraverse', []],
-        ], $visitor2->trace);
+        ], $visitor1->trace);
+        $this->assertEquals([
+            // Sees nodes after changes on entry.
+            ['beforeTraverse', [$str1Node]],
+            ['enterNode', $printNode],
+            ['enterNode', $str2Node],
+            // Sees nodes before changes on leave.
+            ['leaveNode', $str2Node],
+            ['leaveNode', $printNode],
+            ['afterTraverse', [$str1Node]],
+        ], $visitor3->trace);
     }
 
     public function testRemoveFromLeave() {
@@ -71,8 +85,8 @@ public function testRemoveFromLeave() {
         $visitor2 = new NodeVisitorForTesting();
 
         $traverser = new NodeTraverser();
-        $traverser->addVisitor($visitor);
         $traverser->addVisitor($visitor2);
+        $traverser->addVisitor($visitor);
 
         $stmts = [$str1Node, $str2Node];
         $this->assertEquals([$str2Node], $traverser->traverse($stmts));