Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Summary: `suspend`ing within `foreach` bodies in `coroutine` functions were broken. Because `suspend` calls ultimately got rewritten into a `goto`, when there was a `suspend` inside a `foreach` body it would result into a `goto` into the middle of a `foreach` body. As a result, the `foreach_collection` would not be set, causing an HHVM fatal error. Additionally, the behavior of multiple resumption within a foreach loop in php is not often considered. Since the `foreach` construct in php 5/7 behaves differently depends on the type of collection it is iterating over, this introduces some more complexity in the translation as well. The proposed translation is as follows: ``` foreach (expression as $k => $v) { // User code } ``` will be translated into: (the prelude) ``` $__iterator_0 = $__closure->saved___iterator_0; $__iterator_index_0 = $__closure->saved___iterator_index_0; ``` (the body) ``` $__iterator_0 = new \CoroutineForeachHelper($x); $__iterator_index_0 = 0; while ($__iterator_0->valid($__iterator_index_0))) { $key = $__iterator_0->key($__iterator_index_0); $val = $__iterator_0->current($__iterator_index_0); // User code goes here $__iterator_index_0 = $__iterator_0->next($__iterator_index_0); } ``` (and the epilogue) ``` $__closure->saved___iterator_0 = $__iterator_0; $__closure->saved___iterator_index_0 = $__iterator_index_0; ``` and `CoroutineForeachHelper` is defined as follows: ``` class CoroutineForeachHelper<Tk, Tv> { private ?array<Tk> $keys; private ?array<Tv> $values; private int $count = 0; private ?Generator<Tk, Tv, mixed> $generator; public function __construct(mixed $collection) { if (CoroutineHelpers::isArray($collection)) { /* HH_FIXME[4110] */ $this->keys = array_keys($collection); /* HH_FIXME[4110] */ $this->values = array_values($collection); $this->count = count($collection); } else { $this->generator = (() ==> { /* HH_FIXME[4110] */ foreach($collection as $key => $value) { yield $key => $value; } })(); $this->generator->rewind(); } } public function key(int $index): Tk { if ($this->keys) { return $this->keys[$index]; } else { invariant($this->generator, "Failed to initialize generator"); return $this->generator->key(); } } public function current(int $index): Tv { if ($this->values) { return $this->values[$index]; } else { invariant($this->generator, "Failed to initialize generator"); return $this->generator->current(); } } public function next(int $index): int { if ($this->generator && $this->generator->valid() && $this->generator) { $this->generator->next(); } return $index + 1; } public function valid(int $index): bool { if ($this->generator) { return $this->generator->valid(); } else { return $index < $this->count; } } } ``` It is necessary to perform this rewriting first, as the if/elseif blocks and the while blocks are rewritten by `rewrite_if` and `rewrite_while`. Additionally, we only perform this rewrite if there exists a `suspend` call within the `foreach` body to avoid any unnecessary rewriting and reduce the risk of semantic error. Currently, we generate a unique `$__iterator_#` and `$__iterator_index_#` per `foreach`. These may be reused in a future optimization. Additionally, when we save `$__iterator_#` onto the closure class, it is saved as `$saved___iterator_#`, which does not affect correctness, but can be addressed in a later diff. Added a couple simple tests into `/foreach_tests/` that compares coroutine `foreach` translations to default `foreach` behavior Added the `CoroutineForeachHelper` class to classes.hhi Reviewed By: michaeltingley Differential Revision: D6800854 fbshipit-source-id: 4e8297f8296f0aa6e79f08eaac171dfbe430c5f7
- Loading branch information