Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:111193
MIME-Version: 1.0
References: <CABHuAWx16_6rwye6BC4gUctSEdTTV0bq68S=1znNPykwGzXTsQ@mail.gmail.com>
 <CABHuAWwETC=oJ4TWdkZ8sEW1ZbMp08YAZj3HMhH9UqG0J=eoxw@mail.gmail.com>
In-Reply-To: <CABHuAWwETC=oJ4TWdkZ8sEW1ZbMp08YAZj3HMhH9UqG0J=eoxw@mail.gmail.com>
Date: Sun, 26 Jul 2020 18:42:09 +0200
Message-ID: <CAEPJdunkf985JEvz0zTi9DeY+DJUK2emkG_DEbDx5AfDCypo7g@mail.gmail.com>
To: Chris Riley <t.carnage@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000cb355305ab5ae344"
Subject: Re: [PHP-DEV] Re: [RFC][Proposal] Renamed parameters
From: kontakt@beberlei.de (Benjamin Eberlei)

--000000000000cb355305ab5ae344
Content-Type: text/plain; charset="UTF-8"

On Sun, Jul 26, 2020 at 3:52 PM Chris Riley <t.carnage@gmail.com> wrote:

> Hi all,
>
> Thanks for the feedback so far. In light of the feedback received both here
> and privately, I've made 3 changes to the RFC document:
>
> 1. The original option 1, allowing renaming parameters but not requiring an
> explicit opt in to enable them to be called by name has been dropped. The
> proposal is now only for explicit opt in parameters with renaming as a
> possibility. The reasoning for this, is that although I included option 1
> as I thought it might be more likely to be accepted; many people pointed
> out that we are very close to the cutoff date for PHP 8.0 and that
> implementing such a change would likely be too big to get done in time.
> Option 1 would be possible to include in PHP 8.1 as it doesn't break BC,
> this means that the proposal can be brought back targeting 8.1 should
> option 2 not be accepted
>

Can you clarify if opt-in would only be the behavior on userland functions,
or if all internal functions would also change to opt-in?

If no, then the inconsistency must be explained.
if yes then the next steps for each internal API to decide on allowing
named params or not needs to be explained

Both approaches lead to a lot of problems down the road that were nicely
circumvented by auto-optin of all functions.

Implementation wise, with external name, you have only one name on the call
site. That means if you decide to rename an argument,
then you cannot support both the old and the new name for one major release
cycle to allow your users to upgrade smoothly.
As such it is my opinion that this is an inferior approach to having an
alias that allows declaring a second name.

Have you given an E_NOTICE/E_WARNING a thought? A user of a library could
then overwrite a library authors "with" to disallow named parameters.

Can you add an example how the MessageHandler problem would look like with
your proposal? Is the external name inherited? Is it forced to inherit? Can
it be overwritten?


> 2. With respect to the feature freeze date, I've added a possible strategy
> to deal with a staged implementation, should the release managers not feel
> comfortable including the full feature at this late stage.
>

Can you update the Proposed Voting Choices section with the two questions
for the staged implementation? or the one question combining them both? The
wording is going to be signifcant for further discussion.


> 3. I have documented the main objections to the RFC on the RFC itself and
> included my rebuttals; should anyone feel I've not represented their point
> fairly let me know and I'll update.
>

Your "objections" section mentions the inheritance / polymorphism part
being buried/hidden in the RFC, but its a section on its own
https://wiki.php.net/rfc/named_params#parameter_name_changes_during_inheritance
and Nikita sent several mails to the list about this topic, asking for
specific feedback about this outstanding issue alone.

The main change in your RFC from explicitly enabled for all userland and
internal functions to an opt-in approach for userland functions was also
discussed in depth.

Similar approaches to your suggestions were also mentioned again in the RFC
prominently under "Alternatives", overruled by the voted upon
implementation that got 76% acceptance.


>
> Regards,
> Chris
>

At this point I would much prefer to discuss amendments within the current
behavior and not against it.

My counter-proposal is still to have two, potentially three attributes:

- @@NameAlias to define a second name/alias for a parameter. Would help
solve both polymorphic name changes and refactoring of names.
- @@PositionalArgumentsOnly to define on a function or class, throwing an
exception if used with named arguments. This only slightly makes the use
case of named arguments slower.

Maybe the reverse with @@NamedArgumentsOnly - however checking for that
would probably slightly make all positional calls slower, which will
probably stay the primary way of calling functions. Trade offs are unclear
here.

I know attributes "feel" wrong here, being a new feature and not been used
for anything yet in the language. But attributes have the benefit of not
introducing new keywords or syntax to the language and are the right tool
to "re-configure" a feature from its primary behavior. Use of attributes
would also keep open future changes (by introducing new attributes or
arguments to the existing ones) leaving our options open instead of locking
them down further.


