Make ReflectionGenerator::getFunction() legal after generator termination

[TimWolla]

Currently after termination, a Generator loses all its information, except for the return value (if any). This makes it hard to impossible to introspect where that return value is coming from, for example in debuggers.

This PR adds an explicit zend_function* to the struct _zend_generator to allow the retrieval of the function even after termination. This new pointer allows ReflectionGenerator::getFunction() to work correctly at all times. As a side-benefit I was able to add the function name to the __debugInfo(), making it more easily available without going through Reflection, allowing users to distinguish different Generators at a glance.

[TimWolla]

Draft while investigating the ASAN failures. Didn't catch them locally, due to --asan missing from my run-tests.php call.

[Girgias]

I think this makes sense

[dstogov]

As a side-benefit I was able to add the function name to the __debugInfo(), making it more easily available without going through Reflection, allowing users to distinguish different Generators at a glance.

I'm not sure if this makes any benefits, but this is a behavior change and should go through RFC.

Change to ReflectionGenerator::getFunction() may be considered as a fix.

[dstogov]

The patch leads to crashes on few tests.

It's not possible to directly instantiate the Generator class [Zend/tests/generators/errors/generator_instantiate_error.phpt]
Try to instantiate all classes without arguments [Zend/tests/instantiate_all_classes.phpt]
Bug #64007 (There is an ability to create instance of Generator by hand) [ext/reflection/tests/bug64007.phpt]

[TimWolla]

The patch leads to crashes on few tests.

Now fixed, thank you. This was a regression after Ilija's review that I didn't catch locally, because the tests do not contain a yield (which is what I used to filter for Generator tests).

[TimWolla]

I'm not sure if this makes any benefits, but this is a behavior change and should go through RFC.

I believe the addition of a new __debugInfo() handler should fall under the “small self-contained feature” policy. I'm happy to write an RFC if you believe that one is necessary, but I feel that this would unnecessarily take up the voter’s time for such a simple change. Please confirm if I should proceed with an RFC, then I'll split out the __debugInfo() change into a separate PR and leave this one with just the ReflectionGenerator::getFunction() fix.

[dstogov]

I'm not sure if this makes any benefits, but this is a behavior change and should go through RFC.

I believe the addition of a new __debugInfo() handler should fall under the “small self-contained feature” policy. I'm happy to write an RFC if you believe that one is necessary, but I feel that this would unnecessarily take up the voter’s time for such a simple change. Please confirm if I should proceed with an RFC, then I'll split out the __debugInfo() change into a separate PR and leave this one with just the ReflectionGenerator::getFunction() fix.

I wouldn't consider this as a self-contained change, because it is exposed to user (var_dump output).
I don't care about this a lot, and I won't object if other core developers would support this change.

[iluuu1994]

I think this should also adjust getExecutingFile, which doesn't need to rely on execute_data anymore.

This looks good to me otherwise. I don't think changes like these should require RFCs. Adding something to the debug output is barely a change. The technical aspects would be better evaluated by a technical committee than internals, but that proposal was rejected.

[TimWolla]

I think this should also adjust getExecutingFile, which doesn't need to rely on execute_data anymore.

I've considered that, but I'm not sure how useful that is without getExecutingLine(). The naming also implies that the generator must currently be executing, which it no longer is once it terminated.

The filename would also already be available via $r->getFunction()->getFileName().

[iluuu1994]

I think, it's better to allow than to throw when there's no technical reason. The function name is just a bit of a misnomer. But I don't care too much.

[TimWolla]

The function name is just a bit of a misnomer.

At least getExecutingLine() follows the generator’s actual execution:

https://3v4l.org/WYp9l

While of course a single generator can’t execute in different files, it makes sense to me that getExecutingFile() is a companion to getExecutingLine(), rather than a getDefiningFile().

[bwoebi]

@dstogov I don't think this change is significant enough to require a RFC and consensus is fine.

I support this change, provides a bit more visibility.

[nielsdos]

I feel like this PR could provide an (in)direct way of solving #11591
But I'd have to check after merging if we can utilise your functionality.

This changes debug info output, but debug info output changes have happened before without requiring an RFC. At worst, some test cases would have to be adjusted so unless there's complaints I don't think this needs an RFC.

And I agree with Tim about the getExecutingLine() / getExecutingFile() thing.

[dstogov]

I don't object

[alexandre-daubois]

This PR break Symfony CI because it relies on a ReflectionException here:

https://github.com/symfony/symfony/blob/2e0dafae4d20c39580b4ac0df9e574a6182e5a72/src/Symfony/Component/VarDumper/Caster/ReflectionCaster.php#L81-L93

Creating a ReflectionGenerator on a closed generator throws prior to this change, which isn't the case any more. Is it considered a breaking change in userland?

[iluuu1994]

@alexandre-daubois Thanks for letting us know. What is the result of the test? Does castGenerator now work for these cases too? Or does it behave unexpectedly?

[alexandre-daubois]

Here is the update I proposed to the framework: https://github.com/symfony/symfony/pull/57191/files

As you can see, the dump format has changed (which is fine). castGenerator() used to throw at new ReflectionGenerator() when the generator is closed. The vardumper would then display closed: true and that's it.

With this PR, instantiating a ReflectionGenerator on a closed generator doesn't throw anymore. castReflectionGenerator() can now be executed (which wasn't executed when new ReflectionGenerator() threw), making the script fail at this line: https://github.com/symfony/symfony/blob/7ddc5ece5788a54345c494948b1ca775e821ef11/src/Symfony/Component/VarDumper/Caster/ReflectionCaster.php#L137. The message is something like Cannot gather information on a closed generator.

This is why I moved the call to self::castReflectionGenerator() in the try block.

[TimWolla]

@alexandre-daubois Would an explicit ReflectionGenerator::isFinished() (or similar) be helpful to you?

[alexandre-daubois]

It would be a nice addition for sure! Definitely a good idea, cleaner than checking for an exception. I think in the case of this Symfony class, it could help to refactor a bit but the patch I proposed actually works well.
But if someone relies on an ReflectionException as well in its code to know if a generator is closed, going from 8.3 to 8.4 will break the application.