Make ReflectionGenerator::getFunction() legal after generator termination (#14167)

* Make `ReflectionGenerator::getFunction()` legal after generator termination

* Expose the generator function name via `Generator::__debugInfo()`

* Allow creating `ReflectionGenerator` after termination

* Reorder `struct _zend_generator` to avoid a hole

* Adjust `ext/reflection/tests/028.phpt`

This is legal now.

* Fix Generator Closure collection

* Add test to verify the Closure dies with the generator

* NEWS / UPGRADING

NEWS

@@ -207,6 +207,8 @@ PHP                                                                        NEWS
 - Reflection:
   . Implement GH-12908 (Show attribute name/class in ReflectionAttribute dump).
     (nielsdos)
+  . Make ReflectionGenerator::getFunction() legal after generator termination.
+    (timwolla)
 
 - SimpleXML:
   . Fixed bug GH-12192 (SimpleXML infinite loop when getName() is called

UPGRADING

@@ -437,6 +437,10 @@ PHP 8.4 UPGRADE NOTES
   . posix_isatty now sets the error number when the file descriptor/stream argument
     is invalid.
 
+- Reflection:
+  . ReflectionGenerator::getFunction() may now be called after the generator
+    finished executing.
+
 - Sockets:
   . Parameter $backlog of socket_create_listen() now has a default value of SOMAXCONN.
     Previously, it was 128.

Zend/tests/generators/debugInfo_001.phpt

@@ -0,0 +1,82 @@
+--TEST--
+Generators expose the underlying function name in __debugInfo().
+--FILE--
+<?php
+
+function foo() {
+    yield;
+}
+
+$gens = [
+    (new class() {
+        function a() {
+            yield from foo();
+        }
+    })->a(),
+    (function() {
+        yield;
+    })(),
+    foo(),
+];
+
+foreach ($gens as $gen) {
+    echo "Before:", PHP_EOL;
+    var_dump($gen);
+
+    foreach ($gen as $dummy) {
+        echo "Inside:", PHP_EOL;
+        var_dump($gen);
+    }
+
+    echo "After:", PHP_EOL;
+
+    var_dump($gen);
+}
+
+?>
+--EXPECTF--
+Before:
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(%d) "class@anonymous%s::a"
+}
+Inside:
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(%d) "class@anonymous%s::a"
+}
+After:
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(%d) "class@anonymous%s::a"
+}
+Before:
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(%d) "{closure:%s:%d}"
+}
+Inside:
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(%d) "{closure:%s:%d}"
+}
+After:
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(%d) "{closure:%s:%d}"
+}
+Before:
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(3) "foo"
+}
+Inside:
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(3) "foo"
+}
+After:
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(3) "foo"
+}

Zend/tests/generators/gc_with_yield_from.phpt

@@ -27,21 +27,29 @@ gc_collect_cycles();
 print "end\n";
 
 ?>
---EXPECT--
+--EXPECTF--
 int(1)
 collect
 array(4) {
   [0]=>
-  object(Generator)#1 (0) {
+  object(Generator)#%d (1) {
+    ["function"]=>
+    string(3) "gen"
   }
   [1]=>
-  object(Generator)#2 (0) {
+  object(Generator)#%d (1) {
+    ["function"]=>
+    string(3) "gen"
   }
   [2]=>
-  object(Generator)#3 (0) {
+  object(Generator)#%d (1) {
+    ["function"]=>
+    string(3) "gen"
   }
   [3]=>
-  object(Generator)#4 (0) {
+  object(Generator)#%d (1) {
+    ["function"]=>
+    string(4) "root"
   }
 }
 end

Zend/tests/generators/generator_closure_collection.phpt

@@ -0,0 +1,47 @@
+--TEST--
+The Closure object of a generator is freed when the generator is freed.
+--FILE--
+<?php
+
+$genFactory = function() {
+  yield 1;
+  yield 2;
+  yield 3;
+};
+
+$r = WeakReference::create($genFactory);
+$generator = $genFactory();
+unset($genFactory);
+
+var_dump($r->get());
+
+foreach ($generator as $value) var_dump($value);
+
+var_dump($r->get());
+
+unset($generator);
+
+var_dump($r->get());
+
+?>
+--EXPECTF--
+object(Closure)#%d (3) {
+  ["name"]=>
+  string(%d) "{closure:%s:%d}"
+  ["file"]=>
+  string(%d) "%s"
+  ["line"]=>
+  int(%d)
+}
+int(1)
+int(2)
+int(3)
+object(Closure)#%d (3) {
+  ["name"]=>
+  string(%d) "{closure:%s:%d}"
+  ["file"]=>
+  string(%d) "%s"
+  ["line"]=>
+  int(%d)
+}
+NULL

