Remove ArrayAccess from NodeList and enforce contiguous int keys

CHANGELOG.md

@@ -9,6 +9,10 @@ You can find and compare releases at the [GitHub release page](https://github.co
 
 ## Unreleased
 
+### Changed
+
+- Make `NodeList` an actual list
+
 ## v15.1.0
 
 ### Added

src/Language/AST/NodeList.php

@@ -7,92 +7,73 @@
 /**
  * @template T of Node
  *
- * @phpstan-implements \ArrayAccess<array-key, T>
- * @phpstan-implements \IteratorAggregate<array-key, T>
+ * @phpstan-implements \IteratorAggregate<int, T>
  */
-class NodeList implements \ArrayAccess, \IteratorAggregate, \Countable
+class NodeList implements \IteratorAggregate, \Countable
 {
     /**
-     * @var array<Node|array>
+     * @var list<Node|array<string, mixed>>
      *
-     * @phpstan-var array<T|array<string, mixed>>
+     * @phpstan-var list<T|array<string, mixed>> $nodes
      */
-    private array $nodes;
+    protected array $nodes;
 
     /**
-     * @param array<Node|array> $nodes
+     * @param list<Node|array<string, mixed>> $nodes
      *
-     * @phpstan-param array<T|array<string, mixed>> $nodes
+     * @phpstan-param list<T|array<string, mixed>> $nodes
      */
     public function __construct(array $nodes)
     {
         $this->nodes = $nodes;
     }
 
-    /**
-     * @param int|string $offset
-     */
-    #[\ReturnTypeWillChange]
-    public function offsetExists($offset): bool
+    public function has(int $offset): bool
     {
         return isset($this->nodes[$offset]);
     }
 
     /**
-     * @param int|string $offset
-     *
      * @phpstan-return T
      */
-    #[\ReturnTypeWillChange]
-    public function offsetGet($offset): Node
+    public function get(int $offset): Node
     {
-        $item = $this->nodes[$offset];
+        $node = $this->nodes[$offset];
 
-        if (\is_array($item)) {
-            // @phpstan-ignore-next-line not really possible to express the correctness of this in PHP
-            return $this->nodes[$offset] = AST::fromArray($item);
-        }
-
-        return $item;
+        // @phpstan-ignore-next-line not really possible to express the correctness of this in PHP
+        return \is_array($node)
+            ? ($this->nodes[$offset] = AST::fromArray($node))
+            : $node;
     }
 
     /**
-     * @param int|string|null           $offset
-     * @param Node|array<string, mixed> $value
-     *
-     * @phpstan-param T|array<string, mixed> $value
+     * @phpstan-param T $value
      */
-    #[\ReturnTypeWillChange]
-    public function offsetSet($offset, $value): void
+    public function set(int $offset, Node $value): void
     {
-        if (\is_array($value)) {
-            /** @phpstan-var T $value */
-            $value = AST::fromArray($value);
-        }
-
-        // Happens when a Node is pushed via []=
-        if ($offset === null) {
-            $this->nodes[] = $value;
-
-            return;
-        }
-
         $this->nodes[$offset] = $value;
+
+        assert($this->nodes === \array_values($this->nodes));
     }
 
     /**
-     * @param int|string $offset
+     * @phpstan-param T $value
      */
-    #[\ReturnTypeWillChange]
-    public function offsetUnset($offset): void
+    public function add(Node $value): void
+    {
+        $this->nodes[] = $value;
+    }
+
+    public function unset(int $offset): void
     {
         unset($this->nodes[$offset]);
+        $this->nodes = \array_values($this->nodes);
     }
 
     public function getIterator(): \Traversable
     {
         foreach ($this->nodes as $key => $_) {
-            yield $key => $this->offsetGet($key);
+            yield $key => $this->get($key);
         }
     }
 
@@ -104,19 +85,22 @@ public function count(): int
     /**
      * Remove a portion of the NodeList and replace it with something else.
      *
-     * @param T|array<T>|null $replacement
+     * @param Node|array<Node>|null $replacement
+     *
+     * @phpstan-param T|array<T>|null $replacement
      *
      * @phpstan-return NodeList<T> the NodeList with the extracted elements
      */
     public function splice(int $offset, int $length, $replacement = null): NodeList
     {
-        return new NodeList(
-            \array_splice($this->nodes, $offset, $length, $replacement)
-        );
+        $spliced = \array_splice($this->nodes, $offset, $length, $replacement);
+
+        // @phpstan-ignore-next-line generic type mismatch
+        return new NodeList($spliced);
     }
 
     /**
-     * @phpstan-param iterable<array-key, T> $list
+     * @phpstan-param iterable<int, T> $list
      *
      * @phpstan-return NodeList<T>
      */
@@ -126,15 +110,10 @@ public function merge(iterable $list): NodeList
             $list = \iterator_to_array($list);
         }
 
-        return new NodeList(\array_merge($this->nodes, $list));
-    }
+        $merged = \array_merge($this->nodes, $list);
 
