Show partition failure

[AlexandruGG]

I noticed an issue trying to use the new partition method from master. I am not sure exactly what's causing it, but I wrote a test to showcase -> whenever we run count on one of the results from the partition operation it causes this fatal error:

Fatal error: Generator passed to yield from was aborted without proper return and is unable to continue

In the other project where I was trying to use this I dumped the collection before-after the count:

Before

loophp\collection\Collection^ {#5180
  -parameters: array:1 [
    0 => Generator {#5177
      executing: {
        ./vendor/loophp/collection/src/Operation/Filter.php:46 { …}
      }
      closed: false
    }
  ]
  -source: Closure(iterable $iterable): Iterator^ {#5148
    returnType: "Iterator"
    class: "loophp\collection\Collection"
  }
}

After

loophp\collection\Collection^ {#5180
  -parameters: array:1 [
    0 => Generator {#5177
      closed: true
    }
  ]
  -source: Closure(iterable $iterable): Iterator^ {#5148
    returnType: "Iterator"
    class: "loophp\collection\Collection"
  }
}

Based on this it looks like the generator appears closed

[AlexandruGG]

@drupol hey, any clue what's going on here? 😅
have a look at the description and the tests I added

[AlexandruGG]

looks like the issue is here, when we instantiate the collection from a generator, which is what also happens in partition

[AlexandruGG]

with this refactor it works looks like

[drupol]

That's weird, this was supposed to work. I will most probably need to do git bisect in order to see where it comes from.

I made a small reproducer here:

<?php

/**
 * For the full copyright and license information, please view
 * the LICENSE file that was distributed with this source code.
 */

declare(strict_types=1);

namespace App;

use loophp\collection\Collection;

include __DIR__ . '/vendor/autoload.php';

$gen = static fn () => yield from range(1, 10);

$c = Collection::fromIterable($gen());

var_dump($c->all());
var_dump($c->all());
// or
// var_dump($c->count());
// var_dump($c->count());

[AlexandruGG]

That's weird, this was supposed to work. I will most probably need to do git bisect in order to see where it comes from.

I made a small reproducer here:

<?php

/**
 * For the full copyright and license information, please view
 * the LICENSE file that was distributed with this source code.
 */

declare(strict_types=1);

namespace App;

use loophp\collection\Collection;

include __DIR__ . '/vendor/autoload.php';

$gen = static fn () => yield from range(1, 10);

$c = Collection::fromIterable($gen());

var_dump($c->all());
var_dump($c->all());
// or
// var_dump($c->count());
// var_dump($c->count());

@drupol I found this: https://github.com/JeroenDeDauw/RewindableGenerator.

Can you tell what change we need to make in ClosureIterator to replicate the behaviour?

[drupol]

@drupol I found this: https://github.com/JeroenDeDauw/RewindableGenerator.

Can you tell what change we need to make in ClosureIterator to replicate the behaviour?

I have doubts that using it would fix the issue.

The ClosureIterator is a "rewindable" iterator already.

[AlexandruGG]

@drupol I found this: https://github.com/JeroenDeDauw/RewindableGenerator.
Can you tell what change we need to make in ClosureIterator to replicate the behaviour?

I have doubts that using it would fix the issue.

The ClosureIterator is a "rewindable" iterator already.

You're right, the issue is not there but before that. Here's a more complete example:

<?php
declare(strict_types=1);

include __DIR__ . '/../../vendor/autoload.php';

use loophp\collection\Iterator\ClosureIterator;
use loophp\collection\Iterator\IterableIterator;


$gen = static fn () => yield from range(1, 10);

$c = new ClosureIterator($gen);

var_dump(iterator_to_array($c)); // works
var_dump(iterator_to_array($c)); // works

$c = new ClosureIterator(static fn (iterable $iterable): Iterator => new IterableIterator($iterable), $gen());

var_dump(iterator_to_array($c)); // works
var_dump(iterator_to_array($c)); // BOOM

$c = new IterableIterator($gen());

var_dump(iterator_to_array($c)); // works
var_dump(iterator_to_array($c)); // BOOM!

[AlexandruGG]

@drupol I found this: https://github.com/JeroenDeDauw/RewindableGenerator.
Can you tell what change we need to make in ClosureIterator to replicate the behaviour?

I have doubts that using it would fix the issue.
The ClosureIterator is a "rewindable" iterator already.

You're right, the issue is not there but before that. Here's a more complete example:

<?php
declare(strict_types=1);

include __DIR__ . '/../../vendor/autoload.php';

use loophp\collection\Iterator\ClosureIterator;
use loophp\collection\Iterator\IterableIterator;


$gen = static fn () => yield from range(1, 10);

$c = new ClosureIterator($gen);

var_dump(iterator_to_array($c)); // works
var_dump(iterator_to_array($c)); // works

$c = new ClosureIterator(static fn (iterable $iterable): Iterator => new IterableIterator($iterable), $gen());

var_dump(iterator_to_array($c)); // works
var_dump(iterator_to_array($c)); // BOOM

$c = new IterableIterator($gen());

var_dump(iterator_to_array($c)); // works
var_dump(iterator_to_array($c)); // BOOM!

The second and third cases are basically how Collection::fromIterable works

[drupol]

This seems to fix the issue but creates some others:

diff --git a/src/Iterator/ClosureIterator.php b/src/Iterator/ClosureIterator.php
index 8af2e3c0..9d78331d 100644
--- a/src/Iterator/ClosureIterator.php
+++ b/src/Iterator/ClosureIterator.php
@@ -9,7 +9,7 @@ declare(strict_types=1);
 
 namespace loophp\collection\Iterator;
 
-use Generator;
+use Iterator;
 
 /**
  * @internal
@@ -48,10 +48,10 @@ final class ClosureIterator extends ProxyIterator
     }
 
     /**
-     * @return Generator<TKey, T>
+     * @return Iterator<TKey, T>
      */
-    private function getGenerator(): Generator
+    private function getGenerator(): Iterator
     {
-        return yield from ($this->callable)(...$this->arguments);
+        return ($this->callable)(...$this->arguments);
     }
 }

[drupol]

I guess #148 supersedes this?

[AlexandruGG]

I guess #148 supersedes this?

yeah I'll close this one once that gets merged

[drupol]

I'll copy the tests over tomorrow.