# [PATCH] Consistent type names in error messages - Thread: https://externals.io/message/79637 - Original archive: https://news-web.php.net/php.internals/79637 - Started by Andrea Faulds on 2014-12-14T18:35:00+00:00 - Messages: 12 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #79637 — Andrea Faulds — 2014-12-14T18:35:00+00:00 URL: https://externals.io/message/79637#79637 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? -- Andrea Faulds http://ajf.me/ --- ## #79638 — Kalle Sommer Nielsen — 2014-12-14T18:41:39+00:00 URL: https://externals.io/message/79637#79638 · In reply to: #79637 2014-12-14 19:35 GMT+01:00 Andrea Faulds : > 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 --- ## #79639 — Andrea Faulds — 2014-12-14T18:44:39+00:00 URL: https://externals.io/message/79637#79639 · In reply to: #79638 Hi Kalle, > > > 2014-12-14 19:35 GMT+01:00 Andrea Faulds : > > […] > > 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. Aha, yes, I did forget that real is also an alias of float. I suppose this proves my point that all these aliases are confusing: I can’t even remember them all! Also, a sidenote: In case someone goes and checks the manual and tells me that `is_long()` is already an alias of `is_int()` and not the other way around, the manual lies. In fact, `is_int()` is currently an alias of `is_long()` in master. Thanks. -- Andrea Faulds http://ajf.me/ --- ## #79644 — Kalle Sommer Nielsen — 2014-12-14T19:59:58+00:00 URL: https://externals.io/message/79637#79644 · In reply to: #79639 2014-12-14 19:44 GMT+01:00 Andrea Faulds : > Hi Kalle, Hi Andrea > Also, a sidenote: In case someone goes and checks the manual and tells me that `is_long()` is already an alias of `is_int()` and not the other way around, the manual lies. In fact, `is_int()` is currently an alias of `is_long()` in master. > Simple fix: http://pastie.org/9780416 Alternatively I can update the manual to be accurate, but I believe that `is_long()` should be an alias of `is_int()` too, so I'll rather change the C source which it has no impact on userland anyway. -- regards, Kalle Sommer Nielsen kalle@php.net --- ## #79645 — Andrea Faulds — 2014-12-14T20:01:44+00:00 URL: https://externals.io/message/79637#79645 · In reply to: #79644 > > > 2014-12-14 19:44 GMT+01:00 Andrea Faulds : > > […] > > Hi Andrea > > […] > > Simple fix: http://pastie.org/9780416 > > Alternatively I can update the manual to be accurate, but I believe > that `is_long()` should be an alias of `is_int()` too, so I'll rather > change the C source which it has no impact on userland anyway. It wouldn’t break anything, I say make a pull request. -- Andrea Faulds http://ajf.me/ --- ## #79648 — Kalle Sommer Nielsen — 2014-12-14T21:08:31+00:00 URL: https://externals.io/message/79637#79648 · In reply to: #79645 Hi Andrea 2014-12-14 21:01 GMT+01:00 Andrea Faulds : > It wouldn’t break anything, I say make a pull request. See: http://news.php.net/php.cvs/83688 http://news.php.net/php.doc.cvs/12965 -- regards, Kalle Sommer Nielsen kalle@php.net --- ## #79649 — Rowan Collins — 2014-12-14T21:20:32+00:00 URL: https://externals.io/message/79637#79649 · In reply to: #79637 > 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] --- ## #79650 — Andrea Faulds — 2014-12-14T22:09:51+00:00 URL: https://externals.io/message/79637#79650 · In reply to: #79649 Hey Rowan, > > > 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 Yeah, I was inspired by your efforts. :) > 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 Good catch, I’ll handle that too. > 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. Yeah, that was the only manual test fix I did. That’s probably a small BC break for those people who are parsing __toString’s output. Thanks! -- Andrea Faulds http://ajf.me/ --- ## #79692 — Sebastian Bergmann — 2014-12-16T07:43:25+00:00 URL: https://externals.io/message/79637#79692 · In reply to: #79637 Am 14.12.2014 um 19:35 schrieb Andrea Faulds: > Thoughts? +1 for consistency :) --- ## #79822 — Andrea Faulds — 2014-12-20T15:50:58+00:00 URL: https://externals.io/message/79637#79822 · In reply to: #79637 > > > 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/ --- ## #79848 — Andrea Faulds — 2014-12-21T13:24:37+00:00 URL: https://externals.io/message/79637#79848 · In reply to: #79822 > > > > > […] > > 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. Hey, I’ve merged the patch into master, PHP 7 will have consistent type names in its error messages. :) Thanks. -- Andrea Faulds http://ajf.me/ --- ## #79853 — Paul Dragoonis — 2014-12-21T16:29:47+00:00 URL: https://externals.io/message/79637#79853 · In reply to: #79848 > > > > […] hints consistently show “integer” and “float” rather than “long” and “double”: https://github.com/php/php-src/pull/955 > > […] https://gist.github.com/TazeTSchnitzel/c0d780466def9f226318 > > […] think it really needs an RFC. It doesn’t break BC. > > […] > > Hey, > > I’ve merged the patch into master, PHP 7 will have consistent type names in its error messages. :) Thanks for the patch and the other contribs you've been doing lately. > > Thanks. > -- > Andrea Faulds > http://ajf.me/