Re: [RFC] Saner string to number comparisons

Hey Nikita Thanks for the rebase. I just tested this on one of our most largest projects (after verifying that the warning does show in a dummy test case), and all is fine. So from my point of view, there is a theoretical chance of breaking code, but I believe this won't have a large impact, at least not on modern-day projects. Kind regards Brent > On 15 Jul 2020, at 11:10, Nikita Popov <nikita.ppv@gmail.com> wrote: > > On Wed, Jul 15, 2020 at 10:56 AM Brent Roose <brendt@stitcher.io <'З�R�hL > �)�z��mailto:brendt@stitcher.io>> wrote: > >> Hi Nikita >> >> Yes that would be nice, if it's not too much of a hassle. I'm only able to >> test this in one or two large Laravel projects, so it would still be a >> limited test. >> >> Kind regards >> Brent >> > > Done! https://github.com/php/php-src/pull/3917 > <https://github.com/php/php-src/pull/3917> is now based on > current 7.4 > HEAD. Note that it just unconditionally throws a warning, without a way to > disable it. > > Nikita > >> On 15 Jul 2020, at 10:53, Nikita Popov <nikita.ppv@gmail.com <� >> ����Iۯ�T7�K�Bmailto:nikita.ppv@gmail.com>> wrote: >> >> On Wed, Jul 15, 2020 at 10:49 AM Brent Roose <brendt@stitcher.io >> <mailto:brendt@stitcher.io>> wrote: >> >>> Hi Nikita >>> >>> Is the ini setting available in current 7.4 builds? Is it documented >>> somewhere? I'd like to test this change in some of our projects. >>> >> >> We did not introduce an ini setting in PHP 7.4, I only used it for my own >> experiments. The implementation is available at >> https://github.com/php/php-src/pull/3917 >> <https://github.com/php/php-src/pull/3917>. I could rebase >> that to current >> 7.4 if that would be useful. >> >> Nikita >> >>> On 15 Jul 2020, at 10:28, Nikita Popov <nikita.ppv@gmail.com <���i�_��� >>> W(mailto:nikita.ppv@gmail.com>> wrote: >>> >>> On Tue, Jul 14, 2020 at 11:47 PM Björn Larsson <bjorn.x.larsson@telia.com >>> <mailto:bjorn.x.larsson@telia.com> >>>> >>> wrote: >>> >>> Den 2020-07-14 kl. 15:48, skrev Nikita Popov: >>> >>> On Thu, Jul 2, 2020 at 10:09 AM Nikita Popov <nikita.ppv@gmail.com >>> <mailto:nikita.ppv@gmail.com>> >>> >>> wrote: >>> >>> >>> On Mon, Mar 4, 2019 at 6:00 PM Nikita Popov <nikita.ppv@gmail.com >>> <mailto:nikita.ppv@gmail.com>> >>> >>> wrote: >>> >>> >>> On Wed, Feb 27, 2019 at 10:23 AM Zeev Suraski <zeev@php.net >>> <mailto:zeev@php.net>> wrote: >>> >>> >>> On Tue, Feb 26, 2019 at 2:27 PM Nikita Popov <nikita.ppv@gmail.com >>> <mailto: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 >>> <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 >>> <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. >>> >>> And then there was silence... >>> >>> I think I'll just put this up for vote on Friday, and we'll see what >>> >>> people >>> >>> think :) >>> >>> Nikita >>> >>> >>> Seems like a very good idea!! Especially in conjunction with the RFC: >>> - https://wiki.php.net/rfc/saner-numeric-strings >>> <https://wiki.php.net/rfc/saner-numeric-strings> >>> >>> Btw, in the RFC there is a reference to the "Trailing whitespace in >>> numeric >>> strings" RFC. Update to reference "Saner numeric strings" RFC instead? >>> >>> >>> Thanks, I've updated the link to point to the new proposal! >>> >>> Nikita