antecedent / patchwork

Method redefinition (monkey-patching) functionality for PHP.
https://antecedent.github.io/patchwork/
MIT License
444 stars 40 forks source link

Incorrect arguments when redefining variadic functions #114

Closed anomiex closed 3 years ago

anomiex commented 3 years ago

Since 2.1.13, when redefining a variadic function the arguments passed to the callable do not match those passed in the original call.

Test case:

<?php

use function Patchwork\redefine;

function foo( $a, ...$args ) {
    echo "\$a is " . var_export( $a, true ) . "\n";
    echo "\$args are " . var_export( $args, true ) . "\n";
}

foo( 1, 2, 3 );

redefine( 'foo', function ( $a, ...$args ) {
    echo "redefined!\n";
    echo "\$a is " . var_export( $a, true ) . "\n";
    echo "\$args are " . var_export( $args, true ) . "\n";
} );

foo( 4, 5, 6 );

Expected output:

$a is 1
$args are array (
  0 => 2,
  1 => 3,
)
redefined!
$a is 4
$args are array (
  0 => 5,
  1 => 6,
)

Actual output:

$a is 1
$args are array (
  0 => 2,
  1 => 3,
)
redefined!
$a is 4
$args are array (
  0 => 
  array (
    0 => 5,
    1 => 6,
  ),
  1 => 6,
)

Notes

It seems to me that Patchwork\CodeManipulation\Actions\Arguments\constructReferenceArray() needs to know that an arg is variadic, so it can be merged into the result instead of being included as-is. Maybe something like

diff -ur a/src/CodeManipulation/Actions/Arguments.php b/src/CodeManipulation/Actions/Arguments.php
--- a/src/CodeManipulation/Actions/Arguments.php    2021-08-19 11:21:54.242331034 -0400
+++ b/src/CodeManipulation/Actions/Arguments.php    2021-08-19 11:30:55.785185144 -0400
@@ -23,7 +23,12 @@
             $pos = $s->match($pos);
         } else {
             if ($s->is(T_VARIABLE, $pos)) {
-                $result[] = $s->read($pos);
+                $var = $s->read($pos);
+                $previous = $s->skipBack(Source::junk(), $pos);
+                if ($s->is(T_ELLIPSIS, $previous)) {
+                    $var = '...' . $var;
+                }
+                $result[] = $var;
             }
             $pos++;
         }
@@ -37,6 +42,9 @@
 function constructReferenceArray(array $names)
 {
     $names = array_map(function($name) {
+        if ($name[0] === '.') {
+            return $name;
+        }
         return '&' . $name;
     }, $names);
     return '[' . join(', ', $names) . ']';
antecedent commented 3 years ago

Thank you! I have included your test case and made it pass in version 2.1.14. Please reopen if the problem persists.