Re: [PATCH] [RFC] Closures and lambda functions in PHP

Hi Dmitry, First of all: Your patch does really simplify things internally quite a bit - I like it. I have a few issues though:

The patch shouldn't affect opcode caches and other extensions as it doesn't change any structures.

I don't see a problem in changing structures for either extensions nor opcode caches - as long as only entries are added. Binary compability with PHP 5.2 is not provided anyway (by neither 5.3 nor 6) and source compability is not affected if the old members are not touched or their semantics change.

It uses the op_array->static_variables for lexical variables.

That's a point I don't like. Although you use IS_CONSTANT to cleverly mask lexical variables, I really think a separate hash table would be a far better idea, especially for code maintainability.

The patch also fixes several small issues and adds some missing functionality which didn't allow preg_replace_callback() (and may be others) to work with lambda functions.

Oh yes, I somehow missed that, thanks!

Please review.

I (personally) have some smaller issues with the patch and one big issue: Smaller issues: * A separate hash table for the lexical variables would be much cleaner in my eyes. * The segfault that occurs with my patch still occurs with yours (see below for an example) But the one big issue is the syntax: ($foo | $bar) is just extremely painful in my eyes. I wouldn't want to use it - and it would be quite confusing (which side are the normal parameters, which side are the lexical vars?). I do see your point that the 'lexical' keyword inside the function body to actually have an effect on the function semantics is not optimal and that the list of lexical variables is probably better placed in the function definition. I therefore propose the following syntax:

function (parameters) { }                   // no closure, simply lambda

function (parameters) KEYWORD (lexical) { } // closure with lexical vars KEYWORD could be for example 'use'. That probably describes best what the function does: Use/import those variables from the current scope. Example: return function ($x) use ($s) { static $n = 0; $n++; $s = $n.':'.$s; $this->foo($x[0].':'.$s); }; As for simply omitting the keyword, e.g. function () () - as already suggested: I don't like that syntax either. Although I'm not a fan of too much language verbosity (that's why I don't like Fortran, Basic and Pascal), I think in this case, a little more verbosity wouldn't hurt - and typing 'use' is just 3 additional characters. Now for the examples for the smaller issues: Segfault: <?php $a = function () { $GLOBALS['a'] = NULL; echo "destroyed closure\n"; }; var_dump ($a); $a (); ?> This crashes - due to the fact that the currently used op_array is destroyed upon destruction of the variable. This could get even more interesting if the closure called itself recursively. My proposal is to create a copy (but not a reference, just do a normal copy, for resources or objects that will just do the trick) of the variable internally in zend_call_function and zend_do_fcall_common_helper into a dummy zval and destroy that zval after the function call ended. That way, the GC won't kick in until after the execution of the closure. In zend_call_function that's easy - in zend_do_fcall_common helper we have the problem that the variable containing the closure is no longer available. An idea could be that the INIT_FCALL functions always additionally push the lambda zval to the argument stack (inside the function it will be ignored) and the fcall_common_helper will remove that zval from the stack prior to returning (and free it). If a non-closure is called, NULL (or an empty zval or whatever) could be pushed to the stack instead. Hmm, perhap's I'll have a better idea tomorrow. Anyway, since Andi suggested to use objects instead of resources, I'd like to use your patch as a starting point, if there are no objections. Regards, Christian