[CodeQuality] Fix out-of-order items removal in UnusedForeachValueToA…

…rrayKeysRector (#2779)

rules-tests/CodeQuality/Rector/Foreach_/UnusedForeachValueToArrayKeysRector/Fixture/skip_foreach_destruct_keys.php.inc

@@ -0,0 +1,15 @@
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\Foreach_\UnusedForeachValueToArrayKeysRector\Fixture;
+
+final class SkipForeachDestructKeys
+{
+    public function run(array $definitions)
+    {
+        foreach ($definitions as $id => [$domElement, $file]) {
+            if ($file) {
+                return $file;
+            }
+        }
+    }
+}

rules/CodeQuality/Rector/Foreach_/UnusedForeachValueToArrayKeysRector.php

@@ -81,7 +81,12 @@ public function refactor(Node $node): ?Node
 
         // special case of nested array items
         if ($node->valueVar instanceof Array_) {
-            $node->valueVar = $this->refactorArrayForeachValue($node->valueVar, $node);
+            $valueArray = $this->refactorArrayForeachValue($node->valueVar, $node);
+            if ($valueArray instanceof Array_) {
+                $node->valueVar = $valueArray;
+            }
+
+            // not sure what does this mean :)
             if ($node->valueVar->items !== []) {
                 return null;
             }
@@ -102,23 +107,56 @@ public function refactor(Node $node): ?Node
         return $node;
     }
 
-    private function refactorArrayForeachValue(Array_ $array, Foreach_ $foreach): Array_
+    /**
+     * @param int[] $removedKeys
+     */
+    private function isArrayItemsRemovalWithoutChangingOrder(Array_ $array, array $removedKeys): bool
+    {
+        $hasRemovingStarted = false;
+
+        foreach (array_keys($array->items) as $key) {
+            if (in_array($key, $removedKeys, true)) {
+                $hasRemovingStarted = true;
+            } elseif ($hasRemovingStarted) {
+                // we cannot remove the previous item, and not remove the next one, because that would change the order
+                return false;
+            }
+        }
+
+        return true;
+    }
+
+    private function refactorArrayForeachValue(Array_ $array, Foreach_ $foreach): ?Array_
     {
+        // only last items can be removed, without changing the order
+        $removedKeys = [];
+
         foreach ($array->items as $key => $arrayItem) {
             if (! $arrayItem instanceof ArrayItem) {
-                continue;
+                // only known values can be processes
+                return null;
             }
 
             $value = $arrayItem->value;
             if (! $value instanceof Variable) {
-                return $array;
+                // only variables can be processed
+                return null;
             }
 
             if ($this->isVariableUsedInForeach($value, $foreach)) {
                 continue;
             }
 
-            unset($array->items[$key]);
+            $removedKeys[] = $key;
+        }
+
+        if (! $this->isArrayItemsRemovalWithoutChangingOrder($array, $removedKeys)) {
+            return null;
+        }
+
+        // clear removed items
+        foreach ($removedKeys as $removedKey) {
+            unset($array->items[$removedKey]);
         }
 
         return $array;

tests/PhpParser/Node/NodeFactoryTest.php

@@ -41,47 +41,40 @@ public function testCreateArray(array $inputArray, Array_ $expectedArrayNode): v
      */
     public function provideDataForArray(): Iterator
     {
-        $numberNode = new LNumber(1);
-        $stringNode = new String_('a');
-        $trueNode = new ConstFetch(new Name('true'));
-        $falseNode = new ConstFetch(new Name('false'));
-        $nullNode = new ConstFetch(new Name('null'));
+        $lNumber = new LNumber(1);
+        $string = new String_('a');
+        $trueConstFetch = new ConstFetch(new Name('true'));
+        $falseConstFetch = new ConstFetch(new Name('false'));
+        $nullConstEtch = new ConstFetch(new Name('null'));
 
         $array = new Array_();
-        $array->items[] = new ArrayItem($numberNode);
-
+        $array->items[] = new ArrayItem($lNumber);
         yield [[1], $array];
 
         $array = new Array_();
-        $array->items[] = new ArrayItem($numberNode, $stringNode);
-
+        $array->items[] = new ArrayItem($lNumber, $string);
         yield [[
             'a' => 1,
         ], $array];
 
         $array = new Array_();
-        $array->items[] = new ArrayItem($numberNode);
-
-        yield [[$numberNode], $array];
+        $array->items[] = new ArrayItem($lNumber);
+        yield [[$lNumber], $array];
 
         $array = new Array_();
-        $array->items[] = new ArrayItem($stringNode);
-
-        yield [[$stringNode], $array];
+        $array->items[] = new ArrayItem($string);
+        yield [[$string], $array];
 
         $array = new Array_();
-        $array->items[] = new ArrayItem($trueNode);
-
-        yield [[$trueNode], $array];
+        $array->items[] = new ArrayItem($trueConstFetch);
+        yield [[$trueConstFetch], $array];
 
         $array = new Array_();
-        $array->items[] = new ArrayItem($falseNode);
-
-        yield [[$falseNode], $array];
+        $array->items[] = new ArrayItem($falseConstFetch);
+        yield [[$falseConstFetch], $array];
 
         $array = new Array_();
-        $array->items[] = new ArrayItem($nullNode);
-
-        yield [[$nullNode], $array];
+        $array->items[] = new ArrayItem($nullConstEtch);
+        yield [[$nullConstEtch], $array];
     }
 }