Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed GH-16236: Fixed a bug in BcMath\Number::pow() and bcpow() when raising negative powers of 0. #16694

Closed
wants to merge 4 commits into from
Closed

Fixed GH-16236: Fixed a bug in BcMath\Number::pow() and bcpow() when raising negative powers of 0. #16694

wants to merge 4 commits into from
+147 −20

Conversation

SakiTakamachi
Copy link
Member

@SakiTakamachi SakiTakamachi commented Nov 4, 2024 •
edited
Loading

Fixes #16236

The existing function bcpow() returns 0 when it is a negative power of 0. Changing this specification is a BC Break, so will not do so in this PR.

bcpow() should be modified in a similar way.

The BcMath\Number class is a class added in 8.4, so an error will be generated.

@SakiTakamachi SakiTakamachi changed the base branch from master to PHP-8.4 November 4, 2024 07:49
Girgias
Girgias reviewed Nov 4, 2024
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do wonder if we shouldn't have this throw even in 8.4 for bcpow, as this feels like a bugfix to me.

ext/bcmath/libbcmath/src/bcmath.h Outdated
@@ -173,7 +173,7 @@ typedef enum {

raise_mod_status bc_raisemod(bc_num base, bc_num exponent, bc_num mod, bc_num *result, size_t scale);

void bc_raise(bc_num base, long exponent, bc_num *resul, size_t scale);
bool bc_raise(bc_num num1, long exponent, bc_num *result, size_t scale);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to continue using base

ext/bcmath/tests/gh16236.phpt Outdated
Comment on lines 7 to 12
/**
* The existing bcpow() specification returns 0 for negative powers.
* This is mathematically incorrect and will need to be changed to raise an error at some point.
* This test is to ensure the existing specifications until the specifications are changed.
*/
bcpow('0', '-2');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a stub of this to https://wiki.php.net/rfc/deprecations_php_8_5 so it is not forgotten, or follow-up PR to make this emit an E_WARNING or Throwing on master.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Var_dup the result of this to see what the output is?

@SakiTakamachi
Copy link
Member Author

Indeed, hard to see any value in getting 0 here...

OK, I'll fix it

@SakiTakamachi
Copy link
Member Author

Done & fixed var name

nielsdos
nielsdos requested changes Nov 4, 2024
View reviewed changes
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix itself looks fine, but please see comment about the test

ext/bcmath/tests/bcpow.phpt Outdated
@@ -6,7 +6,35 @@ bcmath
bcmath.scale=0
--FILE--
<?php
require(__DIR__ . "/run_bcmath_tests_function.inc");
// slightly modified run_bcmath_tests_function.inc for bcpow testing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to not copy&paste the code. Instead, modify run_bcmath_tests_function.inc to catch exceptions,and if there is one, just print the exception message instead of the result. This will reduce code duplication and would still test these cases. It also generalises nicer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed it in 2e44b1f

nielsdos
nielsdos approved these changes Nov 5, 2024
View reviewed changes
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jorgsowa
Copy link
Contributor

jorgsowa commented Nov 5, 2024 •
edited
Loading

Shouldn't this be deprecated first? It's basically the same case as with pow() https://wiki.php.net/rfc/raising_zero_to_power_of_negative_number

@SakiTakamachi
Copy link
Member Author

hm...
How do you think?
@Girgias

@SakiTakamachi
Copy link
Member Author

@jorgsowa

I thought about this for a bit.

For pow(), 0 ** -1 is INF. I can understand why the result is INF.

But bcpow() gives 0. This is definitely wrong and cannot have been intentional.

Therefore, I would consider this a "bug fix" rather than a "feature change" and expect it to be fixed in 8.4.

@Girgias
Copy link
Member

Girgias commented Nov 6, 2024

Returning 0 is definitely a bug, as that doesn't really make any sense.
Extensions have (well, at least used to have) more leeway in what they can do compared to core/standard/spl as they are less generic and the impact is more restrained.

In any case, considering we already have an RFC with unanimous in support for basically this case, I don't see the value of running this again past internals. Ideally, this should have been caught by the RFC in question but note that GMP already throws a ValueError in this case and used to return a warning and false in PHP 5/7: https://3v4l.org/NdnvS#veol

Aside: it feels like the RFC for standard should have been an E_WARNING rather than a deprecation, but that is neither here or there.

@jorgsowa
Copy link
Contributor

jorgsowa commented Nov 6, 2024

Therefore, I would consider this a "bug fix" rather than a "feature change" and expect it to be fixed in 8.4.

I agree that's a bug, but removing it without any deprecation will cause an impact on the user's code.

There were cases in the past where such fixes were causing problems:
#13866
#16641

Fixing such issues without deprecations puts PHP in an unreliable position.

@Girgias
Copy link
Member

Girgias commented Nov 6, 2024

Fixing such issues without deprecations puts PHP in an unreliable position.

Fixing bugs is not an unreliable position. If an upgrade finds a bug by shouting early, then it is a positive. This mentality that incorrect code needs to continue working really needs to go away.

@SakiTakamachi
Copy link
Member Author

In a little while days I plan to merge this.

@SakiTakamachi SakiTakamachi changed the title Fixed GH-16236: Fixed a bug in BcMath\Number::pow() when raising negative powers of 0. Fixed GH-16236: Fixed a bug in BcMath\Number::pow() and bcpow() when raising negative powers of 0. Nov 19, 2024
SakiTakamachi added a commit that referenced this pull request Nov 19, 2024
@SakiTakamachi
Merge branch 'PHP-8.4'
9b3af02 
* PHP-8.4:
  Fixed a bug in BcMath\Number::pow() and bcpow() when raising negative powers of 0. (#16694)
SakiTakamachi added a commit that referenced this pull request Nov 19, 2024
@SakiTakamachi
[skip ci] NEWS for #16694
fa36346
SakiTakamachi added a commit that referenced this pull request Nov 19, 2024
@SakiTakamachi
Merge branch 'PHP-8.4'
09d25b2 
* PHP-8.4:
  [skip ci] NEWS for #16694
@SakiTakamachi SakiTakamachi deleted the fix/gh16236 branch November 29, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Girgias Girgias Girgias left review comments

@nielsdos nielsdos nielsdos approved these changes

Assignees
No one assigned
Labels
Extension: bcmath
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Segmentation fault (access null pointer) in ext/bcmath/libbcmath/src/rmzero.c:50
4 participants
@SakiTakamachi @jorgsowa @Girgias @nielsdos