Repository: twigphp/Twig (php)

Issue:
loop.last with generator should throw an exception
Hello,

I painfully discovered that `loop.last` is indeed impossible to access when your looping over a generator (after a couple of hours of painful debug), but I was wondering if there's any possibility for Twig to instead throw an exception when people are accessing to `loop.last` which is currently "undefined" (obviously) to make the error clearer about WHY is `loop.last` undefined :)

Pull request:
Introduce a Loop object
Fixes #4096

Patch:
diff --git a/CHANGELOG b/CHANGELOG
index c7569ba9..b745ff72 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,4 +1,5 @@
 # 4.0.0 (2024-XX-XX)
 
+ * Change the compilation of `for` loops to throw an exception when a `loop.*` variable is not defined
  * Make `Environment::getGlobals()` private
  * Drop support for PHP < 8.2
diff --git a/doc/tags/for.rst b/doc/tags/for.rst
index 656d9c07..3caee8dd 100644
--- a/doc/tags/for.rst
+++ b/doc/tags/for.rst
@@ -76,9 +76,9 @@ Variable              Description
 
 .. note::
 
-    The ``loop.length``, ``loop.revindex``, ``loop.revindex0``, and
-    ``loop.last`` variables are only available for PHP arrays, or objects that
-    implement the ``Countable`` interface.
+    When the underlying PHP iterator is not countable, the ``loop.length``,
+    ``loop.revindex``, ``loop.revindex0``, and ``loop.last`` variables are not
+    available and a ``RuntimeException`` is thrown if you try to use them.
 
 The ``else`` Clause
 -------------------
diff --git a/src/Extension/CoreExtension.php b/src/Extension/CoreExtension.php
index 4d7ab7bd..6bff448c 100644
--- a/src/Extension/CoreExtension.php
+++ b/src/Extension/CoreExtension.php
@@ -1259,18 +1259,6 @@ final class CoreExtension extends AbstractExtension
         return $template->$method(...$args);
     }
 
-    /**
-     * @internal
-     */
-    public static function ensureTraversable($seq)
-    {
-        if (is_iterable($seq)) {
-            return $seq;
-        }
-
-        return [];
-    }
-
     /**
      * @internal
      */
diff --git a/src/Node/ForLoopNode.php b/src/Node/ForLoopNode.php
deleted file mode 100644
index 503687c2..00000000
--- a/src/Node/ForLoopNode.php
+++ /dev/null
@@ -1,51 +0,0 @@
-<?php
-
-/*
- * This file is part of Twig.
- *
- * (c) Fabien Potencier
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Twig\Node;
-
-use Twig\Attribute\YieldReady;
-use Twig\Compiler;
-
-/**
- * Internal node used by the for node.
- *
- * @author Fabien Potencier <fabien@symfony.com>
- */
-#[YieldReady]
-class ForLoopNode extends Node
-{
-    public function __construct(int $lineno, ?string $tag = null)
-    {
-        parent::__construct([], ['with_loop' => false, 'ifexpr' => false, 'else' => false], $lineno, $tag);
-    }
-
-    public function compile(Compiler $compiler): void
-    {
-        if ($this->getAttribute('else')) {
-            $compiler->write("\$context['_iterated'] = true;\n");
-        }
-
-        if ($this->getAttribute('with_loop')) {
-            $compiler
-                ->write("++\$context['loop']['index0'];\n")
-                ->write("++\$context['loop']['index'];\n")
-                ->write("\$context['loop']['first'] = false;\n")
-                ->write("if (isset(\$context['loop']['length'])) {\n")
-                ->indent()
-                ->write("--\$context['loop']['revindex0'];\n")
-                ->write("--\$context['loop']['revindex'];\n")
-                ->write("\$context['loop']['last'] = 0 === \$context['loop']['revindex0'];\n")
-                ->outdent()
-                ->write("}\n")
-            ;
-        }
-    }
-}
diff --git a/src/Node/ForNode.php b/src/Node/ForNode.php
index 49f4e863..c0b8dfd3 100644
--- a/src/Node/ForNode.php
+++ b/src/Node/ForNode.php
@@ -25,12 +25,8 @@ use Twig\Node\Expression\AssignNameExpression;
 #[YieldReady]
 class ForNode extends Node
 {
-    private ForLoopNode $loop;
-
     public function __construct(AssignNameExpression $keyTarget, AssignNameExpression $valueTarget, AbstractExpression $seq, ?Node $ifexpr, Node $body, ?Node $else, int $lineno, ?string $tag = null)
     {
-        $body = new Node([$body, $this->loop = new ForLoopNode($lineno, $tag)]);
-
         $nodes = ['key_target' => $keyTarget, 'value_target' => $valueTarget, 'seq' => $seq, 'body' => $body];
         if (null !== $else) {
             $nodes['else'] = $else;
@@ -41,43 +37,22 @@ class ForNode extends Node
 
     public function compile(Compiler $compiler): void
     {
+        $loopName = $compiler->getVarName();
+
         $compiler
             ->addDebugInfo($this)
             ->write("\$context['_parent'] = \$context;\n")
-            ->write("\$context['_seq'] = CoreExtension::ensureTraversable(")
+            ->write("\$$loopName = new \Twig\Runtime\Loop(")
             ->subcompile($this->getNode('seq'))
-            ->raw(");\n")
+            ->raw(", \$context['_parent']);\n")
         ;
 
-        if ($this->hasNode('else')) {
-            $compiler->write("\$context['_iterated'] = false;\n");
-        }
-
         if ($this->getAttribute('with_loop')) {
-            $compiler
-                ->write("\$context['loop'] = [\n")
-                ->write("  'parent' => \$context['_parent'],\n")
-                ->write("  'index0' => 0,\n")
-                ->write("  'index'  => 1,\n")
-                ->write("  'first'  => true,\n")
-                ->write("];\n")
-                ->write("if (is_array(\$context['_seq']) || (is_object(\$context['_seq']) && \$context['_seq'] instanceof \Countable)) {\n")
-                ->indent()
-                ->write("\$length = count(\$context['_seq']);\n")
-                ->write("\$context['loop']['revindex0'] = \$length - 1;\n")
-                ->write("\$context['loop']['revindex'] = \$length;\n")
-                ->write("\$context['loop']['length'] = \$length;\n")
-                ->write("\$context['loop']['last'] = 1 === \$length;\n")
-                ->outdent()
-                ->write("}\n")
-            ;
+            $compiler->write("\$context['loop'] = new \Twig\Runtime\LoopContext(\${$loopName});\n");
         }
 
-        $this->loop->setAttribute('else', $this->hasNode('else'));
-        $this->loop->setAttribute('with_loop', $this->getAttribute('with_loop'));
-
         $compiler
-            ->write("foreach (\$context['_seq'] as ")
+            ->write("foreach (\$$loopName as ")
             ->subcompile($this->getNode('key_target'))
             ->raw(' => ')
             ->subcompile($this->getNode('value_target'))
@@ -90,7 +65,7 @@ class ForNode extends Node
 
         if ($this->hasNode('else')) {
             $compiler
-                ->write("if (!\$context['_iterated']) {\n")
+                ->write("if (!\${$loopName}->iterated()) {\n")
                 ->indent()
                 ->subcompile($this->getNode('else'))
                 ->outdent()
@@ -101,7 +76,7 @@ class ForNode extends Node
         $compiler->write("\$_parent = \$context['_parent'];\n");
 
         // remove some "private" loop variables (needed for nested loops)
-        $compiler->write('unset($context[\'_seq\'], $context[\'_iterated\'], $context[\''.$this->getNode('key_target')->getAttribute('name').'\'], $context[\''.$this->getNode('value_target')->getAttribute('name').'\'], $context[\'_parent\'], $context[\'loop\']);'."\n");
+        $compiler->write('unset($context[\''.$this->getNode('key_target')->getAttribute('name').'\'], $context[\''.$this->getNode('value_target')->getAttribute('name').'\'], $context[\'_parent\'], $context[\'loop\']);'."\n");
 
         // keep the values set in the inner context for variables defined in the outer context
         $compiler->write("\$context = array_intersect_key(\$context, \$_parent) + \$_parent;\n");
diff --git a/src/Runtime/Loop.php b/src/Runtime/Loop.php
new file mode 100644
index 00000000..ab25bc84
--- /dev/null
+++ b/src/Runtime/Loop.php
@@ -0,0 +1,128 @@
+<?php
+
+/*
+ * This file is part of Twig.
+ *
+ * (c) Fabien Potencier
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Twig\Runtime;
+
+use Twig\Error\RuntimeError;
+
+/**
+ * Represents a for loop variable.
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ *
+ * @internal
+ */
+final class Loop implements \Iterator
+{
+    private \Iterator $seq;
+    private bool $iterated;
+    private int $index0;
+    private int $revindex0;
+    private bool $first;
+    private bool $last;
+    private int $length;
+
+    public function __construct($seq, private $parent)
+    {
+        $this->seq = is_iterable($seq) ? (is_array($seq) ? new \ArrayIterator($seq) : $seq) : new \ArrayIterator([]);
+        $this->rewind();
+    }
+
+    public function current(): mixed
+    {
+        return $this->seq->current();
+    }
+
+    public function key(): mixed
+    {
+        return $this->seq->key();
+    }
+
+    public function next(): void
+    {
+        $this->seq->next();
+
+        $this->iterated = true;
+        ++$this->index0;
+        $this->first = false;
+        if (isset($this->length)) {
+            --$this->revindex0;
+            $this->last = 0 === $this->revindex0;
+        }
+    }
+
+    public function rewind(): void
+    {
+        $this->seq->rewind();
+
+        $this->iterated = false;
+        $this->index0 = 0;
+        $this->first = true;
+        if ($this->seq instanceof \Countable) {
+            $length = count($this->seq);
+            $this->revindex0 = $length - 1;
+            $this->length = $length;
+            $this->last = 1 === $length;
+        }
+    }
+
+    public function valid(): bool
+    {
+        return $this->seq->valid();
+    }
+
+    public function iterated(): bool
+    {
+        return $this->iterated;
+    }
+
+    public function getParent(): mixed
+    {
+        return $this->parent;
+    }
+
+    public function getRevindex0(): int
+    {
+        if (!$this->seq instanceof \Countable) {
+            throw new RuntimeError('The "loop.revindex0" variable is not defined as the loop iterates on a non-countable iterator.');
+        }
+
+        return $this->revindex0;
+    }
+
+    public function getIndex0(): int
+    {
+        return $this->index0;
+    }
+
+    public function getLength(): int
+    {
+        if (!$this->seq instanceof \Countable) {
+            throw new RuntimeError('The "loop.length" variable is not defined as the loop iterates on a non-countable iterator.');
+        }
+
+        return $this->length;
+    }
+
+    public function isFirst(): bool
+    {
+        return $this->first;
+    }
+
+    public function isLast(): bool
+    {
+        if (!$this->seq instanceof \Countable) {
+            throw new RuntimeError('The "loop.last" variable is not defined as the loop iterates on a non-countable iterator.');
+        }
+
+        return $this->last;
+    }
+}
diff --git a/src/Runtime/LoopContext.php b/src/Runtime/LoopContext.php
new file mode 100644
index 00000000..facd3062
--- /dev/null
+++ b/src/Runtime/LoopContext.php
@@ -0,0 +1,66 @@
+<?php
+
+/*
+ * This file is part of Twig.
+ *
+ * (c) Fabien Potencier
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Twig\Runtime;
+
+/**
+ * Represents a for loop context variable.
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ *
+ * @internal
+ */
+final class LoopContext
+{
+    public function __construct(private Loop $loop)
+    {
+    }
+
+    public function getParent(): mixed
+    {
+        return $this->loop->getParent();
+    }
+
+    public function getRevindex0(): int
+    {
+        return $this->loop->getRevindex0();
+    }
+
+    public function getRevindex(): int
+    {
+        return $this->loop->getRevindex0() + 1;
+    }
+
+    public function getIndex0(): int
+    {
+        return $this->loop->getIndex0();
+    }
+
+    public function getIndex(): int
+    {
+        return $this->getIndex0() + 1;
+    }
+
+    public function getLength(): int
+    {
+        return $this->loop->getLength();
+    }
+
+    public function isFirst(): bool
+    {
+        return $this->loop->isFirst();
+    }
+
+    public function isLast(): bool
+    {
+        return $this->loop->isLast();
+    }
+}


Test patch:
diff --git a/tests/Fixtures/tags/for/generator.test b/tests/Fixtures/tags/for/generator.test
new file mode 100644
index 00000000..7e9bdbfd
--- /dev/null
+++ b/tests/Fixtures/tags/for/generator.test
@@ -0,0 +1,12 @@
+--TEST--
+"for" tag supports Generators
+--TEMPLATE--
+{% for item in items %}
+{{ item }}
+{% endfor %}
+--DATA--
+return ['items' => (function () { yield 'a'; yield 'b'; yield 'c'; })()]
+--EXPECT--
+a
+b
+c
diff --git a/tests/Fixtures/tags/for/non_countable.test b/tests/Fixtures/tags/for/non_countable.test
new file mode 100644
index 00000000..433e2c2e
--- /dev/null
+++ b/tests/Fixtures/tags/for/non_countable.test
@@ -0,0 +1,19 @@
+--TEST--
+"for" tag loop variable throws when last is used on a non-countable
+--TEMPLATE--
+{% for item in items %}
+  * {{ loop.last }}
+{% endfor %}
+--DATA--
+class ItemsIteratorNotCountable implements \Iterator
+{
+  protected $values = ['foo' => 'bar', 'bar' => 'foo'];
+  public function current(): mixed { return current($this->values); }
+  public function key(): mixed { return key($this->values); }
+  public function next(): void { next($this->values); }
+  public function rewind(): void { reset($this->values); }
+  public function valid(): bool { return false !== current($this->values); }
+}
+return ['items' => new ItemsIteratorNotCountable()]
+--EXCEPTION--
+Twig\Error\RuntimeError: The "loop.last" variable is not defined as the loop iterates on a non-countable iterator in "index.twig" at line 3.
diff --git a/tests/Node/ForTest.php b/tests/Node/ForTest.php
index 3ca4b22a..1a103aff 100644
--- a/tests/Node/ForTest.php
+++ b/tests/Node/ForTest.php
@@ -33,7 +33,7 @@ class ForTest extends NodeTestCase
         $this->assertEquals($keyTarget, $node->getNode('key_target'));
         $this->assertEquals($valueTarget, $node->getNode('value_target'));
         $this->assertEquals($seq, $node->getNode('seq'));
-        $this->assertEquals($body, $node->getNode('body')->getNode('0'));
+        $this->assertEquals($body, $node->getNode('body'));
         $this->assertFalse($node->hasNode('else'));
 
         $else = new PrintNode(new NameExpression('foo', 1), 1);
@@ -57,12 +57,12 @@ class ForTest extends NodeTestCase
         $tests[] = [$node, <<<EOF
 // line 1
 \$context['_parent'] = \$context;
-\$context['_seq'] = CoreExtension::ensureTraversable({$this->getVariableGetter('items')});
-foreach (\$context['_seq'] as \$context["key"] => \$context["item"]) {
+\$__internal_compile_0 = new \Twig\Runtime\Loop({$this->getVariableGetter('items')}, \$context['_parent']);
+foreach (\$__internal_compile_0 as \$context["key"] => \$context["item"]) {
     yield {$this->getVariableGetter('foo')};
 }
 \$_parent = \$context['_parent'];
-unset(\$context['_seq'], \$context['_iterated'], \$context['key'], \$context['item'], \$context['_parent'], \$context['loop']);
+unset(\$context['key'], \$context['item'], \$context['_parent'], \$context['loop']);
 \$context = array_intersect_key(\$context, \$_parent) + \$_parent;
 EOF
         ];
@@ -78,33 +78,13 @@ EOF
         $tests[] = [$node, <<<EOF
 // line 1
 \$context['_parent'] = \$context;
-\$context['_seq'] = CoreExtension::ensureTraversable({$this->getVariableGetter('values')});
-\$context['loop'] = [
-  'parent' => \$context['_parent'],
-  'index0' => 0,
-  'index'  => 1,
-  'first'  => true,
-];
-if (is_array(\$context['_seq']) || (is_object(\$context['_seq']) && \$context['_seq'] instanceof \Countable)) {
-    \$length = count(\$context['_seq']);
-    \$context['loop']['revindex0'] = \$length - 1;
-    \$context['loop']['revindex'] = \$length;
-    \$context['loop']['length'] = \$length;
-    \$context['loop']['last'] = 1 === \$length;
-}
-foreach (\$context['_seq'] as \$context["k"] => \$context["v"]) {
+\$__internal_compile_0 = new \Twig\Runtime\Loop({$this->getVariableGetter('values')}, \$context['_parent']);
+\$context['loop'] = new \Twig\Runtime\LoopContext(\$__internal_compile_0);
+foreach (\$__internal_compile_0 as \$context["k"] => \$context["v"]) {
     yield {$this->getVariableGetter('foo')};
-    ++\$context['loop']['index0'];
-    ++\$context['loop']['index'];
-    \$context['loop']['first'] = false;
-    if (isset(\$context['loop']['length'])) {
-        --\$context['loop']['revindex0'];
-        --\$context['loop']['revindex'];
-        \$context['loop']['last'] = 0 === \$context['loop']['revindex0'];
-    }
 }
 \$_parent = \$context['_parent'];
-unset(\$context['_seq'], \$context['_iterated'], \$context['k'], \$context['v'], \$context['_parent'], \$context['loop']);
+unset(\$context['k'], \$context['v'], \$context['_parent'], \$context['loop']);
 \$context = array_intersect_key(\$context, \$_parent) + \$_parent;
 EOF
         ];
@@ -120,33 +100,13 @@ EOF
         $tests[] = [$node, <<<EOF
 // line 1
 \$context['_parent'] = \$context;
-\$context['_seq'] = CoreExtension::ensureTraversable({$this->getVariableGetter('values')});
-\$context['loop'] = [
-  'parent' => \$context['_parent'],
-  'index0' => 0,
-  'index'  => 1,
-  'first'  => true,
-];
-if (is_array(\$context['_seq']) || (is_object(\$context['_seq']) && \$context['_seq'] instanceof \Countable)) {
-    \$length = count(\$context['_seq']);
-    \$context['loop']['revindex0'] = \$length - 1;
-    \$context['loop']['revindex'] = \$length;
-    \$context['loop']['length'] = \$length;
-    \$context['loop']['last'] = 1 === \$length;
-}
-foreach (\$context['_seq'] as \$context["k"] => \$context["v"]) {
+\$__internal_compile_0 = new \Twig\Runtime\Loop({$this->getVariableGetter('values')}, \$context['_parent']);
+\$context['loop'] = new \Twig\Runtime\LoopContext(\$__internal_compile_0);
+foreach (\$__internal_compile_0 as \$context["k"] => \$context["v"]) {
     yield {$this->getVariableGetter('foo')};
-    ++\$context['loop']['index0'];
-    ++\$context['loop']['index'];
-    \$context['loop']['first'] = false;
-    if (isset(\$context['loop']['length'])) {
-        --\$context['loop']['revindex0'];
-        --\$context['loop']['revindex'];
-        \$context['loop']['last'] = 0 === \$context['loop']['revindex0'];
-    }
 }
 \$_parent = \$context['_parent'];
-unset(\$context['_seq'], \$context['_iterated'], \$context['k'], \$context['v'], \$context['_parent'], \$context['loop']);
+unset(\$context['k'], \$context['v'], \$context['_parent'], \$context['loop']);
 \$context = array_intersect_key(\$context, \$_parent) + \$_parent;
 EOF
         ];
@@ -162,38 +122,16 @@ EOF
         $tests[] = [$node, <<<EOF
 // line 1
 \$context['_parent'] = \$context;
-\$context['_seq'] = CoreExtension::ensureTraversable({$this->getVariableGetter('values')});
-\$context['_iterated'] = false;
-\$context['loop'] = [
-  'parent' => \$context['_parent'],
-  'index0' => 0,
-  'index'  => 1,
-  'first'  => true,
-];
-if (is_array(\$context['_seq']) || (is_object(\$context['_seq']) && \$context['_seq'] instanceof \Countable)) {
-    \$length = count(\$context['_seq']);
-    \$context['loop']['revindex0'] = \$length - 1;
-    \$context['loop']['revindex'] = \$length;
-    \$context['loop']['length'] = \$length;
-    \$context['loop']['last'] = 1 === \$length;
-}
-foreach (\$context['_seq'] as \$context["k"] => \$context["v"]) {
+\$__internal_compile_0 = new \Twig\Runtime\Loop({$this->getVariableGetter('values')}, \$context['_parent']);
+\$context['loop'] = new \Twig\Runtime\LoopContext(\$__internal_compile_0);
+foreach (\$__internal_compile_0 as \$context["k"] => \$context["v"]) {
     yield {$this->getVariableGetter('foo')};
-    \$context['_iterated'] = true;
-    ++\$context['loop']['index0'];
-    ++\$context['loop']['index'];
-    \$context['loop']['first'] = false;
-    if (isset(\$context['loop']['length'])) {
-        --\$context['loop']['revindex0'];
-        --\$context['loop']['revindex'];
-        \$context['loop']['last'] = 0 === \$context['loop']['revindex0'];
-    }
 }
-if (!\$context['_iterated']) {
+if (!\$__internal_compile_0->iterated()) {
     yield {$this->getVariableGetter('foo')};
 }
 \$_parent = \$context['_parent'];
-unset(\$context['_seq'], \$context['_iterated'], \$context['k'], \$context['v'], \$context['_parent'], \$context['loop']);
+unset(\$context['k'], \$context['v'], \$context['_parent'], \$context['loop']);
 \$context = array_intersect_key(\$context, \$_parent) + \$_parent;
 EOF
         ];