Fix #79679: Constructor can not return any value (always void) by mvorisek · Pull Request #5678 · php/php-src

mvorisek · 2020-06-07T10:48:18Z

test for https://bugs.php.net/bug.php?id=79679

current situation with __construct() is:

documented as void
void can not be declared
but return value is allowed

it seems like a bug to me

Please help with impl.

As it introduces a small BC break probably for PHP8 only.

__destruct can be declared with void (thus not clear bug like this) so it should be probably solved in this PR/RFC #4177

mvorisek · 2020-06-07T10:55:32Z

+
+?>
+--EXPECTF--
+Fatal error: A void function must not return a value in %s:%d


Specific Constructor must not return a value message might be better

cmb69 · 2020-06-07T10:59:31Z

void can not be declared

But see https://3v4l.org/dbVah.

mvorisek · 2020-06-07T11:02:10Z

@cmb69 delete that 5 :D

cmb69 · 2020-06-07T11:18:50Z

Oh, right, but in my opinion, declaring void should be permissible.

mvorisek · 2020-06-07T11:22:42Z

So either nothing or void? I am fine with that :) Can you help to implement this?

About the destructor, do you think it should be added to that related RFC/PR?

cmb69 · 2020-06-07T11:49:06Z

About the destructor, do you think it should be added to that related RFC/PR?

I assume you're referring to https://wiki.php.net/rfc/magic-methods-signature. That RFC explicitly excludes changes to __construct() and __destruct() and is already in voting.

Anyhow, potential implementation to allow :void for __construct() and __destruct():

 Zend/zend_compile.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index 86e169893a..509a7fcda0 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -6894,7 +6894,8 @@ void zend_compile_class_decl(znode *result, zend_ast *ast, zend_bool toplevel) /
 			zend_error_noreturn(E_COMPILE_ERROR, "Constructor %s::%s() cannot be static",
 				ZSTR_VAL(ce->name), ZSTR_VAL(ce->constructor->common.function_name));
 		}
-		if (ce->constructor->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE) {
+		if (ce->constructor->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE
+			&& ZEND_TYPE_PURE_MASK(ce->constructor->common.arg_info[-1].type) != MAY_BE_VOID) {
 			zend_error_noreturn(E_COMPILE_ERROR,
 				"Constructor %s::%s() cannot declare a return type",
 				ZSTR_VAL(ce->name), ZSTR_VAL(ce->constructor->common.function_name));
@@ -6904,7 +6905,8 @@ void zend_compile_class_decl(znode *result, zend_ast *ast, zend_bool toplevel) /
 		if (ce->destructor->common.fn_flags & ZEND_ACC_STATIC) {
 			zend_error_noreturn(E_COMPILE_ERROR, "Destructor %s::%s() cannot be static",
 				ZSTR_VAL(ce->name), ZSTR_VAL(ce->destructor->common.function_name));
-		} else if (ce->destructor->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE) {
+		} else if (ce->destructor->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE
+			&& ZEND_TYPE_PURE_MASK(ce->destructor->common.arg_info[-1].type) != MAY_BE_VOID) {
 			zend_error_noreturn(E_COMPILE_ERROR,
 				"Destructor %s::%s() cannot declare a return type",
 				ZSTR_VAL(ce->name), ZSTR_VAL(ce->destructor->common.function_name));

mvorisek · 2020-06-07T12:01:21Z

@cmb69 feel free to commit it here directly, but constructor is very special, I am not sure if this will be not misused by other people by simply adding is everywhere (or is that desired?)

Anyway, I think constructor as well as destructor should be not allowed to return anything because the return can not be used when used with "new" or when called by GC.

capturing value from parent::__constructor() should be probably not allowed as it makes sense to be called only from parent constructor method and value can be passed (if needed for some reasons) by standard properties. At first sight it make look like a limitation, but when you think the parent class can be instantiated directly, it makes sense to not allow this. My point of view.

I assume you're referring to https://wiki.php.net/rfc/magic-methods-signature. That RFC explicitly excludes changes to __construct() and __destruct() and is already in voting.

Yes, but the RFC is saying something that is not fully true. So I think it makes sense to extend it after short internal discussion/approval. That is up to you guys :)

carusogabriel · 2020-06-07T14:03:11Z

Yes, but the RFC is saying something that is not fully true

Do you have an example in any other language where constructors and destructors return anything?

cmb69 · 2020-06-07T14:13:55Z

Do you have an example in any other language where constructors and destructors return anything?

My point is that : void signals, that the function does not return any value. Furthermore, it is (still) possible to call __construct() as regular method (https://3v4l.org/P1JJU).

mvorisek · 2020-06-07T14:26:22Z

Yes, but the RFC is saying something that is not fully true

Do you have an example in any other language where constructors and destructors return anything?

I reacted on, citing:

all languages, including PHP, don't have the concept of Constructors and Destructors “returning” something after their execution

so for these reasons I think it should be part of your RFC and I think the community will be ok with that

mvorisek · 2020-06-07T14:29:07Z

Do you have an example in any other language where constructors and destructors return anything?

My point is that : void signals, that the function does not return any value. Furthermore, it is (still) possible to call __construct() as regular method (https://3v4l.org/P1JJU).

That is true, but based on the proposed RFC for other special methods this will no longer be true, so this argumentation is no longer valid then - I think the only possible scenarion, when void makes sense is generated code then, and I think constructor is so much special and used, that not allowing void there is good, If I remember correctly also Java has some restricted syntax for constructor

nikic · 2020-06-07T14:34:19Z

It really shouldn't be necessary (and by induction, allowed) to add : void just to get the check that constructors mustn't return anything.

mvorisek · 2020-06-13T14:41:45Z

@cmb69 Can you advice what to add to modify the parser to parse every constructor method like if it was defined inside void method?

cmb69 · 2020-06-15T08:42:10Z

I think you can do something like:

 Zend/zend_compile.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index a2c8b9f23e..e64c3e0527 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -6464,8 +6464,15 @@ void zend_compile_func_decl(znode *result, zend_ast *ast, zend_bool toplevel) /*
 		zend_stack_push(&CG(loop_var_stack), (void *) &dummy_var);
 	}
 
-	zend_compile_params(params_ast, return_type_ast,
-		is_method && zend_string_equals_literal_ci(decl->name, "__toString") ? IS_STRING : 0);
+	uint32_t fallback_return_type = 0;
+	if (is_method) {
+		if (zend_string_equals_literal_ci(decl->name, "__toString")) {
+			fallback_return_type = IS_STRING;
+		} else if (zend_string_equals_literal_ci(decl->name, "__construct")) {
+			fallback_return_type = IS_VOID;
+		}
+	}
+	zend_compile_params(params_ast, return_type_ast, fallback_return_type);
 	if (CG(active_op_array)->fn_flags & ZEND_ACC_GENERATOR) {
 		zend_mark_function_as_generator();
 		zend_emit_op(NULL, ZEND_GENERATOR_CREATE, NULL, NULL);

cmb69 · 2020-06-15T09:20:46Z

You need to also allow the :void return type; see #5678 (comment)

cmb69 · 2020-06-16T08:37:14Z

The AppVeyor CI failure indicates that several test cases would need to be updated for this change. I'm not sure about the Travis CI failure; does this PR break pear?

Anyhow, there is now https://wiki.php.net/rfc/constructor_return_type.

mvorisek · 2020-06-16T08:53:40Z

Anyhow, there is now https://wiki.php.net/rfc/constructor_return_type.

@cmb69 I am happy if someone can take this idea to happen. I commented on the related PR and I am closing this PR for now.

moliata · 2020-06-16T15:24:04Z

Since you were the original author of this PR, I will happily add credits to you in the RFC 😄. I personally wanted to only make an RFC for allowing : void on __construct() and __destruct() since that was a more controversial discussion but since you closed the PR, I'll take over it and implement signature validation myself and add it as part of the RFC.

Best regards,
Benas

mvorisek · 2020-06-16T15:40:38Z

Since you were the original author of this PR, I will happily add credits to you in the RFC 😄

Thanks - please add me there as "Michael Voříšek <vorismi3 (at) fel.cvut.cz>".

I already send you some feedback to your email. I pointed out there that currently constructor method can return non-void but when the object is instanced the value can not be read. So for these reasons, I think we should at least offer one paragraph to that in that RFC and vote about it extra.

The same for destructor, what is the possitive of allowing to return something from destructor method?

I propose to imply implicit void by default for these methods and throw on compile time (like when void would be declared in the method header)

moliata · 2020-06-16T16:54:06Z

Thanks, your email was marked as spam for me. Anyways, let's continue our discussion through email, I'll hit you up soon.

Fix #79679: Constructor can not return any value (always void)

5c69644

mvorisek commented Jun 7, 2020

View reviewed changes

impl

24f8f05

allow constructor/destructor void return type

1405a5f

mvorisek mentioned this pull request Jun 16, 2020

[RFC] Allow void return type on constructors/destructors #5717

Closed

2 tasks

mvorisek closed this Jun 16, 2020

mvorisek deleted the patch-2 branch June 16, 2020 08:53

Conversation

mvorisek commented Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvorisek Jun 7, 2020

Choose a reason for hiding this comment

Uh oh!

cmb69 commented Jun 7, 2020

Uh oh!

mvorisek commented Jun 7, 2020

Uh oh!

cmb69 commented Jun 7, 2020

Uh oh!

mvorisek commented Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmb69 commented Jun 7, 2020

Uh oh!

mvorisek commented Jun 7, 2020

Uh oh!

carusogabriel commented Jun 7, 2020

Uh oh!

cmb69 commented Jun 7, 2020

Uh oh!

mvorisek commented Jun 7, 2020

Uh oh!

mvorisek commented Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Jun 7, 2020

Uh oh!

mvorisek commented Jun 13, 2020

Uh oh!

cmb69 commented Jun 15, 2020

Uh oh!

cmb69 commented Jun 15, 2020

Uh oh!

cmb69 commented Jun 16, 2020

Uh oh!

mvorisek commented Jun 16, 2020

Uh oh!

moliata commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvorisek commented Jun 16, 2020

Uh oh!

moliata commented Jun 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mvorisek commented Jun 7, 2020 •

edited

Loading

mvorisek commented Jun 7, 2020 •

edited

Loading

mvorisek commented Jun 7, 2020 •

edited

Loading

moliata commented Jun 16, 2020 •

edited

Loading