overloading fallback for + and += is broken

Perl / perl5

🐪 The Perl programming language

https://dev.perl.org/perl5/

Other

1.9k stars 541 forks source link

overloading fallback for + and += is broken #12223

Open p5pRT opened 12 years ago

p5pRT commented 12 years ago

Migrated from rt.perl.org#113834 (status was 'open')

Searchable as RT113834$

p5pRT commented 12 years ago

From @mhasch

This is a bug report for perl from mhasch@cpan.org\, generated with the help of perlbug 1.39 running under perl 5.17.1.

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 1.998\, depends on a certain order of arguments of assignment operators called by overload\, while overload-1.19 does not behave as assumed.

Math::BigInt states this assumption in a comment in the source code (though unfortunately not in its test suite):

@@ dist/Math-BigInt/lib/Math/BigInt.pm\, lines 48..62 @@ 48 # some shortcuts for speed (assumes that reversed order of arguments is routed 49 # to normal '+' and we thus can always modify first arg. If this is changed\, 50 # this breaks and must be adjusted.) 51 '+=' => sub { $_[0]->badd($_[1]); }\, 52 '-=' => sub { $_[0]->bsub($_[1]); }\, 53 '*=' => sub { $_[0]->bmul($_[1]); }\, 54 '/=' => sub { scalar $_[0]->bdiv($_[1]); }\, 55 '%=' => sub { $_[0]->bmod($_[1]); }\, 56 '^=' => sub { $_[0]->bxor($_[1]); }\, 57 '&=' => sub { $_[0]->band($_[1]); }\, 58 '|=' => sub { $_[0]->bior($_[1]); }\, 59 60 '**=' => sub { $_[0]->bpow($_[1]); }\, 61 '\<\<=' => sub { $_[0]->blsft($_[1]); }\, 62 '>>=' => sub { $_[0]->brsft($_[1]); }\,

Although code like this might seem dangerous at first glance\, it is actually exploiting a documented feature of overload:

@@ lib/overload.pm\, section "Assignments"\, lines 425..435 @@ 425 An object that overloads an assignment operator does so only in 426 respect of assignments to that object. 427 In other words\, Perl never calls the corresponding methods with 428 the third argument (the "swap" argument) set to TRUE. 429 For example\, the operation 430 431 $a *= $b 432 433 cannot lead to C\<$b>'s implementation of C\<*=> being called\, 434 even if C\<$a> is a scalar. 435 (It can\, however\, generate a call to C\<$b>'s method for C\<*>).

This documentation seems now no longer accurate\, as can be seen with this example code:

use overload ( '*' => sub { "ok" }\, '*=' => sub { "not ok" }\, ); my $a = 1; my $b = bless []; print overload->VERSION\, "\t"\, ($a *= $b)\, "\n";

This prints 1.19 and "not ok" with perl-5.17.0 up to blead\, 1.18 and "ok" with perl-5.16.0.

To repair this\, either the documentation of "overload" should be changed and "Math::BigInt" should be adapted accordingly\, or "overload" should be brought back to par with its documentation.

In either case it may prove useful to add something along the lines of my first example to the test suite of Math::BigInt\, even if the main issue is with overload.

-Martin

Flags: category=library severity=critical module=overload

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:
Platform: osname=linux\, osvers=2.6.32-5-686\, archname=i686-linux-64int-ld uname='linux spdevlxcl07 2.6.32-5-686 #1 smp mon oct 3 04:15:24 utc 2011 i686 gnulinux ' config_args='-Dusedevel -Uversiononly -Dprefix=/opt/perl517-i8n12 -Duse64bitint -Duselongdouble -Dcf_email=cpants@localhost -Dperladmin=none -Dusevfork=false -de' hint=recommended\, useposix=true\, d_sigaction=define useithreads=undef\, usemultiplicity=undef useperlio=define\, d_sfio=undef\, uselargefiles=define\, usesocks=undef use64bitint=define\, use64bitall=undef\, uselongdouble=define usemymalloc=n\, bincompat5005=undef Compiler: cc='cc'\, ccflags ='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'\, optimize='-O2'\, cppflags='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion=''\, gccversion='4.4.5'\, gccosandvers='' intsize=4\, longsize=4\, ptrsize=4\, doublesize=8\, byteorder=12345678 d_longlong=define\, longlongsize=8\, d_longdbl=define\, longdblsize=12 ivtype='long long'\, ivsize=8\, nvtype='long double'\, nvsize=12\, Off_t='off_t'\, lseeksize=8 alignbytes=4\, prototype=define Linker and Libraries: ld='cc'\, ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /lib/../lib /usr/lib/../lib /lib /usr/lib /usr/lib64 libs=-lnsl -ldl -lm -lcrypt -lutil -lc perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc libc=/lib/libc-2.11.3.so\, so=so\, useshrplib=false\, libperl=libperl.a gnulibc_version='2.11.3' Dynamic Linking: dlsrc=dl_dlopen.xs\, dlext=so\, d_dlsymun=undef\, ccdlflags='-Wl\,-E' cccdlflags='-fPIC'\, lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'

Locally applied patches:

@INC for perl 5.17.1: /opt/perl517-i8n12/lib/site_perl/5.17.1/i686-linux-64int-ld /opt/perl517-i8n12/lib/site_perl/5.17.1 /opt/perl517-i8n12/lib/5.17.1/i686-linux-64int-ld /opt/perl517-i8n12/lib/5.17.1 /opt/perl517-i8n12/lib/site_perl/5.17.0 /opt/perl517-i8n12/lib/site_perl .

Environment for perl 5.17.1: HOME=/home/cpants LANG=C LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/opt/perl517-i8n12/bin:/home/cpants/bin:/usr/local/bin:/usr/bin:/bin:/usr/games PERL_BADLANG (unset) SHELL=/bin/bash

p5pRT commented 12 years ago

From @doy

On Mon\, Jun 25\, 2012 at 09:53:15AM -0700\, mhasch@cpan.org wrote:

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 1.998\, depends on a certain order of arguments of assignment operators called by overload\, while overload-1.19 does not behave as assumed.

Math::BigInt states this assumption in a comment in the source code (though unfortunately not in its test suite):

@@ dist/Math-BigInt/lib/Math/BigInt.pm\, lines 48..62 @@ 48 # some shortcuts for speed (assumes that reversed order of arguments is routed 49 # to normal '+' and we thus can always modify first arg. If this is changed\, 50 # this breaks and must be adjusted.) 51 '+=' => sub { $_[0]->badd($_[1]); }\, 52 '-=' => sub { $_[0]->bsub($_[1]); }\, 53 '*=' => sub { $_[0]->bmul($_[1]); }\, 54 '/=' => sub { scalar $_[0]->bdiv($_[1]); }\, 55 '%=' => sub { $_[0]->bmod($_[1]); }\, 56 '^=' => sub { $_[0]->bxor($_[1]); }\, 57 '&=' => sub { $_[0]->band($_[1]); }\, 58 '|=' => sub { $_[0]->bior($_[1]); }\, 59 60 '**=' => sub { $_[0]->bpow($_[1]); }\, 61 '\<\<=' => sub { $_[0]->blsft($_[1]); }\, 62 '>>=' => sub { $_[0]->brsft($_[1]); }\,

Although code like this might seem dangerous at first glance\, it is actually exploiting a documented feature of overload:

@@ lib/overload.pm\, section "Assignments"\, lines 425..435 @@ 425 An object that overloads an assignment operator does so only in 426 respect of assignments to that object. 427 In other words\, Perl never calls the corresponding methods with 428 the third argument (the "swap" argument) set to TRUE. 429 For example\, the operation 430 431 $a *= $b 432 433 cannot lead to C\<$b>'s implementation of C\<*=> being called\, 434 even if C\<$a> is a scalar. 435 (It can\, however\, generate a call to C\<$b>'s method for C\<*>).

This documentation seems now no longer accurate\, as can be seen with this example code:

