`array_push($arr, ...$map)` should support non-numeric keys

[mvorisek]

Description

The following code:

https://3v4l.org/0p5Sv

<?php

function array_push_cust(&$arr, ...$args) {
    foreach ($args as $k => $v) {
        if (is_int($k)) {
            $arr[] = $v;
        } else {
            $arr[$k] = $v;
        }
    }
}

$arr = [1, 'a' => 2];
$arr2 = [3, 'b' => 4];

$res = $arr;
array_push_cust($res, ...$arr2);
print_r($res);

$res = $arr;
array_push($res, ...$arr2);
print_r($res);

Resulted in this output:

Array
(
    [0] => 1
    [a] => 2
    [1] => 3
    [b] => 4
)

Fatal error: Uncaught ArgumentCountError: array_push() does not accept unknown named parameters in /in/0p5Sv:21
Stack trace:
#0 /in/0p5Sv(21): array_push(Array, 3, b: 4)
#1 {main}
  thrown in /in/0p5Sv on line 21

But I expected this output instead:

Array
(
    [0] => 1
    [a] => 2
    [1] => 3
    [b] => 4
)
Array
(
    [0] => 1
    [a] => 2
    [1] => 3
    [b] => 4
)

PHP Version

PHP 8.0 and higher

[damianwadley]

This isn't an unpacking problem. This is a named arguments problem. Namely (ha), that array_push does not accept named arguments.
https://3v4l.org/H1V7f

Because the dirty truth here is that argument unpacking and named arguments are like 90% the same thing.

I feel like this is a wontfix, but this is close enough to on-topic with your mailing list thread that I think you could just quickly ask there...

[Girgias]

Yeeeaaaaah, I think this is going to be a wontfix too, those are semantics that were voted on with the named arguments RFC so.

[mvorisek]

Yeeeaaaaah, I think this is going to be a wontfix too, those are semantics that were voted on with the named arguments RFC so.

I checked https://wiki.php.net/rfc/named_params RFC and I did not found any objections there. Please also not here the 2nd arg is "Variable-length argument list" and nor "named argument". Is shown in https://3v4l.org/0p5Sv, the current language design can allow the desired behavour easily.

This feature request would also allow associative array_unshift which is currently also not easily possible.

[damianwadley]

Please also not here the 2nd arg is "Variable-length argument list" and nor "named argument".

That's not how it works.

Using ... for an argument does not mean there is a single argument and it's variable-length. If it were then at least one of these syntaxes should be possible:

function foo(...$args) { }

// "args" takes multiple arguments
foo(args: 1, 2, 3); // no: cannot use positional argument after named argument

// "args" can be specified multiple times
foo(args: 1, args: 2, args: 3); // no: named parameter $args overwrites previous argument

What it means is that additional arguments passed to the function are gathered into that array instead of being ignored. It's an improved syntax for functions so that developers don't need to do magic using func_get_args like before.

function modern($first, ...$rest) { }
function traditional($first) { $rest = func_get_args(); array_shift($rest); }

// both of these result in $first is 1 and $rest is [2, 3]
modern(1, 2, 3);
traditional(1, 2, 3);

With named arguments, what I said before

Because the dirty truth here is that argument unpacking and named arguments are like 90% the same thing.

