Strict type declarations not enforced for Reflection API invocation

6 years ago by lists@rhsoft.net — view source

unread

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

6 years ago by Christoph M. Becker — view source

unread

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:

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

6 years ago by lists@rhsoft.net — view source

unread

Am 10.10.2017 um 16:57 schrieb Christoph M. Becker:

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

6 years ago by Niklas Keller — view source

unread

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:

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

6 years ago by lists@rhsoft.net — view source

unread

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:

    

        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

6 years ago by Christoph M. Becker — view source

unread

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

6 years ago by Nikita Popov — view source

unread

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-

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

6 years ago by Sebastian Bergmann — view source

unread

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?

6 years ago by mail@pmmaga.net — view source

unread

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

6 years ago by Niklas Keller — view source

unread

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

6 years ago by Andrea Faulds — view source

unread

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/