develooper Front page | perl.perl5.porters | Postings from July 2012

Re: [perl #113834] overload 1.19 breaks Math::BigInt 1.998

Thread Previous | Thread Next
From:
Martin Becker
Date:
July 4, 2012 06:47
Subject:
Re: [perl #113834] overload 1.19 breaks Math::BigInt 1.998
Message ID:
20120703115701.GB17266@corcomroe.in-ulm.de
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

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About