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

Hi Christian, I'm fine with your suggestion for lexical variables syntax, but I don't know if we really need brackets around them. For now I changed syntax in the following way. $func = function ($x, $y) use $a, $b, $c { } According to segfault, I added a check that emits fatal error. I don't like to use separate HashTable for lexical variables, because - it takes memory (however it won't be used for all regular op_arrays) - it requires new special modifier for FETCH opcode - opcode caches must check for this additional table and copy it My idea with usage of static_variables doesn't require any opcode cache modification at all. I'm fine if you'll improve my patch (It's mainly yours :) Thanks. Dmitry. Christian Seiler wrote: > 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