Skip to content

feat: single expression functions #17677

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

xepozz
Copy link
Contributor

@xepozz xepozz commented Feb 3, 2025

I'd like to introduce single expression function declarations, as long as Kotlin has: https://kotlinlang.org/docs/functions.html#single-expression-functions

Example: https://pl.kotl.in/u_4w9vJmj

SEF supports expressions, but seems like statements are also supported in some limited way.
E.g. fun statement2() = a = 2 cause error.

I'd support statements in PHP as well, cause there are no difference between returning NULL or a result of expression.
I'd ask for help how to implement short functions always return expression result. I think I need to create a new ast node like short declaration does and implement logic, or it's better to reuse short declaration logic?

RFC: https://wiki.php.net/rfc/single-expression-functions

@iluuu1994
Copy link
Member

This feature was proposed and declined a few years ago. Enough time has passed to propose it again, but it would be beneficial to read the corresponding threads and understand the objections.

https://wiki.php.net/rfc/short-functions
https://externals.io/message/113751

// function buz(): int = 456; // TODO: must be the same as return 456;
// echo buz() . PHP_EOL;

function compare(int $what, int $with): string = switch(true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually kind of shocked that a switch works here, since I thought it was all statements. I would expect match() to be much more commonly used here.

Checking switch() is fine, but please include a match() test as well, as that seems the more common use case.

Copy link
Member

@iluuu1994 iluuu1994 May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really work. If this were a match, the value would fail to be returned, and we'd just return null. This only works because of the explicit return statements in the switch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request: Give this file a better name than "2" to say what it's testing that is different from the other file, and update the test description accordingly.

zend_ast_get_str($3), $6, NULL, $11, $8, NULL); CG(extra_fn_flags) = $9; }
backup_fn_flags function_body backup_fn_flags
{ $$ = zend_ast_create_decl(ZEND_AST_FUNC_DECL, $2 | $11, $1, $4,
zend_ast_get_str($3), $6, NULL, $10, $8, NULL); CG(extra_fn_flags) = $9; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look correct. function foo() = 'bar'; will result in the equivalent of function foo() { 'bar'; return null; }, which doesn't have the desired effect.

@xepozz
Copy link
Contributor Author

xepozz commented May 27, 2025

Is there a reason to develop the feature further without RFC success?

@Crell
Copy link
Contributor

Crell commented May 27, 2025

Generally speaking, at least some voters want to see a working implementation before they vote. Sometimes implementation details do matter (weird side effects, performance, etc.). It doesn't have the be the absolute perfectly polished PR, but at least something that works and is close to the final form.

Also, I wouldn't call the limited discussion on the list so far "without success." If anything it's more positive than I got when proposing my version originally. :-) (You're welcome to borrow liberally from the code and tests there if you'd like.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants