Inconsistent behaviour with AppendIterator and empty generators

[sips-richard]

Description

I've encountered inconsistent behaviour in the handling of empty generators with AppendIterator. Please can you confirm whether this is a bug or some weird expected behaviour.

Scenario 1

When iterating over a generator that yields nothing, foreach has no items to iterate over so var_dump isn't called, as expected.

<?php

function yieldEmpty()
{
    yield from [];
}

foreach (yieldEmpty() as $item) {
    var_dump($item);
}

Scenario 2

When attaching a generator yielding items and a generator yielding nothing and attaching both to an AppendIterator, foreach iterates over the items from the former and the three values are output, as expected.

<?php

function yieldRows()
{
    yield from ['one', 'two', 'three'];
}

function yieldEmpty()
{
    yield from [];
}

$iterator = new AppendIterator();
$iterator->append(yieldRows());
$iterator->append(yieldEmpty());

foreach ($iterator as $item) {
    var_dump($item);
}

Outputs (as expected):

string(3) "one"
string(3) "two"
string(5) "three"

Scenario 3

When attaching ONLY a generator yielding nothing and attaching it to an AppendIterator, foreach unexpectedly fails with an exception.

<?php

function yieldEmpty()
{
    yield from [];
}

$iterator = new AppendIterator();
$iterator->append(yieldEmpty());

foreach ($iterator as $item) {
    var_dump($item);
}

Expected output (following on from scenario 1 and 2):

string(3) "one"
string(3) "two"
string(5) "three"

Actual output:

PHP Fatal error:  Uncaught Exception: Cannot traverse an already closed generator in /Users/richard.coupland/test3.php:11
Stack trace:
#0 /Users/richard.coupland/test3.php(11): AppendIterator->rewind()
#1 {main}
  thrown in /Users/richard.coupland/test3.php on line 11

Fatal error: Uncaught Exception: Cannot traverse an already closed generator in /Users/richard.coupland/test3.php:11
Stack trace:
#0 /Users/richard.coupland/test3.php(11): AppendIterator->rewind()
#1 {main}
  thrown in /Users/richard.coupland/test3.php on line 11

PHP Version

8.2.7

Operating System

No response

[KapitanOczywisty]

You should wrap generators or the whole AppendIterator with NoRewindIterator. AppendIterator doesn't check if it has generator and calls rewind freely. Maybe handling of empty generators can be improved, but I wouldn't rely on that.

[nielsdos]

I do think this is a bug. (1) and (2) should behave the same based on my understanding: https://3v4l.org/idBif
So I think the behaviour of (2) is wrong.

[sips-richard]

@KapitanOczywisty That's actually the fix I went with, but thought it worth reporting the behaviour because it's inconsistent with iterating over an empty generator directly. Either they should both throw the exception or neither should.

[KapitanOczywisty]

@nielsdos Technically it's not a rewind what causes issue, but getting internal iterator when generator->valid() is false, which is called from AppendIterator->rewind(). Probably spl could be fixed somehow, but I don't understand that code.

Also old report: https://bugs.php.net/bug.php?id=72692

[nielsdos]

Both append and rewind call get_generator. The one in append succeeds, but of course for the empty generator, append will close that generator. Therefore, when get_generator gets called again in the rewind function, it complains.
It is necessary to get the generator in the append function, because otherwise you can't do this:

$iterator = new AppendIterator;
$iterator->append(my generator);
var_dump($iterator->next()); // Should print the first entry of my generator

To me it seems like we want to distinguish between a generator that got closed in the append phase, i.e. an empty generator; and a generator that was consumed during the iteration. Note that I'm using the word generator here, but of course we shouldn't break other iterators too... Anyway, I've worked around this problem with the following hacky patch:

diff --git a/ext/spl/spl_iterators.c b/ext/spl/spl_iterators.c
index 97253cfe93..8718ac5fc2 100644
--- a/ext/spl/spl_iterators.c
+++ b/ext/spl/spl_iterators.c
@@ -1367,6 +1367,7 @@ static spl_dual_it_object* spl_dual_it_construct(INTERNAL_FUNCTION_PARAMETERS, z
 			object_init_ex(&intern->u.append.zarrayit, spl_ce_ArrayIterator);
 			zend_call_method_with_0_params(Z_OBJ(intern->u.append.zarrayit), spl_ce_ArrayIterator, &spl_ce_ArrayIterator->constructor, "__construct", NULL);
 			intern->u.append.iterator = spl_ce_ArrayIterator->get_iterator(spl_ce_ArrayIterator, &intern->u.append.zarrayit, 0);
+			intern->u.append.nr_of_skips = 0;
 			return intern;
 		case DIT_RegexIterator:
 		case DIT_RecursiveRegexIterator: {
@@ -2930,6 +2931,10 @@ PHP_METHOD(AppendIterator, append)
 			spl_append_it_next_iterator(intern);
 		} while (Z_OBJ(intern->inner.zobject) != Z_OBJ_P(it));
 		spl_append_it_fetch(intern);
+		if (UNEXPECTED(!intern->inner.iterator)) {
+			/* The iterator was empty */
+			intern->u.append.nr_of_skips++;
+		}
 	}
 } /* }}} */
 
@@ -2964,6 +2969,11 @@ PHP_METHOD(AppendIterator, rewind)
 	SPL_FETCH_AND_CHECK_DUAL_IT(intern, ZEND_THIS);
 
 	intern->u.append.iterator->funcs->rewind(intern->u.append.iterator);
+
+	for (uint32_t i = 0, skips = intern->u.append.nr_of_skips; i < skips; i++) {
+		intern->u.append.iterator->funcs->move_forward(intern->u.append.iterator);
+	}
+
 	if (spl_append_it_next_iterator(intern) == SUCCESS) {
 		spl_append_it_fetch(intern);
 	}
diff --git a/ext/spl/spl_iterators.h b/ext/spl/spl_iterators.h
index ee6339e1f2..e03f6045f5 100644
--- a/ext/spl/spl_iterators.h
+++ b/ext/spl/spl_iterators.h
@@ -136,6 +136,7 @@ typedef struct _spl_dual_it_object {
 		struct {
 			zval                  zarrayit;
 			zend_object_iterator *iterator;
+			uint32_t              nr_of_skips;
 		} append;
 		struct {
 			zend_long        flags;

Here's a test I created: https://gist.github.com/nielsdos/b57405289b77f0f0b6e91dcde6280083 I think it produces the right result.
However, this is pretty horrible and I'm sure there is a better way of doing this.

[nielsdos]

I've dug again into this. It's not necessarily the rewind that's causing a problem, the problem would be fixed too if we could avoid calling get_iterator repeatedly for the same object.
What happens makes a lot of sense, the AppendIterator::append method wants to move to the first valid subiterator if the current position is invalid.

I tried various things, like skipping the work in AppendIterator::rewind if we're already at the start and nothing has been consumed yet. This works because get_iterator won't be called again for the generator, but the problem is that it's observable. In fact, there's tests that assert the rewind is called of the subiterator anyway. Especially with side-effects this solution seems dangerous.

Another trick could be to only call get_iterator once per object, this would work at a cost of increasing the memory consumption, so I'm not too sure about this.

The horrible hack I posted up above is not 100% correct implementation-wise, but would come quite close for an ad-hoc solution. I haven't found anything better unfortunately. Skipping dead iterators in that sort of way is the only way I can see to fix this issue without changing the behaviour in a backwards-incompatible way.