Zend/tests/generators/generator_return_without_value.phpt

@@ -33,11 +33,19 @@ var_dump(gen4());
 
 ?>
 --EXPECTF--
-object(Generator)#%d (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(3) "gen"
 }
-object(Generator)#%d (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(4) "gen2"
 }
-object(Generator)#%d (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(4) "gen3"
 }
-object(Generator)#%d (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(4) "gen4"
 }

Zend/tests/return_types/generators001.phpt

@@ -42,16 +42,30 @@ var_dump(
 ?>
 --EXPECTF--
 object(Generator)#%d (%d) {
+  ["function"]=>
+  string(5) "test1"
 }
 object(Generator)#%d (%d) {
+  ["function"]=>
+  string(5) "test2"
 }
 object(Generator)#%d (%d) {
+  ["function"]=>
+  string(5) "test3"
 }
 object(Generator)#%d (%d) {
+  ["function"]=>
+  string(5) "test4"
 }
 object(Generator)#%d (%d) {
+  ["function"]=>
+  string(5) "test5"
 }
 object(Generator)#%d (%d) {
+  ["function"]=>
+  string(5) "test6"
 }
 object(Generator)#%d (%d) {
+  ["function"]=>
+  string(5) "test7"
 }

Zend/tests/return_types/generators005.phpt

@@ -19,4 +19,6 @@ var_dump($some->getIterator());
 ?>
 --EXPECTF--
 object(Generator)#%d (%d) {
+  ["function"]=>
+  string(27) "SomeCollection::getIterator"
 }

Zend/tests/type_declarations/iterable/iterable_001.phpt

@@ -32,7 +32,9 @@ array(3) {
   [2]=>
   int(3)
 }
-object(Generator)#1 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(3) "gen"
 }
 object(ArrayIterator)#1 (1) {
   ["storage":"ArrayIterator":private]=>

Zend/tests/type_declarations/iterable/iterable_003.phpt

@@ -24,9 +24,11 @@ try {
 }
 
 ?>
---EXPECT--
+--EXPECTF--
 array(0) {
 }
-object(Generator)#2 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(17) "{closure:bar():7}"
 }
 baz(): Return value must be of type Traversable|array, int returned

Zend/tests/type_declarations/union_types/generator_return_containing_extra_types.phpt

@@ -16,6 +16,8 @@ class C implements I {
 var_dump((new C)->test());
 
 ?>
---EXPECT--
-object(Generator)#2 (0) {
+--EXPECTF--
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(7) "C::test"
 }

Zend/zend_generators.c

@@ -166,11 +166,6 @@ ZEND_API void zend_generator_close(zend_generator *generator, bool finished_exec
 			zend_generator_cleanup_unfinished_execution(generator, execute_data, 0);
 		}
 
-		/* Free closure object */
-		if (EX_CALL_INFO() & ZEND_CALL_CLOSURE) {
-			OBJ_RELEASE(ZEND_CLOSURE_OBJECT(EX(func)));
-		}
-
 		efree(execute_data);
 	}
 }
@@ -330,6 +325,10 @@ static void zend_generator_free_storage(zend_object *object) /* {{{ */
 
 	zend_generator_close(generator, 0);
 
+	if (generator->func && (generator->func->common.fn_flags & ZEND_ACC_CLOSURE)) {
+		OBJ_RELEASE(ZEND_CLOSURE_OBJECT(generator->func));
+	}
+
 	/* we can't immediately free them in zend_generator_close() else yield from won't be able to fetch it */
 	zval_ptr_dtor(&generator->value);
 	zval_ptr_dtor(&generator->key);
