Behaviour of `Ds\Set::getIterator()` does not match C implementation

[LeonMelis]

I didn't want to re-open a 5 year old ticket, but I don't think #73 has been correctly solved. The polyfill method of getIterator does not behave the same way as the native C method.

From the discussion in #73

I believe getIterator has been implemented in the extension now, as all the collections implement IteratorAggregate

Both the C implementation and the PHP polyfill of Ds\Set indeed implement the IteratorAggregate interface, and thus implement the getIterator method. In order to be compatible with the IteratorAggregate interface, the getIterator class method should return a Traversable, which is an abstract interface. However, the polyfill returns a Generator instead.

If we now compare the behaviour of getIterator between the C code and PHP polyfill, we notice that:

• In the C implementation Ds\Set::getIterator() simply returns a pointer of itself. This is correct, as the Ds\Set class can be used in foreach, which satisfies the requirements of Traversable. But do note that, the Ds\Set class does not implement the Iterable interface, it does not expose current(), key(), next(), rewind() and valid().
• In the polyfill Ds\Set::getIterator() returns a Generator. A Generator exposes a superset of the Iterable interface. So in the Polyfill, you do get access to current(), key(), next(), rewind() and valid(), which is not the same behaviour as the C code.
• In JetBrain's stubs the return value of Ds\Set::getIterator() is specified as Traversable, which is correct and expected for a class that is supposed to implement the IteratorAggregate interface.

So, with the polyfill you can do this:

// This works with the polyfill, but not with native C implementation of Ds\Set
$it = $set->getIterator();
while ($it->valid()) {
  // Do something with $it->current()
  $it->next();
}

The snippet above will not work with the native C implementation of DS\Set, PHP will throw a hard error for calling undefined methods.

So, in order for the polyfill to correctly mimic the behaviour of native Ds\Set::getIterator, I believe it should be:

/**
  * Get iterator
  * @return Traversable
  */
public function getIterator()
{
   return $this->table;
}

Though I am not 100% sure if the polyfill should return a clone. When looking at the C implementation:

METHOD(getIterator) {
    PARSE_NONE;
    ZVAL_COPY(return_value, getThis());
}

I'm not too familiar with PHP internals, but I believe getThis() returns a pointer instance, and ZVAL_COPY does not dereference (I think?) so that would mean that the returned iterator is just a reference to the Set object instance itself, not a copy.