Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:113416
MIME-Version: 1.0
References: <CAH5C8xUqzFJ4V0uo7_FviMq3Sy9Da3MWNMhh+neMO8ZjEnyQWg@mail.gmail.com>
In-Reply-To: <CAH5C8xUqzFJ4V0uo7_FviMq3Sy9Da3MWNMhh+neMO8ZjEnyQWg@mail.gmail.com>
Date: Mon, 8 Mar 2021 15:58:03 +0100
Message-ID: <CAOWwgpnFPEJDwcfCj=wCm2t9ADk4zrRTxcVfDmaN2MWLomqxaQ@mail.gmail.com>
To: =?UTF-8?B?TcOhdMOpIEtvY3Npcw==?= <kocsismate90@gmail.com>
Cc: PHP Internals List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000d08d6e05bd07a9bd"
Subject: Re: [PHP-DEV] [RFC] [Discussion] Adding return types to internal methods
From: nicolas.grekas+php@gmail.com (Nicolas Grekas)

--000000000000d08d6e05bd07a9bd
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hello Mat=C3=A9,

https://wiki.php.net/rfc/internal_method_return_types) as well as the
> implementation based on the feedback Nikita and Nicolas provided.


Thanks for moving this topic forward, it's very much needed.
I like the word "tentative" you're using to describe the missing return
types.

I would be good with the RFC but I want to give a few different ideas to
challenge it a bit.
Here they are:

   - About using "E_STRICT", I'm not really sold. I think these notices
   should follow the same path as deprecations as far as reporting is
   concerned. By using E_STRICT, many error handlers will throw exceptions,
   while the code is fine really. Deprecations don't lead to exceptions
   usually (at least they never do in any Symfony stack). Semantically, a
   deprecation fits well to me also.
   - Suppressing the notice is very much needed, but I think we can merge
   the corresponding attribute with the TentativeReturnType that you mentio=
n
   in "future scope".
   - As a corollary, we might not need the new methods on ReflectionMethod:
   instead, we might make getAttributes() return a TentativeReturnType
   attribute for internal methods too.

To summarize my proposal, I think we need only one TentativeReturnType
attribute, that we would put on internal methods, and that would also be
allowed on userland methods. ReflectionMethod::gettAttributes() would
report these attributes for both internal and userland methods.

The attribute would need to convey the allowed types. This could be done
with a list of strings. Taking your example from the RFC, this would look
like this:

class MyDateTime extends DateTime{
    /**
     * @return DateTime|false
     */
    #[TentativeReturnType(DateTime::class, 'false')]    public
function modify(string $modifier) { return false; }}

In this example, this would duplicate the docblock, but many docblocks
contain extended return-type info that the engine doesn't understand
anyway (list of types, etc.)

When engine sees these attributes, it would behave as you describe in
the RFC, with one possible extension: it could also validate the
supplied list of types and check LSP on it (when there is a parent
type).

Does this make sense to you?

Nicolas

--000000000000d08d6e05bd07a9bd--