use overload ( '*' => sub { "ok" }\, '*=' => sub { "not ok" }\, ); my $a = 1; my $b = bless []; print overload->VERSION\, "\t"\, ($a *= $b)\, "\n";

This prints 1.19 and "not ok" with perl-5.17.0 up to blead\, 1.18 and "ok" with perl-5.16.0.

To repair this\, either the documentation of "overload" should be changed and "Math::BigInt" should be adapted accordingly\, or "overload" should be brought back to par with its documentation.

In either case it may prove useful to add something along the lines of my first example to the test suite of Math::BigInt\, even if the main issue is with overload.

I agree that this is a bug. Bisecting shows this:

$ perl ../bisect.pl --start=v5.16.0 -- ./perl -Ilib ~/test7.pl [...] There are only 'skip'ped commits left to test. The first bad commit could be any of: f041cf0f9c6469c41de8b73d5f7b426710c3ff8b 5f9f83be9cdcd54449f7f40db078fe367d780475 We cannot bisect more! bisect run cannot continue any more Died at ../bisect.pl line 150. That took 997 seconds

f041cf0 is almost certainly the issue (5f9f83 just fixes a minor build issue that f041cf0 introduced)\, and f041cf0 is:

commit f041cf0f9c6469c41de8b73d5f7b426710c3ff8b Author: Rafael Garcia-Suarez \rgs@consttype\.org Date: Tue Mar 20 09:17:02 2012 +0100

Lookup overloaded assignment operators when trying to swap the arguments

This is in the case where we search for an overloaded operator when passing the AMGf_assign flag (we're executing an assignment operator like +=).

At the very beginning of Perl_amagic_call\, if the flag AMGf_noleft is not passed\, we first try to look up the overload method corresponding to the assignment operator\, then the normal one if fallback is authorized. However\, if this fails\, when trying later to find overload magic with the arguments swapped (if AMGf_noright is not passed)\, this procedure was not used and we looked up directly the base operation from which the assignment operator might be derived. As a consequence of what an operator like += might have looked autogenerated even when fallback=>0 was passed.

This change only necessitates a minor adjustment in lib/overload.t\, where an overloaded += method wasn't corresponding semantically to the overloaded + method of the same class\, which can be seen as a pathological case.

So it sounds like the issue was that 1 += $overloaded wasn't falling back to $overloaded's + overload\, but this fix makes it fall back to $overloaded's += overload\, which is wrong.

-doy

p5pRT commented 12 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 12 years ago

From @doy

On Fri\, Jun 29\, 2012 at 10:53:25AM -0500\, Jesse Luehrs wrote:

On Mon\, Jun 25\, 2012 at 09:53:15AM -0700\, mhasch@cpan.org wrote:

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 1.998\, depends on a certain order of arguments of assignment operators called by overload\, while overload-1.19 does not behave as assumed.

Math::BigInt states this assumption in a comment in the source code (though unfortunately not in its test suite):

@@ dist/Math-BigInt/lib/Math/BigInt.pm\, lines 48..62 @@ 48 # some shortcuts for speed (assumes that reversed order of arguments is routed 49 # to normal '+' and we thus can always modify first arg. If this is changed\, 50 # this breaks and must be adjusted.) 51 '+=' => sub { $_[0]->badd($_[1]); }\, 52 '-=' => sub { $_[0]->bsub($_[1]); }\, 53 '*=' => sub { $_[0]->bmul($_[1]); }\, 54 '/=' => sub { scalar $_[0]->bdiv($_[1]); }\, 55 '%=' => sub { $_[0]->bmod($_[1]); }\, 56 '^=' => sub { $_[0]->bxor($_[1]); }\, 57 '&=' => sub { $_[0]->band($_[1]); }\, 58 '|=' => sub { $_[0]->bior($_[1]); }\, 59 60 '**=' => sub { $_[0]->bpow($_[1]); }\, 61 '\<\<=' => sub { $_[0]->blsft($_[1]); }\, 62 '>>=' => sub { $_[0]->brsft($_[1]); }\,

Although code like this might seem dangerous at first glance\, it is actually exploiting a documented feature of overload:

@@ lib/overload.pm\, section "Assignments"\, lines 425..435 @@ 425 An object that overloads an assignment operator does so only in 426 respect of assignments to that object. 427 In other words\, Perl never calls the corresponding methods with 428 the third argument (the "swap" argument) set to TRUE. 429 For example\, the operation 430 431 $a *= $b 432 433 cannot lead to C\<$b>'s implementation of C\<*=> being called\, 434 even if C\<$a> is a scalar. 435 (It can\, however\, generate a call to C\<$b>'s method for C\<*>).

This documentation seems now no longer accurate\, as can be seen with this example code:

use overload ( '*' => sub { "ok" }\, '*=' => sub { "not ok" }\, ); my $a = 1; my $b = bless []; print overload->VERSION\, "\t"\, ($a *= $b)\, "\n";

This prints 1.19 and "not ok" with perl-5.17.0 up to blead\, 1.18 and "ok" with perl-5.16.0.

To repair this\, either the documentation of "overload" should be changed and "Math::BigInt" should be adapted accordingly\, or "overload" should be brought back to par with its documentation.

In either case it may prove useful to add something along the lines of my first example to the test suite of Math::BigInt\, even if the main issue is with overload.

I agree that this is a bug. Bisecting shows this:

$ perl ../bisect.pl --start=v5.16.0 -- ./perl -Ilib ~/test7.pl [...] There are only 'skip'ped commits left to test. The first bad commit could be any of: f041cf0f9c6469c41de8b73d5f7b426710c3ff8b 5f9f83be9cdcd54449f7f40db078fe367d780475 We cannot bisect more! bisect run cannot continue any more Died at ../bisect.pl line 150. That took 997 seconds

f041cf0 is almost certainly the issue (5f9f83 just fixes a minor build issue that f041cf0 introduced)\, and f041cf0 is:

commit f041cf0f9c6469c41de8b73d5f7b426710c3ff8b Author: Rafael Garcia-Suarez \rgs@consttype\.org Date: Tue Mar 20 09:17:02 2012 +0100
  Lookup overloaded assignment operators when trying to swap the arguments

  This is in the case where we search for an overloaded operator when
  passing the AMGf\_assign flag $we're executing an assignment operator
  like \+=$\.

  At the very beginning of Perl\_amagic\_call\, if the flag AMGf\_noleft is
  not passed\, we first try to look up the overload method corresponding
  to the assignment operator\, then the normal one if fallback is
  authorized\. However\, if this fails\, when trying later to find
  overload magic with the arguments swapped $if AMGf\_noright is not
  passed$\, this procedure was not used and we looked up directly the base
  operation from which the assignment operator might be derived\.
  As a consequence of what an operator like \+= might have looked
  autogenerated even when fallback=>0 was passed\.

  This change only necessitates a minor adjustment in lib/overload\.t\,
  where an overloaded \+= method wasn't corresponding semantically to the
  overloaded \+ method of the same class\, which can be seen as a
  pathological case\.
So it sounds like the issue was that 1 += $overloaded wasn't falling back to $overloaded's + overload\, but this fix makes it fall back to $overloaded's += overload\, which is wrong.

Okay\, so fixing this doesn't appear to be as straightforward as it looks. The issue is that determining what to do with assignment operators when the left side isn't overloaded is tricky.

In 5.16\, "string" .= $overloaded worked fine because it would fall back to using the '.' overload on $overloaded\, even if fallback was set to 0. This was clearly wrong (.= falling back to . is a form of magic autogeneration)\, so it was fixed in f041cf0.

