[PATCH] Consistent type names in error messages

2014-12-14 19:35 GMT+01:00 Andrea Faulds ajf@ajf.me:

Good evening,

Unfortunately, zend_parse_parameters and userland type hint error messages use outdated type names, and don’t even do so consistently. For example:

$ php -r 'fread(0, 0);'
PHP Warning: fread() expects parameter 1 to be resource, integer given in Command line code on line 1

$ php -r 'fread(fopen("data:text/plain,test", "r"), fopen("data:text/plain,test", "r"));'
PHP Warning: fread() expects parameter 2 to be long, resource given in Command line code on line 1

$ php -r 'function foo(foobar $x) {} foo(1.0);'
PHP Catchable fatal error: Argument 1 passed to foo() must be an instance of foobar, double given, called in Command line code on line 1 and defined in Command line code on line 1

Specifically, zend_parse_parameters will always “expect” a “long”, yet when the expected type isn’t an integer and an integer is passed, it says “integer given”. Alongside this, both userland type hint errors and zend_parse_parameters errors refer to “double” and not “float”.

I want to change the type names to be consistent, because I think our current inconsistency is confusing. Integers are sometimes ints or integers, but other times longs. Floats are sometimes floats, but other times doubles. If scalar type hints are ever added, then jettisoning the old aliases means we don’t have to add extra reserved words.

You forgot that they can also be 'real', but on a serious manner, I
don't see any reason not to make the error messages consistent and I
would say go ahead already.

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Good evening,

Unfortunately, zend_parse_parameters and userland type hint error messages use outdated type names, and don’t even do so consistently. For example:

$ php -r 'fread(0, 0);'
PHP Warning: fread() expects parameter 1 to be resource, integer given in Command line code on line 1

$ php -r 'fread(fopen("data:text/plain,test", "r"), fopen("data:text/plain,test", "r"));'
PHP Warning: fread() expects parameter 2 to be long, resource given in Command line code on line 1

$ php -r 'function foo(foobar $x) {} foo(1.0);'
PHP Catchable fatal error: Argument 1 passed to foo() must be an instance of foobar, double given, called in Command line code on line 1 and defined in Command line code on line 1

Specifically, zend_parse_parameters will always “expect” a “long”, yet when the expected type isn’t an integer and an integer is passed, it says “integer given”. Alongside this, both userland type hint errors and zend_parse_parameters errors refer to “double” and not “float”.

I want to change the type names to be consistent, because I think our current inconsistency is confusing. Integers are sometimes ints or integers, but other times longs. Floats are sometimes floats, but other times doubles. If scalar type hints are ever added, then jettisoning the old aliases means we don’t have to add extra reserved words.

I’ve made a patch which makes zend_parse_parameters and userland type hints consistently show “integer” and “float” rather than “long” and “double”: https://github.com/php/php-src/pull/955

I also wrote a GNU sed script which I used to update the tests: https://gist.github.com/TazeTSchnitzel/c0d780466def9f226318

Alongside error messages, there are some other places that could be changed. Obviously the return value of gettype() shouldn’t be changed, but I think that we should, for example, make is_long() an alias of is_int() and not the other way around.

Thoughts?

I had a go at this a few months ago, but haven't updated my patch based
on what's changed in the engine since, so it probably wouldn't merge
cleanly: https://github.com/php/php-src/pull/769

An additional case which it looks like you haven't covered is "Object of
class %s could not be converted to double" in zend_operators.c and
zend_object_handlers.c:
https://github.com/IMSoP/php-src/commit/8dd601ac39a4241c9f119fd2d3e0e46474980a53

Other than the awkward BC implications in gettype(), I see no reason
that the user should ever see the types labelled as "long" or "double"
in their output.

However, note that zend_get_type_by_const does get used in a handful of
non-error contexts, such as the ReflectionClass output shown in
ext/reflection/tests/bug29986.phpt
[https://github.com/TazeTSchnitzel/php-src/commit/c0ee87297298b4ff9b8d68a01f8bbb99f94d9a10#diff-241]
I had a grep around and couldn't see any that were likely to cause any
real concern, though.

Regards,

--
Rowan Collins
[IMSoP]

Am 14.12.2014 um 19:35 schrieb Andrea Faulds:

Thoughts?

+1 for consistency :)

I’ve made a patch which makes zend_parse_parameters and userland type hints consistently show “integer” and “float” rather than “long” and “double”: https://github.com/php/php-src/pull/955

I also wrote a GNU sed script which I used to update the tests: https://gist.github.com/TazeTSchnitzel/c0d780466def9f226318

Hi,

Is anyone here opposed to this, or should I just merge it? I don’t think it really needs an RFC. It doesn’t break BC.

Thanks.

Andrea Faulds
http://ajf.me/