Bug #96329 for Math-BigInt: () breaks non-foreign classes

Sun Jun 08 07:58:03 2014 jpeacock [...] cpan.org - Ticket created

Subject:

() breaks non-foreign classes

Math::Currency is a fully functional Math::BigFloat class that handles the specific rounding rules that mathematical operations with currency requires. It has just come to my attention that a change in Math::BigInt: http://perl5.git.perl.org/perl.git/commitdiff/66a0495875e8130c45cac4fabd5f8d05f2f4c372 have broken certain operations due to changes in objectify(): https://rt.cpan.org/Ticket/Display.html?id=96254 In particular, your assumption that $class->as_float() should return an Math::BigFloat instance is the core of the problem. This method is undocumented in Math::BigInt/Float, but is used by Math::Currency to return a string representation of the correctly rounded value, not a Math::BigFloat object (as you evidently assumed) A simple change to Math::BigInt will DTRT: diff --git a/dist/Math-BigInt/lib/Math/BigInt.pm b/dist/Math-BigInt/lib/Math/BigInt.pm index 238d5f3..c2371ff 100644 --- a/dist/Math-BigInt/lib/Math/BigInt.pm +++ b/dist/Math-BigInt/lib/Math/BigInt.pm @@ -2673,7 +2673,7 @@ sub objectify { # If it is an object of the right class, all is fine. - if ($ref eq $a[0]) { + if ($ref->isa($a[0])) { next; } It is never a good idea to check for an exact class name; instead you should trust inheritance.

Tue Jun 10 08:40:25 2014 peter.john.acklam [...] gmail.com - Correspondence added

Speaking of good ideas, had you run "prove -l" in the .../dist/Math-BigInt directory, you would have seen that your "fix" causes more than 1000 tests to fail. I'll see what I can do, but it would be nice if you could provide a test case, preferably as simple as possible, that illustrates the problem. (I tried to install the latest Math::Currency, but some tests fail, so I'm not sure I can trust it.)

Tue Jun 10 08:40:26 2014 The RT System itself - Status changed from 'new' to 'open'

Thu Jun 12 20:47:16 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #96329] () breaks non-foreign classes
Date:	Thu, 12 Jun 2014 20:47:04 -0400
To:	bug-Math-BigInt [...] rt.cpan.org, jpeacock [...] cpan.org
From:	John Peacock <john.peacock [...] havurah-software.org>

On 06/10/2014 08:40 AM, Peter John Acklam via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=96329 > > > Speaking of good ideas, had you run "prove -l" in the .../dist/Math-BigInt directory, you would have seen that your "fix" causes more than 1000 tests to fail. > > I'll see what I can do, but it would be nice if you could provide a test case, preferably as simple as possible, that illustrates the problem. (I tried to install the latest Math::Currency, but some tests fail, so I'm not sure I can trust it.) >

You're right, that was very lazy of me. I'll try and produce two patches: 1) a test that fails and 2) a patch that fixes it. John

Sat Jun 14 12:06:48 2014 peter.john.acklam [...] gmail.com - Correspondence added

On Thu Jun 12 20:47:16 2014, john.peacock@havurah-software.org wrote: Show quoted text

> I'll try and produce two patches: 1) a test that fails and > 2) a patch that fixes it.

I have a fix. We must make sure $ref is defined, so the following slight modification works: if ($ref && $ref->isa($a[0])) { However, there are other problems with objectify(), so I will submit a patch that hopefully fixes them. Alas, objectify() is not well documented and does not have many tests, so it is too easy to break objectify() without it being caught by a test.

Mon Jun 16 07:26:22 2014 jpeacock [...] cpan.org - Correspondence added

On Sat Jun 14 12:06:48 2014, pjacklam wrote: Show quoted text

> I have a fix. We must make sure $ref is defined, so the following > slight modification works: > > if ($ref && $ref->isa($a[0])) { >

I think the attached is better. Since you are already checking the undef case, reordering the tests will make sure that you are only trying to call ->isa() on an actual object. I haven't been able to get a test written (not enough time for hacking these days). I'll still try and work on it as I can... John

Subject:

BigInt.diff

diff --git a/dist/Math-BigInt/lib/Math/BigInt.pm b/dist/Math-BigInt/lib/Math/BigInt.pm index 238d5f3..d66e7d4 100644 --- a/dist/Math-BigInt/lib/Math/BigInt.pm +++ b/dist/Math-BigInt/lib/Math/BigInt.pm @@ -2669,20 +2669,14 @@ sub objectify { } for my $i (1 .. $count) { - my $ref = ref $a[$i]; - - # If it is an object of the right class, all is fine. - - if ($ref eq $a[0]) { - next; - } - # Don't do anything with undefs. unless (defined($a[$i])) { next; } + my $ref = ref $a[$i]; + # Perl scalars are fed to the appropriate constructor. unless ($ref) { @@ -2696,6 +2690,12 @@ sub objectify { next; } + # If it is an object of the right class, all is fine. + + if ($ref -> isa($a[0])) { + next; + } + # If we want a Math::BigInt, see if the object can become one. # Support the old misnomer as_number().

Mon Jun 16 08:40:57 2014 peter.john.acklam [...] gmail.com - Correspondence added

I submitted a similar patch yesterday to perlbug@perl.org with subject "[PATCH] CPAN RT 96254 and 96329: Correct the handling of subclasses." I tested the patch with Math::Currency. Without the patch, I was able to reproduce the error. With the patch, the output looked OK.

Tue Jun 17 04:49:53 2014 peter.john.acklam [...] gmail.com - Correspondence added

Patch sent upstream: https://rt.perl.org/Ticket/Display.html?id=122118

Fri Oct 26 10:03:29 2018 peter.john.acklam [...] gmail.com - Correspondence added

Fixed by a patch applied to blead on 24 Jun 2014.

Fri Oct 26 10:03:30 2018 peter.john.acklam [...] gmail.com - Status changed from 'open' to 'resolved'