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

Add short closures / arrow functions #3941

Closed
wants to merge 17 commits into from
Closed

Conversation

@nikic
Copy link
Member

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

This comment has been minimized.

Copy link
Member

carusogabriel commented Mar 13, 2019

@nikic Will this syntax be allowed:

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

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

?

@kunicmarko20

This comment has been minimized.

Copy link

kunicmarko20 commented Mar 13, 2019

@carusogabriel If I am correct, this short syntax is for closures only and has nothing to do with methods.

https://github.com/php/php-src/pull/3941/files#diff-7eff82c2c5b45db512a9dc49fb990bb8R170
https://github.com/php/php-src/pull/3941/files#diff-93ad74868f98ff7232ebea00007c8b7fR1270

Based on this you can see that it recognises fn as a Token.

@nikic

This comment has been minimized.

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

@tfont

This comment has been minimized.

Copy link

tfont 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

This comment has been minimized.

Copy link
Member

KalleZ commented Mar 13, 2019

@tfont Please post feedback on the thread on internals

@SammyK

This comment has been minimized.

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

This comment has been minimized.

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 nikic:arrow_function_2 branch from 7188a72 to 1148cbd Apr 23, 2019
@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Apr 23, 2019

@SammyK Thanks, this is fixed now.

@bwoebi

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

jbis9051 commented Apr 25, 2019

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

@theodorejb

This comment has been minimized.

Copy link
Contributor

theodorejb commented Apr 25, 2019

@jbis9051 The syntax is discussed at length in the RFC: https://wiki.php.net/rfc/arrow_functions_v2#syntax.

@jbis9051

This comment has been minimized.

Copy link

jbis9051 commented Apr 25, 2019

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

@jmleroy

This comment has been minimized.

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

This comment has been minimized.

Copy link

jbis9051 commented Apr 25, 2019

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

@nikic

This comment has been minimized.

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 nikic:arrow_function_2 branch from 1148cbd to 5ad4de6 May 2, 2019
nikic added 7 commits May 2, 2019
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

This comment has been minimized.

Copy link
Member Author

nikic commented May 2, 2019

Merged as f3e5bbe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.