Everything

php

Home » Project » Newsgroups » php.internals » Breaking change of rounding behavior in PHP 8.4 »

Breaking change of rounding behavior in PHP 8.4

php.internals

Christoph Becker

2 years ago

Hi all,

there has been a "small" change in the rounding logic of
_php_math_round()[1] a couple of months ago.  A respective ticket,
claiming the new behavior would be a bug[2] had been filed a while ago
without proper triage so far.  I'm bringing this to your attention,
because I'm afraid that there will be (many) more bug reports about this
in the future (note that the change only affects master, and only PHP
8.4.0alpha1 has been released yet, so it is probably not widespreadly
tested yet).  At the very least we should be sure that we want to keep
this change, and document it well, to avoid discussions in every filed
ticket.

My personal stance on this is simple: floating point arithmetic is not
exact per se, and changes to how PHP handles the details should only be
introduced, if they clearly improve things.  This change apparently
improves some cases, but worsens others.  Thus, I think the change
should better be reverted, and if at all, postponed to PHP 9, since I
neither regard this change as bug fix nor as a feature.

What do you think?

[1] <https://github.com/php/php-src/pull/12222>
[2] <https://github.com/php/php-src/issues/14332>

Cheers,
Christoph

Saki Takamachi

2 years ago

Hi Christoph,

> ﻿Hi all,
>
> there has been a "small" change in the rounding logic of
> _php_math_round()[1] a couple of months ago.  A respective ticket,
> claiming the new behavior would be a bug[2] had been filed a while ago
> without proper triage so far.  I'm bringing this to your attention,
> because I'm afraid that there will be (many) more bug reports about this
> in the future (note that the change only affects master, and only PHP
> 8.4.0alpha1 has been released yet, so it is probably not widespreadly
> tested yet).  At the very least we should be sure that we want to keep
> this change, and document it well, to avoid discussions in every filed
> ticket.
>
> My personal stance on this is simple: floating point arithmetic is not
> exact per se, and changes to how PHP handles the details should only be
> introduced, if they clearly improve things.  This change apparently
> improves some cases, but worsens others.  Thus, I think the change
> should better be reverted, and if at all, postponed to PHP 9, since I
> neither regard this change as bug fix nor as a feature.
>
> What do you think?
>
> [1] <https://github.com/php/php-src/pull/12222>
> [2] <https://github.com/php/php-src/issues/14332>
>
> Cheers,
> Christoph

As you know, I am the implementer of that change.

At the time, I thought "just add another digit," but this may have been a more disruptive change than I realized.

I'm not opposed to reverting this, but I don't have any strong opinions on whether it should be reverted.

I'd like to hear other people's opinions as well.

Regards,

Saki

Rob Landers

2 years ago

On Fri, Jul 12, 2024, at 13:24, Christoph M. Becker wrote:

> Hi all,
>
> there has been a "small" change in the rounding logic of
> _php_math_round()[1] a couple of months ago.  A respective ticket,
> claiming the new behavior would be a bug[2] had been filed a while ago
> without proper triage so far.  I'm bringing this to your attention,
> because I'm afraid that there will be (many) more bug reports about this
> in the future (note that the change only affects master, and only PHP
> 8.4.0alpha1 has been released yet, so it is probably not widespreadly
> tested yet).  At the very least we should be sure that we want to keep
> this change, and document it well, to avoid discussions in every filed
> ticket.
>
> My personal stance on this is simple: floating point arithmetic is not
> exact per se, and changes to how PHP handles the details should only be
> introduced, if they clearly improve things.  This change apparently
> improves some cases, but worsens others.  Thus, I think the change
> should better be reverted, and if at all, postponed to PHP 9, since I
> neither regard this change as bug fix nor as a feature.
>
> What do you think?
>
> [1] <https://github.com/php/php-src/pull/12222>
> [2] <https://github.com/php/php-src/issues/14332>
>
> Cheers,
> Christoph
>

I guess the real question is: how many of these numbers should be the same?

https://3v4l.org/tgRMc/rfc#vgit.master

— Rob

Gina P. Banyard

2 years ago

On Friday, 12 July 2024 at 12:24, Christoph M. Becker <cmbecker69@gmx.de> wrote:

> Hi all,
>
> there has been a "small" change in the rounding logic of
> _php_math_round()[1] a couple of months ago. A respective ticket,
> claiming the new behavior would be a bug[2] had been filed a while ago
> without proper triage so far. I'm bringing this to your attention,
> because I'm afraid that there will be (many) more bug reports about this
> in the future (note that the change only affects master, and only PHP
> 8.4.0alpha1 has been released yet, so it is probably not widespreadly
> tested yet). At the very least we should be sure that we want to keep
> this change, and document it well, to avoid discussions in every filed
> ticket.
>
> My personal stance on this is simple: floating point arithmetic is not
> exact per se, and changes to how PHP handles the details should only be
> introduced, if they clearly improve things. This change apparently
> improves some cases, but worsens others. Thus, I think the change
> should better be reverted, and if at all, postponed to PHP 9, since I
> neither regard this change as bug fix nor as a feature.
>
> What do you think?
>
> [1] https://github.com/php/php-src/pull/12222
>
> [2] https://github.com/php/php-src/issues/14332

