[RFC] [Vote] Adding return types to internal methods View as Markdown

On Thu, Apr 22, 2021 at 10:37 AM Nikita Popov nikita.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
2. 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
2. 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

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é

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