Compact can't resolve outer scoped variables using short closures

php.internals

David Rodrigues

3 years ago
Hello! I'm converting my code to use short closures where possible, and I ran into a problem using compact(). Basically, the names used in compact() cannot be accessed due to a bug, reported in 2019 still in PHP 7.4 (ID 78970). https://bugs.php.net/bug.php?id=78970 It seems to me to be a reasonable problem and one that needs attention, as the message is not that "compact cannot be used here", but that "the variable does not exist". The code below may reproduce the problem: $x = 123; (fn() => compact('x'))(); https://3v4l.org/AFARs Is there any possibility of this being fixed? I would love to help, but I don't have much C programming skills, unfortunately. Atenciosamente, David Rodrigues

Marco Pivetta

3 years ago
On Wed, 19 Oct 2022, 19:04 David Rodrigues, <david.proweb@gmail.com> wrote:
> Hello! > > I'm converting my code to use short closures where possible, and I ran into > a problem using compact(). > > Basically, the names used in compact() cannot be accessed due to a bug, > reported in 2019 still in PHP 7.4 (ID 78970). > > https://bugs.php.net/bug.php?id=78970 > > It seems to me to be a reasonable problem and one that needs attention, as > the message is not that "compact cannot be used here", but that "the > variable does not exist". > > The code below may reproduce the problem: > > $x = 123; > (fn() => compact('x'))(); > > https://3v4l.org/AFARs > > Is there any possibility of this being fixed? I would love to help, but I > don't have much C programming skills, unfortunately. >
I'd rather hope for `compact()` to finally be deprecated and targeted for removal 😛 The fact that it still exists precludes (or at least complicates) future optimization of scope + inlining in the engine. Similar thoughts towards `extract()`, I'd say.

David Rodrigues

3 years ago
> I'd rather hope for `compact()` to finally be deprecated and targeted for
removal 😛 I think compact() is a good function for transferring variables from one point to another, but I would think about making improvements as it is confusing (uses the variable name, rather than the variable itself). Regarding the bug, if we use an array it should work perfectly: $x = 123; (fn() => [ 'x' => $x ])(); https://3v4l.org/ov7TM Would it be possible to automatically convert compact() to an array at runtime? So I imagine that any necessary optimization can take place directly over the resulting array, rather than the compact itself. Atenciosamente, David Rodrigues Em qua., 19 de out. de 2022 às 14:09, Marco Pivetta <ocramius@gmail.com> escreveu:

Michał Brzuchalski

3 years ago
śr., 19 paź 2022 o 19:18 David Rodrigues <david.proweb@gmail.com> napisał(a):
> > I'd rather hope for `compact()` to finally be deprecated and targeted for > removal 😛 > > I think compact() is a good function for transferring variables from one > point to another, but I would think about making improvements as it is > confusing (uses the variable name, rather than the variable itself). > > Regarding the bug, if we use an array it should work perfectly: > > $x = 123; > (fn() => [ 'x' => $x ])(); > > https://3v4l.org/ov7TM > > Would it be possible to automatically convert compact() to an array at > runtime? So I imagine that any necessary optimization can take place > directly over the resulting array, rather than the compact itself. >
How would you like it to work, if you pass a variable name variable to compact then? $x = 123; $name = 'x'; (fn () => compact($name))(); I agree with the idea of deprecating compact() & extract() and in long-term variable of variables as well. Cheers, Michał Marcin Brzuchalski

Claude Pache

3 years ago
> > How would you like it to work, if you pass a variable name variable to > compact then? > $x = 123; > $name = 'x'; > (fn () => compact($name))(); >
Although it is difficult to make it work in general (of course), there is the specific case of names given as literal strings, as in the example provided by the OP, that does not suffer from the impossibility of static analysis.
> I agree with the idea of deprecating compact() & extract() and in > long-term variable of variables as well.
However, in `compact('x', 'y')`, the name of the variables are not variable, they are constant. —Claude

Rowan Collins

