Improved up/downgrading breaks LedgerSMB::PGNumber about p5-math-bigint

[ehuelsmann]

Solved this as an issue on our side. Sorry for the noise.

[pjacklam]

The methods in Math::BigFloat should, by default, never return an object of a different class. The only two exceptions are the methods mantissa() and exponent(), which unfortunately return Math::BigInt objects, but they have done so since long before version 1.999818.

If downgrading is enabled, things work differently. If Math::BigFloat is configured to downgrade to Some::Class, then any operation that returns and integer, will convert this integer to a Some::Class object. However, from what I can see in your code, you never enable downgrading, and you don’t seem to be using the bignum module, which enables downgrading from Math::BigFloat to Math::BigInt. Because of this, I find it puzzling that Math::BigFloat returns something that isn’t a Math::BigFloat.

I have not been able to install and run LedgerSMB. It would be very helpful if you could narrow it down (e.g., which method is failing), and I will have a look at it.

[pjacklam]

Ah, I didn't see that you have solved this issue already, so never mind my previous comment. But I am glad you were able to solve it.

[ehuelsmann]

I have no idea how this happened, but up- and downgrading were enabled, outside of what I think happens in our codebase. No idea where the downgrades are coming from, but: setting downgrading explicitly to undef solved the issue.

[pjacklam]

I noticed the latest commit in the LedgerSMB repo, and I am glad this solves the issue, but I really wonder why you experienced the issue. I see nothing in your codebase that enables up- and downgrading, nor do I see anything in Math::BigFloat that should cause this, so this is really puzzling. I see that LedgerSMB has some dependencies which I will take a look at. There might be something there. In any case, I am glad this specific issue is solved.

[ehuelsmann]

I'm afraid that that commit wasn't enough; our 1.11.0 Docker image has the problem. I'll be looking to see if I can have a sequence of steps to reproduce the problem in our application. It manifests itself in multiple places: Numbers are assumed to be PGNumber-s, which means they're expected to have a to_output() method. One of the errors we're seeing is "no such method 'to_output' with object of type Math::BigFloat". We're also seeing Moose slot value violations where slot values are expected to be PGNumber-typed, however, the software suddenly tries to fit Math::BigFloat (or BigInt) types in. Switching back to 1.999818 immediately resolves these problems.

I'll try to produce a complete reproduction recipe on our code base; after that I could probably use some help trying to reduce the reproduction to something small enough for you to work on...

[ehuelsmann]

I found the culprit: Locale::CLDR: it uses bignum!

[ehuelsmann]

Well, that is: the fact that Math::BigFloat/Math::BigInt now has improved up/downgrading in combination with the fact that Locale::CLDR uses 'bignum' and the fact that LedgerSMB uses its own numeric class LedgerSMB::PGNumber is a toxic combination.

[ehuelsmann]

Ok. So, to work around this, I wanted to load bignum before overriding the up- and downgrade settings. This doesn't seem to have the desired effect, since I'm still seeing the incorrect values show up resulting in the same error (indicating that bignum still gets imported and still overwrites the global up- and downgrade values).

[pjacklam]

I am glad you found culprit. However, I believe the real problem is the fundamentally flawed object oriented design of the Math::Big* modules. The original author of these modules didn‘t use a proper object oriented design. The use of global variables is one of the design flaws. I believe that one solution to the issue you are experiencing is to fix this, which is also mentioned in this old CPAN RT: https://rt.cpan.org/Ticket/Display.html?id=78097 Fixing this is not a trivial task, but I am more worried about backwards compatibility. If fixing this CPAN RT breaks backwards compatibility in an unacceptable way, then I will probably have to release a whole new distribution. It might be worth it, though, regardless if this specific issue.

I have thought a lot about the issue you are experiencing and have a few suggestions:

• If you use no bignum; in your code, does that fix the issue?
• If you edit the installed Locale::CLDR::Locales::* modules and replace use bignum with use bigint or use bigfloat, does that fix the issue? Neither bigint nor bigfloat enables upgrading or downgrading. If this solves your issue, perhaps it is possible to convice the author of Locale::CLDR to do the same switch, depending on whether Locale::CLDR needs big integer support or big floating point support.

[ehuelsmann]

I believe the real problem is the fundamentally flawed object oriented design of the Math::Big* modules.

So do I. I don't think there's much you can do about that while remaining backward compatible. So, I'm thinking the fix on our side - which I can only implement on the next round of big releases (because we just released a big one), is to create my own class which wraps Math::BigFloat instead of inheriting from it. That way, BigFloat and bignum can do what they want/need, but my code won't see the direct effects of it, since the wrapped value is expected to be a Math::* type.

[ehuelsmann]

I'm going to look at your suggestions, but have a terrible busy week this week. I'll let you know what comes out of them as soon as I've tried.

[ehuelsmann]

• If you edit the installed Locale::CLDR::Locales::* modules and replace use bignum with use bigint or use bigfloat, does that fix the issue? Neither bigint nor bigfloat enables upgrading or downgrading. If this solves your issue, perhaps it is possible to convice the author of Locale::CLDR to do the same switch, depending on whether Locale::CLDR needs big integer support or big floating point support.

I had a look at this one (by way of the author of Locale::CLDR releasing a trial version) and it works like a charm!

[ehuelsmann]

@ThePilgrim released a new version of Locale::CLDR (v0.34.2) which doesn't use 'bignum' and works like a charm (verified by the original reporter on our software).