Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114098
MIME-Version: 1.0
References: <CAH5C8xX87sVh5NDuQ1EfeEbSxdHawO5yNfk5zHEToAqYGJa7=Q@mail.gmail.com>
 <CADyq6sJ5StVLhgEW86xXcVeETTBUodXWrFkv0Lpin+9+du_WJg@mail.gmail.com>
 <CAF+90c8BJ5Fm05ON8_1WU6oWrgXKk49ynfO+axLJU8MqLhY_-Q@mail.gmail.com> <CADyq6sJ+0jiGca64wyVqHGxJOxmnCnKc8W5CTmoyFJMAS6kQnA@mail.gmail.com>
In-Reply-To: <CADyq6sJ+0jiGca64wyVqHGxJOxmnCnKc8W5CTmoyFJMAS6kQnA@mail.gmail.com>
Date: Thu, 22 Apr 2021 12:13:31 +0200
Message-ID: <CAF+90c-FRa-iiTz2ji3dKMOuumMrLB=fDxyUfAe1V2jc4p7wwg@mail.gmail.com>
To: Marco Pivetta <ocramius@gmail.com>
Cc: =?UTF-8?B?TcOhdMOpIEtvY3Npcw==?= <kocsismate90@gmail.com>, 
	PHP Internals List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000058953405c08cef25"
Subject: Re: [PHP-DEV] [RFC] [Vote] Adding return types to internal methods
From: nikita.ppv@gmail.com (Nikita Popov)

--00000000000058953405c08cef25
Content-Type: text/plain; charset="UTF-8"

On Thu, Apr 22, 2021 at 11:51 AM Marco Pivetta <ocramius@gmail.com> wrote:

> On Thu, Apr 22, 2021 at 10:37 AM Nikita Popov <nikita.ppv@gmail.com>
> wrote:
>
>>
>> To clarify what you're actually suggesting here: Do you want to always
>> have the type returned from getReturnType()
>>
>
> Correct
>
>
>> and only add an additional bool method that tells you whether that is a
>> tentative or real type, so that most code can just treat them as being the
>> same thing?
>>
>
> No, and also not needed, especially since we're converging towards having
> these types explicitly defined in PHP 9.
>
> The use-cases of reflection **on return types** are generally as follows
> (obviously not exclusive list - it's hard to define "use-cases" in large
> groups, and be precise):
>
>  1. use reflection to generate subtypes (mocks, stubs)
>  2. use reflection to build a dependency graph of calls (dependency
> injection containers)
>  3. use reflection to validate data flows (type-checkers)
>  4. use reflection for documentation (doc generators, type-checkers)
>  5. use reflection for data conversion (serializers, ORMs)
>
> In the scenario when `ReflectionFunction#getReturnType()` reports the new
> types immediately:
>
>  1. is safe even if `ReflectionFunction#getReturnType()` starts reporting
> new type the return type is more restrictive, since variance rules allow
> for subclassing to do this.
>  2. is safe, because if `mixed` (previous inferred return type) already
> fits within a dependency graph, the new more precise type already fits too
>  3. is safe because the **returned** type is still more restrictive than
> `mixed`
>  4. is safe because type-checkers already learned to not trust internal
> php-src declared types (and moved to stubs instead)
>  5. is safe because the types declared by what is **returned** during
> deserialization is stricter than the previous `mixed`, and therefore still
> fits
>
> There will be hiccups, like always, but overall there is little space for
> security issues arising from stricter return type declarations: in fact,
> the pre-PHP-8.1 scenario (no type declarations) is much more dangerous.
>
> In the scenario when `ReflectionFunction#getReturnType()` does **not**
> report the new types, and instead these types are declared on new
> "tentative return type" methods (the RFC at vote):
>
>  1. 2. 3. 5. until tools consider the new methods, a starker migration to
> PHP9 is expected. Tools need to be adjusted to consider the new methods for
> 8.1, and to ignore the new methods for 9.0. This will cause a lot of
> downstream work
>  4. generally does not care about internal reflection API (again, these
> tools learned to hop around it with stubs)
>
> In the scenario when `ReflectionFunction#getReturnType()` does report the
> new types, but we add a new method to state that the return type is
> "tentative" (approach proposed by NikiC):
>
>  1. 2. 3. 5. until tools consider the new methods, a starker migration to
> PHP9 is expected. Tools need to be adjusted to consider the new methods for
> 8.1, and to ignore the new methods for 9.0. This will cause a lot of
> downstream work
>  4. generally does not care about internal reflection API (again, these
> tools learned to hop around it with stubs)
>
> Overall, I see the change of reported `ReflectionMethod#getReturnType()`
> as non-problematic, and tooling around reflection would continue working as
> expected, while adding new API requires:
>
>  * more work to get the tooling upgraded to PHP 8.1
>  * more work to get the tooling upgraded to PHP 9.0 (when those methods
> become obsolete)
>  * more risk that users of tools that are not yet up-to-date with PHP 8.1
> experience bugs or weird behavior
>
> In fact, tooling will start reporting incompatibilities in types earlier,
> giving people more runway to fix their subtypes for the upcoming 9.0 change.
>

You make a good case! I think my own perception here is colored by the
usage of types in php-src, where the distinction is very important. But
php-src is probably the only type analyzer that works on hard guarantees. I
do want to add one more use case though:

6. Static analyzers, which might be implementing method compatibility
checks (don't know if psalm/phpstan do that), and which would want to
handle these cases differently. In particular, just like PHP itself, the
wouldn't want to indicate an error for the "tentative return type" +
#[ReturnTypeWillChange] combination.

Regards,
Nikita

--00000000000058953405c08cef25--