Strict type declarations not enforced for Reflection API invocation

  100851
October 10, 2017 13:41 sebastian@php.net (Sebastian Bergmann)
https://bugs.php.net/bug.php?id=75345 is about the fact that strict type
declarations are not enforced when a function or method is invoked via the
Reflection API.

There is a pull request that addresses this (as well as
https://bugs.php.net/bug.php?id=74750) at
https://github.com/php/php-src/pull/2837.

I consider this a serious bug that leads to unexpected, confusing problems
such as
https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-335180273.

I understand Nikita's point of view (see
https://github.com/php/php-src/pull/2837#issuecomment-335405067) that
changing this behavior (aka. fixing this bug) can be considered a
"non-trivial backwards compatibility break". Therefore I would like to
bring this issue to the attention of this list with this mail.
  100852
October 10, 2017 14:03 lists@rhsoft.net ("lists@rhsoft.net")
Am 10.10.2017 um 15:41 schrieb Sebastian Bergmann:
> https://bugs.php.net/bug.php?id=75345 is about the fact that strict type > declarations are not enforced when a function or method is invoked via the > Reflection API. > > There is a pull request that addresses this (as well as > https://bugs.php.net/bug.php?id=74750) at > https://github.com/php/php-src/pull/2837. > > I consider this a serious bug that leads to unexpected, confusing problems > such as > https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-335180273. > > I understand Nikita's point of view (see > https://github.com/php/php-src/pull/2837#issuecomment-335405067) that > changing this behavior (aka. fixing this bug) can be considered a > "non-trivial backwards compatibility break". Therefore I would like to > bring this issue to the attention of this list with this mail
i don't see the argument of "non-trivial backwards compatibility break" in case of a major bug - which code should break? the one rely on broken bahvior? that code is already broken in in such cases it should raise errors as soon as possible instead get extended and then years later when the wrong behavior si fixed the breakage and work to fix bad code relying on the old one is even greater to say it in other words: if i have written code that relys on wrong behavior i would like to know it yesterday and fix it tomorrow instead that i continue to exnted and write such code and that with 7.3 or so i have magnitudes more to change and adopt
  100854
October 10, 2017 14:57 cmbecker69@gmx.de ("Christoph M. Becker")
On 10.10.2017 at 15:41, Sebastian Bergmann wrote:

> I consider this a serious bug that leads to unexpected, confusing problems > such as > https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-335180273. > > I understand Nikita's point of view (see > https://github.com/php/php-src/pull/2837#issuecomment-335405067) that > changing this behavior (aka. fixing this bug) can be considered a > "non-trivial backwards compatibility break". Therefore I would like to > bring this issue to the attention of this list with this mail.
This is most certainly not an *implementation* bug. The RFC which introduced strict typing[1] states: | By default, all PHP files are in weak type-checking mode. And later: | This proposal takes the standpoint that it's up to the caller to | decide how functions should be called. | […] | Therefore, this proposal does not allow internal developers to | “opt-in” to strict typing. In case of bug #75345 the caller is a method of ReflectionFunction, which is defined in weak type-checking mode. One may argue that this is a *design* bug, but after nearly two years of PHP 7 there may be a lots of code relying on the current behavior, so in my opinion changing the behavior could not really be regarded as fixing a bug. [1] <https://wiki.php.net/rfc/scalar_type_hints_v5> -- Christoph M. Becker
  100855
October 10, 2017 15:10 lists@rhsoft.net ("lists@rhsoft.net")
Am 10.10.2017 um 16:57 schrieb Christoph M. Becker:
> On 10.10.2017 at 15:41, Sebastian Bergmann wrote: > >> I consider this a serious bug that leads to unexpected, confusing problems >> such as >> https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-335180273. >> >> I understand Nikita's point of view (see >> https://github.com/php/php-src/pull/2837#issuecomment-335405067) that >> changing this behavior (aka. fixing this bug) can be considered a >> "non-trivial backwards compatibility break". Therefore I would like to >> bring this issue to the attention of this list with this mail. > > This is most certainly not an *implementation* bug. The RFC which > introduced strict typing[1] states: > > | By default, all PHP files are in weak type-checking mode.
yes but the file in question has strict-types enabled declare(strict_types=1);
> And later: > > | This proposal takes the standpoint that it's up to the caller to > | decide how functions should be called. > | […] > | Therefore, this proposal does not allow internal developers to > | “opt-in” to strict typing.
yes but the file in question has strict-types enabled declare(strict_types=1);
> In case of bug #75345 the caller is a method of ReflectionFunction, > which is defined in weak type-checking mode.
yes but the file in question has strict-types enabled declare(strict_types=1);
> One may argue that this is a *design* bug, but after nearly two years of > PHP 7 there may be a lots of code relying on the current behavior, so in > my opinion changing the behavior could not really be regarded as fixing > a bug. > > [1] <https://wiki.php.net/rfc/scalar_type_hints_v5>
yes but the file in question has strict-types enabled declare(strict_types=1); that decalare line was the explicit wish of the person who wrote the php file
  100856
October 10, 2017 16:41 me@kelunik.com (Niklas Keller)
2017-10-10 17:10 GMT+02:00 lists@rhsoft.net <lists@rhsoft.net>:

> > Am 10.10.2017 um 16:57 schrieb Christoph M. Becker: > >> On 10.10.2017 at 15:41, Sebastian Bergmann wrote: >> >> I consider this a serious bug that leads to unexpected, confusing problems >>> such as >>> https://github.com/sebastianbergmann/phpunit/issues/2796# >>> issuecomment-335180273. >>> >>> I understand Nikita's point of view (see >>> https://github.com/php/php-src/pull/2837#issuecomment-335405067) that >>> changing this behavior (aka. fixing this bug) can be considered a >>> "non-trivial backwards compatibility break". Therefore I would like to >>> bring this issue to the attention of this list with this mail. >>> >> >> This is most certainly not an *implementation* bug. The RFC which >> introduced strict typing[1] states: >> >> | By default, all PHP files are in weak type-checking mode. >> > > yes but the file in question has strict-types enabled > declare(strict_types=1);
Yes, but `array_map` also uses weak types for the callback, like any other internal function call. But `call_user_func` is also special-cased, maybe we should do the same with the reflection calls. All in all, two typing modes were a bad idea to begin with, mostly because nobody payed attention to callbacks. Regards, Niklas
  100857
October 10, 2017 16:45 lists@rhsoft.net ("lists@rhsoft.net")
Am 10.10.2017 um 18:41 schrieb Niklas Keller:
> 2017-10-10 17:10 GMT+02:00 lists@rhsoft.net <mailto:lists@rhsoft.net> > <lists@rhsoft.net <mailto:lists@rhsoft.net>>: > > > Am 10.10.2017 um 16:57 schrieb Christoph M. Becker: > > On 10.10.2017 at 15:41, Sebastian Bergmann wrote: > > I consider this a serious bug that leads to unexpected, > confusing problems > such as > https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-335180273 > <https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-335180273>. > > I understand Nikita's point of view (see > https://github.com/php/php-src/pull/2837#issuecomment-335405067 > <https://github.com/php/php-src/pull/2837#issuecomment-335405067>) > that > changing this behavior (aka. fixing this bug) can be > considered a > "non-trivial backwards compatibility break". Therefore I > would like to > bring this issue to the attention of this list with this mail. > > > This is most certainly not an *implementation* bug.  The RFC which > introduced strict typing[1] states: > > | By default, all PHP files are in weak type-checking mode. > > > yes but the file in question has strict-types enabled > declare(strict_types=1); > > > Yes, but `array_map` also uses weak types for the callback, like any > other internal function call. > > But `call_user_func` is also special-cased, maybe we should do the same > with the reflection calls. > > All in all, two typing modes were a bad idea to begin with, mostly > because nobody payed attention to callbacks
the two typing modes are perfect and when you converted a 15 years old codebase to strcit-ytpes and type-hints everywehere you know why - first you start with typehints and you can't do that strict or you will burn out and give up but when i define strict types in a PHP file everything but code in includes has to be strict typed inherited
  100859
October 10, 2017 17:11 cmbecker69@gmx.de ("Christoph M. Becker")
On 10.10.2017 at 18:41, Niklas Keller wrote:

> 2017-10-10 17:10 GMT+02:00 lists@rhsoft.net <lists@rhsoft.net>:
> All in all, two typing modes were a bad idea to begin with, mostly because > nobody payed attention to callbacks.
"nobody" is not quite right. If I remember correctly, this issue has first been brought up on this list by Benjamin Eberlei[1], and it has been discussed. [1] <http://news.php.net/php.internals/82619> -- Christoph M. Becker
  100858
October 10, 2017 16:53 nikita.ppv@gmail.com (Nikita Popov)
On Tue, Oct 10, 2017 at 3:41 PM, Sebastian Bergmann <sebastian@php.net>
wrote:

> https://bugs.php.net/bug.php?id=75345 is about the fact that strict type > declarations are not enforced when a function or method is invoked via the > Reflection API. > > There is a pull request that addresses this (as well as > https://bugs.php.net/bug.php?id=74750) at > https://github.com/php/php-src/pull/2837. > > I consider this a serious bug that leads to unexpected, confusing problems > such as > https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment- > 335180273. > > I understand Nikita's point of view (see > https://github.com/php/php-src/pull/2837#issuecomment-335405067) that > changing this behavior (aka. fixing this bug) can be considered a > "non-trivial backwards compatibility break". Therefore I would like to > bring this issue to the attention of this list with this mail. >
To repeat what I said in a comment on the pull request: I think this is fixing the wrong issue. The problem are not internal function calls, the problem are callbacks. In fact, the proposed fix does not actually fix the problem you encountered in PHPUnit, as it is going to use the strictness mode at the reflection call-site, not the strictness mode used by the file defining the data provider. The proposed patch is going to add a discrepancy between internal and userland code. It means that the internal array_map function will execute the callback using the strictness mode of the array_map caller, while a userland reimplementation of the same function will not be able to match that behavior and always either perform the call strictly or weakly, independent of the array_map caller. I believe that the proper way to fix this is to handle dynamic function invocations differently from direct invocations. Direct invocations should use the strictness level of the call-site, while dynamic invocations should use the strictness level of the declaration-site. This means that the internal array_map and a userland reimplementation will behave consistent. It also means that PHPUnit will be able to respect the strictness level of file defining the data provider, etc. I've seen a number of complaints about our handling of callbacks wrt strict_types, so I think it's worthwhile to at least consider making such a change. Regards, Nikita
  100860
October 11, 2017 05:53 sebastian@php.net (Sebastian Bergmann)
Am 10.10.2017 um 18:53 schrieb Nikita Popov:
> The problem are not internal function calls, the problem are callbacks. In > fact, the proposed fix does not actually fix the problem you encountered in > PHPUnit, as it is going to use the strictness mode at the reflection > call-site, not the strictness mode used by the file defining the data > provider.
I was aware of that (I only saw the pull request but did not look at it), thanks for clarifying.
> I believe that the proper way to fix this is to handle dynamic function > invocations differently from direct invocations. Direct invocations should > use the strictness level of the call-site, while dynamic invocations should > use the strictness level of the declaration-site. This means that the > internal array_map and a userland reimplementation will behave consistent. > It also means that PHPUnit will be able to respect the strictness level of > file defining the data provider, etc. I've seen a number of complaints > about our handling of callbacks wrt strict_types, so I think it's > worthwhile to at least consider making such a change.
I think this makes sense. Is this something that can be achieved for PHP 7.2 or does it have to wait for PHP 7.3?
  100861
October 11, 2017 09:03 mail@pmmaga.net (=?UTF-8?Q?Pedro_Magalh=C3=A3es?=)
On Tue, Oct 10, 2017 at 5:53 PM, Nikita Popov ppv@gmail.com> wrote:
> > The problem are not internal function calls, the problem are callbacks. In > fact, the proposed fix does not actually fix the problem you encountered in > PHPUnit, as it is going to use the strictness mode at the reflection > call-site, not the strictness mode used by the file defining the data > provider. >
For those that didn't have a look at the PR, my goal was to try to ensure that if we are looking for which strictness to use, we should look at the place of the closest "user" call instead of the function (user or internal) that called the current one. The reason for this problem lies in the fact that `ZEND_ARG_USES_STRICT_TYPES()` will simply look at the caller regardless of what it is. Although this is enough to allow calling functions that were defined as non-strict in a strict manner, it does cause this kind of shortcomings with callbacks.
> I believe that the proper way to fix this is to handle dynamic function > invocations differently from direct invocations. Direct invocations should > use the strictness level of the call-site, while dynamic invocations should > use the strictness level of the declaration-site.
I agree that this should be about dynamic vs direct instead of user vs internal but IMHO, making the strictness vary from call-site to declaration-site depending on that may be a bit too confusing. Would it be possible/reasonable to try to find the place where the dynamic call was started? (i.e. the dataProvider, the mapped function for a userland array_map, etc...) Regards, Pedro
  100862
October 11, 2017 11:48 me@kelunik.com (Niklas Keller)
2017-10-11 11:03 GMT+02:00 Pedro Magalhães <mail@pmmaga.net>:

> On Tue, Oct 10, 2017 at 5:53 PM, Nikita Popov ppv@gmail.com> > wrote: > > > > The problem are not internal function calls, the problem are callbacks. > In > > fact, the proposed fix does not actually fix the problem you encountered > in > > PHPUnit, as it is going to use the strictness mode at the reflection > > call-site, not the strictness mode used by the file defining the data > > provider. > > > > For those that didn't have a look at the PR, my goal was to try to ensure > that if we are looking for which strictness to use, we should look at the > place of the closest "user" call instead of the function (user or internal) > that called the current one. The reason for this problem lies in the fact > that `ZEND_ARG_USES_STRICT_TYPES()` will simply look at the caller > regardless of what it is. Although this is enough to allow calling > functions that were defined as non-strict in a strict manner, it does cause > this kind of shortcomings with callbacks. > > > > I believe that the proper way to fix this is to handle dynamic function > > invocations differently from direct invocations. Direct invocations > should > > use the strictness level of the call-site, while dynamic invocations > should > > use the strictness level of the declaration-site. >
I'm not sure about that. That might be reasonable for closures, but not for other dynamic invocations.
> I agree that this should be about dynamic vs direct instead of user vs > internal but IMHO, making the strictness vary from call-site to > declaration-site depending on that may be a bit too confusing. Would it be > possible/reasonable to try to find the place where the dynamic call was > started? (i.e. the dataProvider, the mapped function for a userland > array_map, etc...) >
I don't think that's reasonably possible. Think about some event emitter like that: ``` class EventEmitter { private array $callbacks = []; public function on(string $eventName, callable $callback): void { $this->callbacks[$eventName][] = $callback; } public function emit(string $eventName, $value): void { foreach ($this->callbacks[$eventName] ?? [] as $callback) { $callback($value); } } } ``` Where is a dynamic call to a registered callback started? Depending on the strictness level of the caller of `emit()`? What if that `emit()` is called from another file with another strictness level? Regards, Niklas
  100914
October 22, 2017 19:13 ajf@ajf.me (Andrea Faulds)
Hi Nikita,

Nikita Popov wrote:
> I believe that the proper way to fix this is to handle dynamic function > invocations differently from direct invocations. Direct invocations should > use the strictness level of the call-site, while dynamic invocations should > use the strictness level of the declaration-site. This means that the > internal array_map and a userland reimplementation will behave consistent. > It also means that PHPUnit will be able to respect the strictness level of > file defining the data provider, etc. I've seen a number of complaints > about our handling of callbacks wrt strict_types, so I think it's > worthwhile to at least consider making such a change.
I agree that callbacks are one of the rough edges of strict_types. I've always been hoping we'd get some type of callable typing as a means of solving that, but it hasn't happened yet. As for your specific suggestion, I don't see the benefit of it. If anything, it could make one particular problem worse, namely that of libraries not written with type-hinted callbacks in mind. Calls to such libraries are more likely to blow up if suddenly those calls are made with strict types because the callbacks were declared in a strict_types file. I also dislike the inconsistency of making dynamic calls work differently. Thanks. -- Andrea Faulds https://ajf.me/