On Thu, Apr 22, 2021 at 10:37 AM Nikita Popov <nikita....@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. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/
