webonyx · spawnia · Feb 21, 2023 · Feb 15, 2023
diff --git a/src/Language/AST/NodeList.php b/src/Language/AST/NodeList.php
@@ -7,93 +7,91 @@
 /**
  * @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 array<Node>
      *
-     * @phpstan-var array<T|array<string, mixed>>
+     * @phpstan-var list<T>
      */
-    private array $nodes;
+    private array $nodes = [];
 
     /**
-     * @param array<Node|array> $nodes
+     * @param iterable<Node|array> $nodes
      *
-     * @phpstan-param array<T|array<string, mixed>> $nodes
+     * @phpstan-param iterable<T|array<string, mixed>> $nodes
      */
-    public function __construct(array $nodes)
+    public function __construct(iterable $nodes)
     {
-        $this->nodes = $nodes;
+        foreach ($nodes as $node) {
+            $this->nodes[] = $this->process($node);
+        }
     }
 
-    /**
-     * @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];
+        return $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);
-        }
+    /**
+     * @phpstan-param T $value
+     */
+    public function set(int $offset, Node $value): void
+    {
+        $this->nodes[$offset] = $value;
+
+        assert($this->nodes === \array_values($this->nodes));
+    }
 
-        return $item;
+    /**
+     * @phpstan-param T $value
+     */
+    public function add(Node $value): void
+    {
+        $this->nodes[] = $value;
     }
 
     /**
-     * @param int|string|null           $offset
      * @param Node|array<string, mixed> $value
      *
      * @phpstan-param T|array<string, mixed> $value
+     *
+     * @phpstan-return T
      */
-    #[\ReturnTypeWillChange]
-    public function offsetSet($offset, $value): void
+    private function process($value): Node
     {
         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;
+        return $value;
     }
 
-    /**
-     * @param int|string $offset
-     */
-    #[\ReturnTypeWillChange]
-    public function offsetUnset($offset): void
+    public function remove(Node $node): void
     {
-        unset($this->nodes[$offset]);
+        $foundKey = \array_search($node, $this->nodes, true);
+        if ($foundKey === false) {
+            throw new \InvalidArgumentException('Node not found in NodeList');
+        }
+
+        unset($this->nodes[$foundKey]);
+        $this->nodes = \array_values($this->nodes);
     }
 
     public function getIterator(): \Traversable
     {
-        foreach ($this->nodes as $key => $_) {
-            yield $key => $this->offsetGet($key);
-        }
+        yield from $this->nodes;
     }
 
     public function count(): int
@@ -116,7 +114,7 @@ public function splice(int $offset, int $length, $replacement = null): NodeList
     }
 
     /**
-     * @phpstan-param iterable<array-key, T> $list
+     * @phpstan-param iterable<int, T> $list
      *
      * @phpstan-return NodeList<T>
      */
@@ -129,14 +127,6 @@ public function merge(iterable $list): NodeList
         return new NodeList(\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);
-    }
-
     /**
      * Returns a clone of this instance and all its children, except Location $loc.
      *
@@ -146,8 +136,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;

diff --git a/src/Language/Visitor.php b/src/Language/Visitor.php
@@ -218,17 +218,15 @@ public static function visit(object $root, array $visitor, ?array $keyMap = null
                             assert($node instanceof NodeList, 'Follows from $inArray');
                             $node->splice($editKey, 1);
                             ++$editOffset;
-                        } else {
-                            if ($node instanceof NodeList || \is_array($node)) {
-                                if (! $editValue instanceof Node) {
-                                    $notNode = Utils::printSafe($editValue);
-                                    throw new \Exception("Can only add Node to NodeList, got: {$notNode}.");
-                                }
-
-                                $node[$editKey] = $editValue;
-                            } else {
-                                $node->{$editKey} = $editValue;
+                        } elseif ($node instanceof NodeList) {
+                            if (! $editValue instanceof Node) {
+                                $notNode = Utils::printSafe($editValue);
+                                throw new \Exception("Can only add Node to NodeList, got: {$notNode}.");
                             }
+
+                            $node->set($editKey, $editValue);
+                        } else {
+                            $node->{$editKey} = $editValue;
                         }
                     }
                 }
@@ -250,8 +248,8 @@ public static function visit(object $root, array $visitor, ?array $keyMap = null
                     : null;
                 $node = $parent !== null
                     ? (
-                        $parent instanceof NodeList || \is_array($parent)
-                            ? $parent[$key]
+                        $parent instanceof NodeList
+                            ? $parent->get($key)
                             : $parent->{$key}
                     )
                     : $newRoot;
@@ -265,7 +263,7 @@ public static function visit(object $root, array $visitor, ?array $keyMap = null
             }
 
             $result = null;
-            if (! $node instanceof NodeList && ! \is_array($node)) {
+            if (! $node instanceof NodeList) {
                 if (! ($node instanceof Node)) {
                     throw new \Exception('Invalid AST Node: ' . \json_encode($node));
                 }
@@ -317,7 +315,7 @@ public static function visit(object $root, array $visitor, ?array $keyMap = null
                     'keys' => $keys,
                     'edits' => $edits,
                 ];
-                $inArray = $node instanceof NodeList || \is_array($node);
+                $inArray = $node instanceof NodeList;
 
                 $keys = ($inArray ? $node : $visitorKeys[$node->kind]) ?? [];
                 $index = -1;

diff --git a/src/Validator/Rules/QueryComplexity.php b/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();
                 },

diff --git a/tests/Error/ErrorTest.php b/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(

diff --git a/tests/Error/PrintErrorTest.php b/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',

diff --git a/tests/Executor/ExecutorTest.php b/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);

diff --git a/tests/Language/AST/NodeTest.php b/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);