@@ -354,10 +353,19 @@ static HashTable *zend_generator_get_gc(zend_object *object, zval **table, int *
 	zend_execute_data *call = NULL;
 
 	if (!execute_data) {
-		/* If the generator has been closed, it can only hold on to three values: The value, key
-		 * and retval. These three zvals are stored sequentially starting at &generator->value. */
-		*table = &generator->value;
-		*n = 3;
+		if (UNEXPECTED(generator->func->common.fn_flags & ZEND_ACC_CLOSURE)) {
+			zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create();
+			zend_get_gc_buffer_add_zval(gc_buffer, &generator->value);
+			zend_get_gc_buffer_add_zval(gc_buffer, &generator->key);
+			zend_get_gc_buffer_add_zval(gc_buffer, &generator->retval);
+			zend_get_gc_buffer_add_obj(gc_buffer, ZEND_CLOSURE_OBJECT(generator->func));
+			zend_get_gc_buffer_use(gc_buffer, table, n);
+		} else {
+			/* If the non-closure generator has been closed, it can only hold on to three values: The value, key
+			 * and retval. These three zvals are stored sequentially starting at &generator->value. */
+			*table = &generator->value;
+			*n = 3;
+		}
 		return NULL;
 	}
 
@@ -1010,6 +1018,35 @@ ZEND_METHOD(Generator, getReturn)
 }
 /* }}} */
 
+ZEND_METHOD(Generator, __debugInfo)
+{
+	zend_generator *generator;
+
+	ZEND_PARSE_PARAMETERS_NONE();
+
+	generator = (zend_generator *) Z_OBJ_P(ZEND_THIS);
+
+	array_init(return_value);
+
+	zend_function *func = generator->func;
+
+	zval val;
+	if (func->common.scope) {
+		zend_string *class_name = func->common.scope->name;
+		zend_string *func_name = func->common.function_name;
+		zend_string *combined = zend_string_concat3(
+			ZSTR_VAL(class_name), ZSTR_LEN(class_name),
+			"::", strlen("::"),
+			ZSTR_VAL(func_name), ZSTR_LEN(func_name)
+		);
+		ZVAL_NEW_STR(&val, combined);
+	} else {
+		ZVAL_STR_COPY(&val, func->common.function_name);
+	}
+
+	zend_hash_update(Z_ARR_P(return_value), ZSTR_KNOWN(ZEND_STR_FUNCTION), &val);
+}
+
 /* get_iterator implementation */
 
 static void zend_generator_iterator_dtor(zend_object_iterator *iterator) /* {{{ */

Zend/zend_generators.h

@@ -88,6 +88,10 @@ struct _zend_generator {
 	/* Fake execute_data for stacktraces */
 	zend_execute_data execute_fake;
 
+	/* The underlying function, equivalent to execute_data->func while
+	 * the generator is alive. */
+	zend_function *func;
+
 	/* ZEND_GENERATOR_* flags */
 	uint8_t flags;
 };

Zend/zend_generators.stub.php

@@ -23,6 +23,8 @@ final class Generator implements Iterator
     public function throw(Throwable $exception): mixed {}
 
     public function getReturn(): mixed {}
+
+    public function __debugInfo(): array {}
 }
 
 class ClosedGeneratorException extends Exception

Zend/zend_generators_arginfo.h

@@ -1,5 +1,5 @@
 /* This is a generated file, edit the .stub.php file instead.
- * Stub hash: 0af5e8985dd4645bf23490b8cec312f8fd1fee2e */
+ * Stub hash: d376e984db0db6ccd9356f632f9d7e1382b2afb7 */
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_Generator_rewind, 0, 0, IS_VOID, 0)
 ZEND_END_ARG_INFO()
@@ -24,6 +24,9 @@ ZEND_END_ARG_INFO()
 
 #define arginfo_class_Generator_getReturn arginfo_class_Generator_current
 
+ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_Generator___debugInfo, 0, 0, IS_ARRAY, 0)
+ZEND_END_ARG_INFO()
+
 ZEND_METHOD(Generator, rewind);
 ZEND_METHOD(Generator, valid);
 ZEND_METHOD(Generator, current);
@@ -32,6 +35,7 @@ ZEND_METHOD(Generator, next);
 ZEND_METHOD(Generator, send);
 ZEND_METHOD(Generator, throw);
 ZEND_METHOD(Generator, getReturn);
