From 00b7f83d06233d8a9722f92448407294ebde335c Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Fri, 13 Sep 2024 17:57:04 +0200 Subject: [PATCH 1/3] Bind traits before parent class This more accurately matches the "copy & paste" semantics described in the documentation. Abstract trait methods diverge from this behavior, given that a parent method can satisfy trait methods used in the child. In that case, the method is not copied, but the check is performed after the parent has been bound. Fixes GH-15753 Fixes GH-16198 --- Zend/tests/traits/constant_015.phpt | 14 +---- Zend/tests/traits/error_001.phpt | 4 +- Zend/tests/traits/gh16198.phpt | 36 +++++++++++++ Zend/tests/traits/gh16198_2.phpt | 27 ++++++++++ Zend/zend_inheritance.c | 79 ++++++++++++++++------------- 5 files changed, 111 insertions(+), 49 deletions(-) create mode 100644 Zend/tests/traits/gh16198.phpt create mode 100644 Zend/tests/traits/gh16198_2.phpt diff --git a/Zend/tests/traits/constant_015.phpt b/Zend/tests/traits/constant_015.phpt index 24507ea9e59c..f8a2740d5c31 100644 --- a/Zend/tests/traits/constant_015.phpt +++ b/Zend/tests/traits/constant_015.phpt @@ -15,18 +15,6 @@ class DerivedClass1 extends BaseClass1 { use TestTrait1; } -trait TestTrait2 { - public final const Constant = 123; -} - -class BaseClass2 { - public final const Constant = 456; -} - -class DerivedClass2 extends BaseClass2 { - use TestTrait2; -} - ?> --EXPECTF-- -Fatal error: BaseClass2 and TestTrait2 define the same constant (Constant) in the composition of DerivedClass2. However, the definition differs and is considered incompatible. Class was composed in %s on line %d +Fatal error: DerivedClass1::Constant cannot override final constant BaseClass1::Constant in %s on line %d diff --git a/Zend/tests/traits/error_001.phpt b/Zend/tests/traits/error_001.phpt index a7889da41e2b..c3749395cd53 100644 --- a/Zend/tests/traits/error_001.phpt +++ b/Zend/tests/traits/error_001.phpt @@ -16,7 +16,7 @@ trait foo2 { } -class A extends foo { +class A { use foo { foo2::foo insteadof foo; foo2::foo insteadof foo; @@ -25,4 +25,4 @@ class A extends foo { ?> --EXPECTF-- -Fatal error: Class A cannot extend trait foo in %s on line %d +Fatal error: Required Trait foo2 wasn't added to A in %s on line %d diff --git a/Zend/tests/traits/gh16198.phpt b/Zend/tests/traits/gh16198.phpt new file mode 100644 index 000000000000..f44926cf5ce2 --- /dev/null +++ b/Zend/tests/traits/gh16198.phpt @@ -0,0 +1,36 @@ +--TEST-- +GH-16198: Incorrect trait constant conflict when declared via trait +--FILE-- + +===DONE=== +--EXPECT-- +===DONE=== diff --git a/Zend/tests/traits/gh16198_2.phpt b/Zend/tests/traits/gh16198_2.phpt new file mode 100644 index 000000000000..7599ea1a11b4 --- /dev/null +++ b/Zend/tests/traits/gh16198_2.phpt @@ -0,0 +1,27 @@ +--TEST-- +GH-16198: Usage of super in cloned trait method +--FILE-- + +--EXPECT-- +string(13) "P::__destruct" diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index bcccf978e2a0..9f8338d089ff 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -1135,7 +1135,10 @@ static inheritance_status do_inheritance_check_on_method( #define SEPARATE_METHOD() do { \ if ((flags & ZEND_INHERITANCE_LAZY_CHILD_CLONE) \ - && child_scope != ce && child->type == ZEND_USER_FUNCTION) { \ + && child_scope != ce \ + /* Trait scopes are fixed after inheritance. However, they are always duplicated. */ \ + && !(child_scope->ce_flags & ZEND_ACC_TRAIT) \ + && child->type == ZEND_USER_FUNCTION) { \ /* 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)); \ @@ -2695,7 +2698,7 @@ 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) /* {{{ */ +static void zend_do_traits_method_binding(zend_class_entry *ce, zend_class_entry **traits, HashTable **exclude_tables, zend_class_entry **aliases, bool verify_abstract, bool *contains_abstract_methods) /* {{{ */ { uint32_t i; zend_string *key; @@ -2706,6 +2709,10 @@ 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_MAP_FOREACH_STR_KEY_PTR(&traits[i]->function_table, key, fn) { + if (verify_abstract != (bool) (fn->common.fn_flags & ZEND_ACC_ABSTRACT)) { + *contains_abstract_methods = true; + continue; + } zend_traits_copy_functions(key, fn, ce, exclude_tables[i], aliases); } ZEND_HASH_FOREACH_END(); @@ -2720,15 +2727,15 @@ 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_MAP_FOREACH_STR_KEY_PTR(&traits[i]->function_table, key, fn) { + if (verify_abstract != (bool) (fn->common.fn_flags & ZEND_ACC_ABSTRACT)) { + *contains_abstract_methods = true; + continue; + } zend_traits_copy_functions(key, fn, ce, NULL, aliases); } ZEND_HASH_FOREACH_END(); } } } - - ZEND_HASH_MAP_FOREACH_PTR(&ce->function_table, fn) { - zend_fixup_trait_method(fn, ce); - } ZEND_HASH_FOREACH_END(); } /* }}} */ @@ -3010,33 +3017,6 @@ static void zend_do_traits_property_binding(zend_class_entry *ce, zend_class_ent } /* }}} */ -static void zend_do_bind_traits(zend_class_entry *ce, zend_class_entry **traits) /* {{{ */ -{ - HashTable **exclude_tables; - zend_class_entry **aliases; - - ZEND_ASSERT(ce->num_traits > 0); - - /* complete initialization of trait structures in ce */ - zend_traits_init_trait_structures(ce, traits, &exclude_tables, &aliases); - - /* first care about all methods to be flattened into the class */ - zend_do_traits_method_binding(ce, traits, exclude_tables, aliases); - - if (aliases) { - efree(aliases); - } - - if (exclude_tables) { - efree(exclude_tables); - } - - /* then flatten the constants and properties into it, to, mostly to notify developer about problems */ - zend_do_traits_constant_binding(ce, traits); - zend_do_traits_property_binding(ce, traits); -} -/* }}} */ - #define MAX_ABSTRACT_INFO_CNT 3 #define MAX_ABSTRACT_INFO_FMT "%s%s%s%s" #define DISPLAY_ABSTRACT_FN(idx) \ @@ -3649,6 +3629,15 @@ ZEND_API zend_class_entry *zend_do_link_class(zend_class_entry *ce, zend_string zend_link_hooked_object_iter(ce); #endif + HashTable **trait_exclude_tables; + zend_class_entry **trait_aliases; + bool trait_contains_abstract_methods = false; + if (ce->num_traits) { + zend_traits_init_trait_structures(ce, traits_and_interfaces, &trait_exclude_tables, &trait_aliases); + zend_do_traits_method_binding(ce, traits_and_interfaces, trait_exclude_tables, trait_aliases, false, &trait_contains_abstract_methods); + zend_do_traits_constant_binding(ce, traits_and_interfaces); + zend_do_traits_property_binding(ce, traits_and_interfaces); + } if (parent) { if (!(parent->ce_flags & ZEND_ACC_LINKED)) { add_dependency_obligation(ce, parent); @@ -3656,7 +3645,29 @@ ZEND_API zend_class_entry *zend_do_link_class(zend_class_entry *ce, zend_string zend_do_inheritance(ce, parent); } if (ce->num_traits) { - zend_do_bind_traits(ce, traits_and_interfaces); + if (trait_contains_abstract_methods) { + zend_do_traits_method_binding(ce, traits_and_interfaces, trait_exclude_tables, trait_aliases, true, &trait_contains_abstract_methods); + } + + if (trait_exclude_tables) { + for (i = 0; i < ce->num_traits; i++) { + if (traits_and_interfaces[i]) { + if (trait_exclude_tables[i]) { + zend_hash_destroy(trait_exclude_tables[i]); + FREE_HASHTABLE(trait_exclude_tables[i]); + } + } + } + efree(trait_exclude_tables); + } + if (trait_aliases) { + efree(trait_aliases); + } + + zend_function *fn; + ZEND_HASH_MAP_FOREACH_PTR(&ce->function_table, fn) { + zend_fixup_trait_method(fn, ce); + } ZEND_HASH_FOREACH_END(); } if (ce->num_interfaces) { /* Also copy the parent interfaces here, so we don't need to reallocate later. */ From 5faee303d209c0bff2bba1159bf5a39c224ebc8d Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Thu, 27 Mar 2025 19:22:06 +0100 Subject: [PATCH 2/3] Improve comment --- Zend/zend_inheritance.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index 9f8338d089ff..6233c51a7d11 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -1136,7 +1136,9 @@ static inheritance_status do_inheritance_check_on_method( #define SEPARATE_METHOD() do { \ if ((flags & ZEND_INHERITANCE_LAZY_CHILD_CLONE) \ && child_scope != ce \ - /* Trait scopes are fixed after inheritance. However, they are always duplicated. */ \ + /* Trait methods have already been separated at this point. However, their */ \ + /* scope isn't fixed until after inheritance checks to preserve the scope */ \ + /* in error messages. Skip them here explicitly. */ \ && !(child_scope->ce_flags & ZEND_ACC_TRAIT) \ && child->type == ZEND_USER_FUNCTION) { \ /* op_array wasn't duplicated yet */ \ From 2ace8771651823ab5ab00ce77a54d90cb4e8ddba Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Thu, 27 Mar 2025 19:22:17 +0100 Subject: [PATCH 3/3] Remove now unused trait inheritance check zend_add_trait_method() never overrides a parent method, given that parents are not bound yet. Instead, compatibility is checked in do_inherit_method(), as per usual. --- Zend/zend_inheritance.c | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index 6233c51a7d11..1455600d9d0b 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -2355,7 +2355,6 @@ static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_ { zend_function *existing_fn = NULL; zend_function *new_fn; - bool check_inheritance = false; if ((existing_fn = zend_hash_find_ptr(&ce->function_table, key)) != NULL) { /* if it is the same function with the same visibility and has not been assigned a class scope yet, regardless @@ -2389,8 +2388,6 @@ static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_ ZSTR_VAL(fn->common.scope->name), ZSTR_VAL(fn->common.function_name), ZSTR_VAL(ce->name), ZSTR_VAL(name), ZSTR_VAL(existing_fn->common.scope->name), ZSTR_VAL(existing_fn->common.function_name)); - } else { - check_inheritance = true; } } @@ -2410,19 +2407,6 @@ static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_ function_add_ref(new_fn); fn = zend_hash_update_ptr(&ce->function_table, key, new_fn); zend_add_magic_method(ce, fn, key); - - if (check_inheritance) { - /* Inherited members are overridden by members inserted by traits. - * Check whether the trait method fulfills the inheritance requirements. */ - uint32_t flags = ZEND_INHERITANCE_CHECK_PROTO | ZEND_INHERITANCE_CHECK_VISIBILITY; - if (!(existing_fn->common.scope->ce_flags & ZEND_ACC_TRAIT)) { - flags |= ZEND_INHERITANCE_SET_CHILD_CHANGED |ZEND_INHERITANCE_SET_CHILD_PROTO | - ZEND_INHERITANCE_RESET_CHILD_OVERRIDE; - } - do_inheritance_check_on_method( - fn, fixup_trait_scope(fn, ce), existing_fn, fixup_trait_scope(existing_fn, ce), - ce, NULL, flags); - } } /* }}} */