Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
### Changed

- Use `static` closures as much as possible to reduce the probability of creating circular references by capturing `$this` as it can lead to memory root buffer exhaustion.
- Remove keeping intermediary values of a deferred `Sequence` that is referenced by no one.

### Fixed

Expand Down
73 changes: 73 additions & 0 deletions proofs/sequence.php
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,77 @@ static function($assert, $string, $chunk) {
);
},
);

yield proof(
'Sequende::defer() holds intermediary values even when no longer used',
given(
Set\Sequence::of(Set\Type::any()),
Set\Sequence::of(Set\Type::any()),
),
static function($assert, $prefix, $suffix) {
$initial = Sequence::defer((static function() use ($prefix, $suffix) {
foreach ($prefix as $value) {
yield $value;
}

foreach ($suffix as $value) {
yield $value;
}
})());

// This does a partial read on the generator
$assert->same(
$prefix,
$initial
->take(\count($prefix))
->toList(),
);

// The maps are only here to wrap the generator, it doesn't change
// the values
$another = $initial
->map(static fn($value) => [$value])
->map(static fn($values) => $values[0]);
unset($initial);

// If it didn't store the intermediary values the array would miss
// the prefix values due to the partial read on the initial
// generator due to the ->take()->toList() call above
$assert->same(
[...$prefix, ...$suffix],
$another->toList(),
);
},
);

yield proof(
"Sequence::defer() stack trace doesn't show intermediary sequences when not used",
given(Set\Integers::between(1, 10)),
static function($assert, $calls) {
$expected = null;
$sequence = Sequence::defer((static function() use (&$expected) {
yield null;

throw $expected = new Exception;
})());

for ($i = 0; $i < $calls; $i++) {
$sequence = $sequence->map(static fn($value) => $value);
}

try {
$sequence->toList();
$assert->fail('it should throw');
} catch (Exception $e) {
$assert->same($expected, $e);

$accumulations = \array_filter(
$e->getTrace(),
static fn($frame) => \str_ends_with($frame['file'] ?? '', 'src/Accumulate.php'),
);

$assert->count(1, $accumulations);
}
},
);
};
36 changes: 20 additions & 16 deletions src/Accumulate.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,21 @@
* Simple iterator to cache the results of a generator so it can be iterated
* over multiple times
*
* @template T
* @template S
* @implements \Iterator<T, S>
* @implements \Iterator<S>
* @internal Do not use this in your code
* @psalm-immutable Not really immutable but to simplify declaring immutability of other structures
*/
final class Accumulate implements \Iterator
{
/** @var \Generator<T, S> */
/** @var \Generator<S> */
private \Generator $generator;
/** @var list<T> */
private array $keys = [];
/** @var list<S> */
private array $values = [];
private bool $started = false;

/**
* @param \Generator<T, S> $generator
* @param \Generator<S> $generator
*/
public function __construct(\Generator $generator)
{
Expand All @@ -35,27 +33,31 @@ public function __construct(\Generator $generator)
*/
public function current(): mixed
{
/** @psalm-suppress InaccessibleProperty */
$this->started = true;
/** @psalm-suppress UnusedMethodCall */
$this->pop();

return \current($this->values);
}

/**
* @return T
* @return int<0, max>|null
*/
public function key(): mixed
public function key(): ?int
{
/** @psalm-suppress InaccessibleProperty */
$this->started = true;
/** @psalm-suppress UnusedMethodCall */
$this->pop();

return \current($this->keys);
return \key($this->values);
}

public function next(): void
{
/** @psalm-suppress InaccessibleProperty */
\next($this->keys);
$this->started = true;
/** @psalm-suppress InaccessibleProperty */
\next($this->values);

Expand All @@ -68,13 +70,15 @@ public function next(): void
public function rewind(): void
{
/** @psalm-suppress InaccessibleProperty */
\reset($this->keys);
$this->started = true;
/** @psalm-suppress InaccessibleProperty */
\reset($this->values);
}

public function valid(): bool
{
/** @psalm-suppress InaccessibleProperty */
$this->started = true;
/** @psalm-suppress ImpureMethodCall */
$valid = !$this->reachedCacheEnd() || $this->generator->valid();

Expand All @@ -88,6 +92,11 @@ public function valid(): bool
return $valid;
}

public function started(): bool
{
return $this->started;
}

private function reachedCacheEnd(): bool
{
return \key($this->values) === null;
Expand All @@ -96,11 +105,6 @@ private function reachedCacheEnd(): bool
private function pop(): void
{
if ($this->reachedCacheEnd()) {
/**
* @psalm-suppress InaccessibleProperty
* @psalm-suppress ImpureMethodCall
*/
$this->keys[] = $this->generator->key();
/**
* @psalm-suppress InaccessibleProperty
* @psalm-suppress ImpureMethodCall
Expand Down
Loading