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

  114092
April 22, 2021 07:42 kocsismate90@gmail.com (=?UTF-8?B?TcOhdMOpIEtvY3Npcw==?=)
Hi Internals,

I've just opened the vote about
https://wiki.php.net/rfc/internal_method_return_types
and I will close it on 2021-05-06.

For prior discussion, please see https://externals.io/message/113413

Regards:
Máté
  114093
April 22, 2021 07:55 ocramius@gmail.com (Marco Pivetta)
Hey Máté,

On Thu, Apr 22, 2021, 09:42 Máté Kocsis <kocsismate90@gmail.com> wrote:

> Hi Internals, > > I've just opened the vote about > https://wiki.php.net/rfc/internal_method_return_types > and I will close it on 2021-05-06. > > For prior discussion, please see https://externals.io/message/113413
Overall OK with the RFC, but we do such a.fuss about not breaking compat on inheritance for then breaking it in reflection, where we add two methods that completely shake down the tree. Therefore I voted NO, especially considering the implications this has on libraries like BetterReflection: the return type is there and it is clear, it just hasn't been explicitly declared by the engine, but there isn't a need for dropping it from reflection. 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.) I would vote yes on the RFC if this section were omitted, and reflection started to report the new types in 8.1 (or another 8.x).
  114096
April 22, 2021 08:37 nikita.ppv@gmail.com (Nikita Popov)
On Thu, Apr 22, 2021 at 9:56 AM Marco Pivetta <ocramius@gmail.com> wrote:

> Hey Máté, > > On Thu, Apr 22, 2021, 09:42 Máté Kocsis <kocsismate90@gmail.com> wrote: > > > Hi Internals, > > > > I've just opened the vote about > > https://wiki.php.net/rfc/internal_method_return_types > > and I will close it on 2021-05-06. > > > > For prior discussion, please see https://externals.io/message/113413 > > > Overall OK with the RFC, but we do such a.fuss about not breaking compat on > inheritance for then breaking it in reflection, where we add two methods > that completely shake down the tree. > > Therefore I voted NO, especially considering the implications this has on > libraries like BetterReflection: the return type is there and it is clear, > it just hasn't been explicitly declared by the engine, but there isn't a > need for dropping it from reflection. > > 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.) > > I would vote yes on the RFC if this section were omitted, and reflection > started to report the new types in 8.1 (or another 8.x). >
It seems pretty important to me that there is a distinction between these -- apart from one measly deprecation warning (that can be ignored), these return types effectively do not exist. You can't treat them the same way. If you have a (non-final) method with a tentative return type, then you don't have a guarantee that it actually returns a value of that type. To clarify what you're actually suggesting here: Do you want to always have the type returned from getReturnType() 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? Regards, Nikita
  114097
April 22, 2021 09:51 ocramius@gmail.com (Marco Pivetta)
On Thu, Apr 22, 2021 at 10:37 AM Nikita Popov 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. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/
  114098
April 22, 2021 10:13 nikita.ppv@gmail.com (Nikita Popov)
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 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
  114099
April 22, 2021 10:15 kocsismate90@gmail.com (=?UTF-8?B?TcOhdMOpIEtvY3Npcw==?=)
> > 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: >
I do think it would be problematic, and a new API is a must (either the one Nikita asked about or the current one). Let's consider a reflection-based static analyser: in PHP 8.1, it would report the incompatible user-land return types as a very serious issue all of a sudden, while in fact it's not the case yet. (now I see that Nikita answered the same) Máté
>
  114100
April 22, 2021 10:37 ocramius@gmail.com (Marco Pivetta)
Hey Máté, NikiC,

On Thu, Apr 22, 2021 at 12:15 PM Máté Kocsis <kocsismate90@gmail.com> wrote:

> 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: >> > > I do think it would be problematic, and a new API is a must (either the > one Nikita asked about or the current one). Let's consider a > reflection-based static analyser: in PHP 8.1, it would report the > incompatible > user-land return types as a very serious issue all of a sudden, while in > fact it's not the case yet. > > (now I see that Nikita answered the same) > > Máté >
No, that's not serious: type-checkers operate pre-runtime, so it is good that they proactively report a variance issue when a subtype of an interface is not restrictive on a return types. Compared to the effort to be added to support the new behavior **at runtime**, this is a non-problem. Current static analyzers already report issues like these, and already rely on `ReflectionMethod#isInternal()` to determine if they should care or not about a missing type declaration (hence no need for a new API). Examples: * https://phpstan.org/r/530583f0-fab5-4f46-af6b-332e67b862cf * https://psalm.dev/r/7f987b25a3 In fact, a lot of tooling treats `Reflection*#isInternal()` as a huge red flag that means "here be dragons", which is more than sufficient to move ahead :-) Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/
  114132
April 24, 2021 17:24 tysonandre775@hotmail.com (tyson andre)
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
  114135
April 24, 2021 19:08 ocramius@gmail.com (Marco Pivetta)
Hey Tyson,

On Sat, Apr 24, 2021, 19:24 tyson andre <tysonandre775@hotmail.com> wrote:

> 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. 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?
  114140
April 24, 2021 20:41 tysonandre775@hotmail.com (tyson andre)
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
  114279
May 6, 2021 07:45 kocsismate90@gmail.com (=?UTF-8?B?TcOhdMOpIEtvY3Npcw==?=)
Hi Internals,

I've just closed the vote, and the RFC has been accepted with 17 votes in
favor and 7 against.

Regards:
Máté
  114712
June 3, 2021 14:54 nikita.ppv@gmail.com (Nikita Popov)
On Thu, Apr 22, 2021 at 9:42 AM Máté Kocsis <kocsismate90@gmail.com> wrote:

> Hi Internals, > > I've just opened the vote about > https://wiki.php.net/rfc/internal_method_return_types > and I will close it on 2021-05-06. > > For prior discussion, please see https://externals.io/message/113413 > > Regards: > Máté >
One implication that I didn't fully appreciate is that this also applies to interfaces. The migration to tentative return types hasn't finished yet, but we already have interface JsonSerializable { function jsonSerialize(): mixed; } Which requires implementers of that interface to also specify "mixed" or a subtype of "array" in the implementation. I expect that there will also be at least interface Countable { function count(): int; } This feels a bit different than the class case, in that implementing internal interfaces is common, while extending internal classes is mostly an artifact of us not adding enough "final"s in the early days. I'm okay with this, but wanted to point it out. Regards, Nikita
  114727
June 4, 2021 09:07 kocsismate90@gmail.com (=?UTF-8?B?TcOhdMOpIEtvY3Npcw==?=)
> This feels a bit different than the class case, in that implementing > internal interfaces is common, while extending internal classes is mostly > an artifact of us not adding enough "final"s in the early days. >
I have to admit that this aspect of the feature wasn't accurately covered by RFC, sorry for that. And I agree that interface changes will imply a bit more effort to migrate; however, I believe this is for the good. For example, until a few weeks ago, probably only very few people knew what a custom SessionHandlerInterface::read() implementation should really return in case of an error: neither our stubs, nor the manual had it correctly. I "accidentally" learned about the issue (that false should be returned in case of an error) because a test failed when I was implementing this RFC. Afterwards, I fixed the return type in the stubs as well as in the documentation: https://github.com/php/php-src/pull/6884/files#diff-c6492b1d4bee1c7c3eca01e4b30af39a438ba823efa220fe1d0e5868bd58586eR74 With all that said, I am ok to have some exceptions if it turns out that a return type declaration is too disruptive for the ecosystem, but I'm hopeful that the new return types - even for interfaces - will be beneficial overall. Regards, Máté