overloading fallback for + and += is broken #12223
Comments
From @mhaschThis is a bug report for perl from mhasch@cpan.org, Test case: perl -MMath::BigInt -le '$a = 2; $a -= Math::BigInt->new(1); print $a' This should print 1 but prints -1 under perl-5.17.0 and perl-5.17.1. The root cause seems to be that Math::BigInt, as of version Math::BigInt states this assumption in a comment in the @@ dist/Math-BigInt/lib/Math/BigInt.pm, lines 48..62 @@ Although code like this might seem dangerous at first glance, @@ lib/overload.pm, section "Assignments", lines 425..435 @@ This documentation seems now no longer accurate, as can be use overload ( This prints 1.19 and "not ok" with perl-5.17.0 up to blead, To repair this, either the documentation of "overload" should In either case it may prove useful to add something along the -Martin Flags: Site configuration information for perl 5.17.1: Configured by cpants at Mon Jun 25 09:05:30 CEST 2012. Summary of my perl5 (revision 5 version 17 subversion 1) configuration: Locally applied patches: @INC for perl 5.17.1: Environment for perl 5.17.1: |
From @doyOn Mon, Jun 25, 2012 at 09:53:15AM -0700, mhasch@cpan.org wrote:
I agree that this is a bug. Bisecting shows this: $ perl ../bisect.pl --start=v5.16.0 -- ./perl -Ilib ~/test7.pl f041cf0 is almost certainly the issue (5f9f83 just fixes a minor build issue commit f041cf0 Lookup overloaded assignment operators when trying to swap the arguments This is in the case where we search for an overloaded operator when At the very beginning of Perl_amagic_call, if the flag AMGf_noleft is This change only necessitates a minor adjustment in lib/overload.t, So it sounds like the issue was that 1 += $overloaded wasn't falling -doy |
|
The RT System itself - Status changed from 'new' to 'open' |
From @doyOn Fri, Jun 29, 2012 at 10:53:25AM -0500, Jesse Luehrs wrote:
Okay, so fixing this doesn't appear to be as straightforward as it In 5.16, "string" .= $overloaded worked fine because it would fall back f041cf0 had a small error (it started checking for .= overloading on What we're left with then is a situation where it's impossible to make -doy |
From @doyOn Fri, Jun 29, 2012 at 01:50:35PM -0500, Jesse Luehrs wrote:
Work in progress pushed to doy/overload_fallback_fix_113834. -doy |
From @khwilliamsonOn 06/29/2012 12:50 PM, Jesse Luehrs wrote:
That commit was there because it was needed by mktables; reverting may |
From @doyOn Fri, Jun 29, 2012 at 01:45:08PM -0600, Karl Williamson wrote:
It's needed by mktables because string interpolation is converted into a $res = "foo " . $foo; The case relevant to this issue is the fourth line there, $res .= $bar, -doy |
From mhasch-cpanbugs@cozap.comOn Fri, Jun 29, 2012 at 11:51:19AM -0700, Jesse Luehrs via RT wrote:
The way "overload" has been documented up to now, assignment It escapes me how somebody might want to override ".=" while not I'd argue whether the "fallback" directive must necessarily A solution in order to help an exotic object that does in fact And anyway, I should prefer to change this script rather than -Martin |
From @doyOn Tue, Jul 03, 2012 at 01:57:01PM +0200, Martin Becker wrote:
I agree - assignment operators looking at the right operand is clearly
This would be an argument for returning to the 5.16 behavior. I do think
The mktables thing can easily be fixed, that isn't really the point. The
Yes, I don't think that Math::BigInt is doing anything wrong here. Any -doy |
From @khwilliamsonOn 07/04/2012 03:38 PM, Jesse Luehrs wrote:
I haven't been following this very closely, but fallback => 0 should |
From @doyOn Wed, Jul 04, 2012 at 05:41:15PM -0600, Karl Williamson wrote:
It may have fixed it, but it also broke other things(: The question is: in the case of "$normal .= $overloaded", where -doy |
From @mhaschMy previous post does not seem to have made it into the ticket. On Fri, Jul 06, 2012 at 06:20:44PM +0200, Martin Becker wrote:
The Item "Assignments" in overload's POD could be extended Inline Patchdiff -rU6 perl-5.16.0.orig/lib/overload.pm perl-5.16.0/lib/overload.pm
--- perl-5.16.0.orig/lib/overload.pm 2012-04-25 02:18:34.000000000 +0200
+++ perl-5.16.0/lib/overload.pm 2012-07-09 10:59:40.000000000 +0200
@@ -431,13 +431,16 @@
For example, the operation
$a *= $b
cannot lead to C<$b>'s implementation of C<*=> being called,
even if C<$a> is a scalar.
-(It can, however, generate a call to C<$b>'s method for C<*>).
+It can, however, generate a call to C<$b>'s method for C<*>.
+An assignment operator is mapped to a normal binary operator
+if it is overloaded from the perspective of the right hand side.
+This mapping is not subject to the fallback mode.
=item * I<Non-mutators with a mutator variant>
+ - * / % ** << >> x .
& | ^
Martin |
From @jkeenanOn Mon Jun 25 09:53:15 2012, mhasch@cpan.org wrote:
I tested Math-BigInt v1.99 today against blead. The output is in the There was extensive back-and-forth in this ticket and I don't think all However, if the module's 'make test' is passing, albeit with warnings, Thank you very much. |
From @jkeenanPERL_DL_NONLAZY=1 /Users/jimk/perl5/perlbrew/perls/perl-blead/bin/perl5.17.9 "-MExtUtils::Command::MM" "-e" "test_harness(0, 'inc', 'blib/lib', 'blib/arch')" t/*.t |
From @jkeenanOn Mon Jun 25 09:53:15 2012, mhasch@cpan.org wrote:
I would like to request some clarification about the version of Today I spent a considerable amount of time trying to test the latest However, I now realize that there is no version 1.998 available on CPAN. Can you clarify? Thank you very much. |
From mhasch-p5p@cozap.comOn Mon Feb 18 10:59:26 2013, jkeenan wrote:
Note that the issue is not caught by the test suite of Math-BigInt-1.997. Blead should have Math-BigInt-1.999 in its "dist" subdirectory. -Martin |
From @jkeenanOn Wed Feb 20 15:55:52 2013, mhasch wrote:
Please review the attached patch as a first attempt at writing Thank you very much. |
From @jkeenanFrom 7bd5a610eb0bfcb053647c2f005739a923007c32 Mon Sep 17 00:00:00 2001 Some will fail with overload v1.19. For: RT#113834 dist/Math-BigInt/t/rt-113834.t | 41 ++++++++++++++++++++++++++++++++++++++++ Inline Patchdiff --git a/dist/Math-BigInt/t/rt-113834.t b/dist/Math-BigInt/t/rt-113834.t
new file mode 100644
index 0000000..48b115d
--- /dev/null
+++ b/dist/Math-BigInt/t/rt-113834.t
@@ -0,0 +1,41 @@
+#!/usr/bin/perl
+# See RT #113834
+use strict;
+use warnings;
+use Math::BigInt;
+use Test::More tests => 6;
+
+my $x;
+
+$x = 4;
+$x += Math::BigInt->new(1);
+is($x, 5, "overloading of '+=' worked as expected");
+
+TODO: {
+ local $TODO = "RT #113834: overloading of '-=' faulty in overload 1.19";
+$x = 2;
+$x -= Math::BigInt->new(1);
+is($x, 1, "overloading of '-=' worked as expected");
+}
+
+$x = 7;
+$x *= Math::BigInt->new(3);
+is($x, 21, "overloading of '*=' worked as expected");
+
+TODO: {
+ local $TODO = "RT #113834: overloading of '/=' faulty in overload 1.19";
+$x = 24;
+$x /= Math::BigInt->new(6);
+is($x, 4, "overloading of '/=' worked as expected");
+}
+
+TODO: {
+ local $TODO = "RT #113834: overloading of '%=' faulty in overload 1.19";
+$x = 24;
+$x %= Math::BigInt->new(7);
+is($x, 3, "overloading of '%=' worked as expected");
+
+$x = 24;
+$x %= Math::BigInt->new(6);
+is($x, 0, "overloading of '%=' worked as expected");
+}
--
1.6.3.2 |
From mhasch-p5p@cozap.comOn Wed Feb 20 18:57:27 2013, jkeenan wrote:
Thank you for your suggestion. It is a good idea to I have put together a more complete test based on your With perl-5.17.9, 22 of 33 checks in this test fail. Note that Math-BigInt does not override all assigment -Martin |
From mhasch-p5p@cozap.comperl-5.17.9-BigInt-MHASCH-01.patchdiff -Nrup perl-5.17.9.orig/dist/Math-BigInt/t/rt-113834.t perl-5.17.9/dist/Math-BigInt/t/rt-113834.t
--- perl-5.17.9.orig/dist/Math-BigInt/t/rt-113834.t 1970-01-01 01:00:00.000000000 +0100
+++ perl-5.17.9/dist/Math-BigInt/t/rt-113834.t 2013-02-21 14:06:52.000000000 +0100
@@ -0,0 +1,102 @@
+#!/usr/bin/perl -w
+# [perl #113834] overloaded assignment operator semantics
+
+use strict;
+use Test::More tests => 33;
+
+use Math::BigInt;
+
+my ($x, $y, $z);
+
+TODO: {
+ local $TODO = q{[perl #113834] overloaded assignment operator semantics};
+
+$x = 60;
+$y = Math::BigInt->new(7);
+$z = $x += $y;
+
+is($x, '67', 'x += y stores the correct value in x');
+is($y, '7', 'x += y leaves y unchanged');
+is($z, '67', 'x += y returns the correct value');
+
+$x = 60;
+$y = Math::BigInt->new(7);
+$z = $x -= $y;
+
+is($x, '53', 'x -= y stores the correct value in x');
+is($y, '7', 'x -= y leaves y unchanged');
+is($z, '53', 'x -= y returns the correct value');
+
+$x = 60;
+$y = Math::BigInt->new(7);
+$z = $x *= $y;
+
+is($x, '420', 'x *= y stores the correct value in x');
+is($y, '7', 'x *= y leaves y unchanged');
+is($z, '420', 'x *= y returns the correct value');
+
+$x = 60;
+$y = Math::BigInt->new(7);
+$z = $x /= $y;
+
+is($x, '8', 'x /= y stores the correct value in x');
+is($y, '7', 'x /= y leaves y unchanged');
+is($z, '8', 'x /= y returns the correct value');
+
+$x = 60;
+$y = Math::BigInt->new(7);
+$z = $x %= $y;
+
+is($x, '4', 'x %= y stores the correct value in x');
+is($y, '7', 'x %= y leaves y unchanged');
+is($z, '4', 'x %= y returns the correct value');
+
+$x = 60;
+$y = Math::BigInt->new(7);
+$z = $x ^= $y;
+
+is($x, '59', 'x ^= y stores the correct value in x');
+is($y, '7', 'x ^= y leaves y unchanged');
+is($z, '59', 'x ^= y returns the correct value');
+
+$x = 60;
+$y = Math::BigInt->new(7);
+$z = $x &= $y;
+
+is($x, '4', 'x &= y stores the correct value in x');
+is($y, '7', 'x &= y leaves y unchanged');
+is($z, '4', 'x &= y returns the correct value');
+
+$x = 60;
+$y = Math::BigInt->new(7);
+$z = $x |= $y;
+
+is($x, '63', 'x |= y stores the correct value in x');
+is($y, '7', 'x |= y leaves y unchanged');
+is($z, '63', 'x |= y returns the correct value');
+
+$x = 2;
+$y = Math::BigInt->new(7);
+$z = $x **= $y;
+
+is($x, '128', 'x **= y stores the correct value in x');
+is($y, '7', 'x **= y leaves y unchanged');
+is($z, '128', 'x **= y returns the correct value');
+
+$x = 2;
+$y = Math::BigInt->new(7);
+$z = $x <<= $y;
+
+is($x, '256', 'x <<= y stores the correct value in x');
+is($y, '7', 'x <<= y leaves y unchanged');
+is($z, '256', 'x <<= y returns the correct value');
+
+$x = 600;
+$y = Math::BigInt->new(7);
+$z = $x >>= $y;
+
+is($x, '4', 'x >>= y stores the correct value in x');
+is($y, '7', 'x >>= y leaves y unchanged');
+is($z, '4', 'x >>= y returns the correct value');
+
+}
|
From @rjbsI have pushed 90732c3 to smoke-me/rjbs/revert-ol-change, reverting f041cf0 I would like to get a test for all the relevant bugs: the one fixed by f041cf0 and the one -- |
From @rjbsI could not find the ticket in response to which f041cf0 was written. I've updated the What we want is to fix the fallback of += without making it use the rhs's overloading. Rather -- |
From mhasch-p5p@cozap.comThis issue still needs clarification. What was actually wrong Quoting the commit message of f041cf0:
Is it wrong to directly look at the base operation, once the Therefore the base operation may come into play not for lack of A more radical interpretation of the "fallback=>0" setting, In short, what f041cf0 set out to change does not Another point that remains unsolved if this change is The overload interface could be extended to allow that. -Martin |
From @doyOn Wed, Mar 06, 2013 at 10:06:58AM -0800, Martin Hasch via RT wrote:
Yes, this is essentially what I was trying to get across. Without this -doy |
|
The above tests pass on Perl 5.31.6, and the following outputs "1" as expected: perl -MMath::BigInt -le '$a = 2; $a -= Math::BigInt->new(1); print $a'From how I understand the discussion, the following docpatch should be added to +It can, however, generate a call to C<$b>'s method for C<*>.
+An assignment operator is mapped to a normal binary operator
+if it is overloaded from the perspective of the right hand side.
+This mapping is not subject to the fallback mode.I will open a pull request against Perl to apply that docpatch. The test files should go into the https://github.com/pjacklam/p5-Math-BigInt distribution which is dual-lifed, I assume. If so, @pjacklam, can you confirm or deny this? In either case I think this ticket can be closed. |
Migrated from rt.perl.org#113834 (status was 'open')
Searchable as RT113834$
The text was updated successfully, but these errors were encountered: