Allow variadic arguments to replace non-variadic ones

Any number of arguments can be replaced by a variadic one, so
long as the variadic argument is compatible (in the sense of
contravariance) with the subsumed arguments.

In particular this means that function(...$args) becomes a
near-universal signature: It is compatible with any function
signature that does not accept parameters by-reference.

This also fixes bug #70839, which describes a special case.

Closes GH-5059.

Author: nikic

NEWS

@@ -10,6 +10,8 @@ PHP                                                                        NEWS
     (Nikita)
   . Fixed bug #49555 (Fatal error "Function must be a string" message should be
     renamed). (Nikita)
+  . Fixed bug #70839 (Convertion optional argument to variadic forbidden by LSP
+    checks). (Nikita)
 
 - CURL:
   . Bumped required libcurl version to 7.29.0. (cmb)

UPGRADING

@@ -351,6 +351,17 @@ PHP 8.0 UPGRADE NOTES
   . Added WeakMap.
     RFC: https://wiki.php.net/rfc/weak_maps
   . Added ValueError class.
+  . Any number of function parameters may not be replaced by a variadic
+    argument, as long as the types are compatible. For example, the following
+    code is now allowed:
+
+        class A {
+            public function method(int $many, string $parameters, $here) {}
+        }
+        class B extends A {
+            public function method(...$everything) {}
+        }
+
 
 - Date:
   . Added DateTime::createFromInterface() and

Zend/tests/variadic/illegal_variadic_override_ref.phpt

@@ -0,0 +1,16 @@
+--TEST--
+Illegal variadic inheritance due to reference mismatch
+--FILE--
+<?php
+
+class A {
+    public function test(&$a, &$b) {}
+}
+
+class B extends A {
+    public function test(...$args) {}
+}
+
+?>
+--EXPECTF--
+Fatal error: Declaration of B::test(...$args) must be compatible with A::test(&$a, &$b) in %s on line %d

Zend/tests/variadic/illegal_variadic_override_type.phpt

@@ -0,0 +1,16 @@
+--TEST--
+Illegal variadic inheritance due to type mismatch
+--FILE--
+<?php
+
+class A {
+    public function test(int $a, int $b) {}
+}
+
+class B extends A {
+    public function test(string ...$args) {}
+}
+
+?>
+--EXPECTF--
+Fatal error: Declaration of B::test(string ...$args) must be compatible with A::test(int $a, int $b) in %s on line %d

Zend/tests/variadic/legal_variadic_override.phpt

@@ -0,0 +1,25 @@
+--TEST--
+Cases where non-variadic parameters are allowed to be subsumed by a variadic one
+--FILE--
+<?php
+
+class A {
+    public function test1($a, $b) {}
+    public function test2(int $a, int $b) {}
+    public function test3(int $a, int $b) {}
+    public function test4(int $a, string $b) {}
+    public function test5(&$a, &$b) {}
+}
+
+class B extends A {
+    public function test1(...$args) {}
+    public function test2(...$args) {}
+    public function test3(int ...$args) {}
+    public function test4(int|string ...$args) {}
+    public function test5(&...$args) {}
+}
+
+?>
+===DONE==
+--EXPECT--
+===DONE==

Zend/tests/variadic/removing_parameter_error.phpt

@@ -1,12 +1,8 @@
 --TEST--
-It's not possible to remove required parameter before a variadic parameter
+It is possible to remove required parameter before a variadic parameter
 --FILE--
 <?php
 
-/* Theoretically this should be valid because it weakens the constraint, but
- * PHP does not allow this (for non-variadics), so I'm not allowing it here, too,
- * to stay consistent. */
-
 interface DB {
     public function query($query, ...$params);
 }
@@ -16,5 +12,6 @@ class MySQL implements DB {
 }
 
 ?>
---EXPECTF--
-Fatal error: Declaration of MySQL::query(...$params) must be compatible with DB::query($query, ...$params) in %s on line %d
+===DONE===
+--EXPECT--
+===DONE===

Zend/zend_inheritance.c

