[PHP-DEV] Needs Feedback - Yield without value in reference generator function does not create notice

Mailing Lists php.internals
1

Hello everyone,

In this issue #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: 1. Should we continue with adding the missing notice and merge it also in master? 1. Should we deprecate / remove the usage of yield without value in reference generator functions? - Aggelos Bellos ```
2

Le 26 nov. 2024 à 01:49, aggelos bellos aggelosbellos7@gmail.com a écrit :

Hello everyone,

In this issue #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: 1. Should we continue with adding the missing notice and merge it also in master? 1. 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

3

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 <Issues · php/php-src · GitHub; 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&gt; 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

4

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.