/php-src

Add Closure::fromCallable(). #1906

Merged
merged 4 commits into from Jul 5, 2016
+676 −0

Labels

PHP7.1 RFC

Milestone

No milestone

Assignees

No one assigned

4 participants

@Danack @krakjoe @weltling @php-pulls
@Danack
Danack commented May 15, 2016

Add the ability to create closures from callable as part of RFC: https://wiki.php.net/rfc/closurefromcallable

@Danack Danack Add Closure::fromCallable(). 
Add the ability to create closures from callable as part of RFC: https://wiki.php.net/rfc/closurefromcallable
63ca65d
@pp3345 pp3345 commented on an outdated diff May 15, 2016
View full outdated diff
Zend/zend_closures.c
+ call.arg_info = (zend_internal_arg_info *) mptr->common.prototype;
+ call.scope = mptr->common.scope;
+
+ zend_free_trampoline(mptr);
+ mptr = (zend_function *) &call;
+ }
+
+ ZVAL_OBJ(&instance, fcc.object);
+ zend_create_closure(return_value, mptr, mptr->common.scope, fcc.object ? fcc.object->ce : NULL, fcc.object ? &instance : NULL);
+
+ return SUCCESS;
+}
+
+
+/* {{{ proto Closure Closure::fromCallable(callable callable)
+ Create a closure from a callabl using the current scope. */
@pp3345
pp3345 added a note May 15, 2016

