Is it deliberate that we are not providing the parameter types for
internal functions via reflection? It seems inconsistent:
<?php
declare(strict_types=1);
function ustrlen(string $str) { }
$param_ustrlen = (new
ReflectionFunction('ustrlen'))->getParameters()[0];
$param_strlen = (new ReflectionFunction('strlen'))->getParameters()[0];
echo "$param_ustrlen (".$param_ustrlen->hasType().")\n";
echo "$param_strlen (".$param_strlen->hasType().")\n";
try {
ustrlen(1);
} catch (TypeError $e) {
echo $e->getMessage()."\n";
}
try {
strlen(1);
} catch (TypeError $e) {
echo $e->getMessage()."\n";
}
The output is:
Parameter #0 [ <required> string $str ] (1)
Parameter #0 [ <required> $str ] ()
Argument 1 passed to ustrlen() must be of the type string, integer
given, called in /home/rasmus/prop.php on line 11
strlen() expects parameter 1 to be string, integer given
That is, in both cases a TypeError exception is raised because the type
of the parameter is incorrect. But hasType() on the internal function
parameter claims there is no type even though there obviously is one.
-Rasmus
Is it deliberate that we are not providing the parameter types for
internal functions via reflection? It seems inconsistent:<?php declare(strict_types=1); function ustrlen(string $str) { } $param_ustrlen = (newReflectionFunction('ustrlen'))->getParameters()[0];
$param_strlen = (new ReflectionFunction('strlen'))->getParameters()[0];echo "$param_ustrlen (".$param_ustrlen->hasType().")\n"; echo "$param_strlen (".$param_strlen->hasType().")\n"; try { ustrlen(1); } catch (TypeError $e) { echo $e->getMessage()."\n"; } try { strlen(1); } catch (TypeError $e) { echo $e->getMessage()."\n"; }The output is:
Parameter #0 [ <required> string $str ] (1)
Parameter #0 [ <required> $str ] ()
Argument 1 passed to ustrlen() must be of the type string, integer
given, called in /home/rasmus/prop.php on line 11
strlen()expects parameter 1 to be string, integer givenThat is, in both cases a TypeError exception is raised because the type
of the parameter is incorrect. But hasType() on the internal function
parameter claims there is no type even though there obviously is one.-Rasmus
The difference here is that ustrlen() has an argument type specified in
arginfo, while strlen() triggers an error through zend_parse_parameters().
We have practically no arginfo-level type annotations in the standard
library.
Nikita
The difference here is that ustrlen() has an argument type specified in
arginfo, whilestrlen()triggers an error through
zend_parse_parameters(). We have practically no arginfo-level type
annotations in the standard library.
Obviously we can't look into a function to check what it is passing to
ZPP, but maybe ZPP could be made smarter and have it fill in the arginfo
if it isn't there. That sounds slow though.
Or we bite the bullet and fix the arginfo everywhere to make it
consistent. It wouldn't be that hard to write a script to generate the
bulk of it.
I just don't like the fact that a parameter can raise a type error when
it claims to not have a type. It makes writing static analysis and other
type checking tools much more complicated.
-Rasmus
Hi
2015-06-21 20:58 GMT+02:00 Rasmus Lerdorf rasmus@lerdorf.com:
Or we bite the bullet and fix the arginfo everywhere to make it
consistent. It wouldn't be that hard to write a script to generate the
bulk of it.
I think fixing arginfo sounds like the right solution, writing a
script for it should not be so trival.
--
regards,
Kalle Sommer Nielsen
kalle@php.net
Hi
2015-06-21 20:58 GMT+02:00 Rasmus Lerdorf rasmus@lerdorf.com:
Or we bite the bullet and fix the arginfo everywhere to make it
consistent. It wouldn't be that hard to write a script to generate the
bulk of it.I think fixing arginfo sounds like the right solution, writing a
script for it should not be so trival.
Even the ones that have some arginfo aren't all correct.
For example:
https://github.com/php/php-src/blob/master/Zend/zend_builtin_functions.c#L132
which currently says:
ZEND_BEGIN_ARG_INFO_EX(arginfo_define, 0, 0, 3)
ZEND_ARG_INFO(0, constant_name)
ZEND_ARG_INFO(0, value)
ZEND_ARG_INFO(0, case_insensitive)
ZEND_END_ARG_INFO()
That says there are 3 required params when it should clearly be 2.
But really that block should be:
ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_define, 0, 2, _IS_BOOL,
0, 0)
ZEND_ARG_TYPE_INFO(0, constant_name, IS_STRING, 0)
ZEND_ARG_INFO(0, value)
ZEND_ARG_TYPE_INFO(0, case_insensitive, _IS_BOOL, 0)
ZEND_END_ARG_INFO()
It seems way too lazy to not fix these before we release 7.0. Especially
the ones that are outright wrong. So, having called us lazy, I better
dig in and do something about it. I'll fix up all of the ones in
zend_builtin_functions.c today. And move on to others after. Definitely
a tedious task that we could use some extra hands on.
-Rasmus
Hi
2015-06-21 20:58 GMT+02:00 Rasmus Lerdorf rasmus@lerdorf.com:
Or we bite the bullet and fix the arginfo everywhere to make it
consistent. It wouldn't be that hard to write a script to generate the
bulk of it.I think fixing arginfo sounds like the right solution, writing a
script for it should not be so trival.Even the ones that have some arginfo aren't all correct.
For example:
https://github.com/php/php-src/blob/master/Zend/zend_builtin_functions.c#L132
which currently says:
ZEND_BEGIN_ARG_INFO_EX(arginfo_define, 0, 0, 3)
ZEND_ARG_INFO(0, constant_name)
ZEND_ARG_INFO(0, value)
ZEND_ARG_INFO(0, case_insensitive)
ZEND_END_ARG_INFO()That says there are 3 required params when it should clearly be 2.
But really that block should be:
ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_define, 0, 2, _IS_BOOL,
0, 0)
ZEND_ARG_TYPE_INFO(0, constant_name, IS_STRING, 0)
ZEND_ARG_INFO(0, value)
ZEND_ARG_TYPE_INFO(0, case_insensitive, _IS_BOOL, 0)
ZEND_END_ARG_INFO()It seems way too lazy to not fix these before we release 7.0. Especially
the ones that are outright wrong. So, having called us lazy, I better
dig in and do something about it. I'll fix up all of the ones in
zend_builtin_functions.c today. And move on to others after. Definitely
a tedious task that we could use some extra hands on.
Before making more extensive use of arginfo types, I think we should first
do some adjustments to the way they are handled. In particular adding types
means that internal fcalls will take the branch in
http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_def.h#3669. As this is hot
code, it is likely important to avoid doing those duplicate type checks
(that will also happen in zpp anyway). As such I would suggest to make
arginfo for internal functions reflection-only. Return type hints are
already only enforced in debug mode.
Nikita
Hi!
Before making more extensive use of arginfo types, I think we should first
do some adjustments to the way they are handled. In particular adding types
means that internal fcalls will take the branch in
http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_def.h#3669. As this is hot
code, it is likely important to avoid doing those duplicate type checks
(that will also happen in zpp anyway). As such I would suggest to make
arginfo for internal functions reflection-only. Return type hints are
already only enforced in debug mode.
Agreed, for internals arginfo types should be reflection-only. I imagine
that would be very easy to fix just by checking for function type?
--
Stas Malyshev
smalyshev@gmail.com
Hi!
Before making more extensive use of arginfo types, I think we should first
do some adjustments to the way they are handled. In particular adding types
means that internal fcalls will take the branch in
http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_def.h#3669. As this is hot
code, it is likely important to avoid doing those duplicate type checks
(that will also happen in zpp anyway). As such I would suggest to make
arginfo for internal functions reflection-only. Return type hints are
already only enforced in debug mode.Agreed, for internals arginfo types should be reflection-only. I imagine
that would be very easy to fix just by checking for function type?
Ok, I have fixed the arginfo for all the built-in engine functions.
I wrote a little script which generates it from the doc-comments in the
code. I think the easiest way to do it is to first go through the file
and check the "{{{ proto" comments to make sure the optional args are
denoted correctly and no args are missing.
To run it:
script/dev/genarginfo.php Zend/zend_builtin_functions.c
for example.
-Rasmus
Hi Rasmus,
Your latest changes broke more than 100 tests.
Sorry, but I had to revert your commits and related Bob's fixes.
You may find them in internal-arg-info branch.
Please, don't do experiments on the common code base at beta stage.
On the other hand, I don't see a big problem committing this, when a
complete solution is ready.
Thanks. Dmitry.
Hi!
Before making more extensive use of arginfo types, I think we should
first
do some adjustments to the way they are handled. In particular adding
types
means that internal fcalls will take the branch in
http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_def.h#3669. As this is
hot
code, it is likely important to avoid doing those duplicate type checks
(that will also happen in zpp anyway). As such I would suggest to make
arginfo for internal functions reflection-only. Return type hints are
already only enforced in debug mode.Agreed, for internals arginfo types should be reflection-only. I imagine
that would be very easy to fix just by checking for function type?Ok, I have fixed the arginfo for all the built-in engine functions.
I wrote a little script which generates it from the doc-comments in the
code. I think the easiest way to do it is to first go through the file
and check the "{{{ proto" comments to make sure the optional args are
denoted correctly and no args are missing.To run it:
script/dev/genarginfo.php Zend/zend_builtin_functions.c
for example.
-Rasmus
Hi Rasmus,
Your latest changes broke more than 100 tests.
Sorry, but I had to revert your commits and related Bob's fixes.
You may find them in internal-arg-info branch.Please, don't do experiments on the common code base at beta stage.
On the other hand, I don't see a big problem committing this, when a
complete solution is ready.
Oops, way more test failures than I expected. But I don't understand
some of them. For example, Zend/tests/constants_008.phpt which is:
<?php
define('a', 2);
const a = 1;
if (defined('a')) {
print a;
}
With arginfo filled in correctly for define() and defined() the output is:
../../sapi/cli/php -derror_reporting=-1 constants_008.php
Notice: Constant a already defined in Zend/tests/constants_008.php on line 4
1
And without arginfo:
php -derror_reporting=-1 constants_008.php
Notice: Constant a already defined in Zend/tests/constants_008.php on line 4
2
I wasn't expecting fixing arginfo would change how constants behave.
-Rasmus
Hi Rasmus,
Your latest changes broke more than 100 tests.
Sorry, but I had to revert your commits and related Bob's fixes.
You may find them in internal-arg-info branch.Please, don't do experiments on the common code base at beta stage.
On the other hand, I don't see a big problem committing this, when a
complete solution is ready.Oops, way more test failures than I expected. But I don't understand
some of them. For example, Zend/tests/constants_008.phpt which is:<?php define('a', 2); const a = 1; if (defined('a')) { print a; }With arginfo filled in correctly for
define()anddefined()the output is:../../sapi/cli/php -derror_reporting=-1 constants_008.php
Notice: Constant a already defined in Zend/tests/constants_008.php on line
4
1And without arginfo:
php -derror_reporting=-1 constants_008.php
Notice: Constant a already defined in Zend/tests/constants_008.php on line
4
2I wasn't expecting fixing arginfo would change how constants behave.
Actually, when you added type hints, php compiler stopped optimizing and
using ZEND_DEFINED (and others) instead of internal function calls, but of
course this shouldn't change behavior. I'll need to take a look into
difference.
Thanks. Dmitry.
-Rasmus
Actually, when you added type hints, php compiler stopped optimizing and
using ZEND_DEFINED (and others) instead of internal function calls, but
of course this shouldn't change behavior. I'll need to take a look into
difference.
Right, behaviour shouldn't change, but couldn't we also make it so that
arginfo for internal calls is only used by reflection? We can do all the
type checking we need right in ZPP as we do now. Let's just ignore
arginfo entirely for these calls.
-Rasmus
Actually, when you added type hints, php compiler stopped optimizing and
using ZEND_DEFINED (and others) instead of internal function calls, but
of course this shouldn't change behavior. I'll need to take a look into
difference.Right, behaviour shouldn't change, but couldn't we also make it so that
arginfo for internal calls is only used by reflection? We can do all the
type checking we need right in ZPP as we do now. Let's just ignore
arginfo entirely for these calls.
In general, we can, if we remove corresponding code in zend_vm_def.h, and
also some checks for ZEND_HAS_TYPE_INFO and internal functions in
zend_compile.c.
Thanks. Dmitry.
-Rasmus
On Mon, Jun 22, 2015 at 8:35 PM, Rasmus Lerdorf <rasmus@lerdorf.com
mailto:rasmus@lerdorf.com> wrote:> Actually, when you added type hints, php compiler stopped optimizing and > using ZEND_DEFINED (and others) instead of internal function calls, but > of course this shouldn't change behavior. I'll need to take a look into > difference. Right, behaviour shouldn't change, but couldn't we also make it so that arginfo for internal calls is only used by reflection? We can do all the type checking we need right in ZPP as we do now. Let's just ignore arginfo entirely for these calls.In general, we can, if we remove corresponding code in zend_vm_def.h,
and also some checks for ZEND_HAS_TYPE_INFO and internal functions in
zend_compile.c.
I think that is the way to go. We didn't have any type info there before
so we are not losing any functionality, and obviously adding it produces
weird side effects and slows things down. Plus we should get a very
small performance increase by removing the type info check entirely.
-Rasmus
Hi,
-----Original Message-----
From: Rasmus Lerdorf [mailto:rasmus@lerdorf.com]
Sent: Monday, June 22, 2015 9:10 PM
To: Dmitry Stogov
Cc: Bob Weinand; Anatol Belski; Kalle Sommer Nielsen; Stanislav Malyshev;
Nikita Popov; PHP internals
Subject: Re: [PHP-DEV] hasType() for internal function parameters?On Mon, Jun 22, 2015 at 8:35 PM, Rasmus Lerdorf <rasmus@lerdorf.com
mailto:rasmus@lerdorf.com> wrote:> Actually, when you added type hints, php compiler stopped optimizing and > using ZEND_DEFINED (and others) instead of internal function calls, but > of course this shouldn't change behavior. I'll need to take a look into > difference. Right, behaviour shouldn't change, but couldn't we also make it so that arginfo for internal calls is only used by reflection? We can do all the type checking we need right in ZPP as we do now. Let's just ignore arginfo entirely for these calls.In general, we can, if we remove corresponding code in zend_vm_def.h,
and also some checks for ZEND_HAS_TYPE_INFO and internal functions in
zend_compile.c.I think that is the way to go. We didn't have any type info there before so we are
not losing any functionality, and obviously adding it produces weird side effects
and slows things down. Plus we should get a very small performance increase by
removing the type info check entirely.
Are we talking about including this in 7.0?
-- If so - do you think we can have it ready short after alpha2?
---- If yes - great, let's do it.
---- If no - lets defer it for 7.1.
While I'm still confident we can stabilize it in a beta if there's a strong support, but better we're "boringly stable" in 7.0 than "shiny all in". Some side effect we can stabilize in beta? For the core? For external exts? It clearly suggests itself to check for a couple of step ahead.
Thanks
Anatol
Hi!
So I tried to remove the checks for ZEND_ACC_HAS_TYPE_HINTS:
https://github.com/php/php-src/pull/1354
Turns out there is a lot of tests that assume function with wrong
arguments throws Error, but ZPP and type checks work differently - ZPP
first checks argument number (and doesn't throw if number is wrong but
instead creates warning), while type checks (that shouldn't really
happen on internals but do) check types first and not check argument number.
So I wonder which way is the best to proceed here. Looks like this comes
from 5.6 where internal function types were verified in executor:
https://github.com/php/php-src/blob/PHP-5.6/Zend/zend_vm_def.h#L1974
I'm not sure what to do with it - on one side, I think ZPP should handle
it for internal functions, otherwise we're doing the same work twice. On
the other side, that would be pretty substantial, even if maybe correct,
BC break. Should we keep the ZEND_ACC_HAS_TYPE_HINTS type checks and
accept the fact that this way we're checking everything twice, or should
be clean it up?
--
Stas Malyshev
smalyshev@gmail.com
Hi!
So I tried to remove the checks for ZEND_ACC_HAS_TYPE_HINTS:
https://github.com/php/php-src/pull/1354Turns out there is a lot of tests that assume function with wrong
arguments throws Error, but ZPP and type checks work differently - ZPP
first checks argument number (and doesn't throw if number is wrong but
instead creates warning), while type checks (that shouldn't really
happen on internals but do) check types first and not check argument number.So I wonder which way is the best to proceed here. Looks like this comes
from 5.6 where internal function types were verified in executor:
https://github.com/php/php-src/blob/PHP-5.6/Zend/zend_vm_def.h#L1974I'm not sure what to do with it - on one side, I think ZPP should handle
it for internal functions, otherwise we're doing the same work twice. On
the other side, that would be pretty substantial, even if maybe correct,
BC break. Should we keep the ZEND_ACC_HAS_TYPE_HINTS type checks and
accept the fact that this way we're checking everything twice, or should
be clean it up?
Is it a BC break against real code though? Fixing tests isn't a big
deal. What kind of real code would break by turning calls with the wrong
number of args from an error to a warning?
I suppose it is a bit late in the game to fix this, but it really does
seem very broken to the point where I wonder if we shouldn't just
disable reflection on internal functions entirely since the arginfo is
so woefully incomplete. Plus the double-checking is super messy.
-Rasmus
Hi!
So I tried to remove the checks for ZEND_ACC_HAS_TYPE_HINTS:
https://github.com/php/php-src/pull/1354Turns out there is a lot of tests that assume function with wrong
arguments throws Error, but ZPP and type checks work differently - ZPP
first checks argument number (and doesn't throw if number is wrong but
instead creates warning), while type checks (that shouldn't really
happen on internals but do) check types first and not check argument number.So I wonder which way is the best to proceed here. Looks like this comes
from 5.6 where internal function types were verified in executor:
https://github.com/php/php-src/blob/PHP-5.6/Zend/zend_vm_def.h#L1974I'm not sure what to do with it - on one side, I think ZPP should handle
it for internal functions, otherwise we're doing the same work twice. On
the other side, that would be pretty substantial, even if maybe correct,
BC break. Should we keep the ZEND_ACC_HAS_TYPE_HINTS type checks and
accept the fact that this way we're checking everything twice, or should
be clean it up?Is it a BC break against real code though? Fixing tests isn't a big
deal. What kind of real code would break by turning calls with the wrong
number of args from an error to a warning?I suppose it is a bit late in the game to fix this, but it really does
seem very broken to the point where I wonder if we shouldn't just
disable reflection on internal functions entirely since the arginfo is
so woefully incomplete. Plus the double-checking is super messy.
One thing I really want to work towards in the future is consistency
between user-land and internal functions. There are currently multiple
observable differences. Maybe it would be best to post-pone these
changes until 8.0 where we can do a large consistency pass and handle
it then.
Hi!
Is it a BC break against real code though? Fixing tests isn't a
big deal. What kind of real code would break by turning calls with
the wrong number of args from an error to a warning?
Well, if we move to checking in ZPP only, then some of the errors that
previously were fatals (like iterator_array("")) would now become
warnings. In fact, even type mismatch is a warning for ZPP, which may
be unexpected, as it was a fatal before, so we may want to change that.
But in some situation, like argument number mismatch, we will have a
warning, where before it was a fatal error. Which isn't strictly BC
break but allows some code to pass which previously didn't and may
hide some bugs.
So we may want to be careful there.
Stas Malyshev
smalyshev@gmail.com
Hi!
Is it a BC break against real code though? Fixing tests isn't a
big deal. What kind of real code would break by turning calls with
the wrong number of args from an error to a warning?Well, if we move to checking in ZPP only, then some of the errors that
previously were fatals (like iterator_array("")) would now become
warnings. In fact, even type mismatch is a warning for ZPP, which may
be unexpected, as it was a fatal before, so we may want to change that.But in some situation, like argument number mismatch, we will have a
warning, where before it was a fatal error. Which isn't strictly BC
break but allows some code to pass which previously didn't and may
hide some bugs.So we may want to be careful there.
I suppose so, but this seems like a relatively minor level of BC
breakage especially for a major version bump.
-Rasmus
Hi!
Is it a BC break against real code though? Fixing tests isn't a
big deal. What kind of real code would break by turning calls with
the wrong number of args from an error to a warning?Well, if we move to checking in ZPP only, then some of the errors that
previously were fatals (like iterator_array("")) would now become
warnings. In fact, even type mismatch is a warning for ZPP, which may
be unexpected, as it was a fatal before, so we may want to change that.But in some situation, like argument number mismatch, we will have a
warning, where before it was a fatal error. Which isn't strictly BC
break but allows some code to pass which previously didn't and may
hide some bugs.So we may want to be careful there.
I suppose so, but this seems like a relatively minor level of BC
breakage especially for a major version bump.
I also think it is a rather minor problem. And to minimize the risk to get
such things accepted for 7.x I would rather go with 7.0.
We have time to catch some of the troubles if any until final.
-Rasmus
Levi Morrison wrote:
One thing I really want to work towards in the future is consistency
between user-land and internal functions. There are currently multiple
observable differences.
+1, as they say.
This is the goal we should be aiming for. Having differences between
userland and internal functions seems to be a more important problem
that should be solved compared to a lack of detail in reflection.
Stanislav Malyshev wrote:
Well, if we move to checking in ZPP only, then some of the errors that
previously were fatals (like iterator_array("")) would now become
warnings. In fact, even type mismatch is a warning for ZPP, which may
be unexpected, as it was a fatal before, so we may want to change that.
...
So we may want to be careful there.
I'm pretty sure we do need to be careful and think through the change.
Doing an RFC for this is usually the best way of doing that, rather
than just committing changes to master until something sticks.
If there was to be an RFC, some things to be discussed would be:
- Currently the arg info is separated from the actual parsing of the
parameters. While in a few cases this may be necessary, for the vast
majority of code it is not needed. Having the information be
duplicated is one of the main causes of the arg info not accurately
reflecting the parameters the function actually needs.
Wouldn't the best way to solve this (both now and to prevent future
'drift' of the arg info from the actual required args) be to allow the
arguments to be defined in one place, and then used by both the arg
declarations and the function ZPP, through the clever use of macros?
- How are we going to handle functions that use the ability of
internals to have wildly different acceptable sets of parameters. For
example https://github.com/php/php-src/blob/master/ext/intl/calendar/gregoriancalendar_methods.cpp#L82
can accept either "|Z!s!" or "lll|lll" for the parameters.
This is one of those places where an internal function can behave
differently from a userland function, and so the reflection in PHP
currently isn't powerful enough to give information about these cases.
- Parameter names - although I am personally not in favour of any of
the named parameters RFCs, other people are. One of the big chunks of
work that would be needed for this is to correct the names of the
parameters in arg info to be the same as they are in the manual (or
vice versa), as well as keep them 100% in sync. If people are serious
about wanting named parameters they probably ought to be in the
discussion to clean up the arg info, as otherwise the info would need
to be cleaned up twice, which could be a large duplication of work and
BC break.
cheers
Dan
Hi!
Actually, when you added type hints, php compiler stopped optimizing and
using ZEND_DEFINED (and others) instead of internal function calls, but
This probably needs to be fixed, there's no reason to stop optimizations
for internal functions even with arginfo types, since I think the
consensus is that these types are used only for reflection, so nothing
changed.
--
Stas Malyshev
smalyshev@gmail.com
Hi!
The difference here is that ustrlen() has an argument type specified in
arginfo, whilestrlen()triggers an error through zend_parse_parameters().
We have practically no arginfo-level type annotations in the standard
library.
I think we should start doing this. Of course, some functions have
complicated arguments and we can leave it alone for now, but those that
have a clear type we should make arginfo reflect it.
--
Stas Malyshev
smalyshev@gmail.com