[RFC] Convert exit (and die) from language constructs to functions #13483

Girgias · 2024-02-23T20:53:30Z

The main motivation here is to have the same type juggling semantics as every other function in PHP.
Also, we don't need to have an opcode any more for it.

RFC: https://wiki.php.net/rfc/exit-as-function

arnaud-lb · 2024-02-24T12:15:36Z

Zend/zend_execute.c

+		if (UNEXPECTED(
+			zend_string_equals_literal_ci(Z_STR_P(key), "exit")
+			|| zend_string_equals_literal_ci(Z_STR_P(key), "die")
+		)) {
+			zend_throw_unwind_exit();
+			return FAILURE;
+		}


Could we achieve similar results with by de-sugaring exit; to a function call at the AST level? (e.g. zend_compile_const() can replace the AST with a function call AST and delegate to zend_compile_call()). This would remove the need for special cases at runtime, and also unify the behavior of exit; and exit(); (wrt to disable_functions).

I agree. Special-casing exit/die constant access doesn't sound like a cleaner solution than what we have now. Keeping them in the lexer/parser also keeps BC for AST-based tools.

Yes, and it has side effects like defined(), constant() calls etc.

We may opt to special-case only statement-level "exit" and "die" constant fetches directly (i.e. handle in zend_compile_stmt, in the branch for constants).

Oh, that's actually a smarter way of doing it!

I'll see if I can implement this, as I still struggle with the compiler part.

We may opt to special-case only statement-level "exit" and "die" constant fetches directly (i.e. handle in zend_compile_stmt, in the branch for constants).

I would prefer keeping parser support, as otherwise PHPStan and the likes need to adjust. I don't think there's a reason to break those. Edit: I guess lexer would be sufficient, as almost all of these tools use PHPParser, which defines its own AST. Still, I'd prefer letting the parser handle this.

I was about to suggest something similar, because exit; not actually calling the function but just throwing that unwind exception probably leads to observable behavioral differences in debuggers dependent on whether the user wrote exit; or exit();, no?

I don't think we should constrain ourselves with respect to phpstan etc.; there are worse BC breaks to be had.
I think the lexer, parser and opcodes are aside from any compatibilty guarantees.

@ondrejmirtes has told me that this shouldn't be an issue if PHPParser still works, I'm asking @dseguy if this would affect Exakat or not.

@exakat will need an adaptation, at the parser level. That is not an issue.

Zend/zend_compile.c

dstogov · 2024-05-08T06:15:01Z

Where is the RFC?

Removing ability to use die and exit without brackets, may effect a lot of PHP users.
I wouldn't accept this into PHP-8.*.

Please consider the possibility of adding die/exit functions without removing opcodes.
(This is just a quick idea. May be it doesn't make sense)

iluuu1994 · 2024-05-08T10:07:23Z

@dstogov See zend_compile.c, the constants are turned into function calls.

That said, I'm also a bit sceptical about the proposed benefits. I think an RFC, or at least a list on the mailing list explaining the rationale would be reasonable.

dstogov · 2024-05-08T11:32:21Z

I think an RFC, or at least a list on the mailing list explaining the rationale would be reasonable.

The PR name contains [RFC], but I didn't find the text.

dstogov · 2024-05-08T11:38:22Z

@dstogov See zend_compile.c, the constants are turned into function calls.

Nice hack :), but it needs to be verified (constants may be used in places where calls are not allowed).

Girgias · 2024-05-08T13:42:14Z

The RFC is now live: https://wiki.php.net/rfc/exit-as-function and I have sent an email to internals.

I'm still somewhat ill so I didn't manage to post it yesterday.

iluuu1994 · 2024-05-29T00:04:44Z

As discussed on the list, here's an approach that converts exit/die into the same AST as exit() in the parser. That avoids most changes in zend_compile.c, and preserves BC (for PHP code, but not extensions).
40751f0
It could be extended to handle T_DIE separately, for improved error messages.

And as discussed privately, the stub parser fails when run with a local PHP binary, because exit is still a keyword and not allowed as a function name. As the Makefile prefers local PHP binaries, this needs to be fixed in some other way (e.g. maybe through a magic prefix that the stub generator removes).

rodrigoslayertech · 2024-07-07T12:53:25Z

Good RFC!
Another benefit is that the PHP developers will have a better experience when integrating SAPI CLI development with other non-CLI SAPIs.

For example, if there is a PHP project that integrates HTTP Servers with SAPI non-CLI (which is the PHP standard) with HTTP Server with SAPI CLI, it would be possible to disable exit as a function and prevent exit from completely killing the stack when the code runs in SAPI CLI. In non-CLI SAPIs, exit sends the response to the client, but in CLI, it takes the server offline.

Girgias · 2024-07-07T13:00:04Z

Good RFC! Another benefit is that the developer will have a better experience when integrating SAPI CLI development with other non-CLI SAPIs.

For example, if there is a PHP project that integrates HTTP Servers with SAPI non-CLI (which is the PHP standard) with HTTP Server with SAPI CLI, it would be possible to disable exit as a function and prevent exit from completely killing the stack when the code runs in SAPI CLI. In non-CLI SAPIs, exit sends the response to the client, but in CLI, it takes the server offline.

Note that the RFC prevents adding exit() to the disabled_functions INI setting.

rodrigoslayertech · 2024-07-07T13:02:29Z

Note that the RFC prevents adding exit() to the disabled_functions INI setting.

Yes, but is that in a future scope. That's the intention, right?

Girgias · 2024-07-07T13:09:52Z

Note that the RFC prevents adding exit() to the disabled_functions INI setting.

Yes, but is that in a future scope. That's the intention, right?

It is a possible idea, I don't have any plans on pursuing it as I don't want to argue about the merits of it. But you could do a follow-up RFC if you wanted to!

ext/tokenizer/tests/001.phpt

…and die()

TimWolla

LGTM overall.

build/gen_stub.php

ext/standard/tests/array/array_map_variation16.phpt

ext/ftp/tests/server.inc

TimWolla · 2024-08-14T08:41:04Z

build/gen_stub.php

@@ -83,9 +83,15 @@ function processStubFile(string $stubFile, Context $context, bool $includeOnly =
            }
        }

+        /* As exit() and die() are proper token/keywords an need to hack-around */


Not sure if this is a grammatically correct sentence.

I'll fix it in the commit where I add the NEWS/UPGRADING entry

jrfnl · 2024-08-15T15:12:08Z

Zend/zend_builtin_functions.stub.php

+function exit(string|int $code = 0): never {}
+
+/** @alias exit */
+function die(string|int $code = 0): never {}


The parameter names seem to be incorrect.

It should be $status, not $code.

The RFC explicitly uses that parameter name in the proposal: https://wiki.php.net/rfc/exit-as-function#proposal

It is also the name already used in the manual.

Lastly, the parameter name $status better covers what can be passed: either a status message or a status code.
While $code would read pretty weird when passing a message:

exit(code: 'message');

@Girgias Note: I'll happily try to provide a PR to fix this, including adding a test with named parameter use, but I wanted to check first if this was an oversight or if there was a particular reason for the parameter name change.

#15433 (draft as I've guestimated a patch ;-) )

Follow up on 13483 As previously reported in php#13483 (comment): > The parameter names seem to be incorrect. > > It should be `$status`, not `$code`. > > The RFC explicitly uses that parameter name in the proposal: https://wiki.php.net/rfc/exit-as-function#proposal > > It is also the name already used in the [manual](https://www.php.net/exit). > > Lastly, the parameter name `$status` better covers what can be passed: either a status _message_ or a status _code_. > While `$code` would read pretty weird when passing a message: > ```php > exit(code: 'message'); > ``` This commit attempts to fix this. Includes adding a test for exit/die using a named argument.

Follow up on 13483 As previously reported in #13483 (comment): > The parameter names seem to be incorrect. > > It should be `$status`, not `$code`. > > The RFC explicitly uses that parameter name in the proposal: https://wiki.php.net/rfc/exit-as-function#proposal > > It is also the name already used in the [manual](https://www.php.net/exit). > > Lastly, the parameter name `$status` better covers what can be passed: either a status _message_ or a status _code_. > While `$code` would read pretty weird when passing a message: > ```php > exit(code: 'message'); > ``` This commit attempts to fix this. Includes adding a test for exit/die using a named argument. Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>

petk · 2024-08-18T15:52:57Z

Should the UPGRADING.INTERNALS perhaps also note here that ZEND_EXIT doesn't exist anymore?

Because some extensions will now fail with the next PHP 8.4 release. For example, swoole-src

The list of extensions that uses the ZEND_EXIT is not that long:

uopz
xdebug
vld
pcov
taint
swoole

(some extensions here can probably be also archived and aren't that critical since they don't accept any PRs nor they seem to get moving anywhere)

Girgias · 2024-08-18T16:10:10Z

Ah yes it should

This AST node is no longer used since `exit()` was made a real function in phpGH-13483.

This AST node is no longer used since `exit()` was made a real function in GH-13483.

Girgias added the RFC label Feb 23, 2024

github-actions bot added Category: Engine Extension: ftp Extension: opcache Extension: standard Extension: tokenizer SAPI: phpdbg Category: Optimizer labels Feb 23, 2024

arnaud-lb reviewed Feb 24, 2024

View reviewed changes

Girgias force-pushed the exit-as-function branch 2 times, most recently from 0549f75 to 89aaa8f Compare May 1, 2024 15:07

TimWolla reviewed May 1, 2024

View reviewed changes

Zend/zend_compile.c Outdated Show resolved Hide resolved

Girgias marked this pull request as ready for review May 7, 2024 18:27

Girgias requested review from kocsismate, dstogov and bukka as code owners May 7, 2024 18:27

Girgias force-pushed the exit-as-function branch 2 times, most recently from a9fd352 to 04c54f3 Compare May 11, 2024 14:14

Girgias force-pushed the exit-as-function branch from 04c54f3 to 0b7215b Compare July 26, 2024 10:50

