Handrolling overloading is done incorrectly.

[Abigail]

Version 1.302175 to Test::Builder contains a serious error when it's trying to mimic Perls overloading.

In particular, the last line (line 728) of the method unoverload. The last two lines are:

    my $string_meth = overload::Method( $$thing, $type ) || return;
    $$thing = $$thing->$string_meth();

Here, it is trying to call the method which is called when overloading is present. (I can't figure out why it's handrolling overloading instead of have perl do it, but that's beside the point). The problem is, it's calling the method without arguments. Yet, according the overload documentation, overload methods are called with multiple arguments, and they aren't optional:

Three arguments are passed to all subroutines specified in the "use
overload" directive (with exceptions - see below, particularly
"nomethod").

The first of these is the operand providing the overloaded operator
implementation - in this case, the object whose "minus()" method is
being called.

The second argument is the other operand, or "undef" in the case of a
unary operator.

The third argument is set to TRUE if (and only if) the two operands have
been swapped. Perl may do this to ensure that the first argument ($self)
is an object implementing the overloaded operation, in line with general
object calling conventions.

And for some overload methods, there are even a fourth and fifth argument.

Not passing in mandatory arguments is a problem for code which uses subroutine signatures.

Here is a short program which exhibits the problem:

#!/usr/bin/perl

use 5.028;

use strict;
use warnings;

use experimental 'lexical_subs';

use Test::More;

package Test {
    use overload '""'     => \&stringify,
                 fallback => 1;
    sub stringify ($self, $, $) {
        $$self
    }
}

my $x = bless \do {my $var = 3} => "Test";
say       $x;                     # Fine
is        $x, 3, "Test 1";        # Also fine
is_deeply $x, 3, "Test 2";        # Fails

done_testing;

__END__

Running this gives:

3
ok 1 - Test 1
Too few arguments for subroutine 'Test::stringify' at /usr/lib/pakket/5.28.1/libraries/active/lib/perl5/Test/Builder.pm line 728.

In the call to say, and the call to is, perl calls the overload method, and calls it with three arguments. In the call to is_deeply, it is Test::Builder which calls it, and calls with missing arguments.

Note that using a signature of ($self) isn't going to work, as that will trigger a Too many arguments... error when it's perl who does the overloading.

[haarg]

This is quite old behavior, but it's not obvious why it exists.

Unoverload for a string should be as simple as "$obj". For a number, sprintf "%f", $obj. It's possible for that to lose some precision, but that's the case with the current behavior too.

[exodist]

This behavior Predates me by a long time. I am not sure why it is necessary, and I am not sure anyone active today has the answer. That said, even though the current behavior is demonstrably wrong, it may not be fixable. Fixing this could potentially break a lot of code on cpan and/or darkpan. The solution may be to document this bug and suggest ways to avoid it, such as using Test2::Tools::Compare which intentionally avoided this behavior, or Test::Deep, assuming it has no such issue?

I think the first thing we need to do is figure out when and why this behavior was added. Then we can decide how it might be fixed, then we need to judge the potential impact of such a "fix". At this point changes to Test::More, even bugfixes, only happen if doing so will not break things (or results in less breakage than doing nothing). The real "fix" is to upgrade to newer tools Like Test2 which do not have the same burden for "fixing" things.

[exodist]

Looks like the unoverloading method was added d14da20 so that todo reasons that were overloaded objects would work.

A few other commits then expand its usage such as 8b2f0c6

So initially it was added to force $TODO to be a string, then it was used as a general purpose tool after that. I am not sure why the second commit was necessary as eval "\$a $op \$b" should let the operator determine what $a and $b are and overloading should just happen.

That said, I am pretty sure existing tests will break if we alter this behavior.