+ZEND_METHOD(Generator, __debugInfo);
 
 static const zend_function_entry class_Generator_methods[] = {
 	ZEND_ME(Generator, rewind, arginfo_class_Generator_rewind, ZEND_ACC_PUBLIC)
@@ -42,6 +46,7 @@ static const zend_function_entry class_Generator_methods[] = {
 	ZEND_ME(Generator, send, arginfo_class_Generator_send, ZEND_ACC_PUBLIC)
 	ZEND_ME(Generator, throw, arginfo_class_Generator_throw, ZEND_ACC_PUBLIC)
 	ZEND_ME(Generator, getReturn, arginfo_class_Generator_getReturn, ZEND_ACC_PUBLIC)
+	ZEND_ME(Generator, __debugInfo, arginfo_class_Generator___debugInfo, ZEND_ACC_PUBLIC)
 	ZEND_FE_END
 };
 

Zend/zend_vm_def.h

@@ -4528,6 +4528,7 @@ ZEND_VM_HANDLER(139, ZEND_GENERATOR_CREATE, ANY, ANY)
 
 		/* Save execution context in generator object. */
 		generator = (zend_generator *) Z_OBJ_P(EX(return_value));
+		generator->func = gen_execute_data->func;
 		generator->execute_data = gen_execute_data;
 		generator->frozen_call_stack = NULL;
 		generator->execute_fake.opline = NULL;

Zend/zend_vm_execute.h

@@ -2175,6 +2175,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_GENERATOR_CREATE_SPEC_HANDLER(
 
 		/* Save execution context in generator object. */
 		generator = (zend_generator *) Z_OBJ_P(EX(return_value));
+		generator->func = gen_execute_data->func;
 		generator->execute_data = gen_execute_data;
 		generator->frozen_call_stack = NULL;
 		generator->execute_fake.opline = NULL;

ext/reflection/php_reflection.c

@@ -2251,7 +2251,6 @@ ZEND_METHOD(ReflectionGenerator, __construct)
 {
 	zval *generator, *object;
 	reflection_object *intern;
-	zend_execute_data *ex;
 
 	object = ZEND_THIS;
 	intern = Z_REFLECTION_P(object);
@@ -2260,12 +2259,6 @@ ZEND_METHOD(ReflectionGenerator, __construct)
 		RETURN_THROWS();
 	}
 
-	ex = ((zend_generator *) Z_OBJ_P(generator))->execute_data;
-	if (!ex) {
-		_DO_THROW("Cannot create ReflectionGenerator based on a terminated Generator");
-		RETURN_THROWS();
-	}
-
 	if (intern->ce) {
 		zval_ptr_dtor(&intern->obj);
 	}
@@ -2354,22 +2347,20 @@ ZEND_METHOD(ReflectionGenerator, getExecutingFile)
 ZEND_METHOD(ReflectionGenerator, getFunction)
 {
 	zend_generator *generator = (zend_generator *) Z_OBJ(Z_REFLECTION_P(ZEND_THIS)->obj);
-	zend_execute_data *ex = generator->execute_data;
+	zend_function *func = generator->func;
 
 	if (zend_parse_parameters_none() == FAILURE) {
 		RETURN_THROWS();
 	}
 
-	REFLECTION_CHECK_VALID_GENERATOR(ex)
-
-	if (ex->func->common.fn_flags & ZEND_ACC_CLOSURE) {
+	if (func->common.fn_flags & ZEND_ACC_CLOSURE) {
 		zval closure;
-		ZVAL_OBJ(&closure, ZEND_CLOSURE_OBJECT(ex->func));
-		reflection_function_factory(ex->func, &closure, return_value);
-	} else if (ex->func->op_array.scope) {
-		reflection_method_factory(ex->func->op_array.scope, ex->func, NULL, return_value);
+		ZVAL_OBJ(&closure, ZEND_CLOSURE_OBJECT(func));
+		reflection_function_factory(func, &closure, return_value);
+	} else if (func->op_array.scope) {
+		reflection_method_factory(func->op_array.scope, func, NULL, return_value);
 	} else {
-		reflection_function_factory(ex->func, NULL, return_value);
+		reflection_function_factory(func, NULL, return_value);
 	}
 }
 /* }}} */

ext/reflection/tests/028.phpt

@@ -10,11 +10,9 @@ function foo()
 $g = foo();
 $g->next();
 
-try {
-    $r = new ReflectionGenerator($g);
-} catch (ReflectionException $e) {
-    echo "Done!\n";
-}
+$r = new ReflectionGenerator($g);
+var_dump($r);
 ?>
---EXPECT--
-Done!
+--EXPECTF--
+object(ReflectionGenerator)#%d (0) {
+}

ext/reflection/tests/ReflectionGenerator_after_termination.phpt

@@ -0,0 +1,33 @@
+--TEST--
+Creating ReflectionGenerator is legal after termination.
+--FILE--
+<?php
+
+function foo() {
+    yield;
+}
+
+$gens = [
+    (new class() {
+        function a() {
+            yield from foo();
+        }
+    })->a(),
+    (function() {
+        yield;
+    })(),
+    foo(),
+];
+
+foreach ($gens as $gen) {
+    foreach ($gen as $dummy);
+
+    $ref = new ReflectionGenerator($gen);
+    echo $ref->getFunction()->getName(), PHP_EOL;
+}
+
+?>
+--EXPECTF--
+a
+{closure:%s:%d}
+foo

ext/reflection/tests/ReflectionGenerator_basic.phpt

@@ -35,7 +35,9 @@ foreach ($gens as $gen) {
 
 ?>
 --EXPECTF--
-object(Generator)#2 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(%d) "class@anonymous%s::a"
 }
 array(2) {
   [0]=>
@@ -68,7 +70,9 @@ array(2) {
 }
 int(%d)
 string(%d) "%sReflectionGenerator_basic.%s"
-object(Generator)#6 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(3) "foo"
 }
 object(ReflectionMethod)#8 (2) {
   ["name"]=>
@@ -78,7 +82,9 @@ object(ReflectionMethod)#8 (2) {
 }
 object(class@anonymous)#1 (0) {
 }
-object(Generator)#4 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(%d) "{closure:%s:%d}"
 }
 array(1) {
   [0]=>
@@ -92,14 +98,18 @@ array(1) {
 }
 int(%d)
 string(%d) "%sReflectionGenerator_basic.%s"
-object(Generator)#4 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(%d) "{closure:%s:%d}"
 }
 object(ReflectionFunction)#7 (1) {
   ["name"]=>
   string(%d) "{closure:%s:%d}"
 }
 NULL
-object(Generator)#5 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(3) "foo"
 }
 array(1) {
   [0]=>
@@ -113,7 +123,9 @@ array(1) {
 }
 int(%d)
 string(%d) "%sReflectionGenerator_basic.%s"
-object(Generator)#5 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(3) "foo"
 }
 object(ReflectionFunction)#8 (1) {
   ["name"]=>

ext/reflection/tests/ReflectionGenerator_getFunction.phpt

@@ -0,0 +1,44 @@
+--TEST--
+ReflectionGenerator::getFunction() is legal after termination.
+--FILE--
+<?php
+
+function foo() {
+    yield;
+}
+
+$gens = [
+    (new class() {
+        function a() {
+            yield from foo();
+        }
+    })->a(),
+    (function() {
+        yield;
+    })(),
+    foo(),
+];
+
+foreach ($gens as $gen) {
+    $ref = new ReflectionGenerator($gen);
+
+    echo "Before: ", $ref->getFunction()->getName(), PHP_EOL;
+
+    foreach ($gen as $dummy) {
+        echo "Inside: ", $ref->getFunction()->getName(), PHP_EOL;
+    }
+
+    echo "After: ", $ref->getFunction()->getName(), PHP_EOL;
+}
+
+?>
+--EXPECTF--
+Before: a
+Inside: a
+After: a
+Before: {closure:%s:%d}
+Inside: {closure:%s:%d}
+After: {closure:%s:%d}
+Before: foo
+Inside: foo
+After: foo

ext/reflection/tests/ReflectionGenerator_in_Generator.phpt

@@ -45,7 +45,9 @@ array(1) {
 }
 int(%d)
 string(%d) "%sReflectionGenerator_in_Generator.%s"
-object(Generator)#2 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(%d) "{closure:%s:%d}"
 }
 object(ReflectionFunction)#4 (1) {
   ["name"]=>
@@ -76,7 +78,9 @@ array(2) {
 }
 int(%d)
 string(%d) "%sReflectionGenerator_in_Generator.%s"
-object(Generator)#5 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(%d) "{closure:%s:%d}"
 }
 object(ReflectionFunction)#6 (1) {
   ["name"]=>

ext/zend_test/tests/zend_legacy_iterable.phpt

@@ -16,13 +16,17 @@ var_dump(zend_iterable_legacy(gen(), gen()));
 
 ?>
 ==DONE==
---EXPECT--
+--EXPECTF--
 array(0) {
 }
 array(0) {
 }
-object(Generator)#1 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(3) "gen"
 }
-object(Generator)#1 (0) {
+object(Generator)#%d (1) {
+  ["function"]=>
+  string(3) "gen"
 }
 ==DONE==