ListTag: use array instead of SplDoublyLinkedList

src/tag/ListTag.php

@@ -28,11 +28,17 @@
 use pocketmine\nbt\NbtStreamReader;
 use pocketmine\nbt\NbtStreamWriter;
 use pocketmine\nbt\ReaderTracker;
-use function assert;
+use function array_key_last;
+use function array_map;
+use function array_pop;
+use function array_push;
+use function array_shift;
+use function array_slice;
+use function array_unshift;
+use function array_values;
 use function count;
 use function func_num_args;
 use function get_class;
-use function iterator_to_array;
 use function str_repeat;
 
 /**
@@ -44,20 +50,19 @@ final class ListTag extends Tag implements \Countable, \IteratorAggregate{
 	/** @var int */
 	private $tagType;
 	/**
-	 * @var \SplDoublyLinkedList|Tag[]
-	 * @phpstan-var \SplDoublyLinkedList<Tag>
+	 * @var Tag[]
+	 * @phpstan-var list<Tag>
 	 */
-	private $value;
+	private $value = [];
 
 	/**
 	 * @param Tag[] $value
 	 */
 	public function __construct(array $value = [], int $tagType = NBT::TAG_End){
 		self::restrictArgCount(__METHOD__, func_num_args(), 2);
 		$this->tagType = $tagType;
-		$this->value = new \SplDoublyLinkedList();
 		foreach($value as $tag){
-			$this->push($tag);
+			$this->push($tag); //ensure types get checked
 		}
 	}
 