f041cf0 had a small error (it started checking for .= overloading on $overloaded\, which is also wrong)\, but fixing that was pretty easy. However\, the issue then becomes that "string" .= $overloaded has no way to work\, in the case where the '.' and '.=' overloads return normal strings (which isn't that uncommon) and $overloaded has fallback => 0 set.

What we're left with then is a situation where it's impossible to make "string" .= $overloaded work if $overloaded has fallback => 0 set (and similarly for any other assignment operator). I'm not sure what the right solution here is\, and any input would be appreciated. Also\, should we revert f041cf0 until we figure this out?

-doy

p5pRT commented 12 years ago

From @doy

On Fri\, Jun 29\, 2012 at 01:50:35PM -0500\, Jesse Luehrs wrote:

Okay\, so fixing this doesn't appear to be as straightforward as it looks. The issue is that determining what to do with assignment operators when the left side isn't overloaded is tricky.

In 5.16\, "string" .= $overloaded worked fine because it would fall back to using the '.' overload on $overloaded\, even if fallback was set to 0. This was clearly wrong (.= falling back to . is a form of magic autogeneration)\, so it was fixed in f041cf0.

f041cf0 had a small error (it started checking for .= overloading on $overloaded\, which is also wrong)\, but fixing that was pretty easy. However\, the issue then becomes that "string" .= $overloaded has no way to work\, in the case where the '.' and '.=' overloads return normal strings (which isn't that uncommon) and $overloaded has fallback => 0 set.

What we're left with then is a situation where it's impossible to make "string" .= $overloaded work if $overloaded has fallback => 0 set (and similarly for any other assignment operator). I'm not sure what the right solution here is\, and any input would be appreciated. Also\, should we revert f041cf0 until we figure this out?

Work in progress pushed to doy/overload_fallback_fix_113834.

-doy

p5pRT commented 12 years ago

From @khwilliamson

On 06/29/2012 12:50 PM\, Jesse Luehrs wrote:

On Fri\, Jun 29\, 2012 at 10:53:25AM -0500\, Jesse Luehrs wrote:
On Mon\, Jun 25\, 2012 at 09:53:15AM -0700\, mhasch@cpan.org wrote:

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 1.998\, depends on a certain order of arguments of assignment operators called by overload\, while overload-1.19 does not behave as assumed.

Math::BigInt states this assumption in a comment in the source code (though unfortunately not in its test suite):

@@ dist/Math-BigInt/lib/Math/BigInt.pm\, lines 48..62 @@ 48 # some shortcuts for speed (assumes that reversed order of arguments is routed 49 # to normal '+' and we thus can always modify first arg. If this is changed\, 50 # this breaks and must be adjusted.) 51 '+=' => sub { $_[0]->badd($_[1]); }\, 52 '-=' => sub { $_[0]->bsub($_[1]); }\, 53 '*=' => sub { $_[0]->bmul($_[1]); }\, 54 '/=' => sub { scalar $_[0]->bdiv($_[1]); }\, 55 '%=' => sub { $_[0]->bmod($_[1]); }\, 56 '^=' => sub { $_[0]->bxor($_[1]); }\, 57 '&=' => sub { $_[0]->band($_[1]); }\, 58 '|=' => sub { $_[0]->bior($_[1]); }\, 59 60 '**=' => sub { $_[0]->bpow($_[1]); }\, 61 '\<\<=' => sub { $_[0]->blsft($_[1]); }\, 62 '>>=' => sub { $_[0]->brsft($_[1]); }\,

Although code like this might seem dangerous at first glance\, it is actually exploiting a documented feature of overload:

@@ lib/overload.pm\, section "Assignments"\, lines 425..435 @@ 425 An object that overloads an assignment operator does so only in 426 respect of assignments to that object. 427 In other words\, Perl never calls the corresponding methods with 428 the third argument (the "swap" argument) set to TRUE. 429 For example\, the operation 430 431 $a *= $b 432 433 cannot lead to C\<$b>'s implementation of C\<*=> being called\, 434 even if C\<$a> is a scalar. 435 (It can\, however\, generate a call to C\<$b>'s method for C\<*>).

This documentation seems now no longer accurate\, as can be seen with this example code:

use overload ( '*' => sub { "ok" }\, '*=' => sub { "not ok" }\, ); my $a = 1; my $b = bless []; print overload->VERSION\, "\t"\, ($a *= $b)\, "\n";

This prints 1.19 and "not ok" with perl-5.17.0 up to blead\, 1.18 and "ok" with perl-5.16.0.

To repair this\, either the documentation of "overload" should be changed and "Math::BigInt" should be adapted accordingly\, or "overload" should be brought back to par with its documentation.

In either case it may prove useful to add something along the lines of my first example to the test suite of Math::BigInt\, even if the main issue is with overload.

I agree that this is a bug. Bisecting shows this:

$ perl ../bisect.pl --start=v5.16.0 -- ./perl -Ilib ~/test7.pl [...] There are only 'skip'ped commits left to test. The first bad commit could be any of: f041cf0f9c6469c41de8b73d5f7b426710c3ff8b 5f9f83be9cdcd54449f7f40db078fe367d780475 We cannot bisect more! bisect run cannot continue any more Died at ../bisect.pl line 150. That took 997 seconds

f041cf0 is almost certainly the issue (5f9f83 just fixes a minor build issue that f041cf0 introduced)\, and f041cf0 is:

commit f041cf0f9c6469c41de8b73d5f7b426710c3ff8b Author: Rafael Garcia-Suarez \rgs@consttype\.org Date: Tue Mar 20 09:17:02 2012 +0100
   Lookup overloaded assignment operators when trying to swap the arguments

   This is in the case where we search for an overloaded operator when
   passing the AMGf\_assign flag $we're executing an assignment operator
   like \+=$\.

   At the very beginning of Perl\_amagic\_call\, if the flag AMGf\_noleft is
   not passed\, we first try to look up the overload method corresponding
   to the assignment operator\, then the normal one if fallback is
   authorized\. However\, if this fails\, when trying later to find
   overload magic with the arguments swapped $if AMGf\_noright is not
   passed$\, this procedure was not used and we looked up directly the base
   operation from which the assignment operator might be derived\.
   As a consequence of what an operator like \+= might have looked
   autogenerated even when fallback=>0 was passed\.

   This change only necessitates a minor adjustment in lib/overload\.t\,
   where an overloaded \+= method wasn't corresponding semantically to the
   overloaded \+ method of the same class\, which can be seen as a
   pathological case\.
So it sounds like the issue was that 1 += $overloaded wasn't falling back to $overloaded's + overload\, but this fix makes it fall back to $overloaded's += overload\, which is wrong.
Okay\, so fixing this doesn't appear to be as straightforward as it looks. The issue is that determining what to do with assignment operators when the left side isn't overloaded is tricky.

In 5.16\, "string" .= $overloaded worked fine because it would fall back to using the '.' overload on $overloaded\, even if fallback was set to 0. This was clearly wrong (.= falling back to . is a form of magic autogeneration)\, so it was fixed in f041cf0.

f041cf0 had a small error (it started checking for .= overloading on $overloaded\, which is also wrong)\, but fixing that was pretty easy. However\, the issue then becomes that "string" .= $overloaded has no way to work\, in the case where the '.' and '.=' overloads return normal strings (which isn't that uncommon) and $overloaded has fallback => 0 set.

What we're left with then is a situation where it's impossible to make "string" .= $overloaded work if $overloaded has fallback => 0 set (and similarly for any other assignment operator). I'm not sure what the right solution here is\, and any input would be appreciated. Also\, should we revert f041cf0 until we figure this out?

-doy

That commit was there because it was needed by mktables; reverting may make that fail\, or it may not.

p5pRT commented 12 years ago

From @doy

On Fri\, Jun 29\, 2012 at 01:45:08PM -0600\, Karl Williamson wrote:

Okay\, so fixing this doesn't appear to be as straightforward as it looks. The issue is that determining what to do with assignment operators when the left side isn't overloaded is tricky.

In 5.16\, "string" .= $overloaded worked fine because it would fall back to using the '.' overload on $overloaded\, even if fallback was set to 0. This was clearly wrong (.= falling back to . is a form of magic autogeneration)\, so it was fixed in f041cf0.

f041cf0 had a small error (it started checking for .= overloading on $overloaded\, which is also wrong)\, but fixing that was pretty easy. However\, the issue then becomes that "string" .= $overloaded has no way to work\, in the case where the '.' and '.=' overloads return normal strings (which isn't that uncommon) and $overloaded has fallback => 0 set.

What we're left with then is a situation where it's impossible to make "string" .= $overloaded work if $overloaded has fallback => 0 set (and similarly for any other assignment operator). I'm not sure what the right solution here is\, and any input would be appreciated. Also\, should we revert f041cf0 until we figure this out?

-doy

That commit was there because it was needed by mktables; reverting may make that fail\, or it may not.

It's needed by mktables because string interpolation is converted into a sequence of .= operations - "foo $foo bar $bar baz" becomes effectively:

$res = "foo " . $foo; $res .= " bar "; $res .= $bar; $res .= " baz";

The case relevant to this issue is the fourth line there\, $res .= $bar\, since in that case $res is a string (since that's what Property's .= overload returns) and $bar is an instance of Property.

-doy

p5pRT commented 12 years ago

From mhasch-cpanbugs@cozap.com

On Fri\, Jun 29\, 2012 at 11:51:19AM -0700\, Jesse Luehrs via RT wrote:

On Fri\, Jun 29\, 2012 at 10:53:25AM -0500\, Jesse Luehrs wrote:
On Mon\, Jun 25\, 2012 at 09:53:15AM -0700\, mhasch@cpan.org wrote:

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 1.998\, depends on a certain order of arguments of assignment operators called by overload\, while overload-1.19 does not behave as assumed.

Math::BigInt states this assumption in a comment in the source code (though unfortunately not in its test suite):

@@ dist/Math-BigInt/lib/Math/BigInt.pm\, lines 48..62 @@ 48 # some shortcuts for speed (assumes that reversed order of arguments is routed 49 # to normal '+' and we thus can always modify first arg. If this is changed\, 50 # this breaks and must be adjusted.) 51 '+=' => sub { $_[0]->badd($_[1]); }\, 52 '-=' => sub { $_[0]->bsub($_[1]); }\, 53 '*=' => sub { $_[0]->bmul($_[1]); }\, 54 '/=' => sub { scalar $_[0]->bdiv($_[1]); }\, 55 '%=' => sub { $_[0]->bmod($_[1]); }\, 56 '^=' => sub { $_[0]->bxor($_[1]); }\, 57 '&=' => sub { $_[0]->band($_[1]); }\, 58 '|=' => sub { $_[0]->bior($_[1]); }\, 59 60 '**=' => sub { $_[0]->bpow($_[1]); }\, 61 '\<\<=' => sub { $_[0]->blsft($_[1]); }\, 62 '>>=' => sub { $_[0]->brsft($_[1]); }\,

Although code like this might seem dangerous at first glance\, it is actually exploiting a documented feature of overload:

@@ lib/overload.pm\, section "Assignments"\, lines 425..435 @@ 425 An object that overloads an assignment operator does so only in 426 respect of assignments to that object. 427 In other words\, Perl never calls the corresponding methods with 428 the third argument (the "swap" argument) set to TRUE. 429 For example\, the operation 430 431 $a *= $b 432 433 cannot lead to C\<$b>'s implementation of C\<*=> being called\, 434 even if C\<$a> is a scalar. 435 (It can\, however\, generate a call to C\<$b>'s method for C\<*>).

This documentation seems now no longer accurate\, as can be seen with this example code:

use overload ( '*' => sub { "ok" }\, '*=' => sub { "not ok" }\, ); my $a = 1; my $b = bless []; print overload->VERSION\, "\t"\, ($a *= $b)\, "\n";

This prints 1.19 and "not ok" with perl-5.17.0 up to blead\, 1.18 and "ok" with perl-5.16.0.

To repair this\, either the documentation of "overload" should be changed and "Math::BigInt" should be adapted accordingly\, or "overload" should be brought back to par with its documentation.

In either case it may prove useful to add something along the lines of my first example to the test suite of Math::BigInt\, even if the main issue is with overload.

I agree that this is a bug. Bisecting shows this:

$ perl ../bisect.pl --start=v5.16.0 -- ./perl -Ilib ~/test7.pl [...] There are only 'skip'ped commits left to test. The first bad commit could be any of: f041cf0f9c6469c41de8b73d5f7b426710c3ff8b 5f9f83be9cdcd54449f7f40db078fe367d780475 We cannot bisect more! bisect run cannot continue any more Died at ../bisect.pl line 150. That took 997 seconds

f041cf0 is almost certainly the issue (5f9f83 just fixes a minor build issue that f041cf0 introduced)\, and f041cf0 is:

commit f041cf0f9c6469c41de8b73d5f7b426710c3ff8b Author: Rafael Garcia-Suarez \rgs@consttype\.org Date: Tue Mar 20 09:17:02 2012 +0100
  Lookup overloaded assignment operators when trying to swap the arguments

  This is in the case where we search for an overloaded operator when
  passing the AMGf\_assign flag $we're executing an assignment operator
  like \+=$\.

  At the very beginning of Perl\_amagic\_call\, if the flag AMGf\_noleft is
  not passed\, we first try to look up the overload method corresponding
  to the assignment operator\, then the normal one if fallback is
  authorized\. However\, if this fails\, when trying later to find
  overload magic with the arguments swapped $if AMGf\_noright is not
  passed$\, this procedure was not used and we looked up directly the base
  operation from which the assignment operator might be derived\.
  As a consequence of what an operator like \+= might have looked
  autogenerated even when fallback=>0 was passed\.

  This change only necessitates a minor adjustment in lib/overload\.t\,
  where an overloaded \+= method wasn't corresponding semantically to the
  overloaded \+ method of the same class\, which can be seen as a
  pathological case\.
So it sounds like the issue was that 1 += $overloaded wasn't falling back to $overloaded's + overload\, but this fix makes it fall back to $overloaded's += overload\, which is wrong.
Okay\, so fixing this doesn't appear to be as straightforward as it looks. The issue is that determining what to do with assignment operators when the left side isn't overloaded is tricky.

In 5.16\, "string" .= $overloaded worked fine because it would fall back to using the '.' overload on $overloaded\, even if fallback was set to 0. This was clearly wrong (.= falling back to . is a form of magic autogeneration)\, so it was fixed in f041cf0.

f041cf0 had a small error (it started checking for .= overloading on $overloaded\, which is also wrong)\, but fixing that was pretty easy. However\, the issue then becomes that "string" .= $overloaded has no way to work\, in the case where the '.' and '.=' overloads return normal strings (which isn't that uncommon) and $overloaded has fallback => 0 set.

What we're left with then is a situation where it's impossible to make "string" .= $overloaded work if $overloaded has fallback => 0 set (and similarly for any other assignment operator). I'm not sure what the right solution here is\, and any input would be appreciated. Also\, should we revert f041cf0 until we figure this out?

The way "overload" has been documented up to now\, assignment operators are only available for overloading under government of the left hand operand. This makes sense to me as I generally regard assignment variants of binary operators as shortcut notation but semantically equivalent to the longer notation with the separate equals sign. I take it the reason for providing hooks for assignment variants at all is that these allow implementations to take advantage of optimization opportunities where modifying objects in-place may be cheaper than working with separate intermediate results. There is no similar benefit to be gained for the object on the right hand side of an assignment.

It escapes me how somebody might want to override ".=" while not at the same time overriding ".". I would consider this to be a bug in the overriding class. The overload pragma may deal with this situation as it does in other cases where some operations are overloaded and others are not\, which may for example lead to perl builtin "." handling ``$foo .= $overloaded''.

I'd argue whether the "fallback" directive must necessarily apply to unfolding assignment operations in the same way as it applies to replacing unary negation by subtraction\, or inequality operations by 3-way-comparisons\, etc. After all\, there are other operators with special fallback behaviour\, too\, like dereferencing operators. All I ask is that the behaviour should be well documented. Backwards compatibility should not be given up lightly either.

A solution in order to help an exotic object that does in fact need to distinguish between different ways of being concatenated could involve a new directive enabling swapped-parameter assigment\, or a new group of symbols eliglible for handling both cases rather than only the case of the overloading object being assigned to. I am not yet convinced that "mktables"\, a tool to create the runtime Perl Unicode files\, really needs that kind of objects. In fact\, it looks to me like it performed the fallback to "." explicitly in the implementation of ".=".

And anyway\, I should prefer to change this script rather than review any odd module using overload that could be hit like Math::BigInt from a change of the overload pragma.

-Martin

p5pRT commented 12 years ago

From @doy

On Tue\, Jul 03\, 2012 at 01:57:01PM +0200\, Martin Becker wrote:

Okay\, so fixing this doesn't appear to be as straightforward as it looks. The issue is that determining what to do with assignment operators when the left side isn't overloaded is tricky.

In 5.16\, "string" .= $overloaded worked fine because it would fall back to using the '.' overload on $overloaded\, even if fallback was set to 0. This was clearly wrong (.= falling back to . is a form of magic autogeneration)\, so it was fixed in f041cf0.

f041cf0 had a small error (it started checking for .= overloading on $overloaded\, which is also wrong)\, but fixing that was pretty easy. However\, the issue then becomes that "string" .= $overloaded has no way to work\, in the case where the '.' and '.=' overloads return normal strings (which isn't that uncommon) and $overloaded has fallback => 0 set.

What we're left with then is a situation where it's impossible to make "string" .= $overloaded work if $overloaded has fallback => 0 set (and similarly for any other assignment operator). I'm not sure what the right solution here is\, and any input would be appreciated. Also\, should we revert f041cf0 until we figure this out?

The way "overload" has been documented up to now\, assignment operators are only available for overloading under government of the left hand operand. This makes sense to me as I generally regard assignment variants of binary operators as shortcut notation but semantically equivalent to the longer notation with the separate equals sign. I take it the reason for providing hooks for assignment variants at all is that these allow implementations to take advantage of optimization opportunities where modifying objects in-place may be cheaper than working with separate intermediate results. There is no similar benefit to be gained for the object on the right hand side of an assignment.

I agree - assignment operators looking at the right operand is clearly wrong.

It escapes me how somebody might want to override ".=" while not at the same time overriding ".". I would consider this to be a bug in the overriding class. The overload pragma may deal with this situation as it does in other cases where some operations are overloaded and others are not\, which may for example lead to perl builtin "." handling ``$foo .= $overloaded''.

I'd argue whether the "fallback" directive must necessarily apply to unfolding assignment operations in the same way as it applies to replacing unary negation by subtraction\, or inequality operations by 3-way-comparisons\, etc. After all\, there are other operators with special fallback behaviour\, too\, like dereferencing operators. All I ask is that the behaviour should be well documented. Backwards compatibility should not be given up lightly either.

This would be an argument for returning to the 5.16 behavior. I do think that is likely one of the better options.

A solution in order to help an exotic object that does in fact need to distinguish between different ways of being concatenated could involve a new directive enabling swapped-parameter assigment\, or a new group of symbols eliglible for handling both cases rather than only the case of the overloading object being assigned to. I am not yet convinced that "mktables"\, a tool to create the runtime Perl Unicode files\, really needs that kind of objects. In fact\, it looks to me like it performed the fallback to "." explicitly in the implementation of ".=".

The mktables thing can easily be fixed\, that isn't really the point. The point is\, is it a reasonable expectation that overloading all operations\, even with fallback off\, should make the object usable in all relevant situations? I'm leaning toward yes\, which is why I'm leaning toward just reverting this change.

And anyway\, I should prefer to change this script rather than review any odd module using overload that could be hit like Math::BigInt from a change of the overload pragma.

Yes\, I don't think that Math::BigInt is doing anything wrong here. Any fix to this issue should allow it to work as it is currently written.

-doy

p5pRT commented 12 years ago

From @khwilliamson

On 07/04/2012 03:38 PM\, Jesse Luehrs wrote:

On Tue\, Jul 03\, 2012 at 01:57:01PM +0200\, Martin Becker wrote:

Okay\, so fixing this doesn't appear to be as straightforward as it looks. The issue is that determining what to do with assignment operators when the left side isn't overloaded is tricky.

In 5.16\, "string" .= $overloaded worked fine because it would fall back to using the '.' overload on $overloaded\, even if fallback was set to 0. This was clearly wrong (.= falling back to . is a form of magic autogeneration)\, so it was fixed in f041cf0.

f041cf0 had a small error (it started checking for .= overloading on $overloaded\, which is also wrong)\, but fixing that was pretty easy. However\, the issue then becomes that "string" .= $overloaded has no way to work\, in the case where the '.' and '.=' overloads return normal strings (which isn't that uncommon) and $overloaded has fallback => 0 set.

What we're left with then is a situation where it's impossible to make "string" .= $overloaded work if $overloaded has fallback => 0 set (and similarly for any other assignment operator). I'm not sure what the right solution here is\, and any input would be appreciated. Also\, should we revert f041cf0 until we figure this out?

The way "overload" has been documented up to now\, assignment operators are only available for overloading under government of the left hand operand. This makes sense to me as I generally regard assignment variants of binary operators as shortcut notation but semantically equivalent to the longer notation with the separate equals sign. I take it the reason for providing hooks for assignment variants at all is that these allow implementations to take advantage of optimization opportunities where modifying objects in-place may be cheaper than working with separate intermediate results. There is no similar benefit to be gained for the object on the right hand side of an assignment.

I agree - assignment operators looking at the right operand is clearly wrong.

It escapes me how somebody might want to override ".=" while not at the same time overriding ".". I would consider this to be a bug in the overriding class. The overload pragma may deal with this situation as it does in other cases where some operations are overloaded and others are not\, which may for example lead to perl builtin "." handling ``$foo .= $overloaded''.

I'd argue whether the "fallback" directive must necessarily apply to unfolding assignment operations in the same way as it applies to replacing unary negation by subtraction\, or inequality operations by 3-way-comparisons\, etc. After all\, there are other operators with special fallback behaviour\, too\, like dereferencing operators. All I ask is that the behaviour should be well documented. Backwards compatibility should not be given up lightly either.

This would be an argument for returning to the 5.16 behavior. I do think that is likely one of the better options.

A solution in order to help an exotic object that does in fact need to distinguish between different ways of being concatenated could involve a new directive enabling swapped-parameter assigment\, or a new group of symbols eliglible for handling both cases rather than only the case of the overloading object being assigned to. I am not yet convinced that "mktables"\, a tool to create the runtime Perl Unicode files\, really needs that kind of objects. In fact\, it looks to me like it performed the fallback to "." explicitly in the implementation of ".=".

The mktables thing can easily be fixed\, that isn't really the point. The point is\, is it a reasonable expectation that overloading all operations\, even with fallback off\, should make the object usable in all relevant situations? I'm leaning toward yes\, which is why I'm leaning toward just reverting this change.

And anyway\, I should prefer to change this script rather than review any odd module using overload that could be hit like Math::BigInt from a change of the overload pragma.

Yes\, I don't think that Math::BigInt is doing anything wrong here. Any fix to this issue should allow it to work as it is currently written.

-doy

I haven't been following this very closely\, but fallback => 0 should tell Perl not to self-generate overloaded operations. mktables set that because its overloaded objects do not behave in the way that Perl's overload construction algorithm assumes\, and so Perl would silently generate the wrong thing. The commit was attempting to fix this\, and it appeared to.

p5pRT commented 12 years ago

From @doy

On Wed\, Jul 04\, 2012 at 05:41:15PM -0600\, Karl Williamson wrote:

On 07/04/2012 03:38 PM\, Jesse Luehrs wrote:

On Tue\, Jul 03\, 2012 at 01:57:01PM +0200\, Martin Becker wrote:

Okay\, so fixing this doesn't appear to be as straightforward as it looks. The issue is that determining what to do with assignment operators when the left side isn't overloaded is tricky.

In 5.16\, "string" .= $overloaded worked fine because it would fall back to using the '.' overload on $overloaded\, even if fallback was set to 0. This was clearly wrong (.= falling back to . is a form of magic autogeneration)\, so it was fixed in f041cf0.

f041cf0 had a small error (it started checking for .= overloading on $overloaded\, which is also wrong)\, but fixing that was pretty easy. However\, the issue then becomes that "string" .= $overloaded has no way to work\, in the case where the '.' and '.=' overloads return normal strings (which isn't that uncommon) and $overloaded has fallback => 0 set.

What we're left with then is a situation where it's impossible to make "string" .= $overloaded work if $overloaded has fallback => 0 set (and similarly for any other assignment operator). I'm not sure what the right solution here is\, and any input would be appreciated. Also\, should we revert f041cf0 until we figure this out?

The way "overload" has been documented up to now\, assignment operators are only available for overloading under government of the left hand operand. This makes sense to me as I generally regard assignment variants of binary operators as shortcut notation but semantically equivalent to the longer notation with the separate equals sign. I take it the reason for providing hooks for assignment variants at all is that these allow implementations to take advantage of optimization opportunities where modifying objects in-place may be cheaper than working with separate intermediate results. There is no similar benefit to be gained for the object on the right hand side of an assignment.

I agree - assignment operators looking at the right operand is clearly wrong.

It escapes me how somebody might want to override ".=" while not at the same time overriding ".". I would consider this to be a bug in the overriding class. The overload pragma may deal with this situation as it does in other cases where some operations are overloaded and others are not\, which may for example lead to perl builtin "." handling ``$foo .= $overloaded''.

I'd argue whether the "fallback" directive must necessarily apply to unfolding assignment operations in the same way as it applies to replacing unary negation by subtraction\, or inequality operations by 3-way-comparisons\, etc. After all\, there are other operators with special fallback behaviour\, too\, like dereferencing operators. All I ask is that the behaviour should be well documented. Backwards compatibility should not be given up lightly either.

This would be an argument for returning to the 5.16 behavior. I do think that is likely one of the better options.

A solution in order to help an exotic object that does in fact need to distinguish between different ways of being concatenated could involve a new directive enabling swapped-parameter assigment\, or a new group of symbols eliglible for handling both cases rather than only the case of the overloading object being assigned to. I am not yet convinced that "mktables"\, a tool to create the runtime Perl Unicode files\, really needs that kind of objects. In fact\, it looks to me like it performed the fallback to "." explicitly in the implementation of ".=".

The mktables thing can easily be fixed\, that isn't really the point. The point is\, is it a reasonable expectation that overloading all operations\, even with fallback off\, should make the object usable in all relevant situations? I'm leaning toward yes\, which is why I'm leaning toward just reverting this change.

And anyway\, I should prefer to change this script rather than review any odd module using overload that could be hit like Math::BigInt from a change of the overload pragma.

Yes\, I don't think that Math::BigInt is doing anything wrong here. Any fix to this issue should allow it to work as it is currently written.

-doy

I haven't been following this very closely\, but fallback => 0 should tell Perl not to self-generate overloaded operations. mktables set that because its overloaded objects do not behave in the way that Perl's overload construction algorithm assumes\, and so Perl would silently generate the wrong thing. The commit was attempting to fix this\, and it appeared to.

It may have fixed it\, but it also broke other things(:

The question is: in the case of "$normal .= $overloaded"\, where $overloaded overloads '.' and has fallback => 0\, should perl turn that into "$normal = $normal . $overloaded" so that $overloaded's '.' overload is run? Is that considered a "fallback"?

-doy

p5pRT commented 12 years ago

From @mhasch

My previous post does not seem to have made it into the ticket. I try it again with an unmodified subject line.

On Fri\, Jul 06\, 2012 at 06:20:44PM +0200\, Martin Becker wrote:

On Wed\, Jul 04\, 2012 at 07:12:24PM -0500\, Jesse Luehrs wrote:

The question is: in the case of "$normal .= $overloaded"\, where $overloaded overloads '.' and has fallback => 0\, should perl turn that into "$normal = $normal . $overloaded" so that $overloaded's '.' overload is run? Is that considered a "fallback"?

The fallback mode governs how operators not provided by the overloading package should be treated. One could argue that since "." is called here\, not for lack of an explicit ".=" method\, but for $overloaded being the right hand operand\, no missing method is involved and thus fallback is irrelevant.

My guess is that some users may be surprised to learn that an expression with ".=" reaches their "." method\, but that hardly anybody will be hurt.

Therefore\, I also lean towards keeping the 5.16.0 behaviour\, perhaps with slightly extended documentation. It already states that assignment variants of binary operators are only applicable to the object being assigned to. A little elaboration on this might be sufficient to keep surprises to a minimum.

The Item "Assignments" in overload's POD could be extended like this (slightly polishing my previous patch):

Inline Patch

```diff diff -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 + - * / % ** << >> x . & | ^ Martin ```

p5pRT commented 11 years ago

From @jkeenan

On Mon Jun 25 09:53:15 2012\, mhasch@cpan.org wrote:

This is a bug report for perl from mhasch@cpan.org\, generated with the help of perlbug 1.39 running under perl 5.17.1.

----------------------------------------------------------------- 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 1.998\, depends on a certain order of arguments of assignment operators called by overload\, while overload-1.19 does not behave as assumed.

Math::BigInt states this assumption in a comment in the source code (though unfortunately not in its test suite):

@@ dist/Math-BigInt/lib/Math/BigInt.pm\, lines 48..62 @@ 48 # some shortcuts for speed (assumes that reversed order of arguments is routed 49 # to normal '+' and we thus can always modify first arg. If this is changed\, 50 # this breaks and must be adjusted.) 51 '+=' => sub { $_[0]->badd($_[1]); }\, 52 '-=' => sub { $_[0]->bsub($_[1]); }\, 53 '*=' => sub { $_[0]->bmul($_[1]); }\, 54 '/=' => sub { scalar $_[0]->bdiv($_[1]); }\, 55 '%=' => sub { $_[0]->bmod($_[1]); }\, 56 '^=' => sub { $_[0]->bxor($_[1]); }\, 57 '&=' => sub { $_[0]->band($_[1]); }\, 58 '|=' => sub { $_[0]->bior($_[1]); }\, 59 60 '**=' => sub { $_[0]->bpow($_[1]); }\, 61 '\<\<=' => sub { $_[0]->blsft($_[1]); }\, 62 '>>=' => sub { $_[0]->brsft($_[1]); }\,

Although code like this might seem dangerous at first glance\, it is actually exploiting a documented feature of overload:

@@ lib/overload.pm\, section "Assignments"\, lines 425..435 @@ 425 An object that overloads an assignment operator does so only in 426 respect of assignments to that object. 427 In other words\, Perl never calls the corresponding methods with 428 the third argument (the "swap" argument) set to TRUE. 429 For example\, the operation 430 431 $a *= $b 432 433 cannot lead to C\<$b>'s implementation of C\<*=> being called\, 434 even if C\<$a> is a scalar. 435 (It can\, however\, generate a call to C\<$b>'s method for C\<*>).

This documentation seems now no longer accurate\, as can be seen with this example code:

use overload ( '*' => sub { "ok" }\, '*=' => sub { "not ok" }\, ); my $a = 1; my $b = bless []; print overload->VERSION\, "\t"\, ($a *= $b)\, "\n";

This prints 1.19 and "not ok" with perl-5.17.0 up to blead\, 1.18 and "ok" with perl-5.16.0.

To repair this\, either the documentation of "overload" should be changed and "Math::BigInt" should be adapted accordingly\, or "overload" should be brought back to par with its documentation.

In either case it may prove useful to add something along the lines of my first example to the test suite of Math::BigInt\, even if the main issue is with overload.

-Martin

I tested Math-BigInt v1.99 today against blead. The output is in the attachment. All tests passed\, but there were many instances of an overload-related warning.

There was extensive back-and-forth in this ticket and I don't think all issues have been resolved.

However\, if the module's 'make test' is passing\, albeit with warnings\, then I don't think this RT should still be classified as a blocker for Perl 5.18.

Thank you very much. Jim Keenan

p5pRT commented 11 years ago

From @jkeenan

PERL_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 t/00sig.t ........... skipped: Set the environment variable TEST_SIGNATURE to enable this test. t/01load.t .......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/01load.t .......... 1/2 # Testing Math::BigInt 1.997 # ==> Perl 5.017009\, /Users/jimk/perl5/perlbrew/perls/perl-blead/bin/perl5.17.9 t/01load.t .......... ok
t/02pod.t ........... skipped: Test::Pod 1.22 required for testing POD t/03podcov.t ........ skipped: Test::Pod::Coverage 1.08 required for testing POD coverage t/_e_math.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/_e_math.t ......... ok
t/bare_mbf.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/bare_mbf.t ........ ok
t/bare_mbi.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/bare_mbi.t ........ ok
t/bare_mif.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/bare_mif.t ........ ok
t/big_pi_e.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/big_pi_e.t ........ ok
t/bigfltpm.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/bigfltpm.t ........ ok
t/bigintc.t ......... ok
t/bigintpm.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/bigintpm.t ........ ok
t/bigints.t ......... ok
t/biglog.t .......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/biglog.t .......... ok
t/bigroot.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/bigroot.t ......... ok
t/calling.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/calling.t ......... ok
t/config.t .......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/config.t .......... ok
t/const_mbf.t ....... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/const_mbf.t ....... ok
t/constant.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/constant.t ........ ok
t/downgrade.t ....... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/downgrade.t ....... ok
t/inf_nan.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/inf_nan.t ......... ok
t/isa.t ............. overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/isa.t ............. ok
t/lib_load.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/lib_load.t ........ ok
t/mbf_ali.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/mbf_ali.t ......... ok
t/mbi_ali.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/mbi_ali.t ......... ok
t/mbi_rand.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/mbi_rand.t ........ ok
t/mbimbf.t .......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/mbimbf.t .......... ok
t/nan_cmp.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/nan_cmp.t ......... ok
t/new_overloaded.t .. overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/new_overloaded.t .. ok
t/req_mbf0.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/req_mbf0.t ........ ok
t/req_mbf1.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/req_mbf1.t ........ ok
t/req_mbfa.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/req_mbfa.t ........ ok
t/req_mbfi.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/req_mbfi.t ........ ok
t/req_mbfn.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/req_mbfn.t ........ ok
t/req_mbfw.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/req_mbfw.t ........ ok
t/require.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/require.t ......... ok
t/round.t ........... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/round.t ........... ok
t/rt-16221.t ........ ok
t/sub_ali.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/sub_ali.t ......... ok
t/sub_mbf.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/sub_mbf.t ......... ok
t/sub_mbi.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/sub_mbi.t ......... ok
t/sub_mif.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/sub_mif.t ......... ok
t/trap.t ............ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/trap.t ............ ok
t/upgrade.t ......... overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/upgrade.t ......... ok
t/upgrade2.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/upgrade2.t ........ ok
t/upgradef.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/upgradef.t ........ ok
t/use.t ............. overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/use.t ............. ok
t/use_lib1.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/use_lib1.t ........ ok
t/use_lib2.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/use_lib2.t ........ ok
t/use_lib3.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/use_lib3.t ........ ok
t/use_lib4.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/use_lib4.t ........ ok
t/use_mbfw.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/use_mbfw.t ........ ok
t/with_sub.t ........ overload arg '..' is invalid at /Users/jimk/.cpan/build/Math-BigInt-1.997-9JxLx4/blib/lib/Math/BigInt.pm line 153. t/with_sub.t ........ ok
All tests successful. Files=53\, Tests=27946\, 137 wallclock secs (18.45 usr 2.56 sys + 80.40 cusr 6.13 csys = 107.54 CPU) Result: PASS PJACKLAM/Math-BigInt-1.997.tar.gz /usr/bin/make test -- OK

p5pRT commented 11 years ago

From @jkeenan

On Mon Jun 25 09:53:15 2012\, mhasch@cpan.org wrote:

This is a bug report for perl from mhasch@cpan.org\, generated with the help of perlbug 1.39 running under perl 5.17.1.

----------------------------------------------------------------- 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 1.998\, depends on a certain order of arguments of assignment operators called by overload\, while overload-1.19 does not behave as assumed.

I would like to request some clarification about the version of Math::BigInt which is exhibiting these problems.

Today I spent a considerable amount of time trying to test the latest version of Math::BigInt against blead. I kept wondering why\, when I would say 'get Math::BigInt'\, it would only fetch version 1.997.

However\, I now realize that there is no version 1.998 available on CPAN. http://search.cpan.org/dist/Math-BigInt/ shows 1.997 as the most recent version. In other posts in this thread\, I have described the current state of Math::BigInt 1.997 when tested on blead. But I don't think we have any obligation to support a version not yet available on CPAN.

Can you clarify?

Thank you very much. Jim Keenan

p5pRT commented 11 years ago

From mhasch-p5p@cozap.com

On Mon Feb 18 10:59:26 2013\, jkeenan wrote:

On Mon Jun 25 09:53:15 2012\, mhasch@cpan.org wrote:

This is a bug report for perl from mhasch@cpan.org\, generated with the help of perlbug 1.39 running under perl 5.17.1.

----------------------------------------------------------------- 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 1.998\, depends on a certain order of arguments of assignment operators called by overload\, while overload-1.19 does not behave as assumed.

Math::BigInt states this assumption in a comment in the source code (though unfortunately not in its test suite):

@@ dist/Math-BigInt/lib/Math/BigInt.pm\, lines 48..62 @@ 48 # some shortcuts for speed (assumes that reversed order of arguments is routed 49 # to normal '+' and we thus can always modify first arg. If this is changed\, 50 # this breaks and must be adjusted.) 51 '+=' => sub { $_[0]->badd($_[1]); }\, 52 '-=' => sub { $_[0]->bsub($_[1]); }\, 53 '*=' => sub { $_[0]->bmul($_[1]); }\, 54 '/=' => sub { scalar $_[0]->bdiv($_[1]); }\, 55 '%=' => sub { $_[0]->bmod($_[1]); }\, 56 '^=' => sub { $_[0]->bxor($_[1]); }\, 57 '&=' => sub { $_[0]->band($_[1]); }\, 58 '|=' => sub { $_[0]->bior($_[1]); }\, 59 60 '**=' => sub { $_[0]->bpow($_[1]); }\, 61 '\<\<=' => sub { $_[0]->blsft($_[1]); }\, 62 '>>=' => sub { $_[0]->brsft($_[1]); }\,

Although code like this might seem dangerous at first glance\, it is actually exploiting a documented feature of overload:

@@ lib/overload.pm\, section "Assignments"\, lines 425..435 @@ 425 An object that overloads an assignment operator does so only in 426 respect of assignments to that object. 427 In other words\, Perl never calls the corresponding methods with 428 the third argument (the "swap" argument) set to TRUE. 429 For example\, the operation 430 431 $a *= $b 432 433 cannot lead to C\<$b>'s implementation of C\<*=> being called\, 434 even if C\<$a> is a scalar. 435 (It can\, however\, generate a call to C\<$b>'s method for C\<*>).

This documentation seems now no longer accurate\, as can be seen with this example code:

use overload ( '*' => sub { "ok" }\, '*=' => sub { "not ok" }\, ); my $a = 1; my $b = bless []; print overload->VERSION\, "\t"\, ($a *= $b)\, "\n";

This prints 1.19 and "not ok" with perl-5.17.0 up to blead\, 1.18 and "ok" with perl-5.16.0.

To repair this\, either the documentation of "overload" should be changed and "Math::BigInt" should be adapted accordingly\, or "overload" should be brought back to par with its documentation.

In either case it may prove useful to add something along the lines of my first example to the test suite of Math::BigInt\, even if the main issue is with overload.

-Martin

I tested Math-BigInt v1.99 today against blead. The output is in the attachment. All tests passed\, but there were many instances of an overload-related warning.

There was extensive back-and-forth in this ticket and I don't think all issues have been resolved.

However\, if the module's 'make test' is passing\, albeit with warnings\, then I don't think this RT should still be classified as a blocker for Perl 5.18.

Note that the issue is not caught by the test suite of Math-BigInt-1.997. You should additionally run one of the test cases mentioned above.

Blead should have Math-BigInt-1.999 in its "dist" subdirectory. You might also want to include that version in your evaluation.

-Martin

p5pRT commented 11 years ago

From @jkeenan

On Wed Feb 20 15:55:52 2013\, mhasch wrote:

On Mon Feb 18 10:59:26 2013\, jkeenan wrote:

However\, if the module's 'make test' is passing\, albeit with warnings\, then I don't think this RT should still be classified as a blocker for Perl 5.18.

Note that the issue is not caught by the test suite of Math-BigInt-1.997. You should additionally run one of the test cases mentioned above.

Blead should have Math-BigInt-1.999 in its "dist" subdirectory. You might also want to include that version in your evaluation.

-Martin

Please review the attached patch as a first attempt at writing regression tests for this problem.

Thank you very much. Jim Keenan

p5pRT commented 11 years ago

From @jkeenan

From 7bd5a610eb0bfcb053647c2f005739a923007c32 Mon Sep 17 00:00:00 2001 From: James E Keenan \jkeenan@cpan\.org Date: Wed\, 20 Feb 2013 21:53:43 -0500 Subject: [PATCH] Add regression tests for overloaded assignment operators.

Some will fail with overload v1.19. For: RT#113834

dist/Math-BigInt/t/rt-113834.t | 41 ++++++++++++++++++++++++++++++++++++++++ 1 files changed\, 41 insertions(+)\, 0 deletions(-) create mode 100644 dist/Math-BigInt/t/rt-113834.t

Inline Patch

```diff diff --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 ```

p5pRT commented 11 years ago

From mhasch-p5p@cozap.com

On Wed Feb 20 18:57:27 2013\, jkeenan wrote:

On Wed Feb 20 15:55:52 2013\, mhasch wrote:

On Mon Feb 18 10:59:26 2013\, jkeenan wrote:

However\, if the module's 'make test' is passing\, albeit with warnings\, then I don't think this RT should still be classified as a blocker for Perl 5.18.

Note that the issue is not caught by the test suite of Math-BigInt-1.997. You should additionally run one of the test cases mentioned above.

Blead should have Math-BigInt-1.999 in its "dist" subdirectory. You might also want to include that version in your evaluation.

Please review the attached patch as a first attempt at writing regression tests for this problem.

Thank you for your suggestion. It is a good idea to make Math-BigInt check assigment operator semantics it promises to fulfill. The overload pragma semantics it relies upon in turn will naturally be exposed that way.

I have put together a more complete test based on your suggestion. Note that the issue is not just about some expression results but perhaps even more importantly about which variables will or won't be updated in an assignment.

With perl-5.17.9\, 22 of 33 checks in this test fail. Expression result checks of commutative operators never fail but have been included for completeness.

Note that Math-BigInt does not override all assigment operators there are. Another regression test directly checking overload.c/overload.pm and covering boolean and string modifiers as well would do no harm.

-Martin

p5pRT commented 11 years ago

From mhasch-p5p@cozap.com

perl-5.17.9-BigInt-MHASCH-01.patch

```diff diff -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'); + +} ```

p5pRT commented 11 years ago

From @rjbs

I have pushed 90732c3 to smoke-me/rjbs/revert-ol-change\, reverting f041cf0f

I would like to get a test for all the relevant bugs: the one fixed by f041cf0f and the one introduced by it\, so we can have a new ticket to get both in a good state at once. In the meantime\, I'd rather keep the old bug than swap it out for a new one.

-- rjbs

p5pRT commented 11 years ago

From @rjbs

I could not find the ticket in response to which f041cf0f9c64 was written. I've updated the subject of this ticket.

What we want is to fix the fallback of += without making it use the rhs's overloading. Rather than fix one bug and introduce a new one\, the reversion leaves us with the same bug count — but the bug we knew about. This ticket no longer blocks 5.18.0

-- rjbs

p5pRT commented 11 years ago

From mhasch-p5p@cozap.com

This issue still needs clarification. What was actually wrong with the behaviour of overload-1.18?

Quoting the commit message of f041cf0f9c6469c41de8b73d5f7b426710c3ff8b:

At the very beginning of Perl_amagic_call\, if the flag AMGf_noleft is not passed\, we first try to look up the overload method corresponding to the assignment operator\, then the normal one if fallback is authorized. However\, if this fails\, when trying later to find overload magic with the arguments swapped (if AMGf_noright is not passed)\, this procedure was not used and we looked up directly the base operation from which the assignment operator might be derived. As a consequence of what an operator like += might have looked autogenerated even when fallback=>0 was passed.

Is it wrong to directly look at the base operation\, once the rhs operand may handle the overloading? The rhs operand must not be modified and thus can not act as as the object being assigned to. Intentionally\, assignment operator methods have been defined to only overload assignments to an object\, not assignments from an object.

Therefore the base operation may come into play not for lack of an optional assignment operator method but for simple semantic reasons. Fallback\, on the other hand\, has to do with the treatment of missing methods. It tells whether missing methods may be replaced by something else and\, if so\, by what. So far\, it only covers operators that can be overloaded separately.

A more radical interpretation of the "fallback=>0" setting\, preventing "foo += bar" being handled by bar's "+" operator\, say\, would only make sense with a larger interface that had an own method for the "+= bar" case. Otherwise\, "fallback=>0" would not prevent falling back but would prevent evaluating the expression at all. There would be nothing to not fall back from. A class wanting to make "+= bar" legal could not opt out of fallbacks in general.

In short\, what f041cf0f9c6469c41 set out to change does not look like a bug to me. It may be different from what some people expect. They do have a point in that the documentation might be more explicit about what exactly constitutes a fallback.

Another point that remains unsolved if this change is rejected is the use case that actually wanted a way to override assignment operators in the class of the rhs operand.

The overload interface could be extended to allow that. Of course\, it would need different symbols from those that are already in use. Maybe mirror images of the ordinary assignment operators such as "=+"\, "=-" etc. would do\, as a mnemonic for the fact that these operators are looked at "from the other side". Classes not wanting to catch or define these should not be forced to allow fallbacks\, however\, or we would create a portability problem. There is also the risk of typos going unnoticed.

-Martin

p5pRT commented 11 years ago

From @doy

On Wed\, Mar 06\, 2013 at 10:06:58AM -0800\, Martin Hasch via RT wrote:

This issue still needs clarification. What was actually wrong with the behaviour of overload-1.18?

Quoting the commit message of f041cf0f9c6469c41de8b73d5f7b426710c3ff8b:

At the very beginning of Perl_amagic_call\, if the flag AMGf_noleft is not passed\, we first try to look up the overload method corresponding to the assignment operator\, then the normal one if fallback is authorized. However\, if this fails\, when trying later to find overload magic with the arguments swapped (if AMGf_noright is not passed)\, this procedure was not used and we looked up directly the base operation from which the assignment operator might be derived. As a consequence of what an operator like += might have looked autogenerated even when fallback=>0 was passed.

A more radical interpretation of the "fallback=>0" setting\, preventing "foo += bar" being handled by bar's "+" operator\, say\, would only make sense with a larger interface that had an own method for the "+= bar" case. Otherwise\, "fallback=>0" would not prevent falling back but would prevent evaluating the expression at all. There would be nothing to not fall back from. A class wanting to make "+= bar" legal could not opt out of fallbacks in general.

Yes\, this is essentially what I was trying to get across. Without this fallback\, there is no way to use fallback => 0 and still get an object that can be used in all of the same contexts as the thing (string or whatever) that it's trying to emulate. If we think that fallback => 0 should prevent this\, we really need to add an additional overload option for handling this behavior as well.

-doy

Corion commented 4 years ago

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 overload.pm:

+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.