GeezerVison

What about void return types?

What about void return types?

@nikic I ensured no return type to match the current implementation for __construct, __destruct, __clone, etc. Also, newbie question: what are the difference between no return and void, talking about magic method?

Another point is that I couldn't figure out how to implement the verification of the return of a magic method, the same was __toString does: https://3v4l.org/j4AZu.

I tried this one with __isset but is wrong, because it enforces the real return type, instead of checking it like __toString:

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index c539b4ec10..bf0d9258fc 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -6265,6 +6265,14 @@ zend_op *zend_compile_class_decl(zend_ast *ast, zend_bool toplevel) /* {{{ */
                }
        }
 
+       if (ce->__isset) {
+               if (UNEXPECTED(Z_TYPE_P(ce->__isset) != IS_BOOL)) {
+                       zend_error_noreturn(E_COMPILE_ERROR,
+                               "Method %s::%s() must return a boolean value",
+                               ZSTR_VAL(ce->name), ZSTR_VAL(ce->__isset->common.function_name));
+               }
+       }
+
        if (implements_ast) {
                zend_compile_implements(implements_ast);
        }

I ensured no return type to match the current implementation for __construct, __destruct, __clone

I'd probably rather confront special-casing of these methods, since returning from (as well as calling) constructor, destructor or clone is perfectly valid in current PHP:
https://3v4l.org/rGc85

I tried this one with __isset but is wrong, because it enforces the real return type, instead of checking it like __toString

Should then be something like:

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index c539b4ec10..bf0d9258fc 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -6265,6 +6265,14 @@ zend_op *zend_compile_class_decl(zend_ast *ast, zend_bool toplevel) /* {{{ */
                }
        }
 
+       if (ce->__isset && ce->__isset->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE) {
+               if (UNEXPECTED(Z_TYPE_P(ce->__isset) != IS_BOOL)) {
+                       zend_error_noreturn(E_COMPILE_ERROR,
+                               "Method %s::%s() must return a boolean value",
+                               ZSTR_VAL(ce->name), ZSTR_VAL(ce->__isset->common.function_name));
+               }
+       }
+
        if (implements_ast) {
                zend_compile_implements(implements_ast);
        }

I'd probably rather confront special-casing of these methods, since returning from (as well as calling) constructor, destructor or clone is perfectly valid in current PHP: https://3v4l.org/rGc85

Ok, I got the point of the BC you're trying to say: calling these methods themselves aren't gonna work anymore. So yes, probably after the implementation itself, we'll need a formal RFC.

