Re: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant Parameters
> 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.
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant Parameters
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 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 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.
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 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.
Re: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant Parameters
On Thu, Dec 20, 2018 at 3:35 PM Dmitry Stogov 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). -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant Parameters
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 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 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
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 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