I agree, this situation is extremely suboptimal.

My understanding as to why people declined the "Change the edge case of round()" RFC [1] was because they were worried about the silent change in behaviour.
However, most people seem to have missed the fact, myself included, that regardless of it being accepted or not there would have been changes to the behaviour.
As such we have the worst of both worlds, continuing to have incorrect floating point semantics for people that rely on proper IEEE 754 floating points,
and yet we still have a silent change in behaviour due to the partial bugfix.

Moreover, these fixes made the implementation of round() more complicated, for marginal benefits IMHO.

Best regards,

Gina P. Banyard

[1] https://wiki.php.net/rfc/change_the_edge_case_of_round

Saki Takamachi

2 years ago

Hi Gina,

> I agree, this situation is extremely suboptimal.
>
> My understanding as to why people declined the "Change the edge case of round()" RFC [1] was because they were worried about the silent change in behaviour.
> However, most people seem to have missed the fact, myself included, that regardless of it being accepted or not there would have been changes to the behaviour.
> As such we have the worst of both worlds, continuing to have incorrect floating point semantics for people that rely on proper IEEE 754 floating points,
> and yet we still have a silent change in behaviour due to the partial bugfix.
>
> Moreover, these fixes made the implementation of round() more complicated, for marginal benefits IMHO.
>
> Best regards,
>
> Gina P. Banyard
>
> [1] https://wiki.php.net/rfc/change_the_edge_case_of_round

Opinion on this seems to be more divided than I expected.

I think the fairest approach would be to revert the change that extended the range of rounding to one more digit and hold a RFC vote.

Note that it doesn't take 2/3 to take down this change, it takes 2/3 to keep this change.

How do you think?

Regards,

Saki

Claude Pache

2 years ago

> Le 12 juil. 2024 à 13:24, Christoph M. Becker <cmbecker69@gmx.de> a écrit :
>
> Hi all,
>
> there has been a "small" change in the rounding logic of
> _php_math_round()[1] a couple of months ago.  A respective ticket,
> claiming the new behavior would be a bug[2] had been filed a while ago
> without proper triage so far.  I'm bringing this to your attention,
> because I'm afraid that there will be (many) more bug reports about this
> in the future (note that the change only affects master, and only PHP
> 8.4.0alpha1 has been released yet, so it is probably not widespreadly
> tested yet).  At the very least we should be sure that we want to keep
> this change, and document it well, to avoid discussions in every filed
> ticket.
>
> My personal stance on this is simple: floating point arithmetic is not
> exact per se, and changes to how PHP handles the details should only be
> introduced, if they clearly improve things.  This change apparently
> improves some cases, but worsens others.  Thus, I think the change
> should better be reverted, and if at all, postponed to PHP 9, since I
> neither regard this change as bug fix nor as a feature.
>
> What do you think?
>
> [1] <https://github.com/php/php-src/pull/12222>
> [2] <https://github.com/php/php-src/issues/14332>
>
> Cheers,
> Christoph

Hi,

See [1] and [2], which motivated the change.

I expect that `round($anything) == (int) round($anything)` for any float between PHP_INT_MIN and PHP_INT_MAX, and I think that the change fixed that .

Therefore, I am for keeping the change, and ensuring that there are test cases for 4503599627370495.5 (the largest float with fractional part).

—Claude

[1]: https://github.com/php/php-src/issues/12143#issuecomment-1713465981
[2]: https://3v4l.org/3Q7BC

Christoph Becker

2 years ago

Hi!

On 12.07.2024 at 17:26, Claude Pache wrote:

>
>> Le 12 juil. 2024 à 13:24, Christoph M. Becker <cmbecker69@gmx.de> a écrit :
>>
>> […]  At the very least we should be sure that we want to keep
>> this change, and document it well, to avoid discussions in every filed
>> ticket.
>>
>> […] Thus, I think the change
>> should better be reverted, and if at all, postponed to PHP 9, since I
>> neither regard this change as bug fix nor as a feature.
>
> See [1] and [2], which motivated the change.

Ah, thank you!  I probably should have checked this more thouroughly;
now even I can see that there was a *bug*, so it is okay for me to stick
with the fix (thank you, Saki!), but maybe UPGRADING should clarify the
issue a bit.  Currently, it states:

| The maximum precision that can be handled by round() has been extended
| by one digit.

While that is technically correct, probably few readers understand the
implications.

