Add covariance/contravariance to inherited methods #3732

morrisonlevi · 2019-01-04T16:24:27Z

Return types are covariant; parameter types are contravariant.

RFC: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters

Replaces PR #3712.

nikic · 2019-01-07T12:19:14Z

This currently doesn't warn, even though it should:

<?php
namespace Foo;

class A {
    public function foo(B $x) : A {
        return new self;
    }
}

namespace Bar;

class B extends \Foo\A {
    public function foo(\stdClass $x) : B {
        return new self;
    }
}

var_dump((new \Bar\B)->foo(new \stdClass));

(What I really wanted to do here is show the opposite -- I don't think your code handles classes split across namespaces correctly, i.e. it will not verify variance for all of them together.)

Zend/tests/bug76451.phpt

Zend/tests/typehints/undefined_type_during_variance_check_04.phpt

Zend/zend_vm_def.h

Zend/zend_compile.c

nikic · 2019-01-07T13:28:01Z

Zend/zend_compile.c

-		uint32_t i;
-		for (i = 0; i < list->children; ++i) {
-			zend_compile_top_stmt(list->child[i]);
+		zend_ast **begin = list->child;


As already mentioned, this code is not going to handle namespaces correctly, especially when using braced namespace Foo { ... } form. You'll want classes that are part of multiple braced (or unbraced, but braced is the more complex case) to be part of the same variance region.

I think I finally understand what you are saying:

namespace NS1 { class A implements B {} } namespace NS2 { class X implements Y {} }

The A should be verified after X is declared. Right?

Zend/zend_inheritance.c

nikic · 2019-01-07T13:49:11Z

Zend/zend_inheritance.c

-
-		/* by-ref constraints on arguments are invariant */
-		if (fe_arg_info->pass_by_reference != proto_arg_info->pass_by_reference) {
+		if (!_check_inherited_parameter_type(fe, fe_arg_info, proto, proto_arg_info)) {


Shouldn't this be passing on the -1 state as well?

Or maybe not? I don't really get how or at which point this code interacts with CG(unverified_types). I expected that something would remove a class from that set if verification returns 1 (rather than -1), but don't see it.

Zend/zend_inheritance.c

morrisonlevi · 2019-01-09T16:02:45Z

Perhaps I made a mistake when putting this into a form that would work in a single file test (or maybe the issue is really when it is split across files), but this fatals for me:

namespace Foo {
  class A {
    function foo(B $x): A {
      return new self();
    }
  }
}

namespace Bar {
  class B extends \Foo\A {
    function foo(\stdClass $x): B {
      return new self();
    }
  }
}

namespace {
  var_dump((new \Bar\B())->foo(new \stdClass()));
}

I get:

Fatal error: Declaration of Bar\B::foo(stdClass $x): Bar\B must be compatible with Foo\A::foo(Foo\B $x): Foo\A

This is because Foo\B cannot be found (and indeed, we never defined it). Maybe your test was slightly different and you copied it in wrong? Or maybe I put it into a single-file incorrectly?

nikic · 2019-01-10T09:40:30Z

@morrisonlevi Just checked again, the code I provided (exactly as-is, single file) does not produce an error for me. Your code does error. I guess the different namespace style (semicolon vs brace) matters here.

morrisonlevi · 2019-01-10T14:42:34Z

I verified that in your example it does not emit ZEND_VERIFY_VARIANCE opcodes. Thank you.

nikic · 2019-01-11T08:50:24Z

Zend/zend_compile.c

-		zend_ast_list *list = zend_ast_get_list(ast);
-		zend_ast **begin = list->child;
-		zend_ast **end = begin + list->children;
+		zend_ast_list *const list = zend_ast_get_list(ast);


nit: We don't use postfix const (at least for locals).

This deals with two ranges, and I kept accidentally moving begin. I slapped const on a few variables so that the compiler would catch my mistakes here. I can remove it if we really care.

nikic · 2019-01-11T09:09:59Z

Zend/zend_compile.c

+		/* Compile any stmts between the two type decls (or the end) */
+		for (p = last_decl; p < first_decl; ++p) {
+			zend_compile_stmt(*p);
+		}


Should this really be here? Our early binding only applies to top-level class declarations, not conditional ones, so I would kind of expect that this also holds for variance checks. On the other hand, I don't see a pressing technical reason why we can't do this.

Maybe there is a better way, but this fixed the bug you posted, the one with multiple namespaces without using braces. We need to support variance, even if it's not top-level. Here's another example:

<?php interface A { function m(X $z); } if (true) { class B implements A { function m(Y $z) {} // ? } }

We have a test case for things like this, but I forgot which one it was off the top of my head.

While we do need to support variance checks for it (we certainly need that for everything), what (I think, maybe) we don't need it support is the ability to verify variance for multiple classes at once (with cyclic dependencies). I don't think we can reasonable check variance for both classes together in the example you provided, we'd have to check first A and then B, independently.

I verified that the variance opcodes are delayed correctly for this code:

<?php interface A { function m(X $z): A; } if (true) { class B implements A { function m(X $z): B { return new self(); } } class C extends B { function m(X $z): C { return new self(); } } }

Snippet of opcodes:

L7 #0 JMPZ true J5 L8 #1 DECLARE_CLASS "b" L13 #2 DECLARE_INHERITED_CLASS "c" "B" L13 #3 VERIFY_VARIANCE "b" L13 #4 VERIFY_VARIANCE "c" L21 #5 RETURN<-1> 1

Maybe I misunderstood your point, but it seems to work.

azjezz · 2019-01-19T22:37:34Z

I'm wondering whether this case is covered by this RFC :

<?php declare(strict_types=1);

interface TaskInterface {}

interface TaskHandlerInterface {
    public function handle(TaskInterface $task): void;
}

class TaskHandler implements TaskHandlerInterface {
    public function handle(TaskInterface $task): void {
        /* ... */
    }
}

class MultipleTaskHandler implements TaskHandlerInterface {
    private $handler;
    
    public function __construct(TaskHandlerInterface $handler) {
        $this->handler = $handler;
    }
    
    /**
     * Not permitted, but type-safe :/
     */
    public function handle(TaskInterface ...$tasks): void {
        foreach($tasks as $task) {
            $this->handler->handle($task);
        }
    }
}

theodorejb · 2019-01-20T05:07:53Z

@azjezz I don't think that example is related to contravariance, since it doesn't change the type of the parameter but rather the number of parameters.

TaskHandlerInterface specifies that the handle method must be passed a single TaskInterface argument, but MultipleTaskHandler would allow it to be called with multiple arguments, or no arguments. Is it actually considered type-safe for child methods to support passing more or fewer arguments?

morrisonlevi · 2019-01-20T05:38:39Z

Fewer should be fine, as long as the same number can still be accepted. More should be fine, as long as they are optional. Unless I'm just really tired, the behavior should be permissible.

Return types are covariant; parameter types are contravariant.

This should probably be moved somewhere else and renamed.

This work is based off Nikita's review. Notably: - Implement contravariance in terms of covariance - Improve support for non-consecutive type decls - Adds _inheritance_status enum

TerjeBr · 2019-02-20T20:52:05Z

Hi @morrisonlevi . Thank you for your great work on this.

I am waiting in eager anticipation for this to land, and I wonder if you could please give a rough estimate of when it will be ready?

Anyway, keep up the good work, I appreciate it.

nikic · 2019-03-04T11:27:33Z

I've just reverted the compile-time parent check in deb44d4, because it breaks Mockery. Due to the issue with parent in traits this patch needs to deal with unresolved parent anyway, so I don't think this is problematic.

@davedevelopment As a heads up, it would be good to not generate this code https://github.com/mockery/mockery/blob/4324afeaf9d95b492507e6587abb3f024e2576de/library/Mockery/Mock.php#L603 if there is no parent class, as I expect we'll want to make it a compile-error at some point, even if not in 7.4.

Majkl578 · 2019-03-08T02:23:06Z

@TerjeBr The RFC was accepted for PHP 7.4 so unless some serious problem appears it can be expected to land there.

nikic · 2019-05-08T11:02:16Z

A fun failing case:

<?php
  
$b = new B;
echo "foobar\n";
return;

class A {
    public function test(): X {}
}
class B extends A {
    public function test(): Y {}
}

This will happily create the object without ever verifying variance. The reason is that the classes are early-bound, but the verification is still only performed at the point of definition. We'd have to move the verification to the start of the op array or something -- ideally we'd really not early-bind in this case.

nikic · 2019-06-17T15:32:14Z

Superseded by #4185 and #4194.

madonzy · 2020-01-28T19:03:21Z

Guys, I can see that return types overloading is implemented correctly in PHP 7.4, but why this code throws the fatal error? ProductRequestDataInterface is the sub-type of RequestDataInterface, but it's still wrong? Why?

<?php

interface RequestDataInterface {}
interface ProductRequestDataInterface extends RequestDataInterface {}

interface HandlerInterface {
    function handle(RequestDataInterface $param);
}

class Handler implements HandlerInterface {
    // Fatal error: Declaration of 
    // Handler::handle(ProductRequestDataInterface $param)
    // must be compatible 
    // with HandlerInterface::handle(RequestDataInterface $param)
    function handle(ProductRequestDataInterface $param) {
        // whatever
    }
}

nikic · 2020-01-28T19:07:47Z

@madonzy Please see https://en.wikipedia.org/wiki/Liskov_substitution_principle and https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science) for more information.

TerjeBr · 2020-01-28T19:19:53Z

@madonzy Arguments to methods are contravariant:
It is type safe to allow an overriding method to accept a more general argument than the method in the base class.
But it is not type safe to accept a more specific argument in the overriding method.

madonzy · 2020-01-29T15:09:48Z

@nikic @TerjeBr Thank you, guys. Now everything is clear :)

daniyalhamid · 2021-08-19T23:07:33Z

Hi guys,

In the documentation it says:

A type declaration is considered more specific ... (when) float is changed to int

Then how come the following throws an error:

class A
{
    public function foo(): float
    {
        // ...
    }
}

class B extends A
{
    public function foo(): int
    {
        // ...
    }
}

Thanks!

morrisonlevi mentioned this pull request Jan 4, 2019

Covariant Returns and Contravariant Parameters #3712

Closed

morrisonlevi force-pushed the variance4 branch 2 times, most recently from 1c6bea2 to 20dbef1 Compare January 4, 2019 17:00

petk added the RFC label Jan 4, 2019

morrisonlevi force-pushed the variance4 branch from 20dbef1 to 039f71b Compare January 4, 2019 17:27

nikic reviewed Jan 7, 2019

View reviewed changes

nikic reviewed Jan 11, 2019

View reviewed changes

morrisonlevi added 4 commits January 22, 2019 11:22

Add covariance/contravariance to inherited methods

0b31517

Return types are covariant; parameter types are contravariant.

Extract _get_parent_class_name

ad67313

This should probably be moved somewhere else and renamed.

Improve variance implementation

a0892eb

This work is based off Nikita's review. Notably: - Implement contravariance in terms of covariance - Improve support for non-consecutive type decls - Adds _inheritance_status enum

Support more type decl locations for variance

b781f9f

morrisonlevi force-pushed the variance4 branch from 9d3a948 to cd7d1a8 Compare January 22, 2019 18:24

Consolidate, rename, and add new variance tests

bf4e912

morrisonlevi force-pushed the variance4 branch from cd7d1a8 to bf4e912 Compare January 22, 2019 20:19

davedevelopment mentioned this pull request Mar 8, 2019

Remove references to parent from mock template mockery/mockery#957

Open

nikic mentioned this pull request Mar 27, 2019

Make "prototype" the function which inheritance is checked against #3993

Closed

nikic closed this Jun 17, 2019

Add covariance/contravariance to inherited methods #3732

Add covariance/contravariance to inherited methods #3732

Uh oh!

Conversation

morrisonlevi commented Jan 4, 2019 • edited by nikic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Jan 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

morrisonlevi commented Jan 9, 2019

Uh oh!

nikic commented Jan 10, 2019

Uh oh!

morrisonlevi commented Jan 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azjezz commented Jan 19, 2019

Uh oh!

theodorejb commented Jan 20, 2019

Uh oh!

morrisonlevi commented Jan 20, 2019

Uh oh!

TerjeBr commented Feb 20, 2019

Uh oh!

nikic commented Mar 4, 2019

Uh oh!

Majkl578 commented Mar 8, 2019

Uh oh!

nikic commented May 8, 2019

Uh oh!

nikic commented Jun 17, 2019

Uh oh!

madonzy commented Jan 28, 2020

Uh oh!

nikic commented Jan 28, 2020

Uh oh!

TerjeBr commented Jan 28, 2020

Uh oh!

madonzy commented Jan 29, 2020

Uh oh!

daniyalhamid commented Aug 19, 2021

Uh oh!

Uh oh!

morrisonlevi commented Jan 4, 2019 •

edited by nikic

Loading