Thank you for the feedback and discussion on the Covariant Returns
and Contravariant Parameters RFC.

I have opened voting on this RFC. Given that this is a common
time for holidays for many people around the world it will be open
until at least January 2nd. Happy holidays!

Hi Levi,

It looks like the patch broke something related to opcache.

It crashes at least on Wordpress and Drupal.

The backtrace https://gist.github.com/dstogov/a2305381a5c9982cceca9e4e252d26c7 shows use-after-free in opcache (works fine with master).

Inability to work with opcache, doesn't allow to check the performance impact.

It also broke few tests. Some crash. Some produce different warning/errors.

$ make test TESTS="Zend tests"

...

=====================================================================
FAILED TEST SUMMARY

ZE2 ArrayAccess and ArrayAccessReferenceProxy with references to main array [tests/classes/array_access_011.phpt]
Bug #21478 (Zend/zend_alloc.c :: shutdown_memory_manager produces segfault) [Zend/tests/bug21478.phpt]
Generator methods can yield by reference [Zend/tests/generators/generator_method_by_ref.phpt]
Testing to implement Serializable interface by traits [Zend/tests/traits/interface_003.phpt]
Handling of public fields with traits needs to have same semantics as with normal inheritance, however, we do add strict warnings since it is easier to run into something unexpeted with changing traits. [Zend/tests/traits/property009.phpt]
iterable type#004 - Parameter covariance [Zend/tests/type_declarations/iterable_004.phpt]
iterable type#005 - Return type covariance [Zend/tests/type_declarations/iterable_005.phpt]

I'll try to play with patch and make a full code review on next week.

It would be great, if you fix opcache compatibility.

If it can't be done in reasonable time, it's probably better to cancel voting and restart when ready.

Thanks. Dmitry.

Hi Levi,

I made just few tests, to understand that the implementation is at least incomplete.

The warning message depends on class declaration order and may be emitted or not.

1. This test produces a warning (as RFC proposes)

<?php

class C {}
class D {}
class A {
function bar(C $c) {}
}
class B extends A {
function bar(D $D) {
echo "ok\n";
}
}

?>

2. But this test misses warning

<?php
$q = 1;
if ($q) {
class C {}
class D {}
}

class A {
function bar(C $c) {}
}
class B extends A {
function bar(D $D) {
echo "ok\n";
}
}
?>

• The patch is incompatible with opcache (crashes on Wordpress, Drupal, and probably any real-life app).

• the incompatibility with opcache, doesn't allow to check the performance implication of the patch

• the patch has merge conflicts and travis tests doesn't run.

Personally, I'm not against the proposal, but I'm definitely against this implementation.

Probably, it's better to cancel the voting and restart when the implementation is ready.

It's pity to see, that nobody tries the implementation but blindly vote...

Thanks. Dmitry.

On Tue, Jan 15, 2019 at 1:27 PM Christoph M. Becker cmbecker69@gmx.de
wrote:

I intend to close the vote in a day or two, unless I hear of new>
issues from Dmitry or others.
Any news here?

--
Christoph M. Becker

I sent this a week ago to Christoph only; oops.

I have not heard any news. The vote is now closed. The RFC passes 39
in favor to 1 against.

Special thanks to Nikita and Dmitry who have helped find issues and
review the patch. It will not be merged until the implementation
quality is satisfactory.

As we're moving steadily towards 7.4 feature freeze, I'd like to discuss
what we want to do with this RFC... The current implementation doesn't work
correctly (I've done some more work in
https://github.com/nikic/php-src/commits/variance-7.4, but it's also
incomplete) and I have some doubts about how we're approaching this in
general.

This RFC really has two parts:

1. The actual variance change. This is a very straightforward change and
   there are no issues here.
2. The ability to check variance across multiple consecutive class
   definitions. This allows type declarations to reference classes that are
   declared later in the same file (but within one "block" of declarations).

The second part is technically more dicey and somewhat arbitrary when seen
in the wider scope of how class hoisting and early binding work in PHP:
While PHP supports declaring classes "out of order" in some very simple
cases like this...

class B extends A {}
class A {}

...it will not work for anything more involved than that, for example

class C extends B {}
class B extends A {}
class A {}

will already generate a "class not found" error.

Now the variance RFC tackles one very specific part of this long-standing
issue: The types referenced in parameter and return types may be declared
later in the file (even if used variantly), but all other uses of the types
still need to respect the declaration order.

