[RFC][Vote] Covariant Returns and Contravariant Parameters

  103611
December 19, 2018 23:10 levim@php.net (Levi Morrison)
Thank you for the feedback and discussion on the [Covariant Returns
and Contravariant Parameters RFC][1].

I have opened [voting on this RFC][2]. 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!

  [1]: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters
  [2]: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters#voting
  103614
December 20, 2018 15:26 dmitry@zend.com (Dmitry Stogov)
Hi Levi,


Please, create a Pull Request, to keep inline comments on github.


Thanks. Dmitry.

________________________________
From: Levi Morrison <levim@php.net>
Sent: Thursday, December 20, 2018 2:10:57 AM
To: internals
Subject: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant Parameters

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

I have opened [voting on this RFC][2]. 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!

  [1]: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters
  [2]: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters#voting

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
  103616
December 20, 2018 22:35 dmitry@zend.com (Dmitry Stogov)
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.


________________________________
From: Dmitry Stogov <dmitry@zend.com>
Sent: Thursday, December 20, 2018 6:26:10 PM
To: Levi Morrison; internals
Subject: Re: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant Parameters

Hi Levi,


Please, create a Pull Request, to keep inline comments on github.


Thanks. Dmitry.

________________________________
From: Levi Morrison <levim@php.net>
Sent: Thursday, December 20, 2018 2:10:57 AM
To: internals
Subject: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant Parameters

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

I have opened [voting on this RFC][2]. 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!

  [1]: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters
  [2]: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters#voting

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
  103617
December 20, 2018 23:14 cmbecker69@gmx.de ("Christoph M. Becker")
On 20.12.2018 at 23:35, Dmitry Stogov wrote:

> It looks like the patch broke something related to opcache. > > It crashes at least on Wordpress and Drupal.
Note that <https://bugs.php.net/76937> is still unresolved – apparently a regression from PHP 7.2. The testing code that triggered the problem is still commented out[1].
> Inability to work with opcache, doesn't allow to check the performance impact.
I agree that we should carefully check the performance impact of the proposed change. [1] <https://github.com/drupal/drupal/blob/8.6.x/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php#L141-L144> -- Christoph M. Becker
  103618
