-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Fix self inheritance type checks for traits #18296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
zend_do_traits_constant_binding(ce, traits_and_interfaces); | ||
zend_do_traits_property_binding(ce, traits_and_interfaces); | ||
|
||
zend_function *fn; | ||
ZEND_HASH_MAP_FOREACH_PTR(&ce->function_table, fn) { | ||
zend_fixup_trait_method(fn, ce); | ||
} ZEND_HASH_FOREACH_END(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this also a possible issues for trait constants and properties that declare a type of self
.
Also I'm somehow confused why we need to re-iterate and call zend_fixup_trait_method
when it should already be called by zend_do_traits_method_binding()
.
But isn't the problem maybe with fixup_trait_scope()
not being called at the appropriate time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what happens:
self
in traits remainsself
, it is not inferred.- During inheritance, trait methods are added to the class first.
self
in types remains, and will be replaced withfunc->scope
at runtime and during inheritance checks. - This part is new in this PR: Once we're done with adding trait methods, we go through them again to set their scope to the target class. This is done after traits are added so we can distinguish between methods added through traits and those added directly to the class. This distinction matters, because classes may shadow trait methods, but will error when trying to add the same method from multiple traits.
- When linking the parent class, we perform compatibility checks between child and parent methods. Before this PR,
self
would be incorrectly resolved to the trait instead of the target class, leading to incompatibility errors. - After linking parent, we go through traits again if they contained any abstract methods. Abstract methods are not actually added to the parent, we will only check whether they are correctly implemented, after all inheritance steps have been performed.
- Because traits suck, there's another exception to this:
use T { T::func as func2 }
. In this case, not onlyfunc
but alsofunc2
are methods that need to be implemented.func2
will be copied to the class if it doesn't exist there. We then need to go over these methods again, fix the scope, and more importantly add theZEND_ACC_IMPLICIT_ABSTRACT_CLASS
flag to the class, so that we will actually error due to unimplemented methods.
That said, we can likely move the other zend_fixup_trait_method
iteration into the if (trait_contains_abstract_methods) {
so that we only perform this repeated iteration when we actually added more functions. I'll try this in a second.
Fixes GH-18295