Everything

php

[RFC] Adding return types to internal methods (PHP 8)

php.internals

Máté Kocsis

5 years ago

Hi Internals,

Last year, Nikita started a discussion about adding return types
to non-final internal methods: https://externals.io/message/106539 .

I'd like to restart the conversation, since I've just created an
implementation
for the first step of the migration:
https://github.com/php/php-src/pull/6548
(I had to start a new email thread because I don't have the original one).

My implementation currently emits a deprecation notice during inheritance
validation
for each method which omits the return type of its parent. This means
that the parent return type is not yet enforced when the child omits it,
neither during
inheritance, nor at run-time. However, as soon as the child class declares
return types,
everything will behave as usual, so variance and run-time return value
checks
both apply. Finally, in a couple of years, PHP 9.0 could make the
declaration of
return types required.

If there are concerns about methods which are already declared with the
wrong return
type, we could lax the restrictions even further, and only emit a
deprecation notice
instead of a fatal error in this case as well.

I chose the above approach over inheriting the return type implicitly
(which was suggested
by Sara) because I believe it has a few advantages:
- It causes less BC break: methods not declaring a return type will surely
continue to work
as before, and gradual migration it also supported
- It has negligible run-time impact because diagnostics is emitted maximum
once per
any method which misses the return type. If return types were inherited
implicitly,
incorrect return values would potentially trigger lots of errors.
- It is more straightforward behavior than implicit inheritance, no "magic"
is involved

Even though I prefer the current implementation (or its more lax variant),
I'm also not
opposed to going with implicit inheritance if that's what we settle on.

I appreciate any input, especially about the possible impact of the
different approaches.

Regards:
Máté

Nikita Popov

5 years ago

On Mon, Dec 28, 2020 at 7:54 PM Máté Kocsis <kocsismate90@gmail.com> wrote:

> Hi Internals,
>
> Last year, Nikita started a discussion about adding return types
> to non-final internal methods: https://externals.io/message/106539 .
>
> I'd like to restart the conversation, since I've just created an
> implementation
> for the first step of the migration:
> https://github.com/php/php-src/pull/6548
> (I had to start a new email thread because I don't have the original one).
>
> My implementation currently emits a deprecation notice during inheritance
> validation
> for each method which omits the return type of its parent. This means
> that the parent return type is not yet enforced when the child omits it,
> neither during
> inheritance, nor at run-time. However, as soon as the child class declares
> return types,
> everything will behave as usual, so variance and run-time return value
> checks
> both apply. Finally, in a couple of years, PHP 9.0 could make the
> declaration of
> return types required.
>
> If there are concerns about methods which are already declared with the
> wrong return
> type, we could lax the restrictions even further, and only emit a
> deprecation notice
> instead of a fatal error in this case as well.
>
> I chose the above approach over inheriting the return type implicitly
> (which was suggested
> by Sara) because I believe it has a few advantages:
> - It causes less BC break: methods not declaring a return type will surely
> continue to work
> as before, and gradual migration it also supported
> - It has negligible run-time impact because diagnostics is emitted maximum
> once per
> any method which misses the return type. If return types were inherited
> implicitly,
> incorrect return values would potentially trigger lots of errors.
> - It is more straightforward behavior than implicit inheritance, no "magic"
> is involved
>
> Even though I prefer the current implementation (or its more lax variant),
> I'm also not
> opposed to going with implicit inheritance if that's what we settle on.
>
> I appreciate any input, especially about the possible impact of the
> different approaches.
>
> Regards:
> Máté
>

Is there any way we could make this mechanism more broadly available than
just for internal methods? This is a problem that also shows up when adding
return types in userland libraries.

A concern I have is that for methods with union return types, there
wouldn't be any way to avoid a deprecation warning without having PHP 8 as
the minimum version requirement. And given PHP's general error handling
story, deprecation warnings are not exactly graceful...

Regards,
Nikita

Nicolas Grekas

5 years ago

On Mon, Dec 28, 2020 at 7:54 PM Máté Kocsis <kocsismate90@gmail.com> wrote:

>
> > Hi Internals,
> >
> > Last year, Nikita started a discussion about adding return types
> > to non-final internal methods: https://externals.io/message/106539 .
> >
> > I'd like to restart the conversation, since I've just created an
> > implementation
> > for the first step of the migration:
> > https://github.com/php/php-src/pull/6548
> > (I had to start a new email thread because I don't have the original
> one).
> >
> > My implementation currently emits a deprecation notice during inheritance
> > validation
> > for each method which omits the return type of its parent. This means
> > that the parent return type is not yet enforced when the child omits it,
> > neither during
> > inheritance, nor at run-time. However, as soon as the child class
> declares
> > return types,
> > everything will behave as usual, so variance and run-time return value
> > checks
> > both apply. Finally, in a couple of years, PHP 9.0 could make the
> > declaration of
> > return types required.
> >
> > If there are concerns about methods which are already declared with the
> > wrong return
> > type, we could lax the restrictions even further, and only emit a
> > deprecation notice
> > instead of a fatal error in this case as well.
> >
> > I chose the above approach over inheriting the return type implicitly
> > (which was suggested
> > by Sara) because I believe it has a few advantages:
> > - It causes less BC break: methods not declaring a return type will
> surely
> > continue to work
> > as before, and gradual migration it also supported
> > - It has negligible run-time impact because diagnostics is emitted
> maximum
> > once per
> > any method which misses the return type. If return types were inherited
> > implicitly,
> > incorrect return values would potentially trigger lots of errors.
> > - It is more straightforward behavior than implicit inheritance, no
> "magic"
> > is involved
> >
> > Even though I prefer the current implementation (or its more lax
> variant),
> > I'm also not
> > opposed to going with implicit inheritance if that's what we settle on.
> >
> > I appreciate any input, especially about the possible impact of the
> > different approaches.
> >
> > Regards:
> > Máté
> >
>
> Is there any way we could make this mechanism more broadly available than
> just for internal methods? This is a problem that also shows up when adding
> return types in userland libraries.
>
> A concern I have is that for methods with union return types, there
> wouldn't be any way to avoid a deprecation warning without having PHP 8 as
> the minimum version requirement. And given PHP's general error handling
> story, deprecation warnings are not exactly graceful...
>

We have the same issue in Symfony. We have a special class loader that
triggers a deprecation when a parent class has an `@return` annotation but
the child class doesn't have a return type. Also, we don't trigger a
deprecation when the child class also has the `@return` annotation. The
reason for this is to allow child classes to be deprecation-free.
By duplicating the annotation from the parent, child classes tell they know
a return type should be added in their next major, which should happen
before the next major of their parent class.

PHP could adopt the same strategy using an attribute. When a method has
that attribute, it'd mean the same: skip the deprecation regarding it's
parent classes, and trigger a deprecation when child classes either 1.
don't have an explicit or 2. don't have the same annotation.

WDYT?

Máté Kocsis

5 years ago

Hi Nikita and Nicolas,

After a month of pause, I came back to this topic.

A concern I have is that for methods with union return types, there

> wouldn't be any way to avoid a deprecation warning without having PHP 8 as
> the minimum version requirement. And given PHP's general error handling
> story, deprecation warnings are not exactly graceful...
>

Thanks for pointing out this possible consequence. I do agree that it's a
valid concern, so we should try to do something about it. I could come up
with a few possible mitigations so far:
- instead of E_DEPRECATE, we could emit E_STRICT warnings so that people
can separately tackle this class of errors from the rest
- we could add support for a SuppressReturnTypeDeclaration annotation that
could suppress these notices altogether
- we could postpone the tentative declaration of those method return types
which can't be expressed on PHP 7.4 to a later version (e.g. PHP 8.4) when
the 7.x series will have become EOL for a while

Actually, I counted how many of the return types in question can't be
expressed on PHP 7.4. I found ~270 results out of all the ~1440 methods
which don't yet have a return type declared.

Do you think these ideas would sufficiently answer your concerns?

Is there any way we could make this mechanism more broadly available than

> just for internal methods? This is a problem that also shows up when adding
> return types in userland libraries.

Inspired by Symfony's strategy, a #[ReturnTypeDeclaration("int|string",
"Library Foo v2.0 will declare int|string as return type")] attribute could
be added to methods in order to gradually
let any child classes know that a future version of library Foo will
declare the int|string return type for the method in question. Then
overriding methods should either declare some kind of
return type (unfortunately, even a wrong one would suffice, since it would
be cumbersome to store exact type information in an attribute, if I'm not
mistaken) or add the very same
attribute - but at least the first argument should be exactly the same,
from character to character - if people want to avoid the deprecation
notice.

It has just come to my mind that in case of internal functions, we could
combine the annotation based approach described above together with the
behavior I originally proposed:
- if an overriding method declares a return type then the regular
compatibility checks would kick in, and a notice would be emitted in case
of failure
- if an overriding method doesn't have a return type, but
a ReturnTypeDeclaration attribute, then a notice would be emitted if the
attribute doesn't have the exact same type as the
one we added via PHPDoc in the stubs
- otherwise a notice is emitted

To be honest, this migration path seems to be slightly overcomplicated for
me. So personally, I would be just fine with the ideas I collected in the
beginning of my message for reducing impact.
However, if you think that it makes some sense, I'm ok with going with
something along the lines of it.

Regards:
Máté