-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Auto-capturing multi-statement closures #6246
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
Conversation
// static can be used to unbind $this | ||
$fn = static fn() => isset($this); | ||
var_dump($fn()); | ||
|
||
$fn = static fn() => { return isset($this); }; | ||
var_dump($fn()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add one more static test and check with reflection that $this
is really not bound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mvorisek Gonna wait for the RFC decision before updating that.
This will need a more sophisticated capture analysis to ensure that
does not perform an unnecessary binding of |
Zend/tests/arrow_functions/001.phpt
Outdated
@@ -3,43 +3,87 @@ Basic arrow function functionality check | |||
--FILE-- | |||
<?php | |||
|
|||
// No return value | |||
$foo = fn() => {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, why fn() => {}
rather than just fn() {}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic I feel the => looks better and just follows the consistency of other languages like Typescript / Javascript / Dart, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it may be consistent with other languages, it is more important to be internally consistent within PHP. Especially when I view this in conjunction with #6221, which introduces => expr
for method bodies, it seems like we should be interpreting => expr;
as a shorthand for { return expr; }
. In this context => { stmt; }
does not make sense syntactically.
Of course, there is also an alternative view here: If we add general support for block expressions, then the syntax => { stmt; }
would make sense. But that would have larger implications, in that it should also be usable outside the context of arrow functions. You will want to read up on the discussions we had on this when considering block expressions for match
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree on your point here. I was not aware of #6221, but it does in fact make sense. I would consider your second point as some sort of block expression like Rust have. Could you point me to the discussion regarding block expressions? It peaked my curiosity ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit spread out, https://externals.io/message/109941#109947, #5403, #5448 seem relevant, there's probably more in the other match discussion threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic I will make sure read those discussions before making an RFC. It may take a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think '=>' is not necessary since PHP has the 'fn' keyword
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed here: b82fe0c.
Thank you, @nunomaduro |
This looks wonderful, hope you get it rolling, make an RFC, and get it into voting before the PHP 8.1 release. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@nikic I've just updated the pull request so it makes more sense together with Short Functions. This way, only single line return expressions would have the $values = array_filter($values, fn ($value) { // no more `=>` symbol. |
cc @Crell |
I don't really have strong feelings on this, frankly. The |
Thanks a lot for this PR, @nunomaduro! I really hope that this proposal will move forward and be accepted. I just posted another message on Side question: would it be possible to drop the Like this: $connection->transactional({
// do stuff
}); Right now it's a syntax error, and so is this: $a = {}; So I don't think this would conflict with any current syntax? |
@BenMorel how would it accept params? --> probably no :) |
@mvorisek It wouldn't. This would be an alternate syntax that creates a closure with no parameters and inheriting the outside scope. |
@BenMorel yet another alternate syntax just to save a few keystrokes and to add mental overhead when reading code. Not a fan tbh. |
I think that And if you had |
Alternate brainstorm: So you'd get: fn($x, $y) => {{
blah;
blah;
return blah;
}};
match($beep) {
'beep' => {{
blah;
blah;
return blah;
}},
}; That would, in combination with short functions, technically make this feasible: function foo($x, $y) => {{
blah;
blah;
return blah;
}}; Which is somewhat weird but I don't think would hurt anything. |
b82fe0c
to
64617a9
Compare
RFC under discussion on PHP Mailing Lists > php.internals: https://wiki.php.net/rfc/auto-capture-closure. |
c3a50d6
to
d11a8f2
Compare
When are you going to put this up for a vote? |
@nunomaduro wanted to try and find time to investigate some memory optimizations, but hasn't been able to yet. It will be going to a vote as soon as he tells me he's either done so or isn't going to have time to do so. :-) |
Add also these tests:
and
|
may i ask why it was up for vote, had a bunch... but then got reset? was the publishing for voting originally in error? |
nah, the daily digest the mailing list just sent explained why. |
d11a8f2
to
4a266b4
Compare
@nunomaduro, @Crell, are you planning to pursue the RFC for PHP 8.2? |
I think that mainly depends on if @nunomaduro is able to do the memory optimizations discussed previously. If yes, then probably. |
Thanks for the swift reply, @Crell and @nunomaduro! AIUI, it's not really necessary to have memory optimizations (at least for now). Quoting from a relevant mail by @nikic:
So what should be done is measuring/comparing the performance, and to add relevant clarification to the RFC. IOW, no implementation enhancement are strictly necessary. |
I hope this one will make it to PHP 8.2. As we know, the requirement for manual capturing of variables with annoying use statement is one of the worst features from PHP, there aint another language that forces you to do this. It is enough a big problem that I dont use anonymous functions/closures beyond single line statement(thanks to PHP 7.4 for the short closure), or simple multi-line statements that do not need to capture any variables(yes I dont write use statement for anonymous functions, its a blasphemy to me). However, its quite reasonable that we will need multi-line(at least 3-5 lines are rather common) statement in closures that will need to capture 1-2 outer variables, so adding this to PHP will be helpful. I dont understand why some delusional people were against this saying its easy to abuse. Come on, every other mainstream language that supports closures work this way, what makes one think its prone to being abused in PHP? Most of these people have large chunk of procedural code with hundreds of variables who worry about accidentally capturing variables they dont need, then I am sorry its their problem for writing spaghetti code(and the easy solution to their problem is, just to use traditional anonymous functions, its not going to disappear anyway). Auto-capturing actually helps them avoid writing such long procedural spaghetti code, its a benefit rather than disadvantage to me. Edit: lol why the dislikes? I cant imagine any sane developers who actually enjoy PHP's tedious use statement for manual capturing. Still, no one can provide me with an example that another mainstream language requires such use statement like PHP does currently(apparently this so-called scope-leak is never an issue for developers using other languages), its an abomination that needs to go away. |
@arnaud-lb is working on a new implementation with live variable analysis that only captures variables that aren't immediately overwritten (which was the main concern with this implementation). #8330 @nunomaduro Is it ok with you if I close this PR? |
@iluuu1994 Done. ✅ |
As you may already know, PHP 7.4 has introduced one-liner arrow functions (aka short closures). Now, this pull request adds the possibility of those arrow functions to be multi-line. Let's see an example:
This pull request should be targeting PHP 8.1, and it will be followed by an RFC in the next couple of days - but in short, the advantages are:
use
keyword to be able to access data from the outer scope.fn (/** */) {
is just shorter and simpler thanfunction (/** */) use (/** */) {
.It is my first contribution to the core of PHP, so let me know if something in this pull request needs to be better. 👍🏻