@@ -495,8 +495,9 @@ static inheritance_status zend_do_perform_arg_type_hint_check(
 static inheritance_status zend_do_perform_implementation_check(
 		const zend_function *fe, const zend_function *proto) /* {{{ */
 {
-	uint32_t i, num_args;
+	uint32_t i, num_args, proto_num_args, fe_num_args;
 	inheritance_status status, local_status;
+	zend_bool proto_is_variadic, fe_is_variadic;
 
 	/* If it's a user function then arg_info == NULL means we don't have any parameters but
 	 * we still need to do the arg number checks.  We are only willing to ignore this for internal
@@ -516,9 +517,8 @@ static inheritance_status zend_do_perform_implementation_check(
 	/* If the prototype method is private do not enforce a signature */
 	ZEND_ASSERT(!(proto->common.fn_flags & ZEND_ACC_PRIVATE));
 
-	/* check number of arguments */
-	if (proto->common.required_num_args < fe->common.required_num_args
-		|| proto->common.num_args > fe->common.num_args) {
+	/* The number of required arguments cannot increase. */
+	if (proto->common.required_num_args < fe->common.required_num_args) {
 		return INHERITANCE_ERROR;
 	}
 
@@ -528,35 +528,36 @@ static inheritance_status zend_do_perform_implementation_check(
 		return INHERITANCE_ERROR;
 	}
 
-	if ((proto->common.fn_flags & ZEND_ACC_VARIADIC)
-		&& !(fe->common.fn_flags & ZEND_ACC_VARIADIC)) {
+	proto_is_variadic = (proto->common.fn_flags & ZEND_ACC_VARIADIC) != 0;
+	fe_is_variadic = (fe->common.fn_flags & ZEND_ACC_VARIADIC) != 0;
+
+	/* A variadic function cannot become non-variadic */
+	if (proto_is_variadic && !fe_is_variadic) {
 		return INHERITANCE_ERROR;
 	}
 
-	/* For variadic functions any additional (optional) arguments that were added must be
-	 * checked against the signature of the variadic argument, so in this case we have to
-	 * go through all the parameters of the function and not just those present in the
-	 * prototype. */
-	num_args = proto->common.num_args;
-	if (proto->common.fn_flags & ZEND_ACC_VARIADIC) {
-		num_args++;
-        if (fe->common.num_args >= proto->common.num_args) {
-			num_args = fe->common.num_args;
-			if (fe->common.fn_flags & ZEND_ACC_VARIADIC) {
-				num_args++;
-			}
-		}
-	}
+	/* The variadic argument is not included in the stored argument count. */
+	proto_num_args = proto->common.num_args + proto_is_variadic;
+	fe_num_args = fe->common.num_args + fe_is_variadic;
+	num_args = MAX(proto_num_args, fe_num_args);
 
 	status = INHERITANCE_SUCCESS;
 	for (i = 0; i < num_args; i++) {
-		zend_arg_info *fe_arg_info = &fe->common.arg_info[i];
-
-		zend_arg_info *proto_arg_info;
-		if (i < proto->common.num_args) {
-			proto_arg_info = &proto->common.arg_info[i];
-		} else {
-			proto_arg_info = &proto->common.arg_info[proto->common.num_args];
+		zend_arg_info *proto_arg_info =
+			i < proto_num_args ? &proto->common.arg_info[i] :
+			proto_is_variadic ? &proto->common.arg_info[proto_num_args - 1] : NULL;
+		zend_arg_info *fe_arg_info =
+			i < fe_num_args ? &fe->common.arg_info[i] :
+			fe_is_variadic ? &fe->common.arg_info[fe_num_args - 1] : NULL;
+		if (!proto_arg_info) {
+			/* A new (optional) argument has been added, which is fine. */
+			continue;
+		}
+		if (!fe_arg_info) {
+			/* An argument has been removed. This is considered illegal, because arity checks
+			 * work based on a model where passing more than the declared number of parameters
+			 * to a function is an error. */
+			return INHERITANCE_ERROR;
 		}
 
 		local_status = zend_do_perform_arg_type_hint_check(fe, fe_arg_info, proto, proto_arg_info);