Re: [RFC] Saner string to number comparisons

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 > On 15 Jul 2020, at 10:53, Nikita Popov <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 >> <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 >