Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
+40 −12
Closed

Fix self inheritance type checks for traits #18296

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0afa0ac
Fix self inheritance type checks for traits
iluuu1994 Apr 10, 2025
231980c
Fix other tests
iluuu1994 Apr 10, 2025
a118d62
Only repeat zend_fixup_trait_method() for trait_contains_abstract_met…
iluuu1994 Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Zend/tests/inheritance/bug70957.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,4 @@ class B extends Foo
}
?>
--EXPECTF--
Fatal error: Declaration of T::bar() must be compatible with Foo::bar($a = 'Foo') in %s on line %d
Fatal error: Declaration of B::bar() must be compatible with Foo::bar($a = 'Foo') in %s on line %d
2 changes: 1 addition & 1 deletion Zend/tests/traits/bug78776.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,4 @@ B::createApp();

?>
--EXPECTF--
Fatal error: Cannot make non static method A::createApp() static in class C in %s on line %d
Fatal error: Cannot make non static method A::createApp() static in class B in %s on line %d
2 changes: 1 addition & 1 deletion Zend/tests/traits/bug81192.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@ class B extends A {

?>
--EXPECTF--
Fatal error: Declaration of T::foo(): string must be compatible with A::foo(): int in %sbug81192_trait.inc on line 4
Fatal error: Declaration of B::foo(): string must be compatible with A::foo(): int in %sbug81192_trait.inc on line 4
2 changes: 1 addition & 1 deletion Zend/tests/traits/bugs/abstract-methods05.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,4 @@ class TraitsTest1 {

?>
--EXPECTF--
Fatal error: Declaration of THelloB::hello() must be compatible with THelloA::hello($a) in %s on line %d
Fatal error: Declaration of TraitsTest1::hello() must be compatible with THelloA::hello($a) in %s on line %d
2 changes: 1 addition & 1 deletion Zend/tests/traits/bugs/abstract-methods06.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,4 @@ class TraitsTest1 {

?>
--EXPECTF--
Fatal error: Declaration of THelloB::hello() must be compatible with THelloA::hello($a) in %s on line %d
Fatal error: Declaration of TraitsTest1::hello() must be compatible with THelloA::hello($a) in %s on line %d
21 changes: 21 additions & 0 deletions Zend/tests/traits/gh18295.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
--TEST--
GH-18295: Parent self is substitutable with child self through trait
--FILE--
<?php

class A {
public function create(): ?A {}
}

trait T {
public function create(): self {}
}

class B extends A {
use T;
}

?>
===DONE===
--EXPECT--
===DONE===
2 changes: 1 addition & 1 deletion Zend/tests/traits/inheritance003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,4 @@ $o->sayHello(array());
--EXPECTF--
World!

Fatal error: Declaration of SayWorld::sayHello(Base $d) must be compatible with Base::sayHello(array $a) in %s on line %d
Fatal error: Declaration of MyHelloWorld::sayHello(Base $d) must be compatible with Base::sayHello(array $a) in %s on line %d
2 changes: 1 addition & 1 deletion Zend/tests/type_declarations/variance/trait_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@ class U extends X {

?>
--EXPECTF--
Fatal error: Could not check compatibility between T::method($r): B and X::method($a): A, because class B is not available in %s on line %d
Fatal error: Could not check compatibility between U::method($r): B and X::method($a): A, because class B is not available in %s on line %d
5 changes: 5 additions & 0 deletions Zend/zend_inheritance.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3608,6 +3608,11 @@ ZEND_API zend_class_entry *zend_do_link_class(zend_class_entry *ce, zend_string
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);

zend_function *fn;
ZEND_HASH_MAP_FOREACH_PTR(&ce->function_table, fn) {
zend_fixup_trait_method(fn, ce);
} ZEND_HASH_FOREACH_END();
Comment on lines 3609 to +3615
Copy link
Member

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?

Copy link
Member Author

@iluuu1994 iluuu1994 Apr 14, 2025 •
edited
Loading

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 remains self, it is not inferred.
  • During inheritance, trait methods are added to the class first. self in types remains, and will be replaced with func->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 only func but also func2 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 the ZEND_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.

}
if (parent) {
if (!(parent->ce_flags & ZEND_ACC_LINKED)) {
Expand Down
Loading