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

Hello Maté,

https://wiki.php.net/rfc/internal_method_return_types) as well as the

implementation based on the feedback Nikita and Nicolas provided.

Thanks for moving this topic forward, it's very much needed.
I like the word "tentative" you're using to describe the missing return
types.

I would be good with the RFC but I want to give a few different ideas to
challenge it a bit.
Here they are:

• About using "E_STRICT", I'm not really sold. I think these notices
  should follow the same path as deprecations as far as reporting is
  concerned. By using E_STRICT, many error handlers will throw exceptions,
  while the code is fine really. Deprecations don't lead to exceptions
  usually (at least they never do in any Symfony stack). Semantically, a
  deprecation fits well to me also.
• Suppressing the notice is very much needed, but I think we can merge
  the corresponding attribute with the TentativeReturnType that you mention
  in "future scope".
• As a corollary, we might not need the new methods on ReflectionMethod:
  instead, we might make getAttributes() return a TentativeReturnType
  attribute for internal methods too.

To summarize my proposal, I think we need only one TentativeReturnType
attribute, that we would put on internal methods, and that would also be
allowed on userland methods. ReflectionMethod::gettAttributes() would
report these attributes for both internal and userland methods.

The attribute would need to convey the allowed types. This could be done
with a list of strings. Taking your example from the RFC, this would look
like this:

class MyDateTime extends DateTime{
/**
* @return DateTime|false
*/
#[TentativeReturnType(DateTime::class, 'false')] public
function modify(string $modifier) { return false; }}

In this example, this would duplicate the docblock, but many docblocks
contain extended return-type info that the engine doesn't understand
anyway (list of types, etc.)

When engine sees these attributes, it would behave as you describe in
the RFC, with one possible extension: it could also validate the
supplied list of types and check LSP on it (when there is a parent
type).

Does this make sense to you?

Nicolas

Le 6 mars 2021 à 23:56, Máté Kocsis kocsismate90@gmail.com a écrit :

Hi Internals,

I've just finished the first iteration of the RFC (
https://wiki.php.net/rfc/internal_method_return_types) as well as the
implementation based on the feedback Nikita and Nicolas provided. Thus, I'd
like to start the discussion phase.

Regards:
Máté

Hi,

Two remarks:

(1) The RFC is about return type of non-final methods. The same issue might arise for typed properties, whose type are supposed to be invariant under inheritance; although it is admittedly rarely useful to redeclare them in the subclass. Should typed properties be handled in the same way, or should we simply recommend to not redeclare properties?

(2) Nicolas Grekas has beaten about E_STRICT (E_DEPRECATED is fine in this case). I’m just adding this: By reintroducing E_STRICT you are effectively reverting the Reclassify E_STRICT notices RFC, whose motivation was to “simplify our error model and resolve the currently unclear role of strict standards notices”. Thus, you should propose a clear role of those resurrected E_STRICT notices, that justifies the complication of the error model.

—Claude

Hi Internals,

I've just finished the first iteration of the RFC (
https://wiki.php.net/rfc/internal_method_return_types) as well as the
implementation based on the feedback Nikita and Nicolas provided. Thus, I'd
like to start the discussion phase.

Regards:
Máté

Thanks Máté for the RFC, and thanks Nicolas for your comments.

I think it's worth noting that this RFC tries to address a specific case of
a more general problem: PHP's strict LSP checks make it hard to change
method signatures, in ways that may be perceived as harmless. I think the
two important ones are:

1. The one addressed by the RFC is addition of a return type (or more
   generally, restriction of a return type). This is generally done when the
   method was already guaranteed to return that type, and this fact was not
   explicitly declared previously. Direct users of the API are not affected by
   the change. Overriding methods can be affected in two ways: They made use
   of the lose API contract and returned a different type, in which case the
   implementation needs to be changed. Or, and this is almost always the case,
   they already adhere to the stricter API contract, but now need to
   explicitly declare this fact.

2. The second one not addressed here is the addition of optional
   parameters. Take for example the recent change of mysqli::execute() to
   mysqli::execute(?array $params = null). Once again, direct users of the API
   are not affected by the change. Overriding methods are affected because
   they need to pass through the new parameter. As this is an addition of a
   new optional feature, not passing through the optional parameter only
   becomes a problem if the new optional feature is actually used. Whether not
   passing it is "harmless" depends on whether the API declarer and the API
   user are the same.

The commonality is that both of these changes have no impact on direct API
consumers, only on inheritors (to give a counter example: Adding a new
required parameter breaks direct consumers). In both cases it is
technically easy to adjust the overriding APIs to comply with the new
signature -- the problem is purely one of backwards compatibility
guarantees provided to downstream consumers.

Having said all that, I don't really see a solution for the general
problem, short of allowing methods to opt-out of LSP checks entirely. I
think doing so would be a very bad idea, because the functionality would
certainly get misused for other purposes. As such, let's get back to the
return types...

I think as far as the original motivation is concerned (adding return types
to internal methods), the RFC as proposed does well. The only thing I would
change is to add the ReflectionMethod::getTentativeReturnType() part
independently of the userland aspect. I believe it's important that this is
exposed in reflection, even if the more general userland functionality is
not provided. For example, if you have a mock generator, you'll probably
want to either add the tentative return type as a real return type to
mocks, or at least automatically add the #[SuppressReturnTypeNotice]
attribute.

I'm more ambivalent about the userland side of this proposal. As Nicolas
has already pointed out, the RFC as proposed requires the library to have a
PHP 8.1 minimum version requirement to make use of this functionality. And
libraries requiring PHP 8.1 are likely not the target audience for this
feature -- at least not when it comes to adding return types at all, there
might still be a use when it comes to changing them in the future, as
typesystem capabilities are expanded.

I don't think it makes sense to introduce any solution in this space that
requires PHP 8.1. If we introduce something for the userland side of
things, then it should be compatible with older versions. Nicolas'
suggestion of declaring the new return type inside the attribute
#[TentativeReturnType("Foo|Bar")] would certainly work, and allow us to
provide the same degree of functionality as the internal variant. That is,
we could perform a variance check against the new tentative signature and
throw a deprecation notice on mismatch.

However, just like Máté, I don't really look forward to parsing information
out of type strings. It's manageable now, but keeping in mind future
extensions of the typesystem, dealing with
#[TentativeReturnType("callable(Foo<T : int|string> &...)")] will be
significantly less pleasant.

That leaves Nicolas' last suggestion of not even trying to specify the type
in a way that is understood by the engine -- just indicate that the type is
going to change, and leave the actual type specification to @return. In
this case the attribute would not have any actual effect from the engine
perspective (apart from the interaction with the internal case). Which begs
the question, why does this need to be part of PHP core at all? Is it just
a matter of standardizing on a particular attribute?

If we go down that route, I would suggest not to call this
#[TentativeReturnType], but something like #[ReturnTypeWillChange]. I also
don't think it's necessary to restrict this to methods that don't currently
declare a return type, as it may be necessary to narrow a declared return
type at a later time (e.g. due to typesystem improvements). For example:

class A {
/** @return int[] */
#[ReturnTypeWillChange]
public function method(): array {}
}

class B extends A {
// Accepted, static analyzers happy.
public function method(): int[] {}
}

class C extends A {
// Accepted, but static analyzers will warn.
public function method(): array {}
}

class D extends A {
// Not accepted, as it violates the proper array type.
public function method() {}
}

So tl;dr I would either go with only the internal part of the proposal, or
follow Nicolas' last suggestion, or some variant thereon. The version
that's currently RFC doesn't seem particularly useful due to the version
requirement.

Regards,
Nikita