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

RFC: Support Closures in constant expressions #16458

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Oct 16, 2024

Most notably this allows to use Closures as Attribute parameters.

RFC: https://wiki.php.net/rfc/closures_in_const_expr

@bwoebi
Copy link
Member

bwoebi commented Oct 21, 2024

May I suggest automatically making Closures in constants static? (just the same like if you were to declare some closure without static on top-level code)
Non-static Closures of attributes probably could trivially get a $this reference to the instance of the attribute they are on.

I realize though that for properties we should require static for now. (unless you are going to provide properties with $this on new SomeClass...)

@TimWolla TimWolla force-pushed the closure-in-cost-expr branch from 8b31603 to a75d04c Compare October 21, 2024 11:36
@TimWolla
Copy link
Member Author

May I suggest automatically making Closures in constants static? (just the same like if you were to declare some closure without static on top-level code)

@bwoebi It does not look like top-level closures are implicitly static:

https://3v4l.org/dJiqK

<?php

class Foo { }

$foo = function () {
    var_dump($this);
};

$foo->call(new Foo());

Independent of that, I would value the explicitness of needing to specify the static, because that would allow a clear path towards supporting non-static closures (e.g. for Attributes) in a follow-up.

I realize though that for properties we should require static for now. (unless you are going to provide properties with $this on new SomeClass...)

Yes. I already had a quick chat with @iluuu1994 about this and we agreed that requiring the closure to be static would be a good decision for the initial version of this feature.

@TimWolla TimWolla added the RFC label Oct 24, 2024
@bwoebi
Copy link
Member

bwoebi commented Oct 25, 2024

@TimWolla Oh, yes, there's a difference between static and unbound. But is there any reason a top-level closure should not be unbound?
Like, why would you not permit:

class Foo {}
const CO = function () { var_dump($this); };
CO->call(new Foo);

The only problem is with Closures within classes.

@TimWolla
Copy link
Member Author

But is there any reason a top-level closure should not be unbound?

I assume that would technically be supportable in the exact locations where a new expression would be legal?

Like, why would you not permit:

I believe that the usefulness of a top-level closure is very limited (why would you not write a regular function?). For my initial proposal I want to keep the feature easy to reason about (“closures in const-expressions must always be static”) and also keep the implementation simple(r).

@TimWolla TimWolla requested a review from iluuu1994 October 29, 2024 15:28
@TimWolla TimWolla marked this pull request as ready for review November 19, 2024 07:40
@TimWolla TimWolla requested a review from dstogov as a code owner November 19, 2024 07:40
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

The patch looks good. I don't see problems, but I can't think about all possible consequences.

Zend/zend_ast.c Outdated Show resolved Hide resolved
@TimWolla TimWolla changed the title Allow defining Closures in const-expr RFC: Support Closures in constant expressions Nov 27, 2024
@TimWolla TimWolla force-pushed the closure-in-cost-expr branch 2 times, most recently from c534e21 to bc01615 Compare November 27, 2024 14:57
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

It looks like zend_ast_get_lineno() lacks handling for ZEND_AST_OP_ARRAY.

Couldn't find any other issues.

Zend/tests/closure_const_expr/attributes.phpt Show resolved Hide resolved
Zend/zend_ast.c Outdated Show resolved Hide resolved
Zend/zend_ast.c Outdated Show resolved Hide resolved
Zend/zend_compile.c Outdated Show resolved Hide resolved
@TimWolla
Copy link
Member Author

It looks like zend_ast_get_lineno() lacks handling for ZEND_AST_OP_ARRAY.

Indeed it does. It appears to be unobservable as of now, since ZEND_AST_OP_ARRAY by itself is infallible, but this nevertheless is a bug.

@TimWolla TimWolla requested a review from iluuu1994 November 28, 2024 09:01
@iluuu1994
Copy link
Member

I missed this yesterday, but it seems you're missing support for file cache in ext/opcache/zend_file_cache.c. The rest LGTM.

@TimWolla TimWolla linked an issue Nov 28, 2024 that may be closed by this pull request
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

I see no more issues.

Zend/zend_ast.c Outdated Show resolved Hide resolved
Zend/zend_compile.c Outdated Show resolved Hide resolved
@TimWolla TimWolla force-pushed the closure-in-cost-expr branch from 469df0e to bc0539b Compare December 2, 2024 16:23
RFC: https://wiki.php.net/rfc/closures_in_const_expr

Co-authored-by: Volker Dusch <volker@tideways-gmbh.com>
Co-authored-by: Ilija Tovilo <ilija.tovilo@me.com>
Co-authored-by: Arthur Kurbidaev <artkurbidaev@gmail.com>
@TimWolla TimWolla force-pushed the closure-in-cost-expr branch from bc0539b to 609de12 Compare December 2, 2024 16:34
@TimWolla TimWolla merged commit f6a0bb4 into php:master Dec 2, 2024
10 checks passed
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.

Static anonymous functions as default property value
6 participants