Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Needs Feedback - Yield without value in reference generator function does not create notice »

Needs Feedback - Yield without value in reference generator function does not create notice

php.internals

aggelos bellos

1 year ago
Hello everyone, In this issue #16761 <https://github.com/php/php-src/issues/16761> it was verified that in the above code it should throw a notice for both yields: ``` <?phperror_reporting(E_ALL);function &y() { yield null; // warning yield; // no warning, agreed to be a bug }foreach (y() as &$y); ``` I have created this PR <https://github.com/php/php-src/pull/16882> which adds the "missing" notice but we aren't sure on how to continue on this. There are 2 main questions: 1. Should we continue with adding the missing notice and merge it also in master? 2. Should we deprecate / remove the usage of yield without value in reference generator functions? - Aggelos Bellos

Claude Pache

1 year ago
> Le 26 nov. 2024 à 01:49, aggelos bellos <aggelosbellos7@gmail.com> a écrit : > > Hello everyone, > > In this issue #16761 <https://github.com/php/php-src/issues/16761> it was verified that in the above code it should throw a notice for both yields: > ``` > <?php > error_reporting(E_ALL); > function &y() > { > yield null; // warning > yield; // no warning, agreed to be a bug > } > foreach (y() as &$y); > ``` > I have created this PR <https://github.com/php/php-src/pull/16882> which adds the "missing" notice but we aren't sure on > how to continue on this. > There are 2 main questions: > Should we continue with adding the missing notice and merge it also in master? > Should we deprecate / remove the usage of yield without value in reference generator functions? > > - Aggelos Bellos
Hi, A notice would be consistent with what happens with: `function &f() { return; }; f();`. But don’t add new notices in patch releases, otherwise you may break without warning a non-trivial amount of code that conflates notices with fatal errors. The bug is not serious enough to warrant such a risk. Whether it should be deprecated, is a separate question. You should also consider what to do with: `function &g() { yield null; }`, `function &g() { if (false) yield; }`, `function &f() { return; }`, etc. —Claude

Christoph Becker

1 year ago
On 26.11.2024 at 12:31, Claude Pache wrote:
>> Le 26 nov. 2024 à 01:49, aggelos bellos <aggelosbellos7@gmail.com> a écrit : >> >> In this issue #16761 <https://github.com/php/php-src/issues/16761> it was verified that in the above code it should throw a notice for both yields: >> ``` >> <?php >> error_reporting(E_ALL); >> function &y() >> { >> yield null; // warning >> yield; // no warning, agreed to be a bug >> } >> foreach (y() as &$y); >> ``` >> I have created this PR <https://github.com/php/php-src/pull/16882> which adds the "missing" notice but we aren't sure on >> how to continue on this. >> There are 2 main questions: >> Should we continue with adding the missing notice and merge it also in master? >> Should we deprecate / remove the usage of yield without value in reference generator functions? > > A notice would be consistent with what happens with: `function &f() { return; }; f();`. But don’t add new notices in patch releases, otherwise you may break without warning a non-trivial amount of code that conflates notices with fatal errors. The bug is not serious enough to warrant such a risk. > > Whether it should be deprecated, is a separate question. You should also consider what to do with: `function &g() { yield null; }`, `function &g() { if (false) yield; }`, `function &f() { return; }`, etc.
In my opinion, whether it should be deprecated is not a separate question, but the one we should answer first. If we're not going to deprecate/remove the auto-conversion to a reference, we may as well drop the notice altogether, instead of adding a further notice. Christoph

aggelos bellos

1 year ago
> > > A notice would be consistent with what happens with: `function &f() { > return; }; f();`. But don’t add new notices in patch releases, otherwise > you may break without warning a non-trivial amount of code that conflates > notices with fatal errors. The bug is not serious enough to warrant such a > risk. >
But isn't a bug that the notice is missing in the first place? In my opinion, whether it should be deprecated is not a separate
> question, but the one we should answer first. If we're not going to > deprecate/remove the auto-conversion to a reference, we may as well drop > the notice altogether, instead of adding a further notice. >
Hmm I am not exactly sure if I agree with you. Whatever we decide, it will happen in a new version. It feels right to fix the current supported versions. Regarding the deprecation decision, I checked the RFC https://wiki.php.net/rfc/generators and I found this line: *Only generators specifying the & modifier can be iterated by ref. If you
> try to iterate a non-ref generator by-ref an E_ERROR is thrown.*
Correct me if I am wrong, but this phrase refers to our case. If I understand this correctly it should normally throw an E_ERROR so we should continue perhaps with the deprecation? Other than this, I don't have a strong opinion on if we should deprecate or not this case.