Hi Marco Pivetta,
> In fact, if reflection were to switch to the actual runtime return types of
> those methods, I don't see a reason why downstream consumers would break
> (stubbing tools, code generators, type checkers, dependency solvers, etc.)
If the published library/application had to support older versions (e.g. php 7.4),
but the tentative return types contained types/syntaxes that required php 8..0
(e.g. union types such as string|false, new types such as
mixed/never, etc,)
then the code generators and type checkers and stubbing tools would need to be
updated to exclude the new tentative return types much earlier than absolutely needed.
- Users would benefit from code/tooling working the same way in php 7.x and 8.1 when upgrading to
8.1
and making the tooling unexpectedly stricter may be regarded as a breaking change by end users,
especially for unmaintained tools/libraries.
I'd prefer if the tooling authors and end users had to opt in to use the tentative return
types
and upgrade to a version of tooling that was aware of the tentative return types to start using
them.
- Forcing getReturnType to change immediately would be a barrier to upgrading, especially for users
that aren't deeply familiar with php,
if the php version or library versions being used in production weren't compatible with the
latest versions
of those stubbing tools, code generators, type checkers, dependency solvers, etc. that were aware
of tentative return types
(e.g. a test mock no longer returning null)
To upgrade from php 7.3 (or older) to 8.1, a user may want applications and libraries that worked
the same way in both 7.3 and 8.1,
and would only want to upgrade the applications/libraries (and fix the tentative type notices)
after they stopped using php 7
- PHP 8.0 would be only one year older than 8.1 and automatically generating more user-defined
subclasses with union types
this early on (e.g. and publishing to packagist) would be inconvenient for users still on php 7.
Also, as mentioned by Nikita Popov in https://externals.io/message/113413#114052
**Having a distinction between getReturnType and getTentativeReturnType also allows the
functionality in this RFC to be extended in the future,
e.g. from a getReturnType of BaseType to getTentativeReturnType of
SubType, rather than only being useful when return types are missing**
Additionally, I agree with the points made by Nikita/Máté Kocsis - older releases of static
analyzers would treat getReturnType as if it was definitely the real type,
and falsely treat some type checks as **definitely** redundant/impossible rather than **probably**
redundant/impossible,
leading users to remove those checks with insufficient validation/testing/review, before it's
definitely safe/correct to do so.
- third party code in vendor dependencies (and mocks generated for unit tests) are typically not
analyzed for issues,
but third party code might override internal classes and make those seemingly
redundant/impossible checks actually required).
- E.g. if getReturnType were to change instead of adding getTentativeReturnType,
older releases of phan with --redundant-condition-detection would
falsely report that conditions were definitely impossible/redundant when it is still possible for
subclasses to return different types.
(I'm a maintainer of the static analyzer http://github.com/phan/phan/ and would personally prefer the
getTentativeReturnType approach,
Marco Pivetta(Ocramius) works on/contributes to various php projects/analyzers such as
BetterReflection)
Thanks,
-Tyson