Adding return types to internal methods (PHP 8)

  106539
August 11, 2019 08:43 nikita.ppv@gmail.com (Nikita Popov)
Hi internals,

Something that came up in the arginfo thread: We can now add type
annotations for everything, apart from return types on methods of non-final
classes.

The reason is that adding these return types would require inheriting
classes to specify them as well. The same problem does not exist for
argument types thanks to https://wiki.php.net/rfc/parameter-no-type-variance
(which is exactly why we wanted that RFC).

For example, if we add a "string" return type to DateTimeZone::getName(),
then any userland child of DateTimeZone would also have to specify a string
return type.

The good news: Userland classes can already specify those types *now*,
because adding a return type in a child class is legal. This means that if
we add those return types, userland extensions do not have to bump their
minimum requirement to PHP 8 when adding the return type: They can still be
compatible all the way down to PHP 7 (or 7.1, depending on the type).

What do you think about this? As we are currently annotating everything
with types, and we're at a major version, this would be the ideal time to
make this change. But there's certainly a BC break here. (And, for the
record, this is not the type of BC break where P++ or editions help,
without creating a larger mess.)

Regards,
Nikita
  106542
August 11, 2019 12:37 pollita@php.net (Sara Golemon)
On Sun, Aug 11, 2019 at 3:44 AM Nikita Popov ppv@gmail.com> wrote:

> What do you think about this? As we are currently annotating everything > with types, and we're at a major version, this would be the ideal time to > make this change. But there's certainly a BC break here. (And, for the > record, this is not the type of BC break where P++ or editions help, > without creating a larger mess.) > > I think your original proposal was spot on. It added to the language without creating any BC breaks. The ideal outcome.
This modification adds an admittedly small BC break for an equally small benefit. -1 from me. -Sara P.S. - Perhaps a way to the middle might be to introduce implicit return type hints. If a child method is implemented with no return type specified on a parent method which has one, then the parent's type is assumed at bind time, then we provide a better runtime error if that assumption is violated in a strict_types=1 context which is the only place where strict covariance of return types matters to PHP.
  106543
August 11, 2019 12:50 nikita.ppv@gmail.com (Nikita Popov)
On Sun, Aug 11, 2019 at 2:37 PM Sara Golemon <pollita@php.net> wrote:

> On Sun, Aug 11, 2019 at 3:44 AM Nikita Popov ppv@gmail.com> wrote: > >> What do you think about this? As we are currently annotating everything >> with types, and we're at a major version, this would be the ideal time to >> make this change. But there's certainly a BC break here. (And, for the >> record, this is not the type of BC break where P++ or editions help, >> without creating a larger mess.) >> >> I think your original proposal was spot on. It added to the language > without creating any BC breaks. The ideal outcome. > This modification adds an admittedly small BC break for an equally small > benefit. > > -1 from me. > > -Sara > > P.S. - Perhaps a way to the middle might be to introduce implicit return > type hints. If a child method is implemented with no return type specified > on a parent method which has one, then the parent's type is assumed at bind > time, then we provide a better runtime error if that assumption is violated > in a strict_types=1 context which is the only place where strict covariance > of return types matters to PHP. >
This is an interesting idea. There's still a BC break here, but it's more limited and only applies to code that is actively violating type contracts (that were previously implicit). Imho this should not be bound to strict_types though: strict_types controls coercion behavior of scalar type annotations, and nothing else. It should not be overloaded with this additional meaning (and I don't think the BC break is nowhere near large enough to justify this overloading). Do you have this in mind as something for use by internal classes only, or as a general language feature? I can see the use as a general language feature (see for example the debacle where PHPUnit added "void" annotations to some methods). The migration problem of adding return types is certainly not limited to internal classes. On the other hand it does seem somewhat weird that an (inherited) return type will be enforced that was not explicitly given in the source. Regards, Nikita
  106545
August 11, 2019 13:01 pollita@php.net (Sara Golemon)
On Sun, Aug 11, 2019 at 7:51 AM Nikita Popov ppv@gmail.com> wrote:

> On Sun, Aug 11, 2019 at 2:37 PM Sara Golemon <pollita@php.net> wrote: > > P.S. - Perhaps a way to the middle might be to introduce implicit return > > type hints. If a child method is implemented with no return type > specified > > on a parent method which has one, then the parent's type is assumed at > bind > > time, then we provide a better runtime error if that assumption is > violated > > in a strict_types=1 context which is the only place where strict > covariance > > of return types matters to PHP. > > > > This is an interesting idea. There's still a BC break here, but it's more > limited and only applies to code that is actively violating type contracts > (that were previously implicit). Imho this should not be bound to > strict_types though: strict_types controls coercion behavior of scalar type > annotations, and nothing else. It should not be overloaded with this > additional meaning (and I don't think the BC break is nowhere near large > enough to justify this overloading). > > Fair. Just trying to tread as light as possible, but there is a vanishing point.
> Do you have this in mind as something for use by internal classes only, or > as a general language feature? I can see the use as a general language > feature (see for example the debacle where PHPUnit added "void" annotations > to some methods). The migration problem of adding return types is certainly > not limited to internal classes. On the other hand it does seem somewhat > weird that an (inherited) return type will be enforced that was not > explicitly given in the source. > > I agree that it would be as useful to userspace classes as internal ones. It would allow library authors to embrace strong typing without breaking
consumers who are already conforming to docblock types. It's probably worth proof-of-concepting this and making an RFC irrespective of the push to annotate all the things. If feasible and accepted, I'd be much more on board with including return types for internal methods in that plan however. -Sara
  106546
August 11, 2019 13:23 nikita.ppv@gmail.com (Nikita Popov)
On Sun, Aug 11, 2019 at 3:01 PM Sara Golemon <pollita@php.net> wrote:

> On Sun, Aug 11, 2019 at 7:51 AM Nikita Popov ppv@gmail.com> wrote: > >> On Sun, Aug 11, 2019 at 2:37 PM Sara Golemon <pollita@php.net> wrote: >> > P.S. - Perhaps a way to the middle might be to introduce implicit return >> > type hints. If a child method is implemented with no return type >> specified >> > on a parent method which has one, then the parent's type is assumed at >> bind >> > time, then we provide a better runtime error if that assumption is >> violated >> > in a strict_types=1 context which is the only place where strict >> covariance >> > of return types matters to PHP. >> > >> >> This is an interesting idea. There's still a BC break here, but it's more >> limited and only applies to code that is actively violating type contracts >> (that were previously implicit). Imho this should not be bound to >> strict_types though: strict_types controls coercion behavior of scalar >> type >> annotations, and nothing else. It should not be overloaded with this >> additional meaning (and I don't think the BC break is nowhere near large >> enough to justify this overloading). >> >> Fair. Just trying to tread as light as possible, but there is a > vanishing point. > > >> Do you have this in mind as something for use by internal classes only, or >> as a general language feature? I can see the use as a general language >> feature (see for example the debacle where PHPUnit added "void" >> annotations >> to some methods). The migration problem of adding return types is >> certainly >> not limited to internal classes. On the other hand it does seem somewhat >> weird that an (inherited) return type will be enforced that was not >> explicitly given in the source. >> >> I agree that it would be as useful to userspace classes as internal > ones. It would allow library authors to embrace strong typing without > breaking consumers who are already conforming to docblock types. > > It's probably worth proof-of-concepting this and making an RFC > irrespective of the push to annotate all the things. If feasible and > accepted, I'd be much more on board with including return types for > internal methods in that plan however. >
After thinking about this a bit, this is going to be pretty hard to implement. Currently return type checks are performed using a separate opcode, which means that we need to know the return type at compile-time. At the point where inheritance happens and we know the parent return type, we are no longer able to modify the opcodes of the function. We'd have to integrate the return type check into the main return logic, with a possible negative impact on performance... Nikita
  106547
August 11, 2019 14:29 nicolas.grekas+php@gmail.com (Nicolas Grekas)
Le dim. 11 août 2019 à 15:24, Nikita Popov ppv@gmail.com> a écrit :

> On Sun, Aug 11, 2019 at 3:01 PM Sara Golemon <pollita@php.net> wrote: > > > On Sun, Aug 11, 2019 at 7:51 AM Nikita Popov ppv@gmail.com> > wrote: > > > >> On Sun, Aug 11, 2019 at 2:37 PM Sara Golemon <pollita@php.net> wrote: > >> > P.S. - Perhaps a way to the middle might be to introduce implicit > return > >> > type hints. If a child method is implemented with no return type > >> specified > >> > on a parent method which has one, then the parent's type is assumed at > >> bind > >> > time, then we provide a better runtime error if that assumption is > >> violated > >> > in a strict_types=1 context which is the only place where strict > >> covariance > >> > of return types matters to PHP. > >> > > >> > >> This is an interesting idea. There's still a BC break here, but it's > more > >> limited and only applies to code that is actively violating type > contracts > >> (that were previously implicit). Imho this should not be bound to > >> strict_types though: strict_types controls coercion behavior of scalar > >> type > >> annotations, and nothing else. It should not be overloaded with this > >> additional meaning (and I don't think the BC break is nowhere near large > >> enough to justify this overloading). > >> > >> Fair. Just trying to tread as light as possible, but there is a > > vanishing point. > > > > > >> Do you have this in mind as something for use by internal classes only, > or > >> as a general language feature? I can see the use as a general language > >> feature (see for example the debacle where PHPUnit added "void" > >> annotations > >> to some methods). The migration problem of adding return types is > >> certainly > >> not limited to internal classes. On the other hand it does seem somewhat > >> weird that an (inherited) return type will be enforced that was not > >> explicitly given in the source. > >> > >> I agree that it would be as useful to userspace classes as internal > > ones. It would allow library authors to embrace strong typing without > > breaking consumers who are already conforming to docblock types. > > > > It's probably worth proof-of-concepting this and making an RFC > > irrespective of the push to annotate all the things. If feasible and > > accepted, I'd be much more on board with including return types for > > internal methods in that plan however. > > > > After thinking about this a bit, this is going to be pretty hard to > implement. Currently return type checks are performed using a separate > opcode, which means that we need to know the return type at compile-time. > At the point where inheritance happens and we know the parent return type, > we are no longer able to modify the opcodes of the function. We'd have to > integrate the return type check into the main return logic, with a possible > negative impact on performance... >
Coincidentally, we're working on exactly this in SYmfony: we want to add return-type declarations in v5 (due in November) and we want to provide a continuous upgrade path so that apps that are using v4.4 are warned with a deprecation asking them to add the return type. We're doing this using `@return` annotation in the docblocks: when one is found in base Symfony class, child classes that don't declare their return type get the notice. Because we don't want Symfony to trigger the deprecation when extending itself, we silence it when a child class has an explicit `@return` annotation. This is the way to opt-out from the notice, while telling our userland what they should do, before we do. The related PR is here if you want to have a look: https://github.com/symfony/symfony/pull/30323 I don't know if you'll find applicable, but an idea for PHP itself could be that internal classes expose the planned return-type via a docblock, accessible by `ReflectionMethod::getDocComment()`. This way we could build userland tools to leverage it and trigger deprecations. In general on this thread, I agree with Nikita it would be desireable to add return types, and I also agree with Sara that it should not be a hard BC break. I hope this helped a bit :) NIcolas
  106548
August 11, 2019 16:33 pollita@php.net (Sara Golemon)
On Sun, Aug 11, 2019 at 8:24 AM Nikita Popov ppv@gmail.com> wrote:

> After thinking about this a bit, this is going to be pretty hard to > implement. Currently return type checks are performed using a separate > opcode, which means that we need to know the return type at compile-time. > At the point where inheritance happens and we know the parent return type, > we are no longer able to modify the opcodes of the function. We'd have to > integrate the return type check into the main return logic, with a possible > negative impact on performance... > > Well, we certainly can't do it without some perf impact. 100% agree there. The naïve and simple solution is expensive enough that I'd agree
it's not worth doing. I can see a path to get fairly close to zero, but at the cost of a non-trivial amount of complexity. Hrmmm Yeah, ultimately I'm not sure that gives us enough to justify the cost. Okay, forget that tack pending someone being able to come up with an implementation that doesn't suck. -Sara On Sun, Aug 11, 2019 at 8:24 AM Nikita Popov ppv@gmail.com> wrote:
> On Sun, Aug 11, 2019 at 3:01 PM Sara Golemon <pollita@php.net> wrote: > >> On Sun, Aug 11, 2019 at 7:51 AM Nikita Popov ppv@gmail.com> >> wrote: >> >>> On Sun, Aug 11, 2019 at 2:37 PM Sara Golemon <pollita@php.net> wrote: >>> > P.S. - Perhaps a way to the middle might be to introduce implicit >>> return >>> > type hints. If a child method is implemented with no return type >>> specified >>> > on a parent method which has one, then the parent's type is assumed at >>> bind >>> > time, then we provide a better runtime error if that assumption is >>> violated >>> > in a strict_types=1 context which is the only place where strict >>> covariance >>> > of return types matters to PHP. >>> > >>> >>> This is an interesting idea. There's still a BC break here, but it's more >>> limited and only applies to code that is actively violating type >>> contracts >>> (that were previously implicit). Imho this should not be bound to >>> strict_types though: strict_types controls coercion behavior of scalar >>> type >>> annotations, and nothing else. It should not be overloaded with this >>> additional meaning (and I don't think the BC break is nowhere near large >>> enough to justify this overloading). >>> >>> Fair. Just trying to tread as light as possible, but there is a >> vanishing point. >> >> >>> Do you have this in mind as something for use by internal classes only, >>> or >>> as a general language feature? I can see the use as a general language >>> feature (see for example the debacle where PHPUnit added "void" >>> annotations >>> to some methods). The migration problem of adding return types is >>> certainly >>> not limited to internal classes. On the other hand it does seem somewhat >>> weird that an (inherited) return type will be enforced that was not >>> explicitly given in the source. >>> >>> I agree that it would be as useful to userspace classes as internal >> ones. It would allow library authors to embrace strong typing without >> breaking consumers who are already conforming to docblock types. >> >> It's probably worth proof-of-concepting this and making an RFC >> irrespective of the push to annotate all the things. If feasible and >> accepted, I'd be much more on board with including return types for >> internal methods in that plan however. >> > > After thinking about this a bit, this is going to be pretty hard to > implement. Currently return type checks are performed using a separate > opcode, which means that we need to know the return type at compile-time. > At the point where inheritance happens and we know the parent return type, > we are no longer able to modify the opcodes of the function. We'd have to > integrate the return type check into the main return logic, with a possible > negative impact on performance... > > Nikita > >