From: Nikita Popov Date: Thu, 9 Jan 2020 14:04:33 +0000 (+0100) Subject: Check abstract method signatures coming from traits X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=f74e30c07c2a94921fbfb7b8936324707505bd75;p=php Check abstract method signatures coming from traits RFC: https://wiki.php.net/rfc/abstract_trait_method_validation Closes GH-5068. --- diff --git a/UPGRADING b/UPGRADING index 183ded3ab7..4bd913396a 100644 --- a/UPGRADING +++ b/UPGRADING @@ -163,6 +163,21 @@ PHP 8.0 UPGRADE NOTES accepted, and func as assumed to refer to T1::func. Now it will generate a fatal error instead, and either T1::func or T2::func needs to be written explicitly. + . The signature of abstract methods defined in traits is now checked against + the implementing class method: + + trait MyTrait { + abstract private function neededByTrait(): string; + } + + class MyClass { + use MyTrait; + + // Error, because of return type mismatch. + private function neededByTrait(): int { return 42; } + } + + RFC: https://wiki.php.net/rfc/abstract_trait_method_validation - COM: . Removed the ability to import case-insensitive constants from type @@ -428,8 +443,10 @@ PHP 8.0 UPGRADE NOTES . Some consistency fixes to variable syntax have been applied, for example writing `Foo::BAR::$baz` is now allowed. RFC: https://wiki.php.net/rfc/variable_syntax_tweaks - . Added Stringable. + . Added Stringable interface, which is automatically implemented if a class + defines a __toString() method. RFC: https://wiki.php.net/rfc/stringable + . Traits can now define abstract private methods. - Date: . Added DateTime::createFromInterface() and diff --git a/Zend/tests/traits/abstract_method_1.phpt b/Zend/tests/traits/abstract_method_1.phpt new file mode 100644 index 0000000000..7ec5316b8a --- /dev/null +++ b/Zend/tests/traits/abstract_method_1.phpt @@ -0,0 +1,18 @@ +--TEST-- +Abstract method from trait enforced in class +--FILE-- + +--EXPECTF-- +Fatal error: Declaration of C::neededByTheTrait(array $a, object $b) must be compatible with T::neededByTheTrait(int $a, string $b) in %s on line %d diff --git a/Zend/tests/traits/abstract_method_2.phpt b/Zend/tests/traits/abstract_method_2.phpt new file mode 100644 index 0000000000..3633f9f4e1 --- /dev/null +++ b/Zend/tests/traits/abstract_method_2.phpt @@ -0,0 +1,22 @@ +--TEST-- +Mutually incompatible methods from traits are fine as long as the final method is compatible +--FILE-- + +===DONE=== +--EXPECT-- +===DONE=== diff --git a/Zend/tests/traits/abstract_method_3.phpt b/Zend/tests/traits/abstract_method_3.phpt new file mode 100644 index 0000000000..935cc5cd0d --- /dev/null +++ b/Zend/tests/traits/abstract_method_3.phpt @@ -0,0 +1,18 @@ +--TEST-- +Private abstract method from trait enforced in class +--FILE-- + +--EXPECTF-- +Fatal error: Declaration of C::neededByTheTrait(array $a, object $b) must be compatible with T::neededByTheTrait(int $a, string $b) in %s on line %d diff --git a/Zend/tests/traits/abstract_method_4.phpt b/Zend/tests/traits/abstract_method_4.phpt new file mode 100644 index 0000000000..5b7db22874 --- /dev/null +++ b/Zend/tests/traits/abstract_method_4.phpt @@ -0,0 +1,20 @@ +--TEST-- +Visibility enforcement on abstract trait methods +--FILE-- + +===DONE=== +--EXPECT-- +===DONE=== diff --git a/Zend/tests/traits/abstract_method_5.phpt b/Zend/tests/traits/abstract_method_5.phpt new file mode 100644 index 0000000000..fa6482aa42 --- /dev/null +++ b/Zend/tests/traits/abstract_method_5.phpt @@ -0,0 +1,18 @@ +--TEST-- +Staticness enforcement on abstract trait methods +--FILE-- + +--EXPECTF-- +Fatal error: Cannot make static method T::method() non static in class C in %s on line %d diff --git a/Zend/tests/traits/abstract_method_6.phpt b/Zend/tests/traits/abstract_method_6.phpt new file mode 100644 index 0000000000..a8a3fc4138 --- /dev/null +++ b/Zend/tests/traits/abstract_method_6.phpt @@ -0,0 +1,20 @@ +--TEST-- +Abstract private trait method not implemented +--FILE-- + +--EXPECTF-- +Fatal error: Class C must implement 1 abstract private method (C::method) in %s on line %d diff --git a/Zend/tests/traits/abstract_method_7.phpt b/Zend/tests/traits/abstract_method_7.phpt new file mode 100644 index 0000000000..5e276eb17d --- /dev/null +++ b/Zend/tests/traits/abstract_method_7.phpt @@ -0,0 +1,23 @@ +--TEST-- +Abstract private trait method forwarded as abstract protected method +--FILE-- + +===DONE=== +--EXPECT-- +===DONE=== diff --git a/Zend/tests/traits/bug60217b.phpt b/Zend/tests/traits/bug60217b.phpt index b349cf2c54..05a3adc700 100644 --- a/Zend/tests/traits/bug60217b.phpt +++ b/Zend/tests/traits/bug60217b.phpt @@ -22,4 +22,4 @@ class CBroken { $o = new CBroken; $o->foo(1); --EXPECTF-- -Fatal error: Declaration of TBroken1::foo($a) must be compatible with TBroken2::foo($a, $b = 0) in %s +Fatal error: Declaration of CBroken::foo($a) must be compatible with TBroken2::foo($a, $b = 0) in %s on line %d diff --git a/Zend/tests/traits/bug60217c.phpt b/Zend/tests/traits/bug60217c.phpt index 401444e978..334487aeaa 100644 --- a/Zend/tests/traits/bug60217c.phpt +++ b/Zend/tests/traits/bug60217c.phpt @@ -22,4 +22,4 @@ class CBroken { $o = new CBroken; $o->foo(1); --EXPECTF-- -Fatal error: Declaration of TBroken2::foo($a) must be compatible with TBroken1::foo($a, $b = 0) in %s on line %d +Fatal error: Declaration of CBroken::foo($a) must be compatible with TBroken1::foo($a, $b = 0) in %s on line %d diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 2050e3ba71..ff466c5ef5 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -6111,7 +6111,7 @@ void zend_begin_method_decl(zend_op_array *op_array, zend_string *name, zend_boo } if (op_array->fn_flags & ZEND_ACC_ABSTRACT) { - if (op_array->fn_flags & ZEND_ACC_PRIVATE) { + if ((op_array->fn_flags & ZEND_ACC_PRIVATE) && !(ce->ce_flags & ZEND_ACC_TRAIT)) { zend_error_noreturn(E_COMPILE_ERROR, "%s function %s::%s() cannot be declared private", in_interface ? "Interface" : "Abstract", ZSTR_VAL(ce->name), ZSTR_VAL(name)); } diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index a4371db6f2..ad02380e35 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -34,12 +34,6 @@ static void add_property_compatibility_obligation( zend_class_entry *ce, const zend_property_info *child_prop, const zend_property_info *parent_prop); -static void overridden_ptr_dtor(zval *zv) /* {{{ */ -{ - efree_size(Z_PTR_P(zv), sizeof(zend_function)); -} -/* }}} */ - static void zend_type_copy_ctor(zend_type *type, zend_bool persistent) { if (ZEND_TYPE_HAS_LIST(*type)) { zend_type_list *old_list = ZEND_TYPE_LIST(*type); @@ -536,8 +530,10 @@ static inheritance_status zend_do_perform_implementation_check( && ((proto->common.scope->ce_flags & ZEND_ACC_INTERFACE) == 0 && (proto->common.fn_flags & ZEND_ACC_ABSTRACT) == 0))); - /* If the prototype method is private do not enforce a signature */ - ZEND_ASSERT(!(proto->common.fn_flags & ZEND_ACC_PRIVATE)); + /* If the prototype method is private and not abstract, we do not enforce a signature. + * private abstract methods can only occur in traits. */ + ZEND_ASSERT(!(proto->common.fn_flags & ZEND_ACC_PRIVATE) + || (proto->common.fn_flags & ZEND_ACC_ABSTRACT)); /* The number of required arguments cannot increase. */ if (proto->common.required_num_args < fe->common.required_num_args) { @@ -805,7 +801,7 @@ static void perform_delayable_implementation_check( } } -static zend_always_inline inheritance_status do_inheritance_check_on_method_ex(zend_function *child, zend_function *parent, zend_class_entry *ce, zval *child_zv, zend_bool check_only, zend_bool checked) /* {{{ */ +static zend_always_inline inheritance_status do_inheritance_check_on_method_ex(zend_function *child, zend_function *parent, zend_class_entry *ce, zval *child_zv, zend_bool check_visibility, zend_bool check_only, zend_bool checked) /* {{{ */ { uint32_t child_flags; uint32_t parent_flags = parent->common.fn_flags; @@ -852,7 +848,7 @@ static zend_always_inline inheritance_status do_inheritance_check_on_method_ex(z child->common.fn_flags |= ZEND_ACC_CHANGED; } - if (parent_flags & ZEND_ACC_PRIVATE) { + if ((parent_flags & ZEND_ACC_PRIVATE) && !(parent_flags & ZEND_ACC_ABSTRACT)) { return INHERITANCE_SUCCESS; } @@ -868,7 +864,7 @@ static zend_always_inline inheritance_status do_inheritance_check_on_method_ex(z parent = proto; } - if (!check_only && child->common.prototype != proto) { + if (!check_only && child->common.prototype != proto && child_zv) { do { if (child->common.scope != ce && child->type == ZEND_USER_FUNCTION @@ -876,7 +872,7 @@ static zend_always_inline inheritance_status do_inheritance_check_on_method_ex(z if (ce->ce_flags & ZEND_ACC_INTERFACE) { /* Few parent interfaces contain the same method */ break; - } else if (child_zv) { + } else { /* op_array wasn't duplicated yet */ zend_function *new_function = zend_arena_alloc(&CG(arena), sizeof(zend_op_array)); memcpy(new_function, child, sizeof(zend_op_array)); @@ -888,7 +884,8 @@ static zend_always_inline inheritance_status do_inheritance_check_on_method_ex(z } /* Prevent derived classes from restricting access that was available in parent classes (except deriving from non-abstract ctors) */ - if (!checked && (child_flags & ZEND_ACC_PPP_MASK) > (parent_flags & ZEND_ACC_PPP_MASK)) { + if (!checked && check_visibility + && (child_flags & ZEND_ACC_PPP_MASK) > (parent_flags & ZEND_ACC_PPP_MASK)) { if (check_only) { return INHERITANCE_ERROR; } @@ -907,9 +904,9 @@ static zend_always_inline inheritance_status do_inheritance_check_on_method_ex(z } /* }}} */ -static zend_never_inline void do_inheritance_check_on_method(zend_function *child, zend_function *parent, zend_class_entry *ce, zval *child_zv) /* {{{ */ +static zend_never_inline void do_inheritance_check_on_method(zend_function *child, zend_function *parent, zend_class_entry *ce, zval *child_zv, zend_bool check_visibility) /* {{{ */ { - do_inheritance_check_on_method_ex(child, parent, ce, child_zv, 0, 0); + do_inheritance_check_on_method_ex(child, parent, ce, child_zv, check_visibility, 0, 0); } /* }}} */ @@ -926,9 +923,10 @@ static zend_always_inline void do_inherit_method(zend_string *key, zend_function } if (checked) { - do_inheritance_check_on_method_ex(func, parent, ce, child, 0, checked); + do_inheritance_check_on_method_ex( + func, parent, ce, child, /* check_visibility */ 1, 0, checked); } else { - do_inheritance_check_on_method(func, parent, ce, child); + do_inheritance_check_on_method(func, parent, ce, child, /* check_visibility */ 1); } } else { @@ -1569,7 +1567,7 @@ static void zend_add_magic_methods(zend_class_entry* ce, zend_string* mname, zen } /* }}} */ -static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_string *key, zend_function *fn, HashTable **overridden) /* {{{ */ +static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_string *key, zend_function *fn) /* {{{ */ { zend_function *existing_fn = NULL; zend_function *new_fn; @@ -1583,31 +1581,18 @@ static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_ return; } + /* Abstract method signatures from the trait must be satisfied. */ + if (fn->common.fn_flags & ZEND_ACC_ABSTRACT) { + /* "abstract private" methods in traits were not available prior to PHP 8. + * As such, "abstract protected" was sometimes used to indicate trait requirements, + * even though the "implementing" method was private. Do not check visibility + * requirements to maintain backwards-compatibility with such usage. */ + do_inheritance_check_on_method(existing_fn, fn, ce, NULL, /* check_visibility */ 0); + return; + } + if (existing_fn->common.scope == ce) { /* members from the current class override trait methods */ - /* use temporary *overridden HashTable to detect hidden conflict */ - if (*overridden) { - if ((existing_fn = zend_hash_find_ptr(*overridden, key)) != NULL) { - if (existing_fn->common.fn_flags & ZEND_ACC_ABSTRACT) { - /* Make sure the trait method is compatible with previosly declared abstract method */ - perform_delayable_implementation_check(ce, fn, existing_fn); - } - if (fn->common.fn_flags & ZEND_ACC_ABSTRACT) { - /* Make sure the abstract declaration is compatible with previous declaration */ - perform_delayable_implementation_check(ce, existing_fn, fn); - return; - } - } - } else { - ALLOC_HASHTABLE(*overridden); - zend_hash_init_ex(*overridden, 8, NULL, overridden_ptr_dtor, 0, 0); - } - zend_hash_update_mem(*overridden, key, fn, sizeof(zend_function)); - return; - } else if ((fn->common.fn_flags & ZEND_ACC_ABSTRACT) - && !(existing_fn->common.fn_flags & ZEND_ACC_ABSTRACT)) { - /* Make sure the abstract declaration is compatible with previous declaration */ - perform_delayable_implementation_check(ce, existing_fn, fn); return; } else if (UNEXPECTED((existing_fn->common.scope->ce_flags & ZEND_ACC_TRAIT) && !(existing_fn->common.fn_flags & ZEND_ACC_ABSTRACT))) { @@ -1619,8 +1604,7 @@ static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_ } else { /* inherited members are overridden by members inserted by traits */ /* check whether the trait method fulfills the inheritance requirements */ - do_inheritance_check_on_method(fn, existing_fn, ce, NULL); - fn->common.prototype = NULL; + do_inheritance_check_on_method(fn, existing_fn, ce, NULL, /* check_visibility */ 1); } } @@ -1659,7 +1643,7 @@ static void zend_fixup_trait_method(zend_function *fn, zend_class_entry *ce) /* } /* }}} */ -static void zend_traits_copy_functions(zend_string *fnname, zend_function *fn, zend_class_entry *ce, HashTable **overridden, HashTable *exclude_table, zend_class_entry **aliases) /* {{{ */ +static void zend_traits_copy_functions(zend_string *fnname, zend_function *fn, zend_class_entry *ce, HashTable *exclude_table, zend_class_entry **aliases) /* {{{ */ { zend_trait_alias *alias, **alias_ptr; zend_string *lcname; @@ -1685,7 +1669,7 @@ static void zend_traits_copy_functions(zend_string *fnname, zend_function *fn, z } lcname = zend_string_tolower(alias->alias); - zend_add_trait_method(ce, alias->alias, lcname, &fn_copy, overridden); + zend_add_trait_method(ce, alias->alias, lcname, &fn_copy); zend_string_release_ex(lcname, 0); } alias_ptr++; @@ -1717,7 +1701,7 @@ static void zend_traits_copy_functions(zend_string *fnname, zend_function *fn, z } } - zend_add_trait_method(ce, fn->common.function_name, fnname, &fn_copy, overridden); + zend_add_trait_method(ce, fn->common.function_name, fnname, &fn_copy); } } /* }}} */ @@ -1895,7 +1879,6 @@ static void zend_traits_init_trait_structures(zend_class_entry *ce, zend_class_e static void zend_do_traits_method_binding(zend_class_entry *ce, zend_class_entry **traits, HashTable **exclude_tables, zend_class_entry **aliases) /* {{{ */ { uint32_t i; - HashTable *overridden = NULL; zend_string *key; zend_function *fn; @@ -1904,7 +1887,7 @@ static void zend_do_traits_method_binding(zend_class_entry *ce, zend_class_entry if (traits[i]) { /* copies functions, applies defined aliasing, and excludes unused trait methods */ ZEND_HASH_FOREACH_STR_KEY_PTR(&traits[i]->function_table, key, fn) { - zend_traits_copy_functions(key, fn, ce, &overridden, exclude_tables[i], aliases); + zend_traits_copy_functions(key, fn, ce, exclude_tables[i], aliases); } ZEND_HASH_FOREACH_END(); if (exclude_tables[i]) { @@ -1918,7 +1901,7 @@ static void zend_do_traits_method_binding(zend_class_entry *ce, zend_class_entry for (i = 0; i < ce->num_traits; i++) { if (traits[i]) { ZEND_HASH_FOREACH_STR_KEY_PTR(&traits[i]->function_table, key, fn) { - zend_traits_copy_functions(key, fn, ce, &overridden, NULL, aliases); + zend_traits_copy_functions(key, fn, ce, NULL, aliases); } ZEND_HASH_FOREACH_END(); } } @@ -1927,11 +1910,6 @@ static void zend_do_traits_method_binding(zend_class_entry *ce, zend_class_entry ZEND_HASH_FOREACH_PTR(&ce->function_table, fn) { zend_fixup_trait_method(fn, ce); } ZEND_HASH_FOREACH_END(); - - if (overridden) { - zend_hash_destroy(overridden); - FREE_HASHTABLE(overridden); - } } /* }}} */ @@ -2138,20 +2116,18 @@ typedef struct _zend_abstract_info { static void zend_verify_abstract_class_function(zend_function *fn, zend_abstract_info *ai) /* {{{ */ { - if (fn->common.fn_flags & ZEND_ACC_ABSTRACT) { - if (ai->cnt < MAX_ABSTRACT_INFO_CNT) { - ai->afn[ai->cnt] = fn; - } - if (fn->common.fn_flags & ZEND_ACC_CTOR) { - if (!ai->ctor) { - ai->cnt++; - ai->ctor = 1; - } else { - ai->afn[ai->cnt] = NULL; - } - } else { + if (ai->cnt < MAX_ABSTRACT_INFO_CNT) { + ai->afn[ai->cnt] = fn; + } + if (fn->common.fn_flags & ZEND_ACC_CTOR) { + if (!ai->ctor) { ai->cnt++; + ai->ctor = 1; + } else { + ai->afn[ai->cnt] = NULL; } + } else { + ai->cnt++; } } /* }}} */ @@ -2160,16 +2136,23 @@ void zend_verify_abstract_class(zend_class_entry *ce) /* {{{ */ { zend_function *func; zend_abstract_info ai; - - ZEND_ASSERT((ce->ce_flags & (ZEND_ACC_IMPLICIT_ABSTRACT_CLASS|ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS)) == ZEND_ACC_IMPLICIT_ABSTRACT_CLASS); + zend_bool is_explicit_abstract = (ce->ce_flags & ZEND_ACC_EXPLICIT_ABSTRACT_CLASS) != 0; memset(&ai, 0, sizeof(ai)); ZEND_HASH_FOREACH_PTR(&ce->function_table, func) { - zend_verify_abstract_class_function(func, &ai); + if (func->common.fn_flags & ZEND_ACC_ABSTRACT) { + /* If the class is explicitly abstract, we only check private abstract methods, + * because only they must be declared in the same class. */ + if (!is_explicit_abstract || (func->common.fn_flags & ZEND_ACC_PRIVATE)) { + zend_verify_abstract_class_function(func, &ai); + } + } } ZEND_HASH_FOREACH_END(); if (ai.cnt) { - zend_error_noreturn(E_ERROR, "Class %s contains %d abstract method%s and must therefore be declared abstract or implement the remaining methods (" MAX_ABSTRACT_INFO_FMT MAX_ABSTRACT_INFO_FMT MAX_ABSTRACT_INFO_FMT ")", + zend_error_noreturn(E_ERROR, !is_explicit_abstract + ? "Class %s contains %d abstract method%s and must therefore be declared abstract or implement the remaining methods (" MAX_ABSTRACT_INFO_FMT MAX_ABSTRACT_INFO_FMT MAX_ABSTRACT_INFO_FMT ")" + : "Class %s must implement %d abstract private method%s (" MAX_ABSTRACT_INFO_FMT MAX_ABSTRACT_INFO_FMT MAX_ABSTRACT_INFO_FMT ")", ZSTR_VAL(ce->name), ai.cnt, ai.cnt > 1 ? "s" : "", DISPLAY_ABSTRACT_FN(0), @@ -2450,7 +2433,9 @@ ZEND_API int zend_do_link_class(zend_class_entry *ce, zend_string *lc_parent_nam } else if (parent && parent->num_interfaces) { zend_do_inherit_interfaces(ce, parent); } - if ((ce->ce_flags & (ZEND_ACC_IMPLICIT_ABSTRACT_CLASS|ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS)) == ZEND_ACC_IMPLICIT_ABSTRACT_CLASS) { + if (!(ce->ce_flags & (ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT)) + && (ce->ce_flags & (ZEND_ACC_IMPLICIT_ABSTRACT_CLASS|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS)) + ) { zend_verify_abstract_class(ce); } @@ -2486,7 +2471,8 @@ static inheritance_status zend_can_early_bind(zend_class_entry *ce, zend_class_e if (zv) { zend_function *child_func = Z_FUNC_P(zv); inheritance_status status = - do_inheritance_check_on_method_ex(child_func, parent_func, ce, NULL, 1, 0); + do_inheritance_check_on_method_ex( + child_func, parent_func, ce, NULL, /* check_visibility */ 1, 1, 0); if (UNEXPECTED(status != INHERITANCE_SUCCESS)) { return status;