Conversation
Girgias
force-pushed
the
inherit-interfaces-early
branch
from
cb173f5 to
91c5a7c
Compare
arnaud-lb
left a comment
arnaud-lb
left a comment
There was a problem hiding this comment.
I don't see anything blocking from moving zend_do_inherit_interfaces() at the beginning of do_interface_implementation(), but maybe wait for other reviews.
One potential BC would be that interface_gets_implemented is now called early, but this doesn't appear to be an issue in practice.
The only thing I could see happening is a different compile time error being emitted, but I didn't manage to find such a test case. |
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Can't meaningfully comment on the change itself, as I am not familiar with the code.
| static inline void do_implement_interface(zend_class_entry *ce, zend_class_entry *iface) | ||
| { | ||
| do_implement_interface_ex(ce, iface, iface); | ||
| } |
There was a problem hiding this comment.
Do we really need the _ex? distinction? The function is static and only used 2 times from what I see.
| /* }}} */ | ||
|
|
||
| static inline void do_implement_interface(zend_class_entry *ce, zend_class_entry *iface) /* {{{ */ | ||
| static inline void do_implement_interface_ex(zend_class_entry *ce, zend_class_entry *inherited_face, zend_class_entry *base_iface) |
There was a problem hiding this comment.
Inconsistent naming, face vs. iface. I also find the names confusing. With the name do_implement_interface_ex, it's not clear which interface is being implelmented.
| { | ||
| if (!(ce->ce_flags & ZEND_ACC_INTERFACE) && iface->interface_gets_implemented && iface->interface_gets_implemented(iface, ce) == FAILURE) { | ||
| zend_error_noreturn(E_CORE_ERROR, "%s %s could not implement interface %s", zend_get_object_type_uc(ce), ZSTR_VAL(ce->name), ZSTR_VAL(iface->name)); | ||
| if (!(ce->ce_flags & ZEND_ACC_INTERFACE) && inherited_face->interface_gets_implemented && inherited_face->interface_gets_implemented(base_iface, ce) == FAILURE) { |
There was a problem hiding this comment.
Is this correct? interface_gets_implemented() should be able to validate if an inteface can be implemented, but if we only pass the base_iface, we'll miss validation for the actual inherited interface. E.g. will this break if UnitEnum is added to a class implicitly through an inherited interface?
Is this change needed at all?
4 participants
Needed for #18260 so that different bound types for implicit and explicitly implemented interfaces are checked correctly due to how the implementation stores the types in the HashTable.
I don't see any particular reason as to why the inheritance of interfaces was delayed, thus moving it early doesn't seem like an issue to me.