if (UNEXPECTED(Z_TYPE_P(ce->__isset) != IS_BOOL)) {

This is wrong, ce->__isset is a zend_function* not a zval* ... in zend_begin_method_decl is where magic methods are verified to conform (non-static, public).

@krakjoe Thanks for the help. I’ll implement the type check for the other methods so we can start the discussion on Internals about moving this RFC forward or not.

@carusogabriel I think my preference would be if magic methods could either omit the return type or have the correct one. For example __construct or __construct: void would be both legal. What do you think?

@kocsismate I still have the same opinion: #4177 (comment) :)

But this is something that I'll also raise on internals@/RFC

@kocsismate I still have still opinion: #4177 (comment) :)

But this is something that I'll also raise on internals@/RFC

We've already introduced that kind of magic with the Stringable RFC to automagically add the correct return type to not break inheritance, so I think "enforcing" the void return type and doing some engine magic may be the best way ...

RFC now updated allowing void as a return type for some methods.

Something that I need help to refactor: currently, there are some places with magic methods checks/rules:

• zend_check_magic_method_implementation() at Zend/zend_API.c
• zend_register_functions() at Zend/zend_API.c
• zend_compile_class_decl() at Zend/zend_compile.c
• zend_check_magic_method_attr() at Zend/zend_compile.c
• zend_begin_method_decl() at Zend/zend_compile.c

Shouldn't we centralize it in one place, and if so, where?

@carusogabriel I think, first the correct behaviour which is in line with the RFC should be achieved and tested in more detail, because unfortunately I see some concerning things with the implementation :( Most of them have already been mentioned in previous review comments, but I'll collect them here:

• The run-time behaviour needs tests because they are missing now. Moreover, it seemed to me that in some cases argument and return types were checked run-time, which is probably not what you wanted. So I'd be curious whether if it only happens on my system or not. See for more details: [RFC] Ensure correct magic methods' signatures when typed #4177 (comment)
• Inheritance validations also needs more tests with a non-trivial class hierarchy. If you enable argument/return type variance, then I believe classes have to be validated against their parent classes too. I don't know if that happens now, but the behaviour should be verified by tests. See for more details: [RFC] Ensure correct magic methods' signatures when typed #4177 (comment)
• I think Nikita had a question about reflection in a mail. So adding reflection tests (which ensure that the correct types are reported) would also be very beneficial.
• Although I suggested in an earlier comment ([RFC] Ensure correct magic methods' signatures when typed #4177 (comment)) that error messages should follow the Declaration must be compatible ... format, but now it seems that it has to be tweaked, because Declaration of Foo::__set() must be compatible with Foo::__set(string $name, mixed $value): void doesn't sound very well. :) + this format only works well if proper variance is enabled.

* Inheritance validations also needs more tests with a non-trivial class hierarchy. If you enable argument/return type variance, then I believe classes have to be validated against their parent classes too. I don't know if that happens now, but the behaviour should be verified by tests. See for more details: [#4177 (comment)](https://github.com/php/php-src/pull/4177#discussion_r453231506)

Normal inheritance checks still apply to magic methods, so this should "just work". What is added here are only additional constraints because the methods are not part of an interface. Of course, it would be good to add one test to double check this.

* I think Nikita had a question about reflection in a mail. So adding reflection tests (which ensure that the correct types are reported) would also be very beneficial.

As this RFC does not magically add any types (only checks those declared), I'm okay without additional tests for this.

@kocsismate About your comment here #4177 (review), this is a problem with all the methods, not just the magic ones. This won't be solved in this RFC :)

See https://3v4l.org/gB9nf

@carusogabriel This is not a problem at all, since any type declaration must be enforced at runtime, there should just be no other option IMO. But based on chat messages and what the RFC said, my impression was that it's not the outcome you expect. E.g.:

This RFC only aims to add checks for the methods' signatures but as a Future Scope, a runtime check of what is been returning in the methods could be added

That's why I suggested that you should add tests to double-check the run-time behaviour so that there are no surprises. Additionally, I think the RFC text should be updated if the patch is not in line with it, but the behaviour described in the RFC is fundamentally problematic.

Or am I missing something? Sorry if I didn't get what the idea exactly is. However I think it's useful anyway to clarify it.

@kocsismate Ok, let me try to make myself clearer.

This RFC aims to expand existent checks for the magic method's signatures, and only that. These checks happen at "compile-time", like the ones that already exist:

• https://3v4l.org/Ub54p
• https://3v4l.org/CCL11
• https://3v4l.org/HNkgW

Runtime checks, to check what is been returning from the methods, they are in the "Future Scope" of this RFC. Existent checks for illustration:

• https://3v4l.org/E41eb
• https://3v4l.org/5Rkam

Is that clearer? I can update the RFC if there's something unclear there, just let me know :)

Thanks for your concerns, btw.

@carusogabriel Thanks! Then the text of the RFC text as well as my understanding about your intentions is up-to-date :) However, I became a bit confused because of the example you posted (https://3v4l.org/gB9nf), since type declarations here are ensured at run-time, and I was in the belief that you explicitly don't want this.

Additionally, it seemed to me during testing your patch locally that type declarations are also enforced at run-time. As I don't know how type verification of magic methods exactly work when there are type declarations, I think adding tests would be useful (although I saw there is a custom code for checking the return type of e.g. __toString() and __debugInfo()). Alternatively, we could rely on Nikita's advices. :)

This PR will be rebased based on the changes made by @nikic (dcaf62f...efce369) fixing the problems with 5 different methods checking magic methods in different places :)

Small reminder to rebase this :)

Just dropping by to say that I look forward to seeing this land ❤️

@jrfnl Almost there, I hope to get it ready by this weekend 😄

@nikic @kocsismate Implementation updated. Please, take a look again :)

The implementation looks much better now!

Merged as e3d06fc.

Thanks everyone for your reviews and help 🚀

@carusogabriel why __set_state returns object and not self or static?