SplFixedArray can be used in nested 'foreach' loops

[alexdowad]

This is a first stab at fixing Bug #79404 (https://bugs.php.net/bug.php?id=79404&thanks=3).

In that thread, @nikic suggested changing SplFixedArray to an IteratorAggregate rather Iterator. Are the project maintainers happy with that breaking change? If so, I will work on this issue a bit more and complete the implementation.

For now, this is a "halfway" fix in which SplFixedArray can't make up its mind about whether it wants to be an Iterator or an IteratorAggregate. Pretty stinky if you ask me, but I'm putting it here so others can review and comment. And without further ado, I give you... the commit log:

---

Because SplFixedArray is an Iterator, and stores its own iteration state, it cannot be
used in nested foreach loops. If you try, the inner loop will overwrite the iteration
position of the outer loop, so that the outer loop 'thinks' it is finished after the inner
loop finishes.

To illustrate:

$spl = SplFixedArray::fromArray([0, 1]);
foreach ($spl as $a) {
  foreach ($spl as $b) {
    echo "$a $b";
  }
}

Only prints two lines:

0 0
0 1

Interestingly, though SplFixedArray has methods like ::current(), ::next(), etc., these
are not used by the foreach loop. Rather, an instance of _spl_fixedarray_it is created
internally and used to track the iteration state.

Therefore, if the current iteration position is stored in _spl_fixedarray_it, the nested
foreach loops will not interfere with each other in this way.

However, the problem is that ::current(), ::next(), etc. are still part of the SplFixedArray
interface and users may expect them to 'work' in a foreach loop. For ::next() and ::rewind(),
there is nothing we can do, since the SplFixedArray instance does not keep any reference to
the _spl_fixedarray_it structures being used by the foreach loops. However, we can still keep
::valid(), ::current(), and ::key() working by keeping a current iteration position in each
SplFixedArray instance and updating it at the same time as we update the _spl_fixedarray_it.

Although, they still break in cases like this:

foreach ($spl as $a) {
  foreach ($spl as $b) {
    // do something
  }
  $spl->key(); // will NOT return iteration position of outer loop
}

Overall, this fix stinks and it may be better to bite the bullet and convert SplFixedArray
to IteratorAggregate.

[nikic]

I'd be fine with this PR as-is, as I would consider foreach working correctly to be more important than the ability to combine foreach and explicit calls to iterator methods.

That said, I'd also be fine with making SplFixedArray an IteratorAggregate (implementation should basically be this PR plus #5216), but would like to hear some other opinions on that. That's clearly a backwards compatibility break, but I think the impact would not be terrible (as the 99% use case should be foreach), and this is our chance to make the change...

[alexdowad]

Thanks for the comments. Will wait to see if there will be comments from others.

By the way, should contributors include an update to NEWS in each PR which changes user-visible behavior?

[Girgias]

Thanks for the comments. Will wait to see if there will be comments from others.

By the way, should contributors include an update to NEWS in each PR which changes user-visible behavior?

The NEWS file is updated by the commiter to php-src, as the file changes quite often and making merging more painful.

I'd be fine with this PR as-is, as I would consider foreach working correctly to be more important than the ability to combine foreach and explicit calls to iterator methods.

That said, I'd also be fine with making SplFixedArray an IteratorAggregate (implementation should basically be this PR plus #5216), but would like to hear some other opinions on that. That's clearly a backwards compatibility break, but I think the impact would not be terrible (as the 99% use case should be foreach), and this is our chance to make the change...

I'd prefer that, cause it seems pretty weird to be doing explicit call to the iterating methods when using a foreach

[alexdowad]

@Girgias, you mean would also be happy to have SplFixedArray converted to IteratorAggregate?

[Girgias]

@Girgias, you mean would also be happy to have SplFixedArray converted to IteratorAggregate?

Yes

[karrakoliko]

Just an idea.

In my opinion, there should be kind of "IterationContext", or something like this. As far as this base entity is omitted in architecture - we have bugs/problems like this.

Every foreach(), for(), or $iterator->rewind() should create separate IterationContext somewhere in PHP internals.
Each of these InterationContext's should be deleted when loop is over. May be through garbage collector, i'm not sure.

In the case of netsted foreach loops there should be 2 iteration contexts, for every foreach().

IMO, Traversable/iterable object shouldn't store it's own pointer position, just because a pointer position is not a part of it's state - it's a state of certain context, loop, expression.

[alexdowad]

Just an idea.

In my opinion, there should be kind of "IterationContext", or something like this. As far as this base entity is omitted in architecture - we have bugs/problems like this.

Every foreach(), for(), or $iterator->rewind() should create separate IterationContext somewhere in PHP internals.
Each of these InterationContext's should be deleted when loop is over. May be through garbage collector, i'm not sure.

In the case of netsted foreach loops there should be 2 iteration contexts, for every foreach().

IMO, Traversable/iterable object shouldn't store it's own pointer position, just because a pointer position is not a part of it's state - it's a state of certain context, loop, expression.

That's exactly what the spl_fixedarray_it struct is for. See the content of this PR.

[cmb69]

I'd also be fine with making SplFixedArray an IteratorAggregate

+1.

Note that SplHeap has the same issue: https://3v4l.org/YUYUr.

[karrakoliko]

That's exactly what the spl_fixedarray_it struct is for. See the content of this PR.

@alexdowad today it is "spl_fixedarray_it". Tomorrow we need "spl_heap_it". The day after tomorrow "some_structure_it".

My point is there must be a way to unify behaviour for all the iterables in one place instead of creating such struct's for every one.

[alexdowad]

Is there anywhere (aside from GH) where development of the PHP interpreter and standard libraries is discussed? As I am digging through the code and working on this issue, I keep discovering strange and interesting things and would like to ask an occasional question or two.

[nikic]

@alexdowad https://chat.stackoverflow.com/rooms/11/php

[alexdowad]

I have a patch which converts SplFixedArray to IteratorAggregate, but it is based on @nikic's internal-iterator branch (#5216). Will only work if that one is merged.

[nikic]

I bumped the InternalIterator topic on internals.

[alexdowad]

OK, updated this PR with new code which uses InternalIterator.

[nikic]

The implementation here looks fine. The only question is whether we can/want to do this change for PHP 8, given the backwards-compatibility impact it has. "Yes" from my side.

[nikic]

I'd suggest bringing this up on the mailing list.

[alexdowad]

I'd suggest bringing this up on the mailing list.

Done. I hope my message actually went through.

[morrisonlevi]

As someone with commits in the SPL, I approve this change. The SPL has a lot of warts, but I think this BC break is actually reasonable and should be pretty low impact.

[alexdowad]

Looks like there is not much interest on the internals list. 😝

[nikic]

Internals discussion for reference: https://externals.io/message/110731

Not much feedback, but the feedback there was, was positive. I think we should move ahead here.

[alexdowad]

Pushing an update.

[nikic]

Please also add an UPGRADING note when landing.

[alexdowad]

Please also add an UPGRADING note when landing.

And I guess an entry in NEWS also.

[cmb69]

And I guess an entry in NEWS also.

Yes, please.

[alexdowad]

OK, done.