Re: [RFC] [Vote] Adding return types to internal methods

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. > > From experience, code generated with tooling while running on newer PHP versions is already > incompatible with older PHP versions: you re-generate the code when changing any of the dependencies > anyway (think "no ABI compatibility"). > > This is at least true for all codegen tools I worked/contributed to/used on so far. Mocking libraries and static analyzers that don't result in published code were my largest concern, generated code that gets published was a smaller one. Changing getReturnType would significantly increase the scope of that incompatibility earlier on for users that don't install multiple php versions (users/maintainers may default to whatever is provided by their package manager for convenience) I'd rather have a larger time window with deprecations to change those and have any potentially breaking changes (from the perspective of users of older versions of code generation tools, test libraries, static analyzers) in 9.0 instead of 8.1, to put the (small) BC breaks in major releases where possible. The introduction of many mixed tentative types which makes sense from a type system perspective, but with your alternate proposal for changing getReturnType(), but would result in code generating tools generating a lot of : mixed return types (requiring php 8.0+ runtime) in various interfaces and classes which would be incompatible with a missing return type override due to https://wiki.php.net/rfc/mixed_type_v2#explicit_returns > We're mostly breaking BC (new methods on reflection symbols, requiring special treatment) > for stuff that is really an edge case that is only affecting tooling that would really work just > fine even if the reflection API started to report the real return types now (no API change > whatsoever). > > What's the plan for PHP 9 about these methods? Deprecation/removal? Or are we adding > something that we'll have to drag on forever? The RFC proposal https://wiki.php.net/rfc/internal_method_return_types stated those plans. Unless new information comes up in the case of specific methods such as breaking commonly used frameworks, in almost all cases, I'd assume tentative types in php 8.x would become real types in the next major version (php 9.0). > Non-final internal method return types - when possible - are declared tentatively in PHP 8.1, > **and they will become enforced in PHP 9.0.** It means that in PHP 8.x versions, > a “deprecated” notice is raised during inheritance checks when an internal method > is overridden in a way that the return types are incompatible, > **and PHP 9.0 will make these a fatal error.** A few examples: Tentative return types would also be used by PECLs, so the getTentativeReturnType would continue to be used forever. (I'd expect PECLs would generally add tentative types in n.x.y and change the real type in (n+1).0.0) The alternate design you've proposed of changing getReturnType seems to have issues - For user-defined types (if we allow an annotation mentioning a tentative return type exists without indicating the type), it'd be possible for hasTenativeReturnType to be true but getReturnType to be null, which is the opposite of internal classes - As I'd mentioned before, if return type functionality gets extended to also work on functions that already have return types (user-defined and/or internal), in which case php would need to add ReflectionFunctionAbstract->getRealReturnType, but I'd rather keep the current semantics of getReturnType. I personally expect your alternate proposal to be more controversial due to the larger potential bc break and barriers to upgrading in a minor release rather than a major release, but may be mistaken. Thanks, - Tyson