-    /**
-     * Resets the keys of the stored nodes to contiguous numeric indexes.
-     */
-    public function reindex(): void
-    {
-        $this->nodes = array_values($this->nodes);
+        // @phpstan-ignore-next-line generic type mismatch
+        return new NodeList($merged);
     }
 
     /**
@@ -146,8 +125,8 @@ public function cloneDeep(): self
     {
         /** @var static<T> $cloned */
         $cloned = new static([]);
-        foreach ($this->getIterator() as $key => $node) {
-            $cloned[$key] = $node->cloneDeep();
+        foreach ($this->getIterator() as $node) {
+            $cloned->add($node->cloneDeep());
         }
 
         return $cloned;

src/Language/Visitor.php

@@ -224,7 +224,7 @@ public static function visit(object $root, array $visitor, ?array $keyMap = null
                                 throw new \Exception("Can only add Node to NodeList, got: {$notNode}.");
                             }
 
-                            $node[$editKey] = $editValue;
+                            $node->set($editKey, $editValue);
                         } else {
                             $node->{$editKey} = $editValue;
                         }
@@ -246,7 +246,7 @@ public static function visit(object $root, array $visitor, ?array $keyMap = null
                         ? $index
                         : $keys[$index];
                     $node = $parent instanceof NodeList
-                        ? $parent[$key]
+                        ? $parent->get($key)
                         : $parent->{$key};
                 }
                 if ($node === null) {

src/Validator/Rules/QueryComplexity.php

@@ -61,7 +61,7 @@ public function getVisitor(QueryValidationContext $context): array
                     );
                 },
                 NodeKind::VARIABLE_DEFINITION => function ($def): VisitorOperation {
-                    $this->variableDefs[] = $def;
+                    $this->variableDefs->add($def);
 
                     return Visitor::skipNode();
                 },

tests/Error/ErrorTest.php

@@ -34,8 +34,8 @@ public function testConvertsNodesToPositionsAndLocations(): void
     }');
         $ast = Parser::parse($source);
         /** @var OperationDefinitionNode $operationDefinition */
-        $operationDefinition = $ast->definitions[0];
-        $fieldNode = $operationDefinition->selectionSet->selections[0];
+        $operationDefinition = $ast->definitions->get(0);
+        $fieldNode = $operationDefinition->selectionSet->selections->get(0);
         $e = new Error('msg', [$fieldNode]);
 
         self::assertEquals([$fieldNode], $e->getNodes());
@@ -54,8 +54,8 @@ public function testConvertSingleNodeToPositionsAndLocations(): void
     }');
         $ast = Parser::parse($source);
         /** @var OperationDefinitionNode $operationDefinition */
-        $operationDefinition = $ast->definitions[0];
-        $fieldNode = $operationDefinition->selectionSet->selections[0];
+        $operationDefinition = $ast->definitions->get(0);
+        $fieldNode = $operationDefinition->selectionSet->selections->get(0);
         $e = new Error('msg', $fieldNode); // Non-array value.
 
         self::assertEquals([$fieldNode], $e->getNodes());
