[RFC] [VOTE] Transform exit() from a language construct into a standard function

10 months ago by Christoph M. Becker — view source

unread

Hi Gina!

I have just opened the vote for the "Transform exit() from a language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

As userland PHP developer, I always regarded exit as a control flow
instruction (quite similar to break), and as such I'm not really in
favor of converting it to a proper function (especially since it is not,
because the parantheses could be omitted).

Would that RFC imply that I would need to write \exit or have a use exit clause to avoid dynamic namespace lookup? If so, I would be even
less in favor of that change.

I do understand your point about the type juggling semantics, but I
might have addressed that differently. I almost always use exit
without argument (and if, only with an int), and die always with a
string (and only for quick experiments), and I figure that this might be
what most contemporary code does (at least, I hope that the
do_something() or die() times have long gone). As such, having exit
and die as alias could be changed, sticking with exit as a control
flow instruction, and having die as proper function (which could even
be implemented in userland), where exit would allow an optional int
argument (like break), and die() a required string argument. Of
course, this would be a much bigger BC break, but it seems to me the
cleaner solution.

Cheers,
Christoph

10 months ago by Gina P. Banyard — view source

unread

Hi Gina!

I have just opened the vote for the "Transform exit() from a language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

As userland PHP developer, I always regarded exit as a control flow
instruction (quite similar to break), and as such I'm not really in
favor of converting it to a proper function (especially since it is not,
because the parantheses could be omitted).

You are not the first person to mention this.
I don't have any plans on doing anything else to exit, but I never really considered it to be a control flow instruction, but that is maybe just me.

Would that RFC imply that I would need to write \\exit or have a use exit clause to avoid dynamic namespace lookup? If so, I would be even
less in favor of that change.

No, since the new implementation where the token is preserved this is not needed.
You cannot define the function in a namespace, nor disable it, nor use it as a goto label.
The one benefit of having it become a proper function which can also be used as a statement is:

Using named arguments
Passing it to callable parameters
Usual type juggling semantics

I do understand your point about the type juggling semantics, but I
might have addressed that differently. I almost always use exit
without argument (and if, only with an int), and die always with a
string (and only for quick experiments), and I figure that this might be
what most contemporary code does (at least, I hope that the
do_something() or die() times have long gone). As such, having exit
and die as alias could be changed, sticking with exit as a control
flow instruction, and having die as proper function (which could even
be implemented in userland), where exit would allow an optional int
argument (like break), and die() a required string argument. Of
course, this would be a much bigger BC break, but it seems to me the
cleaner solution.

This might be a good idea in the long term to properly split exit and die and have them take only integers and string respectively,
but I am not going to personally bother with this, even if I think this is a good idea.

Best regards,

Gina P. Banyard

10 months ago by Christoph M. Becker — view source

unread

Would that RFC imply that I would need to write \\exit or have a use exit clause to avoid dynamic namespace lookup? If so, I would be even
less in favor of that change.

No, since the new implementation where the token is preserved this is not needed.
You cannot define the function in a namespace, nor disable it, nor use it as a goto label.

Okay, thanks for the clarification.

The one benefit of having it become a proper function which can also be used as a statement is:

Using named arguments

Passing it to callable parameters

Usual type juggling semantics

Personally, I'm not interested in doing the first two points.

I do understand your point about the type juggling semantics, but I
might have addressed that differently. I almost always use exit
without argument (and if, only with an int), and die always with a
string (and only for quick experiments), and I figure that this might be
what most contemporary code does (at least, I hope that the
do_something() or die() times have long gone). As such, having exit
and die as alias could be changed, sticking with exit as a control
flow instruction, and having die as proper function (which could even
be implemented in userland), where exit would allow an optional int
argument (like break), and die() a required string argument. Of
course, this would be a much bigger BC break, but it seems to me the
cleaner solution.

This might be a good idea in the long term to properly split exit and die and have them take only integers and string respectively,
but I am not going to personally bother with this, even if I think this is a good idea.

Fair enough.

Given that I'm very late to this party, and that I'm not strongly
opposed to the suggested change, I will refrain from voting.

Cheers,
Christoph

10 months ago by Derick Rethans — view source

unread

I have just opened the vote for the "Transform exit() from a
language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

As userland PHP developer, I always regarded exit as a control flow
instruction (quite similar to break), and as such I'm not really in
favor of converting it to a proper function (especially since it is
not, because the parantheses could be omitted).

Xdebug uses exit for exactly that too. For control flow analysis. And I
also always have considered it to be a control flow instruction.

I see no benefit in changing it to a function, especially because
there will never be a function "exit" from it, just only an "entry".
This breaks function execution symmetry (and causes issues with Xdebug
when I last tried to make it work with a development branch for this
RFC).

As the RFC is scarce on mitigations for this, I am currently voting "no"
as I am unsure how certain features in Xdebug could remain working. I
have written to the list on other reasons before
(https://externals.io/message/123277#123450) without a conclusion.

I'll consider changing it to yes if there is a commitment for addressing
these feature-maintaining-requirements to keep Xdebug working, either
through new APIs (think observer) or other mitigations.

cheers,
Derick

--
https://derickrethans.nl | https://xdebug.org | https://dram.io

Author of Xdebug. Like it? Consider supporting me: https://xdebug.org/support

mastodon: @derickr@phpc.social @xdebug@phpc.social

10 months ago by Christoph M. Becker — view source

unread

Xdebug uses exit for exactly that too. For control flow analysis. And I
also always have considered it to be a control flow instruction.

I see no benefit in changing it to a function, especially because
there will never be a function "exit" from it, just only an "entry".
This breaks function execution symmetry (and causes issues with Xdebug
when I last tried to make it work with a development branch for this
RFC).

As the RFC is scarce on mitigations for this, I am currently voting "no"
as I am unsure how certain features in Xdebug could remain working. I
have written to the list on other reasons before
(https://externals.io/message/123277#123450) without a conclusion.

I'll consider changing it to yes if there is a commitment for addressing
these feature-maintaining-requirements to keep Xdebug working, either
through new APIs (think observer) or other mitigations.

Hmm, so far I only had skimmed the RFC and the related discussions, but
now I checked out the suggested implementation[1]. Then I tried to
build with PECL/uopz, and of course that failed because ZEND_EXIT is no
longer there. Okay, quickly drop that usage; build succeeds. Then I ran

<?php
uopz_set_return("exit", function () {echo "hello";}, true);
exit;
?>

And got

hello

Previously, no output was echoed. While that seems to be fixable in
uopz even without further changes to the implementation as mentioned by
Derick regarding Xdebug, I am concerned that changing exit to be a
proper function (although it isn't quite, since the parens could be
omitted) does more harm than good, at least in a minor PHP version. I
would assume that there are more extensions overriding the ZEND_EXIT
handler, which would now need to override the function handler in the
best case, or be unfixable in the worst case. Wrt. to such extensions,
the change might even delay the adoption of PHP 8.4. Had an attempt
been made to assess the BC break regarding dropping the ZEND_EXIT
opcode? From a quick GH search[2] quite some code may be affected.

[1] https://github.com/php/php-src/pull/13483
[2] https://github.com/search?q=ZEND_EXIT&type=code

Cheers,
Christoph

--
Currently listening to Judas Priest "Breaking your code"

10 months ago by tim@bastelstu.be — view source

unread

Hi

Hmm, so far I only had skimmed the RFC and the related discussions, but
now I checked out the suggested implementation[1]. Then I tried to
build with PECL/uopz, and of course that failed because ZEND_EXIT is no
longer there. Okay, quickly drop that usage; build succeeds. Then I ran

<?php
uopz_set_return("exit", function () {echo "hello";}, true);
exit;
?>

And got

hello

Previously, no output was echoed. While that seems to be fixable in

Frankly from a userland developer PoV this looks entirely correct: If I
override the exit() function, then I expect the exit() function to
be overridden.

Best regards
Tim Düsterhus

10 months ago by Christoph M. Becker — view source

unread

Hmm, so far I only had skimmed the RFC and the related discussions, but
now I checked out the suggested implementation[1]. Then I tried to
build with PECL/uopz, and of course that failed because ZEND_EXIT is no
longer there. Okay, quickly drop that usage; build succeeds. Then I ran

<?php
uopz_set_return("exit", function () {echo "hello";}, true);
exit;
?>

And got

hello

Previously, no output was echoed. While that seems to be fixable in

Frankly from a userland developer PoV this looks entirely correct: If I
override the exit() function, then I expect the exit() function to
be overridden.

I should have clarified, that "fixing" means to restore the expected
behavior of uopz, namely that accidential attempts to override exit with
uopz_set_return() were silently ignored, but unless uopz.exit=1 is
set, or uopz_allow_exit(true) is called, exit is ignored. Especially
the latter may be relied upon by tests for legacy code.

Christoph

10 months ago by Gina P. Banyard — view source

unread

Hmm, so far I only had skimmed the RFC and the related discussions, but
now I checked out the suggested implementation[1]. Then I tried to
build with PECL/uopz, and of course that failed because ZEND_EXIT is no
longer there. Okay, quickly drop that usage; build succeeds. Then I ran

<?php
uopz_set_return("exit", function () {echo "hello";}, true);
exit;
?>

And got

hello

Previously, no output was echoed. While that seems to be fixable in

Frankly from a userland developer PoV this looks entirely correct: If I
override the exit() function, then I expect the exit() function to
be overridden.

I should have clarified, that "fixing" means to restore the expected
behavior of uopz, namely that accidential attempts to override exit with
uopz_set_return() were silently ignored, but unless uopz.exit=1 is
set, or uopz_allow_exit(true) is called, exit is ignored. Especially
the latter may be relied upon by tests for legacy code.

Christoph

This sounds like a uopz extension issue that is easily fixed.
I am not sure why we should bend over backwards for extensions that allow to break usual userland semantics while preventing userland behaviour to be better.

Best regards,

Gina P. Banyard

10 months ago by Christoph M. Becker — view source

unread

This sounds like a uopz extension issue that is easily fixed.

Most likely, yes, although still somebody has to provide a fix, and
someone has to do a new release.

I am not sure why we should bend over backwards for extensions that allow to break usual userland semantics while preventing userland behaviour to be better.

We shouldn't do one or the other without having weighted the pros and
cons. So what are the pros regarding improved userland behavior:

we can use named arguments

So we can now write exit(status = 1) or exit(status = "some error message). I don't see any improvement using named arguments for a
single argument function (maybe unless it is a boolean argument). And
for a string argument, I don't think that status is a sensible
parameter name here.

pass to functions as callable

That might be a nice feature, but at least I have never seen the need to
pass exit as callable. And it wouldn't be hard to define a wrapper
function (e.g. my_exit()) if someone really needs to pass it as a
callable.

does not respect strict_types and does not follow usual type juggling
semantics

That might be an issue (although I've never stumbled upon that), but
it seems to me that this could even be handled in the ZEND_EXIT handler.

Perhaps I've missed some pros, but it seems these have been the ones the
RFC explicitly mentions.

So, what are the cons:

removing the ZEND_EXIT opcode

That immediately breaks any extension using it. A quick Github search
lists 3.1 k occurrences[1].

making exit a "proper" function

It still can be called without parentheses, so I don't regard it as a
proper function. It might even be confusing to readers ("oh, now I
can call a function without arguments by omitting the parentheses – but
why doesn't it work for other functions?"). And the PHP manual even
states (emphasis mine):

| exit is a language construct and it can be called without
| parentheses if no status is passed.

Weighting the pros and cons is left as an exercise to the reader.

[1] https://github.com/search?q=ZEND_EXIT&type=code

Thanks,
Christoph

10 months ago by Gina P. Banyard — view source

unread

This sounds like a uopz extension issue that is easily fixed.

Most likely, yes, although still somebody has to provide a fix, and
someone has to do a new release.

Why should I investigate time to fix extensions when this RFC might not even land in the first place?
We do countless engine breaking changes without RFCs and without notices to extension maintainers other than UPGRADING.INTERNALS.

May I remind everyone that PHP 8.1 had a huge breaking change for extensions in utterly rewriting the stream API:

Zend Stream API has been changed to use "zend_string*" instead of "char*" [1]

Extension developers have somehow coped with it and/or just suffered in silence.
We didn't provide ANY help for extension developers to make the move easier.
It is getting frustrating that on one hand we treat extension developers like they are idiots incapable of updating their extension for minor changes in engine APIs and behaviour (or even just including the proper headers circling, back to that whole drama)
While at the same time kicking them in the face with massive, and potentially clunky #ifdef checks, code churn for major engine API changes that get ZERO discussion and expect them to be competent at their job.

Could we maybe start respecting the intelligence of extension developers?
And if not, can we stop pretending that we care about them when we, as a project, clearly don't seem to care in our actual action?
Because I find it a tad strange that every time I point the above stream API design change, the only thing I hear back is crickets.

I am not sure why we should bend over backwards for extensions that allow to break usual userland semantics while preventing userland behaviour to be better.

We shouldn't do one or the other without having weighted the pros and
cons. So what are the pros regarding improved userland behavior:

we can use named arguments

So we can now write exit(status = 1) or exit(status = "some error message). I don't see any improvement using named arguments for a
single argument function (maybe unless it is a boolean argument). And
for a string argument, I don't think that status is a sensible
parameter name here.

Please propose a better name then.

pass to functions as callable

That might be a nice feature, but at least I have never seen the need to
pass exit as callable. And it wouldn't be hard to define a wrapper
function (e.g. my_exit()) if someone really needs to pass it as a
callable.

The fact that one might need to do this workaround is bad already, and not a justification against this RFC.

does not respect strict_types and does not follow usual type juggling
semantics

That might be an issue (although I've never stumbled upon that), but
it seems to me that this could even be handled in the ZEND_EXIT handler.

Now see this is strange to me, as I clearly remember hitting this issue while working on a QA scrip for the PHP documentation,
was not understanding why CI was failing with no output,
until you realised that the issue was me passing a boolean to exit() that was being cast to a string and not the expected integer.

So I would argue you have stumbled upon this.

Perhaps I've missed some pros, but it seems these have been the ones the
RFC explicitly mentions.

So, what are the cons:

removing the ZEND_EXIT opcode

That immediately breaks any extension using it. A quick Github search
lists 3.1 k occurrences[1].

Please see point about the stream layer API change.

making exit a "proper" function

It still can be called without parentheses, so I don't regard it as a
proper function. It might even be confusing to readers ("oh, now I
can call a function without arguments by omitting the parentheses – but
why doesn't it work for other functions?"). And the PHP manual even
states (emphasis mine):

| exit is a language construct and it can be called without
| parentheses if no status is passed.

Weighting the pros and cons is left as an exercise to the reader.

[1] https://github.com/search?q=ZEND_EXIT&type=code

Since when has the documentation been the normative aspect of PHP?
We routinely fix the documentation to explain the Zend engine's true behaviour, even if it doesn't make any sense.

This sentence is true as of now, and may be changed if this RFC passes.
Using current wording on the documentation is.... a strange argument to me.

Best regards,

Gina P. Banyard

[1] https://github.com/php/php-src/blob/PHP-8.1/UPGRADING.INTERNALS#L36

10 months ago by Christoph M. Becker — view source

unread

Most likely, yes, although still somebody has to provide a fix, and
someone has to do a new release.

Why should I investigate time to fix extensions when this RFC might not even land in the first place?

I do not expect anybody who makes approved changes to the APIs to
provide fixes for any of the affected external extensions. It's great
if they help when asked, and it's okay if they don't.

I merely noted that someone has to make these changes to keep these
extensions up-to-date for new PHP releases. And sometimes real life
interferes for the maintainers, so they don't have time (due to other
interests, health issues, a new job, family, etc.) And even if others
would like to help out, sometimes they can't[1].

We do countless engine breaking changes without RFCs and without notices to extension maintainers other than UPGRADING.INTERNALS.

That is exactly my point: we should weight the pros and cons of any API
change carefully. How many extensions may be affected, how hard would
it be to update these extensions, how ugly would the resulting code be
due to PHP_VERSION_ID checks, etc. And on the other hand, what would be
the benefits for the whole ecosystem (php-src, extensions, userland
developers).

Could we maybe start respecting the intelligence of extension developers?
And if not, can we stop pretending that we care about them when we, as a project, clearly don't seem to care in our actual action?

We should not fall into the trap of thinking: we did more seriously
breaking changes in the past, so we don't need to care anymore. After
all, we cannot change what had happened, but we may be able to improve.

So we can now write exit(status = 1) or exit(status = "some error message). I don't see any improvement using named arguments for a
single argument function (maybe unless it is a boolean argument). And
for a string argument, I don't think that status is a sensible
parameter name here.

Please propose a better name then.

That's the crux with some of the union types for legacy functions: there
are no suitable names. setcookie() has an $expires_or_options
parameter, so we might choose $status_or_message here, and although that
seems to be more correct, it would be ridiculous to use this as named
argument.

(For the record: I've got the syntax wrong, because I don't use named
arguments, since these are fixing the symptoms, not the desease, which
is a bad API.)

That might be a nice feature, but at least I have never seen the need to
pass exit as callable. And it wouldn't be hard to define a wrapper
function (e.g. my_exit()) if someone really needs to pass it as a
callable.

The fact that one might need to do this workaround is bad already, and not a justification against this RFC.

The fact that one might need to pass exit implies that they are using
some test helper extension (e.g. uopz) to stub/mock it, or that they
should consider to consult a therapist. ;)

That might be an issue (although I've never stumbled upon that), but
it seems to me that this could even be handled in the ZEND_EXIT handler.

Now see this is strange to me, as I clearly remember hitting this issue while working on a QA scrip for the PHP documentation,
was not understanding why CI was failing with no output,
until you realised that the issue was me passing a boolean to exit() that was being cast to a string and not the expected integer.

I have completely forgotten about this, and cannot really remember even now.

So I would argue you have stumbled upon this.

I stand corrected. However, that doesn't invalidate my argument that at
least some sanity could be introduced in the ZEND_EXIT handler instead
of pursuing the RFC at hand.

It still can be called without parentheses, so I don't regard it as a
proper function. It might even be confusing to readers ("oh, now I
can call a function without arguments by omitting the parentheses – but
why doesn't it work for other functions?"). And the PHP manual even
states (emphasis mine):

| exit is a language construct and it can be called without
| parentheses if no status is passed.

Since when has the documentation been the normative aspect of PHP?
We routinely fix the documentation to explain the Zend engine's true behaviour, even if it doesn't make any sense.

This sentence is true as of now, and may be changed if this RFC passes.
Using current wording on the documentation is.... a strange argument to me.

Fair enough. So just ignore that last part, but what about:

It still can be called without parentheses, so I don't regard it as a
proper function. It might even be confusing to readers ("oh, now I
can call a function without arguments by omitting the parentheses – but
why doesn't it work for other functions?").

Anyhow, perhaps an alternative to the proposal at hand would be to
implement the following proper function in the core:

function quit(int $status = 0) {
exit($status);
}

Users who wish to have something like die("message"), could use it, or

echo "message";
quit()

Or perhaps that could be a set of quit functions; the main point is not
to change anything in the core (for now), but still allowing users to
opt into some cleaner functionality.

[1] https://github.com/krakjoe/pcov/pull/103#issuecomment-2269612656

Cheers,
Christoph

10 months ago by tim@bastelstu.be — view source

unread

Hi

So, what are the cons:

removing the ZEND_EXIT opcode

That immediately breaks any extension using it. A quick Github search
lists 3.1 k occurrences[1].

For me this is a pro, because it makes the implementation simpler going
forward: The engine, the JIT and extensions have one fewer Opcode they
need to deal with / take into account / test. Anything that is capable
of handling functions that throw exceptions will be capable of handling
the proposed exit() function.

Best regards
Tim Düsterhus

10 months ago by Larry Garfield — view source

unread

I have just opened the vote for the "Transform exit() from a
language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

As userland PHP developer, I always regarded exit as a control flow
instruction (quite similar to break), and as such I'm not really in
favor of converting it to a proper function (especially since it is
not, because the parantheses could be omitted).

Xdebug uses exit for exactly that too. For control flow analysis. And I
also always have considered it to be a control flow instruction.

I see no benefit in changing it to a function, especially because
there will never be a function "exit" from it, just only an "entry".
This breaks function execution symmetry (and causes issues with Xdebug
when I last tried to make it work with a development branch for this
RFC).

As the RFC is scarce on mitigations for this, I am currently voting "no"
as I am unsure how certain features in Xdebug could remain working. I
have written to the list on other reasons before
(https://externals.io/message/123277#123450) without a conclusion.

I'll consider changing it to yes if there is a commitment for addressing
these feature-maintaining-requirements to keep Xdebug working, either
through new APIs (think observer) or other mitigations.

cheers,
Derick

While I support language and engine cleanup, if this change causes issues for Xdebug that is a fairly significant problem. For that reason I have shifted my Yes to a No for now. Like Derick, I will switch it back to a Yes should the Xdebug issue be resolved to his satisfaction. But "keep Xdebug working" is a rather mission-critical requirement for any RFC, as a practical matter.

--Larry Garfield

10 months ago by tim@bastelstu.be — view source

unread

Hi

As userland PHP developer, I always regarded exit as a control flow
instruction (quite similar to break), and as such I'm not really in
favor of converting it to a proper function (especially since it is
not, because the parantheses could be omitted).

Xdebug uses exit for exactly that too. For control flow analysis. And I
also always have considered it to be a control flow instruction.

I see no benefit in changing it to a function, especially because
there will never be a function "exit" from it, just only an "entry".
This breaks function execution symmetry (and causes issues with Xdebug
when I last tried to make it work with a development branch for this
RFC).

This is false. The observers are perfectly capable of detecting that the
exit() function returns / throws when observing
zend_observer_fcall_end_handler as demonstrated by the following script:

 &lt;?php
 class MyClass {
 	public function __destruct() {
 		echo __METHOD__, PHP_EOL;
 	}
 }

 function a() {
 	$dummy = new MyClass();

 	exit();
 }

 a();

Running this script as:

 sapi/cli/php -d zend_test.observer.enabled=1 -d

zend_test.observer.observe_all=1 test.php

will return the following output:

 <!-- init 'php-src/test.php' -->
 <file 'php-src/test.php'>
   <!-- init a() -->
   <a>
     <!-- init exit() -->
     <exit>
       <!-- Exception: UnwindExit -->
     </exit>
     <!-- Exception: UnwindExit -->
   </a>
   <!-- init MyClass::__destruct() -->
   <MyClass::__destruct>
 MyClass::__destruct
   </MyClass::__destruct>
   <!-- Exception: UnwindExit -->
 </file 'php-src/test.php'>

Leaving the exit() function will be observed as indicated by </exit>.
It also shows that any destructors will be called and it also shows the
internal UnwindExit exception.

Thus you are able to detect if exit() has successfully been called by
observing an fcall_end and checking for
zend_is_unwind_exit(EG(exception)) to determine if the exception
you're dealing with is the unwind exception.

That way you will also correctly handle that exit() is not successful,
e.g. when a non-stringable class is passed to it and a TypeError is
thrown, because then the output will look something like this:

 <!-- init 'php-src/test.php' -->
 <file 'php-src/test.php'>
   <!-- init a() -->
   <a>
     <!-- init exit() -->
     <exit>
       <!-- Exception: TypeError -->
     </exit>
     <!-- Exception: TypeError -->
   </a>
   <!-- Exception: TypeError -->
 </file 'php-src/test.php'>
 <!-- init Error::__toString() -->
 <Error::__toString>
   <!-- init Error::getTraceAsString() -->
   <Error::getTraceAsString>
   </Error::getTraceAsString>
 </Error::__toString>

 Fatal error: Uncaught TypeError: exit(): Argument #1 ($code) must

be of type string|int, MyClass given in php-src/test.php:11
Stack trace:
#0 php-src/test.php(11): exit(Object(MyClass))
#1 php-src/test.php(14): a()
#2 {main}
thrown in php-src/test.php on line 11

MyClass::__destruct
MyClass::__destruct
</MyClass::__destruct>

Best regards
Tim Düsterhus

10 months ago by Gina P. Banyard — view source

unread

I have just opened the vote for the "Transform exit() from a
language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

As userland PHP developer, I always regarded exit as a control flow
instruction (quite similar to break), and as such I'm not really in
favor of converting it to a proper function (especially since it is
not, because the parantheses could be omitted).

Xdebug uses exit for exactly that too. For control flow analysis. And I
also always have considered it to be a control flow instruction.

I see no benefit in changing it to a function, especially because
there will never be a function "exit" from it, just only an "entry".
This breaks function execution symmetry (and causes issues with Xdebug
when I last tried to make it work with a development branch for this
RFC).

As the RFC is scarce on mitigations for this, I am currently voting "no"
as I am unsure how certain features in Xdebug could remain working. I
have written to the list on other reasons before
(https://externals.io/message/123277#123450) without a conclusion.

I'll consider changing it to yes if there is a commitment for addressing
these feature-maintaining-requirements to keep Xdebug working, either
through new APIs (think observer) or other mitigations.

cheers,
Derick

--
https://derickrethans.nl | https://xdebug.org | https://dram.io

Author of Xdebug. Like it? Consider supporting me: https://xdebug.org/support

mastodon: @derickr@phpc.social @xdebug@phpc.social

I still do not comprehend your issue.
You can either do stuff at compile time, which has been explained before, or use the function call observer for anything runtime related.
The behaviour of exit() is completely observable with the current observer framework, so I don't see why anything else needs to be done.

Best regards,

Gina P. Banyard

10 months ago by Levi Morrison — view source

unread

Hello Internals,

I have just opened the vote for the "Transform exit() from a language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

Best regards,

Gina P. Banyard

After thinking about this several times over the course of discussion
and again now that it's in voting, I have decided to vote no. I am in
favor of this change, I just think given the concerns it's best to
wait for PHP 9.0 to do it. Maybe the concerns with control flow can be
improved by better inference/marking of functions which are
noreturn, at least for known functions (of which \exit definitely
would be).

10 months ago by tim@bastelstu.be — view source

unread

Hi

After thinking about this several times over the course of discussion
and again now that it's in voting, I have decided to vote no. I am in
favor of this change, I just think given the concerns it's best to
wait for PHP 9.0 to do it. Maybe the concerns with control flow can be
improved by better inference/marking of functions which are
noreturn, at least for known functions (of which \exit definitely
would be).

As I've just explained in my response to Derick within the discussion
thread related to this RFC: The information is already there, it just
needs to be used.

For a test I've just added the following code snippet to
zend_compile_call():

 zend_type return_type = fbc->common.arg_info[-1].type;
 if (ZEND_TYPE_CONTAINS_CODE(return_type, IS_NEVER)) {
 	printf("%s has return type never\n", ZSTR_VAL(lcname));
 } else {
 	printf("%s doesn't have return type never\n", ZSTR_VAL(lcname));
 }

and then I executed the following test script:

 &lt;?php

 namespace Foo;

 function a() {
 	\strrev('foo');
 	exit(new \stdClass());
 }

 try {
 	a();
 } catch (\Throwable) {}
 echo "executed", PHP_EOL;

when running this script, I receive the following output:

 strrev doesn't have return type never
 exit has return type never
 executed

Best regards
Tim Düsterhus

10 months ago by kontakt@beberlei.de — view source

unread

Am 30.07.2024, 11:49:52 schrieb Gina P. Banyard internals@gpb.moe:

Hello Internals,

I have just opened the vote for the "Transform exit() from a language
construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

Best regards,

Gina P. Banyard

Hi Gina,

I was ambivalent to this before, but after reading the discussion, voted no
for two reasons:

exit without parenthesis s still not a function, that does not makes
handling this on extension level (tools, profilers) more reliable, instead
more confusing. I don’t mind the work this might cause for an extension, so
i don’t join Christoph’s and Derick’s argument here that this a break for
extensions that disqualifies the change.
If it still does not allow me to set exit as disabled_functions, then
this creates an inconsistency

I also have a question: From a userland perspective, it looks it creates
inconsistency calling exit with global namespace prefix, depending on
parenthesis or? \exit(); will now work, but \exit; will not?

Also if https://github.com/php/php-src/pull/4084 taught me anything, trying
to add eval support for disabled_functions, then maybe we should throw
warnings instead when using that ini setting to disable op codes, instead
of silently ignoring that this won’t work.

10 months ago by kontakt@beberlei.de — view source

unread

Am 06.08.2024, 09:43:21 schrieb Benjamin Außenhofer kontakt@beberlei.de:

Am 30.07.2024, 11:49:52 schrieb Gina P. Banyard internals@gpb.moe:

Hello Internals,

I have just opened the vote for the "Transform exit() from a language
construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

Best regards,

Gina P. Banyard

Hi Gina,

I was ambivalent to this before, but after reading the discussion, voted
no for two reasons:

exit without parenthesis s still not a function, that does not makes
handling this on extension level (tools, profilers) more reliable, instead
more confusing. I don’t mind the work this might cause for an extension, so
i don’t join Christoph’s and Derick’s argument here that this a break for
extensions that disqualifies the change.

Tim pointed out to me that my argument here is wrong, since exit; will be
transformed to exit() at compile time, I did not read this from my first
read-through of the RFC, but now I see it in the text. So this point does
not hold anymore.

If it still does not allow me to set exit as disabled_functions,
then this creates an inconsistency

That left me wondering why disabled_functions does not work, and I see the
PR adds a special case for both functions. So disabling them „could“
potentially work.

I also have a question: From a userland perspective, it looks it creates
inconsistency calling exit with global namespace prefix, depending on
parenthesis or? \exit(); will now work, but \exit; will not?

Also if https://github.com/php/php-src/pull/4084 taught me anything,
trying to add eval support for disabled_functions, then maybe we should
throw warnings instead when using that ini setting to disable op codes,
instead of silently ignoring that this won’t work.

10 months ago by Gina P. Banyard — view source

unread

Am 06.08.2024, 09:43:21 schrieb Benjamin Außenhofer kontakt@beberlei.de:

If it still does not allow me to set exit as disabled_functions, then this creates an inconsistency

That left me wondering why disabled_functions does not work, and I see the PR adds a special case for both functions. So disabling them „could“ potentially work.

Indeed, as it would be a true function and disabling it would work as expected, as initially I did permit disabling it but I decided that might be a bit too weird for people.

In fact one could even redefine it after disabing it (like any other internal function).
It might be a bit harder to support redefining it after it has been disabled now that the implementation is at the parser level (as various people prefered).

In any case, ant technical issue that eval() was having are non-existing here.

Best regards,
Gina P. Banyard

10 months ago by Theodore Brown — view source

unread

Hello Internals,

I have just opened the vote for the "Transform exit() from a language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

I really appreciate RFCs like this which not only make the language more consistent for userland developers, but also simplify PHP's internal implementation, paving the way for future optimizations and new functionality.

In my experience, extension developers nearly always have to make some changes to support each new PHP version, so I'm not sure why that would be a reason to prevent improving the language.

Best regards,
Theodore

10 months ago by Derick Rethans — view source

unread

Hello Internals,

I have just opened the vote for the "Transform exit() from a language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

I really appreciate RFCs like this which not only make the language more consistent for userland developers, but also simplify PHP's internal implementation, paving the way for future optimizations and new functionality.

In my experience, extension developers nearly always have to make some changes to support each new PHP version, so I'm not sure why that would be a reason to prevent improving the language.

This is misrepresenting my concern.

I understand that new versions require changes.

One of my issues is, is that so far I could not find a way to replicate existing functionality with this patch applied.

The RFC does not mention a BC break, nor does it have an entry for UPGRADING.INTERNALS either.

cheers
Derick

10 months ago by Andreas Heigl — view source

unread

Stupid question maybe, but are we voting on the RFC or on the patch?

If the patch does not match what.the RFC proposes, then the patch has a problem. That should IMO though not affect voting on an RFC.

Or am I.missimg something?

Cheers

Andreas

Hello Internals,

I have just opened the vote for the "Transform exit() from a language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

I really appreciate RFCs like this which not only make the language more consistent for userland developers, but also simplify PHP's internal implementation, paving the way for future optimizations and new functionality.

In my experience, extension developers nearly always have to make some changes to support each new PHP version, so I'm not sure why that would be a reason to prevent improving the language.

This is misrepresenting my concern.

I understand that new versions require changes.

One of my issues is, is that so far I could not find a way to replicate existing functionality with this patch applied.

The RFC does not mention a BC break, nor does it have an entry for UPGRADING.INTERNALS either.

cheers
Derick

10 months ago by Bilge — view source

unread

Stupid question maybe, but are we voting on the RFC or on the patch?

I have been reliably informed that votes are always for RFCs.

Cheers,
Bilge

10 months ago by Gina P. Banyard — view source

unread

Stupid question maybe, but are we voting on the RFC or on the patch?

If the patch does not match what.the RFC proposes, then the patch has a problem. That should IMO though not affect voting on an RFC.
Or am I.missimg something?

In theory, it is the RFC idea.
In practice, a lot of the times it is the patch for complex features.

However, it is still within the purview of core developers to veto the implementation of an RFC.
Which could be the case here rather than voting against the RFC outright.

Best regards,
Gina P. Banyard

10 months ago by Andreas Heigl — view source

unread

Hey Gina, hey all

Am 08.08.24 um 15:44 schrieb Gina P. Banyard:

On Wednesday, 7 August 2024 at 17:07, Andreas Heigl andreas@heigl.org
wrote:

Stupid question maybe, but are we voting on the RFC or on the patch?

If the patch does not match what.the RFC proposes, then the patch has
a problem. That should IMO though not affect voting on an RFC.

Or am I.missimg something?

In theory, it is the RFC idea.
In practice, a lot of the times it is the patch for complex features.

However, it is still within the purview of core developers to veto the
implementation of an RFC.
Which could be the case here rather than voting against the RFC outright.

I have no problem that core developers veto a certain implementation of
an RFC. I actually expect them to do so.

But the vote should IMO always and exclusively be based on the RFC.
Not on the implementation. If the voting happens based on the
implementation due to the complexity of the features that means that the
RFC is not wel written and needs to be improved. Or the implementation
is problematic and needs to be vetoed by the core developers.

Why do I think so? It becomes completely intransparent why an RFC was
rejected when the voting happens based on a meager implementation as
after some years no one will understand why a well written RFC was
rejected. Especially when the discussion then also happens off-list and
on the actual code in github as that tears apart the information sources
that need to be taken into consideration in hindsight.

Also the RFC should specify the implementation. If the implementation
doesn't match the RFC then the implementation is faulty and needs to
change until it does match the RFC. But that is an implementation
detail. The voting though should only happen on the description of the
feature.

My 0.02 €

Cheers

Andreas

                                                           ,,,
                                                          (o o)

+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| https://andreas.heigl.org |
+---------------------------------------------------------------------+
| https://hei.gl/appointmentwithandreas |
+---------------------------------------------------------------------+
| GPG-Key: https://hei.gl/keyandreasheiglorg |
+---------------------------------------------------------------------+

10 months ago by Rob Landers — view source

unread

Hey Gina, hey all

Am 08.08.24 um 15:44 schrieb Gina P. Banyard:

On Wednesday, 7 August 2024 at 17:07, Andreas Heigl andreas@heigl.org
wrote:

Stupid question maybe, but are we voting on the RFC or on the patch?

If the patch does not match what.the RFC proposes, then the patch has
a problem. That should IMO though not affect voting on an RFC.

Or am I.missimg something?

In theory, it is the RFC idea.
In practice, a lot of the times it is the patch for complex features.

However, it is still within the purview of core developers to veto the
implementation of an RFC.
Which could be the case here rather than voting against the RFC outright.

I have no problem that core developers veto a certain implementation of
an RFC. I actually expect them to do so.

But the vote should IMO always and exclusively be based on the RFC.
Not on the implementation. If the voting happens based on the
implementation due to the complexity of the features that means that the
RFC is not wel written and needs to be improved. Or the implementation
is problematic and needs to be vetoed by the core developers.

Why do I think so? It becomes completely intransparent why an RFC was
rejected when the voting happens based on a meager implementation as
after some years no one will understand why a well written RFC was
rejected. Especially when the discussion then also happens off-list and
on the actual code in github as that tears apart the information sources
that need to be taken into consideration in hindsight.

I would expect any implementation done before the RFC is voted on to be entirely proof-of-concept, and not expected to be mergable as-is. Basically, a way to test out the new proposed RFC, but may have issues (such as memory leaks or not all edge cases implemented). I, personally, wouldn’t expect an RFC to be declined based on the initial patch. That’s good information to add to the rfc how-to page.

— Rob

10 months ago by Larry Garfield — view source

unread

Hey Gina, hey all

Am 08.08.24 um 15:44 schrieb Gina P. Banyard:

On Wednesday, 7 August 2024 at 17:07, Andreas Heigl andreas@heigl.org
wrote:

Stupid question maybe, but are we voting on the RFC or on the patch?

If the patch does not match what.the RFC proposes, then the patch has
a problem. That should IMO though not affect voting on an RFC.

Or am I.missimg something?

In theory, it is the RFC idea.
In practice, a lot of the times it is the patch for complex features.

However, it is still within the purview of core developers to veto the
implementation of an RFC.
Which could be the case here rather than voting against the RFC outright.

I have no problem that core developers veto a certain implementation of
an RFC. I actually expect them to do so.

But the vote should IMO always and exclusively be based on the RFC.
Not on the implementation. If the voting happens based on the
implementation due to the complexity of the features that means that the
RFC is not wel written and needs to be improved. Or the implementation
is problematic and needs to be vetoed by the core developers.

How exactly would voters veto an implementation if not through the RFC? That's literally the only formal input mechanism they have, and previous attempts to add others have been soundly rejected.

As a historical note, the partial function application RFC was declined despite there being general consensus that the proposal was quite good and quite desireable. The issue was that Nikita felt the implementation proposed with it was too fragile, and wasn't sure how to make it less fragile, so he voted No and several others followed suit. I am fairly confident that if a less-fragile implementation could be found, it would pass handily.

So yes, RFCs have been rejected in the past on "implementation only."

--Larry Garfield

10 months ago by Gina P. Banyard — view source

unread

How exactly would voters veto an implementation if not through the RFC? That's literally the only formal input mechanism they have, and previous attempts to add others have been soundly rejected.

As a historical note, the partial function application RFC was declined despite there being general consensus that the proposal was quite good and quite desireable. The issue was that Nikita felt the implementation proposed with it was too fragile, and wasn't sure how to make it less fragile, so he voted No and several others followed suit. I am fairly confident that if a less-fragile implementation could be found, it would pass handily.

So yes, RFCs have been rejected in the past on "implementation only."

The implementation for the attribute syntax using @[] (before it got revoted) would have been vetoed due to implementation issues if the "Treat namespaced names as single token" RFC wouldn't have been accepted.

I am very much aware that RFCs have been rejected on implementation, but even if an RFC is accepted it can be vetoed by core developers.

And once again, I still do not have any idea why Derick has issues adding support to XDebug.
Especially as Tim seemingly managed to do this while being unfamiliar with the codebase.

Best regards,

Gina P. Banyard

10 months ago by Theodore Brown — view source

unread

I really appreciate RFCs like this which not only make the language more consistent for userland developers, but also simplify PHP's internal implementation, paving the way for future optimizations and new functionality.

In my experience, extension developers nearly always have to make some changes to support each new PHP version, so I'm not sure why that would be a reason to prevent improving the language.

This is misrepresenting my concern.

I understand that new versions require changes.

One of my issues is, is that so far I could not find a way to replicate existing functionality with this patch applied.

The RFC does not mention a BC break, nor does it have an entry for UPGRADING.INTERNALS either.

Hi Derick,

I'm confused by this, since earlier in the thread Tim responded with examples showing how the behavior of exit() can still be observed and correctly handled. If that didn't address your issue, can you explain it further? It seems like right now everyone is left bewildered about what functionality you couldn't replicate.

Kind regards,
Theodore

10 months ago by tim@bastelstu.be — view source

unread

Hi

I'm confused by this, since earlier in the thread Tim responded with examples showing how the behavior of exit() can still be observed and correctly handled. If that didn't address your issue, can you explain it further? It seems like right now everyone is left bewildered about what functionality you couldn't replicate.

Tideways generously sponsored 2 hours of my time to develop a proof of
concept patch to fix Xdebug against the current version of Gina's PR.
Much of that time was spent chasing a reference counting issue in an
unfamiliar codebase. Unfortunately ASAN was not of much help, because
running Xdebug's tests against PHP 8.4 reported some pre-existing memory
leaks.

I have attached two patches for Xdebug:

0001-Support-exit-as-function.patch:

This patch fixes the build of Xdebug, by removing the references to
ZEND_EXIT for PHP 8.4.

It also adds a hook on the new exit() function to "deinitialize" Xdebug,
which will ensure that profiling files are written for the use in CI. My
understanding is that an alternative solution would have been to add a
special xdebug_finalize_profile() function for use in CI instead of
overloading the exit() function.

All the existing tests in Xdebug's codebase pass after this patch,
except that I did not adjust the profiler test expectations to take the
the new exit() function call that was not previously there into account,
because I do not fully understand the output format of the tests.
Nevertheless the changes to the test output looked correct to me.

0002-Exit-coverage.patch:

This patch adds support for the coverage analysis, so that Xdebug
understands that any code after a call to exit() is unreachable. As far
as I can tell this case was not previously tested by any of Xdebug's
tests. I have thus added a test case, but I don't fully understand the
output format of the coverage tests, thus I may have overlooked
something. What I did verify is that the test fails when I remove the
implementation from code_coverage.c.

I'd like to note that this patch is not strictly necessary for Xdebug to
be usable by a user. It might erroneously report unreachable code as
reachable, but by definition unreachable code is useless. Thus a user of
Xdebug could work around this by simply removing the unreachable code.
My understanding is that some userland static analyzers and refactoring
tools are already capable of pointing out such unreachable code.

Given my unfamiliarity of Xdebug's codebase and the limited time I spent
preparing these patches, they should be considered a proof of concept
only. They might or might not match the preferred coding style used in
the Xdebug codebase. It might also be possible that they did not
correctly handle an edge case, due to my lack of knowledge about the
test infrastructure.

Nevertheless I believe that someone more familiar with the codebase will
certainly be able to adapt these patches as necessary to make Xdebug
fully compatible with Gina's PR.

In any case I believe they clearly showcase that it is not impossible to
keep the existing functionality, even with Gina's RFC.

Best regards
Tim Düsterhus

10 months ago by Gina P. Banyard — view source

unread

Hello Internals,

I have just opened the vote for the "Transform exit() from a language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

Best regards,

Gina P. Banyard

Hello Internals,

I just closed the vote.

The final results are 24 votes in favour and 12 against.
Meaning it has reached the 2/3 threshold required for an RFC to be accepted.
This is closer than I'd like, but I guess I'll take it.

Thank you to everyone that has voted.

Best regards,

Gina P. Banyard

10 months ago by Matthew Sewell — view source

unread

Hello Internals,

I have just opened the vote for the "Transform exit() from a language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

Best regards,

Gina P. Banyard

Hello Internals,

I just closed the vote.

The final results are 24 votes in favour and 12 against.
Meaning it has reached the 2/3 threshold required for an RFC to be accepted.
This is closer than I'd like, but I guess I'll take it.

Thank you to everyone that has voted.

Best regards,

Gina P. Banyard

Hi,

Were there more discussions on this off the list? I presume there are often discussions elsewhere between the core group of internal developers.

It's just I never saw a confirmed answer on the Xdebug issue but did see a significant drop off in no votes without any further explanation (appreciating that no-one has to give an explanation) in the last couple of days.

Did the issue get resolved?

Thanks very much,

Matt

10 months ago by Gina P. Banyard — view source

unread

Hello Internals,

I have just opened the vote for the "Transform exit() from a language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

Best regards,

Gina P. Banyard

Hello Internals,

I just closed the vote.

The final results are 24 votes in favour and 12 against.
Meaning it has reached the 2/3 threshold required for an RFC to be accepted.
This is closer than I'd like, but I guess I'll take it.

Thank you to everyone that has voted.

Best regards,

Gina P. Banyard

Hi,

Were there more discussions on this off the list? I presume there are often discussions elsewhere between the core group of internal developers.

It's just I never saw a confirmed answer on the Xdebug issue but did see a significant drop off in no votes without any further explanation (appreciating that no-one has to give an explanation) in the last couple of days.

Did the issue get resolved?

Thanks very much,

Matt

Derick has told me off list that Tim's patches look reasonable.

Best regards,

Gina P. Banyard

10 months ago by tim@bastelstu.be — view source

unread

Hi

Derick has told me off list that Tim's patches look reasonable.

There's also this PR for Xdebug that includes my two patches:
https://github.com/xdebug/xdebug/pull/972 (and some additional fixes on
top of them, because, as I said, I did not clean them up).

Best regards
Tim Düsterhus

10 months ago by Matthew Sewell — view source

unread

Hello Internals,

I have just opened the vote for the "Transform exit() from a language construct into a standard function" RFC:
https://wiki.php.net/rfc/exit-as-function

The vote will last for two weeks until the 13th of August 2024.

Best regards,

Gina P. Banyard

Hello Internals,

I just closed the vote.

The final results are 24 votes in favour and 12 against.
Meaning it has reached the 2/3 threshold required for an RFC to be accepted.
This is closer than I'd like, but I guess I'll take it.

Thank you to everyone that has voted.

Best regards,

Gina P. Banyard

Hi,

Were there more discussions on this off the list? I presume there are often discussions elsewhere between the core group of internal developers.

It's just I never saw a confirmed answer on the Xdebug issue but did see a significant drop off in no votes without any further explanation (appreciating that no-one has to give an explanation) in the last couple of days.

Did the issue get resolved?

Thanks very much,

Matt

Derick has told me off list that Tim's patches look reasonable.

Best regards,

Gina P. Banyard

Thank you very much for the response (and the RFC!).

Best wishes,

Matt