Skip to content

Add short closures / arrow functions #3941

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

Closed
wants to merge 17 commits into from
Closed

Conversation

nikic
Copy link
Member

@nikic nikic commented Mar 13, 2019

Implementation for https://wiki.php.net/rfc/arrow_functions_v2.

This is based on Levi's old implementation of the same.

@nikic nikic added the RFC label Mar 13, 2019
@carusogabriel
Copy link
Contributor

@nikic Will this syntax be allowed:

public class MyClass
{
    private static int $i = 0;

    public static increment(): int => ++self::$i;
}

?

@kunicmarko20
Copy link

@nikic
Copy link
Member Author

nikic commented Mar 13, 2019

@carusogabriel No, this is not supported. It's listed as a possible future extension though: https://wiki.php.net/rfc/arrow_functions_v2#allow_arrow_notation_for_real_functions

@travisfont
Copy link

travisfont commented Mar 13, 2019

Arrow functions are ternary operators to functions.

While they are nice and shorten, they can be hard to read at times; considerably to people who aren't used to them which is surprisedly a majority of PHP programmers.

Having them optional sure, but not necessary.

@KalleZ
Copy link
Member

KalleZ commented Mar 13, 2019

@tfont Please post feedback on the thread on internals

@SammyK
Copy link
Contributor

SammyK commented Apr 21, 2019

This is fantastic - great work folks! :)

I know this PR is still a WIP, but I was playing with it I was able to cause a segfault where I expected a parse error with this little repro:

fn() ==> 42;

This might already be on your radar but just wanted to report it just in case. :)

Backtrace:

php!zend_ast_destroy (/Users/SammyK/Sites/_forks/php-src/Zend/zend_ast.c:789)
php!yydestruct (/Users/SammyK/Sites/_forks/php-src/Zend/zend_language_parser.y:51)
php!zendparse (/Users/SammyK/Sites/_forks/php-src/Zend/zend_language_parser.c:7038)
php!zend_compile (/Users/SammyK/Sites/_forks/php-src/Zend/zend_language_scanner.l:584)
php!compile_file (/Users/SammyK/Sites/_forks/php-src/Zend/zend_language_scanner.l:637)
php!phar_compile_file (/Users/SammyK/Sites/_forks/php-src/ext/phar/phar.c:3347)
php!zend_execute_scripts (/Users/SammyK/Sites/_forks/php-src/Zend/zend.c:1642)
php!php_execute_script (/Users/SammyK/Sites/_forks/php-src/main/main.c:2635)
php!do_cli (/Users/SammyK/Sites/_forks/php-src/sapi/cli/php_cli.c:992)
php!main (/Users/SammyK/Sites/_forks/php-src/sapi/cli/php_cli.c:1384)
libdyld.dylib!start (Unknown Source:0)

@nikic
Copy link
Member Author

nikic commented Apr 22, 2019

Note to self: fn needs to be added to the semi-reserved identifier list.

@nikic nikic force-pushed the arrow_function_2 branch from 7188a72 to 1148cbd Compare April 23, 2019 07:56
@nikic
Copy link
Member Author

nikic commented Apr 23, 2019

@SammyK Thanks, this is fixed now.

@bwoebi
Copy link
Member

bwoebi commented Apr 23, 2019

Can you please add a test for the case where a parameter variable also exists in the parent scope? (ensure that nothing is overwritten, because binding happens only after recv)

e.g.

--FILE--
<?php

$x = 1;
var_dump((fn($x) => $x)(2));
var_dump($x);
var_dump((fn($x = 3) => $x)());
var_dump($x);

?>
--EXPECT--
int(2)
int(1)
int(3)
int(1)

@jbis9051
Copy link

@nikic Why even have the fn token? I think it should be similar to JS and just () => .

@theodorejb
Copy link
Contributor

@jbis9051
Copy link

@theodorejb I should probably read the entire proposal before asking questions.....sorry

@jmleroy
Copy link

jmleroy commented Apr 25, 2019

@theodorejb I cannot find the reason of creating fn instead of reusing function, unless it is for the sake of sparing 6 extra characters.

@jbis9051
Copy link

@jmleroy Its shorter. I would even rather f()=>.

@nikic
Copy link
Member Author

nikic commented Apr 26, 2019

While brevity is a factor, we also want to have some stronger syntactical distinction between closures that automatically capture scope and those that don't. This especially becomes a problem when you take into account that we may add a block variant of this syntax in the future.

@nikic nikic force-pushed the arrow_function_2 branch from 1148cbd to 5ad4de6 Compare May 2, 2019 08:42
nikic added 7 commits May 2, 2019 10:55
Instead of using an empty use list to distinguish them.

This also fixes the pretty printing to print an arrow function.
There's still a bug here because we're missing the parentheses for
the direct call, but that's an existing issue.
Also rename T_ARROW_FUNCTION to PREC_ARROW_FUNCTION, to make it
obvious that this is not a real token.
@nikic
Copy link
Member Author

nikic commented May 2, 2019

Merged as f3e5bbe.

@al-amindev
Copy link

al-amindev commented Jul 2, 2020

How to write this one

$factorial = function ($n) use (&$factorial) {
    return $n == 1 ? 1 : $factorial($n - 1) * $n;
};

echo $factorial(5);

arrow syntax?

@theodorejb
Copy link
Contributor

@mdalamincse Not all closures can be written as arrow functions, since they only support a single statement and don't capture variables by reference.

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.