TimWolla reviewed Jul 26, 2024

View reviewed changes

ext/tokenizer/tests/001.phpt Outdated Show resolved Hide resolved

Girgias added 4 commits August 13, 2024 13:41

Revert test change

ce77cdb

Fix zend_compile

f886231

Workaround for stub generation for prior versions of PHP with exit() …

168bb84

…and die()

Address reviews

1f2ca96

Girgias force-pushed the exit-as-function branch from 846afa3 to 1f2ca96 Compare August 13, 2024 11:43

Revert some unnecessary test changes

401b78c

Girgias force-pushed the exit-as-function branch from 36a9449 to 401b78c Compare August 13, 2024 12:05

TimWolla reviewed Aug 13, 2024

View reviewed changes

build/gen_stub.php Outdated Show resolved Hide resolved

ext/standard/tests/array/array_map_variation16.phpt Outdated Show resolved Hide resolved

iluuu1994 approved these changes Aug 13, 2024

View reviewed changes

ext/ftp/tests/server.inc Outdated Show resolved Hide resolved

Final review remarks

ba16695

TimWolla approved these changes Aug 14, 2024

View reviewed changes

Girgias merged commit a79c70f into php:master Aug 14, 2024
10 checks passed

Girgias deleted the exit-as-function branch August 14, 2024 11:44

Girgias added a commit that referenced this pull request Aug 14, 2024

[skip ci] Add NEWS/UPGRADING + wording amendment for GH-13483

d100caa

jrfnl reviewed Aug 15, 2024

View reviewed changes

jrfnl mentioned this pull request Aug 15, 2024

PHP 8.4 | Exit as function: fix incorrect parameter name #15433

Merged

jrfnl mentioned this pull request Aug 15, 2024

Account for changes introduced in PHP 8.4 PHPCompatibility/PHPCompatibility#1731

Open

53 tasks

This was referenced Aug 18, 2024

Swoole build for PHP-8.4 ZEND_EXIT has been removed swoole/swoole-src#5451

Closed

Build fails with PHP 8.4 krakjoe/uopz#184

Open

TimWolla added a commit to TimWolla/php-src that referenced this pull request Jan 9, 2025

zend_ast: Remove unused ZEND_AST_EXIT

634a5de

This AST node is no longer used since `exit()` was made a real function in phpGH-13483.

TimWolla mentioned this pull request Jan 9, 2025

zend_ast: Remove unused ZEND_AST_EXIT #17417

Merged

TimWolla added a commit that referenced this pull request Jan 9, 2025

zend_ast: Remove unused ZEND_AST_EXIT (#17417)

e421a44

This AST node is no longer used since `exit()` was made a real function in GH-13483.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Convert exit (and die) from language constructs to functions #13483

[RFC] Convert exit (and die) from language constructs to functions #13483

Girgias commented Feb 23, 2024 •

edited

Loading

arnaud-lb Feb 24, 2024

iluuu1994 Feb 24, 2024

bwoebi Feb 24, 2024

Girgias Feb 24, 2024

iluuu1994 Feb 24, 2024 •

edited

Loading

TimWolla Feb 25, 2024

bwoebi Feb 25, 2024

Girgias Feb 25, 2024

dseguy Feb 27, 2024

dstogov commented May 8, 2024

iluuu1994 commented May 8, 2024

dstogov commented May 8, 2024

dstogov commented May 8, 2024

Girgias commented May 8, 2024

iluuu1994 commented May 29, 2024 •

edited

Loading

rodrigoslayertech commented Jul 7, 2024 •

edited

Loading

Girgias commented Jul 7, 2024

rodrigoslayertech commented Jul 7, 2024

Girgias commented Jul 7, 2024

TimWolla left a comment

TimWolla Aug 14, 2024

Girgias Aug 14, 2024

jrfnl Aug 15, 2024

jrfnl Aug 15, 2024

jrfnl Aug 15, 2024

petk commented Aug 18, 2024 •

edited

Loading

Girgias commented Aug 18, 2024

[RFC] Convert exit (and die) from language constructs to functions #13483

[RFC] Convert exit (and die) from language constructs to functions #13483

Conversation

Girgias commented Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iluuu1994 Feb 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstogov commented May 8, 2024

iluuu1994 commented May 8, 2024

dstogov commented May 8, 2024

dstogov commented May 8, 2024

Girgias commented May 8, 2024

iluuu1994 commented May 29, 2024 • edited Loading

rodrigoslayertech commented Jul 7, 2024 • edited Loading

Girgias commented Jul 7, 2024

rodrigoslayertech commented Jul 7, 2024

Girgias commented Jul 7, 2024

TimWolla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petk commented Aug 18, 2024 • edited Loading

Girgias commented Aug 18, 2024

Girgias commented Feb 23, 2024 •

edited

Loading

iluuu1994 Feb 24, 2024 •

edited

Loading

iluuu1994 commented May 29, 2024 •

edited

Loading

rodrigoslayertech commented Jul 7, 2024 •

edited

Loading

petk commented Aug 18, 2024 •

edited

Loading