ListTag: use array instead of SplDoublyLinkedList (#128)

the idea of using linked-list for this was to allow dynamic insertions and deletions midway through the list.

however, this is basically never done in practice, and because SplDoublyLinkedList has O(n) read performance,
we pay a read performance penalty for these write features which are basically never used anyway.

in the future, we should perhaps consider getting rid of the mutation APIs from ListTag, and require creating
a new one to replace it in mutation cases, similar to other tags (aside from TAG_Compound).

Author: dktapps

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()));
+	}
 }