From e72bf636917b820dc22dac89c63657f83ed56348 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Mon, 6 Jan 2020 12:06:51 +0100 Subject: [PATCH] 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. --- NEWS | 2 + UPGRADING | 11 ++++ .../illegal_variadic_override_ref.phpt | 16 ++++++ .../illegal_variadic_override_type.phpt | 16 ++++++ .../variadic/legal_variadic_override.phpt | 25 +++++++++ .../variadic/removing_parameter_error.phpt | 11 ++-- Zend/zend_inheritance.c | 55 ++++++++++--------- 7 files changed, 102 insertions(+), 34 deletions(-) create mode 100644 Zend/tests/variadic/illegal_variadic_override_ref.phpt create mode 100644 Zend/tests/variadic/illegal_variadic_override_type.phpt create mode 100644 Zend/tests/variadic/legal_variadic_override.phpt diff --git a/NEWS b/NEWS index c11d063557f..9c7323ac6f2 100644 --- 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 97f8197f937..42e52a7cac0 100644 --- 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 index 00000000000..fb6475835a6 --- /dev/null +++ b/Zend/tests/variadic/illegal_variadic_override_ref.phpt @@ -0,0 +1,16 @@ +--TEST-- +Illegal variadic inheritance due to reference mismatch +--FILE-- + +--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 index 00000000000..6417029a291 --- /dev/null +++ b/Zend/tests/variadic/illegal_variadic_override_type.phpt @@ -0,0 +1,16 @@ +--TEST-- +Illegal variadic inheritance due to type mismatch +--FILE-- + +--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 index 00000000000..030c65e899b --- /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-- + +===DONE== +--EXPECT-- +===DONE== diff --git a/Zend/tests/variadic/removing_parameter_error.phpt b/Zend/tests/variadic/removing_parameter_error.phpt index cf483c7fece..0c66c1547d3 100644 --- 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-- ---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 6d14e6d573f..3a9deabb06d 100644 --- 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);