(I mean in this context - obviously they're not actually the same thing) comes into play: you are unpacking an array with keys, and the functionality of doing so is that those keys are mapped to arguments. If those keys correspond to parameters that are undefined then that's an error - unless you also include a varargs parameter, in which case PHP will shove them into there.

I didn't say there was nothing to do here - I said that the issue was one of named arguments and not one of unpacking. You'd have the exact same problem yourself if you tried to implement a userland array_push using the traditional func_get_args approach.

But I could see named arguments making sense for array_push (and array_unshift): just like how array_push($array, 1) is an alternative to $array[] = 1, a named version of array_push($array, a: 1) would be the logical alternative to $array["a"] = 1.

[nielsdos]

I prototyped the suggestion of Damian's last paragraph. Patch below.

With the patch the following will work:

<?php
$arr2 = [3, 'b' => 4];
$res = [1, 'a' => 2];
array_push($res, ...$arr2);
print_r($res);

and this too ofc, because it is expanded to this:

<?php
$res = [1, 'a' => 2];
array_push($res, 3, b: 4);
print_r($res);

But there's a caveat: the engine has a restriction that positional arguments may not appear after named arguments, this can give problems for an input like:

<?php
$arr2 = [3, 'b' => 4, 123456];
$res = [1, 'a' => 2];
array_push($res, ...$arr2);
print_r($res);

This will fail because the code gets expanded by the engine to:

array_push($res, 3, b: 4, 123456);

and this violates that rule about positional arguments after named arguments.

Patch (applies on master branch)

Note: this patch is barely tested and I didn't put too much thought into it. It just shows that something like this can kinda be done (albeit with caveats).

diff --git a/ext/standard/array.c b/ext/standard/array.c
index dc0d434d18..92f0d03794 100644
--- a/ext/standard/array.c
+++ b/ext/standard/array.c
@@ -3153,11 +3153,11 @@ PHP_FUNCTION(array_push)
 		    new_var;	/* Variable to be pushed */
 	int i,				/* Loop counter */
 		argc;			/* Number of function arguments */
-
+	zend_array *named_arguments = NULL;
 
 	ZEND_PARSE_PARAMETERS_START(1, -1)
 		Z_PARAM_ARRAY_EX(stack, 0, 1)
-		Z_PARAM_VARIADIC('+', args, argc)
+		Z_PARAM_VARIADIC_WITH_NAMED(args, argc, named_arguments)
 	ZEND_PARSE_PARAMETERS_END();
 
 	/* For each subsequent argument, make it a reference, increase refcount, and add it to the end of the array */
@@ -3165,14 +3165,29 @@ PHP_FUNCTION(array_push)
 		ZVAL_COPY(&new_var, &args[i]);
 
 		if (zend_hash_next_index_insert(Z_ARRVAL_P(stack), &new_var) == NULL) {
-			Z_TRY_DELREF(new_var);
-			zend_throw_error(NULL, "Cannot add element to the array as the next element is already occupied");
-			RETURN_THROWS();
+			goto err;
 		}
 	}
 
+	/* Analogous for named arguments */
+	if (named_arguments) {
+		zend_string *key;
+		ZEND_HASH_MAP_FOREACH_STR_KEY_VAL(named_arguments, key, args) {
+			ZVAL_COPY(&new_var, args);
+
+			if (zend_hash_add(Z_ARRVAL_P(stack), key, &new_var) == NULL) {
+				goto err;
+			}
+		} ZEND_HASH_FOREACH_END();
+	}
+
 	/* Clean up and return the number of values in the stack */
-	RETVAL_LONG(zend_hash_num_elements(Z_ARRVAL_P(stack)));
+	RETURN_LONG(zend_hash_num_elements(Z_ARRVAL_P(stack)));
+
+err:
+	Z_TRY_DELREF(new_var);
+	zend_throw_error(NULL, "Cannot add element to the array as the next element is already occupied");
+	RETURN_THROWS();
 }
 /* }}} */
 

[mvorisek]

repros https://3v4l.org/cvRon (Cannot use positional argument after named argument during unpacking) and https://3v4l.org/GGXgX (Named parameter $arr overwrites previous argument)

1. with these limitations, does the patch makes sense to you?
2. when ...expr argument is passed and the target function signature has ...$args argument, does it makes sense to try to match the parameters first without ... on both sides and then allow any ...?
3. a proxy method without a signature would then need to handle passed ... argument somehow, I did an experiment - https://3v4l.org/HC3D4 - and func_get_args() actually ignores named parameters [1] and ...$args in the method signature is needed, thus point 2. should be feasible to be implemented consistently as there is some special passing for ... already implemented - https://3v4l.org/MhIdm - as 0 key stays 0

[1] https://www.php.net/manual/en/function.func-get-arg.php#refsect1-function.func-get-arg-notes

[nielsdos]

1. To me? Kinda, I need more time to think about it. Its behaviour is at least consistent to the custom function example you gave AFAICT. To developers who want to use this new functionality? I guess they are going to be confused by the limitation.
2. Conceptually that makes sense to me. I'm not sure what that has to do with the patch I sent though? The patch is specific to array_push.
3. Not sure if there's a question here?

[mvorisek]

2. related to the patch for array_push as without removing the exceptions for ...$args some ...expr inputs would not be valid
3. discussion if removal of the exceptions is consistent, and the last repro seems to answer it - the exceptions are actually not consistent nor needed with ...$args