typo: callabl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dstogov dstogov commented on the diff May 16, 2016
View full changes
Zend/zend_closures.c
+
+ if (!zend_is_callable_ex(callable, NULL, 0, NULL, &fcc, error)) {
+ return FAILURE;
+ }
+
+ mptr = fcc.function_handler;
+ if (mptr == NULL) {
+ return FAILURE;
+ }
+
+ if (mptr->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE) {
+ zend_internal_function call;
+ memset(&call, 0, sizeof(zend_internal_function));
+
+ call.type = ZEND_INTERNAL_FUNCTION;
+ call.handler = zend_closure_call_magic;
@dstogov
dstogov added a note May 16, 2016

Call through zend_call_function() is more expensive than through TRAMPOLINE.
Why it's not possible to reuse the same trampoline?
May be it makes sense to move trampoline function into zend_closure and free it together with closure object...

@Danack
Danack added a note Jun 1, 2016

Dmitry - I don't really understand trampolines. So long as this PR is acceptable and the tests all pass, is it okay for you to do any performance improvements you think are good, after this PR is accepted and closed?

@dstogov
dstogov added a note Jun 2, 2016

Yeah. Commit this. I'll take a look, if it's possible to optimise this using trampolines later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikic nikic and 2 others commented on an outdated diff May 16, 2016
View full outdated diff
ext/reflection/php_reflection.c
@@ -6793,6 +6806,10 @@ static const zend_function_entry reflection_zend_extension_functions[] = {
};
/* }}} */
+ZEND_BEGIN_ARG_INFO_EX(arginfo_closure, 0, 0, 1)
+ ZEND_ARG_INFO(0, callable)
+ZEND_END_ARG_INFO()
+
@nikic
nikic added a note May 16, 2016 • edited

What's this for? (And really, all the changes in this file).

@Bilge
Bilge added a note May 16, 2016

More changes = more enterprise.

@Danack
Danack added a note May 16, 2016

@nikic I am bad with computers. I'll clean it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikic nikic commented on the diff Jun 4, 2016
View full changes
.../tests/closures/cloure_from_callbable_reflection.phpt
@@ -0,0 +1,48 @@
+--TEST--
+Imagick::readImage test
@nikic
nikic added a note Jun 4, 2016 • edited

Nope :P

(And the SKIPIF)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikic nikic commented on the diff Jun 4, 2016
View full changes
Zend/zend_closures.c
+
+
+/* {{{ proto Closure Closure::fromCallable(callable callable)
+ Create a closure from a callable using the current scope. */
+ZEND_METHOD(Closure, fromCallable)
+{
+ zval *callable;
+ int success;
+ char *error = NULL;
+
+ if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &callable) == FAILURE) {
+ return;
+ }
+
+ if (Z_TYPE_P(callable) == IS_OBJECT && instanceof_function(Z_OBJCE_P(callable), zend_ce_closure)) {
+ // It's already a closure
@nikic
nikic added a note Jun 4, 2016

For C89 compliance, should use /**/ style comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikic nikic commented on the diff Jun 4, 2016
View full changes
Zend/zend_closures.c
+ zval_ptr_dtor(&fci.params[1]);
+ efree(fci.params);
+}
+
+
+static int zend_create_closure_from_callable(zval *return_value, zval *callable, char **error) {
+ zend_fcall_info_cache fcc;
+ zend_function *mptr;
+ zval instance;
+
+ if (!zend_is_callable_ex(callable, NULL, 0, NULL, &fcc, error)) {
+ return FAILURE;
+ }
+
+ mptr = fcc.function_handler;
+ if (mptr == NULL) {
@nikic
nikic added a note Jun 4, 2016

Can this happen?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikic nikic commented on the diff Jun 4, 2016
View full changes
Zend/zend_closures.c
+ if (mptr->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE) {
+ zend_internal_function call;
+ memset(&call, 0, sizeof(zend_internal_function));
+
+ call.type = ZEND_INTERNAL_FUNCTION;
+ call.handler = zend_closure_call_magic;
+ call.function_name = mptr->common.function_name;
+ call.arg_info = (zend_internal_arg_info *) mptr->common.prototype;
+ call.scope = mptr->common.scope;
+
+ zend_free_trampoline(mptr);
+ mptr = (zend_function *) &call;
+ }
+
+ ZVAL_OBJ(&instance, fcc.object);
+ zend_create_closure(return_value, mptr, mptr->common.scope, fcc.object ? fcc.object->ce : NULL, fcc.object ? &instance : NULL);
@nikic
nikic added a note Jun 4, 2016

This should use zend_create_fake_closure, otherwise there will be much sadness.

@nikic
nikic added a note Jun 4, 2016

I'm not sure that setting called_scope to NULL for static calls is what you want to do. Please check how something like this behaves:

class Foo {
    const BAR = 1;
    public static function method() {
        return static::BAR;
    }
}
var_dump(Closure::fromCallable(['Foo', 'method'])());
@bwoebi
bwoebi added a note Jul 6, 2016

This case works as expected

@nikic
nikic added a note Jul 6, 2016

Yes, because I already fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikic nikic commented on the diff Jun 4, 2016
View full changes
Zend/zend_closures.c
+ if (mptr == NULL) {
+ return FAILURE;
+ }
+
+ if (mptr->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE) {
+ zend_internal_function call;
+ memset(&call, 0, sizeof(zend_internal_function));
+
+ call.type = ZEND_INTERNAL_FUNCTION;
+ call.handler = zend_closure_call_magic;
+ call.function_name = mptr->common.function_name;
+ call.arg_info = (zend_internal_arg_info *) mptr->common.prototype;
+ call.scope = mptr->common.scope;
+
+ zend_free_trampoline(mptr);
+ mptr = (zend_function *) &call;
@nikic
nikic added a note Jun 4, 2016

This retains a reference to a stack value outside the block it was declared in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikic nikic commented on the diff Jun 4, 2016
View full changes
Zend/zend_closures.c
+ return FAILURE;
+ }
+
+ mptr = fcc.function_handler;
+ if (mptr == NULL) {
+ return FAILURE;
+ }
+
+ if (mptr->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE) {
+ zend_internal_function call;
+ memset(&call, 0, sizeof(zend_internal_function));
+
+ call.type = ZEND_INTERNAL_FUNCTION;
+ call.handler = zend_closure_call_magic;
+ call.function_name = mptr->common.function_name;
+ call.arg_info = (zend_internal_arg_info *) mptr->common.prototype;
@nikic
nikic added a note Jun 4, 2016

This doesn't look like the right member?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikic nikic commented on the diff Jun 4, 2016
View full changes
Zend/zend_closures.c
@@ -235,6 +235,105 @@ ZEND_METHOD(Closure, bind)
}
/* }}} */
+static void zend_closure_call_magic(INTERNAL_FUNCTION_PARAMETERS) {
+ zend_fcall_info fci;
+ zend_fcall_info_cache fcc;
+
+ memset(&fci, 0, sizeof(zend_fcall_info));
+ memset(&fci, 0, sizeof(zend_fcall_info_cache));
+
+ fci.size = sizeof(zend_fcall_info);
+ fci.retval = return_value;
+
+ fcc.initialized = 1;
+ fcc.function_handler = (zend_function *) EX(func)->common.arg_info;
+ fci.params = (zval*) emalloc(sizeof(zval) * 2);
@nikic
nikic added a note Jun 4, 2016

Better use stack allocation instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikic nikic commented on the diff Jun 4, 2016
View full changes
Zend/zend_closures.c
+ fci.params = (zval*) emalloc(sizeof(zval) * 2);
+ fci.param_count = 2;
+ ZVAL_STR(&fci.params[0], EX(func)->common.function_name);
+ array_init(&fci.params[1]);
+ zend_copy_parameters_array(ZEND_NUM_ARGS(), &fci.params[1]);
+
+ fci.object = Z_OBJ(EX(This));
+ fcc.object = Z_OBJ(EX(This));
+ fcc.calling_scope = zend_get_executed_scope();
+
+ zend_call_function(&fci, &fcc);
+
+ zval_ptr_dtor(&fci.params[0]);
+ zval_ptr_dtor(&fci.params[1]);
+ efree(fci.params);
+}
@nikic
nikic added a note Jun 4, 2016

Please add a test for a Closure::fromCallable([$closure, '__invoke'])(1, 2, 3). I suspect it will not be handled correctly...

@nikic
nikic added a note Jun 4, 2016

Also generally, whatever issue this code is solving, does the same issue exist for ReflectionMethod::getClosure() and if not, how did they solve it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@krakjoe
krakjoe commented Jun 8, 2016

Can someone tag this with RFC and accepted, it is destined for 7.1.

@php-pulls php-pulls merged commit fc92eee into php:master Jul 5, 2016
1 check passed

1 check passed

Details continuous-integration/travis-ci/pr The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment