Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:110812
MIME-Version: 1.0
References: <CAF+90c821boRO6YsO0SH0JbDb8E8eTHfEK=e4Y5_z9P7+OOnUg@mail.gmail.com>
 <CAJHtzny=SX_ojsohApAY8Xe7c_EMAKGbNe110QQVh7i5yGFm_A@mail.gmail.com>
 <CAF+90c-6iEBPjWuzA6=bAr=85N=JNV0+GqL3_maak3yqVP-e=Q@mail.gmail.com> <CAF+90c9DxVz2Ro3yJ0-jDuPXg5Ffe3F9ERE8wHt=7ERjOm01Sw@mail.gmail.com>
In-Reply-To: <CAF+90c9DxVz2Ro3yJ0-jDuPXg5Ffe3F9ERE8wHt=7ERjOm01Sw@mail.gmail.com>
Date: Thu, 2 Jul 2020 12:18:09 +0200
Message-ID: <CAF+90c_gpNVi=FToBxncLpNVzTSOX9wtG=q_A9GsY3qpD9iVcA@mail.gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000091f20905a972ba07"
Subject: Re: [PHP-DEV] [RFC] Saner string to number comparisons
From: nikita.ppv@gmail.com (Nikita Popov)

--00000000000091f20905a972ba07
Content-Type: text/plain; charset="UTF-8"

On Thu, Jul 2, 2020 at 10:09 AM Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Mon, Mar 4, 2019 at 6:00 PM Nikita Popov <nikita.ppv@gmail.com> wrote:
>
>> On Wed, Feb 27, 2019 at 10:23 AM Zeev Suraski <zeev@php.net> wrote:
>>
>>>
>>>
>>> On Tue, Feb 26, 2019 at 2:27 PM Nikita Popov <nikita.ppv@gmail.com>
>>> wrote:
>>>
>>>> Hi internals,
>>>>
>>>> I think it is well known that == in PHP is a pretty big footgun. It
>>>> doesn't
>>>> have to be. I think that type juggling comparisons in a language like
>>>> PHP
>>>> have some merit, it's just that the particular semantics of == in PHP
>>>> make
>>>> it so dangerous. The biggest WTF factor is probably that 0 == "foobar"
>>>> returns true.
>>>>
>>>> I'd like to bring forward an RFC for PHP 8 to change the semantics of ==
>>>> and other non-strict comparisons, when used between a number and a
>>>> string:
>>>>
>>>> https://wiki.php.net/rfc/string_to_number_comparison
>>>>
>>>> The tl;dr is that if you compare a number and a numeric string, they'll
>>>> be
>>>> compared as numbers. Otherwise, the number is converted into a string
>>>> and
>>>> they'll be compared as strings.
>>>>
>>>> This is a very significant change -- not so much because the actual BC
>>>> breakage is expected to be particularly large, but because it is a
>>>> silent
>>>> change in core language semantics, which makes it hard to determine
>>>> whether
>>>> or not code is affected by the change. There are things we can do about
>>>> this, for example the RFC suggests that we might want to have a
>>>> transition
>>>> mode where we perform the comparison using both the old and the new
>>>> semantics and warn if the result differs.
>>>>
>>>> I think we should give serious consideration to making such a change.
>>>> I'd
>>>> be interested to hear whether other people think this is worthwhile, and
>>>> how we could go about doing it, while minimizing breakage.
>>>>
>>>
>>> I generally like the direction and think we should seriously consider it.
>>>
>>> I think that before we make any decisions on this, or even dive too deep
>>> into the discussion - we actually need to implement this behavior,
>>> including the proposed INI setting you mentioned we might add in 7.4 - and
>>> see what happens in some real world apps, at least in terms of potential
>>> danger (as you say, figuring out whether there's actual breakage would
>>> require a full audit of every potentially problematic sample.  Ultimately,
>>> I think there's no question that if we were to start from scratch, we'd be
>>> going for something along these lines.  But since we're not starting from
>>> scratch - scoping the level of breakage is key here.
>>>
>>> Zeev
>>>
>>
>> Totally agree that assessing the amount of breakage in real code is key
>> here. I have now implemented a warning for PHP 7.4 (for now unconditional,
>> no ini setting) that is thrown whenever the result of a comparison is going
>> to change under the currently proposed rules:
>> https://github.com/php/php-src/pull/3917
>>
>> I've done a few initial tests by running this against the Laravel,
>> Symfony and pear-core. The warning was thrown 2 times for Laravel, 1 times
>> for Symfony and 2 times for pear-core. (See PR for the results.)
>>
>> Both of the warnings in pear-core pointed to implementation bugs. The
>> Symfony warning was due to trailing whitespace not being allowed in numeric
>> strings (something we should definitely change). One of the Laravel
>> warnings is ultimately a false-positive (does not change behavior), though
>> code could be improved to avoid it. I wasn't able to tell whether the other
>> one is problematic, as it affects sorting order.
>>
>> I have to say that this is a lot less warnings than I expected. Makes me
>> wonder if I didn't make an implementation mistake ^^
>>
>> Regards,
>> Nikita
>>
>
> As we're moving closer to PHP 8 feature freeze, I want to give this RFC a
> bump. I've updated the text to account for some changes that have happened
> in the meantime, such as the removal of locale-sensitivity for float to
> string conversions.
>
> It's been quite a while since we discussed this last, and back then the
> discussion was fairly positive. Some experiments with a warning mode also
> showed that the impact, at least in framework/library code, appears to be
> fairly low in practice, contrary to what one might intuitively expect.
>
> Now would be the time to decide whether or not we want to pursue this
> change for PHP 8.
>

Thinking about the detailed behavior of the RFC, I think it might be better
to change the semantics to:

> If one of the operands is NaN, return uncomparable (1).
> Otherwise, convert the int/float operand to string with precision=-1 and
compare both operands as strings (with the usual "smart" logic).

This differs from what the RFC currently specifies only in the behavior of
comparison between a float and a non-numeric strings (because the string
representation of the float currently depends on precision), but I think
specifying it this way simplifies the mental model behind it, and makes the
intended semantics more obvious. The actual implementation would of course
be closer to what is specified in the RFC right now.

Regards,
Nikita

--00000000000091f20905a972ba07--