Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:113912
MIME-Version: 1.0
References: <CAKuGYLTStzEuOmhGKDdNdWP2iX_--mqV17jub0LgVPs+O6Bd+g@mail.gmail.com>
 <CAEPJdu=7qoj4ryx3PELYAbBsPFoBOiWZow+AMiw=QpNsRUa6Yw@mail.gmail.com> <7399011c-a638-5363-5303-c01ac402bb92@gmx.net>
In-Reply-To: <7399011c-a638-5363-5303-c01ac402bb92@gmx.net>
Date: Thu, 1 Apr 2021 17:07:15 +0200
Message-ID: <CAEPJdundKhmev4vOr8rnEm6MoVbSdaggHpQAdd=4X0++eqqx7g@mail.gmail.com>
To: Andreas Leathley <a.leathley@gmx.net>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000da98b005beea962f"
Subject: Re: [PHP-DEV] [VOTE] noreturn type
From: kontakt@beberlei.de (Benjamin Eberlei)

--000000000000da98b005beea962f
Content-Type: text/plain; charset="UTF-8"

On Thu, Apr 1, 2021 at 3:56 PM Andreas Leathley <a.leathley@gmx.net> wrote:

> On 01.04.21 10:56, Benjamin Eberlei wrote:
> > This RFC is using the declaration of the return type, to specify that it
> > never returns. As such noreturn or never is a keyword a long the lines of
> > public or final, but it is not a return type.
> >
> > I don't think the argument for potential optimizations in Opcache to
> > eliminate dead code or allow IDEs to hint at dead code are valuable
> enough
> > to justify this change.
>
> I already use @return no-return (supported by Psalm/PHPStan) in my code
> now, to clarify the code flow, and for me it fits perfectly for return
> types, as not returning is also a result of the function/method. Having
> a return type of "int" or "string" (or even "void") seems misleading
> when the method will never return anyway.
>
> noreturn/never might not be useful in all code, but for some parts like
> in a controller of a web application it makes handling redirects,
> authorization or 404 errors much easier, clearer and less error-prone,
> for example:
>
> ```php
> if (!isset($user)) {
>    $this->notFound(); // this is a noreturn/never method
> }
>
> if (!$this->isGranted('adminPrivileges')) {
>    $this->notAuthorized(); // this is a noreturn/never method
> }
> ```
>
> Adding noreturn/never on a language level would make sure calling
> "$this->notAuthorized();" never continues code flow. This is often also
> a security issue, as seen by an alternative way of handling the above
> use cases:
>
> ```php
> if (!isset($user)) {
>    $this->notFound();
>    return;
> }
>
> if (!$this->isGranted('adminPrivileges')) {
>    $this->notAuthorized();
>    return;
> }
> ```
>
> If you forget a return, suddenly you have issues. So even though
> noreturn/never is a niche language feature not everyone will use, it
> does address a very real use case.


I don't know, you are arguing that you forgot a return, but you also did
not forget to add a never return type. You
could easily fix this by inlining the exception instead.

```php
if (!isset($user)) {
   throw new NotFoundException();
}
```
Even when you insist on the function, it is a one liner that throws
immediately. Adding the never return type there isn't going to catch a
great many bugs.



> Using attributes has the disadvantage
> of not making the intent clear:
>

> ```php
> #[\NoReturn]
> function notAuthorized(): string
> {
>    // Some code
> }
> ```
>
> Now you have a return type of string, but also the information that the
> function will never return anyway. That is confusing, but will be
> inevitable in some cases when implementing interfaces or an abstract
> class. In my opinion, the string return type is wrong in this case and
> can only mislead a developer.
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>

--000000000000da98b005beea962f--