@@ -66,12 +71,7 @@ public function __construct(array $value = [], int $tagType = NBT::TAG_End){
 	 * @phpstan-return list<Tag>
 	 */
 	public function getValue() : array{
-		$value = [];
-		foreach($this->value as $v){
-			$value[] = $v;
-		}
-
-		return $value;
+		return $this->value;
 	}
 
 	/**
@@ -80,50 +80,51 @@ public function getValue() : array{
 	 * @phpstan-return list<mixed>
 	 */
 	public function getAllValues() : array{
-		$result = [];
-		foreach($this->value as $tag){
-			$result[] = $tag->getValue();
-		}
-
-		return $result;
+		return array_map(fn(Tag $t) => $t->getValue(), $this->value);
 	}
 
 	public function count() : int{
-		return $this->value->count();
+		return count($this->value);
 	}
 
 	public function getCount() : int{
-		return $this->value->count();
+		return count($this->value);
 	}
 
 	/**
 	 * Appends the specified tag to the end of the list.
 	 */
 	public function push(Tag $tag) : void{
 		$this->checkTagType($tag);
-		$this->value->push($tag);
+		$this->value[] = $tag;
 	}
 
 	/**
 	 * Removes the last tag from the list and returns it.
 	 */
 	public function pop() : Tag{
-		return $this->value->pop();
+		if(count($this->value) === 0){
+			throw new \LogicException("List is empty");
+		}
+		return array_pop($this->value);
 	}
 
 	/**
 	 * Adds the specified tag to the start of the list.
 	 */
 	public function unshift(Tag $tag) : void{
 		$this->checkTagType($tag);
-		$this->value->unshift($tag);
+		array_unshift($this->value, $tag);
 	}
 
 	/**
 	 * Removes the first tag from the list and returns it.
 	 */
 	public function shift() : Tag{
-		return $this->value->shift();
+		if(count($this->value) === 0){
+			throw new \LogicException("List is empty");
+		}
+		return array_shift($this->value);
 	}
 
 	/**
@@ -135,14 +136,23 @@ public function shift() : Tag{
 	 */
 	public function insert(int $offset, Tag $tag){
 		$this->checkTagType($tag);
-		$this->value->add($offset, $tag);
+		if($offset < 0 || $offset > count($this->value)){
+			throw new \OutOfRangeException("Offset cannot be negative or larger than the list's current size");
+		}
+		$newValue = array_slice($this->value, 0, $offset);
+		$newValue[] = $tag;
+		array_push($newValue, ...array_slice($this->value, $offset));
+		$this->value = $newValue;
 	}
 
 	/**
 	 * Removes a value from the list. All later tags in the list are moved down by 1 position.
 	 */
 	public function remove(int $offset) : void{
-		unset($this->value[$offset]);
+		//to keep phpstan happy we can't directly unset from $this->value
+		$newValue = $this->value;
+		unset($newValue[$offset]);
+		$this->value = array_values($newValue);
 	}
 
 	/**
@@ -161,14 +171,20 @@ public function get(int $offset) : Tag{
 	 * Returns the element in the first position of the list, without removing it.
 	 */
 	public function first() : Tag{
-		return $this->value->bottom();
+		if(count($this->value) === 0){
+			throw new \LogicException("List is empty");
+		}
+		return $this->value[0];
 	}
 
 	/**
 	 * Returns the element in the last position in the list (the end), without removing it.
 	 */
 	public function last() : Tag{
-		return $this->value->top();
+		if(count($this->value) === 0){
+			throw new \LogicException("List is empty");
+		}
+		return $this->value[array_key_last($this->value)];
 	}
 
 	/**
@@ -178,6 +194,9 @@ public function last() : Tag{
 	 */
 	public function set(int $offset, Tag $tag) : void{
 		$this->checkTagType($tag);
+		if($offset < 0 || $offset > count($this->value)){ //allow setting the end offset
+			throw new \OutOfRangeException("Offset cannot be negative or larger than the list's current size");
+		}
 		$this->value[$offset] = $tag;
 	}
 
@@ -192,7 +211,7 @@ public function isset(int $offset) : bool{
 	 * Returns whether there are any tags in the list.
 	 */
 	public function empty() : bool{
-		return $this->value->isEmpty();
+		return count($this->value) === 0;
 	}
 
 	protected function getTypeName() : string{
@@ -218,7 +237,7 @@ public function getTagType() : int{
 	 * @throws \LogicException if the list is not empty
 	 */
 	public function setTagType(int $type){
-		if(!$this->value->isEmpty()){
+		if(count($this->value) > 0){
 			throw new \LogicException("Cannot change tag type of non-empty ListTag");
 		}
 		$this->tagType = $type;
@@ -264,31 +283,22 @@ public static function read(NbtStreamReader $reader, ReaderTracker $tracker) : s
 
 	public function write(NbtStreamWriter $writer) : void{
 		$writer->writeByte($this->tagType);
-		$writer->writeInt($this->value->count());
-		/** @var Tag $tag */
+		$writer->writeInt(count($this->value));
 		foreach($this->value as $tag){
 			$tag->write($writer);
 		}
 	}
 
 	protected function stringifyValue(int $indentation) : string{
 		$str = "{\n";
-		/** @var Tag $tag */
 		foreach($this->value as $tag){
 			$str .= str_repeat("  ", $indentation + 1) . $tag->toString($indentation + 1) . "\n";
 		}
 		return $str . str_repeat("  ", $indentation) . "}";
 	}
 
 	public function __clone(){
-		/** @phpstan-var \SplDoublyLinkedList<Tag> $new */
-		$new = new \SplDoublyLinkedList();
-
-		foreach($this->value as $tag){
-			$new->push($tag->safeClone());
-		}
-
-		$this->value = $new;
+		$this->value = array_map(fn(Tag $t) => $t->safeClone(), $this->value);
 	}
 
 	protected function makeCopy(){
@@ -300,20 +310,16 @@ protected function makeCopy(){
 	 * @phpstan-return \Generator<int, Tag, void, void>
 	 */
 	public function getIterator() : \Generator{
-		//we technically don't need iterator_to_array() here, but I don't feel comfortable relying on "yield from" to
-		//copy the underlying dataset referenced by SplDoublyLinkedList
-		yield from iterator_to_array($this->value, true);
+		yield from $this->value;
 	}
 
 	public function equals(Tag $that) : bool{
 		if(!($that instanceof $this) or count($this->value) !== count($that->value)){
 			return false;
 		}
 
-		$thatValues = $that->getValue(); //SplDoublyLinkedList has O(n) random access, so this is faster than repeated get() calls
 		foreach($this->value as $k => $v){
-			assert(isset($thatValues[$k]), "We checked the count above, so this should not be missing");
-			if(!$v->equals($thatValues[$k])){
+			if(!$v->equals($that->value[$k])){
 				return false;
 			}
 		}

tests/phpstan/configs/phpstan-bugs.neon

@@ -11,3 +11,9 @@ parameters:
 			identifier: arrayValues.list
 			count: 1
 			path: ../../../src/tag/IntArrayTag.php
+
+		-
+			message: '#^Property pocketmine\\nbt\\tag\\ListTag\:\:\$value \(list\<pocketmine\\nbt\\tag\\Tag\>\) does not accept non\-empty\-array\<int\<0, max\>, pocketmine\\nbt\\tag\\Tag\>\.$#'
+			identifier: assign.propertyType
+			count: 1
+			path: ../../../src/tag/ListTag.php

tests/phpunit/tag/ListTagTest.php

@@ -27,6 +27,7 @@
 use pocketmine\nbt\NBT;
 use function array_fill;
 use function array_key_first;
+use function array_keys;
 use function array_map;
 
 class ListTagTest extends TestCase{
@@ -154,4 +155,25 @@ public function testModificationDuringIteration() : void{
 		//if we iterated by-ref, entries are likely to have been skipped
 		self::assertCount(0, $tag);
 	}
+
+	public function testInsert() : void{
+		$list = new ListTag();
+		$list->push(new IntTag(0));
+
+		$list->insert(1, new IntTag(2));
+		$list->insert(1, new IntTag(1)); //displaces int(2)
+
+		self::assertSame([0, 1, 2], $list->getAllValues());
+		self::assertSame([0, 1, 2], array_keys($list->getValue()), "Key order should be consecutive");
+	}
+
+	public function testDelete() : void{
+		$list = new ListTag();
+		foreach(range(0, 2) as $value){
+			$list->push(new IntTag($value));
+		}
+		$list->remove(1);
+		self::assertSame([0, 2], $list->getAllValues());
+		self::assertSame([0, 1], array_keys($list->getValue()));
+	}
 }