Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114052
MIME-Version: 1.0
References: <CAH5C8xUqzFJ4V0uo7_FviMq3Sy9Da3MWNMhh+neMO8ZjEnyQWg@mail.gmail.com>
In-Reply-To: <CAH5C8xUqzFJ4V0uo7_FviMq3Sy9Da3MWNMhh+neMO8ZjEnyQWg@mail.gmail.com>
Date: Wed, 14 Apr 2021 15:30:21 +0200
Message-ID: <CAF+90c8s0+GvL378hhxnah315ae69_56LO=pMXnQisA5-E-5Lw@mail.gmail.com>
To: =?UTF-8?B?TcOhdMOpIEtvY3Npcw==?= <kocsismate90@gmail.com>
Cc: PHP Internals List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000008e5ebd05bfeec01a"
Subject: Re: [PHP-DEV] [RFC] [Discussion] Adding return types to internal methods
From: nikita.ppv@gmail.com (Nikita Popov)

--0000000000008e5ebd05bfeec01a
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Mar 6, 2021 at 11:56 PM M=C3=A1t=C3=A9 Kocsis <kocsismate90@gmail.c=
om> wrote:

> 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=C3=A1t=C3=A9
>

Thanks M=C3=A1t=C3=A9 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 =3D null). Once again, direct users of the A=
PI
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=C3=A1t=C3=A9, I don't really look forward to parsing i=
nformation
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

--0000000000008e5ebd05bfeec01a--