3 years ago
On 19/10/2022 18:04, David Rodrigues wrote:
> It seems to me to be a reasonable problem and one that needs attention, as > the message is not that "compact cannot be used here", but that "the > variable does not exist".
I'd just like to point out that the error message here is 100% correct: there is nothing wrong with using compact() inside a closure, it just only has access *at run-time* to the variables which were captured when the closure was created, which are determined *at compile-time*. $foo = fn() => [$a, compact('a', 'b')]; is essentially compiled to: $foo = function (use $a) { return [$a, compact('a', 'b')] }; When you later execute $foo, compact() can see the local $a, but there is no $b anywhere in the compiled definition. On 20/10/2022 08:15, Claude Pache wrote:
> Although it is difficult to make it work in general (of course), there is the specific case of names given as literal strings, as in the example provided by the OP, that does not suffer from the impossibility of static analysis.
While it would be possible for the compiler to special-case this scenario, I think it could be rather confusing to have a function that is sometimes evaluated at compile-time and sometimes at run-time (other than as a transparent performance optimisation). For instance, each of the following *can* be fully evaluated to a constant at compile-time, but which ones would be? compact('a' . 'b'); $name = 'a'; compact($name); $x = 1; compact('a' . $x); $x = 1; $x++; compact('a' . $x); I think it would be better to look at a new syntax for specifying such arrays that doesn't rely on a run-time function, similar to how in JS  { foo, bar } is short-hand for { foo: $foo, bar: $bar } This has come up before, for instance in Nikita's discussion of named parameter syntaxes: https://wiki.php.net/rfc/named_params#shorthand_syntax_for_matching_parameter_and_variable_name As well as removing the need to pass a variable name as a string, this would allow combining explicit and automatic names in one literal, e.g.: // Current array literal $x = [ 'foo' => $foo, 'bar' => someFunc() ]; // compact() $bar = someFunc(); $x = compact('foo', 'bar'); // Shortest variation in Nikita's RFC linked above $x = [ :$foo, bar: someFunc() ]; I'm personally not a fan of this coding style, because I think variables should be named to be meaningful in the current scope, not somewhere they're coming from or going to, but a dedicated syntax would at least allow that flexibility. Regards,
-- Rowan Tommins [IMSoP]

David Rodrigues

3 years ago
> $foo = fn() => [$a, compact('a', 'b')]; > is essentially compiled to: > $foo = function (use $a) { return [$a, compact('a', 'b')] };
Regarding this, in case you're literally saying that internally "fn()" turns into a common "function", and getting back to the main topic, would it be possible to do the same with compact()? fn() => compact('dummy') turns into: fn() => [ 'dummy' => $dummy ] which turns into: function() use($dummy) { return [ 'dummy' => $dummy ]; } Atenciosamente, David Rodrigues

Rowan Collins

3 years ago
On 20/10/2022 17:48, David Rodrigues wrote:
> Regarding this, in case you're literally saying that internally "fn()" > turns into a common "function", and getting back to the main topic, > would it be possible to do the same with compact()? > > fn() => compact('dummy') > turns into: > fn() => [ 'dummy' => $dummy ] > which turns into: > function() use($dummy) { return [ 'dummy' => $dummy ]; }
Yes, that is what I meant by "it would be possible for the compiler to special-case this scenario"; I explained why I think that would be a bad idea, and suggested an alternative. Regards,
-- Rowan Tommins [IMSoP]

Christoph Becker

3 years ago
On 20.10.2022 at 19:03, Rowan Tommins wrote:
> On 20/10/2022 17:48, David Rodrigues wrote: > >> Regarding this, in case you're literally saying that internally "fn()" >> turns into a common "function", and getting back to the main topic, >> would it be possible to do the same with compact()? >> >> fn() => compact('dummy') >> turns into: >> fn() => [ 'dummy' => $dummy ] >> which turns into: >> function() use($dummy) { return [ 'dummy' => $dummy ]; } > > Yes, that is what I meant by "it would be possible for the compiler to > special-case this scenario";
Well, in the general case, the compiler cannot special-case this, because compact() is a normal function, so might be used as "variable function", or may not refer to the top-level compact() at all (the called symbol may not be fully qualified).
> I explained why I think that would be a bad > idea, and suggested an alternative.
ACK
-- Christoph M. Becker