Making all Traversables an Iterator or IteratorAggregate

Hey Sara,

Am 12.05.2020 um 14:49 schrieb Sara Golemon pollita@php.net:

// WeakMap::getIterator(): Iterator
ZEND_METHOD(WeakMap, getIterator)
{
if (zend_parse_parameters_none() == FAILURE) {
return;
}
zend_create_internal_iterator_zval(return_value, ZEND_THIS);
}

Given that the body of this method seems to usually (always?) be the same,
why not define it in InternalIterator and allow it to be inherited?

Good idea.

There's some bikeshed potential here regarding the class name.

Not personally over-picky, but I do agree that "Internal*" is a bit
awkward. Unfortunately there's not much that's better and appropriate for
exposing to scripts. This might be one of those rare occasions where
exposing "Zend" into userspace makes sense. "PHP" is overloaded into
several meanings that are significant for userspace developers, but
something like "ZendIterator" might convey the right level of "This has to
do with the engine, please move along". Or maybe go verbose:
'IteratorForExtensionClassImplementations'. :p

We call the engine Zend, but eih, that's a rather internal detail we mostly don't leak into userland, and I wouldn't do it here either.
InternalIterator is the better choice I think.

ZEND_ASSERT(scope->get_iterator != zend_user_it_get_new_iterator); >
Does this mean that if I do class Foo implements InternalIterator {} in a
script, I can assert (crash) PHP? I don't see anything obvious (at a
glance) preventing me from inheriting from InternalIterator.

The class is marked final, so userland won't be able to directly extend it. (zend_ce_internal_iterator->ce_flags |= ZEND_ACC_FINAL;)

Bob

This might be one of those rare occasions where
exposing "Zend" into userspace makes sense.

I think 'Engine' would be easier for userland people to grok.

Also, for those brave souls who are implementing PHP in other
technologies, avoiding using an implementation detail of the common
internal engine, would be better for them.

cheers
Dan
Ack

// WeakMap::getIterator(): Iterator
ZEND_METHOD(WeakMap, getIterator)
{
if (zend_parse_parameters_none() == FAILURE) {
return;
}
zend_create_internal_iterator_zval(return_value, ZEND_THIS);
}

Given that the body of this method seems to usually (always?) be the same,
why not define it in InternalIterator and allow it to be inherited?

There's some bikeshed potential here regarding the class name.

Not personally over-picky, but I do agree that "Internal*" is a bit
awkward. Unfortunately there's not much that's better and appropriate for
exposing to scripts. This might be one of those rare occasions where
exposing "Zend" into userspace makes sense. "PHP" is overloaded into
several meanings that are significant for userspace developers, but
something like "ZendIterator" might convey the right level of "This has to
do with the engine, please move along". Or maybe go verbose:
'IteratorForExtensionClassImplementations'. :p

I do not believe we should expose the Zend term into userland.

Let the bike shedding start: Perhaps just "EngineIterator".

cheers,
Derick

--
PHP 7.4 Release Manager
Host of PHP Internals News: https://phpinternals.news
Like Xdebug? Consider supporting me: https://xdebug.org/support
https://derickrethans.nl | https://xdebug.org | https://dram.io
twitter: @derickr and @xdebug

// WeakMap::getIterator(): Iterator
ZEND_METHOD(WeakMap, getIterator)
{
if (zend_parse_parameters_none() == FAILURE) {
return;
}
zend_create_internal_iterator_zval(return_value, ZEND_THIS);
}

Given that the body of this method seems to usually (always?) be the same,
why not define it in InternalIterator and allow it to be inherited?

There's some bikeshed potential here regarding the class name.

Not personally over-picky, but I do agree that "Internal*" is a bit
awkward. Unfortunately there's not much that's better and appropriate for
exposing to scripts. This might be one of those rare occasions where
exposing "Zend" into userspace makes sense. "PHP" is overloaded into
several meanings that are significant for userspace developers, but
something like "ZendIterator" might convey the right level of "This has to
do with the engine, please move along". Or maybe go verbose:
'IteratorForExtensionClassImplementations'. :p

ZEND_ASSERT(scope->get_iterator != zend_user_it_get_new_iterator); >
Does this mean that if I do class Foo implements InternalIterator {} in
a script, I can assert (crash) PHP? I don't see anything obvious (at a
glance) preventing me from inheriting from InternalIterator.

InternalIterator isn't an interface, so it's not possible to implement it
in this way. It's not possible to directly construct an object either.

What can happen is that the getIterator() method is replaced in a child
class, but this will get picked up automatically, and the get_iterator
handler will be changed to zend_user_it_get_new_iterator automatically.

Nikita

wt., 9 cze 2020, 15:33 użytkownik Nikita Popov nikita.ppv@gmail.com
napisał:

On Tue, May 12, 2020 at 10:26 AM Nikita Popov nikita.ppv@gmail.com
wrote:

On Wed, Mar 11, 2020 at 10:50 AM Nikita Popov nikita.ppv@gmail.com
wrote:

Hi internals,

Userland classes that implement Traversable must do so either through
Iterator or IteratorAggregate. The same requirement does not exist for
internal classes: They can implement the internal get_iterator
mechanism,
without exposing either the Iterator or IteratorAggregate APIs. This
makes
them usable in get_iterator(), but incompatible with any Iterator based
APIs.

This should have said: "This makes them usable in foreach(), but
incompatible with any Iterator based APIs."

A lot of internal classes do this, because exposing the userland APIs is

simply a lot of work. I would like to add a general mechanism to make
this
simpler: https://github.com/php/php-src/pull/5216 adds a generic
"InternalIterator" class, that essentially converts the internal
get_iterator interface into a proper Iterator. Internal classes then
only
need to a) implement the IteratorAggregate interface and b) add a
getIterator() method with an implementation looking like this:

// WeakMap::getIterator(): Iterator
ZEND_METHOD(WeakMap, getIterator)
{
if (zend_parse_parameters_none() == FAILURE) {
return;
}
zend_create_internal_iterator_zval(return_value, ZEND_THIS);
}

This allows internal classes to trivially implement IteratorAggregate,
and as such allows us to enforce that all Traversables implement
Iterator
or IteratorAggregate.

Does anyone have thoughts on this change? Mostly this is a feature for
extensions, but also user-visible in that a bunch of classes will switch
from being Traversable to being IteratorAggregate.

We may also want to convert some existing Iterators to
IteratorAggregates.
For example SplFixedArray currently implements Iterator, which means that
it's not possible to have nested loops over SplFixedArray. We could now
easily fix this by switching it to use IteratorAggregate, which will
allow
multiple parallel iterators to work on the same array. Of course, there
is
BC break potential in such a change.

There's some bikeshed potential here regarding the class name. I picked
"InternalIterator" as an iterator for internal classes, but "internal
iteration" is also a technical term (the opposite of "external
iteration"),
so maybe that name isn't ideal.

Unfortunately this bikeshed remains unpainted... The proposed names were:

1. InternalIterator
2. ZendIterator
3. IteratorForExtensionClassImplementations
4. EngineIterator

I'm somewhat partial to the third option, with a less verbose name:
IteratorForExtensions.

ExtensionsIterator??

BR,
Michał

On Tue, May 12, 2020 at 10:26 AM Nikita Popov nikita.ppv@gmail.com
wrote:

On Wed, Mar 11, 2020 at 10:50 AM Nikita Popov nikita.ppv@gmail.com
wrote:

Hi internals,

Userland classes that implement Traversable must do so either through
Iterator or IteratorAggregate. The same requirement does not exist for
internal classes: They can implement the internal get_iterator mechanism,
without exposing either the Iterator or IteratorAggregate APIs. This makes
them usable in get_iterator(), but incompatible with any Iterator based
APIs.

This should have said: "This makes them usable in foreach(), but
incompatible with any Iterator based APIs."

A lot of internal classes do this, because exposing the userland APIs is

simply a lot of work. I would like to add a general mechanism to make this
simpler: https://github.com/php/php-src/pull/5216 adds a generic
"InternalIterator" class, that essentially converts the internal
get_iterator interface into a proper Iterator. Internal classes then only
need to a) implement the IteratorAggregate interface and b) add a
getIterator() method with an implementation looking like this:

// WeakMap::getIterator(): Iterator
ZEND_METHOD(WeakMap, getIterator)
{
if (zend_parse_parameters_none() == FAILURE) {
return;
}
zend_create_internal_iterator_zval(return_value, ZEND_THIS);
}

This allows internal classes to trivially implement IteratorAggregate,
and as such allows us to enforce that all Traversables implement Iterator
or IteratorAggregate.

Does anyone have thoughts on this change? Mostly this is a feature for
extensions, but also user-visible in that a bunch of classes will switch
from being Traversable to being IteratorAggregate.

We may also want to convert some existing Iterators to
IteratorAggregates. For example SplFixedArray currently implements
Iterator, which means that it's not possible to have nested loops over
SplFixedArray. We could now easily fix this by switching it to use
IteratorAggregate, which will allow multiple parallel iterators to work on
the same array. Of course, there is BC break potential in such a change.

There's some bikeshed potential here regarding the class name. I picked
"InternalIterator" as an iterator for internal classes, but "internal
iteration" is also a technical term (the opposite of "external iteration"),
so maybe that name isn't ideal.

Unfortunately this bikeshed remains unpainted... The proposed names were:

1. InternalIterator
2. ZendIterator
3. IteratorForExtensionClassImplementations
4. EngineIterator

I'm somewhat partial to the third option, with a less verbose name:
IteratorForExtensions.

I went ahead and changed the implementation to use IteratorForExtensions.
Is anyone overly unhappy with that one?

@Michal: "ExtensionsIterator" to me sounds like an iterator that iterates
over extensions.

Regards,
Nikita