@@ -73,7 +73,7 @@ public function testConvertsNodeWithStart0ToPositionsAndLocations(): void
       field
     }');
         $ast = Parser::parse($source);
-        $operationNode = $ast->definitions[0];
+        $operationNode = $ast->definitions->get(0);
         $e = new Error('msg', [$operationNode]);
 
         self::assertEquals([$operationNode], $e->getNodes());
@@ -114,8 +114,8 @@ public function testSerializesToIncludeMessageAndLocations(): void
     {
         $ast = Parser::parse('{ field }');
         /** @var OperationDefinitionNode $operationDefinition */
-        $operationDefinition = $ast->definitions[0];
-        $node = $operationDefinition->selectionSet->selections[0];
+        $operationDefinition = $ast->definitions->get(0);
+        $node = $operationDefinition->selectionSet->selections->get(0);
         $e = new Error('msg', [$node]);
 
         self::assertEquals(

tests/Error/PrintErrorTest.php

@@ -63,8 +63,8 @@ public function testPrintsAnErrorWithNodesFromDifferentSources(): void
         ));
 
         /** @var ObjectTypeDefinitionNode $objectDefinitionA */
-        $objectDefinitionA = $sourceA->definitions[0];
-        $fieldTypeA = $objectDefinitionA->fields[0]->type;
+        $objectDefinitionA = $sourceA->definitions->get(0);
+        $fieldTypeA = $objectDefinitionA->fields->get(0)->type;
 
         $sourceB = Parser::parse(new Source(
             'type Foo {
@@ -74,8 +74,8 @@ public function testPrintsAnErrorWithNodesFromDifferentSources(): void
         ));
 
         /** @var ObjectTypeDefinitionNode $objectDefinitionB */
-        $objectDefinitionB = $sourceB->definitions[0];
-        $fieldTypeB = $objectDefinitionB->fields[0]->type;
+        $objectDefinitionB = $sourceB->definitions->get(0);
+        $fieldTypeB = $objectDefinitionB->fields->get(0)->type;
 
         $error = new Error(
             'Example error with two nodes',

tests/Executor/ExecutorTest.php

@@ -257,11 +257,11 @@ public function testProvidesInfoAboutCurrentExecutionState(): void
         Executor::execute($schema, $ast, $rootValue, null, ['var' => '123']);
 
         /** @var OperationDefinitionNode $operationDefinition */
-        $operationDefinition = $ast->definitions[0];
+        $operationDefinition = $ast->definitions->get(0);
 
         self::assertEquals('test', $info->fieldName);
         self::assertCount(1, $info->fieldNodes);
-        self::assertSame($operationDefinition->selectionSet->selections[0], $info->fieldNodes[0]);
+        self::assertSame($operationDefinition->selectionSet->selections->get(0), $info->fieldNodes[0]);
         self::assertSame(Type::string(), $info->returnType);
         self::assertSame($schema->getQueryType(), $info->parentType);
         self::assertEquals(['result'], $info->path);

tests/Language/AST/NodeTest.php

@@ -30,16 +30,16 @@ public function testCloneDeep(): void
         $cloneFields = $clone->fields;
         self::assertNotSameButEquals($nodeFields, $cloneFields);
 
-        $nodeId = $nodeFields[0];
-        $cloneId = $cloneFields[0];
+        $nodeId = $nodeFields->get(0);
+        $cloneId = $cloneFields->get(0);
         self::assertNotSameButEquals($nodeId, $cloneId);
 
         $nodeIdArgs = $nodeId->arguments;
         $cloneIdArgs = $cloneId->arguments;
         self::assertNotSameButEquals($nodeIdArgs, $cloneIdArgs);
 
-        $nodeArg = $nodeIdArgs[0];
-        $cloneArg = $cloneIdArgs[0];
+        $nodeArg = $nodeIdArgs->get(0);
+        $cloneArg = $cloneIdArgs->get(0);
         self::assertNotSameButEquals($nodeArg, $cloneArg);
 
         self::assertSame($node->loc, $clone->loc);