> I expect that `round($anything) == (int) round($anything)` for any float between PHP_INT_MIN and PHP_INT_MAX, and I think that the change fixed that .

It seems to me that this is a reasonable expectation.

> Therefore, I am for keeping the change, and ensuring that there are test cases for 4503599627370495.5 (the largest float with fractional part).

Makes sense (unless there is already such a test, of course).

> [1]: https://github.com/php/php-src/issues/12143#issuecomment-1713465981
> [2]: https://3v4l.org/3Q7BC

Cheers,
Christoph

Saki Takamachi

2 years ago

Hi Christoph,

> ﻿Hi!
>
>> On 12.07.2024 at 17:26, Claude Pache wrote:
>>
>>>> Le 12 juil. 2024 à 13:24, Christoph M. Becker <cmbecker69@gmx.de> a écrit :
>>>
>>> […]  At the very least we should be sure that we want to keep
>>> this change, and document it well, to avoid discussions in every filed
>>> ticket.
>>>
>>> […] Thus, I think the change
>>> should better be reverted, and if at all, postponed to PHP 9, since I
>>> neither regard this change as bug fix nor as a feature.
>>
>> See [1] and [2], which motivated the change.
>
> Ah, thank you!  I probably should have checked this more thouroughly;
> now even I can see that there was a *bug*, so it is okay for me to stick
> with the fix (thank you, Saki!), but maybe UPGRADING should clarify the
> issue a bit.  Currently, it states:
>
> | The maximum precision that can be handled by round() has been extended
> | by one digit.
>
> While that is technically correct, probably few readers understand the
> implications.
>
>> I expect that `round($anything) == (int) round($anything)` for any float between PHP_INT_MIN and PHP_INT_MAX, and I think that the change fixed that .
>
> It seems to me that this is a reasonable expectation.
>
>> Therefore, I am for keeping the change, and ensuring that there are test cases for 4503599627370495.5 (the largest float with fractional part).
>
> Makes sense (unless there is already such a test, of course).
>
>> [1]: https://github.com/php/php-src/issues/12143#issuecomment-1713465981
>> [2]: https://3v4l.org/3Q7BC
>
> Cheers,
> Christoph

If this can be considered a bug fix, then I'm in favor of keeping it as is. (I wasn't sure if this should be considered a feature addition.)

I will update UPGRADING with concrete examples.

Regards,

Saki

Christoph Becker

2 years ago

Hi Saki!

On 13.07.2024 at 15:16, Saki Takamachi wrote:

>
>>> On 12.07.2024 at 17:26, Claude Pache wrote:
>>>
>>> See [1] and [2], which motivated the change.
>>
>> Ah, thank you!  I probably should have checked this more thouroughly;
>> now even I can see that there was a *bug*, so it is okay for me to stick
>> with the fix (thank you, Saki!), […]
>>
>>> [1]: https://github.com/php/php-src/issues/12143#issuecomment-1713465981
>>> [2]: https://3v4l.org/3Q7BC
>
> If this can be considered a bug fix, then I'm in favor of keeping it as is. (I wasn't sure if this should be considered a feature addition.)

Well, if I call round() and tell it to round to zero decimals, and it
doesn't do it (assuming precision=-1), that looks like a bug to me.

> I will update UPGRADING with concrete examples.

Thank you!

Cheers,
Christoph

Saki Takamachi

2 years ago

Hi Christoph,

> 2024/07/13 22:30、Christoph M. Becker <cmbecker69@gmx.de>のメール:
>
> Hi Saki!
>
> On 13.07.2024 at 15:16, Saki Takamachi wrote:
>>
>>>> On 12.07.2024 at 17:26, Claude Pache wrote:
>>>>
>>>> See [1] and [2], which motivated the change.
>>>
>>> Ah, thank you!  I probably should have checked this more thouroughly;
>>> now even I can see that there was a *bug*, so it is okay for me to stick
>>> with the fix (thank you, Saki!), […]
>>>
>>>> [1]: https://github.com/php/php-src/issues/12143#issuecomment-1713465981
>>>> [2]: https://3v4l.org/3Q7BC
>>
>> If this can be considered a bug fix, then I'm in favor of keeping it as is. (I wasn't sure if this should be considered a feature addition.)
>
> Well, if I call round() and tell it to round to zero decimals, and it
> doesn't do it (assuming precision=-1), that looks like a bug to me.
>
>> I will update UPGRADING with concrete examples.
>
> Thank you!
>
> Cheers,
> Christoph

I opened the PR.
https://github.com/php/php-src/pull/14943

FYI, the test case for that value already exists :)
https://github.com/php/php-src/blob/520787bb93990b95d9cf6eaf063ca2b03b76a87f/ext/standard/tests/math/round_gh12143_expand-rounding-target.phpt#L14

Regards,

Saki