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.

diff --git a/NEWS b/NEWS
index c11d063557f2e6e841f92e1c11d7eda9dd252b8b..9c7323ac6f2c7cd0258cac5bc690421f6e26d322 100644 (file)
--- a/NEWS
+++ b/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)

diff --git a/UPGRADING b/UPGRADING
index 97f8197f9379904db578c6de44dd7a6b41071f5c..42e52a7cac06fd4f2c09c60cf796e8a171d7266c 100644 (file)
--- a/UPGRADING
+++ b/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

diff --git a/Zend/tests/variadic/illegal_variadic_override_ref.phpt b/Zend/tests/variadic/illegal_variadic_override_ref.phpt
new file mode 100644 (file)
index 0000000..fb64758
--- /dev/null
+++ b/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

diff --git a/Zend/tests/variadic/illegal_variadic_override_type.phpt b/Zend/tests/variadic/illegal_variadic_override_type.phpt
new file mode 100644 (file)
index 0000000..6417029
--- /dev/null
+++ b/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

diff --git a/Zend/tests/variadic/legal_variadic_override.phpt b/Zend/tests/variadic/legal_variadic_override.phpt
new file mode 100644 (file)
index 0000000..030c65e
--- /dev/null
+++ b/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==

diff --git a/Zend/tests/variadic/removing_parameter_error.phpt b/Zend/tests/variadic/removing_parameter_error.phpt
index cf483c7feceeffe972069b8739039b0d8c427cc1..0c66c1547d39ed0ef67da134ea0cecfd0d64869b 100644 (file)
--- a/Zend/tests/variadic/removing_parameter_error.phpt
+++ b/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===

diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c
index 6d14e6d573f2e93fd6812efbd80bf4821677ef39..3a9deabb06da3d5c02c7b69410408e9ccba535d3 100644 (file)
--- a/Zend/zend_inheritance.c
+++ b/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);