[PHP-DEV] Breaking change of rounding behavior in PHP 8.4

Mailing Lists php.internals
1

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&gt;
[2] <Issues · php/php-src · GitHub;

Cheers,
Christoph

2

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&gt;
[2] <Issues · php/php-src · GitHub;

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

3

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

4

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] Feature GH-12143: Extend the maximum precision round can handle by one digit by SakiTakamachi · Pull Request #12222 · php/php-src · GitHub

[2] `number_format` no longer round properly big float · Issue #14332 · php/php-src · GitHub

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] PHP: rfc:change_the_edge_case_of_round

5

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] PHP: 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

6

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]: Incorrect `round($num, 0, PHP_ROUND_HALF_UP)` result for `$num = 1.4999999999999998 / 4503599627370495.5` · Issue #12143 · php/php-src · GitHub
[2]: Online PHP editor | output for 3Q7BC

Cheers,
Christoph

7

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]: Incorrect `round($num, 0, PHP_ROUND_HALF_UP)` result for `$num = 1.4999999999999998 / 4503599627370495.5` · Issue #12143 · php/php-src · GitHub
[2]: Online PHP editor | output for 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

8

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]: Incorrect `round($num, 0, PHP_ROUND_HALF_UP)` result for `$num = 1.4999999999999998 / 4503599627370495.5` · Issue #12143 · php/php-src · GitHub
[2]: Online PHP editor | output for 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

9

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]: Incorrect `round($num, 0, PHP_ROUND_HALF_UP)` result for `$num = 1.4999999999999998 / 4503599627370495.5` · Issue #12143 · php/php-src · GitHub
[2]: Online PHP editor | output for 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.

FYI, the test case for that value already exists :slight_smile:

Regards,

Saki