Everything

php

Inconsistent class behavior and undocumented(?) BC change

php.internals

Larry Garfield

6 years ago

I am not sure if this is a bug, a feature behaving in a desired but confusing way, or a feature behaving in a confusing and thus undesireable way.  I am therefore reporting it here in order to defer to those who know the answer to such questions better.

Consider the following:

class Ancestor {
public function __construct(int $a, string $b) { }
}

class Child extends Ancestor {
public function __construct(...$args) {
parent::__construct(...$args);
}
}

This works with no compile errors.  For any other method however:

class Ancestor {
public function doStuff(int $a, string $b) { }
}

class Child extends Ancestor {
public function doStuff(...$args) {
parent::doStuff(...$args);
}
}

I get:

Warning: Declaration of Child::doStuff(...$args) should be compatible with Ancestor::doStuff(int $a, string $b)

I am not clear on why __construct() is special in this case; I know __construct() is special where interfaces are concerned, but I didn't realize it was special with regards to basic inheritance.  It seems to happen regardless of the type information presented (or not).

Is this intentional?  Is there a logical way it could be made to work?  Are constructors actually wrong here?  (I hope not, because it's a neat trick.)

Additionally, according to 3v4l.org at least, the error message has changed.  On 7.1-7.3, the exact error message is:

Warning: Declaration of Child::doStuff(int ...$args) should be compatible with Ancestor::doStuff($a, $b) in /in/6NthP on line 15

On 7.4, it reports on a different line:

Warning: Declaration of Child::doStuff(int ...$args) should be compatible with Ancestor::doStuff($a, $b) in /in/6NthP on line 11

Specifically, prior to 7.4, it reports on the LAST line of the class (the closing brace).  As of 7.4 it reports on the line of the method that is inconsistent.

I don't think this is a bad change, per se.  It's actually a good change for debugging, IMO.  But I don't see it listed in the migration guide.  Should it be, in case some failure-mode tests or other automated error systems care?

https://www.php.net/manual/en/migration74.incompatible.php

--
Larry Garfield
larry@garfieldtech.com

Mike Schinkel

6 years ago

Hi Larry,

> I am not clear on why __construct() is special in this case;

I believe that is the Liskok substitution principle at work, and that fact the principle does not apply to constructors.

For reference:

- https://softwareengineering.stackexchange.com/a/302477/9114
- https://www.sitepoint.com/constructors-and-the-myth-of-breaking-the-lsp/

-Mike

Rowan Collins

6 years ago

On 08/12/2019 05:03, Mike Schinkel wrote:

> Hi Larry,
>
>> I am not clear on why __construct() is special in this case;
> I believe that is the Liskok substitution principle at work, and that fact the principle does not apply to constructors.
>
> For reference:
>
> -https://softwareengineering.stackexchange.com/a/302477/9114
> -https://www.sitepoint.com/constructors-and-the-myth-of-breaking-the-lsp/

I'm not sure either of those links do a good job explaining why
constructors in particular are special.

My first thought was "you call them on the class not an instance", but
that's also true of static methods, and yet this is an error:

class Ancestor {
    public static function constructMe(int $a, string $b) { }
}
class Child extends Ancestor {
    public static function constructMe(...$args) {
        return parent::constructMe(...$args);
    }
}

In PHP, it is actually possible to call a static method based on an
instance, using the syntax "$foo::somethingStatic()", so it is
theoretically logical to constrain a set of related classes to have
compatible static methods. But you can also write "new $foo()" to invoke
a constructor based on an instance, so that doesn't explain the
difference either.

The best explanation I can think of is more pragmatic than theoretical:
it's often useful to have related objects with unrelated constructors,
so it's useful for the language not to restrict that. I'd be interested
to hear if there's a more fundamental reason, though.

Regards,

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

Larry Garfield

6 years ago

On Sun, Dec 8, 2019, at 10:14 AM, Rowan Tommins wrote:

> On 08/12/2019 05:03, Mike Schinkel wrote:
> > Hi Larry,
> >
> >> I am not clear on why __construct() is special in this case;
> > I believe that is the Liskok substitution principle at work, and that fact the principle does not apply to constructors.
> >
> > For reference:
> >
> > -https://softwareengineering.stackexchange.com/a/302477/9114
> > -https://www.sitepoint.com/constructors-and-the-myth-of-breaking-the-lsp/
>
>
> I'm not sure either of those links do a good job explaining why
> constructors in particular are special.
>
> My first thought was "you call them on the class not an instance", but
> that's also true of static methods, and yet this is an error:
>
> class Ancestor {
>      public static function constructMe(int $a, string $b) { }
> }
> class Child extends Ancestor {
>      public static function constructMe(...$args) {
>          return parent::constructMe(...$args);
>      }
> }
>
> In PHP, it is actually possible to call a static method based on an
> instance, using the syntax "$foo::somethingStatic()", so it is
> theoretically logical to constrain a set of related classes to have
> compatible static methods. But you can also write "new $foo()" to invoke
> a constructor based on an instance, so that doesn't explain the
> difference either.
>
> The best explanation I can think of is more pragmatic than theoretical:
> it's often useful to have related objects with unrelated constructors,
> so it's useful for the language not to restrict that. I'd be interested
> to hear if there's a more fundamental reason, though.

I recall constructors being special with interfaces; I didn't consciously realize that *all* inheritance rules were skipped for them.  I suppose in that case it's logical that it is why it is, just disappointing; the double-splat trick I was trying above is really nifty, but I guess it only works on constructors.  Sad elePHPant...

--Larry Garfield

Nikita Popov

6 years ago

On Sun, Dec 8, 2019 at 1:29 AM Larry Garfield <larry@garfieldtech.com>
wrote:

> I am not sure if this is a bug, a feature behaving in a desired but
> confusing way, or a feature behaving in a confusing and thus undesireable
> way.  I am therefore reporting it here in order to defer to those who know
> the answer to such questions better.
>
> Consider the following:
>
> class Ancestor {
>     public function __construct(int $a, string $b) { }
> }
>
> class Child extends Ancestor {
>     public function __construct(...$args) {
>         parent::__construct(...$args);
>     }
> }
>
> This works with no compile errors.  For any other method however:
>
> class Ancestor {
>     public function doStuff(int $a, string $b) { }
> }
>
> class Child extends Ancestor {
>     public function doStuff(...$args) {
>         parent::doStuff(...$args);
>     }
> }
>
> I get:
>
> Warning: Declaration of Child::doStuff(...$args) should be compatible with
> Ancestor::doStuff(int $a, string $b)
>
> I am not clear on why __construct() is special in this case; I know
> __construct() is special where interfaces are concerned, but I didn't
> realize it was special with regards to basic inheritance.  It seems to
> happen regardless of the type information presented (or not).
>
> Is this intentional?  Is there a logical way it could be made to work?
> Are constructors actually wrong here?  (I hope not, because it's a neat
> trick.)
>

Others have already explained why constructors are exempted from LSP
checks, so let me reply to your other point...

I believe that your example should indeed be legal in general and created a
PR to fix this: https://github.com/php/php-src/pull/5059

With this change, the variadic argument can replace any number of
non-variadic arguments, as long as it is compatible with them, where
compatibility is, as usual, determined based on type contravariance, as
well as reference passing invariance. Effectively this means that
function(...$args) is compatible with all signatures that do not involve
reference passing.

Does anyone see any soundness issues with this change?

Nikita

Larry Garfield

6 years ago

On Mon, Jan 6, 2020, at 5:16 AM, Nikita Popov wrote:

> Others have already explained why constructors are exempted from LSP
> checks, so let me reply to your other point...
>
> I believe that your example should indeed be legal in general and created a
> PR to fix this: https://github.com/php/php-src/pull/5059
>
> With this change, the variadic argument can replace any number of
> non-variadic arguments, as long as it is compatible with them, where
> compatibility is, as usual, determined based on type contravariance, as
> well as reference passing invariance. Effectively this means that
> function(...$args) is compatible with all signatures that do not involve
> reference passing.
>
> Does anyone see any soundness issues with this change?
>
> Nikita

Nifty!  Since `func(...$args)` is already a universal caller, that gives us a symmetrical way to defer calls, either to a parent or to a delegated method on a wrapped object.  So that means one can implement a pass-through class where most of the methods are:

class Wrapper implements Thing {
public function thing(...$args) { $this->wrapped->thing(...$args); }
}

The symmetry appeals to me, even if it means types get caught only at the lower level.

One possible question; would this mean bypassing type checks if not delegating?  Viz:

interface Foo {
public function bar(Foo, $f, Baz $b, Beep $e);
}

class Impl implements Foo {

public function bar(...$args) { ... }

}

$i = new Impl();

// Will this still report a type error, and if so, where/how?
// If not, does that become a back-door way to dodge an interface?  Is that good?
$i->bar('a', 'b', 'c');

--Larry Garfield

Nikita Popov

6 years ago

On Mon, Jan 6, 2020 at 7:18 PM Larry Garfield <larry@garfieldtech.com>
wrote:

> On Mon, Jan 6, 2020, at 5:16 AM, Nikita Popov wrote:
>
> > Others have already explained why constructors are exempted from LSP
> > checks, so let me reply to your other point...
> >
> > I believe that your example should indeed be legal in general and
> created a
> > PR to fix this: https://github.com/php/php-src/pull/5059
> >
> > With this change, the variadic argument can replace any number of
> > non-variadic arguments, as long as it is compatible with them, where
> > compatibility is, as usual, determined based on type contravariance, as
> > well as reference passing invariance. Effectively this means that
> > function(...$args) is compatible with all signatures that do not involve
> > reference passing.
> >
> > Does anyone see any soundness issues with this change?
> >
> > Nikita
>
> Nifty!  Since `func(...$args)` is already a universal caller, that gives
> us a symmetrical way to defer calls, either to a parent or to a delegated
> method on a wrapped object.  So that means one can implement a pass-through
> class where most of the methods are:
>
> class Wrapper implements Thing {
>   public function thing(...$args) { $this->wrapped->thing(...$args); }
> }
>
> The symmetry appeals to me, even if it means types get caught only at the
> lower level.
>
> One possible question; would this mean bypassing type checks if not
> delegating?  Viz:
>
> interface Foo {
>   public function bar(Foo, $f, Baz $b, Beep $e);
> }
>
> class Impl implements Foo {
>
>   public function bar(...$args) { ... }
>
> }
>
> $i = new Impl();
>
> // Will this still report a type error, and if so, where/how?
> // If not, does that become a back-door way to dodge an interface?  Is
> that good?
> $i->bar('a', 'b', 'c');
>

Yes, you can "bypass" type checks that way. However, this is not related to
variadics, but general contravariance. In particular, you can also simply
drop the types while keeping the explicit parameters:

class Impl implements Foo {

public function bar($f, $b, $e) { ... }

}

As of PHP 7.2 (I think) dropping parameter types is always allowed.

Nikita