Change iteration behavior to be relative to original position

TysonAndre · TysonAndre · commit 5295b23833a8 · 2022-01-08T14:41:01.000-05:00
Make Deque iteration account for calls to shift/unshift tracking the position of the front of the Deque.
Calls to shift/unshift will do the following:
- Increase/Decrease the value returned by the iterator's key()
  by the number of elements added/removed to/from the front of the Deque.
  (`$deque[$iteratorKey] === $iteratorValue` at the time the key and value are returned).
- Repeated calls to shift will cause valid() to return false if the iterator's
  position ends up before the start of the Deque at the time iteration resumes.
- They will not cause the remaining values to be iterated over more than once or skipped.
diff --git a/ext/spl/spl_deque.c b/ext/spl/spl_deque.c
@@ -71,10 +71,19 @@ zend_class_entry *spl_ce_Deque;
 static const zval empty_entry_list[1];
 
 typedef struct _spl_deque_entries {
+	/* The number of elements in the Deque. */
 	size_t size;
 	/* One less than a power of two (the capacity) */
 	size_t mask;
+	/* The offset of the start of the deque in the circular buffer. */
 	size_t offset;
+	/*
+	 * This is a counter which increases when adding elements to the front and decreases when removing elements from the front.
+	 * This is used so that iteration works as expected when the front position is modified during foreach.
+	 *
+	 * This is a uint64_t to work consistently and avoid easily overflowing 4 billion on 32-bit builds.
+	 */
+	uint64_t iteration_offset;
 	/** This is a circular buffer with an offset, size, and capacity(mask + 1) */
 	zval *circular_buffer;
 } spl_deque_entries;
@@ -116,7 +125,8 @@ typedef struct _spl_deque {
 /* Used by InternalIterator returned by Deque->getIterator() */
 typedef struct _spl_deque_it {
 	zend_object_iterator intern;
-	zend_long            current;
+	/* This starts at iteration_offset and increases by one when next() is called. */
+	uint64_t current;
 } spl_deque_it;
 
 static zend_always_inline void spl_deque_push_back(spl_deque *intern, zval *value);
@@ -153,29 +163,43 @@ static bool spl_deque_entries_uninitialized(spl_deque_entries *array)
 	return array->circular_buffer == NULL;
 }
 
-/* Based n zend_hash_check_size. Finds the next largest power of 2. */
+/* Based on zend_hash_check_size which supports 32-bit integers. Finds the next largest power of 2. */
 static inline size_t spl_deque_next_pow2_capacity(size_t nSize) {
 	if (nSize < DEQUE_MIN_CAPACITY) {
 		return DEQUE_MIN_CAPACITY;
 	}
+	/* Note that for values such as 63 or 31 of the form ((2^n) - 1),
+	 * subtracting and xor are the same things for numbers in the range of 0 to the max. */
 #ifdef ZEND_WIN32
 	unsigned long index;
+#if SIZEOF_SIZE_T > 4
+	if (BitScanReverse64(&index, nSize - 1)) {
+		return 0x2u << ((63 - index) ^ 0x3f);
+	}
+#else
 	if (BitScanReverse(&index, nSize - 1)) {
 		return 0x2u << ((31 - index) ^ 0x1f);
-	} else {
-		/* nSize is ensured to be in the valid range, fall back to it
-		   rather than using an undefined bis scan result. */
-		return nSize;
 	}
+#endif
+	/* nSize is ensured to be in the valid range, fall back to it
+	 * rather than using an undefined bit scan result. */
+	return nSize;
 #elif (defined(__GNUC__) || __has_builtin(__builtin_clz))  && defined(PHP_HAVE_BUILTIN_CLZ)
+#if SIZEOF_SIZE_T > SIZEOF_INT
+	return 0x2u << (__builtin_clzl(nSize - 1) ^ (sizeof(long) * 8 - 1));
+#else
 	return 0x2u << (__builtin_clz(nSize - 1) ^ 0x1f);
+#endif
 #else
 	nSize -= 1;
 	nSize |= (nSize >> 1);
 	nSize |= (nSize >> 2);
 	nSize |= (nSize >> 4);
 	nSize |= (nSize >> 8);
 	nSize |= (nSize >> 16);
+#if SIZEOF_SIZE_T > 4
+	nSize |= (nSize >> 32);
+#endif
 	return nSize + 1;
 #endif
 }
