Skip to content
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

Short functions #6221

Closed
wants to merge 9 commits into from
Closed

Short functions #6221

wants to merge 9 commits into from

Conversation

Crell
Copy link
Contributor

@Crell Crell commented Sep 26, 2020

Offer an abbreviated syntax for functions and methods, just like closures have.

@nikic nikic added the RFC label Sep 26, 2020
@tuqqu
Copy link

tuqqu commented Sep 26, 2020

wouldn't it introduce an inconsistency in various function syntax? Syntax of functions/methods would become either function x() {} or function x() => ; and syntax of closures is function () {} or fn() => ;

@azjezz
Copy link

azjezz commented Sep 27, 2020

I have to agree with @tuqqu, this brings more inconsistency in the language.

Functions Anonymous functions consistent
function a() { ... } $a = function() { ... }; ✔️
function a() => ...; $a = fn() => ...;
fn a() => ...; $a = fn() => ...;
fn a() => ... $a = fn() => ...; ✔️

A consistent syntax would be:

<?php

fn pick_one(int $a) => match($a) {
    1 => 'One',
    2 => 'Two',
    3 => 'Three',
    default => 'More',
} // <-- no semicolon



fn test(int $a) => $a + 1

fn test2(int $b): int
    => $b + 1

@zmitic
Copy link

zmitic commented Oct 6, 2020

Can we have this feature in PHP8 or its features have already been frozen? It would be amazing to cut getters/setters from 4 lines to just 1.

@Girgias
Copy link
Member

Girgias commented Oct 6, 2020

Can we have this feature in PHP8 or its features have already been frozen? It would be amazing to cut getters/setters from 4 lines to just 1.

Features have been frozen since July.

@sgolemon
Copy link
Contributor

I'm with @tuqqu and @azjezz both for consistency and just for being that much more terse. 👍

fn foo(...) => expr;

@Crell
Copy link
Contributor Author

Crell commented Oct 20, 2020

@sgolemon I'm open to it if you can help me figure out how to modify the lexer for that without breaking the entire class lexing structure. 😄

@sgolemon
Copy link
Contributor

sgolemon commented Oct 21, 2020

@Crell Rewriting the syntax is easy enough: a8f0185

Edit: Nevermind. In trying to see what it looks like I'm remembering why we added fn in the first place. Trying to equivalentize those isn't going to work.

Edit of edit: Actually, I think I see a way out of the conflict, but the idea was already just a spitball and I didn't entirely love it anyway.

But I've actually done a little more thinking about whether we should.

What if we de-couple function/fn from {}/=> ? The latter is already our real distinction, but we can maybe consider (as a separate RFC discussion) making function/fn usable interchangably. So all of the following would be valid:

function foo() { return BAR; }
fn foo() { return BAR; }
function foo() => BAR;
fn foo() => BAR;

function() { return BAR; };
fn() { return BAR; };
function() => BAR;
fn() => BAR;

We'd still be left with the fact that only the last two have auto-capture which is not great, but it's a pre-existing condition and orthogonal to the rest of this IMO.

@dtakken
Copy link

dtakken commented Oct 21, 2020

wouldn't it introduce an inconsistency in various function syntax? Syntax of functions/methods would become either function x() {} or function x() => ; and syntax of closures is function () {} or fn() => ;

While I agree that this introduces an inconsistency, I really do not like mixing function and fn in a class context:

class Foo
{
    public function one() { ... }
    public fn two() => ...;
    public function three() { ... }
}

The mixed fn / function makes it harder to see that these are just three public functions and one of them happens to be a single expression. A lot of cognitive noise for a syntactic detail. Compare:

class Foo
{
    public function one() { ... }
    public function two() => ...;
    public function three() { ... }
}

I would rather use fn for anonymous functions and function for named functions. That is a different way to reach consistency. However, that would require another RFC to allow using fn for bracketed anonymous functions in stead of function. With that change, anonymous function syntax variants would be:

$a = fn () { ... };  // <-- Requires new RFC
$a = fn => ...;

while named function variants would be:

function foo() { ... }
function foo() => ...;

@Crell Crell mentioned this pull request Oct 23, 2020
@jellynoone
Copy link
Contributor

@Crell any news on this RFC or just busy with enumerations? Hope this is the right place to ask?

@Crell
Copy link
Contributor Author

Crell commented Mar 18, 2021