December 21, 2018 00:12 levim@php.net (Levi Morrison)
On Thu, Dec 20, 2018 at 3:35 PM Dmitry Stogov <dmitry@zend.com> wrote:
> > 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.
What OS and compiler are these on? How are you ensuring that opcache is on when these tests are run? I have not been experiencing these issues, so maybe I am not running it correctly. If I cannot reproduce them soon then I will agree to cancel the voting. There are some known issues outside of Zend. Notably some internal classes do not have valid method signatures with regards to inheritance which this patch exposed. These need fixed regardless of this RFC and I have begun work on some of them (see https://github.com/php/php-src/pull/3686 for one example).
  103619
December 21, 2018 07:28 dmitry@zend.com (Dmitry Stogov)
On 12/21/18 3:12 AM, Levi Morrison wrote:
> On Thu, Dec 20, 2018 at 3:35 PM Dmitry Stogov <dmitry@zend.com> wrote: >> >> 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. > > What OS and compiler are these on?
Linux (Fedora 28), GCC 8.2.1.
> How are you ensuring that opcache > is on when these tests are run? I have not been experiencing these > issues, so maybe I am not running it correctly. If I cannot reproduce > them soon then I will agree to cancel the voting.
You should enable opcache in your php.ini zend_extension=opcache.so opcache.enable=1 opcache.enable_cli=1 opcache.optimization_level=-1 opcache.protect_memory=1 ; this is for testing only Memory corruption bugs may lead to crash or not because of "luck", but it's possible to see them using valgrind. $ make test TESTS="-m tests/classes/array_access_011.phpt" $ cat tests/classes/array_access_011.mem This shows almost the same backtrace, as I already published. Looks like an incorrect reference-counting on some string.
> > There are some known issues outside of Zend. Notably some internal > classes do not have valid method signatures with regards to > inheritance which this patch exposed. These need fixed regardless of > this RFC and I have begun work on some of them (see > https://github.com/php/php-src/pull/3686 for one example).
That PR looks fine. There is also a problem that this PR has merge conflict with master and travis doesn't run tests. Thanks. Dmitry.
  103630
December 24, 2018 12:20 dmitry@zend.com (Dmitry Stogov)
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)





2. But this test misses warning





- 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.

________________________________
From: Dmitry Stogov <dmitry@zend.com>
Sent: Friday, December 21, 2018 10:28:05 AM
To: Levi Morrison
Cc: internals
Subject: Re: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant Parameters



On 12/21/18 3:12 AM, Levi Morrison wrote:
> On Thu, Dec 20, 2018 at 3:35 PM Dmitry Stogov <dmitry@zend.com> wrote: >> >> 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. > > What OS and compiler are these on?
Linux (Fedora 28), GCC 8.2.1.
> How are you ensuring that opcache > is on when these tests are run? I have not been experiencing these > issues, so maybe I am not running it correctly. If I cannot reproduce > them soon then I will agree to cancel the voting.
You should enable opcache in your php.ini zend_extension=opcache.so opcache.enable=1 opcache.enable_cli=1 opcache.optimization_level=-1 opcache.protect_memory=1 ; this is for testing only Memory corruption bugs may lead to crash or not because of "luck", but it's possible to see them using valgrind. $ make test TESTS="-m tests/classes/array_access_011.phpt" $ cat tests/classes/array_access_011.mem This shows almost the same backtrace, as I already published. Looks like an incorrect reference-counting on some string.
> > There are some known issues outside of Zend. Notably some internal > classes do not have valid method signatures with regards to > inheritance which this patch exposed. These need fixed regardless of > this RFC and I have begun work on some of them (see > https://github.com/php/php-src/pull/3686 for one example).
That PR looks fine. There is also a problem that this PR has merge conflict with master and travis doesn't run tests. Thanks. Dmitry.
  103631
December 24, 2018 13:22 cmbecker69@gmx.de ("Christoph M. Becker")
On 24.12.2018 at 13:20, Dmitry Stogov wrote:

> - 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...
Well, one may argue that the voting is about the concept, but not about a (preliminary) implementation. The main concern, in my opinion, is rather whether it is possible to implement the proposal without (much) drawbacks (such as general performance regression). A working patch would be helpful to prevent cases where we accept a proposal, but later face difficulties implementing it and/or can't agree on some of the details[1][2]. We may consider to augment the RFC process to clearly require a working implementation before starting the vote. [1] <https://wiki.php.net/rfc/null_coalesce_equal_operator> [2] <https://wiki.php.net/rfc/notice-for-non-valid-array-container> -- Christoph M. Becker
  103643
December 27, 2018 17:48 levim@php.net (Levi Morrison)
> 2. But this test misses warning > > > $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 code detects this as an error but intentionally does not report it because in other cases a warning is already issued and I did not want to re-issue the warning again. I'll dig into this more to see why it is not issued the first time in this case. However, this should *remain* a warning to be consistent with existing code. When a class extends another non-abstract class and there isn't an interface involved then it's only a warning. We should fix this in PHP 8.0 for all cases.
  103676
January 3, 2019 15:28 levim@php.net (Levi Morrison)
On Wed, Dec 19, 2018 at 4:10 PM Levi Morrison <levim@php.net> wrote:
> > Thank you for the feedback and discussion on the [Covariant Returns > and Contravariant Parameters RFC][1]. > > I have opened [voting on this RFC][2]. 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! > > [1]: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters > [2]: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters#voting
Currently there are 31 yes votes, and only 1 no vote. I will leave the vote open for a few more days while I examine an issue that Dmitry mentioned here on the list. When we have code like the following, no errors or warnings are generated but there should be a warning: I am intentionally not generating a warning at runtime for some cases. The engine already issues a warning at compile time for some similar cases and I didn't want to duplicate the warning at runtime. Apparently this code is not precise enough.
  103683
January 4, 2019 19:17 levim@php.net (Levi Morrison)
On Thu, Jan 3, 2019 at 8:28 AM Levi Morrison <levim@php.net> wrote:
> > On Wed, Dec 19, 2018 at 4:10 PM Levi Morrison <levim@php.net> wrote: > > > > Thank you for the feedback and discussion on the [Covariant Returns > > and Contravariant Parameters RFC][1]. > > > > I have opened [voting on this RFC][2]. 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! > > > > [1]: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters > > [2]: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters#voting > > Currently there are 31 yes votes, and only 1 no vote. I will leave the > vote open for a few more days while I examine an issue that Dmitry > mentioned here on the list. When we have code like the following, no > errors or warnings are generated but there should be a warning: > > class A {} > class X { > function m(A $z) {} > } > class Y extends X { > function m(NotAnA $z) {} > } > ?> > > I am intentionally not generating a warning at runtime for some cases. > The engine already issues a warning at compile time for some similar > cases and I didn't want to duplicate the warning at runtime. > Apparently this code is not precise enough.
I have a new pull request: https://github.com/php/php-src/pull/3732 This fixes all currently known issues with the implementation, including the snippet from my last message. There are a few todo items left that may have issues, but so far we haven't hit those cases in any code. I intend to close the vote in a day or two, unless I hear of new issues from Dmitry or others.
  103750
January 15, 2019 20:27 cmbecker69@gmx.de ("Christoph M. Becker")
On 04.01.2019 at 20:17, Levi Morrison 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
  103778
January 22, 2019 17:58 levim@php.net (Levi Morrison)
On Tue, Jan 15, 2019 at 1:27 PM Christoph M. Becker <cmbecker69@gmx.de> wrote:
> > On 04.01.2019 at 20:17, Levi Morrison 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.
  105640
May 8, 2019 08:05 nikita.ppv@gmail.com (Nikita Popov)
On Tue, Jan 22, 2019 at 6:59 PM Levi Morrison <levim@php.net> wrote:

> On Tue, Jan 15, 2019 at 1:27 PM Christoph M. Becker <cmbecker69@gmx.de> > wrote: > > > > On 04.01.2019 at 20:17, Levi Morrison 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
  105643
May 8, 2019 16:27 bishop@php.net (Bishop Bettini)
On Wed, May 8, 2019 at 4:06 AM Nikita Popov ppv@gmail.com> wrote:

> On Tue, Jan 22, 2019 at 6:59 PM Levi Morrison <levim@php.net> wrote: > > > On Tue, Jan 15, 2019 at 1:27 PM Christoph M. Becker <cmbecker69@gmx.de> > > wrote: > > > > > > On 04.01.2019 at 20:17, Levi Morrison 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.) [1]: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters
  105644
May 8, 2019 16:50 levim@php.net (Levi Morrison)
On Wed, May 8, 2019 at 2:06 AM Nikita Popov ppv@gmail.com> wrote:
> > On Tue, Jan 22, 2019 at 6:59 PM Levi Morrison <levim@php.net> wrote: >> >> On Tue, Jan 15, 2019 at 1:27 PM Christoph M. Becker <cmbecker69@gmx.de> wrote: >> > >> > On 04.01.2019 at 20:17, Levi Morrison 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][1]. 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][2]), which might make things a bit more straightforward (it might not -- turns out parent is not exactly what I thought it was). [1]: https://wiki.php.net/rfc/lsp_errors [2]: https://github.com/php/php-src/commit/deb44d405eb27a6654ad9a57c1e5f641218b22a4