> On Fri, 24 Jul 2020 at 12:12, Chris Riley <t.carnage@gmail.com> wrote:
>
> > Hi all,
> >
> > The named parameters RFC has been accepted, despite significant
> objections
> > from maintainers of larger OSS projects due to the overhead it adds to
> > maintaining backwards compatibility as it has now made method/function
> > parameter names part of the API; a change to them would cause a BC break
> > for any library users who decide to use the new feature.
> >
> > It is likely that the way this will shake out is that some maintainers
> > will accept the additional overhead of including parameter names in their
> > BC guidelines and others will not, this leaves users unsure if they can
> use
> > the new feature without storing up issues in potentially minor/security
> > releases of the libraries they use. This is not really an ideal
> situation.
> >
> > More pressing a point is that the current implementation breaks object
> > polymorphism. Consider this example (simplified from one of my codebases)
> >
> > interface Handler {
> >     public function handle($message);
> > }
> >
> > class RegistrationHandler implements Handler {
> >     public function handle($registraionCommand);
> > }
> >
> > class ForgottenPasswordHandler implements Handler {
> >     public function handle($forgottenPasswordCommand);
> > }
> >
> > class MessageBus {
> >     //...
> >     public function addHandler(string $message, Handler $handler) {
> //... }
> >     public function getHandler(string $messageType): Handler { //... }
> >     public function dispatch($message)
> >     {
> >         $this->getHandler(get_class($message))->handle(message:
> $message);
> >     }
> > }
> >
> > This code breaks at run time.
> >
> > Proposals were made for resolutions to this issue however all of them
> > require trade offs and could potentially break existing code.
> >
> > My proposal to resolve these two issues is to add the ability to rename
> > parameters with a new syntax as follows.
> >
> > function callBar(Foo $internalName:externalName) {
> >     $internalName->bar();
> > }
> >
> > $x = new Foo();
> > callBar(externalName: $x);
> >
> > This allows both the above problems to be resolved, by renaming the
> > internal parameter and keeping the external signature the same.
> >
> > I propose that the RFC would have two voting options.
> >
> > The first would be to implement it as proposed above, this would allow
> any
> > parameter to be called by name regardless of the intentions of the author
> > of the method/function and is closest to the current behaviour.
> >
> > The second option would be to use this syntax to make named parameters in
> > userland code explicitly opt in. As such an additional shortcut syntax
> > would be implemented: $: to designate a named parameter. eg
> >
> > function callBar($:externalName) {
> >     $externalName->bar();
> > }
> >
> > $x = new Foo();
> > callBar(externalName: $x);
> >
> > If a parameter is not opted in, a compile time error is raised:
> >
> > function callBar($externalName) {
> >     $externalName->bar();
> > }
> >
> > $x = new Foo();
> > callBar(externalName: $x); // Error: cannot call parameter $externalName
> > by name.
> >
> > There are pros and cons to this second approach, on the one hand it
> > reduces the usefulness of the named parameter syntax by requiring changes
> > to old code to enable it (although this could probably be automated
> fairly
> > easily) however it does provide a neater solution to the second problem
> in
> > that, to prevent the runtime errors in the second issue example, every
> > child class would need to use the rename syntax on it's parameter to
> > prevent errors, whereas if we went down this route, the parent class
> could
> > just not opt into the named parameter syntax and the code would function
> as
> > expected.
> >
> > Another advantage is that with the ability to rename parameters using the
> > opt in, we gain some flexibility to tighten up the LSP rules relating to
> > named parameter inheritance.
> >
> > class Foo {
> >     public function bar($:param) { //... }
> >     public function baz($internal:external) { //... }
> > }
> >
> > // OK
> > class Bar {
> >     public function bar($renamed:param) { //... }
> >     public function baz($renamed:external) { //... }
> > }
> >
> > // Compile time error cannot rename named parameter $:param (renamed to
> > $:renamedParam)
> > class Baz {
> >     public function bar($:renamedParam) { //... }
> > }
> >
> > // Compile time error cannot rename named parameter $:external (renamed
> to
> > $:renamed)
> > class Baz {
> >     public function baz($internal:renamed) { //... }
> > }
> >
> > While this would be technically possible with the first option (no opt
> in)
> > it would break any existing code which renames a parameter as every
> > parameter would be subject to these rules.
> >
> > I don't have Wiki karma so can't post this yet; but I want to get the
> ball
> > rolling on discussion as feature freeze is coming up fast and if we want
> to
> > go for the second option, that must hit before the named parameter syntax
> > is in a tagged version of PHP.
> >
> > Regards,
> > Chris
> >
>

--000000000000cb355305ab5ae344--