I am planning to sync up with the person working on auto-capture long-lambdas to make sure the syntax we are proposing in the two RFCs is complementary, not conflicting. After we get that sorted out, there probably isn't much more to do besides call a vote, unless I decide to wait for some related RFCs to land (pipe operator, which is blocked by partial application) to help strengthen the argument.

@jellynoone
Copy link
Contributor

Thank you for the explanation, there hasn't been much discussion about the auto-capture long-lambdas and this RFC for a long time so I was worried it might have been forgotten. Good luck on the RFC. :)

Zend/zend_language_parser.y Outdated Show resolved Hide resolved
@@ -548,6 +548,10 @@ function_declaration_statement:
backup_fn_flags '{' inner_statement_list '}' backup_fn_flags
{ $$ = zend_ast_create_decl(ZEND_AST_FUNC_DECL, $2 | $13, $1, $4,
zend_ast_get_str($3), $6, NULL, $11, $8, NULL); CG(extra_fn_flags) = $9; }
| function returns_ref T_STRING backup_doc_comment '(' parameter_list ')' return_type
backup_fn_flags T_DOUBLE_ARROW expr ';' backup_fn_flags
{ $$ = zend_ast_create_decl(ZEND_AST_FUNC_DECL, $2, $1, $4,
Copy link
Contributor

Choose a reason for hiding this comment

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

easy to fix: Should this be $2 | $13 instead like the above declaration? The last occurrence of backup_fn_flags is $13, and that includes flags set while parsing the expr

Can you add a test of the expected behavior of return yield $param for stdClass $param?

	|	T_YIELD { $$ = zend_ast_create(ZEND_AST_YIELD, NULL, NULL); CG(extra_fn_flags) |= ZEND_ACC_GENERATOR; }
	|	T_YIELD expr { $$ = zend_ast_create(ZEND_AST_YIELD, $2, NULL); CG(extra_fn_flags) |= ZEND_ACC_GENERATOR; }
	|	T_YIELD expr T_DOUBLE_ARROW expr { $$ = zend_ast_create(ZEND_AST_YIELD, $4, $2); CG(extra_fn_flags) |= ZEND_ACC_GENERATOR; }
	|	T_YIELD_FROM expr { $$ = zend_ast_create(ZEND_AST_YIELD_FROM, $2); CG(extra_fn_flags) |= ZEND_ACC_GENERATOR; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't even though to test generators with this syntax, good call.

I tried it, and function read($it): iterable => yield from $it; does work already, albeit that's not very useful. yield from $a where $a is a stdClass gives an error on its own, so that seems pre-existing. I'm not sure what you mean to test here.

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 not sure what you mean to test here.

I would have thought there would be some sort of impact of not adding those flags to the ZEND_AST_FUNC_DECL

/* {{{ Returns whether this function is a generator */
ZEND_METHOD(ReflectionFunctionAbstract, isGenerator)
{
	_function_check_flag(INTERNAL_FUNCTION_PARAM_PASSTHRU, ZEND_ACC_GENERATOR);
}
/* }}} */

E.g. adding a test of ReflectionFunction->isGenerator is one thing I had in mind, or whatever ZEND_ACC_GENERATOR would affect.

var_dump((new ReflectionFunction('read'))->isGenerator());

No longer necessary since this was refactored and those flags from the function body now get passed in

Copy link
Contributor

Choose a reason for hiding this comment

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

In case anyone's wondering, return yield from does return something useful - if the expression for yield from is a generator, it returns Generator->getReturn() - otherwise it returns null.

With that said, I think use cases would be very rare, but exist.

php > function example() { yield 'first'; return 'retval'; }
php > function test() { echo "in test\n"; return yield from example(); }
php > $gen = test();
php > foreach ($gen as $value) { echo "value: $value\n"; }
in test
value: first
php > var_export($gen->getReturn());
'retval'
	/* This is the default return value
	 * when the expression is a Generator, it will be overwritten in zend_generator_resume() */
	if (RETURN_VALUE_USED(opline)) {
		ZVAL_NULL(EX_VAR(opline->result.var));
	}

@Crell
Copy link
Contributor Author

Crell commented Jun 15, 2021

RFC failed: https://wiki.php.net/rfc/short-functions

The included cleanup has been redone in #7154, with no functional changes.

Thanks everyone!

@Crell Crell closed this Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants