Skip to content
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

never return type for arrow functions with throw expressions #7900

Closed
weirdan opened this issue Jan 7, 2022 · 13 comments
Closed

never return type for arrow functions with throw expressions #7900

weirdan opened this issue Jan 7, 2022 · 13 comments

Comments

@weirdan
Copy link
Contributor

weirdan commented Jan 7, 2022

Description

The following code:

<?php

fn(): never => throw new Exception;

Resulted in this output:

Fatal error: A never-returning function must not return in /in/qWWuD on line 3

But I expected this output instead:

Obviously, it's caused by implicit return, but throw expressions never actually return.

PHP Version

8.1.x

Operating System

No response

@weirdan
Copy link
Contributor Author

weirdan commented Jan 7, 2022

/cc: @muglug & @ondrejmirtes (authors of the RFC that introduced never).

@ondrejmirtes
Copy link
Contributor

Funny - both PHPStan (https://phpstan.org/r/a18b627c-e478-4091-ab28-429523eece5e) and Psalm (https://psalm.dev/r/88e0288cfd) also report this wrong :)

Unfortunately I don't have the skillset to fix this but I agree this is a PHP bug: https://3v4l.org/MOKDU

@weirdan
Copy link
Contributor Author

weirdan commented Jan 7, 2022

both PHPStan and Psalm also report this wrong :)

It's not exactly wrong as long as PHP doesn't support it.

@ondrejmirtes
Copy link
Contributor

Also true 😊

@bwoebi
Copy link
Member

bwoebi commented Jan 7, 2022

The general problem is that return <expr> is forbidden at compile-time (which the short closure expands to). Now, if we loosen this restriction, we have to completely get rid of the compile-time check, for short closures, and just check at runtime. (We cannot just see throw, because calling another function returning never is equally valid.)

@ricardoboss
Copy link

Would this also apply to functions returning void? https://3v4l.org/YkVBg

@iluuu1994
Copy link
Member

PHP adds an implicit return to arrow functions.

| fn returns_ref backup_doc_comment '(' parameter_list ')' return_type
T_DOUBLE_ARROW backup_fn_flags backup_lex_pos expr backup_fn_flags
{ $$ = zend_ast_create_decl(ZEND_AST_ARROW_FUNC, $2 | $12, $1, $3,
zend_string_init("{closure}", sizeof("{closure}") - 1, 0), $5, NULL,
zend_ast_create(ZEND_AST_RETURN, $11), $7, NULL);
((zend_ast_decl *) $$)->lex_pos = $10;
CG(extra_fn_flags) = $9; }

We could of course not add that implicit return for never. In that case the arrow function would simplify throw if the end of the function is reached without throwing/exiting, just like normal never functions do. I wouldn't classify this as a bug though.

Would this also apply to functions returning void? https://3v4l.org/YkVBg

Yes, theoretically we could do the same here, although it's more controversial. E.g.

// Makes sense, inner function returns `void` (`NULL`), outer function returns `void` too
fn (): void => voidFn()
// Makes sense, inner function might have desired side effects but result is not needed
fn (): void => nonVoidFn()
// Not useful, any side-effect free expression that has no purpose is discarded
fn (): void => 42

That said, this change would at least require a mail to the mailing list and likely an RFC as historically there have been quite a few disagreements with how void/never should behave.

@orklah
Copy link

orklah commented Jan 7, 2022

The void case is definitely weird indeed. There is no way to generate a "void" value without a return; or ending a function without statement. Both of those solutions can't be used here because the return is implicit and an arrow function must have a statement. This is the kind of situation where a null pseudo type would be useful though...

@iluuu1994
Copy link
Member

iluuu1994 commented Jan 7, 2022

There is no way to generate a "void" value without a return; or ending a function without statement.

The absence of a return also generates a "void" value. A normal arrow function essentially gets rewritten to this:

fn (): void => voidFn();
function (): void {
    return voidFn();
}

It should be safe to drop the implicit return here.

fn (): void => voidFn();
function (): void {
    voidFn();
}

The controversial part comes from the fact that now this would also be valid code:

fn (): void => 42;
function (): void {
    42;
}

42 is a useless, but valid expression statement.

@orklah
Copy link

orklah commented Jan 7, 2022

But dropping the implicit return implies that it would need to be added by users, no? Otherwise PHP would need looking at the return type in signature to decide if the statement value must be returned or not? It seems icky

@iluuu1994
Copy link
Member

iluuu1994 commented Jan 7, 2022

Otherwise PHP would need looking at the return type in signature to decide if the statement value must be returned or not? It seems icky

Yes, PHP could add the return depending on whether the function is void/never or not. FWIW other languages do the same. E.g. for Swift this was a problem in the past but they now just drop the value when the closure is explicitly marked as returning ().

Example:

func returnsString() -> String {
    return "Foo"
}

let closure1 = { () -> String in returnsString() }
print(closure1()) // Foo

let closure2 = { () -> () in returnsString() }
print(closure2()) // ()

http://online.swiftplayground.run/

@ricardoboss
Copy link

Yes, PHP could add the return depending on whether the function is void/never or not.

This should only be necessary if the arrow function body solely contains a method/function call, doesn't it? So a literal value would always require a return statement and would (rightly so) cause a fatal error given a different return type.

@iluuu1994
Copy link
Member

Other expressions can also have side effects. E.g.

fn (): void => match ($foo) {
    'foo' => bar(),
}

I wouldn't complicate the matter even more and just do everything or nothing.

@nikic nikic added Feature and removed Bug labels Jan 7, 2022
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Jul 22, 2022
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Jul 22, 2022
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants