# Bug #80248 - Swapping parameter names during inheritance does not throw

- Thread: https://externals.io/message/112121
- Original archive: https://news-web.php.net/php.internals/112121
- Started by vorismi3@fel.cvut.cz on 2020-10-26T13:12:05+00:00
- Messages: 10
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #112121 — vorismi3@fel.cvut.cz — 2020-10-26T13:12:05+00:00

URL: https://externals.io/message/112121#112121

Hi, 

I am writing regarding bug 80248. 

Currently, PHP 8 allows parameter reuse at different position, which I
belive is very dangerous, as passed parameters may be passed in a
different order with different object impl. See https://3v4l.org/X8omS ,
which is against OOP and LSP. 

I belive, we can allow parameter rename, but we should throw if "already
seen parameter name is at a different position". 

I am not that good in C to fix this, so only reporting. I belive, we
should fix this issue asap and merge into PHP 8. 

Also - for further discussion - I think, we can even allow parameter
rename, but still allow to use the old name (which, with fix above, will
always result to the same param position). Example:
https://3v4l.org/kgHWf (now an error, it violates OOP/LSP again) 

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek

---

## #112125 — Rowan Tommins — 2020-10-26T14:36:52+00:00

URL: https://externals.io/message/112121#112125 · In reply to: #112121

> I am writing regarding bug 80248.
> Currently, PHP 8 allows parameter reuse at different position, which I
> belive is very dangerous, as passed parameters may be passed in a
> different order with different object impl.


Hi Michael,

Yes, this was one of the most discussed aspects of named parameters. 
There were a few proposals for how it should work, but in the end none 
of them gained consensus, and Nikita decided to keep things simple.

I suggest having a look at the RFC 
[https://wiki.php.net/rfc/named_params] and its main discussion thread 
[https://externals.io/message/110004] to see the pros and cons of 
various alternatives.

Since none of these were adopted in the proposal, it's not clear that 
this could be considered a bug, rather than a feature change, so there'd 
need to be a very strong justification for re-visiting it this close to 
8.0 availability.

Regards,

-- 
Rowan Tommins (né Collins)
[IMSoP]

---

## #112130 — vorismi3@fel.cvut.cz — 2020-10-28T10:45:44+00:00

URL: https://externals.io/message/112121#112130 · In reply to: #112125

I see, guys, what is the single reason to allowing this dangerous
inheritance? 

https://3v4l.org/X8omS parameters renamed, result with named parameters
is different 

https://3v4l.org/kgHWf renamed parameter, call with named parameters
does not succeed at all (which violated basic principe of OOP
inheritance at least) 

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek



> On 26/10/2020 13:12, Michael Voříšek - ČVUT FEL wrote: 
> 
> > […]
> 
> Hi Michael,
> 
> Yes, this was one of the most discussed aspects of named parameters. There were a few proposals for how it should work, but in the end none of them gained consensus, and Nikita decided to keep things simple.
> 
> I suggest having a look at the RFC [https://wiki.php.net/rfc/named_params] and its main discussion thread [https://externals.io/message/110004] to see the pros and cons of various alternatives.
> 
> Since none of these were adopted in the proposal, it's not clear that this could be considered a bug, rather than a feature change, so there'd need to be a very strong justification for re-visiting it this close to 8.0 availability.
> 
> Regards,
> 
> -- 
> Rowan Tommins (né Collins)
> [IMSoP]

---

## #112131 — Rowan Tommins — 2020-10-28T14:57:39+00:00

URL: https://externals.io/message/112121#112131 · In reply to: #112130

> https://3v4l.org/X8omS parameters renamed, result with named parameters
> is different


While it's easy to construct an example where this happens, it's harder 
to imagine a scenario in real life where:

* a sub-class overloads the function with different parameter names...
* ...that overlap with the original parameter names... (i.e. the call 
will succeed)
* ... but not in the same order...
* ...where calling with ordered parameters results in the expected 
behaviour (i.e. it's not already incorrect code)

It seems more likely in practice that a polymorphic call assuming the 
parameters are in the same order would fail where one assuming they have 
the same names will succeed, e.g.:

class A {
      public function search(string $needle, string $haystack) { ... }
}
class B extends A {
      public function search(string $haystack, string $needle) { ... }
}

$aOrB->search("foo", "foobar"); // incorrect call on instances of B, but 
allowed in every version of PHP

$aOrB->search(needle: "foo", haystack: "foobar"); // correct behaviour 
whether instance of A or B :)


> https://3v4l.org/kgHWf renamed parameter, call with named parameters
> does not succeed at all (which violated basic principe of OOP
> inheritance at least) 


This is the case that is explicitly discussed in the RFC: 
https://wiki.php.net/rfc/named_params#parameter_name_changes_during_inheritance

I'm not sure what more can be said than appears in that summary, and in 
the linked discussion of rejected alternatives. As the RFC says, the 
pragmatic decision was taken to defer these errors to runtime.

It's worth noting that since PHP doesn't have checked exceptions, a 
child method throwing an error that it's parent wouldn't is already 
possible and not considered a violation: https://3v4l.org/3m7eo


Regards,

-- 
Rowan Tommins (né Collins)
[IMSoP]

---

## #112132 — vorismi3@fel.cvut.cz — 2020-10-28T15:14:59+00:00

URL: https://externals.io/message/112121#112132 · In reply to: #112131

I agree - "it's harder to imagine a scenario in real life where". 

From php perspective, swapping parameters in inheritance SHOULD NOT be
allowed, as without named parameters the parameters are not swapped.
Also, if the parameters are typed differently, the example is even
impossible (and nowdays, typed parameters are very common, thus
"commonly impossible"). 

If we can agree, that implementation is not guaranteed to be called with
named parameters only, what real world usecase to defend this current
php behaviour is left? 

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek



> On 28/10/2020 10:45, Michael Voříšek - ČVUT FEL wrote: 
> 
> > […]
> 
> While it's easy to construct an example where this happens, it's harder to imagine a scenario in real life where:
> 
> * a sub-class overloads the function with different parameter names...
> * ...that overlap with the original parameter names... (i.e. the call will succeed)
> * ... but not in the same order...
> * ...where calling with ordered parameters results in the expected behaviour (i.e. it's not already incorrect code)
> 
> It seems more likely in practice that a polymorphic call assuming the parameters are in the same order would fail where one assuming they have the same names will succeed, e.g.:
> 
> class A {
> public function search(string $needle, string $haystack) { ... }
> }
> class B extends A {
> public function search(string $haystack, string $needle) { ... }
> }
> 
> $aOrB->search("foo", "foobar"); // incorrect call on instances of B, but allowed in every version of PHP
> 
> $aOrB->search(needle: "foo", haystack: "foobar"); // correct behaviour whether instance of A or B :)
> 
> > […]
> 
> This is the case that is explicitly discussed in the RFC: https://wiki.php.net/rfc/named_params#parameter_name_changes_during_inheritance
> 
> I'm not sure what more can be said than appears in that summary, and in the linked discussion of rejected alternatives. As the RFC says, the pragmatic decision was taken to defer these errors to runtime.
> 
> It's worth noting that since PHP doesn't have checked exceptions, a child method throwing an error that it's parent wouldn't is already possible and not considered a violation: https://3v4l.org/3m7eo
> 
> Regards,
> 
> -- 
> Rowan Tommins (né Collins)
> [IMSoP]

---

## #112133 — Chase Peeler — 2020-10-28T16:39:13+00:00

URL: https://externals.io/message/112121#112133 · In reply to: #112132

On Wed, Oct 28, 2020 at 11:15 AM Michael Voříšek - ČVUT FEL <
vorismi3@fel.cvut.cz> wrote:

> I agree - "it's harder to imagine a scenario in real life where".
>
> From php perspective, swapping parameters in inheritance SHOULD NOT be
> allowed, as without named parameters the parameters are not swapped.
> Also, if the parameters are typed differently, the example is even
> impossible (and nowdays, typed parameters are very common, thus
> "commonly impossible").
>
>
What do you mean they "aren't swapped?"

class A {
   public function foo ($str1,$str2){
        return strcompare($str1,$str2);
   }
}

class B extends A{
    public function foo($str2,$str1){
      return strcompare($str2,$str);
      //or
     //return parent::foo($str1,$str2);
   }
}

They are swapped in the above example because the code inside B::foo
handles the swapping. And, maybe there is a valid reason that the developer
wanted to emphasize $str2 over $str1 in the subclass.

If we can agree, that implementation is not guaranteed to be called with
> named parameters only, what real world usecase to defend this current
> php behaviour is left?

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,
>
>
Considering that parameter swapping prior to unnamed parameters has always
been supported, what unique issue does it represent that requires we solve
it now? Anything done to prevent it would almost certainly be a huge BC
break. As for real world use-cases, I can think of a few:

1.) Developer the subclass wants to make certain parameters optional which
weren't originally at the end of the parameter list
2.) Parent class broke common practices in terms of parameter order
($needle, $haystack vs $haystack, $needle) and they want their subclass to
follow the more commonly used pattern.
3.) The developer of the subclass just wants them in a different order for
some reason that makes sense to them. Maybe they don't need to support
polymorphism. Maybe the idea is that the subclass will be used INSTEAD of
the parent class and any knowledge of the parent class by other programmers
isn't even necessary.


> Michael Voříšek
>
> 
>
> > […]
> to imagine a scenario in real life where:
> > […]
> will succeed)
> > […]
> behaviour (i.e. it's not already incorrect code)
> > […]
> parameters are in the same order would fail where one assuming they have
> the same names will succeed, e.g.:
> > […]
> allowed in every version of PHP
> > […]
> whether instance of A or B :)
> > […]
>

I agree that this scenario is very contrived. You are looking at a scenario
where multiple examples of poor programming are layered on top of each
other. Trying to prevent such scenarios risks going down a rabbit hole that
removes a lot of freedom from the language.


> > […]
> https://wiki.php.net/rfc/named_params#parameter_name_changes_during_inheritance
> > […]
> the linked discussion of rejected alternatives. As the RFC says, the
> pragmatic decision was taken to defer these errors to runtime.
> > […]
> child method throwing an error that it's parent wouldn't is already
> possible and not considered a violation: https://3v4l.org/3m7eo
> > […]



-- 
Chase Peeler
chasepeeler@gmail.com

---

## #112134 — Rowan Tommins — 2020-10-28T16:43:25+00:00

URL: https://externals.io/message/112121#112134 · In reply to: #112132

> I agree - "it's harder to imagine a scenario in real life where".
> [...]
> If we can agree, that implementation is not guaranteed to be called with
> named parameters only, what real world usecase to defend this current
> php behaviour is left?


You're thinking about this the wrong way around: the simplest 
implementation is to detect non-existent named parameters (which happens 
to include renamed parameters) at run-time; it is additional checks on 
top of that which need to be justified.

Other approaches to the problem require at least one of:

* Significant additional code in the engine to perform additional checks 
and/or name aliasing
* Users to change existing code which works correctly, but would 
theoretically break if used with named parameters

The advantages are almost entirely theoretical, with few realistic 
examples.

So the "pragmatic approach" the RFC refers to concludes that the benefit 
of additional analysis does not outweigh its cost.

Regards,

-- 
Rowan Tommins (né Collins)
[IMSoP]

---

## #112136 — vorismi3@fel.cvut.cz — 2020-10-28T17:06:37+00:00

URL: https://externals.io/message/112121#112136 · In reply to: #112133

see https://3v4l.org/X8omS 

As long as non-named parameters are supported (they always will be),
calling different implementation can produce different results when
called with named/not-named parametrs. 

Let say we have interface/class X with method test(int $offset, int
$limit). 

Let's extend it by test(int $limit, int $offset). 

When it is called with unnamed arguments, both implementations return
the same result and satisfy the interface. But, if the purpose of the
rename is to allow to swap the parameters, what is the advantage if it
does not work with unnamed arguments? 

For better names, I propose to allow rename "to never used name", thus
position is never changed and everything behave consistently across
named/unnamed params. I also propose to accept the old names, as
currently, if not used with the new names, the parameter names are not
resolved, so OOP/LSP is violated. 

So my personal summary is, this behaviour is nonsense and should be not
allowed as there is no practical benefit of it. 

To your example: 

public function foo($str, $needle){ 

is something I do not propose to forbid, only the names have to be new
or match the original position in iface/parent class 

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek



> On Wed, Oct 28, 2020 at 11:15 AM Michael Voříšek - ČVUT FEL <vorismi3@fel.cvut.cz> wrote: 
> 
> > […]
> 
> What do you mean they "aren't swapped?" 
> 
> class A { 
> public function foo ($str1,$str2){ 
> return strcompare($str1,$str2); 
> } 
> } 
> 
> class B extends A{ 
> public function foo($str2,$str1){ 
> return strcompare($str2,$str); 
> //or 
> //return parent::foo($str1,$str2); 
> } 
> } 
> 
> They are swapped in the above example because the code inside B::foo handles the swapping. And, maybe there is a valid reason that the developer wanted to emphasize $str2 over $str1 in the subclass. 
> 
> > […]
> 
> > […]
> 
> Considering that parameter swapping prior to unnamed parameters has always been supported, what unique issue does it represent that requires we solve it now? Anything done to prevent it would almost certainly be a huge BC break. As for real world use-cases, I can think of a few: 
> 
> 1.) Developer the subclass wants to make certain parameters optional which weren't originally at the end of the parameter list 
> 2.) Parent class broke common practices in terms of parameter order ($needle, $haystack vs $haystack, $needle) and they want their subclass to follow the more commonly used pattern. 
> 3.) The developer of the subclass just wants them in a different order for some reason that makes sense to them. Maybe they don't need to support polymorphism. Maybe the idea is that the subclass will be used INSTEAD of the parent class and any knowledge of the parent class by other programmers isn't even necessary.  
> 
> > […]
> 
> I agree that this scenario is very contrived. You are looking at a scenario where multiple examples of poor programming are layered on top of each other. Trying to prevent such scenarios risks going down a rabbit hole that removes a lot of freedom from the language. 
> 
> > […]
> 
> -- 
> Chase Peeler
> chasepeeler@gmail.com

---

## #112137 — vorismi3@fel.cvut.cz — 2020-10-28T17:16:23+00:00

URL: https://externals.io/message/112121#112137 · In reply to: #112134

> Significant additional code in the engine to perform additional checks and/or name aliasing 

I know, but we can do very easily one thing - check and throw if
overriding method has one or more named parameter on different position.
On class creation time, ie. only once, no overhead per call. 

Then calling with named/unnamed parameters is **consistent** (or
resolves to an "Unknown named parameter" error) and... We are safe! 

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek



> On 28/10/2020 15:14, Michael Voříšek - ČVUT FEL wrote: 
> 
> > […]
> 
> You're thinking about this the wrong way around: the simplest implementation is to detect non-existent named parameters (which happens to include renamed parameters) at run-time; it is additional checks on top of that which need to be justified.
> 
> Other approaches to the problem require at least one of:
> 
> * Significant additional code in the engine to perform additional checks and/or name aliasing
> * Users to change existing code which works correctly, but would theoretically break if used with named parameters
> 
> The advantages are almost entirely theoretical, with few realistic examples.
> 
> So the "pragmatic approach" the RFC refers to concludes that the benefit of additional analysis does not outweigh its cost.
> 
> Regards,
> 
> -- 
> Rowan Tommins (né Collins)
> [IMSoP]

---

## #112138 — Rowan Tommins — 2020-10-28T17:38:46+00:00

URL: https://externals.io/message/112121#112138 · In reply to: #112136

> For better names, I propose to allow rename "to never used name", thus
> position is never changed and everything behave consistently across
> named/unnamed params. I also propose to accept the old names, as
> currently, if not used with the new names, the parameter names are not
> resolved, so OOP/LSP is violated. 


This is precisely the behaviour discussed as an alternative in the RFC: 
https://wiki.php.net/rfc/named_params#to_parameter_name_changes_during_inheritance

The reasons it was not implemented are given at the end of that section.


> > […]
>
> I know, but we can do very easily one thing - check and throw if
> overriding method has one or more named parameter on different position.


How confident are you that we can "very easily" do this? Have you looked 
into how it would need to be implemented, and what edge cases we would 
need to consider?

At risk of sounding like a broken record: Nikita did look into it, and 
documented his conclusions in the RFC.


Regards,

-- 
Rowan Tommins (né Collins)
[IMSoP]