On Thu, Apr 22, 2021 at 11:51 AM Marco Pivetta <ocram...@gmail.com> wrote:
> 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. > 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
