Inconsistent class behavior and undocumented(?) BC change

4 years ago by Larry Garfield — view source

unread

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

4 years ago by Mike Schinkel — view source

unread

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:

-Mike

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

4 years ago by Nikita Popov — view source

unread

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

4 years ago by Larry Garfield — view source

unread

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

4 years ago by Nikita Popov — view source

unread

On Mon, Jan 6, 2020 at 7:18 PM Larry Garfield larry@garfieldtech.com
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

4 years ago by Larry Garfield — view source

unread

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