Everything

php

Strict type declarations not enforced for Reflection API invocation

php.internals

Sebastian Bergmann

8 years ago

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.

lists@rhsoft.net

8 years ago

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

Christoph Becker

8 years ago

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

lists@rhsoft.net

8 years ago

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

Niklas Keller

8 years ago

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

lists@rhsoft.net

8 years ago

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

Christoph Becker

8 years ago

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

Nikita Popov

8 years ago

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

Sebastian Bergmann

8 years ago

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?

Pedro Magalhães

8 years ago

On Tue, Oct 10, 2017 at 5:53 PM, Nikita Popov <nikita.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

Niklas Keller

8 years ago

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

Andrew Faulds

8 years ago

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/