Fix array_map optimization with non-literal function or non-literal args

[arnaud-lb]

Non-literal expressions must be evaluated once and memoized to maintain semantics:

$callback = function ($value) {
    global $callback;
    $callback = function () { return 'changed'; };
    return $value + 1;
};

array_map($callback, [1,2]);

// Expected result: [2,3]
// Actual result: [2,'changed']

Unfortunately, we can't memoize pre-bound PFA arguments without breaking pass-by-reference, so we disable the optimization if the callback is a PFA with non-literal arguments. We could improve this when the function is known and we can determine that the argument is not passed by ref.

Bug found by Ryan @ Calif.io.

[TimWolla]

This is unfortunate, since it kills many PFA use cases of this optimization. Would it be possible to translate this to:

$pfa = plusn(?, $obj->value);
$result = [];
foreach (range(1, 2) as $key => $val) {
    $result[$key] = $pfa($val);
}

instead of

$result = [];
foreach (range(1, 2) as $key => $val) {
    $result[$key] = plusn($val, $obj->value);
}

? Potentially an OPcache pass could later inline $pfa if it knows this is safe to keep compiler complexity low?

[arnaud-lb]

I've tried moving the entire array_map optimization to the optimizer with the help of an LLM, in a separate branch: master...arnaud-lb:php-src:array-map-inline-optimizer.

Moving everything to the optimizer allows to inline array_map only when we know that the PFA is going to be desugared.

One difficulty was that it's the first time we increase the number of opcode in the optimizer, so we needed a non-trivial zend_optimizer_insert_oplines() helper.

This is a lot of code, so I wouldn't merge such a change in RC. Doing only the PFA sinking/desugaring part in the optimizer would be similarly complex.

Maybe we can merge this PR as-is, and pursue the optimizer part in master only?

[TimWolla]

Maybe we can merge this PR as-is, and pursue the optimizer part in master only?

Yes, I didn't mean to say that the Optimizer part should ship in PHP 8.6. My suggestion was to “define another temporary for otherwise unoptimizable PFA” rather than disabling the optimization entirely (and then a follow-up in master could inline the temporary where possible). Not sure if that would end up simpler or more complicated than this fix.

Overall the complexity of the optimization in the compiler has already grown (much) further than I initially anticipated and this is another 80 lines to fix a bug, that's why I wanted to make sure that the solution we've selected here is the best one given the constraints.

If it works and you are confident that it is correct and stable, then I'm happy.

[arnaud-lb]

I have mixed feelings about inlining with an unoptimized PFA. In theory that's still beneficial as we avoid the expensive VM reentry, but on the other hand this increases opcode size for less benefits. I wasn't able to measure a benefit or regression in symfony and phpstan benchmarks (with all array_map inlined, including non-PFA ones since these projects do not use PFAs yet).

It adds a few more lines to this PR, too, so I would prefer merging this as-is for now.

Regarding the optimizer, sinking a PFA is likely at least as complex as generating the whole loop, but I haven't explored this enough to be sure, so I will come back to this later. We can still decide to inline array maps with unoptimizable PFAs when we implement the optimizer side.

[TimWolla]

It adds a few more lines to this PR, too, so I would prefer merging this as-is for now.

I'm okay with that.