Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:79649
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.43 as permitted sender)
Message-ID: <548DFF20.6090709@gmail.com>
Date: Sun, 14 Dec 2014 21:20:32 +0000
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0
MIME-Version: 1.0
To: internals@lists.php.net
References: <2F9B99EF-538C-4225-95A4-943F57DADF69@ajf.me>
In-Reply-To: <2F9B99EF-538C-4225-95A4-943F57DADF69@ajf.me>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [PATCH] Consistent type names in error messages
From: rowan.collins@gmail.com (Rowan Collins)

On 14/12/2014 18:35, Andrea Faulds wrote:
> 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]