@@ -184,6 +208,7 @@ static void spl_deque_entries_init_from_array(spl_deque_entries *array, zend_arr
 {
 	zend_long size = zend_hash_num_elements(values);
 	array->offset = 0;
+	array->iteration_offset = 0;
 	array->size = 0; /* reset size and capacity in case emalloc() fails */
 	array->mask = 0;
 	if (size > 0) {
@@ -212,6 +237,7 @@ static void spl_deque_entries_init_from_traversable(spl_deque_entries *array, ze
 	zend_long size = 0, capacity = 0;
 	array->size = 0;
 	array->offset = 0;
+	array->iteration_offset = 0;
 	array->circular_buffer = NULL;
 	zval *circular_buffer = NULL;
 	zval tmp_obj;
@@ -274,6 +300,7 @@ static void spl_deque_entries_copy_ctor(spl_deque_entries *to, const spl_deque_e
 	to->size = 0; /* reset size in case emalloc() fails */
 	to->mask = 0;
 	to->offset = 0;
+	to->iteration_offset = 0;
 	if (!size) {
 		to->circular_buffer = (zval *)empty_entry_list;
 		return;
@@ -321,6 +348,7 @@ static void spl_deque_entries_dtor(spl_deque_entries *array)
 		array->offset = 0;
 		array->size = 0;
 		array->mask = 0;
+		/* Changing iteration_offset doesn't matter */
 		do {
 			if (p == end) {
 				p = circular_buffer;
@@ -504,6 +532,7 @@ PHP_METHOD(Deque, __construct)
 
 	if (iterable == NULL) {
 		intern->array.offset = 0;
+		intern->array.iteration_offset = 0;
 		intern->array.size = 0;
 		intern->array.mask = 0;
 		intern->array.circular_buffer = (zval *)empty_entry_list;
@@ -535,15 +564,17 @@ static void spl_deque_it_dtor(zend_object_iterator *iter)
 
 static void spl_deque_it_rewind(zend_object_iterator *iter)
 {
-	((spl_deque_it*)iter)->current = 0;
+	const spl_deque *object = Z_DEQUE_P(&iter->data);
+	((spl_deque_it*)iter)->current = object->array.iteration_offset;
 }
 
 static int spl_deque_it_valid(zend_object_iterator *iter)
 {
 	const spl_deque_it *iterator = (spl_deque_it*)iter;
 	const spl_deque *object = Z_DEQUE_P(&iter->data);
+	const size_t offset = iterator->current - object->array.iteration_offset;
 
-	if (iterator->current >= 0 && (zend_ulong) iterator->current < object->array.size) {
+	if (offset < object->array.size) {
 		return SUCCESS;
 	}
 
@@ -566,8 +597,9 @@ static zval *spl_deque_it_get_current_data(zend_object_iterator *iter)
 {
 	const spl_deque_it     *iterator = (spl_deque_it*)iter;
 	spl_deque *object   = Z_DEQUE_P(&iter->data);
+	const uint64_t offset = iterator->current - object->array.iteration_offset;
 
-	zval *data = spl_deque_read_offset_helper(object, iterator->current);
+	zval *data = spl_deque_read_offset_helper(object, offset);
 
 	if (UNEXPECTED(data == NULL)) {
 		return &EG(uninitialized_zval);
@@ -580,8 +612,8 @@ static void spl_deque_it_get_current_key(zend_object_iterator *iter, zval *key)
 {
 	const spl_deque_it     *iterator = (spl_deque_it*)iter;
 	const spl_deque *object   = Z_DEQUE_P(&iter->data);
+	const uint64_t offset = iterator->current - object->array.iteration_offset;
 
-	size_t offset = iterator->current;
 	if (offset >= object->array.size) {
 		ZVAL_NULL(key);
 	} else {
@@ -1020,6 +1052,7 @@ PHP_METHOD(Deque, unshift)
 	}
 
 	spl_deque *intern = Z_DEQUE_P(ZEND_THIS);
+	intern->array.iteration_offset -= argc; /* The front moved backwards by the number of elements */
 	size_t old_size = intern->array.size;
 	const size_t new_size = old_size + argc;
 	size_t mask = intern->array.mask;
@@ -1105,6 +1138,7 @@ PHP_METHOD(Deque, shift)
 	}
 
 	intern->array.size--;
+	intern->array.iteration_offset++;  /* The front moved forward */
 	const size_t old_offset = intern->array.offset;
 	const size_t old_mask = intern->array.mask;
 	intern->array.offset++;
diff --git a/ext/spl/tests/Deque/foreach.phpt b/ext/spl/tests/Deque/foreach.phpt
@@ -0,0 +1,95 @@
+--TEST--
+Deque modification during foreach
+--FILE--
+<?php
+
+/** Creates a reference-counted version of a literal string to test reference counting. */
+function mut(string $s) {
+    $s[0] = $s[0];
+    return $s;
+}
+
+echo "Test push/unshift\n";
+$deque = new Deque(['a', 'b']);
+foreach ($deque as $key => $value) {
+    if (strlen($value) === 1) {
+        $deque->push("${value}_");
+        $deque->unshift("_${value}");
+    }
+    printf("Key: %s Value: %s\n", var_export($key, true), var_export($value, true));
+}
+var_dump($deque);
+echo "Test shift\n";
+foreach ($deque as $key => $value) {
+    echo "Shifting $key $value\n";
+    var_dump($deque->shift());
+}
+
+echo "Test shift out of bounds\n";
+$deque = new Deque([mut('a1'), mut('b1'), mut('c1')]);
+foreach ($deque as $key => $value) {
+    $deque->shift();
+    $deque->shift();
+    echo "Saw $key: $value\n";
+    // iteration stops early because iteration position is now out of bounds.
+}
+var_dump($deque);
+
+echo "Test iteration behavior\n";
+$deque = new Deque([mut('a1'), mut('a2')]);
+$it = $deque->getIterator();
+echo json_encode(['valid' => $it->valid(), 'key' => $it->key(), 'value' => $it->current()]), "\n";
+$deque->shift();
+// invalid, outside the range of the deque
+echo json_encode(['valid' => $it->valid(), 'key' => $it->key()]), "\n";
+$it->next();
+echo json_encode(['valid' => $it->valid(), 'key' => $it->key(), 'value' => $it->current()]), "\n";
+$deque->unshift('a', 'b');
+unset($deque);
+echo json_encode(['valid' => $it->valid(), 'key' => $it->key(), 'value' => $it->current()]), "\n";
+
+?>
+--EXPECT--
+Test push/unshift
+Key: 0 Value: 'a'
+Key: 2 Value: 'b'
+Key: 4 Value: 'a_'
+Key: 5 Value: 'b_'
+object(Deque)#1 (6) {
+  [0]=>
+  string(2) "_b"
+  [1]=>
+  string(2) "_a"
+  [2]=>
+  string(1) "a"
+  [3]=>
+  string(1) "b"
+  [4]=>
+  string(2) "a_"
+  [5]=>
+  string(2) "b_"
+}
+Test shift
+Shifting 0 _b
+string(2) "_b"
+Shifting 0 _a
+string(2) "_a"
+Shifting 0 a
+string(1) "a"
+Shifting 0 b
+string(1) "b"
+Shifting 0 a_
+string(2) "a_"
+Shifting 0 b_
+string(2) "b_"
+Test shift out of bounds
+Saw 0: a1
+object(Deque)#2 (1) {
+  [0]=>
+  string(2) "c1"
+}
+Test iteration behavior
+{"valid":true,"key":0,"value":"a1"}
+{"valid":false,"key":null}
+{"valid":true,"key":0,"value":"a2"}
+{"valid":true,"key":2,"value":"a2"}
diff --git a/ext/spl/tests/Deque/iterator.phpt b/ext/spl/tests/Deque/iterator.phpt
@@ -12,16 +12,17 @@ function expect_throws(Closure $cb): void {
     }
 }
 
-// Iterators are associated with an index (as in offsetGet), not a position in the deque.
+// Iterators are associated with a position in the deque relative to the front of the deque *when iteration started*. key() returns the distance from the current start of the deque, or null.
 $dq = new Deque([new stdClass(), strtoupper('test')]);
 $it = $dq->getIterator();
 var_dump($it->key());
 var_dump($it->current());
 var_dump($it->next());
 var_dump($it->valid());
+echo "After shift\n";
 $dq->shift();
 var_dump($it->key());
-expect_throws(fn() => $it->current());
+var_dump($it->current());
 var_dump($it->valid());
 $dq->shift();
 var_dump($it->key()); // null for invalid iterator
@@ -38,9 +39,10 @@ object(stdClass)#2 (0) {
 }
 NULL
 bool(true)
-NULL
-Caught OutOfBoundsException: Index out of range
-bool(false)
+After shift
+int(0)
+string(4) "TEST"
+bool(true)
 NULL
 Caught OutOfBoundsException: Index out of range
 bool(false)
