[RFC] [Discussion] Remove inappropriate inheritance signature checkson private methods

  110251
May 22, 2020 15:43 mail@pmmaga.net (=?UTF-8?Q?Pedro_Magalh=C3=A3es?=)
Hi internals,

I want to put up for discussion an RFC (
https://wiki.php.net/rfc/inheritance_private_methods) that proposes to
remove some inappropriate signature checks that are still done on private
methods. Namely, those checks are:

- When a method has the same name as a parent's final private method
- When a method has the same name as a parent's static private method and
the child's method is non-static, or vice-versa
- When a method has the same name as a parent's concrete private method and
the child's method is abstract

I have 2 open issues on the RFC that I would like to hear some opinions on.
- Whether or not to issue a compiler warning whenever "final private
function" is used, to alert the user that it will not achieve what that
construct is currently used for. The disadvantage of introducing it is the
BC break.
- Whether or not to make an exception to this rule for magic methods. Given
that this is widely to restrict object instantiation and cloning, it could
make sense to still allow the use on those cases. However, I think that the
similar effect that can be achieved with "final protected function" would
cover most of the cases. And if we open up that exception for magic
methods, for the sake of clarity maybe we should just keep the "final
private" behavior on all methods and just change the static and the
abstract behaviors. Some discussion on this subject can be found on the PR (
https://github.com/php/php-src/pull/5401) for this RFC.

Regards,
Pedro Magalhães
  110255
May 22, 2020 16:25 ocramius@gmail.com (Marco Pivetta)
Hey Pedro,

On Fri, May 22, 2020 at 5:43 PM Pedro Magalhães <mail@pmmaga.net> wrote:

> Hi internals, > > I want to put up for discussion an RFC ( > https://wiki.php.net/rfc/inheritance_private_methods) that proposes to > remove some inappropriate signature checks that are still done on private > methods. Namely, those checks are: > > - When a method has the same name as a parent's final private method > - When a method has the same name as a parent's static private method and > the child's method is non-static, or vice-versa > - When a method has the same name as a parent's concrete private method and > the child's method is abstract > > I have 2 open issues on the RFC that I would like to hear some opinions on. > - Whether or not to issue a compiler warning whenever "final private > function" is used, to alert the user that it will not achieve what that > construct is currently used for. The disadvantage of introducing it is the > BC break. > - Whether or not to make an exception to this rule for magic methods. Given > that this is widely to restrict object instantiation and cloning, it could > make sense to still allow the use on those cases. However, I think that the > similar effect that can be achieved with "final protected function" would > cover most of the cases. And if we open up that exception for magic > methods, for the sake of clarity maybe we should just keep the "final > private" behavior on all methods and just change the static and the > abstract behaviors. Some discussion on this subject can be found on the PR > ( > https://github.com/php/php-src/pull/5401) for this RFC. >
Overall, this RFC breaks some design capabilities that are within the language, specifically around `__`-prefixed methods in the language. For instance, I design (on purpose) sealed types as following: ```php abstract class Email { final private function __construct() {} public static function business(): Business { return new Business(); } public static function personal(): Personal { return new Personal(); } } final class Business extends Email {} final class Personal extends Email {} ``` The above approach guarantees that no further subtypes exist for `Email`, other than `Business` or `Personal`, effectively forming a safe union type, which can only be broken by reflection. In addition to that, I often prevent serialization of types that are not intended to be serialized (when not final): ```php abstract class InMemorySecret { final private function __sleep() {} } ``` Effectively, the `final` modifier applies to `private` symbols too, and prevents child classes from deviating from imposed design constraints. It is **intentional** for `private` symbol overrides to not compile in these cases. Both of the above examples only apply to special/magic methods: I can see and understand that for custom methods this RFC may be valid, and adding a further refinement to lift the restriction only on custom methods is a good idea. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/
  110261
May 22, 2020 20:06 weirdan@gmail.com (Bruce Weirdan)
On Fri, May 22, 2020 at 7:26 PM Marco Pivetta <ocramius@gmail.com> wrote:

> Overall, this RFC breaks some design capabilities that are within the > language, specifically around `__`-prefixed methods in the language.
Wouldn't your use cases be covered by `protected final` though, as proposed by Pedro? -- Best regards, Bruce Weirdan mailto: weirdan@gmail.com
  110262
May 22, 2020 20:18 ocramius@gmail.com (Marco Pivetta)
Hey Bruce,


On Fri, May 22, 2020, 22:07 Bruce Weirdan <weirdan@gmail.com> wrote:

> > On Fri, May 22, 2020 at 7:26 PM Marco Pivetta <ocramius@gmail.com> wrote: > >> Overall, this RFC breaks some design capabilities that are within the >> language, specifically around `__`-prefixed methods in the language. > > > Wouldn't your use cases be covered by `protected final` though, as > proposed by Pedro? >
This design is made appositely to prevent the child class from being able to call the constructor: the only viable constructors are the named constructors on the supertype
>
  110279
May 26, 2020 14:34 mail@pmmaga.net (=?UTF-8?Q?Pedro_Magalh=C3=A3es?=)
Hi Marco,

Thanks for the feedback.

About the sealed type example, it is true that `final protected` wouldn't
achieve the same thing. But as an attempt to provide an alternative design,
wouldn't an union type of the desired children be a better choice?
I can see some usefulness in it but IMHO, it is supported by a broken
behavior, it doesn't make sense to talk about private symbol overrides.
That shouldn't be a thing.
With that said, I don't think the RFC has to be all or nothing, but we
would leave an exception behind.

About lifting the restriction only to magic methods, I don't really like
that approach. Anything that makes them more special feels like a step in
the wrong direction. If we were to lift that restriction, I would say that
it should be lifted for any method, leaving the RFC to deal only with the
static and abstract cases.

About the serialization example, I think that goal is equally possible to
achieve with final protected.

Regards,
Pedro

On Fri, May 22, 2020 at 5:25 PM Marco Pivetta <ocramius@gmail.com> wrote:

> Hey Pedro, > > On Fri, May 22, 2020 at 5:43 PM Pedro Magalhães <mail@pmmaga.net> wrote: > >> Hi internals, >> >> I want to put up for discussion an RFC ( >> https://wiki.php.net/rfc/inheritance_private_methods) that proposes to >> remove some inappropriate signature checks that are still done on private >> methods. Namely, those checks are: >> >> - When a method has the same name as a parent's final private method >> - When a method has the same name as a parent's static private method and >> the child's method is non-static, or vice-versa >> - When a method has the same name as a parent's concrete private method >> and >> the child's method is abstract >> >> I have 2 open issues on the RFC that I would like to hear some opinions >> on. >> - Whether or not to issue a compiler warning whenever "final private >> function" is used, to alert the user that it will not achieve what that >> construct is currently used for. The disadvantage of introducing it is the >> BC break. >> - Whether or not to make an exception to this rule for magic methods. >> Given >> that this is widely to restrict object instantiation and cloning, it could >> make sense to still allow the use on those cases. However, I think that >> the >> similar effect that can be achieved with "final protected function" would >> cover most of the cases. And if we open up that exception for magic >> methods, for the sake of clarity maybe we should just keep the "final >> private" behavior on all methods and just change the static and the >> abstract behaviors. Some discussion on this subject can be found on the >> PR ( >> https://github.com/php/php-src/pull/5401) for this RFC. >> > > Overall, this RFC breaks some design capabilities that are within the > language, specifically around `__`-prefixed methods in the language. > > For instance, I design (on purpose) sealed types as following: > > ```php > abstract class Email > { > final private function __construct() {} > public static function business(): Business { > return new Business(); > } > public static function personal(): Personal { > return new Personal(); > } > } > > final class Business extends Email {} > final class Personal extends Email {} > ``` > > The above approach guarantees that no further subtypes exist for `Email`, > other than `Business` or `Personal`, effectively forming a safe union type, > which can only be broken by reflection. > > In addition to that, I often prevent serialization of types that are not > intended to be serialized (when not final): > > ```php > abstract class InMemorySecret > { > final private function __sleep() {} > } > ``` > > Effectively, the `final` modifier applies to `private` symbols too, and > prevents child classes from deviating from imposed design constraints. > > It is **intentional** for `private` symbol overrides to not compile in > these cases. > > Both of the above examples only apply to special/magic methods: I can see > and understand that for custom methods this RFC may be valid, and adding a > further refinement to lift the restriction only on custom methods is a good > idea. > > Marco Pivetta > > http://twitter.com/Ocramius > > http://ocramius.github.com/ > >
  110280
May 26, 2020 14:46 ocramius@gmail.com (Marco Pivetta)
Hey Pedro,

On Tue, May 26, 2020, 16:34 Pedro Magalhães <mail@pmmaga.net> wrote:

> Hi Marco, > > Thanks for the feedback. > > About the sealed type example, it is true that `final protected` wouldn't > achieve the same thing. But as an attempt to provide an alternative design, > wouldn't an union type of the desired children be a better choice? >
Difficult without having type aliases for union types: possible with tools like psalm, but fragile and easy to work around for less experienced devs. The construct in my example is instead very hard to avoid unless you know about `ReflectionClass#newInstanceWithoutConstructor()` it is supported by a broken behavior
>
Not really: the `__`-prefixed methods are indirectly part of the object's API, and I seal them off by design, guiding consumers of my implementation & type. About lifting the restriction only to magic methods, I don't really like
> that approach. Anything that makes them more special feels like a step in > the wrong direction. If we were to lift that restriction, I would say that > it should be lifted for any method, leaving the RFC to deal only with the > static and abstract cases. >
Magic methods are indeed a pain in the language: I'm trying to make the best use for them (there's already enough bad usage out there). About the serialization example, I think that goal is equally possible to
> achieve with final protected. >
Good point! Considering that, as far as I know, only the constructor remains "special".
>
  110283
May 26, 2020 23:07 mail@pmmaga.net (=?UTF-8?Q?Pedro_Magalh=C3=A3es?=)
On Tue, May 26, 2020 at 3:46 PM Marco Pivetta <ocramius@gmail.com> wrote:

> Considering that, as far as I know, only the constructor remains "special". >
Leaving the special case only for constructors instead of all magic methods sounds better to me. Right now, that sounds like a winning compromise for the RFC. It is still an exception to a rule, but one that can be better justified. Thanks, Pedro
  110284
May 27, 2020 07:23 drealecs@gmail.com (=?UTF-8?Q?Alexandru_P=C4=83tr=C4=83nescu?=)
On Wed, May 27, 2020 at 2:08 AM Pedro Magalhães <mail@pmmaga.net> wrote:

> On Tue, May 26, 2020 at 3:46 PM Marco Pivetta <ocramius@gmail.com> wrote: > > > Considering that, as far as I know, only the constructor remains > "special". > > > > Leaving the special case only for constructors instead of all magic methods > sounds better to me. Right now, that sounds like a winning compromise for > the RFC. It is still an exception to a rule, but one that can be better > justified. > > Thanks, > Pedro >
Hi, I've been following this and I agree that constructor can be an exception for now. Anyway, constructor is special, considering inheritance. For example, the declaration signature is not required to be compatible when overridden. You might say that constructors are redeclared instead of overridden, shadowing the parent constructor and final would disallow redeclaration. In terms of what final does to a private method, as I understand, it will just be ignored everywhere? (except for constructor) I mean, final deny overriding but private methods cannot be overridden by design. We should have that in the documentation as well, I guess. Alex
  110285
May 27, 2020 08:40 mail@pmmaga.net (=?UTF-8?Q?Pedro_Magalh=C3=A3es?=)
On Wed, May 27, 2020 at 8:23 AM Alexandru Pătrănescu <drealecs@gmail.com>
wrote:

> In terms of what final does to a private method, as I understand, it will > just be ignored everywhere? (except for constructor) > I mean, final deny overriding but private methods cannot be overridden by > design. We should have that in the documentation as well, I guess. >
Hi Alex. Indeed, it would just be ignored. Optionally, as the other open issue of the RFC currently proposes, we would throw a warning on those cases precisely to warn the user that it is meaningless. And yes, I agree that the docs should be updated to explain that explicitly.. Regards, Pedro