I think that we should be separating these two issues (variance and
declaration order), and land the simple variance support in 7.4, while
tackling the declaration order problem in full separately (in PHP 8,
because I think we may want to make some BC breaking changes, in particular
by making the class hoisting unconditional.)

Thoughts on this approach?

Nikita

On Tue, Jan 15, 2019 at 1:27 PM Christoph M. Becker cmbecker69@gmx.de
wrote:

I intend to close the vote in a day or two, unless I hear of new>
issues from Dmitry or others.
Any news here?

--
Christoph M. Becker

I sent this a week ago to Christoph only; oops.

I have not heard any news. The vote is now closed. The RFC passes 39
in favor to 1 against.

Special thanks to Nikita and Dmitry who have helped find issues and
review the patch. It will not be merged until the implementation
quality is satisfactory.

As we're moving steadily towards 7.4 feature freeze, I'd like to discuss
what we want to do with this RFC... The current implementation doesn't work
correctly (I've done some more work in
https://github.com/nikic/php-src/commits/variance-7.4, but it's also
incomplete) and I have some doubts about how we're approaching this in
general.

This RFC really has two parts:

1. The actual variance change. This is a very straightforward change and
   there are no issues here.
2. The ability to check variance across multiple consecutive class
   definitions. This allows type declarations to reference classes that are
   declared later in the same file (but within one "block" of declarations).

The second part is technically more dicey and somewhat arbitrary when seen
in the wider scope of how class hoisting and early binding work in PHP:
While PHP supports declaring classes "out of order" in some very simple
cases like this...

class B extends A {}
class A {}

...it will not work for anything more involved than that, for example

class C extends B {}
class B extends A {}
class A {}

will already generate a "class not found" error.

Now the variance RFC tackles one very specific part of this long-standing
issue: The types referenced in parameter and return types may be declared
later in the file (even if used variantly), but all other uses of the types
still need to respect the declaration order.

I think that we should be separating these two issues (variance and
declaration order), and land the simple variance support in 7.4, while
tackling the declaration order problem in full separately (in PHP 8,
because I think we may want to make some BC breaking changes, in particular
by making the class hoisting unconditional.)

Thoughts on this approach?

As I read it, the RFC 1 aims to address the variance problem. The
declaration problem is related - but separate. Seems to me, solving the
variance problem is the primary concern. Since declarations are therefore a
secondary concern, I agree with the separation. (More the so because, IMO,
it's better to solve a problem holistically.)

I intend to close the vote in a day or two, unless I hear of new> issues from Dmitry or others.
Any news here?

--
Christoph M. Becker

I sent this a week ago to Christoph only; oops.

I have not heard any news. The vote is now closed. The RFC passes 39
in favor to 1 against.

Special thanks to Nikita and Dmitry who have helped find issues and
review the patch. It will not be merged until the implementation
quality is satisfactory.

As we're moving steadily towards 7.4 feature freeze, I'd like to discuss what we want to do with this RFC... The current implementation doesn't work correctly (I've done some more work in https://github.com/nikic/php-src/commits/variance-7.4, but it's also incomplete) and I have some doubts about how we're approaching this in general.

This RFC really has two parts:

1. The actual variance change. This is a very straightforward change and there are no issues here.
2. The ability to check variance across multiple consecutive class definitions. This allows type declarations to reference classes that are declared later in the same file (but within one "block" of declarations).

The second part is technically more dicey and somewhat arbitrary when seen in the wider scope of how class hoisting and early binding work in PHP: While PHP supports declaring classes "out of order" in some very simple cases like this...

class B extends A {}
class A {}

...it will not work for anything more involved than that, for example

class C extends B {}
class B extends A {}
class A {}

will already generate a "class not found" error.

Now the variance RFC tackles one very specific part of this long-standing issue: The types referenced in parameter and return types may be declared later in the file (even if used variantly), but all other uses of the types still need to respect the declaration order.

I think that we should be separating these two issues (variance and declaration order), and land the simple variance support in 7.4, while tackling the declaration order problem in full separately (in PHP 8, because I think we may want to make some BC breaking changes, in particular by making the class hoisting unconditional.)

Thoughts on this approach?

Nikita

I fully support this approach. I will prepare a patch for simple
variance in PHP 7.4. I intend to leave the existing test cases that
will fail without supporting consecutive declarations, but marked as
expected failures.

I think in PHP 8 we can already benefit from the always generate
fatal error for incompatible method signatures RFC. We might also
be able to make some improvements with compile-time errors on invalid
"parent::" usage (previously done for PHP 7.4 but backed out),
which might make things a bit more straightforward (it might not --
turns out parent is not exactly what I thought it was).

On Tue, Jan 15, 2019 at 1:27 PM Christoph M. Becker cmbecker69@gmx.de
wrote:

I intend to close the vote in a day or two, unless I hear of new>
issues from Dmitry or others.
Any news here?

--
Christoph M. Becker

I sent this a week ago to Christoph only; oops.

I have not heard any news. The vote is now closed. The RFC passes 39
in favor to 1 against.

Special thanks to Nikita and Dmitry who have helped find issues and
review the patch. It will not be merged until the implementation
quality is satisfactory.

As we're moving steadily towards 7.4 feature freeze, I'd like to discuss
what we want to do with this RFC... The current implementation doesn't work
correctly (I've done some more work in
https://github.com/nikic/php-src/commits/variance-7.4, but it's also
incomplete) and I have some doubts about how we're approaching this in
general.

This RFC really has two parts:

1. The actual variance change. This is a very straightforward change and
   there are no issues here.
2. The ability to check variance across multiple consecutive class
   definitions. This allows type declarations to reference classes that are
   declared later in the same file (but within one "block" of declarations).

The second part is technically more dicey and somewhat arbitrary when
seen in the wider scope of how class hoisting and early binding work in
PHP: While PHP supports declaring classes "out of order" in some very
simple cases like this...

class B extends A {}
class A {}

...it will not work for anything more involved than that, for example

class C extends B {}
class B extends A {}
class A {}

will already generate a "class not found" error.

Now the variance RFC tackles one very specific part of this
long-standing issue: The types referenced in parameter and return types may
be declared later in the file (even if used variantly), but all other uses
of the types still need to respect the declaration order.

I think that we should be separating these two issues (variance and
declaration order), and land the simple variance support in 7.4, while
tackling the declaration order problem in full separately (in PHP 8,
because I think we may want to make some BC breaking changes, in particular
by making the class hoisting unconditional.)

Thoughts on this approach?

Nikita

I fully support this approach. I will prepare a patch for simple
variance in PHP 7.4. I intend to leave the existing test cases that
will fail without supporting consecutive declarations, but marked as
expected failures.

I think in PHP 8 we can already benefit from the always generate
fatal error for incompatible method signatures RFC. We might also
be able to make some improvements with compile-time errors on invalid
"parent::" usage (previously done for PHP 7.4 but backed out),
which might make things a bit more straightforward (it might not --
turns out parent is not exactly what I thought it was).

An update on this: The last part of covariance support has landed 1 a few
days ago and is part of 7.4 alpha 1. As already described, full variance is
only supported in conjunction with autoloading. When working in a single
file or with explicit includes, the requirement that classes need to be
declared before being referenced (including reference in variant type
declarations) remains.

This feature turned out to be a lot harder than anticipated. My initial
assumption that this would mostly "just work" if we ignore the single file
case was very wrong...

Nikita

1
https://github.com/php/php-src/commit/8f8fcbbd397370b407dc2552c4bd6ee4ccb0e93b

Am 13.06.2019 um 18:48 schrieb Nikita Popov:

An update on this: The last part of covariance support has landed [1] a few
days ago and is part of 7.4 alpha 1. As already described, full variance is
only supported in conjunction with autoloading. When working in a single
file or with explicit includes, the requirement that classes need to be
declared before being referenced (including reference in variant type
declarations) remains.

Does "only supported in conjunction with autoloading" mean that covariance
is not (completely) supported when preloading is used?

On Sat, Jun 15, 2019 at 9:02 AM Sebastian Bergmann sebastian@php.net
wrote:

Am 13.06.2019 um 18:48 schrieb Nikita Popov:

An update on this: The last part of covariance support has landed [1] a
few
days ago and is part of 7.4 alpha 1. As already described, full variance
is
only supported in conjunction with autoloading. When working in a single
file or with explicit includes, the requirement that classes need to be
declared before being referenced (including reference in variant type
declarations) remains.

Does "only supported in conjunction with autoloading" mean that covariance
is not (completely) supported when preloading is used?

Preloading is indeed currently limited to DAG class references (this
includes class references in inheritance, in types and in constant
expressions). Of course, it will fall back transparently to autoloading for
the cases it cannot handle.

If you run into this in practice (due to covariance or other reasons), feel
free to report a bug -- there's some low hanging fruit in terms of
restrictions we can remove.

Nikita