Revert NodeList changes

Too much breakage for now. Projects such as sailor or lighthouse use AST manipulation extensively.
webonyx · Feb 22, 2023 · e20673b · e20673b
1 parent f042584
commit e20673b
Show file tree

Hide file tree

Showing 13 changed files with 116 additions and 93 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,10 +9,6 @@ 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

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

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->add($def);
+                    $this->variableDefs[] = $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->get(0);
-        $fieldNode = $operationDefinition->selectionSet->selections->get(0);
+        $operationDefinition = $ast->definitions[0];
+        $fieldNode = $operationDefinition->selectionSet->selections[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->get(0);
-        $fieldNode = $operationDefinition->selectionSet->selections->get(0);
+        $operationDefinition = $ast->definitions[0];
+        $fieldNode = $operationDefinition->selectionSet->selections[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->get(0);
+        $operationNode = $ast->definitions[0];
         $e = new Error('msg', [$operationNode]);
 
         self::assertEquals([$operationNode], $e->getNodes());

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->get(0);
-        $fieldTypeA = $objectDefinitionA->fields->get(0)->type;
+        $objectDefinitionA = $sourceA->definitions[0];
+        $fieldTypeA = $objectDefinitionA->fields[0]->type;
 
         $sourceB = Parser::parse(new Source(
             'type Foo {
@@ -74,8 +74,8 @@ public function testPrintsAnErrorWithNodesFromDifferentSources(): void
         ));
 
         /** @var ObjectTypeDefinitionNode $objectDefinitionB */
-        $objectDefinitionB = $sourceB->definitions->get(0);
-        $fieldTypeB = $objectDefinitionB->fields->get(0)->type;
+        $objectDefinitionB = $sourceB->definitions[0];
+        $fieldTypeB = $objectDefinitionB->fields[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->get(0);
+        $operationDefinition = $ast->definitions[0];
 
         self::assertEquals('test', $info->fieldName);
         self::assertCount(1, $info->fieldNodes);
-        self::assertSame($operationDefinition->selectionSet->selections->get(0), $info->fieldNodes[0]);
+        self::assertSame($operationDefinition->selectionSet->selections[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->get(0);
-        $cloneId = $cloneFields->get(0);
+        $nodeId = $nodeFields[0];
+        $cloneId = $cloneFields[0];
         self::assertNotSameButEquals($nodeId, $cloneId);
 
         $nodeIdArgs = $nodeId->arguments;
         $cloneIdArgs = $cloneId->arguments;
         self::assertNotSameButEquals($nodeIdArgs, $cloneIdArgs);
 
-        $nodeArg = $nodeIdArgs->get(0);
-        $cloneArg = $cloneIdArgs->get(0);
+        $nodeArg = $nodeIdArgs[0];
+        $cloneArg = $cloneIdArgs[0];
         self::assertNotSameButEquals($nodeArg, $cloneArg);
 
         self::assertSame($node->loc, $clone->loc);

diff --git a/tests/Language/NodeListTest.php b/tests/Language/NodeListTest.php
@@ -13,24 +13,25 @@ public function testConvertArrayToASTNode(): void
     {
         $nameNode = new NameNode(['value' => 'bar']);
 
-        $nodeList = new NodeList([$nameNode->toArray()]);
+        $key = 'foo';
+        $nodeList = new NodeList([$key => $nameNode->toArray()]);
 
-        self::assertEquals($nameNode, $nodeList->get(0));
+        self::assertEquals($nameNode, $nodeList[$key]);
     }
 
     public function testCloneDeep(): void
     {
         $nameNode = new NameNode(['value' => 'bar']);
 
         $nodeList = new NodeList([
-            $nameNode->toArray(),
-            $nameNode,
+            'array' => $nameNode->toArray(),
+            'instance' => $nameNode,
         ]);
 
         $cloned = $nodeList->cloneDeep();
 
-        self::assertNotSameButEquals($nodeList->get(0), $cloned->get(0));
-        self::assertNotSameButEquals($nodeList->get(1), $cloned->get(1));
+        self::assertNotSameButEquals($nodeList['array'], $cloned['array']);
+        self::assertNotSameButEquals($nodeList['instance'], $cloned['instance']);
     }
 
     private static function assertNotSameButEquals(object $node, object $clone): void
@@ -41,35 +42,42 @@ private static function assertNotSameButEquals(object $node, object $clone): voi
 
     public function testThrowsOnInvalidArrays(): void
     {
-        // @phpstan-ignore-next-line Wrong on purpose
-        $nodeList = new NodeList([['not a valid array representation of an AST node']]);
+        $nodeList = new NodeList([]);
 
-        $this->expectException(InvariantViolation::class);
-        iterator_to_array($nodeList);
+        self::expectException(InvariantViolation::class);
+        // @phpstan-ignore-next-line Wrong on purpose
+        $nodeList[] = ['not a valid array representation of an AST node'];
     }
 
-    public function testAddNodes(): void
+    public function testPushNodes(): void
     {
         /** @var NodeList<NameNode> $nodeList */
         $nodeList = new NodeList([]);
         self::assertCount(0, $nodeList);
 
-        $nodeList->add(new NameNode(['value' => 'foo']));
+        $nodeList[] = new NameNode(['value' => 'foo']);
         self::assertCount(1, $nodeList);
 
-        $nodeList->add(new NameNode(['value' => 'bar']));
+        $nodeList[] = new NameNode(['value' => 'bar']);
         self::assertCount(2, $nodeList);
     }
 
-    public function testUnsetDoesNotBreakList(): void
+    public function testResetContiguousNumericIndexAfterUnset(): void
     {
         $foo = new NameNode(['value' => 'foo']);
         $bar = new NameNode(['value' => 'bar']);
 
         $nodeList = new NodeList([$foo, $bar]);
-        $nodeList->unset(0);
+        unset($nodeList[0]);
+
+        self::assertArrayNotHasKey(0, $nodeList);
+        // @phpstan-ignore-next-line just wrong
+        self::assertArrayHasKey(1, $nodeList);
+
+        $nodeList->reindex();
 
-        self::assertTrue($nodeList->has(0));
-        self::assertSame($bar, $nodeList->get(0));
+        // @phpstan-ignore-next-line just wrong
+        self::assertArrayHasKey(0, $nodeList);
+        self::assertSame($bar, $nodeList[0]);
     }
 }
diff --git a/tests/Language/ParserTest.php b/tests/Language/ParserTest.php
@@ -220,7 +220,7 @@ public function testParsesMultiByteCharacters(): void
         ]);
 
         /** @var OperationDefinitionNode $operationDefinition */
-        $operationDefinition = $result->definitions->get(0);
+        $operationDefinition = $result->definitions[0];
         self::assertEquals($expected, $operationDefinition->selectionSet);
     }
 

diff --git a/tests/Language/SerializationTest.php b/tests/Language/SerializationTest.php
@@ -58,8 +58,8 @@ private static function assertNodesAreEqual(Node $expected, Node $actual, array
 
                 foreach ($expectedValue as $index => $listNode) {
                     $tmpPath2 = $tmpPath;
-                    $tmpPath2[] = (string) $index;
-                    self::assertNodesAreEqual($listNode, $actualValue->get($index), $tmpPath2);
+                    $tmpPath2[] = $index;
+                    self::assertNodesAreEqual($listNode, $actualValue[$index], $tmpPath2);
                 }
             } elseif ($expectedValue instanceof Location) {
                 self::assertInstanceOf(Location::class, $actualValue, $err);